Re: [PATCH RFC v9 2/2] nfsd: Initial implementation of NFSv4 Courteous Server

["J. Bruce Fields" <bfields@fieldses.org>]

On Mon, Jan 10, 2022 at 10:50:53AM -0800, Dai Ngo wrote:
>  static time64_t
>  nfs4_laundromat(struct nfsd_net *nn)
>  {
> @@ -5587,7 +5834,9 @@ nfs4_laundromat(struct nfsd_net *nn)
>  	};
>  	struct nfs4_cpntf_state *cps;
>  	copy_stateid_t *cps_t;
> +	struct nfs4_stid *stid;
>  	int i;
> +	int id;
>  
>  	if (clients_still_reclaiming(nn)) {
>  		lt.new_timeo = 0;
> @@ -5608,8 +5857,41 @@ nfs4_laundromat(struct nfsd_net *nn)
>  	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))
> +		spin_lock(&clp->cl_cs_lock);
> +		if (test_bit(NFSD4_DESTROY_COURTESY_CLIENT, &clp->cl_flags))
> +			goto exp_client;
> +		if (test_bit(NFSD4_COURTESY_CLIENT, &clp->cl_flags)) {
> +			if (ktime_get_boottime_seconds() >= clp->courtesy_client_expiry)
> +				goto exp_client;
> +			/*
> +			 * after umount, v4.0 client is still around
> +			 * waiting to be expired. Check again and if
> +			 * it has no state then expire it.
> +			 */
> +			if (clp->cl_minorversion) {
> +				spin_unlock(&clp->cl_cs_lock);
> +				continue;
> +			}

I'm not following that comment or that logic.

> +		}
> +		if (!state_expired(&lt, clp->cl_time)) {
> +			spin_unlock(&clp->cl_cs_lock);
>  			break;
> +		}
> +		id = 0;
> +		spin_lock(&clp->cl_lock);
> +		stid = idr_get_next(&clp->cl_stateids, &id);
> +		if (stid && !nfs4_anylock_conflict(clp)) {
> +			/* client still has states */

I'm a little confused by that comment.  I think what you just checked is
that the client has some state, *and* nobody is waiting for one of its
locks.  For me, that comment just conufses things.

> +			spin_unlock(&clp->cl_lock);

Is nn->client_lock enough to guarantee that the condition you just
checked still holds?  (Honest question, I'm not sure.)

> +			clp->courtesy_client_expiry =
> +				ktime_get_boottime_seconds() + courtesy_client_expiry;
> +			set_bit(NFSD4_COURTESY_CLIENT, &clp->cl_flags);
> +			spin_unlock(&clp->cl_cs_lock);
> +			continue;
> +		}
> +		spin_unlock(&clp->cl_lock);
> +exp_client:
> +		spin_unlock(&clp->cl_cs_lock);
>  		if (mark_client_expired_locked(clp))
>  			continue;
>  		list_add(&clp->cl_lru, &reaplist);

In general this loop is more complicated than the rest of the logic in
nfs4_laundromat(). I'd be looking for ways to simplify it and/or move some
of it into a helper function.

--b.

> @@ -5689,9 +5971,6 @@ nfs4_laundromat(struct nfsd_net *nn)
>  	return max_t(time64_t, lt.new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
>  }