| Message ID | 4f8d5dc5b51a43efaf16c39398c23a6276e40a30.1386778303.git.dsterba@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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... > 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. 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.... > @@ -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.
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. 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@fromorbit.com Cheers, Andreas
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.
On Fri, Dec 13, 2013 at 10:24:43AM +1100, Dave Chinner wrote: > 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. > > >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. I tend to agree, but the additional complication here is that this is a change to an existing API. We'd need another HAVE_PHYS_LEN flag for non-compressed extents so that userspace can rely on it. Given that it's only useful for that case I think the userspace API introduced is the best we can get. I think however that we should always pass the phys_len argument in the kernel API just to make it less confusing. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks all for feedback, the changes sound ok to me. I'll send v4. david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <adilger@dilger.ca> 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. 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@fromorbit.com > > > Cheers, Andreas > > > > > Cheers, Andreas
On Thu, Jul 17, 2014 at 12:07:57AM -0600, Andreas Dilger wrote: > any progress on this patch series? I'm sorry I got distracted at the end of year and did not finish the 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 fe_phys_length will be always valid, so other the flags are set only if it's not equal to the logical length. > - 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 This is my understanding and contradicts the first point. > - 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. I've kept only FIEMAP_EXTENT_DATA_COMPRESSED, I don't see a need for FIEMAP_EXTENT_PHYS_LENGTH and this would be yet another flag because the FIEMAP_EXTENT_DATA_ENCODED is also implied. I'll send V4, we can discuss the PHYS_LENGTH flag then. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jul 24, 2014, at 1:22 PM, David Sterba <dsterba@suse.cz> wrote: > On Thu, Jul 17, 2014 at 12:07:57AM -0600, Andreas Dilger wrote: >> any progress on this patch series? > > I'm sorry I got distracted at the end of year and did not finish the > 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 > > fe_phys_length will be always valid, so other the flags are set only if it's > not equal to the logical length. > >> - 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 > > This is my understanding and contradicts the first point. I think Dave Chinner's former point was that having fe_phys_length validity depend on FIEMAP_EXTENT_DATA_COMPRESSED is a non-intuitive interface. It is not true that fe_phys_length would always be valid, since that is not the case for older kernels that currently always set this field to 0, so they need some flag to indicate if fe_phys_length is valid. Alternately, userspace could do: if (ext->fe_phys_length == 0) ext->fe_phys_length = ext->fe_logi_length; but that pre-supposes that fe_phys_length == 0 is never a valid value when fe_logi_length is non-zero, and this might introduce errors in some cases. I could imagine that some compression methods might not allocate any space at all if it was all zeroes, and just store a bit in the blockpointer or extent, so having a separate FIEMAP_EXTENT_PHYS_LENGTH is probably safer in the long run. That opens up the question of whether a written zero filled space that gets compressed away is different from a hole, but I'd prefer to just return whatever the file mapping is than interpret it. Cheers, Andreas >> - 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. > > I've kept only FIEMAP_EXTENT_DATA_COMPRESSED, I don't see a need for > FIEMAP_EXTENT_PHYS_LENGTH and this would be yet another flag because the > FIEMAP_EXTENT_DATA_ENCODED is also implied. > > I'll send V4, we can discuss the PHYS_LENGTH flag then. Cheers, Andreas
On Fri, Jul 25, 2014 at 4:04 AM, Andreas Dilger <adilger@dilger.ca> wrote: > > On Jul 24, 2014, at 1:22 PM, David Sterba <dsterba@suse.cz> wrote: >> On Thu, Jul 17, 2014 at 12:07:57AM -0600, Andreas Dilger wrote: >>> any progress on this patch series? >> >> I'm sorry I got distracted at the end of year and did not finish the >> 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 >> >> fe_phys_length will be always valid, so other the flags are set only if it's >> not equal to the logical length. >> >>> - 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 >> >> This is my understanding and contradicts the first point. > > I think Dave Chinner's former point was that having fe_phys_length validity > depend on FIEMAP_EXTENT_DATA_COMPRESSED is a non-intuitive interface. It is > not true that fe_phys_length would always be valid, since that is not the > case for older kernels that currently always set this field to 0, so they > need some flag to indicate if fe_phys_length is valid. Alternately, > userspace could do: > > if (ext->fe_phys_length == 0) > ext->fe_phys_length = ext->fe_logi_length; > > but that pre-supposes that fe_phys_length == 0 is never a valid value when > fe_logi_length is non-zero, and this might introduce errors in some cases. > I could imagine that some compression methods might not allocate any space > at all if it was all zeroes, and just store a bit in the blockpointer or > extent, so having a separate FIEMAP_EXTENT_PHYS_LENGTH is probably safer > in the long run. zfs is an example of this. > That opens up the question of whether a written zero > filled space that gets compressed away is different from a hole, but I'd > prefer to just return whatever the file mapping is than interpret it. > > Cheers, Andreas > >>> - 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. >> >> I've kept only FIEMAP_EXTENT_DATA_COMPRESSED, I don't see a need for >> FIEMAP_EXTENT_PHYS_LENGTH and this would be yet another flag because the >> FIEMAP_EXTENT_DATA_ENCODED is also implied. >> >> I'll send V4, we can discuss the PHYS_LENGTH flag then. > > > Cheers, Andreas > > > > > Regards, Rohan -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the feedback. On Thu, Jul 24, 2014 at 04:34:35PM -0600, Andreas Dilger wrote: > >> - 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 > > > > This is my understanding and contradicts the first point. > > I think Dave Chinner's former point was that having fe_phys_length validity > depend on FIEMAP_EXTENT_DATA_COMPRESSED is a non-intuitive interface. It is > not true that fe_phys_length would always be valid, since that is not the > case for older kernels that currently always set this field to 0, so they > need some flag to indicate if fe_phys_length is valid. I see the backward compatibility issue now. The patches (up to v4) updated all filesystems to fill the physical length with logical, but this should be 0 to keep the backward compatibility and also to keep the changes to existing fiemap users minimal. So for v5, PHYS_LENGTH will be introduced but only used by btrfs together with ENCODED and COMPRESSED. Though this may look too much for my needs to represent compressed extent, it is flexible for future use. > Alternately, > userspace could do: > > if (ext->fe_phys_length == 0) > ext->fe_phys_length = ext->fe_logi_length; > > but that pre-supposes that fe_phys_length == 0 is never a valid value when > fe_logi_length is non-zero, and this might introduce errors in some cases. > I could imagine that some compression methods might not allocate any space > at all if it was all zeroes, and just store a bit in the blockpointer or > extent, so having a separate FIEMAP_EXTENT_PHYS_LENGTH is probably safer > in the long run. Sounds good to me. > That opens up the question of whether a written zero > filled space that gets compressed away is different from a hole, but I'd > prefer to just return whatever the file mapping is than interpret it. It could save some bytes, but I don' see it too practical. I expect the amount of zeroed blocks to be low on average and the current compression methods are able to squeeze long runs of zeros to a few tens of bytes per page. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index ff43802..5ea0ef5 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -4244,7 +4244,7 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, end = 1; } ret = fiemap_fill_next_extent(fieinfo, em_start, disko, - em_len, flags); + em_len, 0, flags); if (ret) goto out_free; } diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 35f65cf..00ffd18 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -2224,6 +2224,7 @@ static int ext4_fill_fiemap_extents(struct inode *inode, (__u64)es.es_lblk << blksize_bits, (__u64)es.es_pblk << blksize_bits, (__u64)es.es_len << blksize_bits, + 0, flags); if (err < 0) break; @@ -4798,7 +4799,7 @@ static int ext4_xattr_fiemap(struct inode *inode, if (physical) error = fiemap_fill_next_extent(fieinfo, 0, physical, - length, flags); + length, 0, flags); return (error < 0 ? error : 0); } diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c index bae9875..c5da773 100644 --- a/fs/ext4/inline.c +++ b/fs/ext4/inline.c @@ -1816,7 +1816,7 @@ int ext4_inline_data_fiemap(struct inode *inode, if (physical) error = fiemap_fill_next_extent(fieinfo, 0, physical, - length, flags); + length, 0, flags); brelse(iloc.bh); out: up_read(&EXT4_I(inode)->xattr_sem); diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index 7119504..86e9e9b 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -1817,7 +1817,7 @@ static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, len = size - start; if (start < size) ret = fiemap_fill_next_extent(fieinfo, start, phys, - len, flags); + len, 0, flags); if (ret == 1) ret = 0; } else { diff --git a/fs/ioctl.c b/fs/ioctl.c index 8ac3fad..e7902c4 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -70,6 +70,7 @@ static int ioctl_fibmap(struct file *filp, int __user *p) * @logical: Extent logical start offset, in bytes * @phys: Extent physical start offset, in bytes * @len: Extent length, in bytes + * @phys_len: Physical extent length in bytes * @flags: FIEMAP_EXTENT flags that describe this extent * * Called from file system ->fiemap callback. Will populate extent @@ -80,10 +81,11 @@ static int ioctl_fibmap(struct file *filp, int __user *p) * extent that will fit in user array. */ #define SET_UNKNOWN_FLAGS (FIEMAP_EXTENT_DELALLOC) -#define SET_NO_UNMOUNTED_IO_FLAGS (FIEMAP_EXTENT_DATA_ENCRYPTED) +#define SET_NO_UNMOUNTED_IO_FLAGS (FIEMAP_EXTENT_DATA_ENCRYPTED | \ + FIEMAP_EXTENT_DATA_COMPRESSED) #define SET_NOT_ALIGNED_FLAGS (FIEMAP_EXTENT_DATA_TAIL|FIEMAP_EXTENT_DATA_INLINE) int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical, - u64 phys, u64 len, u32 flags) + u64 phys, u64 len, u64 phys_len, u32 flags) { struct fiemap_extent extent; struct fiemap_extent __user *dest = fieinfo->fi_extents_start; @@ -110,6 +112,9 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical, extent.fe_length = len; extent.fe_flags = flags; + if (flags & FIEMAP_EXTENT_DATA_COMPRESSED) + extent.fe_phys_length = phys_len; + dest += fieinfo->fi_extents_mapped; if (copy_to_user(dest, &extent, sizeof(extent))) return -EFAULT; @@ -318,10 +323,11 @@ int __generic_block_fiemap(struct inode *inode, flags = FIEMAP_EXTENT_MERGED|FIEMAP_EXTENT_LAST; ret = fiemap_fill_next_extent(fieinfo, logical, phys, size, - flags); + 0, flags); } else if (size) { ret = fiemap_fill_next_extent(fieinfo, logical, - phys, size, flags); + phys, size, + 0, flags); size = 0; } @@ -347,7 +353,7 @@ int __generic_block_fiemap(struct inode *inode, if (start_blk > last_blk && !whole_file) { ret = fiemap_fill_next_extent(fieinfo, logical, phys, size, - flags); + 0, flags); break; } @@ -358,7 +364,7 @@ int __generic_block_fiemap(struct inode *inode, if (size) { ret = fiemap_fill_next_extent(fieinfo, logical, phys, size, - flags); + 0, flags); if (ret) break; } diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c index 7e350c5..b03917a 100644 --- a/fs/nilfs2/inode.c +++ b/fs/nilfs2/inode.c @@ -1018,7 +1018,8 @@ int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, if (size) { /* End of the current extent */ ret = fiemap_fill_next_extent( - fieinfo, logical, phys, size, flags); + fieinfo, logical, phys, size, 0, + flags); if (ret) break; } @@ -1068,7 +1069,8 @@ int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, flags |= FIEMAP_EXTENT_LAST; ret = fiemap_fill_next_extent( - fieinfo, logical, phys, size, flags); + fieinfo, logical, phys, size, 0, + flags); if (ret) break; size = 0; @@ -1084,7 +1086,7 @@ int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, /* Terminate the current extent */ ret = fiemap_fill_next_extent( fieinfo, logical, phys, size, - flags); + 0, flags); if (ret || blkoff > end_blkoff) break; diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c index 767370b..521b0f2 100644 --- a/fs/ocfs2/extent_map.c +++ b/fs/ocfs2/extent_map.c @@ -735,7 +735,7 @@ static int ocfs2_fiemap_inline(struct inode *inode, struct buffer_head *di_bh, phys += offsetof(struct ocfs2_dinode, id2.i_data.id_data); - ret = fiemap_fill_next_extent(fieinfo, 0, phys, id_count, + ret = fiemap_fill_next_extent(fieinfo, 0, phys, id_count, 0, flags); if (ret < 0) return ret; @@ -809,7 +809,7 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, virt_bytes = (u64)le32_to_cpu(rec.e_cpos) << osb->s_clustersize_bits; ret = fiemap_fill_next_extent(fieinfo, virt_bytes, phys_bytes, - len_bytes, fe_flags); + len_bytes, 0, fe_flags); if (ret) break; diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index 27e0e54..31e9f53 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -1000,7 +1000,7 @@ xfs_fiemap_format( fiemap_flags |= FIEMAP_EXTENT_LAST; error = fiemap_fill_next_extent(fieinfo, logical, physical, - length, fiemap_flags); + length, 0, fiemap_flags); if (error > 0) { error = 0; *full = 1; /* user array now full */ diff --git a/include/linux/fs.h b/include/linux/fs.h index 121f11f..1a96f9b 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1479,7 +1479,7 @@ struct fiemap_extent_info { fiemap_extent array */ }; int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical, - u64 phys, u64 len, u32 flags); + u64 phys, u64 len, u64 phys_len, u32 flags); int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags); /* 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]; }; @@ -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 */ #define FIEMAP_EXTENT_DATA_ENCRYPTED 0x00000080 /* Data is encrypted by fs. * Sets EXTENT_ENCODED */ #define FIEMAP_EXTENT_NOT_ALIGNED 0x00000100 /* Extent offsets may not be
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. Extend arguments of fiemap_fill_next_extent and update all users. [1] http://article.gmane.org/gmane.comp.file-systems.ext4/8871 [2] http://thread.gmane.org/gmane.comp.file-systems.ext4/8870 [3] http://thread.gmane.org/gmane.linux.file-systems/77632 (v1) [4] http://www.spinics.net/lists/linux-fsdevel/msg69078.html (v2) Cc: Al Viro <viro@zeniv.linux.org.uk> CC: Andreas Dilger <adilger@dilger.ca> CC: Christoph Hellwig <hch@infradead.org> CC: Mark Fasheh <mfasheh@suse.com> Signed-off-by: David Sterba <dsterba@suse.cz> --- fs/btrfs/extent_io.c | 2 +- fs/ext4/extents.c | 3 ++- fs/ext4/inline.c | 2 +- fs/gfs2/inode.c | 2 +- fs/ioctl.c | 18 ++++++++++++------ fs/nilfs2/inode.c | 8 +++++--- fs/ocfs2/extent_map.c | 4 ++-- fs/xfs/xfs_iops.c | 2 +- include/linux/fs.h | 2 +- include/uapi/linux/fiemap.h | 6 +++++- 10 files changed, 31 insertions(+), 18 deletions(-)