2021-02-24 18:43:05

by J. Bruce Fields

Subject: [PATCH] nfsd: don't abort copies early

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>
---
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

2021-02-24 18:51:31

by Chuck Lever III

Subject: Re: [PATCH] nfsd: don't abort copies early

> On Feb 24, 2021, at 1:39 PM, J. Bruce Fields <[email protected]> wrote:
>
> From: "J. Bruce Fields" <[email protected]>
>
> 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 <[email protected]>

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

2021-02-24 19:34:02

by J. Bruce Fields

Subject: Re: [PATCH] nfsd: don't abort copies early

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;

2021-02-24 19:35:58

by Chuck Lever III

Subject: Re: [PATCH] nfsd: don't abort copies early

> On Feb 24, 2021, at 2:31 PM, J. Bruce Fields <[email protected]> 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

2021-02-25 03:58:04

by J. Bruce Fields

Subject: Re: [PATCH] nfsd: don't abort copies early

On Wed, Feb 24, 2021 at 07:34:10PM +0000, Chuck Lever wrote:
>
>
> > On Feb 24, 2021, at 2:31 PM, J. Bruce Fields <[email protected]> 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
>
>

2021-02-25 15:30:41

by Chuck Lever III

Subject: Re: [PATCH] nfsd: don't abort copies early

> On Feb 24, 2021, at 5:33 PM, Bruce Fields <[email protected]> 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 <[email protected]> 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

2021-02-25 15:34:39

by Olga Kornievskaia

Subject: Re: [PATCH] nfsd: don't abort copies early

On Thu, Feb 25, 2021 at 10:30 AM Chuck Lever <[email protected]> wrote:
>
>
>
> > On Feb 24, 2021, at 5:33 PM, Bruce Fields <[email protected]> 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 <[email protected]> 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
>
>
>

2021-02-25 15:58:46

by J. Bruce Fields

Subject: Re: [PATCH] nfsd: don't abort copies early

On Thu, Feb 25, 2021 at 10:33:04AM -0500, Olga Kornievskaia wrote:
> On Thu, Feb 25, 2021 at 10:30 AM Chuck Lever <[email protected]> wrote:
> > > On Feb 24, 2021, at 5:33 PM, Bruce Fields <[email protected]> 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 <[email protected]> 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
> >
> >
> >

2021-02-25 18:51:05

by Tom Talpey

Subject: Re: [PATCH] nfsd: don't abort copies early

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 <[email protected]> wrote:
>>>> On Feb 24, 2021, at 5:33 PM, Bruce Fields <[email protected]> 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 <[email protected]> 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
>>>
>>>
>>>
>

2021-02-25 19:25:36

by J. Bruce Fields

Subject: Re: [PATCH] nfsd: don't abort copies early

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.