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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2026/7/30 10:29, Boris Burkov 写道:
> 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.
> 
> A fixup work item queued by the final writeback pass can still be in flight
> 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).
> 
> 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.
> 
> 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:
> 
>    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]
>    ...
> 
> Fixes: 4be9c7da6860 ("btrfs: trigger cow fixup via dirty_folio()")
> Assisted-by: LLM (reproduction, analysis)
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>   fs/btrfs/disk-io.c | 21 +++++++++++++++++++++
>   fs/btrfs/inode.c   | 15 +++++++++++++++
>   2 files changed, 36 insertions(+)
> 
> 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);
>   
> +	/*
> +	 * 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 is
> +	 * in btrfs_queue_writepage_fixup() and might finish queueing some final
> +	 * work, racing the btrfs_fs_closing() check there.
> +	 */
> +	flush_workqueue(fs_info->fixup_workers);
> +
>   	/*
>   	 * After we entered close_ctree() autodefrag could be running and before
>   	 * 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);
>   
> +	/*
> +	 * Reclaim workers can run writeback which can queue fixup.
> +	 * After the above cancel_work_sync() calls, any such queueing attempts are
> +	 * guaranteed to see btrfs_fs_closing(), so at this point we can genuinely 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..51d460572317 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2964,6 +2964,21 @@ void btrfs_queue_writepage_fixup(struct btrfs_inode *inode, struct folio *folio)
>   	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>   	struct btrfs_writepage_fixup *fixup;
>   
> +	/*
> +	 * 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)) {

Shouldn't we also clear the fixup and dirty bitmaps for this case?

As the folio is still dirty, and will never really go through writeback 
until fixup work is done.

Thus canceling the corresponding dirty/fixup bits looks more reasonable.

Thanks,
Qu

> +		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));
> +		return;
> +	}
> +
>   	fixup = kzalloc_obj(*fixup, GFP_NOFS);
>   	if (!fixup)
>   		return;