diff mbox series

nfsd: don't abort copies early

Message ID 20210224183950.GB11591@fieldses.org (mailing list archive)
State New
Headers show
Series nfsd: don't abort copies early | expand

Commit Message

bfields@fieldses.org Feb. 24, 2021, 6:39 p.m. UTC
From: "J. Bruce Fields" <bfields@redhat.com>

The typical result of the backwards comparison here is that the source
server in a server-to-server copy will return BAD_STATEID within a few
seconds of the copy starting, instead of giving the copy a full lease
period, so the copy_file_range() call will end up unnecessarily
returning a short read.

Fixes: 624322f1adc5 "NFSD add COPY_NOTIFY operation"
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Chuck Lever III Feb. 24, 2021, 6:47 p.m. UTC | #1
> On Feb 24, 2021, at 1:39 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> The typical result of the backwards comparison here is that the source
> server in a server-to-server copy will return BAD_STATEID within a few
> seconds of the copy starting, instead of giving the copy a full lease
> period, so the copy_file_range() call will end up unnecessarily
> returning a short read.
> 
> Fixes: 624322f1adc5 "NFSD add COPY_NOTIFY operation"
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>

Thanks for your patch. I've pushed it to the for-rc branch of

git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git


> ---
> fs/nfsd/nfs4state.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 423fd6683f3a..61552e89bd89 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5389,7 +5389,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> 	idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) {
> 		cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid);
> 		if (cps->cp_stateid.sc_type == NFS4_COPYNOTIFY_STID &&
> -				cps->cpntf_time > cutoff)
> +				cps->cpntf_time < cutoff)
> 			_free_cpntf_state_locked(nn, cps);
> 	}
> 	spin_unlock(&nn->s2s_cp_lock);
> -- 
> 2.29.2
> 

--
Chuck Lever
bfields@fieldses.org Feb. 24, 2021, 7:31 p.m. UTC | #2
I confess I always have to stare at these comparisons for a minute to
figure out which way they should go.  And the logic in each of these
loops is a little repetitive.

Would it be worth creating a little state_expired() helper to work out
the comparison and the new timeout?

--b.

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 61552e89bd89..00fb3603c29e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5363,6 +5363,22 @@ static bool clients_still_reclaiming(struct nfsd_net *nn)
 	return true;
 }
 
+struct laundry_time {
+	time64_t cutoff;
+	time64_t new_timeo;
+};
+
+bool state_expired(struct laundry_time *lt, time64_t last_refresh)
+{
+	time64_t time_remaining;
+
+	if (last_refresh > lt->cutoff)
+		return true;
+	time_remaining = lt->cutoff - last_refresh;
+	lt->new_timeo = min(lt->new_timeo, time_remaining);
+	return false;
+}
+
 static time64_t
 nfs4_laundromat(struct nfsd_net *nn)
 {
@@ -5372,14 +5388,16 @@ nfs4_laundromat(struct nfsd_net *nn)
 	struct nfs4_ol_stateid *stp;
 	struct nfsd4_blocked_lock *nbl;
 	struct list_head *pos, *next, reaplist;
-	time64_t cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease;
-	time64_t t, new_timeo = nn->nfsd4_lease;
+	struct laundry_time lt = {
+		.cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease,
+		.new_timeo = nn->nfsd4_lease
+	};
 	struct nfs4_cpntf_state *cps;
 	copy_stateid_t *cps_t;
 	int i;
 
 	if (clients_still_reclaiming(nn)) {
-		new_timeo = 0;
+		lt.new_timeo = 0;
 		goto out;
 	}
 	nfsd4_end_grace(nn);
@@ -5389,7 +5407,7 @@ nfs4_laundromat(struct nfsd_net *nn)
 	idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) {
 		cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid);
 		if (cps->cp_stateid.sc_type == NFS4_COPYNOTIFY_STID &&
-				cps->cpntf_time < cutoff)
+				state_expired(&lt, cps->cpntf_time))
 			_free_cpntf_state_locked(nn, cps);
 	}
 	spin_unlock(&nn->s2s_cp_lock);
