Re: [PATCH v5 07/14] iomap: track pending read bytes more optimally

Brian Foster
2025-10-23T19:30:17Z

On Thu, Sep 25, 2025 at 05:26:02PM -0700, Joanne Koong wrote:
> Instead of incrementing read_bytes_pending for every folio range read in
> (which requires acquiring the spinlock to do so), set read_bytes_pending
> to the folio size when the first range is asynchronously read in, keep
> track of how many bytes total are asynchronously read in, and adjust
> read_bytes_pending accordingly after issuing requests to read in all the
> necessary ranges.
>
> iomap_read_folio_ctx->cur_folio_in_bio can be removed since a non-zero
> value for pending bytes necessarily indicates the folio is in the bio.
>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> Suggested-by: "Darrick J. Wong" <djwong@kernel.org>
> ---

Hi Joanne,

I was throwing some extra testing at the vfs-6.19.iomap branch since the
little merge conflict thing with iomap_iter_advance(). I end up hitting
what appears to be a lockup on XFS with 1k FSB (-bsize=1k) running
generic/051. It reproduces fairly reliably within a few iterations or so
and seems to always stall during a read for a dedupe operation:

task:fsstress state:D stack:0 pid:12094 tgid:12094 ppid:12091 task_flags:0x400140 flags:0x00080003
Call Trace:
<TASK>
__schedule+0x2fc/0x7a0
schedule+0x27/0x80
io_schedule+0x46/0x70
folio_wait_bit_common+0x12b/0x310
? __pfx_wake_page_function+0x10/0x10
? __pfx_xfs_vm_read_folio+0x10/0x10 [xfs]
filemap_read_folio+0x85/0xd0
? __pfx_xfs_vm_read_folio+0x10/0x10 [xfs]
do_read_cache_folio+0x7c/0x1b0
vfs_dedupe_file_range_compare.constprop.0+0xaf/0x2d0
__generic_remap_file_range_prep+0x276/0x2a0
generic_remap_file_range_prep+0x10/0x20
xfs_reflink_remap_prep+0x22c/0x300 [xfs]
xfs_file_remap_range+0x84/0x360 [xfs]
vfs_dedupe_file_range_one+0x1b2/0x1d0
? remap_verify_area+0x46/0x140
vfs_dedupe_file_range+0x162/0x220
do_vfs_ioctl+0x4d1/0x940
__x64_sys_ioctl+0x75/0xe0
do_syscall_64+0x84/0x800
? do_syscall_64+0xbb/0x800
? avc_has_perm_noaudit+0x6b/0xf0
? _copy_to_user+0x31/0x40
? cp_new_stat+0x130/0x170
? __do_sys_newfstat+0x44/0x70
? do_syscall_64+0xbb/0x800
? do_syscall_64+0xbb/0x800
? clear_bhb_loop+0x30/0x80
? clear_bhb_loop+0x30/0x80
entry_SYSCALL_64_after_hwframe+0x76/0x7e
RIP: 0033:0x7fe6bbd9a14d
RSP: 002b:00007ffde72cd4e0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 0000000000000068 RCX: 00007fe6bbd9a14d
RDX: 000000000a1394b0 RSI: 00000000c0189436 RDI: 0000000000000004
RBP: 00007ffde72cd530 R08: 0000000000001000 R09: 000000000a11a3fc
R10: 000000000001d6c0 R11: 0000000000000246 R12: 000000000a12cfb0
R13: 000000000a12ba10 R14: 000000000a14e610 R15: 0000000000019000
</TASK>

It wasn't immediately clear to me what the issue was so I bisected and
it landed on this patch. It kind of looks like we're failing to unlock a
folio at some point and then tripping over it later..? I can kill the
fsstress process but then the umount ultimately gets stuck tossing
pagecache [1], so the mount still ends up stuck indefinitely. Anyways,
I'll poke at it some more but I figure you might be able to make sense
of this faster than I can.

Brian

[1] umount stack trace:

task:umount state:D stack:0 pid:12216 tgid:12216 ppid:2514 task_flags:0x400100 flags:0x00080001
Call Trace:
<TASK>
__schedule+0x2fc/0x7a0
schedule+0x27/0x80
io_schedule+0x46/0x70
folio_wait_bit_common+0x12b/0x310
? __pfx_wake_page_function+0x10/0x10
truncate_inode_pages_range+0x42a/0x4d0
xfs_fs_evict_inode+0x1f/0x30 [xfs]
evict+0x112/0x290
evict_inodes+0x209/0x230
generic_shutdown_super+0x42/0x100
kill_block_super+0x1a/0x40
xfs_kill_sb+0x12/0x20 [xfs]
deactivate_locked_super+0x33/0xb0
cleanup_mnt+0xba/0x150
task_work_run+0x5c/0x90
exit_to_user_mode_loop+0x12f/0x170
do_syscall_64+0x1af/0x800
? vfs_statx+0x80/0x160
? do_statx+0x62/0xa0
? __x64_sys_statx+0xaf/0x100
? do_syscall_64+0xbb/0x800
? __x64_sys_statx+0xaf/0x100
? do_syscall_64+0xbb/0x800
? count_memcg_events+0xdd/0x1b0
? handle_mm_fault+0x220/0x340
? do_user_addr_fault+0x2c3/0x7f0
? clear_bhb_loop+0x30/0x80
? clear_bhb_loop+0x30/0x80
entry_SYSCALL_64_after_hwframe+0x76/0x7e
RIP: 0033:0x7fdd641ed5ab
RSP: 002b:00007ffd671182e8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a6
RAX: 0000000000000000 RBX: 0000559b3e2056b0 RCX: 00007fdd641ed5ab
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000559b3e205ac0
RBP: 00007ffd671183c0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000103 R11: 0000000000000246 R12: 0000559b3e2057b8
R13: 0000000000000000 R14: 0000559b3e205ac0 R15: 0000000000000000
</TASK>

> fs/iomap/buffered-io.c | 87 ++++++++++++++++++++++++++++++++----------
> 1 file changed, 66 insertions(+), 21 deletions(-)
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 09e65771a947..4e6258fdb915 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -362,7 +362,6 @@ static void iomap_read_end_io(struct bio *bio)
>
> struct iomap_read_folio_ctx {
> struct folio *cur_folio;
> - bool cur_folio_in_bio;
> void *read_ctx;
> struct readahead_control *rac;
> };
> @@ -380,19 +379,11 @@ static void iomap_bio_read_folio_range(const struct iomap_iter *iter,
> {
> struct folio *folio = ctx->cur_folio;
> const struct iomap *iomap = &iter->iomap;
> - struct iomap_folio_state *ifs = folio->private;
> size_t poff = offset_in_folio(folio, pos);
> loff_t length = iomap_length(iter);
> sector_t sector;
> struct bio *bio = ctx->read_ctx;
>
> - ctx->cur_folio_in_bio = true;
> - if (ifs) {
> - spin_lock_irq(&ifs->state_lock);
> - ifs->read_bytes_pending += plen;
> - spin_unlock_irq(&ifs->state_lock);
> - }
> -
> sector = iomap_sector(iomap, pos);
> if (!bio || bio_end_sector(bio) != sector ||
> !bio_add_folio(bio, folio, plen, poff)) {
> @@ -422,8 +413,57 @@ static void iomap_bio_read_folio_range(const struct iomap_iter *iter,
> }
> }
>
> +static void iomap_read_init(struct folio *folio)
> +{
> + struct iomap_folio_state *ifs = folio->private;
> +
> + if (ifs) {
> + size_t len = folio_size(folio);
> +
> + spin_lock_irq(&ifs->state_lock);
> + ifs->read_bytes_pending += len;
> + spin_unlock_irq(&ifs->state_lock);
> + }
> +}
> +
> +static void iomap_read_end(struct folio *folio, size_t bytes_pending)
> +{
> + struct iomap_folio_state *ifs;
> +
> + /*
> + * If there are no bytes pending, this means we are responsible for
> + * unlocking the folio here, since no IO helper has taken ownership of
> + * it.
> + */
> + if (!bytes_pending) {
> + folio_unlock(folio);
> + return;
> + }
> +
> + ifs = folio->private;
> + if (ifs) {
> + bool end_read, uptodate;
> + size_t bytes_accounted = folio_size(folio) - bytes_pending;
> +
> + spin_lock_irq(&ifs->state_lock);
> + ifs->read_bytes_pending -= bytes_accounted;
> + /*
> + * If !ifs->read_bytes_pending, this means all pending reads
> + * by the IO helper have already completed, which means we need
> + * to end the folio read here. If ifs->read_bytes_pending != 0,
> + * the IO helper will end the folio read.
> + */
> + end_read = !ifs->read_bytes_pending;
> + if (end_read)
> + uptodate = ifs_is_fully_uptodate(folio, ifs);
> + spin_unlock_irq(&ifs->state_lock);
> + if (end_read)
> + folio_end_read(folio, uptodate);
> + }
> +}
> +
> static int iomap_read_folio_iter(struct iomap_iter *iter,
> - struct iomap_read_folio_ctx *ctx)
> + struct iomap_read_folio_ctx *ctx, size_t *bytes_pending)
> {
> const struct iomap *iomap = &iter->iomap;
> loff_t pos = iter->pos;
> @@ -460,6 +500,9 @@ static int iomap_read_folio_iter(struct iomap_iter *iter,
> folio_zero_range(folio, poff, plen);
> iomap_set_range_uptodate(folio, poff, plen);
> } else {
> + if (!*bytes_pending)
> + iomap_read_init(folio);
> + *bytes_pending += plen;
> iomap_bio_read_folio_range(iter, ctx, pos, plen);
> }
>
> @@ -482,17 +525,18 @@ int iomap_read_folio(struct folio *folio, const struct iomap_ops *ops)
> struct iomap_read_folio_ctx ctx = {
> .cur_folio = folio,
> };
> + size_t bytes_pending = 0;
> int ret;
>
> trace_iomap_readpage(iter.inode, 1);
>
> while ((ret = iomap_iter(&iter, ops)) > 0)
> - iter.status = iomap_read_folio_iter(&iter, &ctx);
> + iter.status = iomap_read_folio_iter(&iter, &ctx,
> + &bytes_pending);
>
> iomap_bio_submit_read(&ctx);
>
> - if (!ctx.cur_folio_in_bio)
> - folio_unlock(folio);
> + iomap_read_end(folio, bytes_pending);
>
> /*
> * Just like mpage_readahead and block_read_full_folio, we always
> @@ -504,24 +548,23 @@ int iomap_read_folio(struct folio *folio, const struct iomap_ops *ops)
> EXPORT_SYMBOL_GPL(iomap_read_folio);
>
> static int iomap_readahead_iter(struct iomap_iter *iter,
> - struct iomap_read_folio_ctx *ctx)
> + struct iomap_read_folio_ctx *ctx, size_t *cur_bytes_pending)
> {
> int ret;
>
> while (iomap_length(iter)) {
> if (ctx->cur_folio &&
> offset_in_folio(ctx->cur_folio, iter->pos) == 0) {
> - if (!ctx->cur_folio_in_bio)
> - folio_unlock(ctx->cur_folio);
> + iomap_read_end(ctx->cur_folio, *cur_bytes_pending);
> ctx->cur_folio = NULL;
> }
> if (!ctx->cur_folio) {
> ctx->cur_folio = readahead_folio(ctx->rac);
> if (WARN_ON_ONCE(!ctx->cur_folio))
> return -EINVAL;
> - ctx->cur_folio_in_bio = false;
> + *cur_bytes_pending = 0;
> }
> - ret = iomap_read_folio_iter(iter, ctx);
> + ret = iomap_read_folio_iter(iter, ctx, cur_bytes_pending);
> if (ret)
> return ret;
> }
> @@ -554,16 +597,18 @@ void iomap_readahead(struct readahead_control *rac, const struct iomap_ops *ops)
> struct iomap_read_folio_ctx ctx = {
> .rac = rac,
> };
> + size_t cur_bytes_pending;
>
> trace_iomap_readahead(rac->mapping->host, readahead_count(rac));
>
> while (iomap_iter(&iter, ops) > 0)
> - iter.status = iomap_readahead_iter(&iter, &ctx);
> + iter.status = iomap_readahead_iter(&iter, &ctx,
> + &cur_bytes_pending);
>
> iomap_bio_submit_read(&ctx);
>
> - if (ctx.cur_folio && !ctx.cur_folio_in_bio)
> - folio_unlock(ctx.cur_folio);
> + if (ctx.cur_folio)
> + iomap_read_end(ctx.cur_folio, cur_bytes_pending);
> }
> EXPORT_SYMBOL_GPL(iomap_readahead);
>
> --
> 2.47.3
>
>