Re: [PATCH] fs: add missing fences to I_NEW handling

[Hillf Danton <hdanton@sina.com>]

On Mon, 6 Oct 2025 12:30:01 +0200 Mateusz Guzik wrote:
> On Mon, Oct 6, 2025 at 9:21 AM Hillf Danton <hdanton@sina.com> wrote:
> > On Mon, 6 Oct 2025 03:16:43 +0200 Mateusz Guzik wrote:
> > > That fence synchronizes against threads which went to sleep.
> > >
> > > In the example I'm providing this did not happen.
> > >
> > >   193 static inline void wait_on_inode(struct inode *inode)
> > >   194 {
> > >   195         wait_var_event(inode_state_wait_address(inode, __I_NEW),
> > >   196                       !(READ_ONCE(inode->i_state) & I_NEW));
> > >
> > >   303 #define wait_var_event(var, condition)                                  \
> > >   304 do {                                                                    \
> > >   305         might_sleep();                                                  \
> > >   306         if (condition)                                                  \
> > >   307                 break;                                                  \
> > >
> > > I_NEW is tested here without any locks or fences.
> > >
> > Thanks, got it but given the comment of the current mem barrier in
> > unlock_new_inode(), why did peterZ/Av leave such a huge chance behind?
> >
> My guess is nobody is perfect -- mistakes happen.
> 
I do not think you are so lucky -- the code has been there for quite a while.

> > The condition check in waitqueue_active() matches waitqueue_active() in
> > __wake_up_bit(), and both make it run fast on both the waiter and the
> > waker sides, no?
> 
Your quotation here is different from my reply [2]. Weird.

    The condition check in wait_var_event() matches waitqueue_active() in
    __wake_up_bit(), and both make it run fast on both the waiter and the
    waker sides, no?

[2] https://lore.kernel.org/lkml/20251006072136.8236-1-hdanton@sina.com/

> So happens the commentary above wait_var_event() explicitly mentions
> you want an acquire fence:
>   299  * The condition should normally use smp_load_acquire() or a similarly
>   300  * ordered access to ensure that any changes to memory made before the
>   301  * condition became true will be visible after the wait completes.

What is missed is recheck -- adding self on to wait queue with a true condition
makes no sense.

 * Wait for a @condition to be true, only re-checking when a wake up is
 * received for the given @var (an arbitrary kernel address which need
 * not be directly related to the given condition, but usually is).
> 
> The commentary about waitqueue_active() says you want and even
> stronger fence (smp_mb) for that one:

Yes, it matches the current code, so your change is not needed.

>    104  * Use either while holding wait_queue_head::lock or when used for wakeups
>    105  * with an extra smp_mb() like::
>    106  *
>    107  *      CPU0 - waker                    CPU1 - waiter
>    108  *
>    109  *                                      for (;;) {
>    110  *      @cond = true; 			   prepare_to_wait(&wq_head, &wait, state);
>    111  *      smp_mb();                         // smp_mb() from set_current_state()
>    112  *      if (waitqueue_active(wq_head))         if (@cond)
>    113  *        wake_up(wq_head);                      break;
>    114  *                                        schedule();
>    115  *                                      }
>    116  *                                      finish_wait(&wq_head, &wait);