David,
any progress on this patch series?

I never saw an updated version of this patch series after the last round of
reviews, but it would be great to move it forward.  I have filefrag patches
in my e2fsprogs tree waiting for an updated version of your patch.

I recall the main changes were:
- add FIEMAP_EXTENT_PHYS_LENGTH flag to indicate if fe_phys_length was valid
- rename fe_length to fe_logi_length and #define fe_length fe_logi_length
- always fill in fe_phys_length (= fe_logi_length for uncompressed files)
  and set FIEMAP_EXTENT_PHYS_LENGTH whether the extent is compressed or not
- add WARN_ONCE() in fiemap_fill_next_extent() as described below

I don't know if there was any clear statement about whether there should be
separate FIEMAP_EXTENT_PHYS_LENGTH and FIEMAP_EXTENT_DATA_COMPRESSED flags,
or if the latter should be implicit?  Probably makes sense to have separate
flags.  It should be fine to use:

#define FIEMAP_EXTENT_PHYS_LENGTH	0x00000010

since this flag was never used.

Cheers, Andreas

On Dec 12, 2013, at 5:02 PM, Andreas Dilger [off-list ref] wrote:

On Dec 12, 2013, at 4:24 PM, Dave Chinner [off-list ref] wrote:

quoted

On Thu, Dec 12, 2013 at 04:25:59PM +0100, David Sterba wrote:

quoted

This flag was not accepted when fiemap was proposed [2] due to lack of
in-kernel users. Btrfs has compression for a long time and we'd like to
see that an extent is compressed in the output of 'filefrag' utility
once it's taught about it.

For that purpose, a reserved field from fiemap_extent is used to let the
filesystem store along the physcial extent length when the flag is set.
This keeps compatibility with applications that use FIEMAP.

I'd prefer to just see the new physical length field always filled
out, regardless of whether it is a compressed extent or not. In
terms of backwards compatibility to userspace, it makes no
difference because the value of reserved/unused fields is undefined
by the API. Yes, the implementation zeros them, but there's nothing
in the documentation that says "reserved fields must be zero".
Hence I think we should just set it for every extent.

I'd actually thought the same thing while reading the patch, but I figured
people would object because it implies that old kernels will return a
physical length of 0 bytes (which might be valid) and badly-written tools
will not work correctly on older kernels.  That said, applications _should_
be checking the FIEMAP_EXTENT_DATA_COMPRESSED flag, and I suspect in the
future fewer developers will be confused if fe_phys_length == fe_length
going forward.

If the initial tools get it right (in particular filefrag), then hopefully
others will get it correct also.

quoted

From the point of view of the kernel API (fiemap_fill_next_extent),
passing the physical extent size in the "len" parameter for normal
extents, then passing 0 for the "physical length" makes absolutely
no sense.

IOWs, what you have created is a distinction between the extent's
"logical length" and it's "physical length". For uncompressed
extents, they are both equal and they should both be passed to
fiemap_fill_next_extent as the same value. Extents where they are
different (i.e.  encoded extents) is when they can be different.
Perhaps fiemap_fill_next_extent() should check and warn about
mismatches when they differ and the relevant flags are not set...

Seems reasonable to have a WARN_ONCE() in that case.  That would catch bugs
in the filesystem, code as well:

	WARN_ONCE(phys_len != lgcl_len &&
		  !(flags & FIEMAP_EXTENT_DATA_COMPRESSED),
		  "physical len %llu != logical length %llu without DATA_COMPRESSED\n",
		  phys_len, logical_len, phys_len, logical_len);

quoted

quoted

diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h
index 93abfcd..0e32cae 100644
--- a/include/uapi/linux/fiemap.h
+++ b/include/uapi/linux/fiemap.h

@@ -19,7 +19,9 @@ struct fiemap_extent {

	__u64 fe_physical; /* physical offset in bytes for the start
			    * of the extent from the beginning of the disk */
	__u64 fe_length;   /* length in bytes for this extent */
-	__u64 fe_reserved64[2];
+	__u64 fe_phys_length; /* physical length in bytes, undefined if
+			       * DATA_COMPRESSED not set */
+	__u64 fe_reserved64;
	__u32 fe_flags;    /* FIEMAP_EXTENT_* flags for this extent */
	__u32 fe_reserved[3];
};

The comment for fe_length needs to change, too, because it needs to
indicate that it is the logical extent length and that it may be
different to the fe_phys_length depending on the flags that are set
on the extent.

Would it make sense to rename fe_length to fe_logi_length (or something,
I'm open to suggestions), and have a compat macro:

#define fe_length fe_logi_length

around for older applications?  That way, new developers would start to
use the new name, old applications would still compile for both newer and
older interfaces, and it doesn't affect the ABI at all.

quoted

And, FWIW, I wouldn't mention specific flags in the comment here,
but do it at the definition of the flags that indicate there is
a difference between physical and logical extent lengths....

Actually, I was thinking just the opposite for this field.  It seems useful
that the requirement for DATA_COMPRESSED being set is beside fe_phys_length
so that anyone using this field sees the correlation clearly.  I don't expect
everyone would read and understand the meaning of all the flags when looking
at the data structure.

Cheers, Andreas

quoted

quoted

@@ -50,6 +52,8 @@ struct fiemap {

						    * Sets EXTENT_UNKNOWN. */
#define FIEMAP_EXTENT_ENCODED		0x00000008 /* Data can not be read
						    * while fs is unmounted */
+#define FIEMAP_EXTENT_DATA_COMPRESSED	0x00000040 /* Data is compressed by fs.
+						    * Sets EXTENT_ENCODED */

i.e. here.

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org

Cheers, Andreas

Cheers, Andreas