Re: [PATCH RFC v15 07/11] NFSD: Update find_in_sessionid_hashtbl() to handle courtesy clients

[dai.ngo@oracle.com]

On 3/9/22 7:33 PM, Chuck Lever III wrote:
>> On Mar 9, 2022, at 10:12 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>
>> 
>>> On 3/9/22 2:42 PM, Chuck Lever III wrote:
>>>
>>>>> On Mar 4, 2022, at 7:37 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>>> Update find_in_sessionid_hashtbl to:
>>>> . skip client with CLIENT_EXPIRED flag; discarded courtesy client.
>>>> . if courtesy client was found then clear CLIENT_COURTESY and
>>>>    set CLIENT_RECONNECTED so callers can take appropriate action.
>>>>
>>>> Update nfsd4_sequence and nfsd4_bind_conn_to_session to create client
>>>> record for client with CLIENT_RECONNECTED set.
>>>>
>>>> Update nfsd4_destroy_session to discard client with CLIENT_RECONNECTED
>>>> set.
>>>>
>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>> ---
>>>> fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++++--
>>>> 1 file changed, 32 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index f42d72a8f5ca..34a59c6f446c 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -1963,13 +1963,22 @@ find_in_sessionid_hashtbl(struct nfs4_sessionid *sessionid, struct net *net,
>>>> {
>>>>     struct nfsd4_session *session;
>>>>     __be32 status = nfserr_badsession;
>>>> +    struct nfs4_client *clp;
>>>>
>>>>     session = __find_in_sessionid_hashtbl(sessionid, net);
>>>>     if (!session)
>>>>         goto out;
>>>> +    clp = session->se_client;
>>>> +    if (clp && nfs4_is_courtesy_client_expired(clp)) {
>>>> +        session = NULL;
>>>> +        goto out;
>>>> +    }
>>>>     status = nfsd4_get_session_locked(session);
>>>> -    if (status)
>>>> +    if (status) {
>>>>         session = NULL;
>>>> +        if (clp && test_bit(NFSD4_CLIENT_COURTESY, &clp->cl_flags))
> By the way, I don’t understand why this checks CLIENT_COURTESY to see if the clp should be discarded. Shouldn’t it check CLIENT_RECONNECTED like the other sites?

Thanks, this should check for CLIENT_RECONNECTED as described
in the commit message.

>
>
>>>> +            nfsd4_discard_courtesy_clnt(clp);
>>>> +    }
>>> Here and above: I'm not seeing how @clp can be NULL, but I'm kind
>>> of new to fs/nfsd/nfs4state.c.
>> it seems like @clp can not be NULL since existing code does not
>> check for it. I will remove it to avoid any confusion. Can this
>> be done as a separate clean up patch?
> I don’t think this series is going to make v5.18. We can keep working on improving each of the patches. And I would rather ensure that the series is properly bisectable. I don’t think we’re at a point where the patches are immutable.

okay, I will submit v16 to address review comments.

-Dai