Re: [PATCH 04/11] NFSv4/flexfiles: make dss_info->start_time write-once

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Nov 05, 2026 at 11:14:10AM -0500, Anna Schumaker wrote:
> Hi MIke,
> 
> On Mon, Nov 2, 2026, at 2:11 PM, Mike Snitzer wrote:
> > From: Mike Snitzer <snitzer@hammerspace.com>
> >
> > start_time was written under the stripe lock in
> > nfs4_ff_layoutstat_start_io() and read outside it in
> > ff_layout_encode_ff_layoutupdate().  Pre-existing -- mirror->lock had the
> > same shape -- and benign, since it is a single naturally aligned 8 byte
> > value, but it is an unannotated race either way.
> 
> We can probably drop this last sentence.
> 
> >
> > The lock is the wrong tool for it.  ffl_duration is measured from this
> > stripe's first I/O, so the field is written once and read-only for the
> > rest of the stripe's life.  Say that instead: publish it with
> 
> This portion needs editing too. My brain tripped over "Say that instead"
> while reading.
> 
> > try_cmpxchg64() and let the first I/O win, and read it with READ_ONCE().
> > Only the first I/O to a stripe pays the read-modify-write; every one
> > after it short circuits on the READ_ONCE().
> 
> This sentence is just rewording the previous sentence.

Sorry, I will review all commit headers and code comments.
 
> >
> > ffl->last_report_time two lines away is already published this way, so
> > the pattern is not new to this file.
> >
> > This also stops start_time depending on there being exactly one lock per
> > stripe, which a later patch changes.
> >
> > Signed-off-by: Mike Snitzer <snitzer@hammerspace.com>
> > ---
> >  fs/nfs/flexfilelayout/flexfilelayout.c | 21 ++++++++++++++++++---
> >  fs/nfs/flexfilelayout/flexfilelayout.h |  3 ++-
> >  2 files changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c 
> > b/fs/nfs/flexfilelayout/flexfilelayout.c
> > index 7fc1369df8439..7c328f2ec900d 100644
> > --- a/fs/nfs/flexfilelayout/flexfilelayout.c
> > +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
> > @@ -729,6 +729,22 @@ nfs4_ff_end_busy_timer(struct nfs4_ff_busy_timer 
> > *timer, ktime_t now)
> >  	return ktime_sub(now, start);
> >  }
> > 
> > +/*
> > + * ffl_duration is measured from this stripe's first I/O, so 
> > start_time is
> > + * written once and read-only for the rest of the stripe's life.  
> > Publish it
> > + * with a cmpxchg and let the first I/O win, rather than depending on 
> > a lock
> > + * that every reader would then have to take.
> > + */
> > +static void
> > +nfs4_ff_layoutstat_set_start_time(struct nfs4_ff_layout_ds_stripe 
> > *dss_info,
> > +				  ktime_t now)
> > +{
> > +	ktime_t unset = 0;
> > +
> > +	if (!READ_ONCE(dss_info->start_time))
> > +		try_cmpxchg64(&dss_info->start_time, &unset, now);
> 
> I have the same atomic64_try_cmpxchg() question as in patch 1 here.

Right, I noted that in my reply to patch 1.  I don't think its
something to be overly concerned about.  Given the use of ktime_t
(s64) it really is the more appropriate interface AFAICT.

Happy to take instruction from others on this though!

Thanks,
Mike