| Message ID | 1404143423-24381-16-git-send-email-jlayton@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 30, 2014 at 11:48:44AM -0400, Jeff Layton wrote: > We want to use the nfsd4_compound_state to cache the nfs4_client in > order to optimise away extra lookups of the clid. > > In the v4.0 case, we use this to ensure that we only have to look up the > client at most once per compound for each call into lookup_clientid. For > v4.1+ we set the pointer in the cstate during SEQUENCE processing so we > should never need to do a search for it. > > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 30, 2014 at 11:48:44AM -0400, Jeff Layton wrote: > @@ -2997,6 +3009,38 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4 > return nfserr_bad_seqid; > } > > +static __be32 lookup_clientid(clientid_t *clid, > + struct nfsd4_compound_state *cstate, > + struct nfsd_net *nn) > +{ > + struct nfs4_client *found; > + > + if (cstate->clp) { > + found = cstate->clp; > + if (!same_clid(&found->cl_clientid, clid)) > + return nfserr_stale_clientid; That's new behavior, isn't it? But sending a single compound that references multiple clients sounds nuts even in the 4.0 case, OK. Applying. (And I've merged all but the delegation locking change up through here so far.) --b. > + return nfs_ok; > + } > + > + if (STALE_CLIENTID(clid, nn)) > + return nfserr_stale_clientid; > + > + /* > + * For v4.1+ we get the client in the SEQUENCE op. If we don't have one > + * cached already then we know this is for is for v4.0 and "sessions" > + * will be false. > + */ > + WARN_ON_ONCE(cstate->session); > + found = find_confirmed_client(clid, false, nn); > + if (!found) > + return nfserr_expired; > + > + /* Cache the nfs4_client in cstate! */ > + cstate->clp = found; > + atomic_inc(&found->cl_refcount); > + return nfs_ok; > +} > + > __be32 > nfsd4_process_open1(struct nfsd4_compound_state *cstate, > struct nfsd4_open *open, struct nfsd_net *nn) > @@ -3509,18 +3553,6 @@ void nfsd4_cleanup_open_state(struct nfsd4_open *open, __be32 status) > free_generic_stateid(open->op_stp); > } > > -static __be32 lookup_clientid(clientid_t *clid, bool session, struct nfsd_net *nn, struct nfs4_client **clp) > -{ > - struct nfs4_client *found; > - > - if (STALE_CLIENTID(clid, nn)) > - return nfserr_stale_clientid; > - found = find_confirmed_client(clid, session, nn); > - if (clp) > - *clp = found; > - return found ? nfs_ok : nfserr_expired; > -} > - > __be32 > nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > clientid_t *clid) > @@ -3532,9 +3564,10 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > nfs4_lock_state(); > dprintk("process_renew(%08x/%08x): starting\n", > clid->cl_boot, clid->cl_id); > - status = lookup_clientid(clid, cstate->minorversion, nn, &clp); > + status = lookup_clientid(clid, cstate, nn); > if (status) > goto out; > + clp = cstate->clp; > status = nfserr_cb_path_down; > if (!list_empty(&clp->cl_delegations) > && clp->cl_cb_state != NFSD4_CB_UP) > @@ -3797,22 +3830,19 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate, > stateid_t *stateid, unsigned char typemask, > struct nfs4_stid **s, struct nfsd_net *nn) > { > - struct nfs4_client *cl; > __be32 status; > - bool sessions = cstate->minorversion != 0; > > if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) > return nfserr_bad_stateid; > - status = lookup_clientid(&stateid->si_opaque.so_clid, sessions, > - nn, &cl); > + status = lookup_clientid(&stateid->si_opaque.so_clid, cstate, nn); > if (status == nfserr_stale_clientid) { > - if (sessions) > + if (cstate->session) > return nfserr_bad_stateid; > return nfserr_stale_stateid; > } > if (status) > return status; > - *s = find_stateid_by_type(cl, stateid, typemask); > + *s = find_stateid_by_type(cstate->clp, stateid, typemask); > if (!*s) > return nfserr_bad_stateid; > return nfs_ok; > @@ -4662,7 +4692,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > nfs4_lock_state(); > > if (!nfsd4_has_session(cstate)) { > - status = lookup_clientid(&lockt->lt_clientid, false, nn, NULL); > + status = lookup_clientid(&lockt->lt_clientid, cstate, nn); > if (status) > goto out; > } > @@ -4831,7 +4861,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, > > nfs4_lock_state(); > > - status = lookup_clientid(clid, cstate->minorversion, nn, NULL); > + status = lookup_clientid(clid, cstate, nn); > if (status) > goto out; > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > index 7d8af164523b..312f6483a48e 100644 > --- a/fs/nfsd/xdr4.h > +++ b/fs/nfsd/xdr4.h > @@ -55,6 +55,7 @@ struct nfsd4_compound_state { > struct svc_fh current_fh; > struct svc_fh save_fh; > struct nfs4_stateowner *replay_owner; > + struct nfs4_client *clp; > /* For sessions DRC */ > struct nfsd4_session *session; > struct nfsd4_slot *slot; > -- > 1.9.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 3 Jul 2014 11:18:19 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Mon, Jun 30, 2014 at 11:48:44AM -0400, Jeff Layton wrote: > > @@ -2997,6 +3009,38 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4 > > return nfserr_bad_seqid; > > } > > > > +static __be32 lookup_clientid(clientid_t *clid, > > + struct nfsd4_compound_state *cstate, > > + struct nfsd_net *nn) > > +{ > > + struct nfs4_client *found; > > + > > + if (cstate->clp) { > > + found = cstate->clp; > > + if (!same_clid(&found->cl_clientid, clid)) > > + return nfserr_stale_clientid; > > That's new behavior, isn't it? > Yeah, I suppose it is, but it's hard to understand a valid use-case for sending multiple ops in a compound with different clientids. Certainly no well-behaved client would do that, would it? (Hmm, that might be an interesting pynfs test, come to think of it). > But sending a single compound that references multiple clients sounds > nuts even in the 4.0 case, OK. Applying. > > (And I've merged all but the delegation locking change up through here > so far.) > > --b. > Thanks! > > + return nfs_ok; > > + } > > + > > + if (STALE_CLIENTID(clid, nn)) > > + return nfserr_stale_clientid; > > + > > + /* > > + * For v4.1+ we get the client in the SEQUENCE op. If we > > don't have one > > + * cached already then we know this is for is for v4.0 and > > "sessions" > > + * will be false. > > + */ > > + WARN_ON_ONCE(cstate->session); > > + found = find_confirmed_client(clid, false, nn); > > + if (!found) > > + return nfserr_expired; > > + > > + /* Cache the nfs4_client in cstate! */ > > + cstate->clp = found; > > + atomic_inc(&found->cl_refcount); > > + return nfs_ok; > > +} > > + > > __be32 > > nfsd4_process_open1(struct nfsd4_compound_state *cstate, > > struct nfsd4_open *open, struct nfsd_net *nn) > > @@ -3509,18 +3553,6 @@ void nfsd4_cleanup_open_state(struct > > nfsd4_open *open, __be32 status) free_generic_stateid(open->op_stp); > > } > > > > -static __be32 lookup_clientid(clientid_t *clid, bool session, > > struct nfsd_net *nn, struct nfs4_client **clp) -{ > > - struct nfs4_client *found; > > - > > - if (STALE_CLIENTID(clid, nn)) > > - return nfserr_stale_clientid; > > - found = find_confirmed_client(clid, session, nn); > > - if (clp) > > - *clp = found; > > - return found ? nfs_ok : nfserr_expired; > > -} > > - > > __be32 > > nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state > > *cstate, clientid_t *clid) > > @@ -3532,9 +3564,10 @@ nfsd4_renew(struct svc_rqst *rqstp, struct > > nfsd4_compound_state *cstate, nfs4_lock_state(); > > dprintk("process_renew(%08x/%08x): starting\n", > > clid->cl_boot, clid->cl_id); > > - status = lookup_clientid(clid, cstate->minorversion, nn, > > &clp); > > + status = lookup_clientid(clid, cstate, nn); > > if (status) > > goto out; > > + clp = cstate->clp; > > status = nfserr_cb_path_down; > > if (!list_empty(&clp->cl_delegations) > > && clp->cl_cb_state != NFSD4_CB_UP) > > @@ -3797,22 +3830,19 @@ nfsd4_lookup_stateid(struct > > nfsd4_compound_state *cstate, stateid_t *stateid, unsigned char > > typemask, struct nfs4_stid **s, struct nfsd_net *nn) > > { > > - struct nfs4_client *cl; > > __be32 status; > > - bool sessions = cstate->minorversion != 0; > > > > if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) > > return nfserr_bad_stateid; > > - status = lookup_clientid(&stateid->si_opaque.so_clid, > > sessions, > > - nn, &cl); > > + status = lookup_clientid(&stateid->si_opaque.so_clid, > > cstate, nn); if (status == nfserr_stale_clientid) { > > - if (sessions) > > + if (cstate->session) > > return nfserr_bad_stateid; > > return nfserr_stale_stateid; > > } > > if (status) > > return status; > > - *s = find_stateid_by_type(cl, stateid, typemask); > > + *s = find_stateid_by_type(cstate->clp, stateid, typemask); > > if (!*s) > > return nfserr_bad_stateid; > > return nfs_ok; > > @@ -4662,7 +4692,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct > > nfsd4_compound_state *cstate, nfs4_lock_state(); > > > > if (!nfsd4_has_session(cstate)) { > > - status = lookup_clientid(&lockt->lt_clientid, > > false, nn, NULL); > > + status = lookup_clientid(&lockt->lt_clientid, > > cstate, nn); if (status) > > goto out; > > } > > @@ -4831,7 +4861,7 @@ nfsd4_release_lockowner(struct svc_rqst > > *rqstp, > > nfs4_lock_state(); > > > > - status = lookup_clientid(clid, cstate->minorversion, nn, > > NULL); > > + status = lookup_clientid(clid, cstate, nn); > > if (status) > > goto out; > > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > index 7d8af164523b..312f6483a48e 100644 > > --- a/fs/nfsd/xdr4.h > > +++ b/fs/nfsd/xdr4.h > > @@ -55,6 +55,7 @@ struct nfsd4_compound_state { > > struct svc_fh current_fh; > > struct svc_fh save_fh; > > struct nfs4_stateowner *replay_owner; > > + struct nfs4_client *clp; > > /* For sessions DRC */ > > struct nfsd4_session *session; > > struct nfsd4_slot *slot; > > -- > > 1.9.3 > >
On Thu, Jul 03, 2014 at 11:20:50AM -0400, Jeff Layton wrote: > On Thu, 3 Jul 2014 11:18:19 -0400 > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > On Mon, Jun 30, 2014 at 11:48:44AM -0400, Jeff Layton wrote: > > > @@ -2997,6 +3009,38 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4 > > > return nfserr_bad_seqid; > > > } > > > > > > +static __be32 lookup_clientid(clientid_t *clid, > > > + struct nfsd4_compound_state *cstate, > > > + struct nfsd_net *nn) > > > +{ > > > + struct nfs4_client *found; > > > + > > > + if (cstate->clp) { > > > + found = cstate->clp; > > > + if (!same_clid(&found->cl_clientid, clid)) > > > + return nfserr_stale_clientid; > > > > That's new behavior, isn't it? > > > > Yeah, I suppose it is, but it's hard to understand a valid use-case for > sending multiple ops in a compound with different clientids. Certainly > no well-behaved client would do that, would it? (Hmm, that might be an > interesting pynfs test, come to think of it). We could ask for a clarification in 3530bis if there's not already something there that clearly forbids this, but I'm not sure if it's even worth it. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 3 Jul 2014 11:32:08 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Thu, Jul 03, 2014 at 11:20:50AM -0400, Jeff Layton wrote: > > On Thu, 3 Jul 2014 11:18:19 -0400 > > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > > > On Mon, Jun 30, 2014 at 11:48:44AM -0400, Jeff Layton wrote: > > > > @@ -2997,6 +3009,38 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4 > > > > return nfserr_bad_seqid; > > > > } > > > > > > > > +static __be32 lookup_clientid(clientid_t *clid, > > > > + struct nfsd4_compound_state *cstate, > > > > + struct nfsd_net *nn) > > > > +{ > > > > + struct nfs4_client *found; > > > > + > > > > + if (cstate->clp) { > > > > + found = cstate->clp; > > > > + if (!same_clid(&found->cl_clientid, clid)) > > > > + return nfserr_stale_clientid; > > > > > > That's new behavior, isn't it? > > > > > > > Yeah, I suppose it is, but it's hard to understand a valid use-case for > > sending multiple ops in a compound with different clientids. Certainly > > no well-behaved client would do that, would it? (Hmm, that might be an > > interesting pynfs test, come to think of it). > > We could ask for a clarification in 3530bis if there's not already > something there that clearly forbids this, but I'm not sure if it's even > worth it. > I'm not inclined to pursue it, unless Tom thinks it's worthwhile. Tom, do you happen to care?
On Thu, Jul 03, 2014 at 11:32:08AM -0400, J. Bruce Fields wrote: > > > > + struct nfs4_client *found; > > > > + > > > > + if (cstate->clp) { > > > > + found = cstate->clp; > > > > + if (!same_clid(&found->cl_clientid, clid)) > > > > + return nfserr_stale_clientid; > > > > > > That's new behavior, isn't it? > > > > > > > Yeah, I suppose it is, but it's hard to understand a valid use-case for > > sending multiple ops in a compound with different clientids. Certainly > > no well-behaved client would do that, would it? (Hmm, that might be an > > interesting pynfs test, come to think of it). > > We could ask for a clarification in 3530bis if there's not already > something there that clearly forbids this, but I'm not sure if it's even > worth it. Or just handle it and be done with me. After all we'd just need to put the existing client, and store the new one in the cstate. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 3, 2014 at 12:11 PM, Christoph Hellwig <hch@infradead.org> wrote: > On Thu, Jul 03, 2014 at 11:32:08AM -0400, J. Bruce Fields wrote: >> > > > + struct nfs4_client *found; >> > > > + >> > > > + if (cstate->clp) { >> > > > + found = cstate->clp; >> > > > + if (!same_clid(&found->cl_clientid, clid)) >> > > > + return nfserr_stale_clientid; >> > > >> > > That's new behavior, isn't it? >> > > >> > >> > Yeah, I suppose it is, but it's hard to understand a valid use-case for >> > sending multiple ops in a compound with different clientids. Certainly >> > no well-behaved client would do that, would it? (Hmm, that might be an >> > interesting pynfs test, come to think of it). >> >> We could ask for a clarification in 3530bis if there's not already >> something there that clearly forbids this, but I'm not sure if it's even >> worth it. > > Or just handle it and be done with me. After all we'd just need to put > the existing client, and store the new one in the cstate. It is definitely not allowed in NFSv4.1. There is a special place in hell^W^W^Werrorcode NFS4ERR_SEQUENCE_POS that tells you that your compound is insane.
On Thu, 3 Jul 2014 09:11:40 -0700 Christoph Hellwig <hch@infradead.org> wrote: > On Thu, Jul 03, 2014 at 11:32:08AM -0400, J. Bruce Fields wrote: > > > > > + struct nfs4_client *found; > > > > > + > > > > > + if (cstate->clp) { > > > > > + found = cstate->clp; > > > > > + if (!same_clid(&found->cl_clientid, clid)) > > > > > + return nfserr_stale_clientid; > > > > > > > > That's new behavior, isn't it? > > > > > > > > > > Yeah, I suppose it is, but it's hard to understand a valid use-case for > > > sending multiple ops in a compound with different clientids. Certainly > > > no well-behaved client would do that, would it? (Hmm, that might be an > > > interesting pynfs test, come to think of it). > > > > We could ask for a clarification in 3530bis if there's not already > > something there that clearly forbids this, but I'm not sure if it's even > > worth it. > > Or just handle it and be done with me. After all we'd just need to put > the existing client, and store the new one in the cstate. > Meh, ok... I suppose there's no harm in doing that and since that's the behavior that was there before, we should probably go with it for the v4.0 case. I'll have the next iteration do that instead. Thanks,
On Mon, Jun 30, 2014 at 11:48:44AM -0400, Jeff Layton wrote: > We want to use the nfsd4_compound_state to cache the nfs4_client in > order to optimise away extra lookups of the clid. > > In the v4.0 case, we use this to ensure that we only have to look up the > client at most once per compound for each call into lookup_clientid. For > v4.1+ we set the pointer in the cstate during SEQUENCE processing so we > should never need to do a search for it. The connectathon locking test is failing for me in the nfsv4/krb5i case as of this commit. Which makes no sense to me whatsoever, so it's entirely possible this is some unrelated problem on my side. I'll let you know when I've figured out anything more. --b. > > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> > --- > fs/nfsd/nfs4state.c | 74 +++++++++++++++++++++++++++++++++++++---------------- > fs/nfsd/xdr4.h | 1 + > 2 files changed, 53 insertions(+), 22 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index e9bb39ecebae..4d162a0e4c6d 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -189,6 +189,15 @@ static void put_client_renew_locked(struct nfs4_client *clp) > renew_client_locked(clp); > } > > +static void put_client_renew(struct nfs4_client *clp) > +{ > + struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id); > + > + spin_lock(&nn->client_lock); > + put_client_renew_locked(clp); > + spin_unlock(&nn->client_lock); > +} > + > static __be32 nfsd4_get_session_locked(struct nfsd4_session *ses) > { > __be32 status; > @@ -2395,6 +2404,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, > goto out_put_session; > cstate->slot = slot; > cstate->session = session; > + cstate->clp = clp; > /* Return the cached reply status and set cstate->status > * for nfsd4_proc_compound processing */ > status = nfsd4_replay_cache_entry(resp, seq); > @@ -2429,6 +2439,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, > > cstate->slot = slot; > cstate->session = session; > + cstate->clp = clp; > > out: > switch (clp->cl_cb_state) { > @@ -2465,7 +2476,8 @@ nfsd4_sequence_done(struct nfsd4_compoundres *resp) > } > /* Drop session reference that was taken in nfsd4_sequence() */ > nfsd4_put_session(cs->session); > - } > + } else if (cs->clp) > + put_client_renew(cs->clp); > } > > __be32 > @@ -2997,6 +3009,38 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4 > return nfserr_bad_seqid; > } > > +static __be32 lookup_clientid(clientid_t *clid, > + struct nfsd4_compound_state *cstate, > + struct nfsd_net *nn) > +{ > + struct nfs4_client *found; > + > + if (cstate->clp) { > + found = cstate->clp; > + if (!same_clid(&found->cl_clientid, clid)) > + return nfserr_stale_clientid; > + return nfs_ok; > + } > + > + if (STALE_CLIENTID(clid, nn)) > + return nfserr_stale_clientid; > + > + /* > + * For v4.1+ we get the client in the SEQUENCE op. If we don't have one > + * cached already then we know this is for is for v4.0 and "sessions" > + * will be false. > + */ > + WARN_ON_ONCE(cstate->session); > + found = find_confirmed_client(clid, false, nn); > + if (!found) > + return nfserr_expired; > + > + /* Cache the nfs4_client in cstate! */ > + cstate->clp = found; > + atomic_inc(&found->cl_refcount); > + return nfs_ok; > +} > + > __be32 > nfsd4_process_open1(struct nfsd4_compound_state *cstate, > struct nfsd4_open *open, struct nfsd_net *nn) > @@ -3509,18 +3553,6 @@ void nfsd4_cleanup_open_state(struct nfsd4_open *open, __be32 status) > free_generic_stateid(open->op_stp); > } > > -static __be32 lookup_clientid(clientid_t *clid, bool session, struct nfsd_net *nn, struct nfs4_client **clp) > -{ > - struct nfs4_client *found; > - > - if (STALE_CLIENTID(clid, nn)) > - return nfserr_stale_clientid; > - found = find_confirmed_client(clid, session, nn); > - if (clp) > - *clp = found; > - return found ? nfs_ok : nfserr_expired; > -} > - > __be32 > nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > clientid_t *clid) > @@ -3532,9 +3564,10 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > nfs4_lock_state(); > dprintk("process_renew(%08x/%08x): starting\n", > clid->cl_boot, clid->cl_id); > - status = lookup_clientid(clid, cstate->minorversion, nn, &clp); > + status = lookup_clientid(clid, cstate, nn); > if (status) > goto out; > + clp = cstate->clp; > status = nfserr_cb_path_down; > if (!list_empty(&clp->cl_delegations) > && clp->cl_cb_state != NFSD4_CB_UP) > @@ -3797,22 +3830,19 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate, > stateid_t *stateid, unsigned char typemask, > struct nfs4_stid **s, struct nfsd_net *nn) > { > - struct nfs4_client *cl; > __be32 status; > - bool sessions = cstate->minorversion != 0; > > if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) > return nfserr_bad_stateid; > - status = lookup_clientid(&stateid->si_opaque.so_clid, sessions, > - nn, &cl); > + status = lookup_clientid(&stateid->si_opaque.so_clid, cstate, nn); > if (status == nfserr_stale_clientid) { > - if (sessions) > + if (cstate->session) > return nfserr_bad_stateid; > return nfserr_stale_stateid; > } > if (status) > return status; > - *s = find_stateid_by_type(cl, stateid, typemask); > + *s = find_stateid_by_type(cstate->clp, stateid, typemask); > if (!*s) > return nfserr_bad_stateid; > return nfs_ok; > @@ -4662,7 +4692,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > nfs4_lock_state(); > > if (!nfsd4_has_session(cstate)) { > - status = lookup_clientid(&lockt->lt_clientid, false, nn, NULL); > + status = lookup_clientid(&lockt->lt_clientid, cstate, nn); > if (status) > goto out; > } > @@ -4831,7 +4861,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, > > nfs4_lock_state(); > > - status = lookup_clientid(clid, cstate->minorversion, nn, NULL); > + status = lookup_clientid(clid, cstate, nn); > if (status) > goto out; > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > index 7d8af164523b..312f6483a48e 100644 > --- a/fs/nfsd/xdr4.h > +++ b/fs/nfsd/xdr4.h > @@ -55,6 +55,7 @@ struct nfsd4_compound_state { > struct svc_fh current_fh; > struct svc_fh save_fh; > struct nfs4_stateowner *replay_owner; > + struct nfs4_client *clp; > /* For sessions DRC */ > struct nfsd4_session *session; > struct nfsd4_slot *slot; > -- > 1.9.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 03, 2014 at 04:32:59PM -0400, J. Bruce Fields wrote: > On Mon, Jun 30, 2014 at 11:48:44AM -0400, Jeff Layton wrote: > > We want to use the nfsd4_compound_state to cache the nfs4_client in > > order to optimise away extra lookups of the clid. > > > > In the v4.0 case, we use this to ensure that we only have to look up the > > client at most once per compound for each call into lookup_clientid. For > > v4.1+ we set the pointer in the cstate during SEQUENCE processing so we > > should never need to do a search for it. > > The connectathon locking test is failing for me in the nfsv4/krb5i case > as of this commit. > > Which makes no sense to me whatsoever, so it's entirely possible this is > some unrelated problem on my side. I'll let you know when I've figured > out anything more. It's intermittent. I've reproduced it on the previous commit so I know at least that this one isn't at fault. I doubt it's really dependent on krb5i, at most that's probably just making it more likely to reproduce. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 3 Jul 2014 17:35:26 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Thu, Jul 03, 2014 at 04:32:59PM -0400, J. Bruce Fields wrote: > > On Mon, Jun 30, 2014 at 11:48:44AM -0400, Jeff Layton wrote: > > > We want to use the nfsd4_compound_state to cache the nfs4_client in > > > order to optimise away extra lookups of the clid. > > > > > > In the v4.0 case, we use this to ensure that we only have to look up the > > > client at most once per compound for each call into lookup_clientid. For > > > v4.1+ we set the pointer in the cstate during SEQUENCE processing so we > > > should never need to do a search for it. > > > > The connectathon locking test is failing for me in the nfsv4/krb5i case > > as of this commit. > > > > Which makes no sense to me whatsoever, so it's entirely possible this is > > some unrelated problem on my side. I'll let you know when I've figured > > out anything more. > > It's intermittent. > > I've reproduced it on the previous commit so I know at least that this > one isn't at fault. > > I doubt it's really dependent on krb5i, at most that's probably just > making it more likely to reproduce. > > --b. I haven't been able to reproduce it yet, but I suspect you're hitting this check in lookup_or_create_lock_state: /* with an existing lockowner, seqids must be the same */ status = nfserr_bad_seqid; if (!cstate->minorversion && lock->lk_new_lock_seqid != lo->lo_owner.so_seqid) goto out; Hmmm...there are some changes that go in in this patch wrt to lock seqid handling: nfsd: clean up lockowner refcounting when finding them Perhaps those need to go in earlier? Though when I looked at that originally, I figured that we wouldn't need those until the refcounting changes went in (which is why I didn't put them in). It might be interesting to look at traces and see whether they're consistent with hitting that check (or maybe put some debug printks in)?
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index e9bb39ecebae..4d162a0e4c6d 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -189,6 +189,15 @@ static void put_client_renew_locked(struct nfs4_client *clp) renew_client_locked(clp); } +static void put_client_renew(struct nfs4_client *clp) +{ + struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id); + + spin_lock(&nn->client_lock); + put_client_renew_locked(clp); + spin_unlock(&nn->client_lock); +} + static __be32 nfsd4_get_session_locked(struct nfsd4_session *ses) { __be32 status; @@ -2395,6 +2404,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, goto out_put_session; cstate->slot = slot; cstate->session = session; + cstate->clp = clp; /* Return the cached reply status and set cstate->status * for nfsd4_proc_compound processing */ status = nfsd4_replay_cache_entry(resp, seq); @@ -2429,6 +2439,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, cstate->slot = slot; cstate->session = session; + cstate->clp = clp; out: switch (clp->cl_cb_state) { @@ -2465,7 +2476,8 @@ nfsd4_sequence_done(struct nfsd4_compoundres *resp) } /* Drop session reference that was taken in nfsd4_sequence() */ nfsd4_put_session(cs->session); - } + } else if (cs->clp) + put_client_renew(cs->clp); } __be32 @@ -2997,6 +3009,38 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4 return nfserr_bad_seqid; } +static __be32 lookup_clientid(clientid_t *clid, + struct nfsd4_compound_state *cstate, + struct nfsd_net *nn) +{ + struct nfs4_client *found; + + if (cstate->clp) { + found = cstate->clp; + if (!same_clid(&found->cl_clientid, clid)) + return nfserr_stale_clientid; + return nfs_ok; + } + + if (STALE_CLIENTID(clid, nn)) + return nfserr_stale_clientid; + + /* + * For v4.1+ we get the client in the SEQUENCE op. If we don't have one + * cached already then we know this is for is for v4.0 and "sessions" + * will be false. + */ + WARN_ON_ONCE(cstate->session); + found = find_confirmed_client(clid, false, nn); + if (!found) + return nfserr_expired; + + /* Cache the nfs4_client in cstate! */ + cstate->clp = found; + atomic_inc(&found->cl_refcount); + return nfs_ok; +} + __be32 nfsd4_process_open1(struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct nfsd_net *nn) @@ -3509,18 +3553,6 @@ void nfsd4_cleanup_open_state(struct nfsd4_open *open, __be32 status) free_generic_stateid(open->op_stp); } -static __be32 lookup_clientid(clientid_t *clid, bool session, struct nfsd_net *nn, struct nfs4_client **clp) -{ - struct nfs4_client *found; - - if (STALE_CLIENTID(clid, nn)) - return nfserr_stale_clientid; - found = find_confirmed_client(clid, session, nn); - if (clp) - *clp = found; - return found ? nfs_ok : nfserr_expired; -} - __be32 nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, clientid_t *clid) @@ -3532,9 +3564,10 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, nfs4_lock_state(); dprintk("process_renew(%08x/%08x): starting\n", clid->cl_boot, clid->cl_id); - status = lookup_clientid(clid, cstate->minorversion, nn, &clp); + status = lookup_clientid(clid, cstate, nn); if (status) goto out; + clp = cstate->clp; status = nfserr_cb_path_down; if (!list_empty(&clp->cl_delegations) && clp->cl_cb_state != NFSD4_CB_UP) @@ -3797,22 +3830,19 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid, unsigned char typemask, struct nfs4_stid **s, struct nfsd_net *nn) { - struct nfs4_client *cl; __be32 status; - bool sessions = cstate->minorversion != 0; if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) return nfserr_bad_stateid; - status = lookup_clientid(&stateid->si_opaque.so_clid, sessions, - nn, &cl); + status = lookup_clientid(&stateid->si_opaque.so_clid, cstate, nn); if (status == nfserr_stale_clientid) { - if (sessions) + if (cstate->session) return nfserr_bad_stateid; return nfserr_stale_stateid; } if (status) return status; - *s = find_stateid_by_type(cl, stateid, typemask); + *s = find_stateid_by_type(cstate->clp, stateid, typemask); if (!*s) return nfserr_bad_stateid; return nfs_ok; @@ -4662,7 +4692,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, nfs4_lock_state(); if (!nfsd4_has_session(cstate)) { - status = lookup_clientid(&lockt->lt_clientid, false, nn, NULL); + status = lookup_clientid(&lockt->lt_clientid, cstate, nn); if (status) goto out; } @@ -4831,7 +4861,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, nfs4_lock_state(); - status = lookup_clientid(clid, cstate->minorversion, nn, NULL); + status = lookup_clientid(clid, cstate, nn); if (status) goto out; diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h index 7d8af164523b..312f6483a48e 100644 --- a/fs/nfsd/xdr4.h +++ b/fs/nfsd/xdr4.h @@ -55,6 +55,7 @@ struct nfsd4_compound_state { struct svc_fh current_fh; struct svc_fh save_fh; struct nfs4_stateowner *replay_owner; + struct nfs4_client *clp; /* For sessions DRC */ struct nfsd4_session *session; struct nfsd4_slot *slot;
We want to use the nfsd4_compound_state to cache the nfs4_client in order to optimise away extra lookups of the clid. In the v4.0 case, we use this to ensure that we only have to look up the client at most once per compound for each call into lookup_clientid. For v4.1+ we set the pointer in the cstate during SEQUENCE processing so we should never need to do a search for it. Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> --- fs/nfsd/nfs4state.c | 74 +++++++++++++++++++++++++++++++++++++---------------- fs/nfsd/xdr4.h | 1 + 2 files changed, 53 insertions(+), 22 deletions(-)