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

[dai.ngo@oracle.com]

On 4/27/22 8:44 PM, dai.ngo@oracle.com wrote:
>
> On 4/27/22 6:52 PM, J. Bruce Fields wrote:
>> On Wed, Apr 27, 2022 at 03:52:37PM -0700, dai.ngo@oracle.com wrote:
>>> On 4/27/22 2:56 PM, J. Bruce Fields wrote:
>>>> On Wed, Apr 27, 2022 at 01:52:47AM -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/EXPIRABLE client is resolved by
>>>>> setting it to EXPIRABLE, queue work for the laundromat and return
>>>>> delay to the 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 or EXPIRABLE state is allowed to reconnect and
>>>>> continues to have access to its state. Access to the nfs4_client by
>>>>> the reconnecting thread and the laundromat is serialized via the
>>>>> client_lock.
>>>>>
>>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>>> ---
>>>>>   fs/nfsd/nfs4state.c | 86 
>>>>> +++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>   fs/nfsd/nfsd.h      |  1 +
>>>>>   fs/nfsd/state.h     | 32 ++++++++++++++++++++
>>>>>   3 files changed, 106 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>> index 234e852fcdfa..216bd77a8764 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;
>>>>> @@ -152,6 +154,7 @@ static __be32 get_client_locked(struct 
>>>>> nfs4_client *clp)
>>>>>       if (is_client_expired(clp))
>>>>>           return nfserr_expired;
>>>>>       atomic_inc(&clp->cl_rpc_users);
>>>>> +    clp->cl_state = NFSD4_ACTIVE;
>>>>>       return nfs_ok;
>>>>>   }
>>>>> @@ -172,6 +175,7 @@ renew_client_locked(struct nfs4_client *clp)
>>>>>       list_move_tail(&clp->cl_lru, &nn->client_lru);
>>>>>       clp->cl_time = ktime_get_boottime_seconds();
>>>>> +    clp->cl_state = NFSD4_ACTIVE;
>>>>>   }
>>>>>   static void put_client_renew_locked(struct nfs4_client *clp)
>>>>> @@ -2004,6 +2008,7 @@ static struct nfs4_client 
>>>>> *alloc_client(struct xdr_netobj name)
>>>>>       idr_init(&clp->cl_stateids);
>>>>>       atomic_set(&clp->cl_rpc_users, 0);
>>>>>       clp->cl_cb_state = NFSD4_CB_UNKNOWN;
>>>>> +    clp->cl_state = NFSD4_ACTIVE;
>>>>>       INIT_LIST_HEAD(&clp->cl_idhash);
>>>>>       INIT_LIST_HEAD(&clp->cl_openowners);
>>>>>       INIT_LIST_HEAD(&clp->cl_delegations);
>>>>> @@ -4694,9 +4699,16 @@ nfsd_break_deleg_cb(struct file_lock *fl)
>>>>>       bool ret = false;
>>>>>       struct nfs4_delegation *dp = (struct nfs4_delegation 
>>>>> *)fl->fl_owner;
>>>>>       struct nfs4_file *fp = dp->dl_stid.sc_file;
>>>>> +    struct nfs4_client *clp = dp->dl_stid.sc_client;
>>>>> +    struct nfsd_net *nn;
>>>>>       trace_nfsd_cb_recall(&dp->dl_stid);
>>>>> +    if (!try_to_expire_client(clp)) {
>>>>> +        nn = net_generic(clp->net, nfsd_net_id);
>>>>> +        mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
>>>>> +    }
>>>>> +
>>>>>       /*
>>>>>        * We don't want the locks code to timeout the lease for us;
>>>>>        * we'll remove it ourself if a delegation isn't returned
>>>>> @@ -5605,6 +5617,65 @@ static void nfsd4_ssc_expire_umount(struct 
>>>>> nfsd_net *nn)
>>>>>   }
>>>>>   #endif
>>>>> +/*
>>>>> + * place holder for now, no check for lock blockers yet
>>>>> + */
>>>>> +static bool
>>>>> +nfs4_anylock_blockers(struct nfs4_client *clp)
>>>>> +{
>>>>> +    /*
>>>>> +     * don't want to check for delegation conflict here since
>>>>> +     * we need the state_lock for it. The laundromat willexpire
>>>>> +     * COURTESY later when checking for delegation recall timeout.
>>>>> +     */
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>> +static bool client_has_state_tmp(struct nfs4_client *clp)
>>>>> +{
>>>>> +    if (!list_empty(&clp->cl_delegations) &&
>>>>> +            !client_has_openowners(clp) &&
>>>>> +            list_empty(&clp->async_copies))
>>>>> +        return true;
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +nfs4_get_client_reaplist(struct nfsd_net *nn, struct list_head 
>>>>> *reaplist,
>>>>> +                struct laundry_time *lt)
>>>>> +{
>>>>> +    struct list_head *pos, *next;
>>>>> +    struct nfs4_client *clp;
>>>>> +    bool cour;
>>>>> +
>>>>> +    INIT_LIST_HEAD(reaplist);
>>>>> +    spin_lock(&nn->client_lock);
>>>>> +    list_for_each_safe(pos, next, &nn->client_lru) {
>>>>> +        clp = list_entry(pos, struct nfs4_client, cl_lru);
>>>>> +        if (clp->cl_state == NFSD4_EXPIRABLE)
>>>>> +            goto exp_client;
>>>>> +        if (!state_expired(lt, clp->cl_time))
>>>>> +            break;
>>>>> +        if (!client_has_state_tmp(clp))
>>>>> +            goto exp_client;
>>>>> +        cour = (clp->cl_state == NFSD4_COURTESY);
>>>>> +        if (cour && ktime_get_boottime_seconds() >=
>>>>> +                (clp->cl_time + NFSD_COURTESY_CLIENT_TIMEOUT)) {
>>>>> +            goto exp_client;
>>>>> +        }
>>>>> +        if (nfs4_anylock_blockers(clp)) {
>>>>> +exp_client:
>>>>> +            if (mark_client_expired_locked(clp))
>>>>> +                continue;
>>>>> +            list_add(&clp->cl_lru, reaplist);
>>>>> +            continue;
>>>>> +        }
>>>>> +        if (!cour)
>>>>> +            cmpxchg(&clp->cl_state, NFSD4_ACTIVE, NFSD4_COURTESY);
>>>> I just noticed there's a small race here: a lock conflict (for 
>>>> example)
>>>> could intervene between checking nfs4_anylock_blockers and setting
>>>> COURTESY.
>>> If there is lock conflict intervenes before setting COURTESY then that
>>> lock request is denied since the client is ACTIVE. Does NFSv4, NLM
>>> client retry the lock request? if it does then on next retry the
>>> COURTESY client will be expired.
>> I'm thinking of a local request for a blocking lock.  Yes, the request
>> will be denied, but then the process will block on the lock forever
>> (well, for 24 hours anyway).

Actually if the local request is blocked, it will be blocked for max of
a lease period. When the laundromat runs again, it will detect there is
blockers and expire the COURTESY client.

I will move the setting of COURTESY to earlier as you suggested since
it also makes the code more compact.

-Dai