[PATCH] btrfs: return whole extents in fiemap

[PATCH] btrfs: return whole extents in fiemap

[Boris Burkov]

`xfs_io -c 'fiemap <off> <len>' <file>`
can give surprising results on btrfs that differ from xfs.

btrfs spits out extents trimmed to fit the user input. If the user's
fiemap request has an offset, then rather than returning each whole
extent which intersects that range, we also trim the start extent to not
have start < off.

Documentation in filesystems/fiemap.txt and the xfs_io man page suggests
that returning the whole extent is expected.

Some cases which all yield the same fiemap in xfs, but not btrfs:
dd if=/dev/zero of=$f bs=4k count=1
sudo xfs_io -c 'fiemap 0 1024' $f
  0: [0..7]: 26624..26631
sudo xfs_io -c 'fiemap 2048 1024' $f
  0: [4..7]: 26628..26631
sudo xfs_io -c 'fiemap 2048 4096' $f
  0: [4..7]: 26628..26631
sudo xfs_io -c 'fiemap 3584 512' $f
  0: [7..7]: 26631..26631
sudo xfs_io -c 'fiemap 4091 5' $f
  0: [7..6]: 26631..26630

I believe this is a consequence of the logic for merging contiguous
extents represented by separate extent items. That logic needs to track
the last offset as it loops through the extent items, which happens to
pick up the start offset on the first iteration, and trim off the
beginning of the full extent. To fix it, start `off` at 0 rather than
`start` so that we keep the iteration/merging intact without cutting off
the start of the extent.

after the fix, all the above commands give:
0: [0..7]: 26624..26631

The merging logic is exercised by xfstest generic/483, and I have
written a new xfstest for checking we don't have backwards or
zero-length fiemaps for cases like those above.

Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/extent_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 7ad2169e7487..5b2a8a314adf 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4975,7 +4975,7 @@ int extent_fiemap(struct btrfs_inode *inode, struct fiemap_extent_info *fieinfo,
 		  u64 start, u64 len)
 {
 	int ret = 0;
-	u64 off = start;
+	u64 off = 0;
 	u64 max = start + len;
 	u32 flags = 0;
 	u32 found_type;
-- 
2.30.2

Re: [PATCH] btrfs: return whole extents in fiemap

[Josef Bacik]

On 4/6/21 6:31 PM, Boris Burkov wrote:
> `xfs_io -c 'fiemap <off> <len>' <file>`
> can give surprising results on btrfs that differ from xfs.
> 
> btrfs spits out extents trimmed to fit the user input. If the user's
> fiemap request has an offset, then rather than returning each whole
> extent which intersects that range, we also trim the start extent to not
> have start < off.
> 
> Documentation in filesystems/fiemap.txt and the xfs_io man page suggests
> that returning the whole extent is expected.
> 
> Some cases which all yield the same fiemap in xfs, but not btrfs:
> dd if=/dev/zero of=$f bs=4k count=1
> sudo xfs_io -c 'fiemap 0 1024' $f
>    0: [0..7]: 26624..26631
> sudo xfs_io -c 'fiemap 2048 1024' $f
>    0: [4..7]: 26628..26631
> sudo xfs_io -c 'fiemap 2048 4096' $f
>    0: [4..7]: 26628..26631
> sudo xfs_io -c 'fiemap 3584 512' $f
>    0: [7..7]: 26631..26631
> sudo xfs_io -c 'fiemap 4091 5' $f
>    0: [7..6]: 26631..26630
> 
> I believe this is a consequence of the logic for merging contiguous
> extents represented by separate extent items. That logic needs to track
> the last offset as it loops through the extent items, which happens to
> pick up the start offset on the first iteration, and trim off the
> beginning of the full extent. To fix it, start `off` at 0 rather than
> `start` so that we keep the iteration/merging intact without cutting off
> the start of the extent.
> 
> after the fix, all the above commands give:
> 0: [0..7]: 26624..26631
> 
> The merging logic is exercised by xfstest generic/483, and I have
> written a new xfstest for checking we don't have backwards or
> zero-length fiemaps for cases like those above.
> 
> Signed-off-by: Boris Burkov <boris@bur.io>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH] btrfs: return whole extents in fiemap

[David Sterba]

On Tue, Apr 06, 2021 at 03:31:18PM -0700, Boris Burkov wrote:
> `xfs_io -c 'fiemap <off> <len>' <file>`
> can give surprising results on btrfs that differ from xfs.
> 
> btrfs spits out extents trimmed to fit the user input. If the user's
> fiemap request has an offset, then rather than returning each whole
> extent which intersects that range, we also trim the start extent to not
> have start < off.
> 
> Documentation in filesystems/fiemap.txt and the xfs_io man page suggests
> that returning the whole extent is expected.
> 
> Some cases which all yield the same fiemap in xfs, but not btrfs:
> dd if=/dev/zero of=$f bs=4k count=1
> sudo xfs_io -c 'fiemap 0 1024' $f
>   0: [0..7]: 26624..26631
> sudo xfs_io -c 'fiemap 2048 1024' $f
>   0: [4..7]: 26628..26631
> sudo xfs_io -c 'fiemap 2048 4096' $f
>   0: [4..7]: 26628..26631
> sudo xfs_io -c 'fiemap 3584 512' $f
>   0: [7..7]: 26631..26631
> sudo xfs_io -c 'fiemap 4091 5' $f
>   0: [7..6]: 26631..26630
> 
> I believe this is a consequence of the logic for merging contiguous
> extents represented by separate extent items. That logic needs to track
> the last offset as it loops through the extent items, which happens to
> pick up the start offset on the first iteration, and trim off the
> beginning of the full extent. To fix it, start `off` at 0 rather than
> `start` so that we keep the iteration/merging intact without cutting off
> the start of the extent.
> 
> after the fix, all the above commands give:
> 0: [0..7]: 26624..26631
> 
> The merging logic is exercised by xfstest generic/483, and I have
> written a new xfstest for checking we don't have backwards or
> zero-length fiemaps for cases like those above.
> 
> Signed-off-by: Boris Burkov <boris@bur.io>

Added to misc-next, thanks.

> @@ -4975,7 +4975,7 @@ int extent_fiemap(struct btrfs_inode *inode, struct fiemap_extent_info *fieinfo,
>  		  u64 start, u64 len)
>  {
>  	int ret = 0;
> -	u64 off = start;
> +	u64 off = 0;

I've moved the initialization out of the declaration block and added a
comment, among all the others it looks as yet another zero init but this
must be zero for a specific reason.