[Top] [All Lists]

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

To: David Sterba <dsterba@xxxxxxx>
Subject: Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag
From: Andreas Dilger <adilger@xxxxxxxxx>
Date: Thu, 17 Jul 2014 00:07:57 -0600
Cc: linux-fsdevel <linux-fsdevel@xxxxxxxxxxxxxxx>, linux-nilfs@xxxxxxxxxxxxxxx, xfs@xxxxxxxxxxx, Btrfs Developer List <linux-btrfs@xxxxxxxxxxxxxxx>, Ext4 Developers List <linux-ext4@xxxxxxxxxxxxxxx>, ocfs2-devel@xxxxxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <9520AB36-B728-423A-8EA1-FDD22B79AE90@xxxxxxxxx>
References: <cover.1386778302.git.dsterba@xxxxxxx> <4f8d5dc5b51a43efaf16c39398c23a6276e40a30.1386778303.git.dsterba@xxxxxxx> <20131212232443.GL31386@dastard> <9520AB36-B728-423A-8EA1-FDD22B79AE90@xxxxxxxxx>
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
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 <adilger@xxxxxxxxx> 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.  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 
>                 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@xxxxxxxxxxxxx
> Cheers, Andreas

Cheers, Andreas

Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail

<Prev in Thread] Current Thread [Next in Thread>