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 <adil...@dilger.ca> wrote: > On Dec 12, 2013, at 4:24 PM, Dave Chinner <da...@fromorbit.com> wrote: >> On Thu, Dec 12, 2013 at 04:25:59PM +0100, David Sterba wrote: >>> 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. > >> 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); > >>> 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. > >> 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 > >>> @@ -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 >> da...@fromorbit.com > > > Cheers, Andreas > > > > > Cheers, Andreas
signature.asc
Description: Message signed with OpenPGP using GPGMail