Re: [PATCH] ext4: complete dropbehind writeback in task context

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: data race on io_end->flag corrupts UNWRITTEN and FAILED state
--

commit 578221fefbc4e5e8f3b366b7ce8ee33f43bb2e34
Author: Tal Zussman <tz2294@columbia.edu>

ext4: complete dropbehind writeback in task context

This commit introduces task-context bio completion for bios carrying
dropbehind folios. It sets BIO_COMPLETE_IN_TASK on these bios during
io_submit_add_bh() to ensure their invalidation runs from a worker rather
than interrupt context.

> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 734d856a6d478..e556598ce9e14 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c

[ ... ]

> @@ -458,6 +458,16 @@ static void io_submit_add_bh(struct ext4_io_submit *io,
>  		io_submit_init_bio(io, inode, folio, bh);
>  	if (!bio_add_folio(io->io_bio, folio, bh->b_size, bh_offset(bh)))
>  		goto submit_and_retry;
> +
> +	/*
> +	 * Dropbehind folios can only be invalidated in task context. An
> +	 * io_end with unwritten extents is completed from ext4's workqueue
> +	 * anyway, so only ask the block layer to defer the others.
> +	 */
> +	if (folio_test_dropbehind(folio) &&
> +	    !(io->io_end->flag & EXT4_IO_END_UNWRITTEN))
> +		bio_set_flag(io->io_bio, BIO_COMPLETE_IN_TASK);
> +

[Severity: High]
Does this lockless read of io_end->flag introduce a data race that could lead
to data loss?

During submission, the ext4_writepages thread can map multiple extents that
share a single ext4_io_end. If an early bio completes with an error while the
submission thread is still mapping a subsequent unwritten extent,
ext4_end_bio() can set EXT4_IO_END_FAILED concurrently:

fs/ext4/page-io.c:ext4_end_bio() {
    ...
    io_end->flag |= EXT4_IO_END_FAILED;
    ...
}

Meanwhile, the submission thread might be setting EXT4_IO_END_UNWRITTEN:

fs/ext4/ext4.h:ext4_set_io_unwritten_flag() {
    ...
    io_end->flag |= EXT4_IO_END_UNWRITTEN;
    ...
}

Since both paths use non-atomic bitwise OR without a shared lock, reading
the flag here in io_submit_add_bh() appears to rely on a racy state.

If the UNWRITTEN flag is lost due to a read-modify-write race,
ext4_io_end_defer_completion() will return false and Ext4 could drop the
io_end without converting the unwritten extents on disk. Can this lead to
silent data loss where written data reads as zeroes?

Could this lockless check be made safe against concurrent bio completions, or
perhaps atomic bitops should be used for io_end->flag?

>  	wbc_account_cgroup_owner(io->io_wbc, folio, bh->b_size);
>  	io->io_next_block++;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-ext4-dontcache-complete-in-task-v1-1-6ac801557041@columbia.edu?part=1