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.
--b.
