Re: [PATCH 07/10] netfs: Fix missing barriers by using clear_and_wake_up_bit()

David Howells
2024-12-14T13:44:59Z

[Adding Paul McKenney as he's the expert.]

Akira Yokosawa <akiyks@gmail.com> wrote:

> David Howells wrote:
> > Use clear_and_wake_up_bit() rather than something like:
> >
> > clear_bit_unlock(NETFS_RREQ_IN_PROGRESS, &rreq->flags);
> > wake_up_bit(&rreq->flags, NETFS_RREQ_IN_PROGRESS);
> >
> > as there needs to be a barrier inserted between which is present in
> > clear_and_wake_up_bit().
>
> If I am reading the kernel-doc comment of clear_bit_unlock() [1, 2]:
>
> This operation is atomic and provides release barrier semantics.
>
> correctly, there already seems to be a barrier which should be
> good enough.
>
> [1]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.clear_bit_unlock
> [2]: include/asm-generic/bitops/instrumented-lock.h
>
> >
> > Fixes: 288ace2f57c9 ("netfs: New writeback implementation")
> > Fixes: ee4cdf7ba857 ("netfs: Speed up buffered reading")
>
> So I'm not sure this fixes anything.
>
> What am I missing?

We may need two barriers. You have three things to synchronise:

(1) The stuff you did before unlocking.

(2) The lock bit.

(3) The task state.

clear_bit_unlock() interposes a release barrier between (1) and (2).

Neither clear_bit_unlock() nor wake_up_bit(), however, necessarily interpose a
barrier between (2) and (3). I'm not sure it entirely matters, but it seems
that since we have a function that combines the two, we should probably use
it - though, granted, it might not actually be a fix.

David