* [PATCH] nfsd: don't abort copies early @ 2021-02-24 18:39 J. Bruce Fields 2021-02-24 18:47 ` Chuck Lever 2021-02-24 19:31 ` J. Bruce Fields 0 siblings, 2 replies; 10+ messages in thread From: J. Bruce Fields @ 2021-02-24 18:39 UTC (permalink / raw) To: Chuck Lever; +Cc: linux-nfs, Olga Kornievskaia 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(-) 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] nfsd: don't abort copies early 2021-02-24 18:39 [PATCH] nfsd: don't abort copies early J. Bruce Fields @ 2021-02-24 18:47 ` Chuck Lever 2021-02-24 19:31 ` J. Bruce Fields 1 sibling, 0 replies; 10+ messages in thread From: Chuck Lever @ 2021-02-24 18:47 UTC (permalink / raw) To: Bruce Fields; +Cc: Linux NFS Mailing List, Olga Kornievskaia > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nfsd: don't abort copies early 2021-02-24 18:39 [PATCH] nfsd: don't abort copies early J. Bruce Fields 2021-02-24 18:47 ` Chuck Lever @ 2021-02-24 19:31 ` J. Bruce Fields 2021-02-24 19:34 ` Chuck Lever 1 sibling, 1 reply; 10+ messages in thread From: J. Bruce Fields @ 2021-02-24 19:31 UTC (permalink / raw) To: Chuck Lever; +Cc: linux-nfs, Olga Kornievskaia 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; ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] nfsd: don't abort copies early 2021-02-24 19:31 ` J. Bruce Fields @ 2021-02-24 19:34 ` Chuck Lever 2021-02-24 22:33 ` Bruce Fields 0 siblings, 1 reply; 10+ messages in thread From: Chuck Lever @ 2021-02-24 19:34 UTC (permalink / raw) To: Bruce Fields; +Cc: Linux NFS Mailing List, Olga Kornievskaia > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nfsd: don't abort copies early 2021-02-24 19:34 ` Chuck Lever @ 2021-02-24 22:33 ` Bruce Fields 2021-02-25 15:28 ` Chuck Lever 0 siblings, 1 reply; 10+ messages in thread From: Bruce Fields @ 2021-02-24 22:33 UTC (permalink / raw) To: Chuck Lever; +Cc: Linux NFS Mailing List, Olga Kornievskaia 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 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nfsd: don't abort copies early 2021-02-24 22:33 ` Bruce Fields @ 2021-02-25 15:28 ` Chuck Lever 2021-02-25 15:33 ` Olga Kornievskaia 0 siblings, 1 reply; 10+ messages in thread From: Chuck Lever @ 2021-02-25 15:28 UTC (permalink / raw) To: Bruce Fields; +Cc: Linux NFS Mailing List, Olga Kornievskaia > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nfsd: don't abort copies early 2021-02-25 15:28 ` Chuck Lever @ 2021-02-25 15:33 ` Olga Kornievskaia 2021-02-25 15:57 ` Bruce Fields 0 siblings, 1 reply; 10+ messages in thread From: Olga Kornievskaia @ 2021-02-25 15:33 UTC (permalink / raw) To: Chuck Lever; +Cc: Bruce Fields, Linux NFS Mailing List, Olga Kornievskaia 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 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nfsd: don't abort copies early 2021-02-25 15:33 ` Olga Kornievskaia @ 2021-02-25 15:57 ` Bruce Fields 2021-02-25 18:39 ` Tom Talpey 0 siblings, 1 reply; 10+ messages in thread From: Bruce Fields @ 2021-02-25 15:57 UTC (permalink / raw) To: Olga Kornievskaia; +Cc: Chuck Lever, Linux NFS Mailing List, Olga Kornievskaia 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 > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nfsd: don't abort copies early 2021-02-25 15:57 ` Bruce Fields @ 2021-02-25 18:39 ` Tom Talpey 2021-02-25 19:21 ` Bruce Fields 0 siblings, 1 reply; 10+ messages in thread From: Tom Talpey @ 2021-02-25 18:39 UTC (permalink / raw) To: Bruce Fields, Olga Kornievskaia Cc: Chuck Lever, Linux NFS Mailing List, Olga Kornievskaia 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 >>> >>> >>> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nfsd: don't abort copies early 2021-02-25 18:39 ` Tom Talpey @ 2021-02-25 19:21 ` Bruce Fields 0 siblings, 0 replies; 10+ messages in thread From: Bruce Fields @ 2021-02-25 19:21 UTC (permalink / raw) To: Tom Talpey Cc: Olga Kornievskaia, Chuck Lever, Linux NFS Mailing List, Olga Kornievskaia 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-02-25 19:24 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-02-24 18:39 [PATCH] nfsd: don't abort copies early J. Bruce Fields 2021-02-24 18:47 ` Chuck Lever 2021-02-24 19:31 ` J. Bruce Fields 2021-02-24 19:34 ` Chuck Lever 2021-02-24 22:33 ` Bruce Fields 2021-02-25 15:28 ` Chuck Lever 2021-02-25 15:33 ` Olga Kornievskaia 2021-02-25 15:57 ` Bruce Fields 2021-02-25 18:39 ` Tom Talpey 2021-02-25 19:21 ` Bruce Fields
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox