Re: [PATCH 01/11] NFSv4/flexfiles: move layoutstats accounting to a per-stripe lock

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Nov 05, 2026 at 11:12:44AM -0500, Anna Schumaker wrote:
> Hi Mike,
> 
> On Mon, Nov 2, 2026, at 2:10 PM, Mike Snitzer wrote:
> > From: Jonathan Flynn <jonathan.flynn@hammerspace.com>
> >
> > Since striped layouts were introduced, a mirror owns an array of
> > struct nfs4_ff_layout_ds_stripe and each stripe carries its own
> > read_stat and write_stat.  The lock protecting those statistics stayed
> > behind in the mirror, so every DS RPC in the layout serialises on the
> > same spinlock twice: once in ff_layout_{read,write}_prepare_common()
> > and again on completion in ff_layout_{read,write}_count_stats().
> >
> > For a RAID0-style layout (one mirror, many stripes) that lock is
> > effectively per-file, and striping then multiplies RPC parallelism
> > without multiplying lock parallelism.
> >
> > On a 64-core client reading a single file from a 32-stripe layout with
> > a 128KiB stripe unit (2MB O_DIRECT reads, ~36.7 GB/s, ~275k NFSv3 READ
> > RPCs/s, i.e. ~550k acquisitions/s of one lock), perf reports 45% of all
> > CPU in native_queued_spin_lock_slowpath, and lock:contention_begin
> > attributes 73% of the contention events to the two layoutstats paths:
> >
> >   36.4%  rpc_exit_task -> ff_layout_read_count_stats ->
> >          ff_layout_read_record_layoutstats_done -> _raw_spin_lock
> >   36.3%  ff_layout_read_prepare_v3 ->
> >          ff_layout_read_record_layoutstats_start -> _raw_spin_lock
> >
> > Move the lock into struct nfs4_ff_layout_ds_stripe, next to the
> > statistics it protects, so that I/O to different stripes no longer
> > contends.  The reporting paths take the same per-stripe lock.
> >
> > NFS4_FF_MIRROR_STAT_AVAIL is a mirror-wide bit operation and does not
> > need the lock; set it outside the critical section so the mirror's
> > cacheline is not written under it.
> >
> > ffl->last_report_time is shared by every mirror and stripe of a layout
> > and was already updated under different mirror locks, so it was never
> > serialised.  Make that explicit with a cmpxchg(): whichever racer wins
> > sends the LAYOUTSTATS report.
> >
> > Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
> > Signed-off-by: Mike Snitzer <snitzer@hammerspace.com>
> > ---
> >  fs/nfs/flexfilelayout/flexfilelayout.c | 45 +++++++++++++++-----------
> >  fs/nfs/flexfilelayout/flexfilelayout.h |  2 ++
> >  2 files changed, 28 insertions(+), 19 deletions(-)
> >
> > diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c 
> > b/fs/nfs/flexfilelayout/flexfilelayout.c
> > index 9492d6bc1d1a7..bac0b7f559201 100644
> > --- a/fs/nfs/flexfilelayout/flexfilelayout.c
> > +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
> > @@ -744,13 +747,17 @@ nfs4_ff_layoutstat_start_io(struct 
> > nfs4_ff_layout_mirror *mirror,
> >  		report_interval = (s64)mirror->report_interval * 1000LL;
> >  	else if (layoutstats_timer != 0)
> >  		report_interval = (s64)layoutstats_timer * 1000LL;
> > -	if (ktime_to_ms(ktime_sub(now, ffl->last_report_time)) >=
> > -			report_interval) {
> > -		ffl->last_report_time = now;
> > -		return true;
> > -	}
> > 
> > -	return false;
> > +	/*
> > +	 * last_report_time is shared by every mirror and stripe of this
> > +	 * layout, so it was never serialised by the mirror lock either.
> > +	 * Make that explicit: whoever wins the cmpxchg() reports.
> > +	 */
> > +	last_report = READ_ONCE(ffl->last_report_time);
> > +	if (ktime_to_ms(ktime_sub(now, last_report)) < report_interval)
> > +		return false;
> > +
> > +	return try_cmpxchg64(&ffl->last_report_time, &last_report, now);
> 
> As far as I can tell, this is the first use of try_cmpxchg64 in any filesystem
> code. This has me worried it's the wrong function to be calling here. Is there
> any reason we can't use atomic64_try_cmpxchg(), which is what I see being used
> by other filesystem code?

Not seeing any other caller of atomic64_try_cmpxchg() that isn't using
atomic64_t.

nfs4_flexfile_layout's last_report_time datatype is s64 (ktime_t), not
atomic64_t.

Later in this series nfs4_ff_layoutstat_set_start_time() also uses it
with nfs4_ff_layout_mirror's start_time.