Re: [PATCH v2] btrfs: flush the fixup workers during close_ctree

=E5=9C=A8 2026/7/31 02:08, Boris Burkov =E5=86=99=E9=81=93:
> Reintroducing the COW fixup worker brought back the unmount race fixed
> by commit 41fd1e94066a ("btrfs: wait for fixup workers before stopping
> cleaner kthread during umount") without bringing back the fix.
>=20
> A fixup work item queued by the final writeback pass can still be in fli=
ght
> when close_ctree() stops the cleaner kthread and frees the fs roots.
> While destroy_workqueue() drains the queue, that happens after the
> cleaner thread was freed, so btrfs_add_delayed_iput() called from the
> fixup worker is no longer safe (not to mention that we are already in
> BTRFS_FS_STATE_NO_DELAYED_IPUT when it runs).
>=20
> Therefore we need to bring back explicitly flushing the fixup workqueue
> as in Filipe's original fix. The first flush will catch all the fixup
> writeback queued during the final sync before umount, but some of that
> might hit memory allocation errors and stay fixup in the blocks/folio,
> leading any subsequent writeback triggered *inside* umount (e.g. reclaim
> workers shutting down) to hit it and queue again. To fix that, and the
> possibility of any really long-lived pinned folios getting marked, deny
> queueing new fixup during umount. That allows us to flush twice (once
> before doing a real writeback pass to get the actual data, second time
> to clean up any rather unlikely stragglers right before declaring
> BTRFS_FS_STATE_NO_DELAYED_IPUT) and be certain nothing got re-queued.
>=20
> Reproduced by injecting a one-shot 30s sleep at the head of
> btrfs_writepage_fixup_worker() on a KASAN kernel, running the normal
> reproducing read dio workload before unmount and then observing:
>=20
>    BUG: KASAN: slab-use-after-free in _raw_spin_lock_irqsave+0x35/0x50
>    Read of size 1 at addr ffff88810b4b08f8 by task kworker/u32:5/219
>    Workqueue: btrfs-fixup btrfs_writepage_fixup_worker [btrfs]
>    Call Trace:
>     _raw_spin_lock_irqsave+0x35/0x50
>     try_to_wake_up+0xc0/0x18c0
>     btrfs_writepage_fixup_worker+0x7f3/0xf20 [btrfs]
>    ...
>=20
> Fixes: 4be9c7da6860 ("btrfs: trigger cow fixup via dirty_folio()")
> Assisted-by: LLM (reproduction, analysis)
> Signed-off-by: Boris Burkov <[email protected]>

Reviewed-by: Qu Wenruo <[email protected]>

Thanks,
Qu

> ---
> Changelog:
> v2:
> - clear fixup on skip, following the model of errors in the worker
>=20
>   fs/btrfs/disk-io.c | 21 +++++++++++++++++++++
>   fs/btrfs/inode.c   | 17 +++++++++++++++++
>   2 files changed, 38 insertions(+)
>=20
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 7a2f7006085d..cc4dcd10631a 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4377,6 +4377,18 @@ void __cold close_ctree(struct btrfs_fs_info *fs_=
info)
>   	/* clear out the rbtree of defraggable inodes */
>   	btrfs_cleanup_defrag_inodes(fs_info);
>  =20
> +	/*
> +	 * Before the unmount, we sync down all the writeback which can
> +	 * generate fixup work. We are about to run delalloc for autodefrag so
> +	 * piggy back on that by also flushing the fixup work which can also
> +	 * generate delalloc we would like to get run.
> +	 *
> +	 * After this, it is still possible that some thread doing writeback i=
s
> +	 * in btrfs_queue_writepage_fixup() and might finish queueing some fin=
al
> +	 * work, racing the btrfs_fs_closing() check there.
> +	 */
> +	flush_workqueue(fs_info->fixup_workers);
> +
>   	/*
>   	 * After we entered close_ctree() autodefrag could be running and bef=
ore
>   	 * we parked the cleaner kthread, it dirtied folios of some inode.
> @@ -4469,6 +4481,15 @@ void __cold close_ctree(struct btrfs_fs_info *fs_=
info)
>   	cancel_work_sync(&fs_info->preempt_reclaim_work);
>   	cancel_work_sync(&fs_info->em_shrinker_work);
>  =20
> +	/*
> +	 * Reclaim workers can run writeback which can queue fixup.
> +	 * After the above cancel_work_sync() calls, any such queueing attempt=
s are
> +	 * guaranteed to see btrfs_fs_closing(), so at this point we can genui=
nely fully
> +	 * flush the fixup workqueue. This relies on the belief that *now* no =
thread can
> +	 * still be sitting in btrfs_queue_writepage_fixup().
> +	 */
> +	flush_workqueue(fs_info->fixup_workers);
> +
>   	/*
>   	 * Run delayed iputs again because an async reclaim worker may have
>   	 * added new ones if it was flushing delalloc:
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 78143e241ca4..04efe183d57b 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2964,6 +2964,23 @@ void btrfs_queue_writepage_fixup(struct btrfs_ino=
de *inode, struct folio *folio)
>   	struct btrfs_fs_info *fs_info =3D inode->root->fs_info;
>   	struct btrfs_writepage_fixup *fixup;
>  =20
> +	/*
> +	 * Disallow queueing more fixup during unmount to break the cycle
> +	 * of writeback queuing fixup queuing writeback etc.
> +	 *
> +	 * If it actually hit, then something which was fixup wasn't written
> +	 * which we should warn about.
> +	 */
> +	if (btrfs_fs_closing(fs_info)) {
> +		btrfs_warn_rl(fs_info,
> +	"dropping unqueued fixup blocks at unmount. root %lld ino %llu folio %=
llu",
> +			      btrfs_root_id(inode->root), btrfs_ino(inode),
> +			      folio_pos(folio));
> +		btrfs_folio_clear_fixup_dirty(fs_info, folio,
> +					      folio_pos(folio), folio_size(folio));
> +		return;
> +	}
> +
>   	fixup =3D kzalloc_obj(*fixup, GFP_NOFS);
>   	if (!fixup)
>   		return;