Re: [PATCH] btrfs: zstd: fix lost wakeup when waiting for a workspace

[old king]

Sorry, my two earlier replies went to you only and never reached linux-btrfs.

> This is the typical LLM behavior, if your human brain can not examine
> every possibly wrong conclusion/fix from LLM, LLM will only waste time
> of everyone.

Thanks for the review.

The LLM told me the diagram is missing the line this patch adds. In
your Thread B there is
nothing between prepare_to_wait() and schedule(), which is the unpatched
sequence. The patch puts the re-check there:

        Thread A (last ws holder)  |        Thread B (new ws user)
-----------------------------------+---------------------------------
zstd_put_workspace()               |
...                                               |
|- spin_lock_bh()                       |
|- spin_unlock_bh()                   |
|- cond_wake_up()                    |
   No one is waiting                   |
                                                  |- prepare_to_wait()
                                                  |
                                                  |  // THE CODE ADDED
BY THIS PATCH:
                                                  |- zstd_find_workspace()
                                                  |  WS IS FOUND HERE!
                                                  |
                                                  |  // schedule() is NOT called
                                                  |- finish_wait()

The interleaving you describe is the one the patch handles: if cond_wake_up()
sees an empty queue, thread A's list_add() has already completed, so the
zstd_find_workspace() after prepare_to_wait() finds that workspace.

The reverse order is covered by the barriers. prepare_to_wait() queues the
waiter and then calls set_current_state(), which is a full barrier;
cond_wake_up() -> wq_has_sleeper() has the matching smp_mb() before it reads
the queue. Either A observes the waiter and wakes it, or B observes the
workspace and does not sleep. That is the pattern wait_event() relies on.

Measured, same kernel image for both rows, the wait order selected by a
module parameter. QEMU/TCG, x86_64, 6 concurrent zstd:15 writers, failslab
restricted to the zstd_alloc_workspace() call stack via
CONFIG_FAULT_INJECTION_STACKTRACE_FILTER (probability=100, times=1,
ignore-gfp-wait=0), and an mdelay(400) widening the window between the failed
allocation and prepare_to_wait(). Two allocation failures are forced per
run, so two tasks sit in that window. Instrumentation counts a max level put
whose wakeup found an empty queue while a task was inside the window
(dropped), and a re-check after prepare_to_wait() that returned a workspace
(recovered):

                        | dropped | recovered | result
  ------------------+---------+-----------+---------------------------------
  unpatched     |   136   |       -       | hang, kworker/u16:0:12 blocked
                        |            |               | >122s in
zstd_get_workspace()
  patched         |   140   |       2      | no hang

The wakeups are dropped in both rows, in the window your diagram describes.
The difference is what happens next: in the patched row the two tasks that
would have slept find a workspace in the re-check and never reach schedule().
One of them is the pid that hangs in the unpatched row.

The LLM assistance is the one already declared in the Assisted-by tag. The
reasoning above is checked against the code and against these numbers.

On Sat, Aug 22, 2026 at 5:01 PM Qu Wenruo <wqu@xxxxxxxx> wrote:
>
>
>
> 在 2026/8/22 03:20, FAN YE via B4 Relay 写道:
> > From: FAN YE <fy15309206903@xxxxxxxxx>
> >
> > A writer can sleep forever in zstd_get_workspace() even though a workspace
> > is free.  When zstd_alloc_workspace() fails, the task is queued on
> > zwsm->wait and schedules unconditionally, never re-testing the pool.
> > zstd_put_workspace() publishes the workspace and then calls cond_wake_up(),
> > which only wakes when a sleeper is already visible, so a workspace returned
> > between the failed allocation and prepare_to_wait() wakes nobody.  The
> > window is wide: zstd_alloc_workspace() goes through kvmalloc() and may
> > enter reclaim.
> >
> > Only a max level workspace triggers the wakeup and one is deliberately kept
> > allocated as the fallback every waiter waits for, so once its wakeup is
> > lost the writer stays in TASK_UNINTERRUPTIBLE until some other task happens
> > to return one.  Re-check the pool after prepare_to_wait() has published the
> > waiter, and use the workspace if one turned up.
> >
> > Fixes: 3f93aef535c8 ("btrfs: add zstd compression level support")
> > Assisted-by: Claude:claude-opus-5
> > Signed-off-by: FAN YE <fy15309206903@xxxxxxxxx>
> > ---
> > Reproduced under QEMU/TCG: CONFIG_FAULT_INJECTION_STACKTRACE_FILTER forces
> > zstd_alloc_workspace() to fail exactly once and widens the pre-wait window
> > to 400ms while six concurrent zstd:15 writers race it. Unpatched, a
> > btrfs-delalloc kworker hangs in zstd_get_workspace()'s schedule() (hung_task
> > warning, >120s); the identical race against the patched code does not hang.
> > Compile-tested (W=1, x86_64 defconfig + CONFIG_BTRFS_FS=y).
> > ---
> >   fs/btrfs/zstd.c | 11 ++++++++++-
> >   1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
> > index 86919293fd54..cb15cbd737c4 100644
> > --- a/fs/btrfs/zstd.c
> > +++ b/fs/btrfs/zstd.c
> > @@ -307,8 +307,17 @@ struct list_head *zstd_get_workspace(struct btrfs_fs_info *fs_info, int level)
> >               DEFINE_WAIT(wait);
> >
> >               prepare_to_wait(&zwsm->wait, &wait, TASK_UNINTERRUPTIBLE);
> > -             schedule();
> > +             /*
> > +              * Re-check after being queued: zstd_put_workspace() only
> > +              * wakes a queue that already has a sleeper, so a workspace
> > +              * returned since the failed allocation woke nobody.
> > +              */
> > +             ws = zstd_find_workspace(fs_info, level);
> > +             if (!ws)
> > +                     schedule();
> >               finish_wait(&zwsm->wait, &wait);
> > +             if (ws)
> > +                     return ws;
>
> This doesn't work.
>
>          Thread A (last ws holder)  |       Thread B (new ws user)
> -----------------------------------+--------------------------------
> zstd_put_workspace()               |
> |                                  | zstd_get_workspace()
> |                                  | |- zstd_find_workspace()
> |                                  | |  No WS found
> |                                  | |
> |                                  | |- zstd_alloc_workspace()
> |                                  | |  Allocation failure
> |                                  | |
> |                                  | |- zstd_find_workspace()
> |                                  | |  Still no WS found
> |- spin_lock_bh()                  | |
> |- spin_unlock_bh()                | |
> |- cond_wake_up()                  | |
>     No one is waiting               | |- prepare_to_wait()
>                                     | |- schedule()
>                                     | |- finish_wait()
>
> This is the typical LLM behavior, if your human brain can not examine
> every possibly wrong conclusion/fix from LLM, LLM will only waste time
> of everyone.
> >
> >               goto again;
> >       }
> >
> > ---
> > base-commit: 531ed942bb0df04f6747983fecdedce76a22d07f
> > change-id: 20260821-btrfs-zstd-lost-wakeup-0b0ee88ed52f
> >
> > Best regards,
> > --
> > FAN YE <fy15309206903@xxxxxxxxx>
> >
> >
> >
>