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(<, 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(<, 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);
> }
