[PATCH 0/4 v3] fiemap: introduce EXTENT_DATA

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 <***@zeniv.linux.org.uk>
CC: Andreas Dilger <***@dilger.ca>
CC: Christoph Hellwig <***@infradead.org>
CC: Mark Fasheh <***@suse.com>
Signed-off-by: David Sterba <***@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 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

--
1.7.9

Dave Chinner

2013-12-12 23:24:43 UTC

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

Post by David Sterba
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...

Post by David Sterba
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....

Post by David Sterba
@@ -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
***@fromorbit.com

Andreas Dilger

2013-12-13 00:02:57 UTC

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.

Post by Dave Chinner
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);

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.

Post by Dave Chinner
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

i.e. here.
Cheers,
Dave.
--
Dave Chinner

Cheers, Andreas

Dave Chinner

2013-12-13 01:22:51 UTC

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

Post by Andreas Dilger
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.

Post by Andreas Dilger
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...

Post by Andreas Dilger
then hopefully others will get it correct also.

Agreed.

Seems reasonable to have a WARN_ONCE() in that case. That would catch bugs
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.

Post by David Sterba
--- 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];
};

Would it make sense to rename fe_length to fe_logi_length (or something,
#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.

Well, it's moot if we decide a specific flag for the fe_phys_len
field being valid is decided on ;)

Cheers,

Dave.

--
Dave Chinner
***@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Sterba

2013-12-16 16:49:32 UTC

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-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Andreas Dilger

2014-07-17 06:07:57 UTC

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

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.

i.e. here.
Cheers,
Dave.
--
Dave Chinner

Cheers, Andreas

David Sterba

2014-07-24 19:22:45 UTC

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

Post by Andreas Dilger
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.
- 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.

Post by Andreas Dilger
- 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.

Post by Andreas Dilger
- 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
#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-nilfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Andreas Dilger

2014-07-24 22:34:35 UTC

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

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

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

Cheers, Andreas

Rohan Puri

2014-07-25 06:20:58 UTC

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

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

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

Post by Andreas Dilger
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

Cheers, Andreas

Regards,
Rohan
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Sterba

2014-07-28 16:49:09 UTC

Thanks for the feedback.

This is my understanding and contradicts the first point.

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.

Post by Andreas Dilger
Alternately,
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.

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-ext4" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Christoph Hellwig

2013-12-13 11:06:08 UTC

Post by Dave Chinner
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.

Post by David Sterba
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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Andreas Dilger

2013-12-12 22:22:20 UTC