fiemap: introduce EXTENT_DATA_COMPRESSED flag

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.

From the point of view of the kernel API (fiemap_fill_next_extent),

include/uapi/linux/fiemap.h +0 -0

index 93abfcd..0e32cae 100644

Quoted 8 lines · David Sterba

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...

Quoted 12 lines

include/uapi/linux/fiemap.h +3 -1

index 93abfcd..0e32cae 100644
@@ -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;

include/uapi/linux/fiemap.h struct fiemap_extent { · line 27

Quoted 3 lines

include/uapi/linux/fiemap.h +0 -0

 	__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....

include/uapi/linux/fiemap.h struct fiemap { · line 56

Quoted 6 lines

include/uapi/linux/fiemap.h +2 -0

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

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 10 lines · Dave Chinner

Quoted 8 lines · David Sterba

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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);

Quoted 10 lines

Quoted 10 lines

include/uapi/linux/fiemap.h +3 -1

@@ -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;

Quoted 8 lines

include/uapi/linux/fiemap.h struct fiemap_extent { · line 27

Quoted 3 lines

include/uapi/linux/fiemap.h +0 -0

 	__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.

Quoted 3 lines

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.

Quoted 10 lines · Dave Chinner

Quoted 8 lines · David Sterba

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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);

Quoted 12 lines

Quoted 12 lines

include/uapi/linux/fiemap.h +3 -1

index 93abfcd..0e32cae 100644
@@ -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;

Quoted 8 lines

include/uapi/linux/fiemap.h struct fiemap_extent { · line 27

Quoted 3 lines

include/uapi/linux/fiemap.h +0 -0

 	__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.

Quoted 3 lines

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

Quoted 11 lines

include/uapi/linux/fiemap.h struct fiemap { · line 56

Quoted 6 lines

include/uapi/linux/fiemap.h +2 -0

@@ -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,

Quoted 4 lines

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Cheers, Andreas

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.

Quoted 1 line · Andreas Dilger

any progress on this patch series?

I'm sorry I got distracted at the end of year and did not finish the series.

Quoted 6 lines

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.

Quoted 3 lines

- 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.

Quoted 9 lines

- 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

Quoted 1 line

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.

Quoted 13 lines · David Sterba

Quoted 1 line · Andreas Dilger

any progress on this patch series?

I'm sorry I got distracted at the end of year and did not finish the series.

Quoted 6 lines

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

Quoted 8 lines

fe_phys_length will be always valid, so other the flags are set only if it's not equal to the logical length.

Quoted 3 lines

- 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

Quoted 11 lines

Quoted 10 lines

- 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.

Quoted 5 lines

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

Quoted 5 lines

Quoted 3 lines

- 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.

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.

Quoted 1 line

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.

Set the EXTENT_DATA_COMPRESSED flag together with EXTENT_ENCODED as defined by fiemap spec.

Signed-off-by David Sterba <[email protected]>

---
fs/btrfs/extent_io.c | 9 +++++++--

1 files changed, 7 insertions(+), 2 deletions(-)

fs/btrfs/extent_io.c +0 -0

index 5ea0ef5..8a28f15 100644

The original FIEMAP patch did not define this bit, btrfs will make use of it. The defined constant maintains the same value as originally proposed.

Currently, the 'filefrag' utility has no way to recognize and denote a compressed extent. As implemented in btrfs right now, the compression step splits a big extent into smaller chunks and this is reported as a heavily fragmented file. Adding the flag to filefrag will at least give some explanation why, this has been confusing users for some time already.

V3:
Based on feedback from Andreas, implement #1 from V2, current users of fiemap_fill_next_extent (fs/, ext4, gfs2, ocfs2, nilfs2, xfs) updated accordingly, no functional change.

V2:
Based on feedback from Andreas, the fiemap_extent is now able to hold the physical extent length, to be filled by the filesystem callback.

The filesystems do not have access to the structure that is passed back to userspace and are supposed to call fiemap_fill_next_extent, there's no direct way to fill fe_phys_length. There are two ways to pass it:

1) extend fiemap_fill_next_extent to take phys_length and update all users (ext4, gfs2, ocfs2, nilfs2, xfs)

2) add new function that takes arguments for all the fiemap_extent items, newly added phys_length compared to fiemap_fill_next_extent

David Sterba (4):
fiemap: fix comment at EXTENT_DATA_ENCRYPTED
fiemap: add EXTENT_DATA_COMPRESSED flag
btrfs: set FIEMAP_EXTENT_DATA_COMPRESSED for compressed extents Documentation/fiemap: Document the DATA_COMPRESSED flag

Documentation/filesystems/fiemap.txt |   17 +++++++++++++----
fs/btrfs/extent_io.c                 |    9 +++++++--
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          |    8 ++++++--
11 files changed, 51 insertions(+), 24 deletions(-)

Signature · 1 line

1.7.9