| Message ID | 20210224183950.GB11591@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: don't abort copies early | expand |
> 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
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(<, 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(<, 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(<, 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(<, 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(<, 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;
> 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(<, 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(<, 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(<, 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(<, 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(<, 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
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(<, 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(<, 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(<, 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(<, 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(<, 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 > >
> 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(<, 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(<, 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(<, 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(<, 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(<, 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
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(<, 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(<, 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(<, 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(<, 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(<, 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 > > >
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(<, 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(<, 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(<, 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(<, 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(<, 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 > > > > > >
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(<, 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(<, 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(<, 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(<, 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(<, 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 >>> >>> >>> >
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 --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);