[RFC,v3,00/26] ext4: use iomap for regular file's buffered IO path and enable large foilo

  From: Zhang Yi <yi.zhang@huawei.com>

Hello,

This is the third version of RFC patch series that convert ext4 regular
file's buffered IO path to iomap and enable large folio. It's rebased on
6.7 and Christoph's "map multiple blocks per ->map_blocks in iomap
writeback" series [1]. I've fixed all issues found in the last about 3
weeks of stress tests and fault injection tests in v2. I hope I've
covered most of the corner cases, and any comments are welcome. :)

Changes since v2:
 - Update patch 1-6 to v3 [2].
 - iomap_zero and iomap_unshare don't need to update i_size and call
   iomap_write_failed(), introduce a new helper iomap_write_end_simple()
   to avoid doing that.
 - Factor out ext4_[ext|ind]_map_blocks() parts from ext4_map_blocks(),
   introduce a new helper ext4_iomap_map_one_extent() to allocate
   delalloc blocks in writeback, which is always under i_data_sem in
   write mode. This is done to prevent the writing back delalloc
   extents become stale if it raced by truncate.
 - Add a lock detection in mapping_clear_large_folios().
Changes since v1:
 - Introduce seq count for iomap buffered write and writeback to protect
   races from extents changes, e.g. truncate, mwrite.
 - Always allocate unwritten extents for new blocks, drop dioread_lock
   mode, and make no distinctions between dioread_lock and
   dioread_nolock.
 - Don't add ditry data range to jinode, drop data=ordered mode, and
   make no distinctions between data=ordered and data=writeback mode.
 - Postpone updating i_disksize to endio.
 - Allow splitting extents and use reserved space in endio.
 - Instead of reimplement a new delayed mapping helper
   ext4_iomap_da_map_blocks() for buffer write, try to reuse
   ext4_da_map_blocks().
 - Add support for disabling large folio on active inodes.
 - Support online defragmentation, make file fall back to buffer_head
   and disable large folio in ext4_move_extents().
 - Move ext4_nonda_switch() in advance to prevent deadlock in mwrite.
 - Add dirty_len and pos trace info to trace_iomap_writepage_map().
 - Update patch 1-6 to v2.

This series only support ext4 with the default features and mount
options, doesn't support inline_data, bigalloc, dax, fs_verity, fs_crypt
and data=journal mode, ext4 would fall back to buffer_head path
automatically if you enabled these features/options. Although it has
many limitations now, it can satisfy the requirements of common cases
and bring a great performance benefit.

Patch 1-6: this is a preparation series, it changes ext4_map_blocks()
and ext4_set_iomap() to recognize delayed only extents, I've send it out
separately [2].

Patch 7-8: these are two minor iomap changes, the first one is don't
update i_size and don't call iomap_write_failed() in zero_range, the
second one is for debug in iomap writeback path that I've discussed whit
Christoph [3].

Patch 9-15: this is another preparation series, including some changes
for delayed extents. Firstly, it factor out buffer_head from
ext4_da_map_blocks(), make it to support adding multi-blocks once a
time. Then make unwritten to written extents conversion in endio use to
reserved space, reduce the risk of potential data loss. Finally,
introduce a sequence counter for extent status tree, which is useful
for iomap buffer write and write back.

Patch 16-22: Implement buffered IO iomap path for read, write, mmap,
zero range, truncate and writeback, replace current buffered_head path.
Please look at the following patch for details.

Patch 23-26: Convert to iomap for regular file's buffered IO path
besides inline_data, bigalloc, dax, fs_verity, fs_crypt, and
data=journal mode, and enable large folio. It should be note that
buffered iomap path hasn't support Online defrag yet, so we need fall
back to buffer_head and disable large folio automatically if user call
EXT4_IOC_MOVE_EXT.

About Tests:
 - kvm-xfstests in auto mode, and about 3 weeks of stress tests and
   fault injection tests.
 - A performance tests below.

   Fio tests with psync on my machine with Intel Xeon Gold 6240 CPU
   with 400GB system ram, 200GB ramdisk and 1TB nvme ssd disk.

   == buffer read ==

                  buffer head        iomap with large folio
   type     bs    IOPS    BW(MiB/s)  IOPS    BW(MiB/s)
   ----------------------------------------------------
   hole     4K    565k    2206       811k    3167
   hole     64K   45.1k   2820       78.1k   4879
   hole     1M    2744    2744       4890    4891
   ramdisk  4K    436k    1703       554k    2163
   ramdisk  64K   29.6k   1848       44.0k   2747
   ramdisk  1M    1994    1995       2809    2809
   nvme     4K    306k    1196       324k    1267
   nvme     64K   19.3k   1208       24.3k   1517
   nvme     1M    1694    1694       2256    2256

   == buffer write ==

                                       buffer head    ext4_iomap    
   type   Overwrite Sync Writeback bs  IOPS   BW      IOPS   BW
   -------------------------------------------------------------
   cache    N       N    N         4K   395k   1544   415k   1621
   cache    N       N    N         64K  30.8k  1928   80.1k  5005
   cache    N       N    N         1M   1963   1963   5641   5642
   cache    Y       N    N         4K   423k   1652   443k   1730
   cache    Y       N    N         64K  33.0k  2063   80.8k  5051
   cache    Y       N    N         1M   2103   2103   5588   5589
   ramdisk  N       N    Y         4K   362k   1416   307k   1198
   ramdisk  N       N    Y         64K  22.4k  1399   64.8k  4050
   ramdisk  N       N    Y         1M   1670   1670   4559   4560
   ramdisk  N       Y    N         4K   9830   38.4   13.5k  52.8
   ramdisk  N       Y    N         64K  5834   365    10.1k  629
   ramdisk  N       Y    N         1M   1011   1011   2064   2064
   ramdisk  Y       N    Y         4K   397k   1550   409k   1598
   ramdisk  Y       N    Y         64K  29.2k  1827   73.6k  4597
   ramdisk  Y       N    Y         1M   1837   1837   4985   4985
   ramdisk  Y       Y    N         4K   173k   675    182k   710
   ramdisk  Y       Y    N         64K  17.7k  1109   33.7k  2105
   ramdisk  Y       Y    N         1M   1128   1129   1790   1791
   nvme     N       N    Y         4K   298k   1164   290k   1134
   nvme     N       N    Y         64K  21.5k  1343   57.4k  3590
   nvme     N       N    Y         1M   1308   1308   3664   3664
   nvme     N       Y    N         4K   10.7k  41.8   12.0k  46.9
   nvme     N       Y    N         64K  5962   373    8598   537
   nvme     N       Y    N         1M   676    677    1417   1418
   nvme     Y       N    Y         4K   366k   1430   373k   1456
   nvme     Y       N    Y         64K  26.7k  1670   56.8k  3547
   nvme     Y       N    Y         1M   1745   1746   3586   3586
   nvme     Y       Y    N         4K   59.0k  230    61.2k  239
   nvme     Y       Y    N         64K  13.0k  813    21.0k  1311
   nvme     Y       Y    N         1M   683    683    1368   1369
 
TODO
 - Keep on doing stress tests and fixing.
 - I will rebase and resend my another patch set "ext4: more accurate
   metadata reservaion for delalloc mount option[4]" later, it's useful
   for iomap conversion. After this series, I suppose we could totally
   drop ext4_nonda_switch() and prevent the risk of data loss caused by
   extents splitting.
 - Support for more features and mount options in the future.

[1] https://lore.kernel.org/linux-fsdevel/20231207072710.176093-1-hch@lst.de/
[2] https://lore.kernel.org/linux-ext4/20240105033018.1665752-1-yi.zhang@huaweicloud.com/
[3] https://lore.kernel.org/linux-fsdevel/20231207150311.GA18830@lst.de/
[4] https://lore.kernel.org/linux-ext4/20230824092619.1327976-1-yi.zhang@huaweicloud.com/

Thanks,
Yi.

---
v2: https://lore.kernel.org/linux-ext4/20240102123918.799062-1-yi.zhang@huaweicloud.com/
v1: https://lore.kernel.org/linux-ext4/20231123125121.4064694-1-yi.zhang@huaweicloud.com/

Zhang Yi (26):
  ext4: refactor ext4_da_map_blocks()
  ext4: convert to exclusive lock while inserting delalloc extents
  ext4: correct the hole length returned by ext4_map_blocks()
  ext4: add a hole extent entry in cache after punch
  ext4: make ext4_map_blocks() distinguish delalloc only extent
  ext4: make ext4_set_iomap() recognize IOMAP_DELALLOC map type
  iomap: don't increase i_size if it's not a write operation
  iomap: add pos and dirty_len into trace_iomap_writepage_map
  ext4: allow inserting delalloc extents with multi-blocks
  ext4: correct delalloc extent length
  ext4: also mark extent as delalloc if it's been unwritten
  ext4: factor out bh handles to ext4_da_get_block_prep()
  ext4: use reserved metadata blocks when splitting extent in endio
  ext4: factor out ext4_map_{create|query}_blocks()
  ext4: introduce seq counter for extent entry
  ext4: add a new iomap aops for regular file's buffered IO path
  ext4: implement buffered read iomap path
  ext4: implement buffered write iomap path
  ext4: implement writeback iomap path
  ext4: implement mmap iomap path
  ext4: implement zero_range iomap path
  ext4: writeback partial blocks before zero range
  ext4: fall back to buffer_head path for defrag
  ext4: partially enable iomap for regular file's buffered IO path
  filemap: support disable large folios on active inode
  ext4: enable large folio for regular file with iomap buffered IO path

 fs/ext4/ext4.h              |  14 +-
 fs/ext4/ext4_jbd2.c         |   6 +
 fs/ext4/ext4_jbd2.h         |   7 +
 fs/ext4/extents.c           | 149 +++---
 fs/ext4/extents_status.c    |  39 +-
 fs/ext4/extents_status.h    |   4 +-
 fs/ext4/file.c              |  19 +-
 fs/ext4/ialloc.c            |   5 +
 fs/ext4/inode.c             | 891 +++++++++++++++++++++++++++---------
 fs/ext4/move_extent.c       |  35 ++
 fs/ext4/page-io.c           | 107 +++++
 fs/ext4/super.c             |   3 +
 fs/iomap/buffered-io.c      |  30 +-
 fs/iomap/trace.h            |  43 +-
 include/linux/pagemap.h     |  14 +
 include/trace/events/ext4.h |  31 +-
 mm/readahead.c              |   6 +-
 17 files changed, 1109 insertions(+), 294 deletions(-)
  

