On Mon, 20 Nov 2023, Chuck Lever wrote: > On Mon, Nov 20, 2023 at 08:37:55AM +1100, NeilBrown wrote: > > On Fri, 17 Nov 2023, Jeff Layton wrote: > > > On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote: > > > > The protocol for creating a new state in nfsd is to allocated the state > > > > leaving it largely uninitialised, add that state to the ->cl_stateids > > > > idr so as to reserve a state id, then complete initialisation of the > > > > state and only set ->sc_type to non-zero once the state is fully > > > > initialised. > > > > > > > > If a state is found in the idr with ->sc_type == 0, it is ignored. > > > > The ->cl_lock list is used to avoid races - it is held while checking > > > > sc_type during lookup, and held when a non-zero value is stored in > > > > ->sc_type. > > > > > > > > ... except... hash_delegation_locked() finalises the initialisation of a > > > > delegation state, but does NOT hold ->cl_lock. > > > > > > > > So this patch takes ->cl_lock at the appropriate time w.r.t other locks, > > > > and so ensures there are no races (which are extremely unlikely in any > > > > case). > > > > > > > > Signed-off-by: NeilBrown <neilb@xxxxxxx> > > > > --- > > > > fs/nfsd/nfs4state.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > > > index 65fd5510323a..6368788a7d4e 100644 > > > > --- a/fs/nfsd/nfs4state.c > > > > +++ b/fs/nfsd/nfs4state.c > > > > @@ -1317,6 +1317,7 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp) > > > > > > > > lockdep_assert_held(&state_lock); > > > > lockdep_assert_held(&fp->fi_lock); > > > > + lockdep_assert_held(&clp->cl_lock); > > > > > > > > if (nfs4_delegation_exists(clp, fp)) > > > > return -EAGAIN; > > > > @@ -5609,12 +5610,14 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > > > > goto out_unlock; > > > > > > > > spin_lock(&state_lock); > > > > + spin_lock(&clp->cl_lock); > > > > spin_lock(&fp->fi_lock); > > > > if (fp->fi_had_conflict) > > > > status = -EAGAIN; > > > > else > > > > status = hash_delegation_locked(dp, fp); > > > > spin_unlock(&fp->fi_lock); > > > > + spin_unlock(&clp->cl_lock); > > > > spin_unlock(&state_lock); > > > > > > > > if (status) > > > > > > I know it's (supposedly) an unlikely race, but should we send this to > > > stable? > > > > I don't know. Once upon a time, "stable" meant something. There was a > > clear list of rules. Those seem to have been torn up. Now it seems to > > be whatever some machine-learning tool chooses. > > If that tool chooses this patch (which I suspect it will), I won't > > object. But I don't think it is worth encouraging it. > > We've asked Sasha and GregKH not to use AUTOSEL on NFSD patches, > promising that we will explicitly mark anything that should be > back-ported. Oh good - I didn't know that. Thanks. NeilBrown
