| Message ID | 20240829-delstid-v3-1-271c60806c5d@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: implement the "delstid" draft | expand |
On Thu, Aug 29, 2024 at 09:26:39AM -0400, Jeff Layton wrote: > From: NeilBrown <neilb@suse.de> > > It is not safe to dereference fl->c.flc_owner without first confirming > fl->fl_lmops is the expected manager. nfsd4_deleg_getattr_conflict() > tests fl_lmops but largely ignores the result and assumes that flc_owner > is an nfs4_delegation anyway. This is wrong. > > With this patch we restore the "!= &nfsd_lease_mng_ops" case to behave > as it did before the changed mentioned below. This the same as the > current code, but without any reference to a possible delegation. > > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation") > Signed-off-by: NeilBrown <neilb@suse.de> > Signed-off-by: Jeff Layton <jlayton@kernel.org> I've already applied this to nfsd-fixes. If I include this commit in both nfsd-fixes and nfsd-next then the linux-next merge whines about duplicate patches. Stephen Rothwell suggested git-merging nfsd-fixes and nfsd-next but I'm not quite confident enough to try that. Barring another solution, merging this series will have to wait a few days before the two trees can sync up. > --- > fs/nfsd/nfs4state.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index b6bf39c64d78..eaa11d42d1b1 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -8854,7 +8854,15 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry, > */ > if (type == F_RDLCK) > break; > - goto break_lease; > + > + nfsd_stats_wdeleg_getattr_inc(nn); > + spin_unlock(&ctx->flc_lock); > + > + status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ)); > + if (status != nfserr_jukebox || > + !nfsd_wait_for_delegreturn(rqstp, inode)) > + return status; > + return 0; > } > if (type == F_WRLCK) { > struct nfs4_delegation *dp = fl->c.flc_owner; > @@ -8863,7 +8871,6 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry, > spin_unlock(&ctx->flc_lock); > return 0; > } > -break_lease: > nfsd_stats_wdeleg_getattr_inc(nn); > dp = fl->c.flc_owner; > refcount_inc(&dp->dl_stid.sc_count); > > -- > 2.46.0 >
On Fri, 30 Aug 2024, Chuck Lever wrote: > On Thu, Aug 29, 2024 at 09:26:39AM -0400, Jeff Layton wrote: > > From: NeilBrown <neilb@suse.de> > > > > It is not safe to dereference fl->c.flc_owner without first confirming > > fl->fl_lmops is the expected manager. nfsd4_deleg_getattr_conflict() > > tests fl_lmops but largely ignores the result and assumes that flc_owner > > is an nfs4_delegation anyway. This is wrong. > > > > With this patch we restore the "!= &nfsd_lease_mng_ops" case to behave > > as it did before the changed mentioned below. This the same as the > > current code, but without any reference to a possible delegation. > > > > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation") > > Signed-off-by: NeilBrown <neilb@suse.de> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > I've already applied this to nfsd-fixes. > > If I include this commit in both nfsd-fixes and nfsd-next then the > linux-next merge whines about duplicate patches. Stephen Rothwell > suggested git-merging nfsd-fixes and nfsd-next but I'm not quite > confident enough to try that. > > Barring another solution, merging this series will have to wait a > few days before the two trees can sync up. Hmmm.... I would probably always rebase nfsd-next on nfsd-fixes, which I would rebase on the most recent of rc0, rc1, or the latest rc to receive nfsd patches. nfsd-fixes is currently based on 6.10-rc7, while -next is based on 6.11-rc5. Why the 6.10 base?? NeilBrown
> On Aug 30, 2024, at 2:01 AM, NeilBrown <neilb@suse.de> wrote: > > On Fri, 30 Aug 2024, Chuck Lever wrote: >> On Thu, Aug 29, 2024 at 09:26:39AM -0400, Jeff Layton wrote: >>> From: NeilBrown <neilb@suse.de> >>> >>> It is not safe to dereference fl->c.flc_owner without first confirming >>> fl->fl_lmops is the expected manager. nfsd4_deleg_getattr_conflict() >>> tests fl_lmops but largely ignores the result and assumes that flc_owner >>> is an nfs4_delegation anyway. This is wrong. >>> >>> With this patch we restore the "!= &nfsd_lease_mng_ops" case to behave >>> as it did before the changed mentioned below. This the same as the >>> current code, but without any reference to a possible delegation. >>> >>> Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation") >>> Signed-off-by: NeilBrown <neilb@suse.de> >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >> >> I've already applied this to nfsd-fixes. >> >> If I include this commit in both nfsd-fixes and nfsd-next then the >> linux-next merge whines about duplicate patches. Stephen Rothwell >> suggested git-merging nfsd-fixes and nfsd-next but I'm not quite >> confident enough to try that. >> >> Barring another solution, merging this series will have to wait a >> few days before the two trees can sync up. > > Hmmm.... I would probably always rebase nfsd-next on nfsd-fixes, which > I would rebase on the most recent of rc0, rc1, or the latest rc to > receive nfsd patches. That straightforward strategy leaves NFSD fixes scattered throughout the merge history. > nfsd-fixes is currently based on 6.10-rc7, while -next is based on > 6.11-rc5. > > Why the 6.10 base?? nfsd-fixes extends the branch I initially submitted to Linus for the last merge window. That makes it easy to locate the full set of NFSD commits that go into each kernel release in the order they were submitted and keeps the merge topology simpler. B - C - D - E <<< nfsd / \ \ - A - X - Y - Z - AA - <<< master Or, that's the theory anyway. And generally it's not an irritant except for right near the end of the development cycle. I'm not 100% wedded to this approach, and I am interested in discussion and improvement. Stephen Rothwell suggested that I provide a single merged branch for development and for linux-next to pull. I almost understand how to do that, but it's still a bit beyond my current everyday tooling (stgit). -- Chuck Lever
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index b6bf39c64d78..eaa11d42d1b1 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -8854,7 +8854,15 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry, */ if (type == F_RDLCK) break; - goto break_lease; + + nfsd_stats_wdeleg_getattr_inc(nn); + spin_unlock(&ctx->flc_lock); + + status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ)); + if (status != nfserr_jukebox || + !nfsd_wait_for_delegreturn(rqstp, inode)) + return status; + return 0; } if (type == F_WRLCK) { struct nfs4_delegation *dp = fl->c.flc_owner; @@ -8863,7 +8871,6 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry, spin_unlock(&ctx->flc_lock); return 0; } -break_lease: nfsd_stats_wdeleg_getattr_inc(nn); dp = fl->c.flc_owner; refcount_inc(&dp->dl_stid.sc_count);