ext4: bmap & fiemap conversion to use iomap

fs/ext4/extents.c +0 -1

@@ -5130,40 +4927,42 @@ static int ext4_xattr_fiemap(struct inode *inode,
 				EXT4_I(inode)->i_extra_isize;
 		physical += offset;
 		length = EXT4_SB(inode->i_sb)->s_inode_size - offset;
-		flags |= FIEMAP_EXTENT_DATA_INLINE;
 		brelse(iloc.bh);
 	} else { /* external block */
 		physical = (__u64)EXT4_I(inode)->i_file_acl << blockbits;
 		length = inode->i_sb->s_blocksize;
 	}

fs/ext4/extents.c

Quoted 14 lines · Ritesh Harjani

Since ext4 already defines necessary iomap_ops required to move to iomap for fiemap, so this patch makes those changes to use existing iomap_ops for ext4_fiemap and thus removes all unwanted code.

Signed-off-by Ritesh Harjani <[email protected]>

---
  fs/ext4/extents.c | 279 +++++++---------------------------------------
  fs/ext4/inline.c  |  41 -------
  2 files changed, 38 insertions(+), 282 deletions(-)

fs/ext4/extents.c +0 -0

index 0de548bb3c90..901caee2fcb1 100644

<snip> Just a cursory glance...

Quoted 11 lines

fs/ext4/extents.c +0 -1

@@ -5130,40 +4927,42 @@ static int ext4_xattr_fiemap(struct inode *inode,
  				EXT4_I(inode)->i_extra_isize;
  		physical += offset;
  		length = EXT4_SB(inode->i_sb)->s_inode_size - offset;
-		flags |= FIEMAP_EXTENT_DATA_INLINE;
  		brelse(iloc.bh);
  	} else { /* external block */
  		physical = (__u64)EXT4_I(inode)->i_file_acl << blockbits;
  		length = inode->i_sb->s_blocksize;
  	}
 

