On Thu, Dec 12, 2013 at 05:02:57PM -0700, Andreas Dilger wrote: > On Dec 12, 2013, at 4:24 PM, Dave Chinner <david@xxxxxxxxxxxxx> 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. Well, that's a problem regardless of whether new kernels return a physical length by default or not. I think I'd prefer a flag that says specifically whether the fe_phys_len field is valid or not. Old kernels will never set the flag, new kernels can always set the flag... > 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. I think an explicit flag is better than relying on a flag that defines the encoding to imply the physical length field is valid. > If the initial tools get it right (in particular filefrag), I'd think xfs_io is the first target - because we'll need xfstests coverage of this before there's a filefrag release that supports it... > then hopefully others will get it correct also. Agreed. > > 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); Yup, pretty much what I was thinking. > >> --- 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. Sounds like a good idea. > > 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. Well, it's moot if we decide a specific flag for the fe_phys_len field being valid is decided on ;) Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html