Re: [PATCH RFC v21 1/7] NFSD: add courteous server support for thread with only delegation

["J. Bruce Fields" <bfields@fieldses.org>]

On Mon, Apr 25, 2022 at 02:35:27PM -0700, dai.ngo@oracle.com wrote:
> On 4/25/22 1:40 PM, J. Bruce Fields wrote:
> >On Mon, Apr 25, 2022 at 12:42:58PM -0700, dai.ngo@oracle.com wrote:
> >>static inline bool try_to_expire_client(struct nfs4_client *clp)
> >>{
> >>         bool ret;
> >>
> >>         spin_lock(&clp->cl_cs_lock);
> >>         if (clp->cl_state == NFSD4_EXPIRABLE) {
> >>                 spin_unlock(&clp->cl_cs_lock);
> >>                 return false;            <<<====== was true
> >>         }
> >>         ret = NFSD4_COURTESY ==
> >>                 cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
> >>         spin_unlock(&clp->cl_cs_lock);
> >>         return ret;
> >>}
> >So, try_to_expire_client(), as I said, should be just
> >
> >   static bool try_to_expire_client(struct nfs4_client *clp)
> >   {
> >	return COURTESY == cmpxchg(clp->cl_state, COURTESY, EXPIRABLE);
> >   }
> >
> >Except, OK, I did forget that somebody else could have already set
> >EXPIRABLE, and we still want to tell the caller to retry in that case,
> >so, oops, let's make it:
> >
> >	return ACTIVE != cmpxchg(clp->cl_state, COURTESY, EXPIRABLE);
> 
> So functionally this is the same as what i have in the patch, except this
> makes it simpler?

Right.

And my main complaint is still about the code that fails lookups of
EXPIRABLE clients.  We shouldn't need to modify find_in_sessionid_hastbl
or other client lookups.

> Do we need to make any change in try_to_activate_client to work with
> this change in try_to_expire_client?
> 
> >
> >In other words: set EXPIRABLE if and only if the state is COURTESY, and
> >then tell the caller to retry the operation if and only if the state was
> >previously COURTESY or EXPIRABLE.
> >
> >But we shouldn't need the cl_cs_lock,
> 
> The cl_cs_lock is to synchronize between COURTESY client trying to
> reconnect (try_to_activate_client) and another thread is trying to
> resolve a delegation/share/lock conflict (try_to_expire_client).
> So you don't think this is necessary?

Correct, it's not necessary.

The only synchronization is provided by mark_client_expired_locked and
get_client_locked.

We don't need try_to_activate_client.  Just set cl_state to ACTIVE in
get_client_locked.

--b.