fs/ext4/extents.c static int ext4_xattr_fiemap(struct inode *inode, · line 4940

Quoted 9 lines

fs/ext4/extents.c +5 -4

-	if (physical)
-		error = fiemap_fill_next_extent(fieinfo, 0, physical,
-						length, flags);
-	return (error < 0 ? error : 0);
+	iomap->addr = physical;
+	iomap->offset = 0;
+	iomap->length = length;
+	iomap->type = IOMAP_INLINE;
+	iomap->flags = 0;

Er... external "ACL" blocks aren't IOMAP_INLINE.

Hello Darrick,

Quoted 31 lines · Darrick J. Wong

fs/ext4/extents.c

Quoted 14 lines · Ritesh Harjani

Since ext4 already defines necessary iomap_ops required to move to iomap for fiemap, so this patch makes those changes to use existing iomap_ops for ext4_fiemap and thus removes all unwanted code.

Signed-off-by Ritesh Harjani <[email protected]>

---
  fs/ext4/extents.c | 279 +++++++---------------------------------------
  fs/ext4/inline.c  |  41 -------
  2 files changed, 38 insertions(+), 282 deletions(-)

fs/ext4/extents.c +0 -0

index 0de548bb3c90..901caee2fcb1 100644

<snip> Just a cursory glance...

Quoted 13 lines

fs/ext4/extents.c +0 -4

@@ -5130,40 +4927,42 @@ static int ext4_xattr_fiemap(struct inode *inode,
  				EXT4_I(inode)->i_extra_isize;
  		physical += offset;
  		length = EXT4_SB(inode->i_sb)->s_inode_size - offset;
-		flags |= FIEMAP_EXTENT_DATA_INLINE;
  		brelse(iloc.bh);
  	} else { /* external block */
  		physical = (__u64)EXT4_I(inode)->i_file_acl << blockbits;
  		length = inode->i_sb->s_blocksize;
  	}
-	if (physical)
-		error = fiemap_fill_next_extent(fieinfo, 0, physical,
-						length, flags);

Quoted 8 lines

fs/ext4/extents.c static int ext4_xattr_fiemap(struct inode *inode, · line 4940

Quoted 6 lines

fs/ext4/extents.c +5 -1

-	return (error < 0 ? error : 0);
+	iomap->addr = physical;
+	iomap->offset = 0;
+	iomap->length = length;
+	iomap->type = IOMAP_INLINE;
+	iomap->flags = 0;

Er... external "ACL" blocks aren't IOMAP_INLINE.

Sorry, I should have mentioned about this too in the cover letter. So current patchset is mainly only converting bmap & fiemap to use iomap APIs. Even the original implementation does not have external ACL block implemented for xattr_fiemap.

Let me spend some time to implement it. But I would still like to keep that as a separate patch.

But thanks for looking into it. There's this point 2.a & 2.b mentioned in the cover letter where I could really use your help in understanding if all of that is a known behavior from iomap_fiemap side (whenever you have some time of course :) )

-ritesh

Quoted 43 lines · Ritesh Harjani

Hello Darrick,

Quoted 39 lines · Darrick J. Wong

fs/ext4/extents.c

Quoted 14 lines · Ritesh Harjani

Since ext4 already defines necessary iomap_ops required to move to iomap for fiemap, so this patch makes those changes to use existing iomap_ops for ext4_fiemap and thus removes all unwanted code.

Signed-off-by Ritesh Harjani <[email protected]>

---
   fs/ext4/extents.c | 279 +++++++---------------------------------------
   fs/ext4/inline.c  |  41 -------
   2 files changed, 38 insertions(+), 282 deletions(-)

fs/ext4/extents.c +0 -0

index 0de548bb3c90..901caee2fcb1 100644

<snip> Just a cursory glance...

Quoted 9 lines

fs/ext4/extents.c +0 -1

@@ -5130,40 +4927,42 @@ static int ext4_xattr_fiemap(struct inode *inode,
   				EXT4_I(inode)->i_extra_isize;
   		physical += offset;
   		length = EXT4_SB(inode->i_sb)->s_inode_size - offset;
-		flags |= FIEMAP_EXTENT_DATA_INLINE;
   		brelse(iloc.bh);
   	} else { /* external block */
   		physical = (__u64)EXT4_I(inode)->i_file_acl << blockbits;
   		length = inode->i_sb->s_blocksize;

fs/ext4/extents.c static int ext4_xattr_fiemap(struct inode *inode, · line 4939

Quoted 10 lines

fs/ext4/extents.c +5 -4

   	}
-	if (physical)
-		error = fiemap_fill_next_extent(fieinfo, 0, physical,
-						length, flags);
-	return (error < 0 ? error : 0);
+	iomap->addr = physical;
+	iomap->offset = 0;
+	iomap->length = length;
+	iomap->type = IOMAP_INLINE;
+	iomap->flags = 0;

Er... external "ACL" blocks aren't IOMAP_INLINE.

Quoted 4 lines

Sorry, I should have mentioned about this too in the cover letter. So current patchset is mainly only converting bmap & fiemap to use iomap APIs. Even the original implementation does not have external ACL block implemented for xattr_fiemap.

Er ... yes it did. The "} else { /* external block */" block sets physical to i_file_acl.

Quoted 8 lines

Let me spend some time to implement it. But I would still like to keep that as a separate patch.

But thanks for looking into it. There's this point 2.a & 2.b mentioned in the cover letter where I could really use your help in understanding if all of that is a known behavior from iomap_fiemap side (whenever you have some time of course :) )

i'll go have a look.

--D

Quoted 2 lines

-ritesh

Hello All,

Background
==========
There are RFCv2 patches to move ext4 bmap & fiemap calls to use iomap APIs. This reduces the users of ext4_get_block API and thus a step towards getting rid of buffer_heads from ext4. Also reduces a lot of code by making use of existing iomap_ops (except for xattr implementation).

Testing (done on ext4 master branch)
========
'xfstests -g auto' passes with default mkfs/mount configuration (v/s which also pass with vanilla kernel without this patch). Except generic/473 which also failes on XFS. This seems to be the test case issue since it expects the data in slightly different way as compared to what iomap returns.
Point 2.a below describes more about this.

Observations/Review required
============================
1. bmap related old v/s new method differences:-
a. In case if addr > INT_MAX, it issues a warning and
returns 0 as the block no. While earlier it used to return the truncated value with no warning.

	b. block no. is only returned in case of iomap->type is IOMAP_MAPPED,
	   but not when iomap->type is IOMAP_UNWRITTEN. While with previously
	   we used to get block no. for both of above cases.

2. Fiemap related old v/s new method differences:-
a. iomap_fiemap returns the disk extent information in exact
correspondence with start of user requested logical offset till the length requested by user. While in previous implementation the returned information used to give the complete extent information if the range requested by user lies in between the extent mapping.

b. iomap_fiemap adds the FIEMAP_EXTENT_LAST flag also at the last
fiemap_extent mapping range requested by the user via fm_length ( if that has a valid mapped extent on the disk).

But if the user
requested for more fm_length which could not be mapped in the last fiemap_extent, then the flag is not set.

e.g. output for above differences 2.a & 2.b
===========================================
create a file with below cmds.
1. fallocate -o 0 -l 8K testfile.txt
2. xfs_io -c "pwrite 8K 8K" testfile.txt
3. check extent mapping:- xfs_io -c "fiemap -v" testfile.txt
4. run this binary on with and without these patches:- ./a.out (test_fiemap_diff.c) [4]

o/p of xfs_io -c "fiemap -v"
============================================
With this patch on patched kernel:-
testfile.txt:

 EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS
   0: [0..15]:         122802736..122802751    16 0x800
   1: [16..31]:        122687536..122687551    16   0x1

without patch on vanilla kernel (no difference):-
testfile.txt:

 EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS
   0: [0..15]:         332211376..332211391    16 0x800
   1: [16..31]:        332722392..332722407    16   0x1

o/p of a.out without patch:-
================
riteshh-> ./a.out
logical: [ 0.. 15] phys: 332211376..332211391 flags: 0x800 tot: 16
(0) extent flag = 2048

o/p of a.out with patch (both point 2.a & 2.b could be seen)
=======================
riteshh-> ./a.out
logical: [ 0.. 7] phys: 122802736..122802743 flags: 0x801 tot: 8
(0) extent flag = 2049

FYI - In test_fiemap_diff.c test we had
a. fm_extent_count = 1
b. fm_start = 0
c. fm_length = 4K
Whereas when we change fm_extent_count = 32, then we don't see any difference.

e.g. output for above difference listed in point 1.b
====================================================

o/p without patch (block no returned for unwritten block as well) =========Testing IOCTL FIBMAP=========
File size = 16384, blkcnt = 4, blocksize = 4096

  0   41526422
  1   41526423
  2   41590299
  3   41590300

o/p with patch (0 returned for unwritten block)
=========Testing IOCTL FIBMAP=========
File size = 16384, blkcnt = 4, blocksize = 4096

  0          0          0
  1          0          0
  2   15335942      29953
  3   15335943      29953

Summary:-
========
Due to some of the observational differences to user, listed above, requesting to please help with a careful review in moving this to iomap. Digging into some older threads, it looks like these differences should be fine, since the same tools have been working fine with XFS (which uses iomap based implementation) [1]
Also as Ted suggested in [3]: Fiemap & bmap spec could be made based on the ext4 implementation. But since all the tools also work with xfs which uses iomap based fiemap, so we should be good there.

References of some previous discussions:
=======================================
[1]: https://www.spinics.net/lists/linux-fsdevel/msg128370.html
[2]: https://www.spinics.net/lists/linux-fsdevel/msg127675.html
[3]: https://www.spinics.net/lists/linux-fsdevel/msg128368.html
[4]: https://raw.githubusercontent.com/riteshharjani/LinuxStudy/master/tools/test_fiemap_diff.c
[RFCv1]: https://www.spinics.net/lists/linux-ext4/msg67077.html

Ritesh Harjani (4):
ext4: Add IOMAP_F_MERGED for non-extent based mapping
ext4: Optimize ext4_ext_precache for 0 depth
ext4: Move ext4 bmap to use iomap infrastructure.
ext4: Move ext4_fiemap to use iomap infrastructure

 fs/ext4/extents.c | 288 +++++++---------------------------------------
 fs/ext4/inline.c  |  41 -------
 fs/ext4/inode.c   |   6 +-
 3 files changed, 49 insertions(+), 286 deletions(-)

Signature · 1 line

2.21.0

Quoted 18 lines · Ritesh Harjani

Hello All,

Background
==========
There are RFCv2 patches to move ext4 bmap & fiemap calls to use iomap APIs. This reduces the users of ext4_get_block API and thus a step towards getting rid of buffer_heads from ext4. Also reduces a lot of code by making use of existing iomap_ops (except for xattr implementation).

Testing (done on ext4 master branch)
========
'xfstests -g auto' passes with default mkfs/mount configuration (v/s which also pass with vanilla kernel without this patch). Except generic/473 which also failes on XFS. This seems to be the test case issue since it expects the data in slightly different way as compared to what iomap returns.
Point 2.a below describes more about this.

Quoted 6 lines

Observations/Review required
============================
1. bmap related old v/s new method differences:-
a. In case if addr > INT_MAX, it issues a warning and
returns 0 as the block no. While earlier it used to return the truncated value with no warning.

Good...

Quoted 3 lines

	b. block no. is only returned in case of iomap->type is IOMAP_MAPPED,
	   but not when iomap->type is IOMAP_UNWRITTEN. While with previously
	   we used to get block no. for both of above cases.

Assuming the only remaining usecase of bmap is to tell old bootloaders where to find vmlinuz blocks on disk, I don't see much reason to map unwritten blocks -- there's no data there, and if your bootloader writes to the filesystem(!) then you can't read whatever was written there anyway.

Uh, can we put this ioctl on the deprecation list, please? :)

Quoted 6 lines

2. Fiemap related old v/s new method differences:-
a. iomap_fiemap returns the disk extent information in exact
correspondence with start of user requested logical offset till the length requested by user. While in previous implementation the returned information used to give the complete extent information if the range requested by user lies in between the extent mapping.

This is a topic of much disagreement. The FIEMAP documentation says that the call must return data for the requested range, but *may* return a mapping that extends beyond the requested range.

XFS (and now iomap) only return data for the requested range, whereas ext4 has (had?) the behavior you describe. generic/473 was an attempt to enforce the ext4 behavior across all filesystems, but I put it in my dead list and never run it.

So it's a behavioral change, but the new behavior isn't forbidden.

Quoted 4 lines

b. iomap_fiemap adds the FIEMAP_EXTENT_LAST flag also at the last
fiemap_extent mapping range requested by the user via fm_length ( if that has a valid mapped extent on the disk).

That sounds like a bug. _LAST is supposed to be set on the last extent in the file, not the last record in the queried dataset.

Quoted 4 lines

But if the user
requested for more fm_length which could not be mapped in the last fiemap_extent, then the flag is not set.

Yes... if there were more extents to map than there was space in the map array, then _LAST should remain unset to encourage userspace to come back for the rest of the mappings.

(Unless maybe I'm misunderstanding here...)

Quoted 61 lines

e.g. output for above differences 2.a & 2.b
===========================================
create a file with below cmds.
1. fallocate -o 0 -l 8K testfile.txt
2. xfs_io -c "pwrite 8K 8K" testfile.txt
3. check extent mapping:- xfs_io -c "fiemap -v" testfile.txt
4. run this binary on with and without these patches:- ./a.out (test_fiemap_diff.c) [4]

o/p of xfs_io -c "fiemap -v"
============================================
With this patch on patched kernel:-
testfile.txt:

  EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS
    0: [0..15]:         122802736..122802751    16 0x800
    1: [16..31]:        122687536..122687551    16   0x1

without patch on vanilla kernel (no difference):-
testfile.txt:

  EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS
    0: [0..15]:         332211376..332211391    16 0x800
    1: [16..31]:        332722392..332722407    16   0x1

o/p of a.out without patch:-
================
riteshh-> ./a.out
logical: [ 0.. 15] phys: 332211376..332211391 flags: 0x800 tot: 16
(0) extent flag = 2048

o/p of a.out with patch (both point 2.a & 2.b could be seen)
=======================
riteshh-> ./a.out
logical: [ 0.. 7] phys: 122802736..122802743 flags: 0x801 tot: 8
(0) extent flag = 2049

FYI - In test_fiemap_diff.c test we had
a. fm_extent_count = 1
b. fm_start = 0
c. fm_length = 4K
Whereas when we change fm_extent_count = 32, then we don't see any difference.

e.g. output for above difference listed in point 1.b
====================================================

o/p without patch (block no returned for unwritten block as well) =========Testing IOCTL FIBMAP=========
File size = 16384, blkcnt = 4, blocksize = 4096

   0   41526422
   1   41526423
   2   41590299
   3   41590300

o/p with patch (0 returned for unwritten block)
=========Testing IOCTL FIBMAP=========
File size = 16384, blkcnt = 4, blocksize = 4096

   0          0          0
   1          0          0
   2   15335942      29953
   3   15335943      29953

Quoted 10 lines

Summary:-
========
Due to some of the observational differences to user, listed above, requesting to please help with a careful review in moving this to iomap. Digging into some older threads, it looks like these differences should be fine, since the same tools have been working fine with XFS (which uses iomap based implementation) [1]
Also as Ted suggested in [3]: Fiemap & bmap spec could be made based on the ext4 implementation. But since all the tools also work with xfs which uses iomap based fiemap, so we should be good there.

<nod> Thanks for the worked example output. :)

--D

Quoted 24 lines

References of some previous discussions:
=======================================
[1]: https://www.spinics.net/lists/linux-fsdevel/msg128370.html
[2]: https://www.spinics.net/lists/linux-fsdevel/msg127675.html
[3]: https://www.spinics.net/lists/linux-fsdevel/msg128368.html
[4]: https://raw.githubusercontent.com/riteshharjani/LinuxStudy/master/tools/test_fiemap_diff.c
[RFCv1]: https://www.spinics.net/lists/linux-ext4/msg67077.html

Ritesh Harjani (4):
ext4: Add IOMAP_F_MERGED for non-extent based mapping
ext4: Optimize ext4_ext_precache for 0 depth
ext4: Move ext4 bmap to use iomap infrastructure.
ext4: Move ext4_fiemap to use iomap infrastructure

  fs/ext4/extents.c | 288 +++++++---------------------------------------
  fs/ext4/inline.c  |  41 -------
  fs/ext4/inode.c   |   6 +-
  3 files changed, 49 insertions(+), 286 deletions(-)

Signature · 1 line

2.21.0

Quoted 32 lines · Darrick J. Wong

Quoted 18 lines · Ritesh Harjani

Hello All,

Background
==========
There are RFCv2 patches to move ext4 bmap & fiemap calls to use iomap APIs. This reduces the users of ext4_get_block API and thus a step towards getting rid of buffer_heads from ext4. Also reduces a lot of code by making use of existing iomap_ops (except for xattr implementation).

Testing (done on ext4 master branch)
========
'xfstests -g auto' passes with default mkfs/mount configuration (v/s which also pass with vanilla kernel without this patch). Except generic/473 which also failes on XFS. This seems to be the test case issue since it expects the data in slightly different way as compared to what iomap returns.
Point 2.a below describes more about this.

Quoted 6 lines

Observations/Review required
============================
1. bmap related old v/s new method differences:-
    a. In case if addr > INT_MAX, it issues a warning and returns 0 as the block no. While earlier it used to return the truncated value with no warning.

Good...

Quoted 3 lines

    b. block no. is only returned in case of iomap->type is IOMAP_MAPPED, but not when iomap->type is IOMAP_UNWRITTEN. While with previously we used to get block no. for both of above cases.

Quoted 5 lines

Assuming the only remaining usecase of bmap is to tell old bootloaders where to find vmlinuz blocks on disk, I don't see much reason to map unwritten blocks -- there's no data there, and if your bootloader writes to the filesystem(!) then you can't read whatever was written there anyway.

Yes, no objection there. Just wanted to get it reviewed.

Quoted 14 lines

Uh, can we put this ioctl on the deprecation list, please? :)

Quoted 6 lines

2. Fiemap related old v/s new method differences:-
    a. iomap_fiemap returns the disk extent information in exact correspondence with start of user requested logical offset till the length requested by user. While in previous implementation the returned information used to give the complete extent information if the range requested by user lies in between the extent mapping.

This is a topic of much disagreement.  The FIEMAP documentation says that the call must return data for the requested range, but *may* return a mapping that extends beyond the requested range.

Quoted 6 lines

XFS (and now iomap) only return data for the requested range, whereas ext4 has (had?) the behavior you describe.  generic/473 was an attempt to enforce the ext4 behavior across all filesystems, but I put it in my dead list and never run it.

So it's a behavioral change, but the new behavior isn't forbidden.

Sure, thanks.

Quoted 1 line

Quoted 6 lines

Quoted 3 lines

    b. iomap_fiemap adds the FIEMAP_EXTENT_LAST flag also at the last fiemap_extent mapping range requested by the user via fm_length ( if that has a valid mapped extent on the disk).

That sounds like a bug.  _LAST is supposed to be set on the last extent in the file, not the last record in the queried dataset.

Thought so too, sure will spend some time to try fixing it.

Quoted 85 lines

Quoted 6 lines

But if the user
       requested for more fm_length which could not be mapped in the last fiemap_extent, then the flag is not set.

Yes... if there were more extents to map than there was space in the map array, then _LAST should remain unset to encourage userspace to come back for the rest of the mappings.

(Unless maybe I'm misunderstanding here...)

Quoted 61 lines

e.g. output for above differences 2.a & 2.b
===========================================
create a file with below cmds.
1. fallocate -o 0 -l 8K testfile.txt
2. xfs_io -c "pwrite 8K 8K" testfile.txt
3. check extent mapping:- xfs_io -c "fiemap -v" testfile.txt
4. run this binary on with and without these patches:- ./a.out (test_fiemap_diff.c) [4]

o/p of xfs_io -c "fiemap -v"
============================================
With this patch on patched kernel:- testfile.txt:
  EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS 0: [0..15]:         122802736..122802751    16 0x800 1: [16..31]:        122687536..122687551    16   0x1

without patch on vanilla kernel (no difference):- testfile.txt:
  EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS 0: [0..15]:         332211376..332211391    16 0x800 1: [16..31]:        332722392..332722407    16   0x1

o/p of a.out without patch:-
================
riteshh-> ./a.out
logical: [       0..      15] phys: 332211376..332211391 flags: 0x800 tot: 16
(0) extent flag = 2048

o/p of a.out with patch (both point 2.a & 2.b could be seen)
=======================
riteshh-> ./a.out
logical: [       0..       7] phys: 122802736..122802743 flags: 0x801 tot: 8
(0) extent flag = 2049

FYI - In test_fiemap_diff.c test we had
a. fm_extent_count = 1
b. fm_start = 0
c. fm_length = 4K
Whereas when we change fm_extent_count = 32, then we don't see any difference.

e.g. output for above difference listed in point 1.b
====================================================

o/p without patch (block no returned for unwritten block as well) =========Testing IOCTL FIBMAP=========
File size = 16384, blkcnt = 4, blocksize = 4096
   0   41526422
   1   41526423
   2   41590299
   3   41590300

o/p with patch (0 returned for unwritten block)
=========Testing IOCTL FIBMAP=========
File size = 16384, blkcnt = 4, blocksize = 4096
   0          0          0
   1          0          0
   2   15335942      29953
   3   15335943      29953

Quoted 10 lines

Summary:-
========
Due to some of the observational differences to user, listed above, requesting to please help with a careful review in moving this to iomap. Digging into some older threads, it looks like these differences should be fine, since the same tools have been working fine with XFS (which uses iomap based implementation) [1]
Also as Ted suggested in [3]: Fiemap & bmap spec could be made based on the ext4 implementation. But since all the tools also work with xfs which uses iomap based fiemap, so we should be good there.

Quoted 1 line

<nod> Thanks for the worked example output. :)

Thanks for the review. :)

