Re: [PATCH] btrfs: move async csum generation out of experimental features

[Qu Wenruo <wqu@suse.com>]

在 2026/9/3 22:14, Daniel Vacek 写道:
> On Thu, 3 Sept 2026 at 13:27, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>> 在 2026/9/3 18:50, Daniel Vacek 写道:
>>> On Thu, 3 Sept 2026 at 10:40, Qu Wenruo <wqu@suse.com> wrote:
>>>> Commit dd57c78aec39 ("btrfs: introduce btrfs_bio::async_csum")
>>>> introduced asynchronous data checksum generation for data writes.
>>>>
>>>> That feature can improve write performance, and was introduced in v6.19.
>>>> We have not experienced bugs related to that, so it's time to move it out
>>>> of experimental features for end users.
>>>>
>>>> Furthermore since the async checksum generation means
>>>> should_async_write() will always return false, we no longer need to
>>>> maintain the fs_info->workers workqueue, as the async checksum
>>>> generation is using the system_percpu_wq.
>>>
>>> I understand these are two related changes in one patch here. If
>>> removing async writes depends on async checksums, it would be nice to
>>> split this into a patch series.
>>>
>>> Though I guess can live with that. Looks good enough to me.
>>>
>>> Reviewed-by: Daniel Vacek <neelx@suse.com>
>>
>> Thanks a lot for the review.
>>
>>
>> I just want to mention that, Sashiko mentioned a problem that I believe
>> is valid, but not sure how realistic it will be in the real world.
>>
>>
>> Sashiko mentioned that, the old worker workqueue has WQ_MEM_RECLAIM
>> flag, thus there will always be a rescuer to ensure forward progress.
>> (Although for x86_64 and other common archs or EXPERIMENTAL builds, we
>> never utilize that worker anyway)
>>
>> But the new schedule_work() is using system_percpu_wq, which doesn't has
>> that flag.
> 
> Well, I think squashing this fixup to my patch is the right thing to do anyways:
> 
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index 0fed4e0d32d5..984d87ad79b0 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -855,7 +855,7 @@ int btrfs_csum_one_bio(struct btrfs_bio *bbio, bool async)
>       }
>       bio_inc_remaining(bio);
>       INIT_WORK(&bbio->csum_work, csum_one_bio_work);
> -    schedule_work(&bbio->csum_work);
> +    queue_work(fs_info->endio_workers, &bbio->csum_work);
>       return 0;
>   }

This has one new problem though.

The csum_one_bio_work() can queue a new work into the same endio_workers.

It happens like this:

csum_one_bio_work()
|- bio_endio()
|  |- btrfs_simple_end_io()
       |- queue_work() into endio_workers again

This can cause problems with flush_workqueue() in close_ctree(), where 
we only flush the workqueue once, which doesn't ensure newly created 
work is also flushed.

We can use drain_workqueue() instead to fix it easily though.

Mind to send out a proper patch for it?

Thanks,
Qu

