On Mon, Apr 25, 2022 at 03:24:49PM -0700, dai.ngo@xxxxxxxxxx wrote:
>
> On 4/25/22 2:48 PM, J. Bruce Fields wrote:
> >On Mon, Apr 25, 2022 at 02:35:27PM -0700, dai.ngo@xxxxxxxxxx wrote:
> >>On 4/25/22 1:40 PM, J. Bruce Fields wrote:
> >>>On Mon, Apr 25, 2022 at 12:42:58PM -0700, dai.ngo@xxxxxxxxxx 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.