ritesh

Quoted 28 lines

--D

Quoted 25 lines

References of some previous discussions:
=======================================
[1]: https://www.spinics.net/lists/linux-fsdevel/msg128370.html
[2]: https://www.spinics.net/lists/linux-fsdevel/msg127675.html
[3]: https://www.spinics.net/lists/linux-fsdevel/msg128368.html
[4]: https://raw.githubusercontent.com/riteshharjani/LinuxStudy/master/tools/test_fiemap_diff.c

[RFCv1]: https://www.spinics.net/lists/linux-ext4/msg67077.html

Ritesh Harjani (4):
   ext4: Add IOMAP_F_MERGED for non-extent based mapping
   ext4: Optimize ext4_ext_precache for 0 depth
   ext4: Move ext4 bmap to use iomap infrastructure.
   ext4: Move ext4_fiemap to use iomap infrastructure

  fs/ext4/extents.c | 288 +++++++--------------------------------------- fs/ext4/inline.c  |  41 -------
  fs/ext4/inode.c   |   6 +-

  3 files changed, 49 insertions(+), 286 deletions(-)

Signature · 1 line

2.21.0

Quoted 77 lines · Ritesh Harjani

Quoted 39 lines · Darrick J. Wong

Quoted 23 lines · Ritesh Harjani

Hello All,

Background
==========
There are RFCv2 patches to move ext4 bmap & fiemap calls to use iomap APIs.
This reduces the users of ext4_get_block API and thus a step towards getting
rid of buffer_heads from ext4. Also reduces a lot of code by making use of
existing iomap_ops (except for xattr implementation).

Testing (done on ext4 master branch)
========
'xfstests -g auto' passes with default mkfs/mount configuration (v/s which also pass with vanilla kernel without this patch). Except generic/473 which also failes on XFS. This seems to be the test case issue
since it expects the data in slightly different way as compared to what iomap
returns.
Point 2.a below describes more about this.

Quoted 6 lines

Observations/Review required
============================
1. bmap related old v/s new method differences:-
    a. In case if addr > INT_MAX, it issues a warning and returns 0 as the block no. While earlier it used to return the truncated value with no warning.

Good...

Quoted 5 lines

    b. block no. is only returned in case of iomap->type is IOMAP_MAPPED,
       but not when iomap->type is IOMAP_UNWRITTEN. While with previously
       we used to get block no. for both of above cases.

Quoted 5 lines

Assuming the only remaining usecase of bmap is to tell old bootloaders where to find vmlinuz blocks on disk, I don't see much reason to map unwritten blocks -- there's no data there, and if your bootloader writes to the filesystem(!) then you can't read whatever was written there anyway.

Yes, no objection there. Just wanted to get it reviewed.

Quoted 16 lines

Uh, can we put this ioctl on the deprecation list, please? :)

Quoted 8 lines

2. Fiemap related old v/s new method differences:-
    a. iomap_fiemap returns the disk extent information in exact correspondence with start of user requested logical offset till the
       length requested by user. While in previous implementation the returned information used to give the complete extent information if
       the range requested by user lies in between the extent mapping.

This is a topic of much disagreement.  The FIEMAP documentation says that the call must return data for the requested range, but *may* return a mapping that extends beyond the requested range.

Quoted 6 lines

XFS (and now iomap) only return data for the requested range, whereas ext4 has (had?) the behavior you describe.  generic/473 was an attempt to enforce the ext4 behavior across all filesystems, but I put it in my dead list and never run it.

So it's a behavioral change, but the new behavior isn't forbidden.

Sure, thanks.

Quoted 1 line

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

    b. iomap_fiemap adds the FIEMAP_EXTENT_LAST flag also at the last fiemap_extent mapping range requested by the user via fm_length ( if that has a valid mapped extent on the disk).

That sounds like a bug.  _LAST is supposed to be set on the last extent in the file, not the last record in the queried dataset.

Thought so too, sure will spend some time to try fixing it.

Looked into this. I think below should fix our above reported problem with current iomap code.
If no objection I will send send PATCHv3 with below fix as the first patch in the series.

fs/iomap/fiemap.c +0 -0

index bccf305ea9ce..ee53991810d5 100644

fs/iomap/fiemap.c +2 -0

+                       flags |= FIEMAP_EXTENT_LAST;
+               ret = iomap_to_fiemap(fi, &ctx.prev, flags);
                if (ret < 0)
                        return ret;

        }

-ritesh

Quoted 129 lines

Quoted 94 lines

Quoted 7 lines

But if the user
       requested for more fm_length which could not be mapped in the last
       fiemap_extent, then the flag is not set.

Yes... if there were more extents to map than there was space in the map array, then _LAST should remain unset to encourage userspace to come back for the rest of the mappings.

