Re: [Ocfs2-devel] [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag
On Fri, Jul 25, 2014 at 4:04 AM, Andreas Dilger adil...@dilger.ca wrote: On Jul 24, 2014, at 1:22 PM, David Sterba dste...@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_LENGTH0x0010 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag
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 0x0010 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag
On Jul 24, 2014, at 1:22 PM, David Sterba dste...@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_LENGTH0x0010 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 signature.asc Description: Message signed with OpenPGP using GPGMail
Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag
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 0x0010
since this flag was never used.
Cheers, Andreas
On Dec 12, 2013, at 5:02 PM, Andreas Dilger adil...@dilger.ca wrote:
On Dec 12, 2013, at 4:24 PM, Dave Chinner da...@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,
Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4 v3] fiemap: add 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.
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 v...@zeniv.linux.org.uk
CC: Andreas Dilger adil...@dilger.ca
CC: Christoph Hellwig h...@infradead.org
CC: Mark Fasheh mfas...@suse.com
Signed-off-by: David Sterba dste...@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(-)
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
Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag
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_ENCODED0x0008 /* Data can not be
read
* while fs is unmounted */
+#define FIEMAP_EXTENT_DATA_COMPRESSED0x0040 /* Data is
compressed by fs.
+ * Sets EXTENT_ENCODED */
i.e. here.
Cheers,
Dave.
--
Dave Chinner
da...@fromorbit.com
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag
On Dec 12, 2013, at 4:24 PM, Dave Chinner da...@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_ENCODED0x0008 /* Data can not be
read
* while fs is unmounted */
+#define FIEMAP_EXTENT_DATA_COMPRESSED 0x0040 /* Data is
compressed by fs.
+* Sets EXTENT_ENCODED */
i.e. here.
Cheers,
Dave.
--
Dave Chinner
da...@fromorbit.com
Cheers, Andreas
Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag
On Thu, Dec 12, 2013 at 05:02:57PM -0700, Andreas Dilger wrote:
On Dec 12, 2013, at 4:24 PM, Dave Chinner da...@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
