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