Message-Id: <9520AB36-B728-423A-8EA1-FDD22B79AE90@dilger.ca>
Date:	Thu, 12 Dec 2013 17:02:57 -0700
From:	Andreas Dilger <adilger@...ger.ca>
To:	Dave Chinner <david@...morbit.com>
Cc:	David Sterba <dsterba@...e.cz>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	linux-nilfs@...r.kernel.org, Mark Fasheh <mfasheh@...e.com>,
	xfs@....sgi.com, Christoph Hellwig <hch@...radead.org>,
	"linux-btrfs@...r.kernel.org Btrfs" <linux-btrfs@...r.kernel.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Ext4 Developers List <linux-ext4@...r.kernel.org>,
	ocfs2-devel@....oracle.com
Subject: Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag

On Dec 12, 2013, at 4:24 PM, Dave Chinner <david@...morbit.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
> david@...morbit.com


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists