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

[Jan Kara]

On Mon 12-10-26 14:04:37, Tal Zussman wrote:
> ext4_end_bio() ends writeback directly from bio completion, which
> usually runs in interrupt context, unless the io_end has unwritten
> extents to convert and is completed from ext4's workqueue.
> folio_end_dropbehind() only invalidates the folios of an RWF_DONTCACHE
> write when it runs in task context, and silently skips them otherwise.
> So on ext4, dontcache writes that overwrite blocks that are already
> written, or that allocate blocks without unwritten extents, leave their
> folios in the page cache. Dropping them was best effort so far, as there
> was no way to get task context from bio completion.
> 
> Use the task-context bio completion infrastructure instead: set
> BIO_COMPLETE_IN_TASK on bios that carry dropbehind folios in
> io_submit_add_bh(), so that bio_endio() runs their completion from a
> worker. ext4's iomap buffered I/O path already gets this from iomap, so
> only the buffer_head based writeback is affected. Skip the flag when the
> io_end already has unwritten extents, as ext4 then defers the completion
> itself. A later extent can still make the io_end unwritten after an
> earlier bio was built. That bio's completion is then deferred twice,
> which is not optimal, but harmless.
> 
> This changes behavior only for bios that carry dropbehind folios: their
> completion now runs from a worker instead of in interrupt context.
> 
> On the unpatched kernel a dontcache overwrite of already-written blocks,
> or a dontcache write into a new file on a dioread_lock mount, leaves
> every folio in the page cache (16384/16384 for a 64M file). With the
> patch, none remain in the page cache. A dontcache write into a new file
> with unwritten extents already drops its folios on both kernels, as
> expected, and a plain write keeps them.
> 
> Signed-off-by: Tal Zussman <tz2294@xxxxxxxxxxxx>

I guess this is OK as a short term fix but having two different offloading
mechanisms for IO completion is just asking for trouble and we should use 
BIO_COMPLETE_IN_TASK when unwritten extent conversion is needed as well.
But for this fix feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/ext4/page-io.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 734d856a6d47..e556598ce9e1 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);
> +
>  	wbc_account_cgroup_owner(io->io_wbc, folio, bh->b_size);
>  	io->io_next_block++;
>  }
> 
> ---
> base-commit: 35aefefd7aba0801bab635b11943f932626be3b2
> change-id: 20260924-ext4-dontcache-complete-in-task-2340ed5f3114
> 
> Best regards,
> --  
> Tal Zussman <tz2294@xxxxxxxxxxxx>
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR