Re: [PATCH] btrfs: zstd: keep the last max level workspace out of reclaim

[old king]

Thanks Sashiko for the report and Qu for the reply.

I will confirm and test this report.

FAN YE

On Sun, Aug 23, 2026 at 7:10 AM Qu Wenruo <wqu@xxxxxxxx> wrote:
>
>
>
> 在 2026/8/22 22:15, FAN YE via B4 Relay 写道:
> > From: FAN YE <fy15309206903@xxxxxxxxx>
> >
> > zstd_put_workspace() makes the "hide this workspace from the reclaim timer"
> > decision only when the workspace is returned at its own level.
> > Decompression always asks for level 0, so a max level workspace borrowed by
> > a read skips the whole block and the test for being the last max level
> > workspace is never made: it goes back to idle_ws[] still linked on the lru.
> > A read borrowing one while a write holds the other is enough to leave every
> > max level workspace on the lru, where the reclaim timer can then free them
> > all and clear the level bit.
> >
> > Once no max level workspace is left, zstd_put_workspace() never reaches
> > cond_wake_up() and a task sleeping in zstd_get_workspace() after a failed
> > allocation has no possible waker.  Make the decision on every put of a max
> > level workspace and unlink it from the lru when it is the last one;
> > list_del_init() in zstd_find_workspace() keeps the entry usable for that.
> > The test also no longer hides workspaces of other levels, which it did
> > whenever no max level workspace happened to be idle.
> >
> > Fixes: 3f93aef535c8 ("btrfs: add zstd compression level support")
> > Assisted-by: Claude:claude-opus-5
> > Signed-off-by: FAN YE <fy15309206903@xxxxxxxxx>
>
> Sashiko found a high level problem:
>
> https://sashiko.dev/#/patchset/20260822-btrfs-zstd-max-level-reclaim-v1-1-0eb13c127480%40gmail.com
> > ---
> > Reproduced under QEMU/TCG.  Both arms are the same kernel with the reclaim
> > interval shortened to 100ms and a module param picking the old or the new
> > zstd_put_workspace(); the workload is compress-force=zstd:15, three rounds
> > of four concurrent writers followed by drop_caches, four readers and two
> > writers.  "unprotected" counts puts of a max level workspace after which
> > nothing left in idle_ws[] is off the lru; "borrowed" counts a max level
> > workspace taken and returned by a lower level request, the path this patch
> > changes.
> >
> >                      borrowed   unprotected   timer cleared the level bit
> >    current code       245/339       118/216                          1/0
> >    this patch         300/379           0/0                          0/0
> >
> > borrowed is of the same order in both arms, so the zeroes are not "the code
> > was never reached".  The last column needs the reclaim timer to tick inside
> > the window, so it is a coincidence rather than the criterion.
> >
> > Compile-tested (W=1, x86_64 defconfig + CONFIG_BTRFS_FS=y).  Independent of
> > and applies without my lost wakeup fix for zstd_get_workspace(),
> > 20260821-btrfs-zstd-lost-wakeup-v1-1-84f358d4ea67@xxxxxxxxx.
> > ---
> >   fs/btrfs/zstd.c | 25 ++++++++++++-------------
> >   1 file changed, 12 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
> > index 86919293fd54..8abc4e456f32 100644
> > --- a/fs/btrfs/zstd.c
> > +++ b/fs/btrfs/zstd.c
> > @@ -260,7 +260,7 @@ static struct list_head *zstd_find_workspace(struct btrfs_fs_info *fs_info, int
> >                       /* keep its place if it's a lower level using this */
> >                       workspace->req_level = level;
> >                       if (clip_level(level) == workspace->level)
> > -                             list_del(&workspace->lru_list);
> > +                             list_del_init(&workspace->lru_list);
> >                       if (list_empty(&zwsm->idle_ws[i]))
> >                               clear_bit(i, &zwsm->active_map);
> >                       spin_unlock_bh(&zwsm->lock);
> > @@ -335,18 +335,17 @@ void zstd_put_workspace(struct btrfs_fs_info *fs_info, struct list_head *ws)
> >       ASSERT(zwsm);
> >       spin_lock_bh(&zwsm->lock);
> >
> > -     /* A node is only taken off the lru if we are the corresponding level */
> > -     if (clip_level(workspace->req_level) == workspace->level) {
> > -             /* Hide a max level workspace from reclaim */
> > -             if (list_empty(&zwsm->idle_ws[ZSTD_BTRFS_MAX_LEVEL - 1])) {
> > -                     INIT_LIST_HEAD(&workspace->lru_list);
> > -             } else {
> > -                     workspace->last_used = jiffies;
> > -                     list_add(&workspace->lru_list, &zwsm->lru_list);
> > -                     if (!timer_pending(&zwsm->timer))
> > -                             mod_timer(&zwsm->timer,
> > -                                       jiffies + ZSTD_BTRFS_RECLAIM_JIFFIES);
> > -             }
> > +     /* Forward progress depends on always keeping one max level workspace */
> > +     if (workspace->level == clip_level(ZSTD_BTRFS_MAX_LEVEL) &&
> > +         list_empty(&zwsm->idle_ws[ZSTD_BTRFS_MAX_LEVEL - 1])) {
> > +             list_del_init(&workspace->lru_list);
> > +     } else if (clip_level(workspace->req_level) == workspace->level) {
> > +             /* A node is only taken off the lru if we are the corresponding level */
> > +             workspace->last_used = jiffies;
> > +             list_add(&workspace->lru_list, &zwsm->lru_list);
> > +             if (!timer_pending(&zwsm->timer))
> > +                     mod_timer(&zwsm->timer,
> > +                               jiffies + ZSTD_BTRFS_RECLAIM_JIFFIES);
> >       }
> >
> >       set_bit(workspace->level, &zwsm->active_map);
> >
> > ---
> > base-commit: 26260251022fbc2f248a3d747a9b2b961b18d2d8
> > change-id: 20260822-btrfs-zstd-max-level-reclaim-5ab59a71f83e
> >
> > Best regards,
> > --
> > FAN YE <fy15309206903@xxxxxxxxx>
> >
> >
> >
>