2021-03-04 06:34:26

by J. Bruce Fields

Subject: [PATCH] nfsd: helper for laundromat expiry calculations

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

We do this same logic repeatedly, and it's easy to get the sense of the
comparison wrong.

Signed-off-by: J. Bruce Fields <[email protected]>
---
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;
+}
+
static time64_t
nfs4_laundromat(struct nfsd_net *nn)
{
@@ -5372,14 +5388,16 @@ nfs4_laundromat(struct nfsd_net *nn)
struct nfs4_ol_stateid *stp;
struct nfsd4_blocked_lock *nbl;
struct list_head *pos, *next, reaplist;
- time64_t cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease;
- time64_t t, new_timeo = nn->nfsd4_lease;
+ struct laundry_time lt = {
+ .cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease,
+ .new_timeo = nn->nfsd4_lease
+ };
struct nfs4_cpntf_state *cps;
copy_stateid_t *cps_t;
int i;

if (clients_still_reclaiming(nn)) {
- new_timeo = 0;
+ lt.new_timeo = 0;
goto out;
}
nfsd4_end_grace(nn);
@@ -5389,7 +5407,7 @@ nfs4_laundromat(struct nfsd_net *nn)
idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) {
cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid);
if (cps->cp_stateid.sc_type == NFS4_COPYNOTIFY_STID &&
- cps->cpntf_time < cutoff)
+ state_expired(&lt, cps->cpntf_time))
_free_cpntf_state_locked(nn, cps);
}
spin_unlock(&nn->s2s_cp_lock);
@@ -5397,11 +5415,8 @@ nfs4_laundromat(struct nfsd_net *nn)
spin_lock(&nn->client_lock);
list_for_each_safe(pos, next, &nn->client_lru) {
clp = list_entry(pos, struct nfs4_client, cl_lru);
- if (clp->cl_time > cutoff) {
- t = clp->cl_time - cutoff;
- new_timeo = min(new_timeo, t);
+ if (!state_expired(&lt, clp->cl_time))
break;
- }
if (mark_client_expired_locked(clp)) {
trace_nfsd_clid_expired(&clp->cl_clientid);
continue;
@@ -5418,11 +5433,8 @@ nfs4_laundromat(struct nfsd_net *nn)
spin_lock(&state_lock);
list_for_each_safe(pos, next, &nn->del_recall_lru) {
dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
- if (dp->dl_time > cutoff) {
- t = dp->dl_time - cutoff;
- new_timeo = min(new_timeo, t);
+ if (!state_expired(&lt, 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(&lt, oo->oo_time))
break;
- }
list_del_init(&oo->oo_close_lru);
stp = oo->oo_last_closed_stid;
oo->oo_last_closed_stid = NULL;
@@ -5468,11 +5477,8 @@ nfs4_laundromat(struct nfsd_net *nn)
while (!list_empty(&nn->blocked_locks_lru)) {
nbl = list_first_entry(&nn->blocked_locks_lru,
struct nfsd4_blocked_lock, nbl_lru);
- if (nbl->nbl_time > cutoff) {
- t = nbl->nbl_time - cutoff;
- new_timeo = min(new_timeo, t);
+ if (!state_expired(&lt, 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

2021-03-04 14:01:45

by J. Bruce Fields

Subject: Re: [PATCH] nfsd: helper for laundromat expiry calculations

On Wed, Mar 03, 2021 at 09:59:48PM +0000, Chuck Lever wrote:
>
>
> > On Mar 3, 2021, at 4:55 PM, Bruce Fields <[email protected]> wrote:
> >
> > On Wed, Mar 03, 2021 at 09:35:18PM +0000, Chuck Lever wrote:
> >>
> >>
> >>> On Mar 2, 2021, at 10:46 AM, Bruce Fields <[email protected]> wrote:
> >>>
> >>> From: "J. Bruce Fields" <[email protected]>
> >>>
> >>> We do this same logic repeatedly, and it's easy to get the sense of the
> >>> comparison wrong.
> >>>
> >>> Signed-off-by: J. Bruce Fields <[email protected]>
> >>> ---
> >>> 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.

2021-03-04 14:02:46

by J. Bruce Fields

Subject: Re: [PATCH] nfsd: helper for laundromat expiry calculations

On Wed, Mar 03, 2021 at 09:35:18PM +0000, Chuck Lever wrote:
>
>
> > On Mar 2, 2021, at 10:46 AM, Bruce Fields <[email protected]> wrote:
> >
> > From: "J. Bruce Fields" <[email protected]>
> >
> > We do this same logic repeatedly, and it's easy to get the sense of the
> > comparison wrong.
> >
> > Signed-off-by: J. Bruce Fields <[email protected]>
> > ---
> > 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(&lt, cps->cpntf_time))
> > _free_cpntf_state_locked(nn, cps);
> > }
> > spin_unlock(&nn->s2s_cp_lock);
> > @@ -5397,11 +5415,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> > spin_lock(&nn->client_lock);
> > list_for_each_safe(pos, next, &nn->client_lru) {
> > clp = list_entry(pos, struct nfs4_client, cl_lru);
> > - if (clp->cl_time > cutoff) {
> > - t = clp->cl_time - cutoff;
> > - new_timeo = min(new_timeo, t);
> > + if (!state_expired(&lt, clp->cl_time))
> > break;
> > - }
> > if (mark_client_expired_locked(clp)) {
> > trace_nfsd_clid_expired(&clp->cl_clientid);
> > continue;
> > @@ -5418,11 +5433,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> > spin_lock(&state_lock);
> > list_for_each_safe(pos, next, &nn->del_recall_lru) {
> > dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> > - if (dp->dl_time > cutoff) {
> > - t = dp->dl_time - cutoff;
> > - new_timeo = min(new_timeo, t);
> > + if (!state_expired(&lt, 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(&lt, oo->oo_time))
> > break;
> > - }
> > list_del_init(&oo->oo_close_lru);
> > stp = oo->oo_last_closed_stid;
> > oo->oo_last_closed_stid = NULL;
> > @@ -5468,11 +5477,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> > while (!list_empty(&nn->blocked_locks_lru)) {
> > nbl = list_first_entry(&nn->blocked_locks_lru,
> > struct nfsd4_blocked_lock, nbl_lru);
> > - if (nbl->nbl_time > cutoff) {
> > - t = nbl->nbl_time - cutoff;
> > - new_timeo = min(new_timeo, t);
> > + if (!state_expired(&lt, 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
>
>
>

2021-03-04 14:04:09

by Chuck Lever

Subject: Re: [PATCH] nfsd: helper for laundromat expiry calculations

> On Mar 2, 2021, at 10:46 AM, Bruce Fields <[email protected]> wrote:
>
> From: "J. Bruce Fields" <[email protected]>
>
> We do this same logic repeatedly, and it's easy to get the sense of the
> comparison wrong.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> 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(&lt, cps->cpntf_time))
> _free_cpntf_state_locked(nn, cps);
> }
> spin_unlock(&nn->s2s_cp_lock);
> @@ -5397,11 +5415,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> spin_lock(&nn->client_lock);
> list_for_each_safe(pos, next, &nn->client_lru) {
> clp = list_entry(pos, struct nfs4_client, cl_lru);
> - if (clp->cl_time > cutoff) {
> - t = clp->cl_time - cutoff;
> - new_timeo = min(new_timeo, t);
> + if (!state_expired(&lt, clp->cl_time))
> break;
> - }
> if (mark_client_expired_locked(clp)) {
> trace_nfsd_clid_expired(&clp->cl_clientid);
> continue;
> @@ -5418,11 +5433,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> spin_lock(&state_lock);
> list_for_each_safe(pos, next, &nn->del_recall_lru) {
> dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> - if (dp->dl_time > cutoff) {
> - t = dp->dl_time - cutoff;
> - new_timeo = min(new_timeo, t);
> + if (!state_expired(&lt, 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(&lt, oo->oo_time))
> break;
> - }
> list_del_init(&oo->oo_close_lru);
> stp = oo->oo_last_closed_stid;
> oo->oo_last_closed_stid = NULL;
> @@ -5468,11 +5477,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> while (!list_empty(&nn->blocked_locks_lru)) {
> nbl = list_first_entry(&nn->blocked_locks_lru,
> struct nfsd4_blocked_lock, nbl_lru);
> - if (nbl->nbl_time > cutoff) {
> - t = nbl->nbl_time - cutoff;
> - new_timeo = min(new_timeo, t);
> + if (!state_expired(&lt, 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

2021-03-04 14:24:35

by Chuck Lever

Subject: Re: [PATCH] nfsd: helper for laundromat expiry calculations

> On Mar 3, 2021, at 4:55 PM, Bruce Fields <[email protected]> wrote:
>
> On Wed, Mar 03, 2021 at 09:35:18PM +0000, Chuck Lever wrote:
>>
>>
>>> On Mar 2, 2021, at 10:46 AM, Bruce Fields <[email protected]> wrote:
>>>
>>> From: "J. Bruce Fields" <[email protected]>
>>>
>>> We do this same logic repeatedly, and it's easy to get the sense of the
>>> comparison wrong.
>>>
>>> Signed-off-by: J. Bruce Fields <[email protected]>
>>> ---
>>> 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(&lt, cps->cpntf_time))
>>> _free_cpntf_state_locked(nn, cps);
>>> }
>>> spin_unlock(&nn->s2s_cp_lock);
>>> @@ -5397,11 +5415,8 @@ nfs4_laundromat(struct nfsd_net *nn)
>>> spin_lock(&nn->client_lock);
>>> list_for_each_safe(pos, next, &nn->client_lru) {
>>> clp = list_entry(pos, struct nfs4_client, cl_lru);
>>> - if (clp->cl_time > cutoff) {
>>> - t = clp->cl_time - cutoff;
>>> - new_timeo = min(new_timeo, t);
>>> + if (!state_expired(&lt, clp->cl_time))
>>> break;
>>> - }
>>> if (mark_client_expired_locked(clp)) {
>>> trace_nfsd_clid_expired(&clp->cl_clientid);
>>> continue;
>>> @@ -5418,11 +5433,8 @@ nfs4_laundromat(struct nfsd_net *nn)
>>> spin_lock(&state_lock);
>>> list_for_each_safe(pos, next, &nn->del_recall_lru) {
>>> dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
>>> - if (dp->dl_time > cutoff) {
>>> - t = dp->dl_time - cutoff;
>>> - new_timeo = min(new_timeo, t);
>>> + if (!state_expired(&lt, 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(&lt, oo->oo_time))
>>> break;
>>> - }
>>> list_del_init(&oo->oo_close_lru);
>>> stp = oo->oo_last_closed_stid;
>>> oo->oo_last_closed_stid = NULL;
>>> @@ -5468,11 +5477,8 @@ nfs4_laundromat(struct nfsd_net *nn)
>>> while (!list_empty(&nn->blocked_locks_lru)) {
>>> nbl = list_first_entry(&nn->blocked_locks_lru,
>>> struct nfsd4_blocked_lock, nbl_lru);
>>> - if (nbl->nbl_time > cutoff) {
>>> - t = nbl->nbl_time - cutoff;
>>> - new_timeo = min(new_timeo, t);
>>> + if (!state_expired(&lt, 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

2021-03-04 23:36:37

by Chuck Lever

Subject: Re: [PATCH] nfsd: helper for laundromat expiry calculations

> On Mar 2, 2021, at 10:46 AM, Bruce Fields <[email protected]> wrote:
>
> From: "J. Bruce Fields" <[email protected]>
>
> We do this same logic repeatedly, and it's easy to get the sense of the
> comparison wrong.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> 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(&lt, cps->cpntf_time))
> _free_cpntf_state_locked(nn, cps);
> }
> spin_unlock(&nn->s2s_cp_lock);
> @@ -5397,11 +5415,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> spin_lock(&nn->client_lock);
> list_for_each_safe(pos, next, &nn->client_lru) {
> clp = list_entry(pos, struct nfs4_client, cl_lru);
> - if (clp->cl_time > cutoff) {
> - t = clp->cl_time - cutoff;
> - new_timeo = min(new_timeo, t);
> + if (!state_expired(&lt, clp->cl_time))
> break;
> - }
> if (mark_client_expired_locked(clp)) {
> trace_nfsd_clid_expired(&clp->cl_clientid);
> continue;
> @@ -5418,11 +5433,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> spin_lock(&state_lock);
> list_for_each_safe(pos, next, &nn->del_recall_lru) {
> dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> - if (dp->dl_time > cutoff) {
> - t = dp->dl_time - cutoff;
> - new_timeo = min(new_timeo, t);
> + if (!state_expired(&lt, 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(&lt, oo->oo_time))
> break;
> - }
> list_del_init(&oo->oo_close_lru);
> stp = oo->oo_last_closed_stid;
> oo->oo_last_closed_stid = NULL;
> @@ -5468,11 +5477,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> while (!list_empty(&nn->blocked_locks_lru)) {
> nbl = list_first_entry(&nn->blocked_locks_lru,
> struct nfsd4_blocked_lock, nbl_lru);
> - if (nbl->nbl_time > cutoff) {
> - t = nbl->nbl_time - cutoff;
> - new_timeo = min(new_timeo, t);
> + if (!state_expired(&lt, 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