| Message ID | 20210302154623.GA2263@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: helper for laundromat expiry calculations | expand |
> On Mar 2, 2021, at 10:46 AM, Bruce Fields <bfields@fieldses.org> wrote: > > From: "J. Bruce Fields" <bfields@redhat.com> > > We do this same logic repeatedly, and it's easy to get the sense of the > comparison wrong. > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > fs/nfsd/nfs4state.c | 49 +++++++++++++++++++++++++-------------------- > 1 file changed, 27 insertions(+), 22 deletions(-) > > My original version of this patch... actually got the sense of the > comparison wrong! > > Now actually tested. Thanks, and pushed to the for-next topic branch of git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 61552e89bd89..8e6938902b49 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 = last_refresh - lt->cutoff; > + 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(<, dp->dl_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(<, nbl->nbl_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; > -- > 2.29.2 > -- Chuck Lever
> On Mar 2, 2021, at 10:46 AM, Bruce Fields <bfields@fieldses.org> wrote: > > From: "J. Bruce Fields" <bfields@redhat.com> > > We do this same logic repeatedly, and it's easy to get the sense of the > comparison wrong. > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > fs/nfsd/nfs4state.c | 49 +++++++++++++++++++++++++-------------------- > 1 file changed, 27 insertions(+), 22 deletions(-) > > My original version of this patch... actually got the sense of the > comparison wrong! > > Now actually tested. > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 61552e89bd89..8e6938902b49 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 = last_refresh - lt->cutoff; > + lt->new_timeo = min(lt->new_timeo, time_remaining); > + return false; > +} > + /home/cel/src/linux/linux/fs/nfsd/nfs4state.c:5371:6: warning: no previous prototype for ‘state_expired’ [-Wmissing-prototypes] 5371 | bool state_expired(struct laundry_time *lt, time64_t last_refresh) | ^~~~~~~~~~~~~ Should this new helper be static, or instead perhaps these items should be defined in a header file. > 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(<, dp->dl_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(<, nbl->nbl_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; > -- > 2.29.2 > -- Chuck Lever
On Wed, Mar 03, 2021 at 09:35:18PM +0000, Chuck Lever wrote: > > > > On Mar 2, 2021, at 10:46 AM, Bruce Fields <bfields@fieldses.org> wrote: > > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > We do this same logic repeatedly, and it's easy to get the sense of the > > comparison wrong. > > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > --- > > fs/nfsd/nfs4state.c | 49 +++++++++++++++++++++++++-------------------- > > 1 file changed, 27 insertions(+), 22 deletions(-) > > > > My original version of this patch... actually got the sense of the > > comparison wrong! > > > > Now actually tested. > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 61552e89bd89..8e6938902b49 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 = last_refresh - lt->cutoff; > > + lt->new_timeo = min(lt->new_timeo, time_remaining); > > + return false; > > +} > > + > > /home/cel/src/linux/linux/fs/nfsd/nfs4state.c:5371:6: warning: no previous prototype for ‘state_expired’ [-Wmissing-prototypes] > 5371 | bool state_expired(struct laundry_time *lt, time64_t last_refresh) > | ^~~~~~~~~~~~~ > > Should this new helper be static, or instead perhaps these items > should be defined in a header file. Whoops, should have been static, yes, do you want to fix it up or should I resend? --b. > > > 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(<, dp->dl_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(<, nbl->nbl_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; > > -- > > 2.29.2 > > > > -- > Chuck Lever > > >
> On Mar 3, 2021, at 4:55 PM, Bruce Fields <bfields@fieldses.org> wrote: > > On Wed, Mar 03, 2021 at 09:35:18PM +0000, Chuck Lever wrote: >> >> >>> On Mar 2, 2021, at 10:46 AM, Bruce Fields <bfields@fieldses.org> wrote: >>> >>> From: "J. Bruce Fields" <bfields@redhat.com> >>> >>> We do this same logic repeatedly, and it's easy to get the sense of the >>> comparison wrong. >>> >>> Signed-off-by: J. Bruce Fields <bfields@redhat.com> >>> --- >>> fs/nfsd/nfs4state.c | 49 +++++++++++++++++++++++++-------------------- >>> 1 file changed, 27 insertions(+), 22 deletions(-) >>> >>> My original version of this patch... actually got the sense of the >>> comparison wrong! >>> >>> Now actually tested. >>> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >>> index 61552e89bd89..8e6938902b49 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 = last_refresh - lt->cutoff; >>> + lt->new_timeo = min(lt->new_timeo, time_remaining); >>> + return false; >>> +} >>> + >> >> /home/cel/src/linux/linux/fs/nfsd/nfs4state.c:5371:6: warning: no previous prototype for ‘state_expired’ [-Wmissing-prototypes] >> 5371 | bool state_expired(struct laundry_time *lt, time64_t last_refresh) >> | ^~~~~~~~~~~~~ >> >> Should this new helper be static, or instead perhaps these items >> should be defined in a header file. > > Whoops, should have been static, yes, do you want to fix it up or should > I resend? I've corrected it in the for-next topic branch in git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git > --b. > >> >>> 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(<, dp->dl_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(<, nbl->nbl_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; >>> -- >>> 2.29.2 >>> >> >> -- >> Chuck Lever -- Chuck Lever
On Wed, Mar 03, 2021 at 09:59:48PM +0000, Chuck Lever wrote: > > > > On Mar 3, 2021, at 4:55 PM, Bruce Fields <bfields@fieldses.org> wrote: > > > > On Wed, Mar 03, 2021 at 09:35:18PM +0000, Chuck Lever wrote: > >> > >> > >>> On Mar 2, 2021, at 10:46 AM, Bruce Fields <bfields@fieldses.org> wrote: > >>> > >>> From: "J. Bruce Fields" <bfields@redhat.com> > >>> > >>> We do this same logic repeatedly, and it's easy to get the sense of the > >>> comparison wrong. > >>> > >>> Signed-off-by: J. Bruce Fields <bfields@redhat.com> > >>> --- > >>> fs/nfsd/nfs4state.c | 49 +++++++++++++++++++++++++-------------------- > >>> 1 file changed, 27 insertions(+), 22 deletions(-) > >>> > >>> My original version of this patch... actually got the sense of the > >>> comparison wrong! > >>> > >>> Now actually tested. > >>> > >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > >>> index 61552e89bd89..8e6938902b49 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 = last_refresh - lt->cutoff; > >>> + lt->new_timeo = min(lt->new_timeo, time_remaining); > >>> + return false; > >>> +} > >>> + > >> > >> /home/cel/src/linux/linux/fs/nfsd/nfs4state.c:5371:6: warning: no previous prototype for ‘state_expired’ [-Wmissing-prototypes] > >> 5371 | bool state_expired(struct laundry_time *lt, time64_t last_refresh) > >> | ^~~~~~~~~~~~~ > >> > >> Should this new helper be static, or instead perhaps these items > >> should be defined in a header file. > > > > Whoops, should have been static, yes, do you want to fix it up or should > > I resend? > > I've corrected it in the for-next topic branch in > > git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git Thank you! --b.
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 61552e89bd89..8e6938902b49 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 = last_refresh - lt->cutoff; + 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(<, dp->dl_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(<, nbl->nbl_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;