Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 25 Feb 2021 10:57:15 -0500
From:   Bruce Fields <bfields@fieldses.org>
To:     Olga Kornievskaia <aglo@umich.edu>
Cc:     Chuck Lever <chuck.lever@oracle.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Olga Kornievskaia <kolga@netapp.com>
Subject: Re: [PATCH] nfsd: don't abort copies early
Message-ID: <20210225155715.GD17634@fieldses.org>
References: <20210224183950.GB11591@fieldses.org>
 <20210224193135.GC11591@fieldses.org>
 <59A5F476-EE0C-454F-8365-3F16505D9D45@oracle.com>
 <20210224223349.GB25689@fieldses.org>
 <CDFF4F84-1A0C-4E4C-A18B-AC39F715E6D8@oracle.com>
 <CAN-5tyFLLLfMwZVFrnQaCnR36Rfq_hPsmLEOLG2Qtrpc+pT7fA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAN-5tyFLLLfMwZVFrnQaCnR36Rfq_hPsmLEOLG2Qtrpc+pT7fA@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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(&lt, cps->cpntf_time))
> > >>>                     _free_cpntf_state_locked(nn, cps);
> > >>>     }
> > >>>     spin_unlock(&nn->s2s_cp_lock);
> > >>> @@ -5397,11 +5415,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> > >>>     spin_lock(&nn->client_lock);
> > >>>     list_for_each_safe(pos, next, &nn->client_lru) {
> > >>>             clp = list_entry(pos, struct nfs4_client, cl_lru);
> > >>> -           if (clp->cl_time > cutoff) {
> > >>> -                   t = clp->cl_time - cutoff;
> > >>> -                   new_timeo = min(new_timeo, t);
> > >>> +           if (!state_expired(&lt, clp->cl_time))
> > >>>                     break;
> > >>> -           }
> > >>>             if (mark_client_expired_locked(clp)) {
> > >>>                     trace_nfsd_clid_expired(&clp->cl_clientid);
> > >>>                     continue;
> > >>> @@ -5418,11 +5433,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> > >>>     spin_lock(&state_lock);
> > >>>     list_for_each_safe(pos, next, &nn->del_recall_lru) {
> > >>>             dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> > >>> -           if (dp->dl_time > cutoff) {
> > >>> -                   t = dp->dl_time - cutoff;
> > >>> -                   new_timeo = min(new_timeo, t);
> > >>> +           if (!state_expired(&lt, clp->cl_time))
> > >>>                     break;
> > >>> -           }
> > >>>             WARN_ON(!unhash_delegation_locked(dp));
> > >>>             list_add(&dp->dl_recall_lru, &reaplist);
> > >>>     }
> > >>> @@ -5438,11 +5450,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> > >>>     while (!list_empty(&nn->close_lru)) {
> > >>>             oo = list_first_entry(&nn->close_lru, struct nfs4_openowner,
> > >>>                                     oo_close_lru);
> > >>> -           if (oo->oo_time > cutoff) {
> > >>> -                   t = oo->oo_time - cutoff;
> > >>> -                   new_timeo = min(new_timeo, t);
> > >>> +           if (!state_expired(&lt, oo->oo_time))
> > >>>                     break;
> > >>> -           }
> > >>>             list_del_init(&oo->oo_close_lru);
> > >>>             stp = oo->oo_last_closed_stid;
> > >>>             oo->oo_last_closed_stid = NULL;
> > >>> @@ -5468,11 +5477,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> > >>>     while (!list_empty(&nn->blocked_locks_lru)) {
> > >>>             nbl = list_first_entry(&nn->blocked_locks_lru,
> > >>>                                     struct nfsd4_blocked_lock, nbl_lru);
> > >>> -           if (nbl->nbl_time > cutoff) {
> > >>> -                   t = nbl->nbl_time - cutoff;
> > >>> -                   new_timeo = min(new_timeo, t);
> > >>> +           if (!state_expired(&lt, oo->oo_time))
> > >>>                     break;
> > >>> -           }
> > >>>             list_move(&nbl->nbl_lru, &reaplist);
> > >>>             list_del_init(&nbl->nbl_list);
> > >>>     }
> > >>> @@ -5485,8 +5491,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> > >>>             free_blocked_lock(nbl);
> > >>>     }
> > >>> out:
> > >>> -   new_timeo = max_t(time64_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> > >>> -   return new_timeo;
> > >>> +   return max_t(time64_t, lt.new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> > >>> }
> > >>>
> > >>> static struct workqueue_struct *laundry_wq;
> > >>
> > >> --
> > >> Chuck Lever
> > >>
> > >>
> >
> > --
> > Chuck Lever
> >
> >
> >