(Unless maybe I'm misunderstanding here...)

Quoted 65 lines

e.g. output for above differences 2.a & 2.b
===========================================
create a file with below cmds.
1. fallocate -o 0 -l 8K testfile.txt
2. xfs_io -c "pwrite 8K 8K" testfile.txt
3. check extent mapping:- xfs_io -c "fiemap -v" testfile.txt
4. run this binary on with and without these patches:- ./a.out (test_fiemap_diff.c) [4]

o/p of xfs_io -c "fiemap -v"
============================================
With this patch on patched kernel:- testfile.txt:
  EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS 0: [0..15]:         122802736..122802751    16 0x800 1: [16..31]:        122687536..122687551    16   0x1

without patch on vanilla kernel (no difference):- testfile.txt:
  EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS 0: [0..15]:         332211376..332211391    16 0x800 1: [16..31]:        332722392..332722407    16   0x1

o/p of a.out without patch:-
================
riteshh-> ./a.out
logical: [       0..      15] phys: 332211376..332211391 flags:
0x800 tot: 16
(0) extent flag = 2048

o/p of a.out with patch (both point 2.a & 2.b could be seen)
=======================
riteshh-> ./a.out
logical: [       0..       7] phys: 122802736..122802743 flags:
0x801 tot: 8
(0) extent flag = 2049

FYI - In test_fiemap_diff.c test we had
a. fm_extent_count = 1
b. fm_start = 0
c. fm_length = 4K
Whereas when we change fm_extent_count = 32, then we don't see any difference.

e.g. output for above difference listed in point 1.b
====================================================

o/p without patch (block no returned for unwritten block as well) =========Testing IOCTL FIBMAP=========
File size = 16384, blkcnt = 4, blocksize = 4096
   0   41526422
   1   41526423
   2   41590299
   3   41590300

o/p with patch (0 returned for unwritten block)
=========Testing IOCTL FIBMAP=========
File size = 16384, blkcnt = 4, blocksize = 4096
   0          0          0
   1          0          0
   2   15335942      29953
   3   15335943      29953

Quoted 14 lines

Summary:-
========
Due to some of the observational differences to user, listed above, requesting to please help with a careful review in moving this to iomap. Digging into some older threads, it looks like these differences should be fine,
since the same tools have been working fine with XFS (which uses iomap based
implementation) [1]
Also as Ted suggested in [3]: Fiemap & bmap spec could be made based on the ext4
implementation. But since all the tools also work with xfs which uses iomap
based fiemap, so we should be good there.

Quoted 1 line

<nod> Thanks for the worked example output. :)

Thanks for the review. :)

ritesh

Quoted 28 lines

--D

Quoted 25 lines

References of some previous discussions:
=======================================
[1]: https://www.spinics.net/lists/linux-fsdevel/msg128370.html
[2]: https://www.spinics.net/lists/linux-fsdevel/msg127675.html
[3]: https://www.spinics.net/lists/linux-fsdevel/msg128368.html
[4]: https://raw.githubusercontent.com/riteshharjani/LinuxStudy/master/tools/test_fiemap_diff.c

[RFCv1]: https://www.spinics.net/lists/linux-ext4/msg67077.html

Ritesh Harjani (4):
   ext4: Add IOMAP_F_MERGED for non-extent based mapping
   ext4: Optimize ext4_ext_precache for 0 depth
   ext4: Move ext4 bmap to use iomap infrastructure.
   ext4: Move ext4_fiemap to use iomap infrastructure

  fs/ext4/extents.c | 288 +++++++--------------------------------------- fs/ext4/inline.c  |  41 -------
  fs/ext4/inode.c   |   6 +-

  3 files changed, 49 insertions(+), 286 deletions(-)

Signature · 1 line

2.21.0

Quoted 99 lines · Ritesh Harjani

Quoted 77 lines · Ritesh Harjani

Quoted 39 lines · Darrick J. Wong

Quoted 23 lines · Ritesh Harjani

Hello All,

Background
==========
There are RFCv2 patches to move ext4 bmap & fiemap calls to use iomap APIs.
This reduces the users of ext4_get_block API and thus a step towards getting
rid of buffer_heads from ext4. Also reduces a lot of code by making use of
existing iomap_ops (except for xattr implementation).

Testing (done on ext4 master branch)
========
'xfstests -g auto' passes with default mkfs/mount configuration (v/s which also pass with vanilla kernel without this patch). Except generic/473 which also failes on XFS. This seems to be the test case issue
since it expects the data in slightly different way as compared to what iomap
returns.
Point 2.a below describes more about this.

Quoted 6 lines

Observations/Review required
============================
1. bmap related old v/s new method differences:-
    a. In case if addr > INT_MAX, it issues a warning and returns 0 as the block no. While earlier it used to return the truncated value with no warning.

Good...

Quoted 5 lines

    b. block no. is only returned in case of iomap->type is
IOMAP_MAPPED,
       but not when iomap->type is IOMAP_UNWRITTEN. While with
previously
       we used to get block no. for both of above cases.

Quoted 5 lines

Assuming the only remaining usecase of bmap is to tell old bootloaders where to find vmlinuz blocks on disk, I don't see much reason to map unwritten blocks -- there's no data there, and if your bootloader writes to the filesystem(!) then you can't read whatever was written there anyway.

Yes, no objection there. Just wanted to get it reviewed.

Quoted 16 lines

Uh, can we put this ioctl on the deprecation list, please? :)

Quoted 8 lines

2. Fiemap related old v/s new method differences:-
    a. iomap_fiemap returns the disk extent information in exact correspondence with start of user requested logical
offset till the
       length requested by user. While in previous implementation the returned information used to give the complete extent
information if
       the range requested by user lies in between the extent mapping.

This is a topic of much disagreement.  The FIEMAP documentation says that the call must return data for the requested range, but *may* return a mapping that extends beyond the requested range.

Quoted 6 lines

XFS (and now iomap) only return data for the requested range, whereas ext4 has (had?) the behavior you describe.  generic/473 was an attempt to enforce the ext4 behavior across all filesystems, but I put it in my dead list and never run it.

So it's a behavioral change, but the new behavior isn't forbidden.

Sure, thanks.

Quoted 1 line

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

    b. iomap_fiemap adds the FIEMAP_EXTENT_LAST flag also at the last fiemap_extent mapping range requested by the user via fm_length ( if that has a valid mapped extent on the disk).

That sounds like a bug.  _LAST is supposed to be set on the last extent in the file, not the last record in the queried dataset.

Thought so too, sure will spend some time to try fixing it.

Looked into this. I think below should fix our above reported problem with current iomap code.
If no objection I will send send PATCHv3 with below fix as the first patch in the series.

fs/iomap/fiemap.c +0 -0

index bccf305ea9ce..ee53991810d5 100644

