by Benny Halevy

[permalink] [raw]

Subject: Re: [pnfs] [RFC 01/11] nfsd: introduce nfs4_client state

On Dec. 17, 2009, 0:33 +0200, Peter Staubach <[email protected]> wrote:
> Benny Halevy wrote:
>> Signed-off-by: Benny Halevy <[email protected]>
>> ---
>> fs/nfsd/state.h | 8 ++++++++
>> 1 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index fefeae2..7e67eca 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -186,6 +186,13 @@ struct nfsd4_sessionid {
>>
>> #define HEXDIR_LEN 33 /* hex version of 16 byte md5 of cl_name plus '\0' */
>>
>> +/* nfs4_client states */
>> +enum nfs4_client_state {
>> + CL_STATE_NORMAL,
>> + CL_STATE_RENEW,
>> + CL_STATE_EXPIRED,
>> +};
>> +
>> /*
>> * struct nfs4_client - one per client. Clientids live here.
>> * o Each nfs4_client is hashed by clientid.
>> @@ -214,6 +221,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_state; /* renew / expiry state */
>
> The use of an atomic here seems complex and makes the
> implementation seem fragile to me. Is the atomic_cmpxchg()
> really going to save much time over just using a spinlock
> and normal loads and stores to retrieve and get the value
> of cl_state? A spinlock would make things much more
> obvious how they were supposed to work, to me, at least.
>

This can be done of course, though logically, I'd still
consider defining the equivalent of cmpxchg as a static
inline helper since it does exactly what I want to to.
As for efficiency the cmpxchg is more as efficient as
an uncontended fine grain lock, with no chance of spinning.
atomic_t also has a smaller memory footprint than a
fine grain lock (per nsf4_client). Using a coarse grain
lock is possible but will needless increase the chances
for contention.

Benny

> Thanx...
>
> ps
>
>> u32 cl_firststate; /* recovery dir creation */
>>
>> /* for nfs41 */
>

2009-12-17 20:19:08

by J.Bruce Fields

[permalink] [raw]

Subject: Re: [RFC 04/11] nfsd: mark client for renew

On Thu, Dec 17, 2009 at 12:23:41AM +0200, Benny Halevy wrote:
> On Dec. 17, 2009, 0:06 +0200, "J. Bruce Fields" <[email protected]> wrote:
> > On Wed, Dec 16, 2009 at 11:56:05PM +0200, Benny Halevy wrote:
> >> On Dec. 16, 2009, 22:53 +0200, " J. Bruce Fields" <[email protected]> wrote:
> >>> On Wed, Dec 16, 2009 at 03:48:39PM -0500, J. Bruce Fields wrote:
> >>>> On Wed, Dec 16, 2009 at 07:41:12PM +0200, Benny Halevy wrote:
> >>>>> static inline void
> >>>>> +mark_client_for_renew(struct nfsd4_compound_state *cstate,
> >>>>> + struct nfs4_client *clp)
> >>>>> +{
> >>>>> + int old_state = atomic_cmpxchg(&clp->cl_state, CL_STATE_NORMAL,
> >>>>> + CL_STATE_RENEW);
> >>>>> +
> >>>>> + if (old_state != CL_STATE_EXPIRED) {
> >>>>> + cstate->renew_client = clp;
> >>>>> + get_nfs4_client(clp);
> >>>> If cstate->renew_client is already set, then we may leak a reference
> >>>> count here, right?
> >>>>
> >>>> Also: at least in the v4 case, I don't think there's anything preventing
> >>>> cstate->renew_client being already set to a different client. (With
> >>>> sessions hopefully that's not possible.)
> >>>>
> >> Yeah. That's a valid point. So if it is already set then
> >> if it is the same client we can do nothing, else we can renew
> >> the previous one here put it and replace cstate->renew_client.
> >
> > I'm not sure if that can only happen if someone's intentionally messing
> > with us, or if maybe there's a legitimate case where it can happen
> > (maybe when a new client is created by some op in the compound?) But,
> > yes, that sounds like a safe way to handle it.
> >
>
> Can the client send a compound with multiple I/Os for different
> stateids (and even filehandles) that lead back to different clientids?

I don't know of anything in rfc 3530 that forbids it, but I doubt it
would be useful to any legitimate client.

--b.