On Wed, May 12, 2010 at 07:26:46AM +0300, Benny Halevy wrote:
> On May. 12, 2010, 5:40 +0300, " J. Bruce Fields" <bfields@xxxxxxxxxxxxxx> wrote:
> > Something still doesn't look quite right: a sequence operation
> > increments cl_count from 1 to 2; then, say, an exchangeid in another
> > thread expires the client, dropping cl_count from 2 to 1; then the
> > laundromat runs, sees cl_count 1, and decides it can expire the
> > client. Meanwhile, the original compound is still running.
> >
> > Am I missing something?
>
> Yeah. exchange_id doesn't touch cl_refcount. It may call expire_client
> explicitly which will unhash the client but will not destroy it if cl_refcount > 0
> the laundromat won't the client either after that since it's not on the lru list
> any more (and even if it would, it's refcount is still great than zero so it would
> have been ignored)
Right, that was the whole point. Thanks!
--b.
>
> Benny
>
> >
> > --b.
> >
> > On Wed, May 12, 2010 at 12:13:54AM +0300, Benny Halevy wrote:
> >> Get a refcount on the client on SEQUENCE,
> >> Release the refcount and renew the client when all respective compounds completed.
> >> Do not expire the client by the laundromat while in use.
> >> If the client was expired via another path, free it when the compounds
> >> complete and the refcount reaches 0.
> >>
> >> Note that unhash_client_locked must call list_del_init on cl_lru as
> >> it may be called twice for the same client (once from nfs4_laundromat
> >> and then from expire_client)
> >>
> >> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
> >> ---
> >> fs/nfsd/nfs4state.c | 27 ++++++++++++++++++++++++---
> >> fs/nfsd/nfs4xdr.c | 3 ++-
> >> fs/nfsd/state.h | 1 +
> >> 3 files changed, 27 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index 378ee86..1a8fb39 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -701,6 +701,22 @@ free_client(struct nfs4_client *clp)
> >> kfree(clp);
> >> }
> >>
> >> +void
> >> +release_session_client(struct nfsd4_session *session)
> >> +{
> >> + struct nfs4_client *clp = session->se_client;
> >> +
> >> + spin_lock(&client_lock);
> >> + BUG_ON(clp->cl_refcount <= 0);
> >> + if (--clp->cl_refcount <= 0) {
> >> + free_client(clp);
> >> + session->se_client = NULL;
> >> + } else if (clp->cl_refcount == 1)
> >> + renew_client_locked(clp);
> >> + spin_unlock(&client_lock);
> >> + nfsd4_put_session(session);
> >> +}
> >> +
> >> /* must be called under the client_lock */
> >> static inline void
> >> unhash_client_locked(struct nfs4_client *clp)
> >> @@ -1477,8 +1493,7 @@ out:
> >> /* Hold a session reference until done processing the compound. */
> >> if (cstate->session) {
> >> nfsd4_get_session(cstate->session);
> >> - /* Renew the clientid on success and on replay */
> >> - renew_client_locked(session->se_client);
> >> + session->se_client->cl_refcount++;
> >> }
> >> spin_unlock(&client_lock);
> >> dprintk("%s: return %d\n", __func__, ntohl(status));
> >> @@ -2599,7 +2614,13 @@ nfs4_laundromat(void)
> >> clientid_val = t;
> >> break;
> >> }
> >> - list_move(&clp->cl_lru, &reaplist);
> >> + if (clp->cl_refcount > 1) {
> >> + dprintk("NFSD: client in use (clientid %08x)\n",
> >> + clp->cl_clientid.cl_id);
> >> + continue;
> >> + }
> >> + unhash_client_locked(clp);
> >> + list_add(&clp->cl_lru, &reaplist);
> >> }
> >> spin_unlock(&client_lock);
> >> list_for_each_safe(pos, next, &reaplist) {
> >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> >> index 5c2de47..126d0ca 100644
> >> --- a/fs/nfsd/nfs4xdr.c
> >> +++ b/fs/nfsd/nfs4xdr.c
> >> @@ -3313,7 +3313,8 @@ nfs4svc_encode_compoundres(struct svc_rqst *rqstp, __be32 *p, struct nfsd4_compo
> >> dprintk("%s: SET SLOT STATE TO AVAILABLE\n", __func__);
> >> cs->slot->sl_inuse = false;
> >> }
> >> - nfsd4_put_session(cs->session);
> >> + /* Renew the clientid on success and on replay */
> >> + release_session_client(cs->session);
> >> }
> >> return 1;
> >> }
> >> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> >> index 37e1dff..f263174 100644
> >> --- a/fs/nfsd/state.h
> >> +++ b/fs/nfsd/state.h
> >> @@ -419,6 +419,7 @@ extern int nfs4_has_reclaimed_state(const char *name, bool use_exchange_id);
> >> extern void nfsd4_recdir_purge_old(void);
> >> extern int nfsd4_create_clid_dir(struct nfs4_client *clp);
> >> extern void nfsd4_remove_clid_dir(struct nfs4_client *clp);
> >> +extern void release_session_client(struct nfsd4_session *);
> >>
> >> static inline void
> >> nfs4_put_stateowner(struct nfs4_stateowner *so)
> >> --
> >> 1.6.5.1
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
