On 4/8/22 9:31 AM, J. Bruce Fields wrote:
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().
Thanks Bruce for pointing this out. I will fix it with a wait lock
in the next patch. This seems heavy but I have not been able to
fix it without the wait lock. This is should be a rare condition so
it would not cause much overhead.
+ 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(<, 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,
I don't understand, we only set EXPIRED when we want to expire the
courtesy client.
so isn't this unnecessary?
When the client is checked to see if it it can be in COURTESY_CLIENT
state, we did not check if there is any delegation recall callback is
pending, we only check for lock blockers) so that is why we check it
here and expire it.
Maybe I did not give the right answer since I'm not clear of the
question yet.
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.
I will wait for the above issue to be resolved first to better
understand your concern here.
Thanks,
-Dai
--b.
+ continue;
+ }
+ spin_unlock(&clp->cl_cs_lock);
WARN_ON(!unhash_delegation_locked(dp));
list_add(&dp->dl_recall_lru, &reaplist);
}
--
2.9.5