fs/iomap/fiemap.c int iomap_fiemap(struct inode *inode, struct · line 107

Quoted 9 lines

fs/iomap/fiemap.c +4 -1

@@ -100,7 +100,12 @@ int iomap_fiemap(struct inode *inode, struct
 fiemap_extent_info *fi,
         }
 
         if (ctx.prev.type != IOMAP_HOLE) {
-               ret = iomap_to_fiemap(fi, &ctx.prev, FIEMAP_EXTENT_LAST);
+               u32 flags = 0;
+               loff_t isize = i_size_read(inode);
+
+               if (ctx.prev.offset + ctx.prev.length >= isize)

What happens if ctx.prev actually is the last iomap in the file, but isize extends beyond that? e.g.,

# xfs_io -f -c 'pwrite 0 64k' /a
# truncate -s 100m /a
# filefrag -v /a

I think we need the fiemap variant of the iomap_begin functions to pass a flag in the iomap that the fiemap implementation can pick up.

--D

Quoted 140 lines

fs/iomap/fiemap.c +2 -0

+                       flags |= FIEMAP_EXTENT_LAST;
+               ret = iomap_to_fiemap(fi, &ctx.prev, flags);
                 if (ret < 0)
                         return ret;

         }

-ritesh

Quoted 129 lines

Quoted 94 lines

Quoted 7 lines

But if the user
       requested for more fm_length which could not be mapped in
the last
       fiemap_extent, then the flag is not set.

Yes... if there were more extents to map than there was space in the map array, then _LAST should remain unset to encourage userspace to come back for the rest of the mappings.

(Unless maybe I'm misunderstanding here...)

Quoted 65 lines

e.g. output for above differences 2.a & 2.b
===========================================
create a file with below cmds.
1. fallocate -o 0 -l 8K testfile.txt
2. xfs_io -c "pwrite 8K 8K" testfile.txt
3. check extent mapping:- xfs_io -c "fiemap -v" testfile.txt
4. run this binary on with and without these patches:- ./a.out (test_fiemap_diff.c) [4]

o/p of xfs_io -c "fiemap -v"
============================================
With this patch on patched kernel:- testfile.txt:
  EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS 0: [0..15]:         122802736..122802751    16 0x800 1: [16..31]:        122687536..122687551    16   0x1

without patch on vanilla kernel (no difference):- testfile.txt:
  EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS 0: [0..15]:         332211376..332211391    16 0x800 1: [16..31]:        332722392..332722407    16   0x1

o/p of a.out without patch:-
================
riteshh-> ./a.out
logical: [       0..      15] phys: 332211376..332211391 flags:
0x800 tot: 16
(0) extent flag = 2048

o/p of a.out with patch (both point 2.a & 2.b could be seen)
=======================
riteshh-> ./a.out
logical: [       0..       7] phys: 122802736..122802743 flags:
0x801 tot: 8
(0) extent flag = 2049

FYI - In test_fiemap_diff.c test we had
a. fm_extent_count = 1
b. fm_start = 0
c. fm_length = 4K
Whereas when we change fm_extent_count = 32, then we don't see any difference.

e.g. output for above difference listed in point 1.b
====================================================

o/p without patch (block no returned for unwritten block as well) =========Testing IOCTL FIBMAP=========
File size = 16384, blkcnt = 4, blocksize = 4096
   0   41526422
   1   41526423
   2   41590299
   3   41590300

o/p with patch (0 returned for unwritten block)
=========Testing IOCTL FIBMAP=========
File size = 16384, blkcnt = 4, blocksize = 4096
   0          0          0
   1          0          0
   2   15335942      29953
   3   15335943      29953

Quoted 14 lines

Summary:-
========
Due to some of the observational differences to user, listed above, requesting to please help with a careful review in moving this to iomap. Digging into some older threads, it looks like these differences should be fine,
since the same tools have been working fine with XFS (which uses iomap based
implementation) [1]
Also as Ted suggested in [3]: Fiemap & bmap spec could be made based on the ext4
implementation. But since all the tools also work with xfs which uses iomap
based fiemap, so we should be good there.

Quoted 1 line

<nod> Thanks for the worked example output. :)

Thanks for the review. :)

ritesh

Quoted 28 lines

--D

Quoted 25 lines

References of some previous discussions:
=======================================
[1]: https://www.spinics.net/lists/linux-fsdevel/msg128370.html
[2]: https://www.spinics.net/lists/linux-fsdevel/msg127675.html
[3]: https://www.spinics.net/lists/linux-fsdevel/msg128368.html
[4]: https://raw.githubusercontent.com/riteshharjani/LinuxStudy/master/tools/test_fiemap_diff.c

[RFCv1]: https://www.spinics.net/lists/linux-ext4/msg67077.html

Ritesh Harjani (4):
   ext4: Add IOMAP_F_MERGED for non-extent based mapping
   ext4: Optimize ext4_ext_precache for 0 depth
   ext4: Move ext4 bmap to use iomap infrastructure.
   ext4: Move ext4_fiemap to use iomap infrastructure

   fs/ext4/extents.c | 288 +++++++---------------------------------------
   fs/ext4/inline.c  |  41 -------
   fs/ext4/inode.c   |   6 +-
   3 files changed, 49 insertions(+), 286 deletions(-)

Signature · 1 line

2.21.0

Quoted 113 lines · Darrick J. Wong

Quoted 99 lines · Ritesh Harjani

Quoted 77 lines · Ritesh Harjani

Quoted 39 lines · Darrick J. Wong

Quoted 23 lines · Ritesh Harjani

Hello All,

Background
==========
There are RFCv2 patches to move ext4 bmap & fiemap calls to use iomap APIs.
This reduces the users of ext4_get_block API and thus a step towards getting
rid of buffer_heads from ext4. Also reduces a lot of code by making use of
existing iomap_ops (except for xattr implementation).

Testing (done on ext4 master branch)
========
'xfstests -g auto' passes with default mkfs/mount configuration (v/s which also pass with vanilla kernel without this patch). Except generic/473 which also failes on XFS. This seems to be the test case issue
since it expects the data in slightly different way as compared to what iomap
returns.
Point 2.a below describes more about this.

Quoted 6 lines

Observations/Review required
============================
1. bmap related old v/s new method differences:-
    a. In case if addr > INT_MAX, it issues a warning and returns 0 as the block no. While earlier it used to return the truncated value with no warning.

Good...

Quoted 5 lines

    b. block no. is only returned in case of iomap->type is
IOMAP_MAPPED,
       but not when iomap->type is IOMAP_UNWRITTEN. While with
previously
       we used to get block no. for both of above cases.

Quoted 5 lines

Assuming the only remaining usecase of bmap is to tell old bootloaders where to find vmlinuz blocks on disk, I don't see much reason to map unwritten blocks -- there's no data there, and if your bootloader writes to the filesystem(!) then you can't read whatever was written there anyway.

Yes, no objection there. Just wanted to get it reviewed.

Quoted 16 lines

Uh, can we put this ioctl on the deprecation list, please? :)

Quoted 8 lines

2. Fiemap related old v/s new method differences:-
    a. iomap_fiemap returns the disk extent information in exact correspondence with start of user requested logical
offset till the
       length requested by user. While in previous implementation the returned information used to give the complete extent
information if
       the range requested by user lies in between the extent mapping.

This is a topic of much disagreement.  The FIEMAP documentation says that the call must return data for the requested range, but *may* return a mapping that extends beyond the requested range.

Quoted 6 lines

XFS (and now iomap) only return data for the requested range, whereas ext4 has (had?) the behavior you describe.  generic/473 was an attempt to enforce the ext4 behavior across all filesystems, but I put it in my dead list and never run it.

So it's a behavioral change, but the new behavior isn't forbidden.

Sure, thanks.

Quoted 1 line

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

    b. iomap_fiemap adds the FIEMAP_EXTENT_LAST flag also at the last fiemap_extent mapping range requested by the user via fm_length ( if that has a valid mapped extent on the disk).

That sounds like a bug.  _LAST is supposed to be set on the last extent in the file, not the last record in the queried dataset.

Thought so too, sure will spend some time to try fixing it.

Looked into this. I think below should fix our above reported problem with current iomap code.
If no objection I will send send PATCHv3 with below fix as the first patch in the series.

fs/iomap/fiemap.c +0 -0

index bccf305ea9ce..ee53991810d5 100644

