diff mbox

[v2,022/117] nfsd: Cache the client that was looked up in lookup_clientid()

Message ID 1403810017-16062-23-git-send-email-jlayton@primarydata.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jeff Layton June 26, 2014, 7:12 p.m. UTC 
From: Trond Myklebust <trond.myklebust@primarydata.com>

We want to use the nfsd4_compound_state to cache the nfs4_client
in order to optimise away extra lookups of the clid.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfsd/nfs4state.c | 44 +++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

Comments

Christoph Hellwig June 29, 2014, 12:14 p.m. UTC | #1 
On Thu, Jun 26, 2014 at 03:12:02PM -0400, Jeff Layton wrote:
> From: Trond Myklebust <trond.myklebust@primarydata.com>
> 
> We want to use the nfsd4_compound_state to cache the nfs4_client
> in order to optimise away extra lookups of the clid.

Should mention that this is only for 4.0.  Actually, kooking at
"nfsd: Allow struct nfsd4_compound_state to cache the nfs4_client" that
patch seems to mostly be a few random code cleanups that should have
a different title so it might make more sense to move the caching
of the client for 4.1+ to this patch as well.

And please write proper, detailed changelogs :)

> +static __be32 lookup_clientid(clientid_t *clid,
> +		struct nfsd4_compound_state *cstate,
> +		struct nfsd_net *nn)

Now that we don't look up the client idea for the normal case but just
verify it we should probably rename this function to
nfsd4_verify_clientid or something similar.

While it doesn't belong into this patch:  why don't we cache the
nfsd_net in the nfsd4_compound_state

>  {
>  	struct nfs4_client *found;
>  
> +	if (cstate->clp != NULL) {
> +		found = cstate->clp;
> +		if (!same_clid(&found->cl_clientid, clid))
> +			return nfserr_stale_clientid;
> +	} else {
> +		if (STALE_CLIENTID(clid, nn))
> +			return nfserr_stale_clientid;
> +		/*
> +		 * Usually for v4.1+ we get the client in the SEQUENCE op, so

Usually implices there might be a case where we don't get it.  Which
case would that be?

> +		 * if we don't have one cached already then we know this is for
> +		 * is for v4.0 and "sessions" will be false.
> +		 */
> +		found = find_confirmed_client(clid, false, nn);
> +		/* Cache the nfs4_client in cstate! */
> +		if (found) {
> +			cstate->clp = found;
> +			atomic_inc(&found->cl_refcount);
> +		}
> +	}
>  	return found ? nfs_ok : nfserr_expired;

The whole thing could be simplified a little more:

	if (cstate->clp) {
		if (!same_clid(&cstate->clp->cl_clientid, clid))
			return nfserr_stale_clientid;
		retur nfs_ok;
	}

	if (STALE_CLIENTID(clid, nn))
		return nfserr_stale_clientid;

	found = find_confirmed_client(clid, false, nn);
	if (!found)
		return nfserr_expired;

	cstate->clp = found;
	atomic_inc(&found->cl_refcount);
	return nfs_ok;

> +	status = lookup_clientid(&stateid->si_opaque.so_clid, cstate, nn);
>  	if (status == nfserr_stale_clientid) {
> +		if (cstate->session)
>  			return nfserr_bad_stateid;
>  		return nfserr_stale_stateid;
>  	}

Why do we return a different error here for the 4.1+ case?  And why not
in other places using lookup_clientid?

--
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
Jeff Layton June 29, 2014, 2:57 p.m. UTC | #2 
On Sun, 29 Jun 2014 05:14:19 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Thu, Jun 26, 2014 at 03:12:02PM -0400, Jeff Layton wrote:
> > From: Trond Myklebust <trond.myklebust@primarydata.com>
> > 
> > We want to use the nfsd4_compound_state to cache the nfs4_client
> > in order to optimise away extra lookups of the clid.
> 
> Should mention that this is only for 4.0.  Actually, kooking at
> "nfsd: Allow struct nfsd4_compound_state to cache the nfs4_client" that
> patch seems to mostly be a few random code cleanups that should have
> a different title so it might make more sense to move the caching
> of the client for 4.1+ to this patch as well.
> 
> And please write proper, detailed changelogs :)
> 

Ok, I'll reshuffle those patches and squash them together, and flesh
out the changelog a bit more.

> > +static __be32 lookup_clientid(clientid_t *clid,
> > +		struct nfsd4_compound_state *cstate,
> > +		struct nfsd_net *nn)
> 
> Now that we don't look up the client idea for the normal case but just
> verify it we should probably rename this function to
> nfsd4_verify_clientid or something similar.
> 

