Re: [PATCH RFC v2 1/6] fs: add i_state helpers

[Linus Torvalds <torvalds@linux-foundation.org>]

On Fri, 23 Aug 2024 at 10:52, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The wake_up_var() infrastructure that the inode code uses is a bit
> more involved. Not only can the variable be anything at all (so the
> operations you can do on it are obviously largely unbounded), but the
> inode hack in particular then uses one thing for the actual variable,
> and another thing for the address that is used to match up waits and
> wakeups.

.. btw, that doesn't mean we can't have helpers for the common cases.
They might have to be macros (so that they just work regardless of the
type), but having a

    set_var_and_wake(var, value);

macro that just expands to something like (completely untested "maybe
this works" macro):

  #define set_var_and_wake(var,value) do {        \
        __auto_type __set_ptr = &(var);          \
        *(__set_ptr) = (value);                 \
        smp_mb();                               \
        wake_up_var(__set_ptr);                 \
  } while (0)

doesn't sound too bad for at least some common patterns.

Looking around, we do seem to have a pattern of

   smp_store_release() -> wake_up_var()

instead of a memory barrier. I don't think that actually works. The
smp_store_release() means that *earlier* accesses will be bounded by
the store operation, but *later* accesses - including very much the
"look if the wait queue is empty" check - are totally unordered by it,
and can be done before the store by the CPU.

But I haven't thought deeply about it, that was just my gut reaction
when seeing the pattern. It superficially makes sense, but I think
it's entirely wrong (it's a "smp_load_acquire()" that would order with
later accesses, but there is no "store with acquire semantics"
operation).

              Linus