@@ -5397,11 +5415,8 @@ 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 (clp->cl_time > cutoff) {
-			t = clp->cl_time - cutoff;
-			new_timeo = min(new_timeo, t);
+		if (!state_expired(&lt, clp->cl_time))
 			break;
-		}
 		if (mark_client_expired_locked(clp)) {
 			trace_nfsd_clid_expired(&clp->cl_clientid);
 			continue;
@@ -5418,11 +5433,8 @@ nfs4_laundromat(struct nfsd_net *nn)
 	spin_lock(&state_lock);
 	list_for_each_safe(pos, next, &nn->del_recall_lru) {
 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
-		if (dp->dl_time > cutoff) {
-			t = dp->dl_time - cutoff;
-			new_timeo = min(new_timeo, t);
+		if (!state_expired(&lt, clp->cl_time))
 			break;
-		}
 		WARN_ON(!unhash_delegation_locked(dp));
 		list_add(&dp->dl_recall_lru, &reaplist);
 	}
@@ -5438,11 +5450,8 @@ nfs4_laundromat(struct nfsd_net *nn)
 	while (!list_empty(&nn->close_lru)) {
 		oo = list_first_entry(&nn->close_lru, struct nfs4_openowner,
 					oo_close_lru);
-		if (oo->oo_time > cutoff) {
-			t = oo->oo_time - cutoff;
-			new_timeo = min(new_timeo, t);
+		if (!state_expired(&lt, oo->oo_time))
 			break;
-		}
 		list_del_init(&oo->oo_close_lru);
 		stp = oo->oo_last_closed_stid;
 		oo->oo_last_closed_stid = NULL;
@@ -5468,11 +5477,8 @@ nfs4_laundromat(struct nfsd_net *nn)
 	while (!list_empty(&nn->blocked_locks_lru)) {
 		nbl = list_first_entry(&nn->blocked_locks_lru,
 					struct nfsd4_blocked_lock, nbl_lru);
-		if (nbl->nbl_time > cutoff) {
-			t = nbl->nbl_time - cutoff;
-			new_timeo = min(new_timeo, t);
+		if (!state_expired(&lt, oo->oo_time))
 			break;
-		}
 		list_move(&nbl->nbl_lru, &reaplist);
 		list_del_init(&nbl->nbl_list);
 	}
@@ -5485,8 +5491,7 @@ nfs4_laundromat(struct nfsd_net *nn)
 		free_blocked_lock(nbl);
 	}
 out:
-	new_timeo = max_t(time64_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
-	return new_timeo;
+	return max_t(time64_t, lt.new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
 }
 
 static struct workqueue_struct *laundry_wq;
Chuck Lever III Feb. 24, 2021, 7:34 p.m. UTC | #3
> On Feb 24, 2021, at 2:31 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> I confess I always have to stare at these comparisons for a minute to
> figure out which way they should go.  And the logic in each of these
> loops is a little repetitive.
> 
> Would it be worth creating a little state_expired() helper to work out
> the comparison and the new timeout?

That's better, but aren't there already operators on time64_t data objects
that can be used for this?


> --b.
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 61552e89bd89..00fb3603c29e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5363,6 +5363,22 @@ static bool clients_still_reclaiming(struct nfsd_net *nn)
> 	return true;
> }
> 
> +struct laundry_time {
> +	time64_t cutoff;
> +	time64_t new_timeo;
> +};
> +
> +bool state_expired(struct laundry_time *lt, time64_t last_refresh)
> +{
> +	time64_t time_remaining;
> +
> +	if (last_refresh > lt->cutoff)
> +		return true;
> +	time_remaining = lt->cutoff - last_refresh;
> +	lt->new_timeo = min(lt->new_timeo, time_remaining);
> +	return false;
> +}
> +
> static time64_t
> nfs4_laundromat(struct nfsd_net *nn)
> {
> @@ -5372,14 +5388,16 @@ nfs4_laundromat(struct nfsd_net *nn)
> 	struct nfs4_ol_stateid *stp;
> 	struct nfsd4_blocked_lock *nbl;
> 	struct list_head *pos, *next, reaplist;
> -	time64_t cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease;
> -	time64_t t, new_timeo = nn->nfsd4_lease;
> +	struct laundry_time lt = {
> +		.cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease,
> +		.new_timeo = nn->nfsd4_lease
> +	};
> 	struct nfs4_cpntf_state *cps;
> 	copy_stateid_t *cps_t;
> 	int i;
> 
> 	if (clients_still_reclaiming(nn)) {
> -		new_timeo = 0;
> +		lt.new_timeo = 0;
> 		goto out;
> 	}
> 	nfsd4_end_grace(nn);
> @@ -5389,7 +5407,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> 	idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) {
> 		cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid);
> 		if (cps->cp_stateid.sc_type == NFS4_COPYNOTIFY_STID &&
> -				cps->cpntf_time < cutoff)
> +				state_expired(&lt, cps->cpntf_time))
> 			_free_cpntf_state_locked(nn, cps);
> 	}
> 	spin_unlock(&nn->s2s_cp_lock);
> @@ -5397,11 +5415,8 @@ 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 (clp->cl_time > cutoff) {
> -			t = clp->cl_time - cutoff;
> -			new_timeo = min(new_timeo, t);
> +		if (!state_expired(&lt, clp->cl_time))
> 			break;
> -		}
> 		if (mark_client_expired_locked(clp)) {
> 			trace_nfsd_clid_expired(&clp->cl_clientid);
> 			continue;
> @@ -5418,11 +5433,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> 	spin_lock(&state_lock);
> 	list_for_each_safe(pos, next, &nn->del_recall_lru) {
> 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> -		if (dp->dl_time > cutoff) {
> -			t = dp->dl_time - cutoff;
> -			new_timeo = min(new_timeo, t);
> +		if (!state_expired(&lt, clp->cl_time))
> 			break;
> -		}
> 		WARN_ON(!unhash_delegation_locked(dp));
> 		list_add(&dp->dl_recall_lru, &reaplist);
> 	}
> @@ -5438,11 +5450,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> 	while (!list_empty(&nn->close_lru)) {
> 		oo = list_first_entry(&nn->close_lru, struct nfs4_openowner,
> 					oo_close_lru);
> -		if (oo->oo_time > cutoff) {
> -			t = oo->oo_time - cutoff;
> -			new_timeo = min(new_timeo, t);
> +		if (!state_expired(&lt, oo->oo_time))
> 			break;
> -		}
> 		list_del_init(&oo->oo_close_lru);
> 		stp = oo->oo_last_closed_stid;
> 		oo->oo_last_closed_stid = NULL;
> @@ -5468,11 +5477,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> 	while (!list_empty(&nn->blocked_locks_lru)) {
> 		nbl = list_first_entry(&nn->blocked_locks_lru,
> 					struct nfsd4_blocked_lock, nbl_lru);
> -		if (nbl->nbl_time > cutoff) {
> -			t = nbl->nbl_time - cutoff;
> -			new_timeo = min(new_timeo, t);
> +		if (!state_expired(&lt, oo->oo_time))
> 			break;
> -		}
> 		list_move(&nbl->nbl_lru, &reaplist);
> 		list_del_init(&nbl->nbl_list);
> 	}
> @@ -5485,8 +5491,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> 		free_blocked_lock(nbl);
> 	}
> out:
> -	new_timeo = max_t(time64_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> -	return new_timeo;
> +	return max_t(time64_t, lt.new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> }
> 
> static struct workqueue_struct *laundry_wq;

--
Chuck Lever
bfields@fieldses.org Feb. 24, 2021, 10:33 p.m. UTC | #4
On Wed, Feb 24, 2021 at 07:34:10PM +0000, Chuck Lever wrote:
> 
> 
> > On Feb 24, 2021, at 2:31 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > 
> > I confess I always have to stare at these comparisons for a minute to
> > figure out which way they should go.  And the logic in each of these
> > loops is a little repetitive.
> > 
> > Would it be worth creating a little state_expired() helper to work out
> > the comparison and the new timeout?
> 
> That's better, but aren't there already operators on time64_t data objects
> that can be used for this?

No idea.

--b.

> 
> 
> > --b.
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 61552e89bd89..00fb3603c29e 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5363,6 +5363,22 @@ static bool clients_still_reclaiming(struct nfsd_net *nn)
> > 	return true;
> > }
> > 
> > +struct laundry_time {
> > +	time64_t cutoff;
> > +	time64_t new_timeo;
> > +};
> > +
> > +bool state_expired(struct laundry_time *lt, time64_t last_refresh)
> > +{
> > +	time64_t time_remaining;
> > +
> > +	if (last_refresh > lt->cutoff)
> > +		return true;
> > +	time_remaining = lt->cutoff - last_refresh;
> > +	lt->new_timeo = min(lt->new_timeo, time_remaining);
> > +	return false;
> > +}
> > +
> > static time64_t
> > nfs4_laundromat(struct nfsd_net *nn)
> > {
> > @@ -5372,14 +5388,16 @@ nfs4_laundromat(struct nfsd_net *nn)
> > 	struct nfs4_ol_stateid *stp;
> > 	struct nfsd4_blocked_lock *nbl;
> > 	struct list_head *pos, *next, reaplist;
> > -	time64_t cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease;
> > -	time64_t t, new_timeo = nn->nfsd4_lease;
> > +	struct laundry_time lt = {
> > +		.cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease,
> > +		.new_timeo = nn->nfsd4_lease
> > +	};
> > 	struct nfs4_cpntf_state *cps;
> > 	copy_stateid_t *cps_t;
> > 	int i;
> > 
> > 	if (clients_still_reclaiming(nn)) {
> > -		new_timeo = 0;
> > +		lt.new_timeo = 0;
> > 		goto out;
> > 	}
> > 	nfsd4_end_grace(nn);
> > @@ -5389,7 +5407,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> > 	idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) {
> > 		cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid);
> > 		if (cps->cp_stateid.sc_type == NFS4_COPYNOTIFY_STID &&
> > -				cps->cpntf_time < cutoff)
> > +				state_expired(&lt, cps->cpntf_time))
> > 			_free_cpntf_state_locked(nn, cps);
> > 	}
> > 	spin_unlock(&nn->s2s_cp_lock);
> > @@ -5397,11 +5415,8 @@ 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 (clp->cl_time > cutoff) {
> > -			t = clp->cl_time - cutoff;
> > -			new_timeo = min(new_timeo, t);
> > +		if (!state_expired(&lt, clp->cl_time))
> > 			break;
> > -		}
> > 		if (mark_client_expired_locked(clp)) {
> > 			trace_nfsd_clid_expired(&clp->cl_clientid);
> > 			continue;
> > @@ -5418,11 +5433,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> > 	spin_lock(&state_lock);
> > 	list_for_each_safe(pos, next, &nn->del_recall_lru) {
> > 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> > -		if (dp->dl_time > cutoff) {
> > -			t = dp->dl_time - cutoff;
> > -			new_timeo = min(new_timeo, t);
> > +		if (!state_expired(&lt, clp->cl_time))
> > 			break;
> > -		}
> > 		WARN_ON(!unhash_delegation_locked(dp));
> > 		list_add(&dp->dl_recall_lru, &reaplist);
> > 	}
> > @@ -5438,11 +5450,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> > 	while (!list_empty(&nn->close_lru)) {
> > 		oo = list_first_entry(&nn->close_lru, struct nfs4_openowner,
> > 					oo_close_lru);
> > -		if (oo->oo_time > cutoff) {
> > -			t = oo->oo_time - cutoff;
> > -			new_timeo = min(new_timeo, t);
> > +		if (!state_expired(&lt, oo->oo_time))
> > 			break;
> > -		}
> > 		list_del_init(&oo->oo_close_lru);
> > 		stp = oo->oo_last_closed_stid;
> > 		oo->oo_last_closed_stid = NULL;
> > @@ -5468,11 +5477,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> > 	while (!list_empty(&nn->blocked_locks_lru)) {
> > 		nbl = list_first_entry(&nn->blocked_locks_lru,
> > 					struct nfsd4_blocked_lock, nbl_lru);
> > -		if (nbl->nbl_time > cutoff) {
> > -			t = nbl->nbl_time - cutoff;
> > -			new_timeo = min(new_timeo, t);
> > +		if (!state_expired(&lt, oo->oo_time))
> > 			break;
> > -		}
> > 		list_move(&nbl->nbl_lru, &reaplist);
> > 		list_del_init(&nbl->nbl_list);
> > 	}
> > @@ -5485,8 +5491,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> > 		free_blocked_lock(nbl);
> > 	}
> > out:
> > -	new_timeo = max_t(time64_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> > -	return new_timeo;
> > +	return max_t(time64_t, lt.new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> > }
> > 
> > static struct workqueue_struct *laundry_wq;
> 
> --
> Chuck Lever
> 
>
Chuck Lever III Feb. 25, 2021, 3:28 p.m. UTC | #5
> On Feb 24, 2021, at 5:33 PM, Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Wed, Feb 24, 2021 at 07:34:10PM +0000, Chuck Lever wrote:
>> 
>> 
>>> On Feb 24, 2021, at 2:31 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>>> 
>>> I confess I always have to stare at these comparisons for a minute to
>>> figure out which way they should go.  And the logic in each of these
>>> loops is a little repetitive.
>>> 
>>> Would it be worth creating a little state_expired() helper to work out
>>> the comparison and the new timeout?
>> 
>> That's better, but aren't there already operators on time64_t data objects
>> that can be used for this?
> 
> No idea.

I was thinking of jiffies, I guess. time_before() and time_after().

Checking the definition of time64_t, from include/linux/time64.h:

typedef __s64 time64_t;

Signed, hrm. And no comparison helpers.

I might be a little concerned about wrapping time values. But any
concerns can be addressed in the new common helper state_expired(),
possibly as a subsequent patch.

If you feel it's ready, can you send me the below clean-up as an
official patch with description and SoB?


> --b.
> 
>> 
>> 
>>> --b.
>>> 
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 61552e89bd89..00fb3603c29e 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -5363,6 +5363,22 @@ static bool clients_still_reclaiming(struct nfsd_net *nn)
>>> 	return true;
>>> }
>>> 
>>> +struct laundry_time {
>>> +	time64_t cutoff;
>>> +	time64_t new_timeo;
>>> +};
>>> +
>>> +bool state_expired(struct laundry_time *lt, time64_t last_refresh)
>>> +{
>>> +	time64_t time_remaining;
>>> +
>>> +	if (last_refresh > lt->cutoff)
>>> +		return true;
>>> +	time_remaining = lt->cutoff - last_refresh;
>>> +	lt->new_timeo = min(lt->new_timeo, time_remaining);
>>> +	return false;
>>> +}
>>> +
>>> static time64_t
>>> nfs4_laundromat(struct nfsd_net *nn)
>>> {
>>> @@ -5372,14 +5388,16 @@ nfs4_laundromat(struct nfsd_net *nn)
>>> 	struct nfs4_ol_stateid *stp;
>>> 	struct nfsd4_blocked_lock *nbl;
>>> 	struct list_head *pos, *next, reaplist;
>>> -	time64_t cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease;
>>> -	time64_t t, new_timeo = nn->nfsd4_lease;
>>> +	struct laundry_time lt = {
>>> +		.cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease,
>>> +		.new_timeo = nn->nfsd4_lease
>>> +	};
>>> 	struct nfs4_cpntf_state *cps;
>>> 	copy_stateid_t *cps_t;
>>> 	int i;
>>> 
>>> 	if (clients_still_reclaiming(nn)) {
>>> -		new_timeo = 0;
>>> +		lt.new_timeo = 0;
>>> 		goto out;
>>> 	}
>>> 	nfsd4_end_grace(nn);
>>> @@ -5389,7 +5407,7 @@ nfs4_laundromat(struct nfsd_net *nn)
>>> 	idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) {
>>> 		cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid);
>>> 		if (cps->cp_stateid.sc_type == NFS4_COPYNOTIFY_STID &&
>>> -				cps->cpntf_time < cutoff)
>>> +				state_expired(&lt, cps->cpntf_time))
>>> 			_free_cpntf_state_locked(nn, cps);
>>> 	}
>>> 	spin_unlock(&nn->s2s_cp_lock);
>>> @@ -5397,11 +5415,8 @@ 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 (clp->cl_time > cutoff) {
>>> -			t = clp->cl_time - cutoff;
>>> -			new_timeo = min(new_timeo, t);
>>> +		if (!state_expired(&lt, clp->cl_time))
>>> 			break;
>>> -		}
>>> 		if (mark_client_expired_locked(clp)) {
>>> 			trace_nfsd_clid_expired(&clp->cl_clientid);
>>> 			continue;
>>> @@ -5418,11 +5433,8 @@ nfs4_laundromat(struct nfsd_net *nn)
>>> 	spin_lock(&state_lock);
>>> 	list_for_each_safe(pos, next, &nn->del_recall_lru) {
>>> 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
>>> -		if (dp->dl_time > cutoff) {
>>> -			t = dp->dl_time - cutoff;
>>> -			new_timeo = min(new_timeo, t);
>>> +		if (!state_expired(&lt, clp->cl_time))
>>> 			break;
>>> -		}
>>> 		WARN_ON(!unhash_delegation_locked(dp));
>>> 		list_add(&dp->dl_recall_lru, &reaplist);
>>> 	}
>>> @@ -5438,11 +5450,8 @@ nfs4_laundromat(struct nfsd_net *nn)
>>> 	while (!list_empty(&nn->close_lru)) {
>>> 		oo = list_first_entry(&nn->close_lru, struct nfs4_openowner,
>>> 					oo_close_lru);
>>> -		if (oo->oo_time > cutoff) {
>>> -			t = oo->oo_time - cutoff;
>>> -			new_timeo = min(new_timeo, t);
>>> +		if (!state_expired(&lt, oo->oo_time))
>>> 			break;
>>> -		}
>>> 		list_del_init(&oo->oo_close_lru);
>>> 		stp = oo->oo_last_closed_stid;
>>> 		oo->oo_last_closed_stid = NULL;
>>> @@ -5468,11 +5477,8 @@ nfs4_laundromat(struct nfsd_net *nn)
>>> 	while (!list_empty(&nn->blocked_locks_lru)) {
>>> 		nbl = list_first_entry(&nn->blocked_locks_lru,
>>> 					struct nfsd4_blocked_lock, nbl_lru);
>>> -		if (nbl->nbl_time > cutoff) {
>>> -			t = nbl->nbl_time - cutoff;
>>> -			new_timeo = min(new_timeo, t);
>>> +		if (!state_expired(&lt, oo->oo_time))
>>> 			break;
>>> -		}
>>> 		list_move(&nbl->nbl_lru, &reaplist);
>>> 		list_del_init(&nbl->nbl_list);
>>> 	}
>>> @@ -5485,8 +5491,7 @@ nfs4_laundromat(struct nfsd_net *nn)
>>> 		free_blocked_lock(nbl);
>>> 	}
>>> out:
>>> -	new_timeo = max_t(time64_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
>>> -	return new_timeo;
>>> +	return max_t(time64_t, lt.new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
>>> }
>>> 
>>> static struct workqueue_struct *laundry_wq;
>> 
>> --
>> Chuck Lever
>> 
>> 

--
Chuck Lever
Olga Kornievskaia Feb. 25, 2021, 3:33 p.m. UTC | #6
On Thu, Feb 25, 2021 at 10:30 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>
> > On Feb 24, 2021, at 5:33 PM, Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Wed, Feb 24, 2021 at 07:34:10PM +0000, Chuck Lever wrote:
> >>
> >>
> >>> On Feb 24, 2021, at 2:31 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >>>
> >>> I confess I always have to stare at these comparisons for a minute to
> >>> figure out which way they should go.  And the logic in each of these
> >>> loops is a little repetitive.
> >>>
> >>> Would it be worth creating a little state_expired() helper to work out
> >>> the comparison and the new timeout?
> >>
> >> That's better, but aren't there already operators on time64_t data objects
> >> that can be used for this?
> >
> > No idea.
>
> I was thinking of jiffies, I guess. time_before() and time_after().

Just my 2c. My initial original patches used time_after(). It was
specifically changed by somebody later to use the current api.

> Checking the definition of time64_t, from include/linux/time64.h:
>
> typedef __s64 time64_t;
>
> Signed, hrm. And no comparison helpers.
>
> I might be a little concerned about wrapping time values. But any
> concerns can be addressed in the new common helper state_expired(),
> possibly as a subsequent patch.
>
> If you feel it's ready, can you send me the below clean-up as an
> official patch with description and SoB?
>
>
> > --b.
> >
> >>
> >>
> >>> --b.
> >>>
> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >>> index 61552e89bd89..00fb3603c29e 100644
> >>> --- a/fs/nfsd/nfs4state.c
> >>> +++ b/fs/nfsd/nfs4state.c
> >>> @@ -5363,6 +5363,22 @@ static bool clients_still_reclaiming(struct nfsd_net *nn)
> >>>     return true;
> >>> }
> >>>
> >>> +struct laundry_time {
> >>> +   time64_t cutoff;
> >>> +   time64_t new_timeo;
> >>> +};
> >>> +
> >>> +bool state_expired(struct laundry_time *lt, time64_t last_refresh)
> >>> +{
> >>> +   time64_t time_remaining;
> >>> +
> >>> +   if (last_refresh > lt->cutoff)
> >>> +           return true;
> >>> +   time_remaining = lt->cutoff - last_refresh;
> >>> +   lt->new_timeo = min(lt->new_timeo, time_remaining);
> >>> +   return false;
> >>> +}
> >>> +
> >>> static time64_t
> >>> nfs4_laundromat(struct nfsd_net *nn)
> >>> {
> >>> @@ -5372,14 +5388,16 @@ nfs4_laundromat(struct nfsd_net *nn)
> >>>     struct nfs4_ol_stateid *stp;
> >>>     struct nfsd4_blocked_lock *nbl;
> >>>     struct list_head *pos, *next, reaplist;
> >>> -   time64_t cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease;
> >>> -   time64_t t, new_timeo = nn->nfsd4_lease;
> >>> +   struct laundry_time lt = {
> >>> +           .cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease,
> >>> +           .new_timeo = nn->nfsd4_lease
> >>> +   };
> >>>     struct nfs4_cpntf_state *cps;
> >>>     copy_stateid_t *cps_t;
> >>>     int i;
> >>>
> >>>     if (clients_still_reclaiming(nn)) {
> >>> -           new_timeo = 0;
> >>> +           lt.new_timeo = 0;
> >>>             goto out;
> >>>     }
> >>>     nfsd4_end_grace(nn);
> >>> @@ -5389,7 +5407,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> >>>     idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) {
> >>>             cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid);
> >>>             if (cps->cp_stateid.sc_type == NFS4_COPYNOTIFY_STID &&
> >>> -                           cps->cpntf_time < cutoff)
> >>> +                           state_expired(&lt, cps->cpntf_time))
> >>>                     _free_cpntf_state_locked(nn, cps);
> >>>     }
> >>>     spin_unlock(&nn->s2s_cp_lock);
> >>> @@ -5397,11 +5415,8 @@ 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 (clp->cl_time > cutoff) {
> >>> -                   t = clp->cl_time - cutoff;
> >>> -                   new_timeo = min(new_timeo, t);
> >>> +           if (!state_expired(&lt, clp->cl_time))
> >>>                     break;
> >>> -           }
> >>>             if (mark_client_expired_locked(clp)) {
> >>>                     trace_nfsd_clid_expired(&clp->cl_clientid);
> >>>                     continue;
> >>> @@ -5418,11 +5433,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> >>>     spin_lock(&state_lock);
> >>>     list_for_each_safe(pos, next, &nn->del_recall_lru) {
> >>>             dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> >>> -           if (dp->dl_time > cutoff) {
> >>> -                   t = dp->dl_time - cutoff;
> >>> -                   new_timeo = min(new_timeo, t);
> >>> +           if (!state_expired(&lt, clp->cl_time))
> >>>                     break;
> >>> -           }
> >>>             WARN_ON(!unhash_delegation_locked(dp));
> >>>             list_add(&dp->dl_recall_lru, &reaplist);
> >>>     }
> >>> @@ -5438,11 +5450,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> >>>     while (!list_empty(&nn->close_lru)) {
> >>>             oo = list_first_entry(&nn->close_lru, struct nfs4_openowner,
> >>>                                     oo_close_lru);
> >>> -           if (oo->oo_time > cutoff) {
> >>> -                   t = oo->oo_time - cutoff;
> >>> -                   new_timeo = min(new_timeo, t);
> >>> +           if (!state_expired(&lt, oo->oo_time))
> >>>                     break;
> >>> -           }
> >>>             list_del_init(&oo->oo_close_lru);
> >>>             stp = oo->oo_last_closed_stid;
> >>>             oo->oo_last_closed_stid = NULL;
> >>> @@ -5468,11 +5477,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> >>>     while (!list_empty(&nn->blocked_locks_lru)) {
> >>>             nbl = list_first_entry(&nn->blocked_locks_lru,
> >>>                                     struct nfsd4_blocked_lock, nbl_lru);
> >>> -           if (nbl->nbl_time > cutoff) {
> >>> -                   t = nbl->nbl_time - cutoff;
> >>> -                   new_timeo = min(new_timeo, t);
> >>> +           if (!state_expired(&lt, oo->oo_time))
> >>>                     break;
> >>> -           }
> >>>             list_move(&nbl->nbl_lru, &reaplist);
> >>>             list_del_init(&nbl->nbl_list);
> >>>     }
> >>> @@ -5485,8 +5491,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> >>>             free_blocked_lock(nbl);
> >>>     }
> >>> out:
> >>> -   new_timeo = max_t(time64_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> >>> -   return new_timeo;
> >>> +   return max_t(time64_t, lt.new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> >>> }
> >>>
> >>> static struct workqueue_struct *laundry_wq;
> >>
> >> --
> >> Chuck Lever
> >>
> >>
>
> --
> Chuck Lever
>
>
>
bfields@fieldses.org Feb. 25, 2021, 3:57 p.m. UTC | #7
On Thu, Feb 25, 2021 at 10:33:04AM -0500, Olga Kornievskaia wrote:
> On Thu, Feb 25, 2021 at 10:30 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > On Feb 24, 2021, at 5:33 PM, Bruce Fields <bfields@fieldses.org> wrote:
> > > On Wed, Feb 24, 2021 at 07:34:10PM +0000, Chuck Lever wrote:
> > >>> On Feb 24, 2021, at 2:31 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > >>>
> > >>> I confess I always have to stare at these comparisons for a minute to
> > >>> figure out which way they should go.  And the logic in each of these
> > >>> loops is a little repetitive.
> > >>>
> > >>> Would it be worth creating a little state_expired() helper to work out
> > >>> the comparison and the new timeout?
> > >>
> > >> That's better, but aren't there already operators on time64_t data objects
> > >> that can be used for this?
> > >
> > > No idea.
> >
> > I was thinking of jiffies, I guess. time_before() and time_after().
> 
> Just my 2c. My initial original patches used time_after(). It was
> specifically changed by somebody later to use the current api.

Yes, that was part of some Y2038 project on Arnd Bergman's part:

	20b7d86f29d3 nfsd: use boottime for lease expiry calculation

I think 64-bit time_t is good for something like a hundred billion
years, so wraparound isn't an issue.

I think the conversion was correct, so the bug was there from the start.

Easy mistake to make, and hard to hit in testing, because on an
otherwise unoccupied server and fast network the laundromat probably
won't run while your COPY is in progress.

--b.

> 
> > Checking the definition of time64_t, from include/linux/time64.h:
> >
> > typedef __s64 time64_t;
> >
> > Signed, hrm. And no comparison helpers.
> >
> > I might be a little concerned about wrapping time values. But any
> > concerns can be addressed in the new common helper state_expired(),
> > possibly as a subsequent patch.
> >
> > If you feel it's ready, can you send me the below clean-up as an
> > official patch with description and SoB?
> >
> >
> > > --b.
> > >
> > >>
> > >>
> > >>> --b.
> > >>>
> > >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > >>> index 61552e89bd89..00fb3603c29e 100644
> > >>> --- a/fs/nfsd/nfs4state.c
> > >>> +++ b/fs/nfsd/nfs4state.c
> > >>> @@ -5363,6 +5363,22 @@ static bool clients_still_reclaiming(struct nfsd_net *nn)
> > >>>     return true;
> > >>> }
> > >>>
> > >>> +struct laundry_time {
> > >>> +   time64_t cutoff;
> > >>> +   time64_t new_timeo;
> > >>> +};
> > >>> +
> > >>> +bool state_expired(struct laundry_time *lt, time64_t last_refresh)
> > >>> +{
> > >>> +   time64_t time_remaining;
> > >>> +
> > >>> +   if (last_refresh > lt->cutoff)
> > >>> +           return true;
> > >>> +   time_remaining = lt->cutoff - last_refresh;
> > >>> +   lt->new_timeo = min(lt->new_timeo, time_remaining);
> > >>> +   return false;
> > >>> +}
> > >>> +
> > >>> static time64_t
> > >>> nfs4_laundromat(struct nfsd_net *nn)
> > >>> {
> > >>> @@ -5372,14 +5388,16 @@ nfs4_laundromat(struct nfsd_net *nn)
> > >>>     struct nfs4_ol_stateid *stp;
> > >>>     struct nfsd4_blocked_lock *nbl;
> > >>>     struct list_head *pos, *next, reaplist;
> > >>> -   time64_t cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease;
> > >>> -   time64_t t, new_timeo = nn->nfsd4_lease;
> > >>> +   struct laundry_time lt = {
> > >>> +           .cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease,
> > >>> +           .new_timeo = nn->nfsd4_lease
> > >>> +   };
> > >>>     struct nfs4_cpntf_state *cps;
> > >>>     copy_stateid_t *cps_t;
> > >>>     int i;
> > >>>
> > >>>     if (clients_still_reclaiming(nn)) {
> > >>> -           new_timeo = 0;
> > >>> +           lt.new_timeo = 0;
> > >>>             goto out;
> > >>>     }
> > >>>     nfsd4_end_grace(nn);
> > >>> @@ -5389,7 +5407,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> > >>>     idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) {
> > >>>             cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid);
> > >>>             if (cps->cp_stateid.sc_type == NFS4_COPYNOTIFY_STID &&
> > >>> -                           cps->cpntf_time < cutoff)
> > >>> +                           state_expired(&lt, cps->cpntf_time))
> > >>>                     _free_cpntf_state_locked(nn, cps);
> > >>>     }
> > >>>     spin_unlock(&nn->s2s_cp_lock);
> > >>> @@ -5397,11 +5415,8 @@ 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 (clp->cl_time > cutoff) {
> > >>> -                   t = clp->cl_time - cutoff;
> > >>> -                   new_timeo = min(new_timeo, t);
> > >>> +           if (!state_expired(&lt, clp->cl_time))
> > >>>                     break;
> > >>> -           }
> > >>>             if (mark_client_expired_locked(clp)) {
> > >>>                     trace_nfsd_clid_expired(&clp->cl_clientid);
> > >>>                     continue;
> > >>> @@ -5418,11 +5433,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> > >>>     spin_lock(&state_lock);
> > >>>     list_for_each_safe(pos, next, &nn->del_recall_lru) {
> > >>>             dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> > >>> -           if (dp->dl_time > cutoff) {
> > >>> -                   t = dp->dl_time - cutoff;
> > >>> -                   new_timeo = min(new_timeo, t);
> > >>> +           if (!state_expired(&lt, clp->cl_time))
> > >>>                     break;
> > >>> -           }
> > >>>             WARN_ON(!unhash_delegation_locked(dp));
> > >>>             list_add(&dp->dl_recall_lru, &reaplist);
> > >>>     }
> > >>> @@ -5438,11 +5450,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> > >>>     while (!list_empty(&nn->close_lru)) {
> > >>>             oo = list_first_entry(&nn->close_lru, struct nfs4_openowner,
> > >>>                                     oo_close_lru);
> > >>> -           if (oo->oo_time > cutoff) {
> > >>> -                   t = oo->oo_time - cutoff;
> > >>> -                   new_timeo = min(new_timeo, t);
> > >>> +           if (!state_expired(&lt, oo->oo_time))
> > >>>                     break;
> > >>> -           }
> > >>>             list_del_init(&oo->oo_close_lru);
> > >>>             stp = oo->oo_last_closed_stid;
> > >>>             oo->oo_last_closed_stid = NULL;
> > >>> @@ -5468,11 +5477,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> > >>>     while (!list_empty(&nn->blocked_locks_lru)) {
> > >>>             nbl = list_first_entry(&nn->blocked_locks_lru,
> > >>>                                     struct nfsd4_blocked_lock, nbl_lru);
> > >>> -           if (nbl->nbl_time > cutoff) {
> > >>> -                   t = nbl->nbl_time - cutoff;
> > >>> -                   new_timeo = min(new_timeo, t);
> > >>> +           if (!state_expired(&lt, oo->oo_time))
> > >>>                     break;
> > >>> -           }
> > >>>             list_move(&nbl->nbl_lru, &reaplist);
> > >>>             list_del_init(&nbl->nbl_list);
> > >>>     }
> > >>> @@ -5485,8 +5491,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> > >>>             free_blocked_lock(nbl);
> > >>>     }
> > >>> out:
> > >>> -   new_timeo = max_t(time64_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> > >>> -   return new_timeo;
> > >>> +   return max_t(time64_t, lt.new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> > >>> }
> > >>>
> > >>> static struct workqueue_struct *laundry_wq;
> > >>
> > >> --
> > >> Chuck Lever
> > >>
> > >>
> >
> > --
> > Chuck Lever
> >
> >
> >
Tom Talpey Feb. 25, 2021, 6:39 p.m. UTC | #8
On 2/25/2021 10:57 AM, Bruce Fields wrote:
> On Thu, Feb 25, 2021 at 10:33:04AM -0500, Olga Kornievskaia wrote:
>> On Thu, Feb 25, 2021 at 10:30 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>> On Feb 24, 2021, at 5:33 PM, Bruce Fields <bfields@fieldses.org> wrote:
>>>> On Wed, Feb 24, 2021 at 07:34:10PM +0000, Chuck Lever wrote:
>>>>>> On Feb 24, 2021, at 2:31 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>>>>>>
>>>>>> I confess I always have to stare at these comparisons for a minute to
>>>>>> figure out which way they should go.  And the logic in each of these
>>>>>> loops is a little repetitive.
>>>>>>
>>>>>> Would it be worth creating a little state_expired() helper to work out
>>>>>> the comparison and the new timeout?
>>>>>
>>>>> That's better, but aren't there already operators on time64_t data objects
>>>>> that can be used for this?
>>>>
>>>> No idea.
>>>
>>> I was thinking of jiffies, I guess. time_before() and time_after().
>>
>> Just my 2c. My initial original patches used time_after(). It was
>> specifically changed by somebody later to use the current api.
> 
> Yes, that was part of some Y2038 project on Arnd Bergman's part:
> 
> 	20b7d86f29d3 nfsd: use boottime for lease expiry calculation
> 
> I think 64-bit time_t is good for something like a hundred billion
> years, so wraparound isn't an issue.

Well, the universe is somewhere below 14B so that's good, but is this
time guaranteed to be monotonic? If someone sets the system time, many
clocks can go backward...

Tom.

> I think the conversion was correct, so the bug was there from the start.
> 
> Easy mistake to make, and hard to hit in testing, because on an
> otherwise unoccupied server and fast network the laundromat probably
> won't run while your COPY is in progress.
> 
> --b.
> 
>>
>>> Checking the definition of time64_t, from include/linux/time64.h:
>>>
>>> typedef __s64 time64_t;
>>>
>>> Signed, hrm. And no comparison helpers.
>>>
>>> I might be a little concerned about wrapping time values. But any
>>> concerns can be addressed in the new common helper state_expired(),
>>> possibly as a subsequent patch.
>>>
>>> If you feel it's ready, can you send me the below clean-up as an
>>> official patch with description and SoB?
>>>
>>>
>>>> --b.
>>>>
>>>>>
>>>>>
>>>>>> --b.
>>>>>>
>>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>>> index 61552e89bd89..00fb3603c29e 100644
>>>>>> --- a/fs/nfsd/nfs4state.c
>>>>>> +++ b/fs/nfsd/nfs4state.c
>>>>>> @@ -5363,6 +5363,22 @@ static bool clients_still_reclaiming(struct nfsd_net *nn)
>>>>>>      return true;
>>>>>> }
>>>>>>
>>>>>> +struct laundry_time {
>>>>>> +   time64_t cutoff;
>>>>>> +   time64_t new_timeo;
>>>>>> +};
>>>>>> +
>>>>>> +bool state_expired(struct laundry_time *lt, time64_t last_refresh)
>>>>>> +{
>>>>>> +   time64_t time_remaining;
>>>>>> +
>>>>>> +   if (last_refresh > lt->cutoff)
>>>>>> +           return true;
>>>>>> +   time_remaining = lt->cutoff - last_refresh;
>>>>>> +   lt->new_timeo = min(lt->new_timeo, time_remaining);
>>>>>> +   return false;
>>>>>> +}
>>>>>> +
>>>>>> static time64_t
>>>>>> nfs4_laundromat(struct nfsd_net *nn)
>>>>>> {
>>>>>> @@ -5372,14 +5388,16 @@ nfs4_laundromat(struct nfsd_net *nn)
>>>>>>      struct nfs4_ol_stateid *stp;
>>>>>>      struct nfsd4_blocked_lock *nbl;
>>>>>>      struct list_head *pos, *next, reaplist;
>>>>>> -   time64_t cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease;
>>>>>> -   time64_t t, new_timeo = nn->nfsd4_lease;
>>>>>> +   struct laundry_time lt = {
>>>>>> +           .cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease,
>>>>>> +           .new_timeo = nn->nfsd4_lease
>>>>>> +   };
>>>>>>      struct nfs4_cpntf_state *cps;
>>>>>>      copy_stateid_t *cps_t;
>>>>>>      int i;
>>>>>>
>>>>>>      if (clients_still_reclaiming(nn)) {
>>>>>> -           new_timeo = 0;
>>>>>> +           lt.new_timeo = 0;
>>>>>>              goto out;
>>>>>>      }
>>>>>>      nfsd4_end_grace(nn);
>>>>>> @@ -5389,7 +5407,7 @@ nfs4_laundromat(struct nfsd_net *nn)
>>>>>>      idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) {
>>>>>>              cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid);
>>>>>>              if (cps->cp_stateid.sc_type == NFS4_COPYNOTIFY_STID &&
>>>>>> -                           cps->cpntf_time < cutoff)
>>>>>> +                           state_expired(&lt, cps->cpntf_time))
>>>>>>                      _free_cpntf_state_locked(nn, cps);
>>>>>>      }
>>>>>>      spin_unlock(&nn->s2s_cp_lock);
>>>>>> @@ -5397,11 +5415,8 @@ 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 (clp->cl_time > cutoff) {
>>>>>> -                   t = clp->cl_time - cutoff;
>>>>>> -                   new_timeo = min(new_timeo, t);
>>>>>> +           if (!state_expired(&lt, clp->cl_time))
>>>>>>                      break;
>>>>>> -           }
>>>>>>              if (mark_client_expired_locked(clp)) {
>>>>>>                      trace_nfsd_clid_expired(&clp->cl_clientid);
>>>>>>                      continue;
>>>>>> @@ -5418,11 +5433,8 @@ nfs4_laundromat(struct nfsd_net *nn)
>>>>>>      spin_lock(&state_lock);
>>>>>>      list_for_each_safe(pos, next, &nn->del_recall_lru) {
>>>>>>              dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
>>>>>> -           if (dp->dl_time > cutoff) {
>>>>>> -                   t = dp->dl_time - cutoff;
>>>>>> -                   new_timeo = min(new_timeo, t);
>>>>>> +           if (!state_expired(&lt, clp->cl_time))
>>>>>>                      break;
>>>>>> -           }
>>>>>>              WARN_ON(!unhash_delegation_locked(dp));
>>>>>>              list_add(&dp->dl_recall_lru, &reaplist);
>>>>>>      }
>>>>>> @@ -5438,11 +5450,8 @@ nfs4_laundromat(struct nfsd_net *nn)
>>>>>>      while (!list_empty(&nn->close_lru)) {
>>>>>>              oo = list_first_entry(&nn->close_lru, struct nfs4_openowner,
>>>>>>                                      oo_close_lru);
>>>>>> -           if (oo->oo_time > cutoff) {
>>>>>> -                   t = oo->oo_time - cutoff;
>>>>>> -                   new_timeo = min(new_timeo, t);
>>>>>> +           if (!state_expired(&lt, oo->oo_time))
>>>>>>                      break;
>>>>>> -           }
>>>>>>              list_del_init(&oo->oo_close_lru);
>>>>>>              stp = oo->oo_last_closed_stid;
>>>>>>              oo->oo_last_closed_stid = NULL;
>>>>>> @@ -5468,11 +5477,8 @@ nfs4_laundromat(struct nfsd_net *nn)
>>>>>>      while (!list_empty(&nn->blocked_locks_lru)) {
>>>>>>              nbl = list_first_entry(&nn->blocked_locks_lru,
>>>>>>                                      struct nfsd4_blocked_lock, nbl_lru);
>>>>>> -           if (nbl->nbl_time > cutoff) {
>>>>>> -                   t = nbl->nbl_time - cutoff;
>>>>>> -                   new_timeo = min(new_timeo, t);
>>>>>> +           if (!state_expired(&lt, oo->oo_time))
>>>>>>                      break;
>>>>>> -           }
>>>>>>              list_move(&nbl->nbl_lru, &reaplist);
>>>>>>              list_del_init(&nbl->nbl_list);
>>>>>>      }
>>>>>> @@ -5485,8 +5491,7 @@ nfs4_laundromat(struct nfsd_net *nn)
>>>>>>              free_blocked_lock(nbl);
>>>>>>      }
>>>>>> out:
>>>>>> -   new_timeo = max_t(time64_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
>>>>>> -   return new_timeo;
>>>>>> +   return max_t(time64_t, lt.new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
>>>>>> }
>>>>>>
>>>>>> static struct workqueue_struct *laundry_wq;
>>>>>
>>>>> --
>>>>> Chuck Lever
>>>>>
>>>>>
>>>
>>> --
>>> Chuck Lever
>>>
>>>
>>>
>
bfields@fieldses.org Feb. 25, 2021, 7:21 p.m. UTC | #9
On Thu, Feb 25, 2021 at 01:39:34PM -0500, Tom Talpey wrote:
> On 2/25/2021 10:57 AM, Bruce Fields wrote:
> >Yes, that was part of some Y2038 project on Arnd Bergman's part:
> >
> >	20b7d86f29d3 nfsd: use boottime for lease expiry calculation
> >
> >I think 64-bit time_t is good for something like a hundred billion
> >years, so wraparound isn't an issue.
> 
> Well, the universe is somewhere below 14B so that's good, but is this
> time guaranteed to be monotonic? If someone sets the system time, many
> clocks can go backward...

Yeah, Arnd makes a note about this in the above commit.

--b.
diff mbox series

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 423fd6683f3a..61552e89bd89 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5389,7 +5389,7 @@  nfs4_laundromat(struct nfsd_net *nn)
 	idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) {
 		cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid);
 		if (cps->cp_stateid.sc_type == NFS4_COPYNOTIFY_STID &&
-				cps->cpntf_time > cutoff)
+				cps->cpntf_time < cutoff)
 			_free_cpntf_state_locked(nn, cps);
 	}
 	spin_unlock(&nn->s2s_cp_lock);