fs/iomap/fiemap.c int iomap_fiemap(struct inode *inode, struct · line 107

Quoted 9 lines

fs/iomap/fiemap.c +4 -1

@@ -100,7 +100,12 @@ int iomap_fiemap(struct inode *inode, struct
 fiemap_extent_info *fi,
         }
 
         if (ctx.prev.type != IOMAP_HOLE) {
-               ret = iomap_to_fiemap(fi, &ctx.prev, FIEMAP_EXTENT_LAST);
+               u32 flags = 0;
+               loff_t isize = i_size_read(inode);
+
+               if (ctx.prev.offset + ctx.prev.length >= isize)

What happens if ctx.prev actually is the last iomap in the file, but isize extends beyond that? e.g.,

Quoted 3 lines

# xfs_io -f -c 'pwrite 0 64k' /a
# truncate -s 100m /a
# filefrag -v /a

Err.. should never miss this truncate case.

Digging further, I see even generic_block_fiemap() does not take care of this case if the file isize is extended by truncate.
It happens to mark _LAST only based on i_size_read(). It seems only ext* family and hpfs filesystem seems to be using this currently. And gfs2 was the user of this api till sometime back before it finally moved to use iomap_fiemap() api.

Quoted 3 lines

I think we need the fiemap variant of the iomap_begin functions to pass a flag in the iomap that the fiemap implementation can pick up.

Sure, let me do some digging on this one. One challenge which I think would be for filesystems to tell *efficiently* on whether this is the last extent or not (without checking on every iomap_begin call about, if there exist a next extent on the disk by doing more I/O - that's why *efficiently*).

If done then -
we could use IOMAP_FIEMAP as the flag to pass to iomap_begin functions and it could return us the iomap->type marked with IOMAP_EXTENT_LAST which could represent that this is actually the last extent on disk for this inode.

Quoted 140 lines · Ritesh Harjani

Quoted 113 lines · Darrick J. Wong

Quoted 99 lines · Ritesh Harjani

Quoted 77 lines · Ritesh Harjani

Quoted 39 lines · Darrick J. Wong

Quoted 23 lines · Ritesh Harjani

Hello All,

Background
==========
There are RFCv2 patches to move ext4 bmap & fiemap calls to use iomap APIs.
This reduces the users of ext4_get_block API and thus a step towards getting
rid of buffer_heads from ext4. Also reduces a lot of code by making use of
existing iomap_ops (except for xattr implementation).

Testing (done on ext4 master branch)
========
'xfstests -g auto' passes with default mkfs/mount configuration (v/s which also pass with vanilla kernel without this patch). Except generic/473 which also failes on XFS. This seems to be the test case issue
since it expects the data in slightly different way as compared to what iomap
returns.
Point 2.a below describes more about this.

Quoted 6 lines

Observations/Review required
============================
1. bmap related old v/s new method differences:-
    a. In case if addr > INT_MAX, it issues a warning and returns 0 as the block no. While earlier it used to return the truncated value with no warning.

Good...

Quoted 5 lines

    b. block no. is only returned in case of iomap->type is
IOMAP_MAPPED,
       but not when iomap->type is IOMAP_UNWRITTEN. While with
previously
       we used to get block no. for both of above cases.

Quoted 5 lines

Assuming the only remaining usecase of bmap is to tell old bootloaders where to find vmlinuz blocks on disk, I don't see much reason to map unwritten blocks -- there's no data there, and if your bootloader writes to the filesystem(!) then you can't read whatever was written there anyway.

Yes, no objection there. Just wanted to get it reviewed.

Quoted 16 lines

Uh, can we put this ioctl on the deprecation list, please? :)

Quoted 8 lines

2. Fiemap related old v/s new method differences:-
    a. iomap_fiemap returns the disk extent information in exact correspondence with start of user requested logical
offset till the
       length requested by user. While in previous implementation the returned information used to give the complete extent
information if
       the range requested by user lies in between the extent mapping.

This is a topic of much disagreement.  The FIEMAP documentation says that the call must return data for the requested range, but *may* return a mapping that extends beyond the requested range.

Quoted 6 lines

XFS (and now iomap) only return data for the requested range, whereas ext4 has (had?) the behavior you describe.  generic/473 was an attempt to enforce the ext4 behavior across all filesystems, but I put it in my dead list and never run it.

So it's a behavioral change, but the new behavior isn't forbidden.

Sure, thanks.

Quoted 1 line

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

    b. iomap_fiemap adds the FIEMAP_EXTENT_LAST flag also at the last fiemap_extent mapping range requested by the user via fm_length ( if that has a valid mapped extent on the disk).

That sounds like a bug.  _LAST is supposed to be set on the last extent in the file, not the last record in the queried dataset.

Thought so too, sure will spend some time to try fixing it.

Looked into this. I think below should fix our above reported problem with current iomap code.
If no objection I will send send PATCHv3 with below fix as the first patch in the series.

fs/iomap/fiemap.c +0 -0

index bccf305ea9ce..ee53991810d5 100644

fs/iomap/fiemap.c int iomap_fiemap(struct inode *inode, struct · line 107

Quoted 9 lines

fs/iomap/fiemap.c +4 -1

@@ -100,7 +100,12 @@ int iomap_fiemap(struct inode *inode, struct
 fiemap_extent_info *fi,
          }
 
          if (ctx.prev.type != IOMAP_HOLE) {
-               ret = iomap_to_fiemap(fi, &ctx.prev, FIEMAP_EXTENT_LAST);
+               u32 flags = 0;
+               loff_t isize = i_size_read(inode);
+
+               if (ctx.prev.offset + ctx.prev.length >= isize)

What happens if ctx.prev actually is the last iomap in the file, but isize extends beyond that? e.g.,

Quoted 3 lines

# xfs_io -f -c 'pwrite 0 64k' /a
# truncate -s 100m /a
# filefrag -v /a

Err.. should never miss this truncate case.

Digging further, I see even generic_block_fiemap() does not take care of this case if the file isize is extended by truncate.
It happens to mark _LAST only based on i_size_read(). It seems only ext* family and hpfs filesystem seems to be using this currently. And gfs2 was the user of this api till sometime back before it finally moved to use iomap_fiemap() api.

Quoted 3 lines

I think we need the fiemap variant of the iomap_begin functions to pass a flag in the iomap that the fiemap implementation can pick up.

Sure, let me do some digging on this one. One challenge which I think would be for filesystems to tell *efficiently* on whether this is the last extent or not (without checking on every iomap_begin call about, if there exist a next extent on the disk by doing more I/O - that's why *efficiently*).

Quoted 5 lines

If done then -
we could use IOMAP_FIEMAP as the flag to pass to iomap_begin functions and it could return us the iomap->type marked with IOMAP_EXTENT_LAST which could represent that this is actually the last extent on disk for this inode.

So I think IOMAP_EXTENT_LAST should be treated as an optional flag. If the fs can provide it in a cheap way, do so. Otherwise don't bother. Because ultimately, the FIEMAP caller has to deal with not seeing IOMAP_EXTENT_LAST anyway (e.g. if the file has no extents or if someone modifies the file between the calls). So maybe we need to rather update the documentation that the IOMAP_EXTENT_LAST is best-effort only?

So I was making some changes along the above lines and I think we can take below approach for filesystem which could determine the _EXTENT_LAST relatively easily and for cases if it cannot
as Jan also mentioned we could keep the current behavior as is and let iomap core decide the last disk extent.

Quoted 55 lines

Quoted 49 lines

Quoted 25 lines

Quoted 11 lines

Looked into this. I think below should fix our above reported problem with current iomap code.
If no objection I will send send PATCHv3 with below fix as the first patch in the series.

fs/iomap/fiemap.c +0 -0

index bccf305ea9ce..ee53991810d5 100644

fs/iomap/fiemap.c int iomap_fiemap(struct inode *inode, struct · line 108

Quoted 10 lines

fs/iomap/fiemap.c +4 -1

@@ -100,7 +100,12 @@ int iomap_fiemap(struct inode *inode, struct
 fiemap_extent_info *fi,
           }
 
           if (ctx.prev.type != IOMAP_HOLE) {
-               ret = iomap_to_fiemap(fi, &ctx.prev, 
 FIEMAP_EXTENT_LAST);
+               u32 flags = 0;
+               loff_t isize = i_size_read(inode);
+
+               if (ctx.prev.offset + ctx.prev.length >= isize)

What happens if ctx.prev actually is the last iomap in the file, but isize extends beyond that?  e.g.,

Quoted 3 lines

# xfs_io -f -c 'pwrite 0 64k' /a
# truncate -s 100m /a
# filefrag -v /a

Err.. should never miss this truncate case.

Digging further, I see even generic_block_fiemap() does not take care of this case if the file isize is extended by truncate.
It happens to mark _LAST only based on i_size_read(). It seems only ext* family and hpfs filesystem seems to be using this currently. And gfs2 was the user of this api till sometime back before it finally moved to use iomap_fiemap() api.

Quoted 3 lines

I think we need the fiemap variant of the iomap_begin functions to pass a flag in the iomap that the fiemap implementation can pick up.

Sure, let me do some digging on this one. One challenge which I think would be for filesystems to tell *efficiently* on whether this is the last extent or not (without checking on every iomap_begin call about, if there exist a next extent on the disk by doing more I/O - that's why *efficiently*).

Quoted 5 lines

If done then -
we could use IOMAP_FIEMAP as the flag to pass to iomap_begin functions and it could return us the iomap->type marked with IOMAP_EXTENT_LAST which could represent that this is actually the last extent on disk for this inode.

Quoted 7 lines

So I think IOMAP_EXTENT_LAST should be treated as an optional flag. If the fs can provide it in a cheap way, do so. Otherwise don't bother. Because ultimately, the FIEMAP caller has to deal with not seeing IOMAP_EXTENT_LAST anyway (e.g. if the file has no extents or if someone modifies the file between the calls). So maybe we need to rather update the documentation that the IOMAP_EXTENT_LAST is best-effort only?

So I was making some changes along the above lines and I think we can take below approach for filesystem which could determine the _EXTENT_LAST relatively easily and for cases if it cannot
as Jan also mentioned we could keep the current behavior as is and let
iomap core decide the last disk extent.

For this -
1. We could use IOMAP_FIEMAP flag approach to pass this flag to ->iomap_begin fs functions. But we also will need IOMAP_F_EXTENT_LAST & IOMAP_F_EXTENT_NOT_LAST flag for filesystem to tell that whether this is the last extent or not. IOMAP_F_EXTENT_NOT_LAST is also needed so that FS can tell to iomap that the FS is capable of letting the iomap core know that whether it is the last extent or not. If this is not specified then iomap core will retain it's current behavior to mark FIEMAP_EXTENT_LAST.

Now in this case for ext4 we could simply call ext4_map_blocks second time with the next lblk to map to see if there exist a next mapping or any hole. Here again if the next lblk is a hole which is less the INT_MAX blocks then again it will mean that the current is not the last extent mapping. With that we could determine if the current mapping is the last extent mapping or not.

In case of non-extent based mapping, we still may not be able to detect the last disk block very easily. Because ext4_map_blocks may just give mapping upto the next direct block.
(e.g. in case if the file is data followed by a hole - case described by Darrick above)
I see generic_block_fiemap also does not detect the last block mapping correctly and relies on i_size_read(inode) for that.

So to keep the ext4_fiemap behavior same when
moving to iomap framework, we can make the above changes where in
_EXTENT_LAST will be mainly set by FS for extent based mapping and for non-extent it will let the iomap current code determine last mapped extent/block.

But - as per Jan in above comment, if the caller anyways have to always
determine that whether it is the last extent or not, then I am not sure
whether we should add this extra complexity or not, but I guess
this should be mainly so that the current behavior of ext4_fiemap should not change while moving to iomap. Your thoughts pls?

Raw version of ext4_iomap_begin_report implementation to determine last extent, which I am currently testing based on above discussed idea... will post the full version soon. But wanted to
get some initial feedback on this one.

fs/ext4/inode.c +0 -0

index 0f8a196d8a61..85c755f2989b 100644

fs/ext4/inode.c +3 -0

+       return true;
+}
+
  static int ext4_iomap_begin_report(struct inode *inode, loff_t offset,
                                    loff_t length, unsigned int flags,

                                   struct iomap *iomap, struct iomap
*srcmap)

fs/ext4/inode.c +20 -0

@@ -3548,16 +3567,36 @@ static int ext4_iomap_begin_report(struct inode 
 *inode, loff_t offset,
         map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits,
                           EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1;
 
+
+       if ((flags & IOMAP_FIEMAP) &&
+           !ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
+               if (offset >= i_size_read(inode)) {
+                       map.m_flags = 0;
+                       map.m_pblk = 0;
+                       goto set_iomap;
+               }
+       }
+
         ret = ext4_map_blocks(NULL, inode, &map, 0);
 
         if (ret < 0)
                 return ret;
         if (ret == 0)
                 delalloc = ext4_iomap_is_delalloc(inode, &map);
 
+set_iomap:
         ext4_set_iomap(inode, iomap, &map, offset, length);
         if (delalloc && iomap->type == IOMAP_HOLE)
                 iomap->type = IOMAP_DELALLOC;
 
+       if (flags & IOMAP_FIEMAP &&
+           ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
+               ext4_lblk_t lblk = map.m_lblk + map.m_len;
+               if (ext4_is_last_extent(inode, lblk)) {
+                       iomap->flags |= IOMAP_F_EXTENT_LAST;
+               } else {
+                       iomap->flags |= IOMAP_F_EXTENT_NOT_LAST;
+               }
+       }
         return 0;

 }

-ritesh

Quoted 5 lines · Ritesh Harjani

So I was making some changes along the above lines and I think we can take below approach for filesystem which could determine the _EXTENT_LAST relatively easily and for cases if it cannot
as Jan also mentioned we could keep the current behavior as is and let iomap core decide the last disk extent.

Well, given that _EXTENT_LAST never worked properly on any file system since it was added this actually changes behavior and could break existing users. I'd rather update the documentation to match reality rather than writing a lot of code for a feature no one obviously cared about for years.

Quoted 7 lines · Christoph Hellwig

Quoted 5 lines · Ritesh Harjani

So I was making some changes along the above lines and I think we can take below approach for filesystem which could determine the _EXTENT_LAST relatively easily and for cases if it cannot
as Jan also mentioned we could keep the current behavior as is and let iomap core decide the last disk extent.

Quoted 5 lines

Well, given that _EXTENT_LAST never worked properly on any file system since it was added this actually changes behavior and could break existing users. I'd rather update the documentation to match reality rather than writing a lot of code for a feature no one obviously cared about for years.

Well I agree to this. Since either ways the _EXTENT_LAST has never worked properly or in the same manner across different filesystems. In ext4 itself it works differently for extent v/s non-extent based FS. So updating the documentation would be a right way to go from here.

Ted/Jan - do you agree here:-
Shall we move ahead with this patch series in converting ext4_fiemap to use iomap APIs, without worrying about how _EXTENT_LAST is being set via iomap core code?