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 03:24:49PM -0700, dai.ngo@oracle.com wrote:
> 
> On 4/25/22 2:48 PM, J. Bruce Fields wrote:
> >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.
> 
> ok, what you suggested seems to work. I will remove try_to_activate_client
> and just set cl_state to ACTIVE and test it to see if there is any problems
> that we haven't thought of with this scheme.

Thanks!

--b.