Depends on what you mean by "normal case". For v4.0 we do look it up at
least once for each call into here. For v4.1 we get the pointer to the
client "for free" by virtue of looking up the session in the SEQUENCE
op. So, I think we are doing a bit more than just verifying the
clientid here. The name seems appropriate to me.

> While it doesn't belong into this patch:  why don't we cache the
> nfsd_net in the nfsd4_compound_state
> 

I'll go further and say that it doesn't really belong in this series.

If we have a pointer to the client, then we automatically have one to
the struct net. We certainly could cache the nfsd_net pointer and spare
some calls to net_generic, but I'd prefer that that be done as a
separate series on top of this one if it's worthwhile. I this series
brings us enough changes as it is. :)

> >  {
> >  	struct nfs4_client *found;
> >  
> > +	if (cstate->clp != NULL) {
> > +		found = cstate->clp;
> > +		if (!same_clid(&found->cl_clientid, clid))
> > +			return nfserr_stale_clientid;
> > +	} else {
> > +		if (STALE_CLIENTID(clid, nn))
> > +			return nfserr_stale_clientid;
> > +		/*
> > +		 * Usually for v4.1+ we get the client in the SEQUENCE op, so
> 
> Usually implices there might be a case where we don't get it.  Which
> case would that be?
> 

There are certain calls that don't require a SEQUENCE operation:

    BIND_CONN_TO_SESSION
    EXCHANGE_ID
    CREATE_SESSION
    DESTROY_SESSION

Granted, for those we don't use lookup_clientid but the "Usually" still
applies, in general. That said, if it bothers you I'll remove it.

> > +		 * if we don't have one cached already then we know this is for
> > +		 * is for v4.0 and "sessions" will be false.
> > +		 */
> > +		found = find_confirmed_client(clid, false, nn);
> > +		/* Cache the nfs4_client in cstate! */
> > +		if (found) {
> > +			cstate->clp = found;
> > +			atomic_inc(&found->cl_refcount);
> > +		}
> > +	}
> >  	return found ? nfs_ok : nfserr_expired;
> 
> The whole thing could be simplified a little more:
> 
> 	if (cstate->clp) {
> 		if (!same_clid(&cstate->clp->cl_clientid, clid))
> 			return nfserr_stale_clientid;
> 		retur nfs_ok;
> 	}
> 
> 	if (STALE_CLIENTID(clid, nn))
> 		return nfserr_stale_clientid;
> 
> 	found = find_confirmed_client(clid, false, nn);
> 	if (!found)
> 		return nfserr_expired;
> 
> 	cstate->clp = found;
> 	atomic_inc(&found->cl_refcount);
> 	return nfs_ok;
> 

Ok, I'll look into that.

> > +	status = lookup_clientid(&stateid->si_opaque.so_clid, cstate, nn);
> >  	if (status == nfserr_stale_clientid) {
> > +		if (cstate->session)
> >  			return nfserr_bad_stateid;
> >  		return nfserr_stale_stateid;
> >  	}
> 
> Why do we return a different error here for the 4.1+ case?  And why not
> in other places using lookup_clientid?
> 

That change predates this set, but see commit a8a7c6776f8d7478.

nfsd4_lookup_stateid is trying to look up a specific stateid -- the
other places that call lookup_clientid aren't necessarily doing so.
Returning that error may not be applicable there.
Christoph Hellwig June 30, 2014, 10:34 a.m. UTC | #3 
On Sun, Jun 29, 2014 at 10:57:02AM -0400, Jeff Layton wrote:
> Depends on what you mean by "normal case". For v4.0 we do look it up at
> least once for each call into here. For v4.1 we get the pointer to the
> client "for free" by virtue of looking up the session in the SEQUENCE
> op. So, I think we are doing a bit more than just verifying the
> clientid here. The name seems appropriate to me.

So everything but the first call for 4.0 is just a small verification.
I'm not going to hold up the series for something as trivial, but the
name still smells a little..

> > While it doesn't belong into this patch:  why don't we cache the
> > nfsd_net in the nfsd4_compound_state
> > 
> 
> I'll go further and say that it doesn't really belong in this series.

Anything we can keep out of this series is good, agreed..

> There are certain calls that don't require a SEQUENCE operation:
> 
>     BIND_CONN_TO_SESSION
>     EXCHANGE_ID
>     CREATE_SESSION
>     DESTROY_SESSION
> 
> Granted, for those we don't use lookup_clientid but the "Usually" still
> applies, in general. That said, if it bothers you I'll remove it.

my major issue is the combination of the usually with this:

> > > +		 * if we don't have one cached already then we know this is for
> > > +		 * is for v4.0 and "sessions" will be false.

so we can have a case where we are 4.1 but don't have a cached session.
This is getting into deep nitpick land, but comments that aren't
entirely coherent always confuse me.

--
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
Jeff Layton June 30, 2014, 10:59 a.m. UTC | #4 
On Mon, 30 Jun 2014 03:34:53 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Sun, Jun 29, 2014 at 10:57:02AM -0400, Jeff Layton wrote:
> > Depends on what you mean by "normal case". For v4.0 we do look it up at
> > least once for each call into here. For v4.1 we get the pointer to the
> > client "for free" by virtue of looking up the session in the SEQUENCE
> > op. So, I think we are doing a bit more than just verifying the
> > clientid here. The name seems appropriate to me.
> 
> So everything but the first call for 4.0 is just a small verification.
> I'm not going to hold up the series for something as trivial, but the
> name still smells a little..
> 

Fair enough. The "verify" name smells to me a little since we aren't
guaranteed to have one when the function is called. "lookup" seems
reasonable since we are looking it up, it's just that once this patch
goes in we'll usually have it cached in the cstate.

> > > While it doesn't belong into this patch:  why don't we cache the
> > > nfsd_net in the nfsd4_compound_state
> > > 
> > 
> > I'll go further and say that it doesn't really belong in this
> > series.
> 
> Anything we can keep out of this series is good, agreed..
> 
> > There are certain calls that don't require a SEQUENCE operation:
> > 
> >     BIND_CONN_TO_SESSION
> >     EXCHANGE_ID
> >     CREATE_SESSION
> >     DESTROY_SESSION
> > 
> > Granted, for those we don't use lookup_clientid but the "Usually"
> > still applies, in general. That said, if it bothers you I'll remove
> > it.
> 
> my major issue is the combination of the usually with this:
> 
> > > > +		 * if we don't have one cached already then we
> > > > know this is for
> > > > +		 * is for v4.0 and "sessions" will be false.
> 
> so we can have a case where we are 4.1 but don't have a cached
> session. This is getting into deep nitpick land, but comments that
> aren't entirely coherent always confuse me.
> 

Ok, that's understandable. How about this?

+       /*
+        * 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.
+        */
+       found = find_confirmed_client(clid, false, nn);

FWIW, the main reason for the comment is to explain why we're
hardcoding the "session" arg to false here instead of using
cstate->session.
Christoph Hellwig June 30, 2014, 11:03 a.m. UTC | #5 
On Mon, Jun 30, 2014 at 06:59:13AM -0400, Jeff Layton wrote:
> +       /*
> +        * 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.
> +        */
> +       found = find_confirmed_client(clid, false, nn);
> 
> FWIW, the main reason for the comment is to explain why we're
> hardcoding the "session" arg to false here instead of using
> cstate->session.

I'd either just use it to future proof it, or add an assert that it's
really not set..

--
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
Jeff Layton June 30, 2014, 11:23 a.m. UTC | #6 
On Mon, 30 Jun 2014 04:03:22 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Jun 30, 2014 at 06:59:13AM -0400, Jeff Layton wrote:
> > +       /*
> > +        * 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.
> > +        */
> > +       found = find_confirmed_client(clid, false, nn);
> > 
> > FWIW, the main reason for the comment is to explain why we're
> > hardcoding the "session" arg to false here instead of using
> > cstate->session.
> 
> I'd either just use it to future proof it, or add an assert that it's
> really not set..
> 

An assertion sounds like a good idea. I'll add one in there...
diff mbox

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f782ed5cad63..fb78d1ab2655 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3521,15 +3521,31 @@  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)
+static __be32 lookup_clientid(clientid_t *clid,
+		struct nfsd4_compound_state *cstate,
+		struct nfsd_net *nn)
 {
 	struct nfs4_client *found;
 
-	if (STALE_CLIENTID(clid, nn))
-		return nfserr_stale_clientid;
-	found = find_confirmed_client(clid, session, nn);
-	if (clp)
-		*clp = found;
+	if (cstate->clp != NULL) {
+		found = cstate->clp;
+		if (!same_clid(&found->cl_clientid, clid))
+			return nfserr_stale_clientid;
+	} else {
+		if (STALE_CLIENTID(clid, nn))
+			return nfserr_stale_clientid;
+		/*
+		 * Usually for v4.1+ we get the client in the SEQUENCE op, so
+		 * if we don't have one cached already then we know this is for
+		 * is for v4.0 and "sessions" will be false.
+		 */
+		found = find_confirmed_client(clid, false, nn);
+		/* Cache the nfs4_client in cstate! */
+		if (found) {
+			cstate->clp = found;
+			atomic_inc(&found->cl_refcount);
+		}
+	}
 	return found ? nfs_ok : nfserr_expired;
 }
 
@@ -3544,9 +3560,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)
@@ -3809,22 +3826,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;
@@ -4674,7 +4688,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;
 	}
@@ -4844,7 +4858,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;