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

[dai.ngo@oracle.com]

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:
>> On 4/25/22 11:51 AM, J. Bruce Fields wrote:
>>> On Sat, Apr 23, 2022 at 11:44:09AM -0700, Dai Ngo wrote:
>>>> This patch provides courteous server support for delegation only.
>>>> Only expired client with delegation but no conflict and no open
>>>> or lock state is allowed to be in COURTESY state.
>>>>
>>>> Delegation conflict with COURTESY client is resolved by setting it
>>>> to EXPIRABLE, queue work for the laundromat and return delay to
>>>> caller. Conflict is resolved when the laudromat runs and expires
>>>> the EXIRABLE client while the NFS client retries the OPEN request.
>>>> Local thread request that gets conflict is doing the retry in
>>>> __break_lease.
>>>>
>>>> Client in COURTESY state is allowed to reconnect and continues to
>>>> have access to its state while client in EXPIRABLE state is not.
>>>> To prevent 2 clients with the same name are on the conf_name_tree,
>>>> nfsd4_setclientid is modified to expire confirmed client in EXPIRABLE
>>>> state.
>>>>
>>>> The spinlock cl_cs_lock is used to handle race conditions between
>>>> conflict resolver, nfsd_break_deleg_cb, and the COURTESY client
>>>> doing reconnect.
>>>>
>>>> find_in_sessionid_hashtbl, find_confirmed_client_by_name and
>>>> find_confirmed_client are modified to activate COURTESY client by
>>>> setting it to ACTIVE state and skip client in EXPIRABLE state.
>>>>
>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>> ---
>>>>   fs/nfsd/nfs4state.c | 129 +++++++++++++++++++++++++++++++++++++++++++++-------
>>>>   fs/nfsd/nfsd.h      |   1 +
>>>>   fs/nfsd/state.h     |  63 +++++++++++++++++++++++++
>>>>   3 files changed, 177 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index 234e852fcdfa..fea5e24e7d94 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -125,6 +125,8 @@ static void free_session(struct nfsd4_session *);
>>>>   static const struct nfsd4_callback_ops nfsd4_cb_recall_ops;
>>>>   static const struct nfsd4_callback_ops nfsd4_cb_notify_lock_ops;
>>>> +static struct workqueue_struct *laundry_wq;
>>>> +
>>>>   static bool is_session_dead(struct nfsd4_session *ses)
>>>>   {
>>>>   	return ses->se_flags & NFS4_SESSION_DEAD;
>>>> @@ -690,6 +692,14 @@ static unsigned int file_hashval(struct svc_fh *fh)
>>>>   static struct hlist_head file_hashtbl[FILE_HASH_SIZE];
>>>> +static inline void
>>>> +run_laundromat(struct nfsd_net *nn, bool wait)
>>>> +{
>>>> +	mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
>>>> +	if (wait)
>>>> +		flush_workqueue(laundry_wq);
>>>> +}
>>> Let's keep those two things separate.  The "wait" argument isn't
>>> self-documenting when reading the calling code.
>> we can use enum to spell out the intention.
>>
>>>    And the
>>> mod_delayed_work call isn't always needed before flush_workqueue.
>> ok. I'll keep them separate.
>>
>>>> +
>>>>   static void
>>>>   __nfs4_file_get_access(struct nfs4_file *fp, u32 access)
>>>>   {
>>>> @@ -1939,6 +1949,11 @@ find_in_sessionid_hashtbl(struct nfs4_sessionid *sessionid, struct net *net,
>>>>   	session = __find_in_sessionid_hashtbl(sessionid, net);
>>>>   	if (!session)
>>>>   		goto out;
>>>> +	if (!try_to_activate_client(session->se_client)) {
>>>> +		/* client is EXPIRABLE */
>>>> +		session = NULL;
>>>> +		goto out;
>>> No, we definitely don't want to do this.  As I said before, an
>>> "expirable client" should be treated in every way exactly like any
>>> regular active client.  Literally the only difference is that the
>>> laundromat can try to expire it.
>> Do you mean leave the state as EXPIRABLE but allow the callers
>> to use the client as an ACTIVE client:
> The only place we should even be checking whether a client is EXPIRABLE
> is in the laundromat thread, in the loop over the client_lru.  Even
> there, EXPIRABLE being set doesn't mean the client *has* to be expired,
> the real decision is left to mark_client_expired_locked(), as it
> currently is.
>
>> 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?

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?

-Dai

>   and certainly shouldn't be failing
> lookups of EXPIRABLE clients.
>
>
>
> --b.
>
>
>> -Dai
>>
>>> And then all this code becomes unnecessary:
>>>
>>>> @@ -702,4 +727,42 @@ extern void nfsd4_client_record_remove(struct nfs4_client *clp);
>>>>   extern int nfsd4_client_record_check(struct nfs4_client *clp);
>>>>   extern void nfsd4_record_grace_done(struct nfsd_net *nn);
>>>> +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 true;
>>>> +	}
>>>> +	ret = NFSD4_COURTESY ==
>>>> +		cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
>>>> +	spin_unlock(&clp->cl_cs_lock);
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static inline bool try_to_activate_client(struct nfs4_client *clp)
>>>> +{
>>>> +	bool ret;
>>>> +	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>>>> +
>>>> +	/* sync with laundromat */
>>>> +	lockdep_assert_held(&nn->client_lock);
>>>> +
>>>> +	/* sync with try_to_expire_client */
>>>> +	spin_lock(&clp->cl_cs_lock);
>>>> +	if (clp->cl_state == NFSD4_ACTIVE) {
>>>> +		spin_unlock(&clp->cl_cs_lock);
>>>> +		return true;
>>>> +	}
>>>> +	if (clp->cl_state == NFSD4_EXPIRABLE) {
>>>> +		spin_unlock(&clp->cl_cs_lock);
>>>> +		return false;
>>>> +	}
>>>> +	ret = NFSD4_COURTESY ==
>>>> +		cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_ACTIVE);
>>>> +	spin_unlock(&clp->cl_cs_lock);
>>>> +	return ret;
>>>> +}
>>> --b.