| Message ID | 20241206025723.3537777-2-neilb@suse.de (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | nfsd: use new wake_up_var interface | expand |
On Fri, 2024-12-06 at 13:55 +1100, NeilBrown wrote: > The wake_up_var interface is fragile as barriers are sometimes needed. > There are now new interfaces so that most wake-ups can use an interface > that is guaranteed to have all barriers needed. > > This patch changes the wake up on cl_cb_inflight to use > atomic_dec_and_wake_up(). > > It also changes the wake up on rp_locked to use store_release_wake_up(). > This involves changing rp_locked from atomic_t to int. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfs4callback.c | 3 +-- > fs/nfsd/nfs4state.c | 16 ++++++---------- > fs/nfsd/state.h | 2 +- > 3 files changed, 8 insertions(+), 13 deletions(-) > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > index 3877b53e429f..a8dc9de2f7fb 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -1036,8 +1036,7 @@ static void nfsd41_cb_inflight_begin(struct nfs4_client *clp) > static void nfsd41_cb_inflight_end(struct nfs4_client *clp) > { > > - if (atomic_dec_and_test(&clp->cl_cb_inflight)) > - wake_up_var(&clp->cl_cb_inflight); > + atomic_dec_and_wake_up(&clp->cl_cb_inflight); > } > > static void nfsd41_cb_inflight_wait_complete(struct nfs4_client *clp) > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 741b9449f727..9fbf7c8f0a3e 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -4739,7 +4739,7 @@ static void init_nfs4_replay(struct nfs4_replay *rp) > rp->rp_status = nfserr_serverfault; > rp->rp_buflen = 0; > rp->rp_buf = rp->rp_ibuf; > - atomic_set(&rp->rp_locked, RP_UNLOCKED); > + rp->rp_locked = RP_UNLOCKED; > } > > static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate, > @@ -4747,9 +4747,9 @@ static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate, > { > if (!nfsd4_has_session(cstate)) { > wait_var_event(&so->so_replay.rp_locked, > - atomic_cmpxchg(&so->so_replay.rp_locked, > - RP_UNLOCKED, RP_LOCKED) != RP_LOCKED); > - if (atomic_read(&so->so_replay.rp_locked) == RP_UNHASHED) > + cmpxchg(&so->so_replay.rp_locked, > + RP_UNLOCKED, RP_LOCKED) != RP_LOCKED); nit: try_cmpxchg() generates more efficient assembly. Can we switch to that here too? > + if (so->so_replay.rp_locked == RP_UNHASHED) > return -EAGAIN; > cstate->replay_owner = nfs4_get_stateowner(so); > } > @@ -4762,9 +4762,7 @@ void nfsd4_cstate_clear_replay(struct nfsd4_compound_state *cstate) > > if (so != NULL) { > cstate->replay_owner = NULL; > - atomic_set(&so->so_replay.rp_locked, RP_UNLOCKED); > - smp_mb__after_atomic(); > - wake_up_var(&so->so_replay.rp_locked); > + store_release_wake_up(&so->so_replay.rp_locked, RP_UNLOCKED); > nfs4_put_stateowner(so); > } > } > @@ -5069,9 +5067,7 @@ move_to_close_lru(struct nfs4_ol_stateid *s, struct net *net) > * Some threads with a reference might be waiting for rp_locked, > * so tell them to stop waiting. > */ > - atomic_set(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED); > - smp_mb__after_atomic(); > - wake_up_var(&oo->oo_owner.so_replay.rp_locked); > + store_release_wake_up(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED); > wait_event(close_wq, refcount_read(&s->st_stid.sc_count) == 2); > > release_all_access(s); > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > index e16bb3717fb9..ba30b2335b66 100644 > --- a/fs/nfsd/state.h > +++ b/fs/nfsd/state.h > @@ -505,7 +505,7 @@ struct nfs4_replay { > unsigned int rp_buflen; > char *rp_buf; > struct knfsd_fh rp_openfh; > - atomic_t rp_locked; > + int rp_locked; > char rp_ibuf[NFSD4_REPLAY_ISIZE]; > }; > Looks good otherwise. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Fri, 06 Dec 2024, Jeff Layton wrote: > On Fri, 2024-12-06 at 13:55 +1100, NeilBrown wrote: > > The wake_up_var interface is fragile as barriers are sometimes needed. > > There are now new interfaces so that most wake-ups can use an interface > > that is guaranteed to have all barriers needed. > > > > This patch changes the wake up on cl_cb_inflight to use > > atomic_dec_and_wake_up(). > > > > It also changes the wake up on rp_locked to use store_release_wake_up(). > > This involves changing rp_locked from atomic_t to int. > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > fs/nfsd/nfs4callback.c | 3 +-- > > fs/nfsd/nfs4state.c | 16 ++++++---------- > > fs/nfsd/state.h | 2 +- > > 3 files changed, 8 insertions(+), 13 deletions(-) > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > index 3877b53e429f..a8dc9de2f7fb 100644 > > --- a/fs/nfsd/nfs4callback.c > > +++ b/fs/nfsd/nfs4callback.c > > @@ -1036,8 +1036,7 @@ static void nfsd41_cb_inflight_begin(struct nfs4_client *clp) > > static void nfsd41_cb_inflight_end(struct nfs4_client *clp) > > { > > > > - if (atomic_dec_and_test(&clp->cl_cb_inflight)) > > - wake_up_var(&clp->cl_cb_inflight); > > + atomic_dec_and_wake_up(&clp->cl_cb_inflight); > > } > > > > static void nfsd41_cb_inflight_wait_complete(struct nfs4_client *clp) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 741b9449f727..9fbf7c8f0a3e 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -4739,7 +4739,7 @@ static void init_nfs4_replay(struct nfs4_replay *rp) > > rp->rp_status = nfserr_serverfault; > > rp->rp_buflen = 0; > > rp->rp_buf = rp->rp_ibuf; > > - atomic_set(&rp->rp_locked, RP_UNLOCKED); > > + rp->rp_locked = RP_UNLOCKED; > > } > > > > static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate, > > @@ -4747,9 +4747,9 @@ static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate, > > { > > if (!nfsd4_has_session(cstate)) { > > wait_var_event(&so->so_replay.rp_locked, > > - atomic_cmpxchg(&so->so_replay.rp_locked, > > - RP_UNLOCKED, RP_LOCKED) != RP_LOCKED); > > - if (atomic_read(&so->so_replay.rp_locked) == RP_UNHASHED) > > + cmpxchg(&so->so_replay.rp_locked, > > + RP_UNLOCKED, RP_LOCKED) != RP_LOCKED); > > nit: try_cmpxchg() generates more efficient assembly. Can we switch to > that here too? Does it? try_cmpxchg() makes loops smaller (as described in atomic_t.txt). I think it wins when the "old" value has to be updated each time around the loop. In this case the "old" value is always the same. NeilBrown > > > + if (so->so_replay.rp_locked == RP_UNHASHED) > > return -EAGAIN; > > cstate->replay_owner = nfs4_get_stateowner(so); > > } > > @@ -4762,9 +4762,7 @@ void nfsd4_cstate_clear_replay(struct nfsd4_compound_state *cstate) > > > > if (so != NULL) { > > cstate->replay_owner = NULL; > > - atomic_set(&so->so_replay.rp_locked, RP_UNLOCKED); > > - smp_mb__after_atomic(); > > - wake_up_var(&so->so_replay.rp_locked); > > + store_release_wake_up(&so->so_replay.rp_locked, RP_UNLOCKED); > > nfs4_put_stateowner(so); > > } > > } > > @@ -5069,9 +5067,7 @@ move_to_close_lru(struct nfs4_ol_stateid *s, struct net *net) > > * Some threads with a reference might be waiting for rp_locked, > > * so tell them to stop waiting. > > */ > > - atomic_set(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED); > > - smp_mb__after_atomic(); > > - wake_up_var(&oo->oo_owner.so_replay.rp_locked); > > + store_release_wake_up(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED); > > wait_event(close_wq, refcount_read(&s->st_stid.sc_count) == 2); > > > > release_all_access(s); > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > > index e16bb3717fb9..ba30b2335b66 100644 > > --- a/fs/nfsd/state.h > > +++ b/fs/nfsd/state.h > > @@ -505,7 +505,7 @@ struct nfs4_replay { > > unsigned int rp_buflen; > > char *rp_buf; > > struct knfsd_fh rp_openfh; > > - atomic_t rp_locked; > > + int rp_locked; > > char rp_ibuf[NFSD4_REPLAY_ISIZE]; > > }; > > > > Looks good otherwise. > > Reviewed-by: Jeff Layton <jlayton@kernel.org> >
On Fri, 2024-12-06 at 17:10 +1100, NeilBrown wrote: > On Fri, 06 Dec 2024, Jeff Layton wrote: > > On Fri, 2024-12-06 at 13:55 +1100, NeilBrown wrote: > > > The wake_up_var interface is fragile as barriers are sometimes needed. > > > There are now new interfaces so that most wake-ups can use an interface > > > that is guaranteed to have all barriers needed. > > > > > > This patch changes the wake up on cl_cb_inflight to use > > > atomic_dec_and_wake_up(). > > > > > > It also changes the wake up on rp_locked to use store_release_wake_up(). > > > This involves changing rp_locked from atomic_t to int. > > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > --- > > > fs/nfsd/nfs4callback.c | 3 +-- > > > fs/nfsd/nfs4state.c | 16 ++++++---------- > > > fs/nfsd/state.h | 2 +- > > > 3 files changed, 8 insertions(+), 13 deletions(-) > > > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > > index 3877b53e429f..a8dc9de2f7fb 100644 > > > --- a/fs/nfsd/nfs4callback.c > > > +++ b/fs/nfsd/nfs4callback.c > > > @@ -1036,8 +1036,7 @@ static void nfsd41_cb_inflight_begin(struct nfs4_client *clp) > > > static void nfsd41_cb_inflight_end(struct nfs4_client *clp) > > > { > > > > > > - if (atomic_dec_and_test(&clp->cl_cb_inflight)) > > > - wake_up_var(&clp->cl_cb_inflight); > > > + atomic_dec_and_wake_up(&clp->cl_cb_inflight); > > > } > > > > > > static void nfsd41_cb_inflight_wait_complete(struct nfs4_client *clp) > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > > index 741b9449f727..9fbf7c8f0a3e 100644 > > > --- a/fs/nfsd/nfs4state.c > > > +++ b/fs/nfsd/nfs4state.c > > > @@ -4739,7 +4739,7 @@ static void init_nfs4_replay(struct nfs4_replay *rp) > > > rp->rp_status = nfserr_serverfault; > > > rp->rp_buflen = 0; > > > rp->rp_buf = rp->rp_ibuf; > > > - atomic_set(&rp->rp_locked, RP_UNLOCKED); > > > + rp->rp_locked = RP_UNLOCKED; > > > } > > > > > > static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate, > > > @@ -4747,9 +4747,9 @@ static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate, > > > { > > > if (!nfsd4_has_session(cstate)) { > > > wait_var_event(&so->so_replay.rp_locked, > > > - atomic_cmpxchg(&so->so_replay.rp_locked, > > > - RP_UNLOCKED, RP_LOCKED) != RP_LOCKED); > > > - if (atomic_read(&so->so_replay.rp_locked) == RP_UNHASHED) > > > + cmpxchg(&so->so_replay.rp_locked, > > > + RP_UNLOCKED, RP_LOCKED) != RP_LOCKED); > > > > nit: try_cmpxchg() generates more efficient assembly. Can we switch to > > that here too? > > Does it? try_cmpxchg() makes loops smaller (as described in > atomic_t.txt). I think it wins when the "old" value has to be updated > each time around the loop. In this case the "old" value is always the > same. > > In most cases, it does, because we have to return "old" in the case of the traditional cmpxchg() operation. From atomic_t.txt: int atomic_cmpxchg(atomic_t *ptr, int old, int new) { (void)atomic_try_cmpxchg(ptr, &old, new); return old; } That said, in this case it my not be a win. You need something to return that value anyway, so it can properly act as a wait_var_event() condition. > > > > > + if (so->so_replay.rp_locked == RP_UNHASHED) > > > return -EAGAIN; > > > cstate->replay_owner = nfs4_get_stateowner(so); > > > } > > > @@ -4762,9 +4762,7 @@ void nfsd4_cstate_clear_replay(struct nfsd4_compound_state *cstate) > > > > > > if (so != NULL) { > > > cstate->replay_owner = NULL; > > > - atomic_set(&so->so_replay.rp_locked, RP_UNLOCKED); > > > - smp_mb__after_atomic(); > > > - wake_up_var(&so->so_replay.rp_locked); > > > + store_release_wake_up(&so->so_replay.rp_locked, RP_UNLOCKED); > > > nfs4_put_stateowner(so); > > > } > > > } > > > @@ -5069,9 +5067,7 @@ move_to_close_lru(struct nfs4_ol_stateid *s, struct net *net) > > > * Some threads with a reference might be waiting for rp_locked, > > > * so tell them to stop waiting. > > > */ > > > - atomic_set(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED); > > > - smp_mb__after_atomic(); > > > - wake_up_var(&oo->oo_owner.so_replay.rp_locked); > > > + store_release_wake_up(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED); > > > wait_event(close_wq, refcount_read(&s->st_stid.sc_count) == 2); > > > > > > release_all_access(s); > > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > > > index e16bb3717fb9..ba30b2335b66 100644 > > > --- a/fs/nfsd/state.h > > > +++ b/fs/nfsd/state.h > > > @@ -505,7 +505,7 @@ struct nfs4_replay { > > > unsigned int rp_buflen; > > > char *rp_buf; > > > struct knfsd_fh rp_openfh; > > > - atomic_t rp_locked; > > > + int rp_locked; > > > char rp_ibuf[NFSD4_REPLAY_ISIZE]; > > > }; > > > > > > > Looks good otherwise. > > > > Reviewed-by: Jeff Layton <jlayton@kernel.org> > > >
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index 3877b53e429f..a8dc9de2f7fb 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -1036,8 +1036,7 @@ static void nfsd41_cb_inflight_begin(struct nfs4_client *clp) static void nfsd41_cb_inflight_end(struct nfs4_client *clp) { - if (atomic_dec_and_test(&clp->cl_cb_inflight)) - wake_up_var(&clp->cl_cb_inflight); + atomic_dec_and_wake_up(&clp->cl_cb_inflight); } static void nfsd41_cb_inflight_wait_complete(struct nfs4_client *clp) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 741b9449f727..9fbf7c8f0a3e 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -4739,7 +4739,7 @@ static void init_nfs4_replay(struct nfs4_replay *rp) rp->rp_status = nfserr_serverfault; rp->rp_buflen = 0; rp->rp_buf = rp->rp_ibuf; - atomic_set(&rp->rp_locked, RP_UNLOCKED); + rp->rp_locked = RP_UNLOCKED; } static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate, @@ -4747,9 +4747,9 @@ static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate, { if (!nfsd4_has_session(cstate)) { wait_var_event(&so->so_replay.rp_locked, - atomic_cmpxchg(&so->so_replay.rp_locked, - RP_UNLOCKED, RP_LOCKED) != RP_LOCKED); - if (atomic_read(&so->so_replay.rp_locked) == RP_UNHASHED) + cmpxchg(&so->so_replay.rp_locked, + RP_UNLOCKED, RP_LOCKED) != RP_LOCKED); + if (so->so_replay.rp_locked == RP_UNHASHED) return -EAGAIN; cstate->replay_owner = nfs4_get_stateowner(so); } @@ -4762,9 +4762,7 @@ void nfsd4_cstate_clear_replay(struct nfsd4_compound_state *cstate) if (so != NULL) { cstate->replay_owner = NULL; - atomic_set(&so->so_replay.rp_locked, RP_UNLOCKED); - smp_mb__after_atomic(); - wake_up_var(&so->so_replay.rp_locked); + store_release_wake_up(&so->so_replay.rp_locked, RP_UNLOCKED); nfs4_put_stateowner(so); } } @@ -5069,9 +5067,7 @@ move_to_close_lru(struct nfs4_ol_stateid *s, struct net *net) * Some threads with a reference might be waiting for rp_locked, * so tell them to stop waiting. */ - atomic_set(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED); - smp_mb__after_atomic(); - wake_up_var(&oo->oo_owner.so_replay.rp_locked); + store_release_wake_up(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED); wait_event(close_wq, refcount_read(&s->st_stid.sc_count) == 2); release_all_access(s); diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h index e16bb3717fb9..ba30b2335b66 100644 --- a/fs/nfsd/state.h +++ b/fs/nfsd/state.h @@ -505,7 +505,7 @@ struct nfs4_replay { unsigned int rp_buflen; char *rp_buf; struct knfsd_fh rp_openfh; - atomic_t rp_locked; + int rp_locked; char rp_ibuf[NFSD4_REPLAY_ISIZE]; };
The wake_up_var interface is fragile as barriers are sometimes needed. There are now new interfaces so that most wake-ups can use an interface that is guaranteed to have all barriers needed. This patch changes the wake up on cl_cb_inflight to use atomic_dec_and_wake_up(). It also changes the wake up on rp_locked to use store_release_wake_up(). This involves changing rp_locked from atomic_t to int. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/nfs4callback.c | 3 +-- fs/nfsd/nfs4state.c | 16 ++++++---------- fs/nfsd/state.h | 2 +- 3 files changed, 8 insertions(+), 13 deletions(-)