Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag

[Dave Chinner <david@fromorbit.com>]

On Thu, Dec 12, 2013 at 05:02:57PM -0700, Andreas Dilger wrote:
> On Dec 12, 2013, at 4:24 PM, Dave Chinner <david@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. 

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@fromorbit.com