On Sat, Jan 27, 2024 at 09:58:00AM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Refactor and cleanup ext4_da_map_blocks(), reduce some unnecessary
> parameters and branches, no logic changes.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>

Thanks, applied.  I've taken the first six patches in this series
since they are clearly bug fixes / prepartory changes for the rest of
the series.  (There were one minor patch conflicts that I fixed up.)

    	     	    	     	   - Ted
  

On Sat, Jan 27, 2024 at 09:58:03AM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> In order to cache hole extents in the extent status tree and keep the
> hole length as long as possible, re-add a hole entry to the cache just
> after punching a hole.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>

Thanks, applied.
  

On Sat, Jan 27, 2024 at 09:58:05AM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Since ext4_map_blocks() can recognize a delayed allocated only extent,
> make ext4_set_iomap() can also recognize it, and remove the useless
> separate check in ext4_iomap_begin_report().
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>

Thanks, applied.
  

On Sat, Jan 27, 2024 at 09:57:59AM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Hello,
> 
> This is the third version of RFC patch series that convert ext4 regular
> file's buffered IO path to iomap and enable large folio. It's rebased on
> 6.7 and Christoph's "map multiple blocks per ->map_blocks in iomap
> writeback" series [1]. I've fixed all issues found in the last about 3
> weeks of stress tests and fault injection tests in v2. I hope I've
> covered most of the corner cases, and any comments are welcome. :)
> 
> Changes since v2:
>  - Update patch 1-6 to v3 [2].
>  - iomap_zero and iomap_unshare don't need to update i_size and call
>    iomap_write_failed(), introduce a new helper iomap_write_end_simple()
>    to avoid doing that.
>  - Factor out ext4_[ext|ind]_map_blocks() parts from ext4_map_blocks(),
>    introduce a new helper ext4_iomap_map_one_extent() to allocate
>    delalloc blocks in writeback, which is always under i_data_sem in
>    write mode. This is done to prevent the writing back delalloc
>    extents become stale if it raced by truncate.
>  - Add a lock detection in mapping_clear_large_folios().
> Changes since v1:
>  - Introduce seq count for iomap buffered write and writeback to protect
>    races from extents changes, e.g. truncate, mwrite.
>  - Always allocate unwritten extents for new blocks, drop dioread_lock
>    mode, and make no distinctions between dioread_lock and
>    dioread_nolock.
>  - Don't add ditry data range to jinode, drop data=ordered mode, and
>    make no distinctions between data=ordered and data=writeback mode.
>  - Postpone updating i_disksize to endio.
>  - Allow splitting extents and use reserved space in endio.
>  - Instead of reimplement a new delayed mapping helper
>    ext4_iomap_da_map_blocks() for buffer write, try to reuse
>    ext4_da_map_blocks().
>  - Add support for disabling large folio on active inodes.
>  - Support online defragmentation, make file fall back to buffer_head
>    and disable large folio in ext4_move_extents().
>  - Move ext4_nonda_switch() in advance to prevent deadlock in mwrite.
>  - Add dirty_len and pos trace info to trace_iomap_writepage_map().
>  - Update patch 1-6 to v2.
> 
> This series only support ext4 with the default features and mount
> options, doesn't support inline_data, bigalloc, dax, fs_verity, fs_crypt
> and data=journal mode, ext4 would fall back to buffer_head path

Do you plan to add bigalloc or !extents support as a part 2 patchset?

An ext2 port to iomap has been (vaguely) in the works for a while,
though iirc willy never got the performance to match because iomap
didn't have a mechanism for the caller to tell it "run the IO now even
though you don't have a complete page, because the indirect block is the
next block after the 11th block".

--D

> automatically if you enabled these features/options. Although it has
> many limitations now, it can satisfy the requirements of common cases
> and bring a great performance benefit.
> 
> Patch 1-6: this is a preparation series, it changes ext4_map_blocks()
> and ext4_set_iomap() to recognize delayed only extents, I've send it out
> separately [2].
> 
> Patch 7-8: these are two minor iomap changes, the first one is don't
> update i_size and don't call iomap_write_failed() in zero_range, the
> second one is for debug in iomap writeback path that I've discussed whit
> Christoph [3].
> 
> Patch 9-15: this is another preparation series, including some changes
> for delayed extents. Firstly, it factor out buffer_head from
> ext4_da_map_blocks(), make it to support adding multi-blocks once a
> time. Then make unwritten to written extents conversion in endio use to
> reserved space, reduce the risk of potential data loss. Finally,
> introduce a sequence counter for extent status tree, which is useful
> for iomap buffer write and write back.
> 
> Patch 16-22: Implement buffered IO iomap path for read, write, mmap,
> zero range, truncate and writeback, replace current buffered_head path.
> Please look at the following patch for details.
> 
> Patch 23-26: Convert to iomap for regular file's buffered IO path
> besides inline_data, bigalloc, dax, fs_verity, fs_crypt, and
> data=journal mode, and enable large folio. It should be note that
> buffered iomap path hasn't support Online defrag yet, so we need fall
> back to buffer_head and disable large folio automatically if user call
> EXT4_IOC_MOVE_EXT.
> 
> About Tests:
>  - kvm-xfstests in auto mode, and about 3 weeks of stress tests and
>    fault injection tests.
>  - A performance tests below.
> 
>    Fio tests with psync on my machine with Intel Xeon Gold 6240 CPU
>    with 400GB system ram, 200GB ramdisk and 1TB nvme ssd disk.
> 
>    == buffer read ==
> 
>                   buffer head        iomap with large folio
>    type     bs    IOPS    BW(MiB/s)  IOPS    BW(MiB/s)
>    ----------------------------------------------------
>    hole     4K    565k    2206       811k    3167
>    hole     64K   45.1k   2820       78.1k   4879
>    hole     1M    2744    2744       4890    4891
>    ramdisk  4K    436k    1703       554k    2163
>    ramdisk  64K   29.6k   1848       44.0k   2747
>    ramdisk  1M    1994    1995       2809    2809
>    nvme     4K    306k    1196       324k    1267
>    nvme     64K   19.3k   1208       24.3k   1517
>    nvme     1M    1694    1694       2256    2256
> 
>    == buffer write ==
> 
>                                        buffer head    ext4_iomap    
>    type   Overwrite Sync Writeback bs  IOPS   BW      IOPS   BW
>    -------------------------------------------------------------
>    cache    N       N    N         4K   395k   1544   415k   1621
>    cache    N       N    N         64K  30.8k  1928   80.1k  5005
>    cache    N       N    N         1M   1963   1963   5641   5642
>    cache    Y       N    N         4K   423k   1652   443k   1730
>    cache    Y       N    N         64K  33.0k  2063   80.8k  5051
>    cache    Y       N    N         1M   2103   2103   5588   5589
>    ramdisk  N       N    Y         4K   362k   1416   307k   1198
>    ramdisk  N       N    Y         64K  22.4k  1399   64.8k  4050
>    ramdisk  N       N    Y         1M   1670   1670   4559   4560
>    ramdisk  N       Y    N         4K   9830   38.4   13.5k  52.8
>    ramdisk  N       Y    N         64K  5834   365    10.1k  629
>    ramdisk  N       Y    N         1M   1011   1011   2064   2064
>    ramdisk  Y       N    Y         4K   397k   1550   409k   1598
>    ramdisk  Y       N    Y         64K  29.2k  1827   73.6k  4597
>    ramdisk  Y       N    Y         1M   1837   1837   4985   4985
>    ramdisk  Y       Y    N         4K   173k   675    182k   710
>    ramdisk  Y       Y    N         64K  17.7k  1109   33.7k  2105
>    ramdisk  Y       Y    N         1M   1128   1129   1790   1791
>    nvme     N       N    Y         4K   298k   1164   290k   1134
>    nvme     N       N    Y         64K  21.5k  1343   57.4k  3590
>    nvme     N       N    Y         1M   1308   1308   3664   3664
>    nvme     N       Y    N         4K   10.7k  41.8   12.0k  46.9
>    nvme     N       Y    N         64K  5962   373    8598   537
>    nvme     N       Y    N         1M   676    677    1417   1418
>    nvme     Y       N    Y         4K   366k   1430   373k   1456
>    nvme     Y       N    Y         64K  26.7k  1670   56.8k  3547
>    nvme     Y       N    Y         1M   1745   1746   3586   3586
>    nvme     Y       Y    N         4K   59.0k  230    61.2k  239
>    nvme     Y       Y    N         64K  13.0k  813    21.0k  1311
>    nvme     Y       Y    N         1M   683    683    1368   1369
>  
> TODO
>  - Keep on doing stress tests and fixing.
>  - I will rebase and resend my another patch set "ext4: more accurate
>    metadata reservaion for delalloc mount option[4]" later, it's useful
>    for iomap conversion. After this series, I suppose we could totally
>    drop ext4_nonda_switch() and prevent the risk of data loss caused by
>    extents splitting.
>  - Support for more features and mount options in the future.
> 
> [1] https://lore.kernel.org/linux-fsdevel/20231207072710.176093-1-hch@lst.de/
> [2] https://lore.kernel.org/linux-ext4/20240105033018.1665752-1-yi.zhang@huaweicloud.com/
> [3] https://lore.kernel.org/linux-fsdevel/20231207150311.GA18830@lst.de/
> [4] https://lore.kernel.org/linux-ext4/20230824092619.1327976-1-yi.zhang@huaweicloud.com/
> 
> Thanks,
> Yi.
> 
> ---
> v2: https://lore.kernel.org/linux-ext4/20240102123918.799062-1-yi.zhang@huaweicloud.com/
> v1: https://lore.kernel.org/linux-ext4/20231123125121.4064694-1-yi.zhang@huaweicloud.com/
> 
> Zhang Yi (26):
>   ext4: refactor ext4_da_map_blocks()
>   ext4: convert to exclusive lock while inserting delalloc extents
>   ext4: correct the hole length returned by ext4_map_blocks()
>   ext4: add a hole extent entry in cache after punch
>   ext4: make ext4_map_blocks() distinguish delalloc only extent
>   ext4: make ext4_set_iomap() recognize IOMAP_DELALLOC map type
>   iomap: don't increase i_size if it's not a write operation
>   iomap: add pos and dirty_len into trace_iomap_writepage_map
>   ext4: allow inserting delalloc extents with multi-blocks
>   ext4: correct delalloc extent length
>   ext4: also mark extent as delalloc if it's been unwritten
>   ext4: factor out bh handles to ext4_da_get_block_prep()
>   ext4: use reserved metadata blocks when splitting extent in endio
>   ext4: factor out ext4_map_{create|query}_blocks()
>   ext4: introduce seq counter for extent entry
>   ext4: add a new iomap aops for regular file's buffered IO path
>   ext4: implement buffered read iomap path
>   ext4: implement buffered write iomap path
>   ext4: implement writeback iomap path
>   ext4: implement mmap iomap path
>   ext4: implement zero_range iomap path
>   ext4: writeback partial blocks before zero range
>   ext4: fall back to buffer_head path for defrag
>   ext4: partially enable iomap for regular file's buffered IO path
>   filemap: support disable large folios on active inode
>   ext4: enable large folio for regular file with iomap buffered IO path
> 
>  fs/ext4/ext4.h              |  14 +-
>  fs/ext4/ext4_jbd2.c         |   6 +
>  fs/ext4/ext4_jbd2.h         |   7 +
>  fs/ext4/extents.c           | 149 +++---
>  fs/ext4/extents_status.c    |  39 +-
>  fs/ext4/extents_status.h    |   4 +-
>  fs/ext4/file.c              |  19 +-
>  fs/ext4/ialloc.c            |   5 +
>  fs/ext4/inode.c             | 891 +++++++++++++++++++++++++++---------
>  fs/ext4/move_extent.c       |  35 ++
>  fs/ext4/page-io.c           | 107 +++++
>  fs/ext4/super.c             |   3 +
>  fs/iomap/buffered-io.c      |  30 +-
>  fs/iomap/trace.h            |  43 +-
>  include/linux/pagemap.h     |  14 +
>  include/trace/events/ext4.h |  31 +-
>  mm/readahead.c              |   6 +-
>  17 files changed, 1109 insertions(+), 294 deletions(-)
> 
> -- 
> 2.39.2
> 
>
  

"Darrick J. Wong" <djwong@kernel.org> writes:

> On Sat, Jan 27, 2024 at 09:57:59AM +0800, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>> 
>> Hello,
>> 
>> This is the third version of RFC patch series that convert ext4 regular
>> file's buffered IO path to iomap and enable large folio. It's rebased on
>> 6.7 and Christoph's "map multiple blocks per ->map_blocks in iomap
>> writeback" series [1]. I've fixed all issues found in the last about 3
>> weeks of stress tests and fault injection tests in v2. I hope I've
>> covered most of the corner cases, and any comments are welcome. :)
>> 
>> Changes since v2:
>>  - Update patch 1-6 to v3 [2].
>>  - iomap_zero and iomap_unshare don't need to update i_size and call
>>    iomap_write_failed(), introduce a new helper iomap_write_end_simple()
>>    to avoid doing that.
>>  - Factor out ext4_[ext|ind]_map_blocks() parts from ext4_map_blocks(),
>>    introduce a new helper ext4_iomap_map_one_extent() to allocate
>>    delalloc blocks in writeback, which is always under i_data_sem in
>>    write mode. This is done to prevent the writing back delalloc
>>    extents become stale if it raced by truncate.
>>  - Add a lock detection in mapping_clear_large_folios().
>> Changes since v1:
>>  - Introduce seq count for iomap buffered write and writeback to protect
>>    races from extents changes, e.g. truncate, mwrite.
>>  - Always allocate unwritten extents for new blocks, drop dioread_lock
>>    mode, and make no distinctions between dioread_lock and
>>    dioread_nolock.
>>  - Don't add ditry data range to jinode, drop data=ordered mode, and
>>    make no distinctions between data=ordered and data=writeback mode.
>>  - Postpone updating i_disksize to endio.
>>  - Allow splitting extents and use reserved space in endio.
>>  - Instead of reimplement a new delayed mapping helper
>>    ext4_iomap_da_map_blocks() for buffer write, try to reuse
>>    ext4_da_map_blocks().
>>  - Add support for disabling large folio on active inodes.
>>  - Support online defragmentation, make file fall back to buffer_head
>>    and disable large folio in ext4_move_extents().
>>  - Move ext4_nonda_switch() in advance to prevent deadlock in mwrite.
>>  - Add dirty_len and pos trace info to trace_iomap_writepage_map().
>>  - Update patch 1-6 to v2.
>> 
>> This series only support ext4 with the default features and mount
>> options, doesn't support inline_data, bigalloc, dax, fs_verity, fs_crypt
>> and data=journal mode, ext4 would fall back to buffer_head path
>
> Do you plan to add bigalloc or !extents support as a part 2 patchset?
>

Hi Darrick,

> An ext2 port to iomap has been (vaguely) in the works for a while,

yes, we have [1][2]. I am in the process of rebasing that work on the latest
upstream. It's been a while since my last post since I have been pulled
into some other internal work, sorry about that.

> though iirc willy never got the performance to match because iomap

Ohh, can you help me provide details on what performance benchmark was
run? I can try and run them when I rebase.

> didn't have a mechanism for the caller to tell it "run the IO now even
> though you don't have a complete page, because the indirect block is the
> next block after the 11th block".

Do you mean this for a large folio? I still didn't get the problem you
are referring here. Can you please help me explain why could that be a
problem?

[1]: https://lore.kernel.org/linux-ext4/9cdd449fc1d63cf2dba17cfa2fa7fb29b8f96a46.1700506526.git.ritesh.list@gmail.com/
[2]: https://lore.kernel.org/linux-ext4/8734wnj53k.fsf@doe.com/

