Bruce, the following patchset compiles but is not fully tested yet.
It is based on top of http://marc.info/?l=linux-nfs&m=127288934008791&w=2
Please review.
Thanks,
Benny
[RFC 1/3] nfsd4: use local variable in nfs4svc_encode_compoundres
[RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session
[RFC 3/3] nfsd4: do not expire nfs41 clients while in use
'cs' is already computed, re-use it.
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4xdr.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index b04583c..19ff5a3 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3311,9 +3311,9 @@ nfs4svc_encode_compoundres(struct svc_rqst *rqstp, __be32 *p, struct nfsd4_compo
if (cs->status != nfserr_replay_cache) {
nfsd4_store_cache_entry(resp);
dprintk("%s: SET SLOT STATE TO AVAILABLE\n", __func__);
- resp->cstate.slot->sl_inuse = false;
+ cs->slot->sl_inuse = false;
}
- nfsd4_put_session(resp->cstate.session);
+ nfsd4_put_session(cs->session);
}
return 1;
}
--
1.6.3.3
Although expire_client unhashes the session form the session table
so no new compounds can find it, there's no refcount to keep the
nfs4_client structure around while it's in use and referenced
in the compound state via the session structure.
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4callback.c | 2 +-
fs/nfsd/nfs4state.c | 4 +++-
fs/nfsd/nfs4xdr.c | 1 +
fs/nfsd/state.h | 6 ++++++
4 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 7e32bd3..fef1dbe 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -571,7 +571,7 @@ nfsd4_probe_callback(struct nfs4_client *clp)
}
/* the task holds a reference to the nfs4_client struct */
- atomic_inc(&clp->cl_count);
+ get_nfs4_client(clp);
do_probe_callback(clp);
}
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6dbcaf1..50b75af 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1458,8 +1458,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
out:
/* Hold a session reference until done processing the compound. */
- if (cstate->session)
+ if (cstate->session) {
nfsd4_get_session(cstate->session);
+ get_nfs4_client(cstate->session->se_client);
+ }
spin_unlock(&sessionid_lock);
/* Renew the clientid on success and on replay */
if (cstate->session) {
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 19ff5a3..aed733c 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3313,6 +3313,7 @@ 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;
}
+ put_nfs4_client(cs->session->se_client);
nfsd4_put_session(cs->session);
}
return 1;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index fefeae2..e3c002e 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -231,6 +231,12 @@ struct nfs4_client {
/* wait here for slots */
};
+static inline void
+get_nfs4_client(struct nfs4_client *clp)
+{
+ atomic_inc(&clp->cl_count);
+}
+
/* struct nfs4_client_reset
* one per old client. Populates reset_str_hashtbl. Filled from conf_id_hashtbl
* upon lease reset, or from upcall to state_daemon (to read in state
--
1.6.3.3
Add a cl_use_count atomic counter to struct nfs4_client.
Hold a use count while the client is in use by a compound that begins
with SEQUENCE.
Renew the client when the comound completes.
The laundromat skips clients that have a positive use count.
Otherwise, the laundromat marks the client as expired by setting
cl_use_count to -1.
Note that we don't want to use the state lock to synchronize with nfsd4_sequence
as we want to diminish the state lock scope.
An alternative to using atomic_cmpxchg is to grab the sessionid_lock spin lock
while accessing cl_use_count, but that would cause some contention in the
laundromat if it needs to lock and unlock it for every client.
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4state.c | 40 ++++++++++++++++++++++++++++++++++------
fs/nfsd/nfs4xdr.c | 2 ++
fs/nfsd/state.h | 9 +++++++++
3 files changed, 45 insertions(+), 6 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 50b75af..c95ddca 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -647,6 +647,14 @@ renew_client(struct nfs4_client *clp)
clp->cl_time = get_seconds();
}
+void
+renew_nfs4_client(struct nfs4_client *clp)
+{
+ nfs4_lock_state();
+ renew_client(clp);
+ nfs4_unlock_state();
+}
+
/* SETCLIENTID and SETCLIENTID_CONFIRM Helper functions */
static int
STALE_CLIENTID(clientid_t *clid)
@@ -715,6 +723,24 @@ put_nfs4_client(struct nfs4_client *clp)
free_client(clp);
}
+/*
+ * atomically mark client as used, as long as it's not already expired.
+ * return 0 on success, or a negative value if client already expired.
+ */
+int
+use_nfs4_client(struct nfs4_client *clp)
+{
+ int orig;
+
+ do {
+ orig = atomic_read(&clp->cl_use_count);
+ if (orig < 0)
+ return orig;
+ } while (atomic_cmpxchg(&clp->cl_use_count, orig, orig + 1) != orig);
+
+ return 0;
+}
+
static void
expire_client(struct nfs4_client *clp)
{
@@ -840,6 +866,7 @@ static struct nfs4_client *create_client(struct xdr_netobj name, char *recdir,
memcpy(clp->cl_recdir, recdir, HEXDIR_LEN);
atomic_set(&clp->cl_count, 1);
+ atomic_set(&clp->cl_use_count, 0);
atomic_set(&clp->cl_cb_conn.cb_set, 0);
INIT_LIST_HEAD(&clp->cl_idhash);
INIT_LIST_HEAD(&clp->cl_strhash);
@@ -1423,6 +1450,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
if (!session)
goto out;
+ /* keep the client from expiring */
+ if (use_nfs4_client(cstate->session->se_client) < 0)
+ goto out;
+
status = nfserr_badslot;
if (seq->slotid >= session->se_fchannel.maxreqs)
goto out;
@@ -1463,12 +1494,6 @@ out:
get_nfs4_client(cstate->session->se_client);
}
spin_unlock(&sessionid_lock);
- /* Renew the clientid on success and on replay */
- if (cstate->session) {
- nfs4_lock_state();
- renew_client(session->se_client);
- nfs4_unlock_state();
- }
dprintk("%s: return %d\n", __func__, ntohl(status));
return status;
}
@@ -2575,6 +2600,9 @@ nfs4_laundromat(void)
nfsd4_end_grace();
list_for_each_safe(pos, next, &client_lru) {
clp = list_entry(pos, struct nfs4_client, cl_lru);
+ /* skip client if in use (cl_use_count is greater than zero) */
+ if (atomic_cmpxchg(&clp->cl_use_count, 0, -1) > 0)
+ continue;
if (time_after((unsigned long)clp->cl_time, (unsigned long)cutoff)) {
t = clp->cl_time - cutoff;
if (clientid_val > t)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index aed733c..0f0b857 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3313,6 +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;
}
+ renew_nfs4_client(cs->session->se_client);
+ unuse_nfs4_client(cs->session->se_client);
put_nfs4_client(cs->session->se_client);
nfsd4_put_session(cs->session);
}
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index e3c002e..806d9b8 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -214,6 +214,7 @@ struct nfs4_client {
nfs4_verifier cl_confirm; /* generated by server */
struct nfs4_cb_conn cl_cb_conn; /* callback info */
atomic_t cl_count; /* ref count */
+ atomic_t cl_use_count; /* session use count */
u32 cl_firststate; /* recovery dir creation */
/* for nfs41 */
@@ -237,6 +238,12 @@ get_nfs4_client(struct nfs4_client *clp)
atomic_inc(&clp->cl_count);
}
+static inline void
+unuse_nfs4_client(struct nfs4_client *clp)
+{
+ atomic_dec(&clp->cl_use_count);
+}
+
/* struct nfs4_client_reset
* one per old client. Populates reset_str_hashtbl. Filled from conf_id_hashtbl
* upon lease reset, or from upcall to state_daemon (to read in state
@@ -384,6 +391,8 @@ extern void nfs4_unlock_state(void);
extern int nfs4_in_grace(void);
extern __be32 nfs4_check_open_reclaim(clientid_t *clid);
extern void put_nfs4_client(struct nfs4_client *clp);
+extern int use_nfs4_client(struct nfs4_client *clp);
+extern void renew_nfs4_client(struct nfs4_client *clp);
extern void nfs4_free_stateowner(struct kref *kref);
extern int set_callback_cred(void);
extern void nfsd4_probe_callback(struct nfs4_client *clp);
--
1.6.3.3
On Mon, May 03, 2010 at 07:31:48PM +0300, Benny Halevy wrote:
> Although expire_client unhashes the session form the session table
> so no new compounds can find it, there's no refcount to keep the
> nfs4_client structure around while it's in use and referenced
> in the compound state via the session structure.
The code in my for-2.6.35 branch already has the cl_count removed (and
doesn't use it for callbacks any more, instead destroying callbacks
before the client is destroyed).
So we need to add a new usage count. I'd prefer to call it something
different, since it's being used for something different (cl_users?)
since it's being used for something different, and I'd rather avoid
confusion with the previous one.
--b.
>
> Signed-off-by: Benny Halevy <[email protected]>
> ---
> fs/nfsd/nfs4callback.c | 2 +-
> fs/nfsd/nfs4state.c | 4 +++-
> fs/nfsd/nfs4xdr.c | 1 +
> fs/nfsd/state.h | 6 ++++++
> 4 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 7e32bd3..fef1dbe 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -571,7 +571,7 @@ nfsd4_probe_callback(struct nfs4_client *clp)
> }
>
> /* the task holds a reference to the nfs4_client struct */
> - atomic_inc(&clp->cl_count);
> + get_nfs4_client(clp);
>
> do_probe_callback(clp);
> }
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 6dbcaf1..50b75af 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1458,8 +1458,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>
> out:
> /* Hold a session reference until done processing the compound. */
> - if (cstate->session)
> + if (cstate->session) {
> nfsd4_get_session(cstate->session);
> + get_nfs4_client(cstate->session->se_client);
> + }
> spin_unlock(&sessionid_lock);
> /* Renew the clientid on success and on replay */
> if (cstate->session) {
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 19ff5a3..aed733c 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3313,6 +3313,7 @@ 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;
> }
> + put_nfs4_client(cs->session->se_client);
> nfsd4_put_session(cs->session);
> }
> return 1;
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index fefeae2..e3c002e 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -231,6 +231,12 @@ struct nfs4_client {
> /* wait here for slots */
> };
>
> +static inline void
> +get_nfs4_client(struct nfs4_client *clp)
> +{
> + atomic_inc(&clp->cl_count);
> +}
> +
> /* struct nfs4_client_reset
> * one per old client. Populates reset_str_hashtbl. Filled from conf_id_hashtbl
> * upon lease reset, or from upcall to state_daemon (to read in state
> --
> 1.6.3.3
>
On Mon, May 03, 2010 at 07:31:33PM +0300, Benny Halevy wrote:
> 'cs' is already computed, re-use it.
Thanks, applied.
--b.
>
> Signed-off-by: Benny Halevy <[email protected]>
> ---
> fs/nfsd/nfs4xdr.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index b04583c..19ff5a3 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3311,9 +3311,9 @@ nfs4svc_encode_compoundres(struct svc_rqst *rqstp, __be32 *p, struct nfsd4_compo
> if (cs->status != nfserr_replay_cache) {
> nfsd4_store_cache_entry(resp);
> dprintk("%s: SET SLOT STATE TO AVAILABLE\n", __func__);
> - resp->cstate.slot->sl_inuse = false;
> + cs->slot->sl_inuse = false;
> }
> - nfsd4_put_session(resp->cstate.session);
> + nfsd4_put_session(cs->session);
> }
> return 1;
> }
> --
> 1.6.3.3
>
On Mon, May 03, 2010 at 06:36:20PM -0400, J. Bruce Fields wrote:
> On Mon, May 03, 2010 at 07:31:48PM +0300, Benny Halevy wrote:
> > Although expire_client unhashes the session form the session table
> > so no new compounds can find it, there's no refcount to keep the
> > nfs4_client structure around while it's in use and referenced
> > in the compound state via the session structure.
>
> The code in my for-2.6.35 branch already has the cl_count removed (and
> doesn't use it for callbacks any more, instead destroying callbacks
> before the client is destroyed).
>
> So we need to add a new usage count.
(Which you do in the next patch, OK!)
--b.
> I'd prefer to call it something
> different, since it's being used for something different (cl_users?)
> since it's being used for something different, and I'd rather avoid
> confusion with the previous one.
>
> --b.
>
>
>
> >
> > Signed-off-by: Benny Halevy <[email protected]>
> > ---
> > fs/nfsd/nfs4callback.c | 2 +-
> > fs/nfsd/nfs4state.c | 4 +++-
> > fs/nfsd/nfs4xdr.c | 1 +
> > fs/nfsd/state.h | 6 ++++++
> > 4 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > index 7e32bd3..fef1dbe 100644
> > --- a/fs/nfsd/nfs4callback.c
> > +++ b/fs/nfsd/nfs4callback.c
> > @@ -571,7 +571,7 @@ nfsd4_probe_callback(struct nfs4_client *clp)
> > }
> >
> > /* the task holds a reference to the nfs4_client struct */
> > - atomic_inc(&clp->cl_count);
> > + get_nfs4_client(clp);
> >
> > do_probe_callback(clp);
> > }
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 6dbcaf1..50b75af 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1458,8 +1458,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> >
> > out:
> > /* Hold a session reference until done processing the compound. */
> > - if (cstate->session)
> > + if (cstate->session) {
> > nfsd4_get_session(cstate->session);
> > + get_nfs4_client(cstate->session->se_client);
> > + }
> > spin_unlock(&sessionid_lock);
> > /* Renew the clientid on success and on replay */
> > if (cstate->session) {
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index 19ff5a3..aed733c 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -3313,6 +3313,7 @@ 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;
> > }
> > + put_nfs4_client(cs->session->se_client);
> > nfsd4_put_session(cs->session);
> > }
> > return 1;
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index fefeae2..e3c002e 100644
> > --- a/fs/nfsd/state.h
> > +++ b/fs/nfsd/state.h
> > @@ -231,6 +231,12 @@ struct nfs4_client {
> > /* wait here for slots */
> > };
> >
> > +static inline void
> > +get_nfs4_client(struct nfs4_client *clp)
> > +{
> > + atomic_inc(&clp->cl_count);
> > +}
> > +
> > /* struct nfs4_client_reset
> > * one per old client. Populates reset_str_hashtbl. Filled from conf_id_hashtbl
> > * upon lease reset, or from upcall to state_daemon (to read in state
> > --
> > 1.6.3.3
> >
Benny Halevy wrote:
> Add a cl_use_count atomic counter to struct nfs4_client.
> Hold a use count while the client is in use by a compound that begins
> with SEQUENCE.
> Renew the client when the comound completes.
> The laundromat skips clients that have a positive use count.
> Otherwise, the laundromat marks the client as expired by setting
> cl_use_count to -1.
>
> Note that we don't want to use the state lock to synchronize with nfsd4_sequence
> as we want to diminish the state lock scope.
> An alternative to using atomic_cmpxchg is to grab the sessionid_lock spin lock
> while accessing cl_use_count, but that would cause some contention in the
> laundromat if it needs to lock and unlock it for every client.
>
> Signed-off-by: Benny Halevy <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 40 ++++++++++++++++++++++++++++++++++------
> fs/nfsd/nfs4xdr.c | 2 ++
> fs/nfsd/state.h | 9 +++++++++
> 3 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 50b75af..c95ddca 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -647,6 +647,14 @@ renew_client(struct nfs4_client *clp)
> clp->cl_time = get_seconds();
> }
>
> +void
> +renew_nfs4_client(struct nfs4_client *clp)
> +{
> + nfs4_lock_state();
> + renew_client(clp);
> + nfs4_unlock_state();
> +}
> +
> /* SETCLIENTID and SETCLIENTID_CONFIRM Helper functions */
> static int
> STALE_CLIENTID(clientid_t *clid)
> @@ -715,6 +723,24 @@ put_nfs4_client(struct nfs4_client *clp)
> free_client(clp);
> }
>
> +/*
> + * atomically mark client as used, as long as it's not already expired.
> + * return 0 on success, or a negative value if client already expired.
> + */
> +int
> +use_nfs4_client(struct nfs4_client *clp)
> +{
> + int orig;
> +
> + do {
> + orig = atomic_read(&clp->cl_use_count);
> + if (orig < 0)
> + return orig;
> + } while (atomic_cmpxchg(&clp->cl_use_count, orig, orig + 1) != orig);
> +
> + return 0;
> +}
> +
> static void
> expire_client(struct nfs4_client *clp)
> {
> @@ -840,6 +866,7 @@ static struct nfs4_client *create_client(struct xdr_netobj name, char *recdir,
>
> memcpy(clp->cl_recdir, recdir, HEXDIR_LEN);
> atomic_set(&clp->cl_count, 1);
> + atomic_set(&clp->cl_use_count, 0);
> atomic_set(&clp->cl_cb_conn.cb_set, 0);
> INIT_LIST_HEAD(&clp->cl_idhash);
> INIT_LIST_HEAD(&clp->cl_strhash);
> @@ -1423,6 +1450,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> if (!session)
> goto out;
>
> + /* keep the client from expiring */
> + if (use_nfs4_client(cstate->session->se_client) < 0)
> + goto out;
> +
> status = nfserr_badslot;
> if (seq->slotid >= session->se_fchannel.maxreqs)
> goto out;
> @@ -1463,12 +1494,6 @@ out:
> get_nfs4_client(cstate->session->se_client);
> }
> spin_unlock(&sessionid_lock);
> - /* Renew the clientid on success and on replay */
> - if (cstate->session) {
> - nfs4_lock_state();
> - renew_client(session->se_client);
> - nfs4_unlock_state();
> - }
> dprintk("%s: return %d\n", __func__, ntohl(status));
> return status;
> }
> @@ -2575,6 +2600,9 @@ nfs4_laundromat(void)
> nfsd4_end_grace();
> list_for_each_safe(pos, next, &client_lru) {
> clp = list_entry(pos, struct nfs4_client, cl_lru);
> + /* skip client if in use (cl_use_count is greater than zero) */
> + if (atomic_cmpxchg(&clp->cl_use_count, 0, -1) > 0)
> + continue;
I'll move that check after the time comparison
since there's no need for the atomic instruction if the client's
time out hasn't expired yet.
Benny
> if (time_after((unsigned long)clp->cl_time, (unsigned long)cutoff)) {
> t = clp->cl_time - cutoff;
> if (clientid_val > t)
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index aed733c..0f0b857 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3313,6 +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;
> }
> + renew_nfs4_client(cs->session->se_client);
> + unuse_nfs4_client(cs->session->se_client);
> put_nfs4_client(cs->session->se_client);
> nfsd4_put_session(cs->session);
> }
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index e3c002e..806d9b8 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -214,6 +214,7 @@ struct nfs4_client {
> nfs4_verifier cl_confirm; /* generated by server */
> struct nfs4_cb_conn cl_cb_conn; /* callback info */
> atomic_t cl_count; /* ref count */
> + atomic_t cl_use_count; /* session use count */
> u32 cl_firststate; /* recovery dir creation */
>
> /* for nfs41 */
> @@ -237,6 +238,12 @@ get_nfs4_client(struct nfs4_client *clp)
> atomic_inc(&clp->cl_count);
> }
>
> +static inline void
> +unuse_nfs4_client(struct nfs4_client *clp)
> +{
> + atomic_dec(&clp->cl_use_count);
> +}
> +
> /* struct nfs4_client_reset
> * one per old client. Populates reset_str_hashtbl. Filled from conf_id_hashtbl
> * upon lease reset, or from upcall to state_daemon (to read in state
> @@ -384,6 +391,8 @@ extern void nfs4_unlock_state(void);
> extern int nfs4_in_grace(void);
> extern __be32 nfs4_check_open_reclaim(clientid_t *clid);
> extern void put_nfs4_client(struct nfs4_client *clp);
> +extern int use_nfs4_client(struct nfs4_client *clp);
> +extern void renew_nfs4_client(struct nfs4_client *clp);
> extern void nfs4_free_stateowner(struct kref *kref);
> extern int set_callback_cred(void);
> extern void nfsd4_probe_callback(struct nfs4_client *clp);
On May. 04, 2010, 4:12 +0300, " J. Bruce Fields" <[email protected]> wrote:
> On Mon, May 03, 2010 at 06:36:20PM -0400, J. Bruce Fields wrote:
>> On Mon, May 03, 2010 at 07:31:48PM +0300, Benny Halevy wrote:
>>> Although expire_client unhashes the session form the session table
>>> so no new compounds can find it, there's no refcount to keep the
>>> nfs4_client structure around while it's in use and referenced
>>> in the compound state via the session structure.
>> The code in my for-2.6.35 branch already has the cl_count removed (and
>> doesn't use it for callbacks any more, instead destroying callbacks
>> before the client is destroyed).
>>
>> So we need to add a new usage count.
>
> (Which you do in the next patch, OK!)
Right :)
This patch fixes another race in which a client can be expired using
expire_client(), not from the laundromat path, while it's being referenced
by the session since we look up the session while holding just the sessionid
lock and not the state lock. Therefore we must take a refcount on the
client while inside the sessionid lock.
Looks like the new usage count in your for-2.6.35 world (that I _think_
could just be a kref now) is needed for delegations as well, right?
Benny
>
> --b.
>
>> I'd prefer to call it something
>> different, since it's being used for something different (cl_users?)
>> since it's being used for something different, and I'd rather avoid
>> confusion with the previous one.
>>
>> --b.
>>
>>
>>
>>> Signed-off-by: Benny Halevy <[email protected]>
>>> ---
>>> fs/nfsd/nfs4callback.c | 2 +-
>>> fs/nfsd/nfs4state.c | 4 +++-
>>> fs/nfsd/nfs4xdr.c | 1 +
>>> fs/nfsd/state.h | 6 ++++++
>>> 4 files changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
>>> index 7e32bd3..fef1dbe 100644
>>> --- a/fs/nfsd/nfs4callback.c
>>> +++ b/fs/nfsd/nfs4callback.c
>>> @@ -571,7 +571,7 @@ nfsd4_probe_callback(struct nfs4_client *clp)
>>> }
>>>
>>> /* the task holds a reference to the nfs4_client struct */
>>> - atomic_inc(&clp->cl_count);
>>> + get_nfs4_client(clp);
>>>
>>> do_probe_callback(clp);
>>> }
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 6dbcaf1..50b75af 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -1458,8 +1458,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>>>
>>> out:
>>> /* Hold a session reference until done processing the compound. */
>>> - if (cstate->session)
>>> + if (cstate->session) {
>>> nfsd4_get_session(cstate->session);
>>> + get_nfs4_client(cstate->session->se_client);
>>> + }
>>> spin_unlock(&sessionid_lock);
>>> /* Renew the clientid on success and on replay */
>>> if (cstate->session) {
>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>> index 19ff5a3..aed733c 100644
>>> --- a/fs/nfsd/nfs4xdr.c
>>> +++ b/fs/nfsd/nfs4xdr.c
>>> @@ -3313,6 +3313,7 @@ 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;
>>> }
>>> + put_nfs4_client(cs->session->se_client);
>>> nfsd4_put_session(cs->session);
>>> }
>>> return 1;
>>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>>> index fefeae2..e3c002e 100644
>>> --- a/fs/nfsd/state.h
>>> +++ b/fs/nfsd/state.h
>>> @@ -231,6 +231,12 @@ struct nfs4_client {
>>> /* wait here for slots */
>>> };
>>>
>>> +static inline void
>>> +get_nfs4_client(struct nfs4_client *clp)
>>> +{
>>> + atomic_inc(&clp->cl_count);
>>> +}
>>> +
>>> /* struct nfs4_client_reset
>>> * one per old client. Populates reset_str_hashtbl. Filled from conf_id_hashtbl
>>> * upon lease reset, or from upcall to state_daemon (to read in state
>>> --
>>> 1.6.3.3
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 04, 2010 at 10:11:40AM +0300, Benny Halevy wrote:
> On May. 04, 2010, 4:12 +0300, " J. Bruce Fields" <[email protected]> wrote:
> > On Mon, May 03, 2010 at 06:36:20PM -0400, J. Bruce Fields wrote:
> >> On Mon, May 03, 2010 at 07:31:48PM +0300, Benny Halevy wrote:
> >>> Although expire_client unhashes the session form the session table
> >>> so no new compounds can find it, there's no refcount to keep the
> >>> nfs4_client structure around while it's in use and referenced
> >>> in the compound state via the session structure.
> >> The code in my for-2.6.35 branch already has the cl_count removed (and
> >> doesn't use it for callbacks any more, instead destroying callbacks
> >> before the client is destroyed).
> >>
> >> So we need to add a new usage count.
> >
> > (Which you do in the next patch, OK!)
>
> Right :)
>
> This patch fixes another race in which a client can be expired using
> expire_client(), not from the laundromat path, while it's being referenced
> by the session since we look up the session while holding just the sessionid
> lock and not the state lock. Therefore we must take a refcount on the
> client while inside the sessionid lock.
>
> Looks like the new usage count in your for-2.6.35 world (that I _think_
> could just be a kref now) is needed for delegations as well, right?
We should be able to destroy any delegations, recalls, or other
callbacks before we destroy the client, so I don't think we need a
reference count for those.
--b.
>
> Benny
>
> >
> > --b.
> >
> >> I'd prefer to call it something
> >> different, since it's being used for something different (cl_users?)
> >> since it's being used for something different, and I'd rather avoid
> >> confusion with the previous one.
> >>
> >> --b.
> >>
> >>
> >>
> >>> Signed-off-by: Benny Halevy <[email protected]>
> >>> ---
> >>> fs/nfsd/nfs4callback.c | 2 +-
> >>> fs/nfsd/nfs4state.c | 4 +++-
> >>> fs/nfsd/nfs4xdr.c | 1 +
> >>> fs/nfsd/state.h | 6 ++++++
> >>> 4 files changed, 11 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> >>> index 7e32bd3..fef1dbe 100644
> >>> --- a/fs/nfsd/nfs4callback.c
> >>> +++ b/fs/nfsd/nfs4callback.c
> >>> @@ -571,7 +571,7 @@ nfsd4_probe_callback(struct nfs4_client *clp)
> >>> }
> >>>
> >>> /* the task holds a reference to the nfs4_client struct */
> >>> - atomic_inc(&clp->cl_count);
> >>> + get_nfs4_client(clp);
> >>>
> >>> do_probe_callback(clp);
> >>> }
> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >>> index 6dbcaf1..50b75af 100644
> >>> --- a/fs/nfsd/nfs4state.c
> >>> +++ b/fs/nfsd/nfs4state.c
> >>> @@ -1458,8 +1458,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> >>>
> >>> out:
> >>> /* Hold a session reference until done processing the compound. */
> >>> - if (cstate->session)
> >>> + if (cstate->session) {
> >>> nfsd4_get_session(cstate->session);
> >>> + get_nfs4_client(cstate->session->se_client);
> >>> + }
> >>> spin_unlock(&sessionid_lock);
> >>> /* Renew the clientid on success and on replay */
> >>> if (cstate->session) {
> >>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> >>> index 19ff5a3..aed733c 100644
> >>> --- a/fs/nfsd/nfs4xdr.c
> >>> +++ b/fs/nfsd/nfs4xdr.c
> >>> @@ -3313,6 +3313,7 @@ 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;
> >>> }
> >>> + put_nfs4_client(cs->session->se_client);
> >>> nfsd4_put_session(cs->session);
> >>> }
> >>> return 1;
> >>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> >>> index fefeae2..e3c002e 100644
> >>> --- a/fs/nfsd/state.h
> >>> +++ b/fs/nfsd/state.h
> >>> @@ -231,6 +231,12 @@ struct nfs4_client {
> >>> /* wait here for slots */
> >>> };
> >>>
> >>> +static inline void
> >>> +get_nfs4_client(struct nfs4_client *clp)
> >>> +{
> >>> + atomic_inc(&clp->cl_count);
> >>> +}
> >>> +
> >>> /* struct nfs4_client_reset
> >>> * one per old client. Populates reset_str_hashtbl. Filled from conf_id_hashtbl
> >>> * upon lease reset, or from upcall to state_daemon (to read in state
> >>> --
> >>> 1.6.3.3
> >>>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Tue, May 04, 2010 at 10:11:40AM +0300, Benny Halevy wrote:
> On May. 04, 2010, 4:12 +0300, " J. Bruce Fields" <[email protected]> wrote:
> > On Mon, May 03, 2010 at 06:36:20PM -0400, J. Bruce Fields wrote:
> >> On Mon, May 03, 2010 at 07:31:48PM +0300, Benny Halevy wrote:
> >>> Although expire_client unhashes the session form the session table
> >>> so no new compounds can find it, there's no refcount to keep the
> >>> nfs4_client structure around while it's in use and referenced
> >>> in the compound state via the session structure.
> >> The code in my for-2.6.35 branch already has the cl_count removed (and
> >> doesn't use it for callbacks any more, instead destroying callbacks
> >> before the client is destroyed).
> >>
> >> So we need to add a new usage count.
> >
> > (Which you do in the next patch, OK!)
>
> Right :)
>
> This patch fixes another race in which a client can be expired using
> expire_client(), not from the laundromat path,
Ouch, OK, I'd forgotten that case.
--b.
> while it's being referenced
> by the session since we look up the session while holding just the sessionid
> lock and not the state lock. Therefore we must take a refcount on the
> client while inside the sessionid lock.
>
> Looks like the new usage count in your for-2.6.35 world (that I _think_
> could just be a kref now) is needed for delegations as well, right?
>
> Benny
>
> >
> > --b.
> >
> >> I'd prefer to call it something
> >> different, since it's being used for something different (cl_users?)
> >> since it's being used for something different, and I'd rather avoid
> >> confusion with the previous one.
> >>
> >> --b.
> >>
> >>
> >>
> >>> Signed-off-by: Benny Halevy <[email protected]>
> >>> ---
> >>> fs/nfsd/nfs4callback.c | 2 +-
> >>> fs/nfsd/nfs4state.c | 4 +++-
> >>> fs/nfsd/nfs4xdr.c | 1 +
> >>> fs/nfsd/state.h | 6 ++++++
> >>> 4 files changed, 11 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> >>> index 7e32bd3..fef1dbe 100644
> >>> --- a/fs/nfsd/nfs4callback.c
> >>> +++ b/fs/nfsd/nfs4callback.c
> >>> @@ -571,7 +571,7 @@ nfsd4_probe_callback(struct nfs4_client *clp)
> >>> }
> >>>
> >>> /* the task holds a reference to the nfs4_client struct */
> >>> - atomic_inc(&clp->cl_count);
> >>> + get_nfs4_client(clp);
> >>>
> >>> do_probe_callback(clp);
> >>> }
> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >>> index 6dbcaf1..50b75af 100644
> >>> --- a/fs/nfsd/nfs4state.c
> >>> +++ b/fs/nfsd/nfs4state.c
> >>> @@ -1458,8 +1458,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> >>>
> >>> out:
> >>> /* Hold a session reference until done processing the compound. */
> >>> - if (cstate->session)
> >>> + if (cstate->session) {
> >>> nfsd4_get_session(cstate->session);
> >>> + get_nfs4_client(cstate->session->se_client);
> >>> + }
> >>> spin_unlock(&sessionid_lock);
> >>> /* Renew the clientid on success and on replay */
> >>> if (cstate->session) {
> >>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> >>> index 19ff5a3..aed733c 100644
> >>> --- a/fs/nfsd/nfs4xdr.c
> >>> +++ b/fs/nfsd/nfs4xdr.c
> >>> @@ -3313,6 +3313,7 @@ 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;
> >>> }
> >>> + put_nfs4_client(cs->session->se_client);
> >>> nfsd4_put_session(cs->session);
> >>> }
> >>> return 1;
> >>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> >>> index fefeae2..e3c002e 100644
> >>> --- a/fs/nfsd/state.h
> >>> +++ b/fs/nfsd/state.h
> >>> @@ -231,6 +231,12 @@ struct nfs4_client {
> >>> /* wait here for slots */
> >>> };
> >>>
> >>> +static inline void
> >>> +get_nfs4_client(struct nfs4_client *clp)
> >>> +{
> >>> + atomic_inc(&clp->cl_count);
> >>> +}
> >>> +
> >>> /* struct nfs4_client_reset
> >>> * one per old client. Populates reset_str_hashtbl. Filled from conf_id_hashtbl
> >>> * upon lease reset, or from upcall to state_daemon (to read in state
> >>> --
> >>> 1.6.3.3
> >>>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On May. 04, 2010, 20:45 +0300, "J. Bruce Fields" <[email protected]> wrote:
> On Tue, May 04, 2010 at 10:11:40AM +0300, Benny Halevy wrote:
>> On May. 04, 2010, 4:12 +0300, " J. Bruce Fields" <[email protected]> wrote:
>>> On Mon, May 03, 2010 at 06:36:20PM -0400, J. Bruce Fields wrote:
>>>> On Mon, May 03, 2010 at 07:31:48PM +0300, Benny Halevy wrote:
>>>>> Although expire_client unhashes the session form the session table
>>>>> so no new compounds can find it, there's no refcount to keep the
>>>>> nfs4_client structure around while it's in use and referenced
>>>>> in the compound state via the session structure.
>>>> The code in my for-2.6.35 branch already has the cl_count removed (and
>>>> doesn't use it for callbacks any more, instead destroying callbacks
>>>> before the client is destroyed).
>>>>
>>>> So we need to add a new usage count.
>>>
>>> (Which you do in the next patch, OK!)
>>
>> Right :)
>>
>> This patch fixes another race in which a client can be expired using
>> expire_client(), not from the laundromat path,
>
> Ouch, OK, I'd forgotten that case.
I'm working on a new version on top of your for-2.6.35 branch
that will solve both cases using the new ref count.
Benny
>
> --b.
>
>> while it's being referenced
>> by the session since we look up the session while holding just the sessionid
>> lock and not the state lock. Therefore we must take a refcount on the
>> client while inside the sessionid lock.
>>
>> Looks like the new usage count in your for-2.6.35 world (that I _think_
>> could just be a kref now) is needed for delegations as well, right?
>>
>> Benny
>>
>>>
>>> --b.
>>>
>>>> I'd prefer to call it something
>>>> different, since it's being used for something different (cl_users?)
>>>> since it's being used for something different, and I'd rather avoid
>>>> confusion with the previous one.
>>>>
>>>> --b.
>>>>
>>>>
>>>>
>>>>> Signed-off-by: Benny Halevy <[email protected]>
>>>>> ---
>>>>> fs/nfsd/nfs4callback.c | 2 +-
>>>>> fs/nfsd/nfs4state.c | 4 +++-
>>>>> fs/nfsd/nfs4xdr.c | 1 +
>>>>> fs/nfsd/state.h | 6 ++++++
>>>>> 4 files changed, 11 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
>>>>> index 7e32bd3..fef1dbe 100644
>>>>> --- a/fs/nfsd/nfs4callback.c
>>>>> +++ b/fs/nfsd/nfs4callback.c
>>>>> @@ -571,7 +571,7 @@ nfsd4_probe_callback(struct nfs4_client *clp)
>>>>> }
>>>>>
>>>>> /* the task holds a reference to the nfs4_client struct */
>>>>> - atomic_inc(&clp->cl_count);
>>>>> + get_nfs4_client(clp);
>>>>>
>>>>> do_probe_callback(clp);
>>>>> }
>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>> index 6dbcaf1..50b75af 100644
>>>>> --- a/fs/nfsd/nfs4state.c
>>>>> +++ b/fs/nfsd/nfs4state.c
>>>>> @@ -1458,8 +1458,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>>>>>
>>>>> out:
>>>>> /* Hold a session reference until done processing the compound. */
>>>>> - if (cstate->session)
>>>>> + if (cstate->session) {
>>>>> nfsd4_get_session(cstate->session);
>>>>> + get_nfs4_client(cstate->session->se_client);
>>>>> + }
>>>>> spin_unlock(&sessionid_lock);
>>>>> /* Renew the clientid on success and on replay */
>>>>> if (cstate->session) {
>>>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>>>> index 19ff5a3..aed733c 100644
>>>>> --- a/fs/nfsd/nfs4xdr.c
>>>>> +++ b/fs/nfsd/nfs4xdr.c
>>>>> @@ -3313,6 +3313,7 @@ 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;
>>>>> }
>>>>> + put_nfs4_client(cs->session->se_client);
>>>>> nfsd4_put_session(cs->session);
>>>>> }
>>>>> return 1;
>>>>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>>>>> index fefeae2..e3c002e 100644
>>>>> --- a/fs/nfsd/state.h
>>>>> +++ b/fs/nfsd/state.h
>>>>> @@ -231,6 +231,12 @@ struct nfs4_client {
>>>>> /* wait here for slots */
>>>>> };
>>>>>
>>>>> +static inline void
>>>>> +get_nfs4_client(struct nfs4_client *clp)
>>>>> +{
>>>>> + atomic_inc(&clp->cl_count);
>>>>> +}
>>>>> +
>>>>> /* struct nfs4_client_reset
>>>>> * one per old client. Populates reset_str_hashtbl. Filled from conf_id_hashtbl
>>>>> * upon lease reset, or from upcall to state_daemon (to read in state
>>>>> --
>>>>> 1.6.3.3
>>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>