Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 8 Apr 2022 12:31:35 -0400
From:   "J. Bruce Fields" <bfields@fieldses.org>
To:     Dai Ngo <dai.ngo@oracle.com>
Cc:     chuck.lever@oracle.com, linux-nfs@vger.kernel.org
Subject: Re: [PATCH RFC v20 09/10] NFSD: Update laundromat to handle courtesy
 client
Message-ID: <20220408163135.GA6423@fieldses.org>
References: <1649285133-16765-1-git-send-email-dai.ngo@oracle.com>
 <1649285133-16765-10-git-send-email-dai.ngo@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1649285133-16765-10-git-send-email-dai.ngo@oracle.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Wed, Apr 06, 2022 at 03:45:32PM -0700, Dai Ngo wrote:
>  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;
> +	struct list_head cslist;
>  
>  	INIT_LIST_HEAD(reaplist);
> +	INIT_LIST_HEAD(&cslist);
>  	spin_lock(&nn->client_lock);
>  	list_for_each_safe(pos, next, &nn->client_lru) {
>  		clp = list_entry(pos, struct nfs4_client, cl_lru);
>  		if (!state_expired(lt, clp->cl_time))
>  			break;
> -		if (mark_client_expired_locked(clp))
> +
> +		if (!client_has_state(clp))
> +			goto exp_client;
> +
> +		if (clp->cl_cs_client_state == NFSD4_CLIENT_EXPIRED)
> +			goto exp_client;
> +		cour = (clp->cl_cs_client_state == NFSD4_CLIENT_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;
> -		list_add(&clp->cl_lru, reaplist);
> +		}
> +		if (!cour) {
> +			spin_lock(&clp->cl_cs_lock);
> +			clp->cl_cs_client_state = NFSD4_CLIENT_COURTESY;
> +			spin_unlock(&clp->cl_cs_lock);
> +			list_add(&clp->cl_cs_list, &cslist);
> +		}
>  	}
>  	spin_unlock(&nn->client_lock);
> +
> +	while (!list_empty(&cslist)) {
> +		clp = list_first_entry(&cslist, struct nfs4_client, cl_cs_list);
> +		list_del_init(&clp->cl_cs_list);
> +		spin_lock(&clp->cl_cs_lock);
> +		/*
> +		 * Client might have re-connected. Make sure it's
> +		 * still in courtesy state before removing its record.
> +		 */

Good to be careful, but, then....

> +		if (clp->cl_cs_client_state != NFSD4_CLIENT_COURTESY) {
> +			spin_unlock(&clp->cl_cs_lock);
> +			continue;
> +		}
> +		spin_unlock(&clp->cl_cs_lock);

.... I'm not seeing what prevents a client from reconnecting right here,
after we drop cl_cs_lock but before we call
nfsd4_client_record_remove().

> +		nfsd4_client_record_remove(clp);
> +	}
>  }
>  
>  static time64_t
> @@ -5794,6 +5876,13 @@ nfs4_laundromat(struct nfsd_net *nn)
>  		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
>  		if (!state_expired(&lt, dp->dl_time))
>  			break;
> +		spin_lock(&clp->cl_cs_lock);
> +		if (clp->cl_cs_client_state == NFSD4_CLIENT_COURTESY) {
> +			clp->cl_cs_client_state = NFSD4_CLIENT_EXPIRED;
> +			spin_unlock(&clp->cl_cs_lock);

In an earlier patch you set the client to EXPIRED as soon as we got a
lease break, so isn't this unnecessary?

I guess there could be a case like:

	1. lease break comes in
	2. client fails to renew, transitions to courtesy
	3. delegation callback times out

though it'd seem better to catch that at step 2 if we can.

--b.

> +			continue;
> +		}
> +		spin_unlock(&clp->cl_cs_lock);
>  		WARN_ON(!unhash_delegation_locked(dp));
>  		list_add(&dp->dl_recall_lru, &reaplist);
>  	}
> -- 
> 2.9.5