-ritesh
  

On Mon, Feb 12, 2024 at 02:46:10PM +0530, Ritesh Harjani wrote:
> "Darrick J. Wong" <djwong@kernel.org> writes:
> > though iirc willy never got the performance to match because iomap
> 
> Ohh, can you help me provide details on what performance benchmark was
> run? I can try and run them when I rebase.

I didn't run a benchmark, we just knew what would happen (on rotating
storage anyway).

> > didn't have a mechanism for the caller to tell it "run the IO now even
> > though you don't have a complete page, because the indirect block is the
> > next block after the 11th block".
> 
> Do you mean this for a large folio? I still didn't get the problem you
> are referring here. Can you please help me explain why could that be a
> problem?

A classic ext2 filesystem lays out a 16kB file like this (with 512
byte blocks):

file offset	disk block
0-6KiB		1000-1011
6KiB-16KiB	1013-1032

What's in block 1012?  The indirect block!  The block which tells ext2
that blocks 12-31 of the file are in disk blocks 1013-1032.  So we can't
issue the read for them until we've finished the read for block 1012.

Buffer heads have a solution for this, BH_Boundary.  ext2 sets it for
block 11 which prompts mpage.c to submit the read immediately (see
the various calls to buffer_boundary()).  Then ext2 will submit the read
for block 1012 and the two reads will be coalesced by the IO scheduler.
So we still end up doing two reads instead of one, but that's
unavoidable because fragmentation might have meant that 6KiB-16KiB were
not stored at 1013-1032.

There's no equivalent iomap solution.  What needs to happen is:

 - iomap_folio_state->read_bytes_pending needs to be initialised to
   folio_size(), not 0.
 - Remove "ifs->read_bytes_pending += plen" from iomap_readpage_iter()
 - Subtract plen in the iomap_block_needs_zeroing() case
 - Submit a bio at the end of each iomap_readpage_iter() call

Now iomap will behave the same way as mpage, only without needing a
flag to do it (instead it will assume that the filesystem coalesces
adjacent ranges, which it should do anyway for good performance).
  

On 2024/2/12 14:18, Darrick J. Wong wrote:
> On Sat, Jan 27, 2024 at 09:57:59AM +0800, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> Hello,
>>
>> This is the third version of RFC patch series that convert ext4 regular
>> file's buffered IO path to iomap and enable large folio. It's rebased on
>> 6.7 and Christoph's "map multiple blocks per ->map_blocks in iomap
>> writeback" series [1]. I've fixed all issues found in the last about 3
>> weeks of stress tests and fault injection tests in v2. I hope I've
>> covered most of the corner cases, and any comments are welcome. :)
>>
>> Changes since v2:
>>  - Update patch 1-6 to v3 [2].
>>  - iomap_zero and iomap_unshare don't need to update i_size and call
>>    iomap_write_failed(), introduce a new helper iomap_write_end_simple()
>>    to avoid doing that.
>>  - Factor out ext4_[ext|ind]_map_blocks() parts from ext4_map_blocks(),
>>    introduce a new helper ext4_iomap_map_one_extent() to allocate
>>    delalloc blocks in writeback, which is always under i_data_sem in
>>    write mode. This is done to prevent the writing back delalloc
>>    extents become stale if it raced by truncate.
>>  - Add a lock detection in mapping_clear_large_folios().
>> Changes since v1:
>>  - Introduce seq count for iomap buffered write and writeback to protect
>>    races from extents changes, e.g. truncate, mwrite.
>>  - Always allocate unwritten extents for new blocks, drop dioread_lock
>>    mode, and make no distinctions between dioread_lock and
>>    dioread_nolock.
>>  - Don't add ditry data range to jinode, drop data=ordered mode, and
>>    make no distinctions between data=ordered and data=writeback mode.
>>  - Postpone updating i_disksize to endio.
>>  - Allow splitting extents and use reserved space in endio.
>>  - Instead of reimplement a new delayed mapping helper
>>    ext4_iomap_da_map_blocks() for buffer write, try to reuse
>>    ext4_da_map_blocks().
>>  - Add support for disabling large folio on active inodes.
>>  - Support online defragmentation, make file fall back to buffer_head
>>    and disable large folio in ext4_move_extents().
>>  - Move ext4_nonda_switch() in advance to prevent deadlock in mwrite.
>>  - Add dirty_len and pos trace info to trace_iomap_writepage_map().
>>  - Update patch 1-6 to v2.
>>
>> This series only support ext4 with the default features and mount
>> options, doesn't support inline_data, bigalloc, dax, fs_verity, fs_crypt
>> and data=journal mode, ext4 would fall back to buffer_head path
> 
> Do you plan to add bigalloc or !extents support as a part 2 patchset?

Hello,

Sorry for the late reply since I was on the vacation of Chinese New Year.
I've been working on bigalloc support recently and it's going relatively
well, but have no plans to support !extents yet, I would start looking
into it after I finish rebasing my another patch set "ext4: more
accurate metadata reservaion for delalloc mount option" mentioned in my
TODO list.

> 
> An ext2 port to iomap has been (vaguely) in the works for a while,
> though iirc willy never got the performance to match because iomap
> didn't have a mechanism for the caller to tell it "run the IO now even
> though you don't have a complete page, because the indirect block is the
> next block after the 11th block".
> 