> 
> 
> That should make sashiko happy as well as btrfs end_io work running on
> btrfs end_io work queues.
> 
> And we should stay on the safe side wrt memory reclaim.
> 
> --nX
> 
> 
>> And since the csum generation is needed for data writeback, if under
>> very heavy memory pressure, there may be no worker to ensure the csum
>> work can be queued, in that case, it will hang the writeback (which is
>> triggered to reclaim memory).
>>
>> If we really want to address that problem, your idea about splitting the
>> patch will make a lot of sense.
>> We will need to keep the old infrastructure first, convert the worker to
>> a regular workqueue, and call queue_work(), then fully remove unused code.
>>
>>
>> But on the other hand, I doubt how pratically it is for anyone to stall
>> system_percpu_wq.
>> It's very common utilized across the whole kernel, and if it really
>> stalls I think there are a lot of more things to bother before btrfs.
>>
>> Another thing is, if we use fs_info::workers, we will follow the
>> existing wq flags, which means no PERCPU flag.
>> And according to the existing docs for those flags, PERCPU wq has a
>> better performance due to CPU locality.
>>
>>
>> So overall I think the current version is good enough, but not perfect.
>> On the other hand, using a WQ_MEM_RECLAIM seems more "correct", but will
>> definitely introduce some extra pentalty.
>>
>> If someone else has some idea on this Sashiko review, or very familiar
>> with the WQ_MEM_RECLAIM situation, any comment will be appreciated.
>>
>> Thanks,
>> Qu
>>
>>>
>>>> Suggested-by: Daniel Vacek <neelx@suse.com>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>    fs/btrfs/bio.c       | 136 +------------------------------------------
>>>>    fs/btrfs/disk-io.c   |  20 +------
>>>>    fs/btrfs/file-item.c |   6 +-
>>>>    fs/btrfs/file-item.h |   2 +-
>>>>    fs/btrfs/fs.h        |  10 ----
>>>>    fs/btrfs/super.c     |   1 -
>>>>    6 files changed, 4 insertions(+), 171 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
>>>> index 771b7d598aee..e08006543c3d 100644
>>>> --- a/fs/btrfs/bio.c
>>>> +++ b/fs/btrfs/bio.c
>>>> @@ -569,136 +569,7 @@ static int btrfs_bio_csum(struct btrfs_bio *bbio)
>>>>    {
>>>>           if (bbio->bio.bi_opf & REQ_META)
>>>>                   return btree_csum_one_bio(bbio);
>>>> -#ifdef CONFIG_BTRFS_EXPERIMENTAL
>>>> -       return btrfs_csum_one_bio(bbio, true);
>>>> -#else
>>>> -       return btrfs_csum_one_bio(bbio, false);
>>>> -#endif
>>>> -}
>>>> -
>>>> -/*
>>>> - * Async submit bios are used to offload expensive checksumming onto the worker
>>>> - * threads.
>>>> - */
>>>> -struct async_submit_bio {
>>>> -       struct btrfs_bio *bbio;
>>>> -       struct btrfs_io_context *bioc;
>>>> -       struct btrfs_io_stripe smap;
>>>> -       int mirror_num;
>>>> -       struct btrfs_work work;
>>>> -};
>>>> -
>>>> -/*
>>>> - * In order to insert checksums into the metadata in large chunks, we wait
>>>> - * until bio submission time.   All the pages in the bio are checksummed and
>>>> - * sums are attached onto the ordered extent record.
>>>> - *
>>>> - * At IO completion time the csums attached on the ordered extent record are
>>>> - * inserted into the btree.
>>>> - */
>>>> -static void run_one_async_start(struct btrfs_work *work)
>>>> -{
>>>> -       struct async_submit_bio *async =
>>>> -               container_of(work, struct async_submit_bio, work);
>>>> -       int ret;
>>>> -
>>>> -       ret = btrfs_bio_csum(async->bbio);
>>>> -       if (ret)
>>>> -               async->bbio->bio.bi_status = errno_to_blk_status(ret);
>>>> -}
>>>> -
>>>> -/*
>>>> - * In order to insert checksums into the metadata in large chunks, we wait
>>>> - * until bio submission time.   All the pages in the bio are checksummed and
>>>> - * sums are attached onto the ordered extent record.
>>>> - *
>>>> - * At IO completion time the csums attached on the ordered extent record are
>>>> - * inserted into the tree.
>>>> - *
>>>> - * If called with @do_free == true, then it will free the work struct.
>>>> - */
>>>> -static void run_one_async_done(struct btrfs_work *work, bool do_free)
>>>> -{
>>>> -       struct async_submit_bio *async =
>>>> -               container_of(work, struct async_submit_bio, work);
>>>> -       struct bio *bio = &async->bbio->bio;
>>>> -
>>>> -       if (do_free) {
>>>> -               kfree(container_of(work, struct async_submit_bio, work));
>>>> -               return;
>>>> -       }
>>>> -
>>>> -       /* If an error occurred we just want to clean up the bio and move on. */
>>>> -       if (bio->bi_status) {
>>>> -               btrfs_bio_end_io(async->bbio, bio->bi_status);
>>>> -               return;
>>>> -       }
>>>> -
>>>> -       /*
>>>> -        * All of the bios that pass through here are from async helpers.
>>>> -        * Use REQ_BTRFS_CGROUP_PUNT to issue them from the owning cgroup's
>>>> -        * context.  This changes nothing when cgroups aren't in use.
>>>> -        */
>>>> -       bio->bi_opf |= REQ_BTRFS_CGROUP_PUNT;
>>>> -       btrfs_submit_bio(bio, async->bioc, &async->smap, async->mirror_num);
>>>> -}
>>>> -
>>>> -static bool should_async_write(struct btrfs_bio *bbio)
>>>> -{
>>>> -       struct btrfs_fs_info *fs_info = bbio->inode->root->fs_info;
>>>> -       bool auto_csum_mode = true;
>>>> -
>>>> -#ifdef CONFIG_BTRFS_EXPERIMENTAL
>>>> -       /*
>>>> -        * Write bios will calculate checksum and submit bio at the same time.
>>>> -        * Unless explicitly required don't offload serial csum calculate and bio
>>>> -        * submit into a workqueue.
>>>> -        */
>>>> -       return false;
>>>> -#endif
>>>> -
>>>> -       /* Submit synchronously if the checksum implementation is fast. */
>>>> -       if (auto_csum_mode && test_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags))
>>>> -               return false;
>>>> -
>>>> -       /*
>>>> -        * Try to defer the submission to a workqueue to parallelize the
>>>> -        * checksum calculation unless the I/O is issued synchronously.
>>>> -        */
>>>> -       if (op_is_sync(bbio->bio.bi_opf))
>>>> -               return false;
>>>> -
>>>> -       /* Zoned devices require I/O to be submitted in order. */
>>>> -       if ((bbio->bio.bi_opf & REQ_META) && btrfs_is_zoned(fs_info))
>>>> -               return false;
>>>> -
>>>> -       return true;
>>>> -}
>>>> -
>>>> -/*
>>>> - * Submit bio to an async queue.
>>>> - *
>>>> - * Return true if the work has been successfully submitted, else false.
>>>> - */
>>>> -static bool btrfs_wq_submit_bio(struct btrfs_bio *bbio,
>>>> -                               struct btrfs_io_context *bioc,
>>>> -                               struct btrfs_io_stripe *smap, int mirror_num)
>>>> -{
>>>> -       struct btrfs_fs_info *fs_info = bbio->inode->root->fs_info;
>>>> -       struct async_submit_bio *async;
>>>> -
>>>> -       async = kmalloc_obj(*async, GFP_NOFS);
>>>> -       if (!async)
>>>> -               return false;
>>>> -
>>>> -       async->bbio = bbio;
>>>> -       async->bioc = bioc;
>>>> -       async->smap = *smap;
>>>> -       async->mirror_num = mirror_num;
>>>> -
>>>> -       btrfs_init_work(&async->work, run_one_async_start, run_one_async_done);
>>>> -       btrfs_queue_work(fs_info->workers, &async->work);
>>>> -       return true;
>>>> +       return btrfs_csum_one_bio(bbio);
>>>>    }
>>>>
>>>>    static u64 btrfs_append_map_length(struct btrfs_bio *bbio, u64 map_length)
>>>> @@ -806,10 +677,6 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
>>>>                   if (!(inode->flags & BTRFS_INODE_NODATASUM) &&
>>>>                       !test_bit(BTRFS_FS_STATE_NO_DATA_CSUMS, &fs_info->fs_state) &&
>>>>                       !btrfs_is_data_reloc_root(inode->root) && !bbio->is_remap) {
>>>> -                       if (should_async_write(bbio) &&
>>>> -                           btrfs_wq_submit_bio(bbio, bioc, &smap, mirror_num))
>>>> -                               goto done;
>>>> -
>>>>                           ret = btrfs_bio_csum(bbio);
>>>>                           status = errno_to_blk_status(ret);
>>>>                           if (status)
>>>> @@ -824,7 +691,6 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
>>>>           }
>>>>
>>>>           btrfs_submit_bio(bio, bioc, &smap, mirror_num);
>>>> -done:
>>>>           return map_length == length;
>>>>
>>>>    fail:
>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>> index a1d83ad9a4c0..0046f3cb75a1 100644
>>>> --- a/fs/btrfs/disk-io.c
>>>> +++ b/fs/btrfs/disk-io.c
>>>> @@ -1776,7 +1776,6 @@ static void btrfs_stop_all_workers(struct btrfs_fs_info *fs_info)
>>>>           if (fs_info->fixup_workers)
>>>>                   destroy_workqueue(fs_info->fixup_workers);
>>>>           btrfs_destroy_workqueue(fs_info->delalloc_workers);
>>>> -       btrfs_destroy_workqueue(fs_info->workers);
>>>>           if (fs_info->endio_workers)
>>>>                   destroy_workqueue(fs_info->endio_workers);
>>>>           if (fs_info->rmw_workers)
>>>> @@ -1968,9 +1967,6 @@ static int btrfs_init_workqueues(struct btrfs_fs_info *fs_info)
>>>>           unsigned int flags = WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_UNBOUND;
>>>>           unsigned int ordered_flags = WQ_MEM_RECLAIM | WQ_FREEZABLE;
>>>>
>>>> -       fs_info->workers =
>>>> -               btrfs_alloc_workqueue(fs_info, "worker", flags, max_active, 16);
>>>> -
>>>>           fs_info->delalloc_workers =
>>>>                   btrfs_alloc_workqueue(fs_info, "delalloc",
>>>>                                         flags, max_active, 2);
>>>> @@ -2005,8 +2001,7 @@ static int btrfs_init_workqueues(struct btrfs_fs_info *fs_info)
>>>>           fs_info->discard_ctl.discard_workers =
>>>>                   alloc_ordered_workqueue("btrfs-discard", WQ_FREEZABLE);
>>>>
>>>> -       if (!(fs_info->workers &&
>>>> -             fs_info->delalloc_workers && fs_info->flush_workers &&
>>>> +       if (!(fs_info->delalloc_workers && fs_info->flush_workers &&
>>>>                 fs_info->endio_workers && fs_info->endio_meta_workers &&
>>>>                 fs_info->endio_write_workers &&
>>>>                 fs_info->endio_freespace_worker && fs_info->rmw_workers &&
>>>> @@ -4437,19 +4432,6 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
>>>>            */
>>>>           btrfs_flush_workqueue(fs_info->delalloc_workers);
>>>>
>>>> -       /*
>>>> -        * We can have ordered extents getting their last reference dropped from
>>>> -        * the fs_info->workers queue because for async writes for data bios we
>>>> -        * queue a work for that queue, at btrfs_wq_submit_bio(), that runs
>>>> -        * run_one_async_done() which calls btrfs_bio_end_io() in case the bio
>>>> -        * has an error, and that later function can do the final
>>>> -        * btrfs_put_ordered_extent() on the ordered extent attached to the bio,
>>>> -        * which adds a delayed iput for the inode. So we must flush the queue
>>>> -        * so that we don't have delayed iputs after committing the current
>>>> -        * transaction below and stopping the cleaner and transaction kthreads.
>>>> -        */
>>>> -       btrfs_flush_workqueue(fs_info->workers);
>>>> -
>>>>           /*
>>>>            * When finishing a compressed write bio we schedule a work queue item
>>>>            * to finish an ordered extent - end_bbio_compressed_write()
>>>> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
>>>> index 0fed4e0d32d5..7dbd9b8eec11 100644
>>>> --- a/fs/btrfs/file-item.c
>>>> +++ b/fs/btrfs/file-item.c
>>>> @@ -825,7 +825,7 @@ static void csum_one_bio_work(struct work_struct *work)
>>>>    /*
>>>>     * Calculate checksums of the data contained inside a bio.
>>>>     */
>>>> -int btrfs_csum_one_bio(struct btrfs_bio *bbio, bool async)
>>>> +int btrfs_csum_one_bio(struct btrfs_bio *bbio)
>>>>    {
>>>>           struct btrfs_ordered_extent *ordered = bbio->ordered;
>>>>           struct btrfs_inode *inode = bbio->inode;
>>>> @@ -849,10 +849,6 @@ int btrfs_csum_one_bio(struct btrfs_bio *bbio, bool async)
>>>>           btrfs_add_ordered_sum(ordered, sums);
>>>>
>>>>           bbio->csum_saved_iter = bio->bi_iter;
>>>> -       if (!async) {
>>>> -               csum_one_bio(bbio);
>>>> -               return 0;
>>>> -       }
>>>>           bio_inc_remaining(bio);
>>>>           INIT_WORK(&bbio->csum_work, csum_one_bio_work);
>>>>           schedule_work(&bbio->csum_work);
>>>> diff --git a/fs/btrfs/file-item.h b/fs/btrfs/file-item.h
>>>> index 6c678787c770..60a0eb17b3f6 100644
>>>> --- a/fs/btrfs/file-item.h
>>>> +++ b/fs/btrfs/file-item.h
>>>> @@ -64,7 +64,7 @@ int btrfs_lookup_file_extent(struct btrfs_trans_handle *trans,
>>>>    int btrfs_insert_data_csums(struct btrfs_trans_handle *trans,
>>>>                               struct btrfs_root *root,
>>>>                               struct btrfs_ordered_sum *sums);
>>>> -int btrfs_csum_one_bio(struct btrfs_bio *bbio, bool async);
>>>> +int btrfs_csum_one_bio(struct btrfs_bio *bbio);
>>>>    int btrfs_alloc_dummy_sum(struct btrfs_bio *bbio);
>>>>    int btrfs_lookup_csums_range(struct btrfs_root *root, u64 start, u64 end,
>>>>                                struct list_head *list, int search_commit,
>>>> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
>>>> index 3eba8438593c..203f7131f737 100644
>>>> --- a/fs/btrfs/fs.h
>>>> +++ b/fs/btrfs/fs.h
>>>> @@ -696,16 +696,6 @@ struct btrfs_fs_info {
>>>>           /* All fs/file tree roots that have delalloc inodes. */
>>>>           struct list_head delalloc_roots;
>>>>
>>>> -       /*
>>>> -        * There is a pool of worker threads for checksumming during writes and
>>>> -        * a pool for checksumming after reads.  This is because readers can
>>>> -        * run with FS locks held, and the writers may be waiting for those
>>>> -        * locks.  We don't want ordering in the pending list to cause
>>>> -        * deadlocks, and so the two are serviced separately.
>>>> -        *
>>>> -        * A third pool does submit_bio to avoid deadlocking with the other two.
>>>> -        */
>>>> -       struct btrfs_workqueue *workers;
>>>>           struct btrfs_workqueue *delalloc_workers;
>>>>           struct btrfs_workqueue *flush_workers;
>>>>           struct workqueue_struct *endio_workers;
>>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>>> index 464129b1b0d4..54f2da47c567 100644
>>>> --- a/fs/btrfs/super.c
>>>> +++ b/fs/btrfs/super.c
>>>> @@ -1237,7 +1237,6 @@ static void btrfs_resize_thread_pool(struct btrfs_fs_info *fs_info,
>>>>           btrfs_info(fs_info, "resize thread pool %d -> %d",
>>>>                  old_pool_size, new_pool_size);
>>>>
>>>> -       btrfs_workqueue_set_max(fs_info->workers, new_pool_size);
>>>>           btrfs_workqueue_set_max(fs_info->delalloc_workers, new_pool_size);
>>>>           btrfs_workqueue_set_max(fs_info->caching_workers, new_pool_size);
>>>>           workqueue_set_max_active(fs_info->endio_workers, new_pool_size);
>>>> --
>>>> 2.55.0
>>>>
>>>
>>