[0/2] nfsd: improvements for wake_up_bit/var

Message

NeilBrown Aug. 30, 2024, 7:03 a.m. UTC

I've been digging into wake_up_bit and wake_up_var recently.  There are
a lot of places where the required barriers aren't quite right.

This patch fixes them up for nfsd.  The bugs are mostly minor, though
the rp_locked on might be a credible problem on weakly ordered hosts
(e.g.  power64).

NeilBrown

 [PATCH 1/2] nfsd: use clear_and_wake_up_bit()
 [PATCH 2/2] nfsd: avoid races with wake_up_var()

Comments

Santosh Pradhan Aug. 30, 2024, 10:40 a.m. UTC | #1

Hi Neil,
Do we need a barrier in nfs4_disable_swap() as well ?

10830         set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state);
10831         clear_bit(NFS4CLNT_MANAGER_AVAILABLE, &clp->cl_state);
>>> HERE>>>>
10832         wake_up_var(&clp->cl_state);

Regards,
Santosh

On Fri, Aug 30, 2024 at 12:37 PM NeilBrown <[e-mail]> wrote:
>
> I've been digging into wake_up_bit and wake_up_var recently.  There are
> a lot of places where the required barriers aren't quite right.
>
> This patch fixes them up for nfsd.  The bugs are mostly minor, though
> the rp_locked on might be a credible problem on weakly ordered hosts
> (e.g.  power64).
>
> NeilBrown
>
>  [PATCH 1/2] nfsd: use clear_and_wake_up_bit()
>  [PATCH 2/2] nfsd: avoid races with wake_up_var()
>

NeilBrown Aug. 30, 2024, 11:32 a.m. UTC | #2

On Fri, 30 Aug 2024, Santosh Pradhan wrote:
> Hi Neil,
> Do we need a barrier in nfs4_disable_swap() as well ?
> 
> 10830         set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state);
> 10831         clear_bit(NFS4CLNT_MANAGER_AVAILABLE, &clp->cl_state);
> >>> HERE>>>>
> 10832         wake_up_var(&clp->cl_state);

Probably but that is in fs/nfs/, not fs/nfsd/.

Thanks,
NeilBrown


> 
> Regards,
> Santosh
> 
> On Fri, Aug 30, 2024 at 12:37 PM NeilBrown <[e-mail]> wrote:
> >
> > I've been digging into wake_up_bit and wake_up_var recently.  There are
> > a lot of places where the required barriers aren't quite right.
> >
> > This patch fixes them up for nfsd.  The bugs are mostly minor, though
> > the rp_locked on might be a credible problem on weakly ordered hosts
> > (e.g.  power64).
> >
> > NeilBrown
> >
> >  [PATCH 1/2] nfsd: use clear_and_wake_up_bit()
> >  [PATCH 2/2] nfsd: avoid races with wake_up_var()
> >
>

Jeff Layton Aug. 30, 2024, 12:25 p.m. UTC | #3

On Fri, 2024-08-30 at 17:03 +1000, NeilBrown wrote:
> I've been digging into wake_up_bit and wake_up_var recently.  There are
> a lot of places where the required barriers aren't quite right.
> 
> This patch fixes them up for nfsd.  The bugs are mostly minor, though
> the rp_locked on might be a credible problem on weakly ordered hosts
> (e.g.  power64).
> 
> NeilBrown
> 
>  [PATCH 1/2] nfsd: use clear_and_wake_up_bit()
>  [PATCH 2/2] nfsd: avoid races with wake_up_var()

Nice catches.

Reviewed-by: Jeff Layton <[e-mail]>

Chuck Lever III Aug. 30, 2024, 2:38 p.m. UTC | #4

On Fri, 30 Aug 2024 17:03:15 +1000, NeilBrown wrote:
> I've been digging into wake_up_bit and wake_up_var recently.  There are
> a lot of places where the required barriers aren't quite right.
> 
> This patch fixes them up for nfsd.  The bugs are mostly minor, though
> the rp_locked on might be a credible problem on weakly ordered hosts
> (e.g.  power64).
> 
> [...]

Applied to nfsd-next for v6.12, thanks!

[1/2] nfsd: use clear_and_wake_up_bit()
      commit: 2b9a19d16beda1b2ca5edab47d74b73d4d958b12
[2/2] nfsd: avoid races with wake_up_var()
      commit: a2bf7d13821603fb90c3f6e695bd5fb4ee19de71

Both of these patches threw compilation errors. I corrected those
issues before applying. Please check my work.

checkpatch.pl complained about the lack of comment on the added
barriers. I felt the code was self-explanatory so made no change.

NeilBrown Sept. 2, 2024, 12:51 a.m. UTC | #5

On Sat, 31 Aug 2024, Chuck Lever wrote:
> On Fri, 30 Aug 2024 17:03:15 +1000, NeilBrown wrote:
> > I've been digging into wake_up_bit and wake_up_var recently.  There are
> > a lot of places where the required barriers aren't quite right.
> > 
> > This patch fixes them up for nfsd.  The bugs are mostly minor, though
> > the rp_locked on might be a credible problem on weakly ordered hosts
> > (e.g.  power64).
> > 
> > [...]
> 
> Applied to nfsd-next for v6.12, thanks!
> 
> [1/2] nfsd: use clear_and_wake_up_bit()
>       commit: 2b9a19d16beda1b2ca5edab47d74b73d4d958b12
> [2/2] nfsd: avoid races with wake_up_var()
>       commit: a2bf7d13821603fb90c3f6e695bd5fb4ee19de71
> 
> Both of these patches threw compilation errors. I corrected those
> issues before applying. Please check my work.

Thanks for that ...  I really shouldn't post patches in the afternoon -
I'm not thinking as clearly :-(

> 
> checkpatch.pl complained about the lack of comment on the added
> barriers. I felt the code was self-explanatory so made no change.
> 

I'd really like a wake_up_var_after_atomic() but Linus wasn't keen on
going in that direction.

if/when my improvements land
  https://lore.kernel.org/all/[e-mail]/
we could change rp_locked to an "int" and use
   store_release_wake_up()
which includes that required barrier.

NeilBrown