Thanks for pointing this out and the explanation given by Matthew. IIUC,
this problem also affects ext4 in !extents mode, but not affects bigalloc,
right?

Thanks,
Yi.
  

On Fri, Mar 01, 2024 at 10:19:30AM +1100, Dave Chinner wrote:
> On Thu, Feb 29, 2024 at 04:59:34PM +0800, Zhang Yi wrote:
> > Hello, Dave!
> > 
> > On 2024/2/29 6:25, Dave Chinner wrote:
> > > On Wed, Feb 28, 2024 at 04:53:32PM +0800, Zhang Yi wrote:
> > >> On 2024/2/13 13:46, Christoph Hellwig wrote:
> > >>> Wouldn't it make more sense to just move the size manipulation to the
> > >>> write-only code?  An untested version of that is below.  With this
> > >>> the naming of the status variable becomes even more confusing than
> > >>> it already is, maybe we need to do a cleanup of the *_write_end
> > >>> calling conventions as it always returns the passed in copied value
> > >>> or 0.
> > >>>
> > >>> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > >>> index 3dab060aed6d7b..8401a9ca702fc0 100644
> > >>> --- a/fs/iomap/buffered-io.c
> > >>> +++ b/fs/iomap/buffered-io.c
> > >>> @@ -876,34 +876,13 @@ static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
> > >>>  		size_t copied, struct folio *folio)
> > >>>  {
> > >>>  	const struct iomap *srcmap = iomap_iter_srcmap(iter);
> > >>> -	loff_t old_size = iter->inode->i_size;
> > >>> -	size_t ret;
> > >>> -
> > >>> -	if (srcmap->type == IOMAP_INLINE) {
> > >>> -		ret = iomap_write_end_inline(iter, folio, pos, copied);
> > >>> -	} else if (srcmap->flags & IOMAP_F_BUFFER_HEAD) {
> > >>> -		ret = block_write_end(NULL, iter->inode->i_mapping, pos, len,
> > >>> -				copied, &folio->page, NULL);
> > >>> -	} else {
> > >>> -		ret = __iomap_write_end(iter->inode, pos, len, copied, folio);
> > >>> -	}
> > >>> -
> > >>> -	/*
> > >>> -	 * Update the in-memory inode size after copying the data into the page
> > >>> -	 * cache.  It's up to the file system to write the updated size to disk,
> > >>> -	 * preferably after I/O completion so that no stale data is exposed.
> > >>> -	 */
> > >>> -	if (pos + ret > old_size) {
> > >>> -		i_size_write(iter->inode, pos + ret);
> > >>> -		iter->iomap.flags |= IOMAP_F_SIZE_CHANGED;
> > >>> -	}
> > >>
> > >> I've recently discovered that if we don't increase i_size in
> > >> iomap_zero_iter(), it would break fstests generic/476 on xfs. xfs
> > >> depends on iomap_zero_iter() to increase i_size in some cases.
> > >>
> > >>  generic/476 75s ... _check_xfs_filesystem: filesystem on /dev/pmem2 is inconsistent (r)
> > >>  (see /home/zhangyi/xfstests-dev/results//xfs/generic/476.full for details)
> > >>
> > >>  _check_xfs_filesystem: filesystem on /dev/pmem2 is inconsistent (r)
> > >>  *** xfs_repair -n output ***
> > >>  Phase 1 - find and verify superblock...
> > >>  Phase 2 - using internal log
> > >>          - zero log...
> > >>          - scan filesystem freespace and inode maps...
> > >>  sb_fdblocks 10916, counted 10923
> > >>          - found root inode chunk
> > >>  ...
> > >>
> > >> After debugging and analysis, I found the root cause of the problem is
> > >> related to the pre-allocations of xfs. xfs pre-allocates some blocks to
> > >> reduce fragmentation during buffer append writing, then if we write new
> > >> data or do file copy(reflink) after the end of the pre-allocating range,
> > >> xfs would zero-out and write back the pre-allocate space(e.g.
> > >> xfs_file_write_checks() -> xfs_zero_range()), so we have to update
> > >> i_size before writing back in iomap_zero_iter(), otherwise, it will
> > >> result in stale delayed extent.
> > > 
> > > Ok, so this is long because the example is lacking in clear details
> > > so to try to understand it I've laid it out in detail to make sure
> > > I've understood it correctly.
> > > 
> > 
> > Thanks for the graph, the added detail makes things clear and easy to
> > understand. To be honest, it's not exactly the same as the results I
> > captured and described (the position A\B\C\D\E\F I described is
> > increased one by one), but the root cause of the problem is the same,
> > so it doesn't affect our understanding of the problem.
> 
> OK, good :)
> 
> .....
> 
> > > However, if this did actually write zeroes to disk, this would end
> > > up with:
> > > 
> > > 	A          C           B     E       F      D
> > > 	+wwwwwwwwww+DDDDDDDDDDD+zzzzz+rrrrrrr+dddddd+
> > > 	                      EOF   EOF
> > >                       (in memory)   (on disk)
> > > 
> > > Which is wrong - the file extension and zeros should not get exposed
> > > to the user until the entire reflink completes. This would expose
> > > zeros at the EOF and a file size that the user never asked for after
> > > a crash. Experience tells me that they would report this as
> > > "filesystem corrupting data on crash".
> > > 
> > > If we move where i_size gets updated by iomap_zero_iter(), we get:
> > > 
> > > 	A          C           B     E       F      D
> > > 	+wwwwwwwwww+DDDDDDDDDDD+zzzzz+rrrrrrr+dddddd+
> > > 	                            EOF
> > >                                 (in memory)
> > > 		                 (on disk)
> > > 
> > > Which is also wrong, because now the user can see the size change
> > > and read zeros in the middle of the clone operation, which is also
> > > wrong.
> > > 
> > > IOWs, we do not want to move the in-memory or on-disk EOF as a
> > > result of zeroing delalloc extents beyond EOF as it opens up
> > > transient, non-atomic on-disk states in the event of a crash.
> > > 
> > > So, catch-22: we need to move the in-memory EOF to write back zeroes
> > > beyond EOF, but that would move the on-disk EOF to E before the
> > > clone operation starts. i.e. it makes clone non-atomic.
> > 
> > Make sense. IIUC, I also notice that xfs_file_write_checks() zero
> > out EOF blocks if the later write offset is beyond the size of the
> > file.  Think about if we replace the reflink operation to a buffer
> > write E to F, although it doesn't call xfs_flush_unmap_range()
> > directly, but if it could be raced by another background write
> > back, and trigger the same problem (I've not try to reproduce it,
> > so please correct me if I understand wrong).
> 
> Correct, but the write is about to extend the file size when it
> writes into the cache beyond the zeroed region. There is no cache
> invalidate possible in this path, so the write of data moves the
> in-memory EOF past the zeroes in cache and everything works just
> fine.
> 
> If it races with concurrent background writeback, the writeback will
> skip the zeroed range beyond EOF until they are exposed by the first
> data write beyond the zeroed post-eof region which moves the
> in-memory EOF.
> 
> truncate(to a larger size) also does this same zeroing - the page
> cache is zeroed before we move the EOF in memory, and so the
> writeback will only occur once the in-memory EOF is moved. i.e. it
> effectively does:
> 
> 	xfs_zero_range(oldsize to newsize)
> 	truncate_setsize(newsize)
> 	filemap_write_and_wait_range(old size to new size)
> 
> > > What should acutally result from the iomap_zero_range() call from
> > > xfs_reflink_remap_prep() is a state like this:
> > > 
> > > 	A          C           B     E       F      D
> > > 	+wwwwwwwwww+DDDDDDDDDDD+uuuuu+rrrrrrr+dddddd+
> > > 	          EOF         EOF
> > >                (on disk)  (in memory)
> > > 
> > > where 'u' are unwritten extent blocks.
> > > 
> > 
> > Yeah, this is a good solution.
> > 
> > In xfs_file_write_checks(), I don't fully understand why we need
> > the xfs_zero_range().
> 
> The EOF block may only be partially written. Hence on extension, we
> have to guarantee the part of that block beyond the current EOF is
> zero if the write leaves a hole between the current EOF and the
> start of the new extending write.
> 
> > Theoretically, iomap have already handled
> > partial block zeroing for both buffered IO and DIO, so I guess
> > the only reason we still need it is to handle pre-allocated blocks
> > (no?).
> 
> Historically speaking, Linux is able to leak data beyond EOF on
> writeback of partial EOF blocks (e.g. mmap() can write to the EOF
> page beyond EOF without failing. We try to mitigate these cases
> where we can, but we have to consider that at any time the data in
> the cache beyond EOF can be non-zero thanks to mmap() and so any
> file extension *must* zero any region beyond EOF cached in the page
> cache.
> 
> > If so，would it be better to call xfs_free_eofblocks() to
> > release all the preallocated extents in range? If not, maybe we
> > could only zero out mapped partial blocks and also release
> > preallocated extents?
> 
> No, that will cause all sorts of other performance problems,
> especially for reflinked files that triggering COW
> operations...
> 
> > 
> > In xfs_reflink_remap_prep(), I read the commit 410fdc72b05a ("xfs:
> > zero posteof blocks when cloning above eof"), xfs used to release
> > preallocations, the change log said it didn't work because of the
> > PREALLOC flag, but the 'force' parameter is 'true' when calling
> > xfs_can_free_eofblocks(), so I don't get the problem met. Could we
> > fall back to use xfs_free_eofblocks() and make a state like this?
> > 
> >  	A          C           B     E       F      D
> >  	+wwwwwwwwww+DDDDDDDDDDD+hhhhh+rrrrrrr+dddddd+
> >  	          EOF         EOF
> >                 (on disk)  (in memory)
> 
> It could, but that then requires every place that may call
> xfs_zero_range() to be aware of this need to trim EOF blocks to do
> the right thing in all cases. We don't want to remove speculative
> delalloc in the write() path nor in the truncate(up) case, and so it
> doesn't fix the general problem of zeroing specualtive delalloc
> beyond EOF requiring writeback to push page caceh pages to disk
> before the inode size has been updated.
> 
> The general solution is to have zeroing of speculative prealloc
> extents beyond EOF simply convert the range to unwritten and then
> invalidate any cached pages over that range. At this point, we are
> guaranteed to have zeroes across that range, all without needing to
> do any IO at all...

That (separate iomap ops that do the delalloc -> unwritten allocation)
seems a lot more straightforward to me than whacking preallocations.

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>