[PATCH 00/11] NFSv4/flexfiles: scale layoutstats for striped layouts

[PATCH 00/11] NFSv4/flexfiles: scale layoutstats for striped layouts

[Mike Snitzer]

Hi,

Since striped flexfiles layouts were introduced, each stripe of a mirror
carries its own read_stat and write_stat, but the lock protecting them
stayed in the mirror.  For a RAID0-style layout (one mirror, many
stripes) that lock is effectively per-file: every DS RPC takes it twice,
once at prepare and once at completion, so striping multiplies RPC
parallelism without multiplying lock parallelism.

On a 64-core client reading one file from a 32-stripe layout with a
128KiB stripe unit (~36.7 GB/s, ~275k NFSv3 READ RPCs/s), perf reports
45% of all CPU in native_queued_spin_lock_slowpath, and 73% of the lock
contention events come from the two layoutstats paths.

This series removes that bottleneck and, along the way, fixes what the
server is told in LAYOUTSTATS for a striped mirror.

Patches 1-2 (Jonathan) move the layoutstats lock into the stripe, next
to the statistics it protects, and remove the now-unused mirror lock.

Patches 3-8 keep the per-stripe hot path to the stripe's own cachelines:
  3: cacheline align the per-stripe statistics so neighbouring stripes
     do not false-share
  4: make dss_info->start_time write-once (try_cmpxchg64), removing its
     dependence on any lock
  5: drop the redundant atomic from the busy timer
  6: pair the in-flight count with the layoutstats updates, under the
     same lock, with lockdep assertions
  7: derive ffil_ops_requested rather than storing it
  8: give each direction its own lock and cacheline, so an I/O dirties
     one line (the stripe shrinks from 384 to 320 bytes)

Patches 9-10 fix LAYOUTSTATS reporting for striped mirrors:
  9: drop NFS4_FF_MIRROR_STAT_AVAIL.  The per-mirror bit cost two atomic
     RMWs per RPC on the mirror's shared cacheline, and since the first
     stripe reported cleared it, a report carried only one
     ff_layoutupdate4 per mirror: always the lowest usable dss_id.
 10: rotate reporting across a mirror's stripes.  Callers cap a report
     at four entries; resume each mirror's scan where the previous
     report stopped, so every busy stripe is reported within
     ceil(busy_stripes / 4) report intervals instead of only the first
     four ever being reported.

Patch 11 takes the report decision (pointer chases into the mirror and
layout, ktime_to_ms(), the last_report_time cmpxchg) out of the critical
section, leaving the lock to cover two writes on one cacheline.

The wire encoding is unchanged; the values reported remain cumulative
since the layout was granted.

The series is based on Anna's linux-next (nfs-for-7.4-1).

Thanks,
Mike

Jonathan Flynn (2):
  NFSv4/flexfiles: move layoutstats accounting to a per-stripe lock
  NFSv4/flexfiles: drop the now-unused per-mirror lock

Mike Snitzer (9):
  NFSv4/flexfiles: cacheline align the per-stripe layoutstats
  NFSv4/flexfiles: make dss_info->start_time write-once
  NFSv4/flexfiles: drop the redundant atomic from the busy timer
  NFSv4/flexfiles: pair the in-flight count with the layoutstats updates
  NFSv4/flexfiles: derive ffil_ops_requested rather than storing it
  NFSv4/flexfiles: give each direction its own lock and cacheline
  NFSv4/flexfiles: drop NFS4_FF_MIRROR_STAT_AVAIL
  NFSv4/flexfiles: rotate LAYOUTSTATS reporting across a mirror's stripes
  NFSv4/flexfiles: take the report decision out of the critical section

 fs/nfs/flexfilelayout/flexfilelayout.c | 251 ++++++++++++++++---------
 fs/nfs/flexfilelayout/flexfilelayout.h |  74 ++++++--
 2 files changed, 218 insertions(+), 107 deletions(-)


base-commit: ef30b3efb7e8bae94d790a6503a595e1051323e7
-- 
2.52.0

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

[Mike Snitzer]

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
@@ -293,8 +293,10 @@ static struct nfs4_ff_layout_mirror *ff_layout_alloc_mirror(u32 dss_count,
 		return NULL;
 	}
 
-	for (u32 dss_id = 0; dss_id < mirror->dss_count; dss_id++)
+	for (u32 dss_id = 0; dss_id < mirror->dss_count; dss_id++) {
+		spin_lock_init(&mirror->dss[dss_id].lock);
 		nfs_localio_file_init(&mirror->dss[dss_id].nfl);
+	}
 
 	return mirror;
 }
@@ -736,6 +738,7 @@ nfs4_ff_layoutstat_start_io(struct nfs4_ff_layout_mirror *mirror,
 {
 	s64 report_interval = FF_LAYOUTSTATS_REPORT_INTERVAL;
 	struct nfs4_flexfile_layout *ffl = FF_LAYOUT_FROM_HDR(mirror->layout);
+	ktime_t last_report;
 
 	nfs4_ff_start_busy_timer(&layoutstat->busy_timer, now);
 	if (!mirror->dss[dss_id].start_time)
@@ -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);
 }
 
 static void
@@ -794,13 +801,13 @@ nfs4_ff_layout_stat_io_start_read(struct inode *inode,
 {
 	bool report;
 
-	spin_lock(&mirror->lock);
+	spin_lock(&mirror->dss[dss_id].lock);
 	report = nfs4_ff_layoutstat_start_io(
 		mirror, dss_id, &mirror->dss[dss_id].read_stat, now);
 	nfs4_ff_layout_stat_io_update_requested(
 		&mirror->dss[dss_id].read_stat, requested);
+	spin_unlock(&mirror->dss[dss_id].lock);
 	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
-	spin_unlock(&mirror->lock);
 
 	if (report)
 		pnfs_report_layoutstat(inode, nfs_io_gfp_mask());
@@ -813,12 +820,12 @@ nfs4_ff_layout_stat_io_end_read(struct rpc_task *task,
 		__u64 requested,
 		__u64 completed)
 {
-	spin_lock(&mirror->lock);
+	spin_lock(&mirror->dss[dss_id].lock);
 	nfs4_ff_layout_stat_io_update_completed(&mirror->dss[dss_id].read_stat,
 			requested, completed,
 			ktime_get(), task->tk_start);
+	spin_unlock(&mirror->dss[dss_id].lock);
 	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
-	spin_unlock(&mirror->lock);
 }
 
 static void
@@ -829,7 +836,7 @@ nfs4_ff_layout_stat_io_start_write(struct inode *inode,
 {
 	bool report;
 
-	spin_lock(&mirror->lock);
+	spin_lock(&mirror->dss[dss_id].lock);
 	report = nfs4_ff_layoutstat_start_io(
 		mirror,
 		dss_id,
@@ -838,8 +845,8 @@ nfs4_ff_layout_stat_io_start_write(struct inode *inode,
 	nfs4_ff_layout_stat_io_update_requested(
 		&mirror->dss[dss_id].write_stat,
 		requested);
+	spin_unlock(&mirror->dss[dss_id].lock);
 	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
-	spin_unlock(&mirror->lock);
 
 	if (report)
 		pnfs_report_layoutstat(inode, nfs_io_gfp_mask());
@@ -856,11 +863,11 @@ nfs4_ff_layout_stat_io_end_write(struct rpc_task *task,
 	if (committed == NFS_UNSTABLE)
 		requested = completed = 0;
 
-	spin_lock(&mirror->lock);
+	spin_lock(&mirror->dss[dss_id].lock);
 	nfs4_ff_layout_stat_io_update_completed(&mirror->dss[dss_id].write_stat,
 			requested, completed, ktime_get(), task->tk_start);
+	spin_unlock(&mirror->dss[dss_id].lock);
 	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
-	spin_unlock(&mirror->lock);
 }
 
 static struct nfs4_ff_layout_ds *
@@ -3071,13 +3078,13 @@ ff_layout_encode_ff_layoutupdate(struct xdr_stream *xdr,
 	p = xdr_reserve_space(xdr, 4 + fh->size);
 	xdr_encode_opaque(p, fh->data, fh->size);
 	/* ff_io_latency4 read */
-	spin_lock(&dss_info->mirror->lock);
+	spin_lock(&dss_info->lock);
 	ff_layout_encode_io_latency(xdr,
 				    &dss_info->read_stat.io_stat);
 	/* ff_io_latency4 write */
 	ff_layout_encode_io_latency(xdr,
 				    &dss_info->write_stat.io_stat);
-	spin_unlock(&dss_info->mirror->lock);
+	spin_unlock(&dss_info->lock);
 	/* nfstime4 */
 	ff_layout_encode_nfstime(xdr,
 				 ktime_sub(ktime_get(),
@@ -3153,7 +3160,7 @@ ff_layout_mirror_prepare_stats(struct pnfs_layout_hdr *lo,
 			       NFS4_DEVICEID4_SIZE);
 			devinfo->offset = 0;
 			devinfo->length = NFS4_MAX_UINT64;
-			spin_lock(&mirror->lock);
+			spin_lock(&dss_info->lock);
 			devinfo->read_count =
 			    dss_info->read_stat.io_stat.ops_completed;
 			devinfo->read_bytes =
@@ -3162,7 +3169,7 @@ ff_layout_mirror_prepare_stats(struct pnfs_layout_hdr *lo,
 			    dss_info->write_stat.io_stat.ops_completed;
 			devinfo->write_bytes =
 			    dss_info->write_stat.io_stat.bytes_completed;
-			spin_unlock(&mirror->lock);
+			spin_unlock(&dss_info->lock);
 			devinfo->layout_type = LAYOUT_FLEX_FILES;
 			devinfo->ld_private.ops = &layoutstat_ops;
 			priv->dss_info = dss_info;
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h
index 09c3cd6964fd6..54aa492bc0bac 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.h
+++ b/fs/nfs/flexfilelayout/flexfilelayout.h
@@ -86,6 +86,8 @@ struct nfs4_ff_layout_ds_stripe {
 	const struct cred __rcu		*ro_cred;
 	const struct cred __rcu		*rw_cred;
 	struct nfs_file_localio		nfl;
+	/* Protects read_stat, write_stat and start_time below */
+	spinlock_t			lock;
 	struct nfs4_ff_layoutstat	read_stat;
 	struct nfs4_ff_layoutstat	write_stat;
 	ktime_t				start_time;
-- 
2.52.0

[PATCH 02/11] NFSv4/flexfiles: drop the now-unused per-mirror lock

[Mike Snitzer]

From: Jonathan Flynn <jonathan.flynn@hammerspace.com>

With layoutstats accounting moved to the per-stripe lock, nothing takes
mirror->lock any more.  Remove it.

The only users of mirror->lock in this tree were the layoutstats paths
the previous patch converted.

Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
Signed-off-by: Mike Snitzer <snitzer@hammerspace.com>
---
 fs/nfs/flexfilelayout/flexfilelayout.c | 1 -
 fs/nfs/flexfilelayout/flexfilelayout.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index bac0b7f559201..7fc1369df8439 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -280,7 +280,6 @@ static struct nfs4_ff_layout_mirror *ff_layout_alloc_mirror(u32 dss_count,
 	if (mirror == NULL)
 		return NULL;
 
-	spin_lock_init(&mirror->lock);
 	refcount_set(&mirror->ref, 1);
 	INIT_LIST_HEAD(&mirror->mirrors);
 
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h
index 54aa492bc0bac..8fa2d8851166a 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.h
+++ b/fs/nfs/flexfilelayout/flexfilelayout.h
@@ -99,7 +99,6 @@ struct nfs4_ff_layout_mirror {
 	u32				dss_count;
 	struct nfs4_ff_layout_ds_stripe *dss;
 	refcount_t			ref;
-	spinlock_t			lock;
 	unsigned long			flags;
 	u32				report_interval;
 };
-- 
2.52.0

[PATCH 03/11] NFSv4/flexfiles: cacheline align the per-stripe layoutstats

[Mike Snitzer]

From: Mike Snitzer <snitzer@hammerspace.com>

Moving the layoutstats lock into struct nfs4_ff_layout_ds_stripe stopped
stripes serialising on one spinlock, but it left them sharing cachelines
with each other.

The lock and the statistics it guards sat at the tail of a struct whose
sizeof() was 296 and whose alignof() was only 8, so in mirror->dss[]
stripe i began at i * 296.  296 mod 64 = 40, so every stripe started 40
bytes further into a line than the last and no stripe's lock was line
aligned.  Acquiring stripe i's lock therefore invalidated the line
holding stripe i+1's lock and counters.  A RAID0-style layout -- one
mirror, many stripes -- is exactly the case the per-stripe lock was
introduced for, so this false sharing ate into the win.

Align the first member of the guarded set.  That does two things: the
lock now starts on a line boundary, so the guarded set occupies lines of
its own and shares none with the read-mostly members above; and it raises
the struct's alignment to 64, which forces sizeof() up to a multiple of
64 and makes the array stride line aligned.  The stride is what actually
separates stripe i from stripe i+1.

Verified with pahole on flexfilelayout.o built with DWARF5.  The layout
is identical on x86_64, arm64 4K pages and arm64 64K pages -- cachelines
are 64 bytes on both architectures, so page size does not enter into it
-- and was taken with neither CONFIG_DEBUG_SPINLOCK nor
CONFIG_PROVE_LOCKING, both of which fatten spinlock_t and change it:

  /* --- cacheline 3 boundary (192 bytes) --- */
  spinlock_t                 lock __aligned(64);  /*   192     4 */
  /* XXX 4 bytes hole */
  struct nfs4_ff_layoutstat  read_stat;           /*   200    72 */
  struct nfs4_ff_layoutstat  write_stat;          /*   272    72 */
  ktime_t                    start_time;          /*   344     8 */
  /* size: 384, cachelines: 6 */
  } __attribute__((__aligned__(64)));

A read RPC now dirties two lines of its own stripe and a write three, and
neither disturbs a neighbouring stripe.  Later patches give each
direction a line of its own.

Cost is 296 -> 384 bytes per stripe, i.e. 2816 bytes for a 32 stripe
layout, which is negligible beside the rest of the per-file layout state.
384 is also a multiple of 128, so the stride holds for implementations
with a 128 byte coherency granule.

Note that the alignment attribute only orders the stripes relative to the
start of mirror->dss[], not that array's own base.  The base is aligned
too, under either allocator kvmalloc() may pick for it:

  - From the slab.  kmalloc() documents a size based alignment guarantee
    for power-of-two sizes only, but the smallest possible request is now
    384 bytes, and every kmalloc bucket at or above that is a power of
    two whose objects SLUB lays out at multiples of the object size from
    a page aligned slab, so the base is at least 512 byte aligned.  The
    exceptions, kmalloc-96 and kmalloc-192, are both smaller than one
    stripe and cannot be selected.

  - From vmalloc.  A stripe array wide enough to need a high order
    allocation is served by the vmalloc() fallback instead, which returns
    page aligned memory.

Either is comfortably more than the 64 bytes this needs.

Signed-off-by: Mike Snitzer <snitzer@hammerspace.com>
---
 fs/nfs/flexfilelayout/flexfilelayout.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h
index 8fa2d8851166a..f48860450cf28 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.h
+++ b/fs/nfs/flexfilelayout/flexfilelayout.h
@@ -14,6 +14,7 @@
 #define FF_FLAGS_NO_IO_THRU_MDS  2
 #define FF_FLAGS_NO_READ_IO      4
 
+#include <linux/cache.h>
 #include <linux/refcount.h>
 #include "../pnfs.h"
 
@@ -86,8 +87,17 @@ struct nfs4_ff_layout_ds_stripe {
 	const struct cred __rcu		*ro_cred;
 	const struct cred __rcu		*rw_cred;
 	struct nfs_file_localio		nfl;
-	/* Protects read_stat, write_stat and start_time below */
-	spinlock_t			lock;
+	/*
+	 * Protects read_stat, write_stat and start_time below.
+	 *
+	 * Cacheline aligned so that the layoutstats accounting done on
+	 * every I/O to this stripe does not share cachelines with the
+	 * read-mostly members above, nor with the neighbouring stripes
+	 * in mirror->dss[]: aligning the first member of the guarded
+	 * set also rounds sizeof() up to a multiple of the cacheline,
+	 * which is what makes the array stride line-aligned.
+	 */
+	spinlock_t			lock ____cacheline_aligned_in_smp;
 	struct nfs4_ff_layoutstat	read_stat;
 	struct nfs4_ff_layoutstat	write_stat;
 	ktime_t				start_time;
-- 
2.52.0

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

[Mike Snitzer]

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.

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
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().

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);
+}
+
 static bool
 nfs4_ff_layoutstat_start_io(struct nfs4_ff_layout_mirror *mirror,
 			    u32 dss_id,
@@ -740,8 +756,7 @@ nfs4_ff_layoutstat_start_io(struct nfs4_ff_layout_mirror *mirror,
 	ktime_t last_report;
 
 	nfs4_ff_start_busy_timer(&layoutstat->busy_timer, now);
-	if (!mirror->dss[dss_id].start_time)
-		mirror->dss[dss_id].start_time = now;
+	nfs4_ff_layoutstat_set_start_time(&mirror->dss[dss_id], now);
 	if (mirror->report_interval != 0)
 		report_interval = (s64)mirror->report_interval * 1000LL;
 	else if (layoutstats_timer != 0)
@@ -3087,7 +3102,7 @@ ff_layout_encode_ff_layoutupdate(struct xdr_stream *xdr,
 	/* nfstime4 */
 	ff_layout_encode_nfstime(xdr,
 				 ktime_sub(ktime_get(),
-					   dss_info->start_time));
+					   READ_ONCE(dss_info->start_time)));
 	/* bool */
 	p = xdr_reserve_space(xdr, 4);
 	*p = cpu_to_be32(false);
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h
index f48860450cf28..658940979883a 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.h
+++ b/fs/nfs/flexfilelayout/flexfilelayout.h
@@ -88,7 +88,7 @@ struct nfs4_ff_layout_ds_stripe {
 	const struct cred __rcu		*rw_cred;
 	struct nfs_file_localio		nfl;
 	/*
-	 * Protects read_stat, write_stat and start_time below.
+	 * Protects read_stat and write_stat below.
 	 *
 	 * Cacheline aligned so that the layoutstats accounting done on
 	 * every I/O to this stripe does not share cachelines with the
@@ -100,6 +100,7 @@ struct nfs4_ff_layout_ds_stripe {
 	spinlock_t			lock ____cacheline_aligned_in_smp;
 	struct nfs4_ff_layoutstat	read_stat;
 	struct nfs4_ff_layoutstat	write_stat;
+	/* Published once by the first I/O; see nfs4_ff_layoutstat_set_start_time() */
 	ktime_t				start_time;
 };
 
-- 
2.52.0

[PATCH 05/11] NFSv4/flexfiles: drop the redundant atomic from the busy timer

[Mike Snitzer]

From: Mike Snitzer <snitzer@hammerspace.com>

nfs4_ff_busy_timer::n_ops is an atomic_t, but every access to it is
already serialised by the stripe lock:

  nfs4_ff_start_busy_timer() has one caller, nfs4_ff_layoutstat_start_io(),
  reached only from ff_layout_{read,write}_record_layoutstats_start()
  with dss[dss_id].lock held.

  nfs4_ff_end_busy_timer() has one caller,
  nfs4_ff_layout_stat_io_update_completed(), reached only from
  ff_layout_{read,write}_record_layoutstats_done() with the same lock
  held.

The two reporting paths, ff_layout_mirror_prepare_stats() and
ff_layout_encode_ff_layoutupdate(), take the stripe lock too and do not
touch n_ops at all.  There is no lockless reader.

So the atomic_inc_return()/atomic_dec_return() are locked read-modify-
writes on a line the CPU already owns exclusively -- pure serialisation
overhead, paid twice per DS RPC on the hottest path in this file, at the
275k-450k RPC/s the per-stripe lock work was measured at.

Make n_ops a plain int and document the locking on the struct.  Its size
is unchanged, so the stripe layout is unaffected.  The underflow check is
preserved.

Signed-off-by: Mike Snitzer <snitzer@hammerspace.com>
---
 fs/nfs/flexfilelayout/flexfilelayout.c | 6 ++----
 fs/nfs/flexfilelayout/flexfilelayout.h | 6 +++++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index 7c328f2ec900d..6a474fa7b0446 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -711,9 +711,8 @@ static void
 nfs4_ff_start_busy_timer(struct nfs4_ff_busy_timer *timer, ktime_t now)
 {
 	/* first IO request? */
-	if (atomic_inc_return(&timer->n_ops) == 1) {
+	if (++timer->n_ops == 1)
 		timer->start_time = now;
-	}
 }
 
 static ktime_t
@@ -721,8 +720,7 @@ nfs4_ff_end_busy_timer(struct nfs4_ff_busy_timer *timer, ktime_t now)
 {
 	ktime_t start;
 
-	if (atomic_dec_return(&timer->n_ops) < 0)
-		WARN_ON_ONCE(1);
+	WARN_ON_ONCE(--timer->n_ops < 0);
 
 	start = timer->start_time;
 	timer->start_time = now;
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h
index 658940979883a..d97001abc0d22 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.h
+++ b/fs/nfs/flexfilelayout/flexfilelayout.h
@@ -64,9 +64,13 @@ struct nfs4_ff_io_stat {
 	ktime_t				aggregate_completion_time;
 };
 
+/*
+ * Both members are serialised by the enclosing stripe's
+ * nfs4_ff_layout_ds_stripe::lock; see nfs4_ff_{start,end}_busy_timer().
+ */
 struct nfs4_ff_busy_timer {
 	ktime_t start_time;
-	atomic_t n_ops;
+	int n_ops;
 };
 
 struct nfs4_ff_layoutstat {
-- 
2.52.0

[PATCH 06/11] NFSv4/flexfiles: pair the in-flight count with the layoutstats updates

[Mike Snitzer]

From: Mike Snitzer <snitzer@hammerspace.com>

The busy timer's in-flight op count and the requested-side statistics are
two halves of the same accounting, but they lived in two helpers that the
callers invoked back to back.  Nothing stopped a new requested-side path
from updating one and missing the other.

Fold nfs4_ff_layout_stat_io_update_requested() into
nfs4_ff_layoutstat_start_io() so both happen in one place, and rename
nfs4_ff_layout_stat_io_update_completed() to nfs4_ff_layoutstat_end_io()
for symmetry -- it already dropped the in-flight count alongside the
completed-side statistics.

Rename nfs4_ff_busy_timer::n_ops to ops_in_flight while here.  It is what
the field holds, and it makes its relationship to the op counters legible
at the point of use.

Both helpers now take the stripe rather than mirror + dss_id, which lets
them assert the lock they run under.  Since the previous patch made
ops_in_flight a plain int, that lock is the only thing keeping it
consistent, so assert it rather than document it; lockdep_assert_held()
compiles out without CONFIG_PROVE_LOCKING.  The callers get a dss_info
local, which is what they were open coding.

No functional change.

Signed-off-by: Mike Snitzer <snitzer@hammerspace.com>
---
 fs/nfs/flexfilelayout/flexfilelayout.c | 104 ++++++++++++++-----------
 fs/nfs/flexfilelayout/flexfilelayout.h |   2 +-
 2 files changed, 58 insertions(+), 48 deletions(-)

diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index 6a474fa7b0446..bfe8288a14a08 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -711,7 +711,7 @@ static void
 nfs4_ff_start_busy_timer(struct nfs4_ff_busy_timer *timer, ktime_t now)
 {
 	/* first IO request? */
-	if (++timer->n_ops == 1)
+	if (++timer->ops_in_flight == 1)
 		timer->start_time = now;
 }
 
@@ -720,7 +720,7 @@ nfs4_ff_end_busy_timer(struct nfs4_ff_busy_timer *timer, ktime_t now)
 {
 	ktime_t start;
 
-	WARN_ON_ONCE(--timer->n_ops < 0);
+	WARN_ON_ONCE(--timer->ops_in_flight < 0);
 
 	start = timer->start_time;
 	timer->start_time = now;
@@ -743,18 +743,31 @@ nfs4_ff_layoutstat_set_start_time(struct nfs4_ff_layout_ds_stripe *dss_info,
 		try_cmpxchg64(&dss_info->start_time, &unset, now);
 }
 
+/*
+ * Account for an I/O about to be sent to this stripe, and return true if a
+ * LAYOUTSTATS report is due.
+ *
+ * The in-flight count and the requested-side counters are bumped here
+ * together, so a new requested-side path cannot update one and miss the
+ * other.
+ */
 static bool
-nfs4_ff_layoutstat_start_io(struct nfs4_ff_layout_mirror *mirror,
-			    u32 dss_id,
+nfs4_ff_layoutstat_start_io(struct nfs4_ff_layout_ds_stripe *dss_info,
 			    struct nfs4_ff_layoutstat *layoutstat,
-			    ktime_t now)
+			    __u64 requested, ktime_t now)
 {
-	s64 report_interval = FF_LAYOUTSTATS_REPORT_INTERVAL;
+	struct nfs4_ff_layout_mirror *mirror = dss_info->mirror;
 	struct nfs4_flexfile_layout *ffl = FF_LAYOUT_FROM_HDR(mirror->layout);
+	s64 report_interval = FF_LAYOUTSTATS_REPORT_INTERVAL;
 	ktime_t last_report;
 
+	lockdep_assert_held(&dss_info->lock);
+
 	nfs4_ff_start_busy_timer(&layoutstat->busy_timer, now);
-	nfs4_ff_layoutstat_set_start_time(&mirror->dss[dss_id], now);
+	layoutstat->io_stat.ops_requested++;
+	layoutstat->io_stat.bytes_requested += requested;
+
+	nfs4_ff_layoutstat_set_start_time(dss_info, now);
 	if (mirror->report_interval != 0)
 		report_interval = (s64)mirror->report_interval * 1000LL;
 	else if (layoutstats_timer != 0)
@@ -772,27 +785,25 @@ nfs4_ff_layoutstat_start_io(struct nfs4_ff_layout_mirror *mirror,
 	return try_cmpxchg64(&ffl->last_report_time, &last_report, now);
 }
 
+/*
+ * Account for an I/O that has completed on this stripe.  The in-flight count
+ * is dropped here, alongside the completed-side counters, for the same reason
+ * it is bumped in nfs4_ff_layoutstat_start_io().
+ */
 static void
-nfs4_ff_layout_stat_io_update_requested(struct nfs4_ff_layoutstat *layoutstat,
-		__u64 requested)
-{
-	struct nfs4_ff_io_stat *iostat = &layoutstat->io_stat;
-
-	iostat->ops_requested++;
-	iostat->bytes_requested += requested;
-}
-
-static void
-nfs4_ff_layout_stat_io_update_completed(struct nfs4_ff_layoutstat *layoutstat,
-		__u64 requested,
-		__u64 completed,
-		ktime_t time_completed,
-		ktime_t time_started)
+nfs4_ff_layoutstat_end_io(struct nfs4_ff_layout_ds_stripe *dss_info,
+			  struct nfs4_ff_layoutstat *layoutstat,
+			  __u64 requested,
+			  __u64 completed,
+			  ktime_t time_completed,
+			  ktime_t time_started)
 {
 	struct nfs4_ff_io_stat *iostat = &layoutstat->io_stat;
 	ktime_t completion_time = ktime_sub(time_completed, time_started);
 	ktime_t timer;
 
+	lockdep_assert_held(&dss_info->lock);
+
 	iostat->ops_completed++;
 	iostat->bytes_completed += completed;
 	iostat->bytes_not_delivered += requested - completed;
@@ -811,14 +822,13 @@ nfs4_ff_layout_stat_io_start_read(struct inode *inode,
 		u32 dss_id,
 		__u64 requested, ktime_t now)
 {
+	struct nfs4_ff_layout_ds_stripe *dss_info = &mirror->dss[dss_id];
 	bool report;
 
-	spin_lock(&mirror->dss[dss_id].lock);
-	report = nfs4_ff_layoutstat_start_io(
-		mirror, dss_id, &mirror->dss[dss_id].read_stat, now);
-	nfs4_ff_layout_stat_io_update_requested(
-		&mirror->dss[dss_id].read_stat, requested);
-	spin_unlock(&mirror->dss[dss_id].lock);
+	spin_lock(&dss_info->lock);
+	report = nfs4_ff_layoutstat_start_io(dss_info, &dss_info->read_stat,
+					     requested, now);
+	spin_unlock(&dss_info->lock);
 	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
 
 	if (report)
@@ -832,11 +842,13 @@ nfs4_ff_layout_stat_io_end_read(struct rpc_task *task,
 		__u64 requested,
 		__u64 completed)
 {
-	spin_lock(&mirror->dss[dss_id].lock);
-	nfs4_ff_layout_stat_io_update_completed(&mirror->dss[dss_id].read_stat,
-			requested, completed,
-			ktime_get(), task->tk_start);
-	spin_unlock(&mirror->dss[dss_id].lock);
+	struct nfs4_ff_layout_ds_stripe *dss_info = &mirror->dss[dss_id];
+
+	spin_lock(&dss_info->lock);
+	nfs4_ff_layoutstat_end_io(dss_info, &dss_info->read_stat,
+				  requested, completed,
+				  ktime_get(), task->tk_start);
+	spin_unlock(&dss_info->lock);
 	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
 }
 
@@ -846,18 +858,13 @@ nfs4_ff_layout_stat_io_start_write(struct inode *inode,
 		u32 dss_id,
 		__u64 requested, ktime_t now)
 {
+	struct nfs4_ff_layout_ds_stripe *dss_info = &mirror->dss[dss_id];
 	bool report;
 
-	spin_lock(&mirror->dss[dss_id].lock);
-	report = nfs4_ff_layoutstat_start_io(
-		mirror,
-		dss_id,
-		&mirror->dss[dss_id].write_stat,
-		now);
-	nfs4_ff_layout_stat_io_update_requested(
-		&mirror->dss[dss_id].write_stat,
-		requested);
-	spin_unlock(&mirror->dss[dss_id].lock);
+	spin_lock(&dss_info->lock);
+	report = nfs4_ff_layoutstat_start_io(dss_info, &dss_info->write_stat,
+					     requested, now);
+	spin_unlock(&dss_info->lock);
 	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
 
 	if (report)
@@ -872,13 +879,16 @@ nfs4_ff_layout_stat_io_end_write(struct rpc_task *task,
 		__u64 completed,
 		enum nfs3_stable_how committed)
 {
+	struct nfs4_ff_layout_ds_stripe *dss_info = &mirror->dss[dss_id];
+
 	if (committed == NFS_UNSTABLE)
 		requested = completed = 0;
 
-	spin_lock(&mirror->dss[dss_id].lock);
-	nfs4_ff_layout_stat_io_update_completed(&mirror->dss[dss_id].write_stat,
-			requested, completed, ktime_get(), task->tk_start);
-	spin_unlock(&mirror->dss[dss_id].lock);
+	spin_lock(&dss_info->lock);
+	nfs4_ff_layoutstat_end_io(dss_info, &dss_info->write_stat,
+				  requested, completed,
+				  ktime_get(), task->tk_start);
+	spin_unlock(&dss_info->lock);
 	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
 }
 
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h
index d97001abc0d22..b0653a652d3b8 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.h
+++ b/fs/nfs/flexfilelayout/flexfilelayout.h
@@ -70,7 +70,7 @@ struct nfs4_ff_io_stat {
  */
 struct nfs4_ff_busy_timer {
 	ktime_t start_time;
-	int n_ops;
+	int ops_in_flight;
 };
 
 struct nfs4_ff_layoutstat {
-- 
2.52.0

[PATCH 07/11] NFSv4/flexfiles: derive ffil_ops_requested rather than storing it

[Mike Snitzer]

From: Mike Snitzer <snitzer@hammerspace.com>

nfs4_ff_io_stat::ops_requested carries no information that the stripe
does not already hold.  An op is in flight from the moment it is counted
as requested until it is counted as completed, and the previous patch
put both halves of that accounting in one place each, so

	ops_requested == ops_completed + ops_in_flight

holds at all times under the lock.

Drop the field and compute the value at encode time.  It had exactly one
producer and one consumer, the LAYOUTSTATS encoder, so nothing else
needs to change and the wire format is untouched -- ffil_ops_requested
is still a uint64, and nfs4_ff_ops_requested() returns __u64.
ff_layout_encode_io_latency() now takes the whole nfs4_ff_layoutstat,
since the in-flight count lives beside the statistics rather than in
them.

This takes 8 bytes off each direction's statistics, which is what lets
each direction fit in a cacheline of its own; see the next patch.

Signed-off-by: Mike Snitzer <snitzer@hammerspace.com>
---
 fs/nfs/flexfilelayout/flexfilelayout.c | 12 +++++-------
 fs/nfs/flexfilelayout/flexfilelayout.h | 19 ++++++++++++++++++-
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index bfe8288a14a08..846544977cf30 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -764,7 +764,6 @@ nfs4_ff_layoutstat_start_io(struct nfs4_ff_layout_ds_stripe *dss_info,
 	lockdep_assert_held(&dss_info->lock);
 
 	nfs4_ff_start_busy_timer(&layoutstat->busy_timer, now);
-	layoutstat->io_stat.ops_requested++;
 	layoutstat->io_stat.bytes_requested += requested;
 
 	nfs4_ff_layoutstat_set_start_time(dss_info, now);
@@ -3066,12 +3065,13 @@ ff_layout_encode_nfstime(struct xdr_stream *xdr,
 
 static void
 ff_layout_encode_io_latency(struct xdr_stream *xdr,
-			    struct nfs4_ff_io_stat *stat)
+			    const struct nfs4_ff_layoutstat *layoutstat)
 {
+	const struct nfs4_ff_io_stat *stat = &layoutstat->io_stat;
 	__be32 *p;
 
 	p = xdr_reserve_space(xdr, 5 * 8);
-	p = xdr_encode_hyper(p, stat->ops_requested);
+	p = xdr_encode_hyper(p, nfs4_ff_ops_requested(layoutstat));
 	p = xdr_encode_hyper(p, stat->bytes_requested);
 	p = xdr_encode_hyper(p, stat->ops_completed);
 	p = xdr_encode_hyper(p, stat->bytes_completed);
@@ -3101,11 +3101,9 @@ ff_layout_encode_ff_layoutupdate(struct xdr_stream *xdr,
 	xdr_encode_opaque(p, fh->data, fh->size);
 	/* ff_io_latency4 read */
 	spin_lock(&dss_info->lock);
-	ff_layout_encode_io_latency(xdr,
-				    &dss_info->read_stat.io_stat);
+	ff_layout_encode_io_latency(xdr, &dss_info->read_stat);
 	/* ff_io_latency4 write */
-	ff_layout_encode_io_latency(xdr,
-				    &dss_info->write_stat.io_stat);
+	ff_layout_encode_io_latency(xdr, &dss_info->write_stat);
 	spin_unlock(&dss_info->lock);
 	/* nfstime4 */
 	ff_layout_encode_nfstime(xdr,
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h
index b0653a652d3b8..23f665fef4985 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.h
+++ b/fs/nfs/flexfilelayout/flexfilelayout.h
@@ -55,7 +55,6 @@ struct nfs4_ff_layout_ds_err {
 };
 
 struct nfs4_ff_io_stat {
-	__u64				ops_requested;
 	__u64				bytes_requested;
 	__u64				ops_completed;
 	__u64				bytes_completed;
@@ -78,6 +77,24 @@ struct nfs4_ff_layoutstat {
 	struct nfs4_ff_busy_timer busy_timer;
 };
 
+/*
+ * ffil_ops_requested is not stored.  An op is in flight from the moment it is
+ * counted as requested until it is counted as completed, so
+ *
+ *	ops_requested == ops_completed + ops_in_flight
+ *
+ * holds by construction; see nfs4_ff_layoutstat_start_io() and
+ * nfs4_ff_layoutstat_end_io(), which maintain both halves together.
+ *
+ * Caller must hold the lock guarding this nfs4_ff_layoutstat.
+ */
+static inline __u64
+nfs4_ff_ops_requested(const struct nfs4_ff_layoutstat *layoutstat)
+{
+	return layoutstat->io_stat.ops_completed +
+	       layoutstat->busy_timer.ops_in_flight;
+}
+
 struct nfs4_ff_layout_mirror;
 
 struct nfs4_ff_layout_ds_stripe {
-- 
2.52.0

[PATCH 08/11] NFSv4/flexfiles: give each direction its own lock and cacheline

[Mike Snitzer]

From: Mike Snitzer <snitzer@hammerspace.com>

struct nfs4_ff_layout_ds_stripe shrinks from 384 bytes to 320, and an
I/O to a stripe now dirties one cacheline instead of two.  The 384 was
what "NFSv4/flexfiles: cacheline align the per-stripe layoutstats" had to
pay to get the array stride line aligned; this gets that back and takes
another 64 bytes off, on top of removing the last shared lock between the
two I/O directions.

Cacheline aligning the stripe's lock stopped stripes sharing lines with
each other, but within a stripe an I/O still dirtied more than one line,
and reads and writes still serialised on one spinlock.

The set one RPC touches is not the whole guarded region.  A read takes
the lock, bumps the in-flight count and updates every field of read_stat
-- bytes_requested and the busy timer on the way out, ops_completed,
bytes_completed, bytes_not_delivered and the two ktime_t sums on
completion -- while write_stat stays untouched, and a write is the mirror
image.  The two directions share nothing but the lock.

So stop sharing the lock.  Move it into struct nfs4_ff_layoutstat,
alongside the in-flight count and the statistics it guards, and fold
nfs4_ff_busy_timer in as a bare ktime_t.  With ops_requested derived
rather than stored, that comes to

	spinlock_t             lock              4
	int                    ops_in_flight     4
	struct nfs4_ff_io_stat io_stat          48
	ktime_t                busy_start_time   8

which is 64 bytes with no padding.  Align the structure to a cacheline
and each direction occupies one, exactly.

An I/O now dirties a single line, and a read and a write to the same
stripe touch neither the same lock nor the same line.  The two reporting
paths, ff_layout_mirror_prepare_stats() and
ff_layout_encode_ff_layoutupdate(), take each lock in turn; they need no
atomicity across the pair, as the statistics are advisory and were never
sampled atomically with respect to in-flight I/O anyway.

pahole on flexfilelayout.o built with DWARF5, identical on x86_64, arm64
4K pages and arm64 64K pages:

  struct nfs4_ff_layoutstat {
          spinlock_t              lock;             /*    0     4 */
          int                     ops_in_flight;    /*    4     4 */
          struct nfs4_ff_io_stat  io_stat;          /*    8    48 */
          ktime_t                 busy_start_time;  /*   56     8 */
          /* size: 64, cachelines: 1, members: 4 */
  } __attribute__((__aligned__(64)));

  ktime_t                    start_time;  /*   136     8 */
  /* XXX 48 bytes hole */
  /* --- cacheline 3 boundary (192 bytes) --- */
  struct nfs4_ff_layoutstat  read_stat;   /*   192    64 */
  /* --- cacheline 4 boundary (256 bytes) --- */
  struct nfs4_ff_layoutstat  write_stat;  /*   256    64 */
  /* size: 320, cachelines: 5 */

start_time stays out of both lines, since it neither fits nor needs to.
"NFSv4/flexfiles: make dss_info->start_time write-once" removed its
dependence on any lock, and nothing writes it after the stripe's first
I/O, so its line never bounces; the same I/O already reads fh_versions,
stateid and the creds from the lines around it.  In the hole ahead of
read_stat it is free.

The array stride stays a multiple of 64 and of 128, so stripes remain
separated on implementations with a 128 byte coherency granule.

Note that CONFIG_DEBUG_SPINLOCK or CONFIG_PROVE_LOCKING fatten spinlock_t
enough to push nfs4_ff_layoutstat to 128 bytes and the stripe to 448.
Correctness is unaffected; such kernels simply do not get the packing.

Signed-off-by: Mike Snitzer <snitzer@hammerspace.com>
---
 fs/nfs/flexfilelayout/flexfilelayout.c | 53 ++++++++++++++------------
 fs/nfs/flexfilelayout/flexfilelayout.h | 46 +++++++++++-----------
 2 files changed, 52 insertions(+), 47 deletions(-)

diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index 846544977cf30..0b8562efabb24 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -293,7 +293,8 @@ static struct nfs4_ff_layout_mirror *ff_layout_alloc_mirror(u32 dss_count,
 	}
 
 	for (u32 dss_id = 0; dss_id < mirror->dss_count; dss_id++) {
-		spin_lock_init(&mirror->dss[dss_id].lock);
+		spin_lock_init(&mirror->dss[dss_id].read_stat.lock);
+		spin_lock_init(&mirror->dss[dss_id].write_stat.lock);
 		nfs_localio_file_init(&mirror->dss[dss_id].nfl);
 	}
 
@@ -708,22 +709,22 @@ static u32 calc_dss_id_from_commit(struct pnfs_layout_segment *lseg,
 }
 
 static void
-nfs4_ff_start_busy_timer(struct nfs4_ff_busy_timer *timer, ktime_t now)
+nfs4_ff_start_busy_timer(struct nfs4_ff_layoutstat *layoutstat, ktime_t now)
 {
 	/* first IO request? */
-	if (++timer->ops_in_flight == 1)
-		timer->start_time = now;
+	if (++layoutstat->ops_in_flight == 1)
+		layoutstat->busy_start_time = now;
 }
 
 static ktime_t
-nfs4_ff_end_busy_timer(struct nfs4_ff_busy_timer *timer, ktime_t now)
+nfs4_ff_end_busy_timer(struct nfs4_ff_layoutstat *layoutstat, ktime_t now)
 {
 	ktime_t start;
 
-	WARN_ON_ONCE(--timer->ops_in_flight < 0);
+	WARN_ON_ONCE(--layoutstat->ops_in_flight < 0);
 
-	start = timer->start_time;
-	timer->start_time = now;
+	start = layoutstat->busy_start_time;
+	layoutstat->busy_start_time = now;
 	return ktime_sub(now, start);
 }
 
@@ -761,9 +762,9 @@ nfs4_ff_layoutstat_start_io(struct nfs4_ff_layout_ds_stripe *dss_info,
 	s64 report_interval = FF_LAYOUTSTATS_REPORT_INTERVAL;
 	ktime_t last_report;
 
-	lockdep_assert_held(&dss_info->lock);
+	lockdep_assert_held(&layoutstat->lock);
 
-	nfs4_ff_start_busy_timer(&layoutstat->busy_timer, now);
+	nfs4_ff_start_busy_timer(layoutstat, now);
 	layoutstat->io_stat.bytes_requested += requested;
 
 	nfs4_ff_layoutstat_set_start_time(dss_info, now);
@@ -801,13 +802,13 @@ nfs4_ff_layoutstat_end_io(struct nfs4_ff_layout_ds_stripe *dss_info,
 	ktime_t completion_time = ktime_sub(time_completed, time_started);
 	ktime_t timer;
 
-	lockdep_assert_held(&dss_info->lock);
+	lockdep_assert_held(&layoutstat->lock);
 
 	iostat->ops_completed++;
 	iostat->bytes_completed += completed;
 	iostat->bytes_not_delivered += requested - completed;
 
-	timer = nfs4_ff_end_busy_timer(&layoutstat->busy_timer, time_completed);
+	timer = nfs4_ff_end_busy_timer(layoutstat, time_completed);
 	iostat->total_busy_time =
 			ktime_add(iostat->total_busy_time, timer);
 	iostat->aggregate_completion_time =
@@ -824,10 +825,10 @@ nfs4_ff_layout_stat_io_start_read(struct inode *inode,
 	struct nfs4_ff_layout_ds_stripe *dss_info = &mirror->dss[dss_id];
 	bool report;
 
-	spin_lock(&dss_info->lock);
+	spin_lock(&dss_info->read_stat.lock);
 	report = nfs4_ff_layoutstat_start_io(dss_info, &dss_info->read_stat,
 					     requested, now);
-	spin_unlock(&dss_info->lock);
+	spin_unlock(&dss_info->read_stat.lock);
 	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
 
 	if (report)
@@ -843,11 +844,11 @@ nfs4_ff_layout_stat_io_end_read(struct rpc_task *task,
 {
 	struct nfs4_ff_layout_ds_stripe *dss_info = &mirror->dss[dss_id];
 
-	spin_lock(&dss_info->lock);
+	spin_lock(&dss_info->read_stat.lock);
 	nfs4_ff_layoutstat_end_io(dss_info, &dss_info->read_stat,
 				  requested, completed,
 				  ktime_get(), task->tk_start);
-	spin_unlock(&dss_info->lock);
+	spin_unlock(&dss_info->read_stat.lock);
 	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
 }
 
@@ -860,10 +861,10 @@ nfs4_ff_layout_stat_io_start_write(struct inode *inode,
 	struct nfs4_ff_layout_ds_stripe *dss_info = &mirror->dss[dss_id];
 	bool report;
 
-	spin_lock(&dss_info->lock);
+	spin_lock(&dss_info->write_stat.lock);
 	report = nfs4_ff_layoutstat_start_io(dss_info, &dss_info->write_stat,
 					     requested, now);
-	spin_unlock(&dss_info->lock);
+	spin_unlock(&dss_info->write_stat.lock);
 	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
 
 	if (report)
@@ -883,11 +884,11 @@ nfs4_ff_layout_stat_io_end_write(struct rpc_task *task,
 	if (committed == NFS_UNSTABLE)
 		requested = completed = 0;
 
-	spin_lock(&dss_info->lock);
+	spin_lock(&dss_info->write_stat.lock);
 	nfs4_ff_layoutstat_end_io(dss_info, &dss_info->write_stat,
 				  requested, completed,
 				  ktime_get(), task->tk_start);
-	spin_unlock(&dss_info->lock);
+	spin_unlock(&dss_info->write_stat.lock);
 	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
 }
 
@@ -3100,11 +3101,13 @@ ff_layout_encode_ff_layoutupdate(struct xdr_stream *xdr,
 	p = xdr_reserve_space(xdr, 4 + fh->size);
 	xdr_encode_opaque(p, fh->data, fh->size);
 	/* ff_io_latency4 read */
-	spin_lock(&dss_info->lock);
+	spin_lock(&dss_info->read_stat.lock);
 	ff_layout_encode_io_latency(xdr, &dss_info->read_stat);
+	spin_unlock(&dss_info->read_stat.lock);
 	/* ff_io_latency4 write */
+	spin_lock(&dss_info->write_stat.lock);
 	ff_layout_encode_io_latency(xdr, &dss_info->write_stat);
-	spin_unlock(&dss_info->lock);
+	spin_unlock(&dss_info->write_stat.lock);
 	/* nfstime4 */
 	ff_layout_encode_nfstime(xdr,
 				 ktime_sub(ktime_get(),
@@ -3180,16 +3183,18 @@ ff_layout_mirror_prepare_stats(struct pnfs_layout_hdr *lo,
 			       NFS4_DEVICEID4_SIZE);
 			devinfo->offset = 0;
 			devinfo->length = NFS4_MAX_UINT64;
-			spin_lock(&dss_info->lock);
+			spin_lock(&dss_info->read_stat.lock);
 			devinfo->read_count =
 			    dss_info->read_stat.io_stat.ops_completed;
 			devinfo->read_bytes =
 			    dss_info->read_stat.io_stat.bytes_completed;
+			spin_unlock(&dss_info->read_stat.lock);
+			spin_lock(&dss_info->write_stat.lock);
 			devinfo->write_count =
 			    dss_info->write_stat.io_stat.ops_completed;
 			devinfo->write_bytes =
 			    dss_info->write_stat.io_stat.bytes_completed;
-			spin_unlock(&dss_info->lock);
+			spin_unlock(&dss_info->write_stat.lock);
 			devinfo->layout_type = LAYOUT_FLEX_FILES;
 			devinfo->ld_private.ops = &layoutstat_ops;
 			priv->dss_info = dss_info;
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h
index 23f665fef4985..876fdb6999473 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.h
+++ b/fs/nfs/flexfilelayout/flexfilelayout.h
@@ -64,18 +64,22 @@ struct nfs4_ff_io_stat {
 };
 
 /*
- * Both members are serialised by the enclosing stripe's
- * nfs4_ff_layout_ds_stripe::lock; see nfs4_ff_{start,end}_busy_timer().
+ * One direction's statistics for one stripe, with the lock that guards them.
+ *
+ * Exactly one cacheline, and aligned to one, so that an I/O dirties a single
+ * line: the lock, the in-flight count and every counter it touches are all in
+ * here, and the other direction's copy is in a line of its own.  Adding to
+ * this structure, or letting CONFIG_DEBUG_SPINLOCK or CONFIG_PROVE_LOCKING
+ * fatten spinlock_t, spills it into a second line.
  */
-struct nfs4_ff_busy_timer {
-	ktime_t start_time;
-	int ops_in_flight;
-};
-
 struct nfs4_ff_layoutstat {
-	struct nfs4_ff_io_stat io_stat;
-	struct nfs4_ff_busy_timer busy_timer;
-};
+	/* Protects every other member of this structure */
+	spinlock_t			lock;
+	int				ops_in_flight;
+	struct nfs4_ff_io_stat		io_stat;
+	/* Start of the interval io_stat.total_busy_time accrues */
+	ktime_t				busy_start_time;
+} ____cacheline_aligned_in_smp;
 
 /*
  * ffil_ops_requested is not stored.  An op is in flight from the moment it is
@@ -91,8 +95,7 @@ struct nfs4_ff_layoutstat {
 static inline __u64
 nfs4_ff_ops_requested(const struct nfs4_ff_layoutstat *layoutstat)
 {
-	return layoutstat->io_stat.ops_completed +
-	       layoutstat->busy_timer.ops_in_flight;
+	return layoutstat->io_stat.ops_completed + layoutstat->ops_in_flight;
 }
 
 struct nfs4_ff_layout_mirror;
@@ -108,21 +111,18 @@ struct nfs4_ff_layout_ds_stripe {
 	const struct cred __rcu		*ro_cred;
 	const struct cred __rcu		*rw_cred;
 	struct nfs_file_localio		nfl;
+	/* Published once by the first I/O; see nfs4_ff_layoutstat_set_start_time() */
+	ktime_t				start_time;
 	/*
-	 * Protects read_stat and write_stat below.
-	 *
-	 * Cacheline aligned so that the layoutstats accounting done on
-	 * every I/O to this stripe does not share cachelines with the
-	 * read-mostly members above, nor with the neighbouring stripes
-	 * in mirror->dss[]: aligning the first member of the guarded
-	 * set also rounds sizeof() up to a multiple of the cacheline,
-	 * which is what makes the array stride line-aligned.
+	 * A line each, and each carrying its own lock, so that a read and a
+	 * write to this stripe neither serialise against each other nor
+	 * share a line.  Their alignment also rounds this structure's
+	 * sizeof() up to a multiple of the cacheline, which is what keeps
+	 * the mirror->dss[] stride line-aligned and so stops stripe i
+	 * sharing a line with stripe i+1.
 	 */
-	spinlock_t			lock ____cacheline_aligned_in_smp;
 	struct nfs4_ff_layoutstat	read_stat;
 	struct nfs4_ff_layoutstat	write_stat;
-	/* Published once by the first I/O; see nfs4_ff_layoutstat_set_start_time() */
-	ktime_t				start_time;
 };
 
 struct nfs4_ff_layout_mirror {
-- 
2.52.0

[PATCH 09/11] NFSv4/flexfiles: drop NFS4_FF_MIRROR_STAT_AVAIL

[Mike Snitzer]

From: Mike Snitzer <snitzer@hammerspace.com>

The "this needs reporting" hint was a bit in nfs4_ff_layout_mirror::flags,
set on every DS RPC and consumed once per report interval.  It was wrong
in both directions.

It cost too much.  set_bit() ran twice per RPC -- once from
nfs4_ff_layout_stat_io_start_{read,write}() and again from the matching
_end_ -- on a mirror-wide cacheline that every stripe of the mirror
shares.  That is the same false sharing the per-stripe lock was
introduced to remove, and after the preceding patches gave each direction
a line of its own it was the last write to the mirror's line left on the
hot path.  Two unconditional atomic read-modify-writes per RPC, at the
275k-450k RPC/s this series was measured at, to set a hint read once
every 60 seconds.

And it said too little.  The bit is per mirror, but the statistics it
gates are per stripe, and ff_layout_mirror_prepare_stats() tests it
inside the per-stripe loop.  The first stripe with a usable deviceid
cleared it, so every other stripe of that mirror hit the test_and_clear
and skipped.

  What the server sees today: for a mirror with N stripes, a LAYOUTSTATS
  report carries exactly one ff_layoutupdate4, for the lowest usable
  dss_id, no matter how many stripes are busy or how the I/O is spread
  across them.  Stripes 1..N-1 are reported only when the layout is
  returned.  For the 32-stripe RAID0 layout this series targets, the
  server is picking data servers on 1/32 of the picture, and always the
  same 1/32 -- a stripe that is idle while the other 31 are saturated
  reports zeros, and a stripe that is saturated while stripe 0 is idle
  never reports at all.

So drop the bit rather than fix it.  The reporting path already reads
ops_completed for both directions under their locks; remember what it
reported and skip a stripe whose count has not moved since.  That is
strictly more accurate than the flag was -- it is per stripe by
construction, and it cannot be left set by I/O that has already been
reported -- and it costs the I/O paths nothing.

  What the server sees after this patch: up to dev_limit
  ff_layoutupdate4 entries, one per stripe that has completed I/O since
  its own last report, in dss_id order.  The numbers themselves are
  unchanged -- same cumulative ops_completed and bytes_completed, same
  deviceid, same encoding -- there are just more of them, and they
  describe the stripes that did the work.  A stripe with no completions
  since its last report is now omitted rather than reported as an
  unchanged repeat; a server differencing successive reports sees the
  same thing either way.  LAYOUTRETURN still reports every stripe it can
  fit, as before.

nfs4_ff_layout_ds_stripe::last_reported_ops is deliberately out in the
read-mostly region rather than in either nfs4_ff_layoutstat: it is
touched only by ff_layout_mirror_prepare_stats(), about once per report
interval, so it must not consume one of the 64 bytes an I/O dirties.  It
lands in the existing hole ahead of read_stat, so sizeof is still 320.
Both callers of ff_layout_mirror_prepare_stats() hold the inode's i_lock,
which serialises it; no new lock is needed.

NFS4_FF_MIRROR_STAT_AVAIL was the only bit ever defined on
nfs4_ff_layout_mirror::flags, so the field goes too.

Note that dev_limit is 4 in both callers, so N > 4 stripes still cannot
all be reported at once; the next patch rotates which ones are.

Signed-off-by: Mike Snitzer <snitzer@hammerspace.com>
---
 fs/nfs/flexfilelayout/flexfilelayout.c | 43 +++++++++++++++-----------
 fs/nfs/flexfilelayout/flexfilelayout.h | 12 +++++--
 2 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index 0b8562efabb24..670780bb3d353 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -829,7 +829,6 @@ nfs4_ff_layout_stat_io_start_read(struct inode *inode,
 	report = nfs4_ff_layoutstat_start_io(dss_info, &dss_info->read_stat,
 					     requested, now);
 	spin_unlock(&dss_info->read_stat.lock);
-	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
 
 	if (report)
 		pnfs_report_layoutstat(inode, nfs_io_gfp_mask());
@@ -849,7 +848,6 @@ nfs4_ff_layout_stat_io_end_read(struct rpc_task *task,
 				  requested, completed,
 				  ktime_get(), task->tk_start);
 	spin_unlock(&dss_info->read_stat.lock);
-	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
 }
 
 static void
@@ -865,7 +863,6 @@ nfs4_ff_layout_stat_io_start_write(struct inode *inode,
 	report = nfs4_ff_layoutstat_start_io(dss_info, &dss_info->write_stat,
 					     requested, now);
 	spin_unlock(&dss_info->write_stat.lock);
-	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
 
 	if (report)
 		pnfs_report_layoutstat(inode, nfs_io_gfp_mask());
@@ -889,7 +886,6 @@ nfs4_ff_layout_stat_io_end_write(struct rpc_task *task,
 				  requested, completed,
 				  ktime_get(), task->tk_start);
 	spin_unlock(&dss_info->write_stat.lock);
-	set_bit(NFS4_FF_MIRROR_STAT_AVAIL, &mirror->flags);
 }
 
 static struct nfs4_ff_layout_ds *
@@ -3156,6 +3152,7 @@ ff_layout_mirror_prepare_stats(struct pnfs_layout_hdr *lo,
 	struct nfs4_ff_layout_mirror *mirror;
 	struct nfs4_ff_layout_ds_stripe *dss_info;
 	struct nfs4_ff_layout_ds *mirror_ds;
+	__u64 read_count, read_bytes, write_count, write_bytes, ops;
 	int i = 0, dss_id;
 
 	rcu_read_lock();
@@ -3167,13 +3164,31 @@ ff_layout_mirror_prepare_stats(struct pnfs_layout_hdr *lo,
 			mirror_ds = rcu_dereference(dss_info->mirror_ds);
 			if (IS_ERR_OR_NULL(mirror_ds))
 				continue;
-			if (!test_and_clear_bit(NFS4_FF_MIRROR_STAT_AVAIL,
-						&mirror->flags) &&
+
+			spin_lock(&dss_info->read_stat.lock);
+			read_count = dss_info->read_stat.io_stat.ops_completed;
+			read_bytes = dss_info->read_stat.io_stat.bytes_completed;
+			spin_unlock(&dss_info->read_stat.lock);
+			spin_lock(&dss_info->write_stat.lock);
+			write_count = dss_info->write_stat.io_stat.ops_completed;
+			write_bytes = dss_info->write_stat.io_stat.bytes_completed;
+			spin_unlock(&dss_info->write_stat.lock);
+
+			/*
+			 * Nothing has completed on this stripe since it last
+			 * reported, so do not spend one of the few devinfo
+			 * slots on repeating those numbers.  A LAYOUTRETURN is
+			 * the last chance to report and says everything.
+			 */
+			ops = read_count + write_count;
+			if (ops == dss_info->last_reported_ops &&
 			    type != NFS4_FF_OP_LAYOUTRETURN)
 				continue;
+
 			/* mirror refcount put in cleanup_layoutstats */
 			if (!refcount_inc_not_zero(&mirror->ref))
 				continue;
+			dss_info->last_reported_ops = ops;
 			/* The pin holds a reference; it is exchanged out only
 			 * under i_lock.  Put in ff_layout_free_layoutstats().
 			 */
@@ -3183,18 +3198,10 @@ ff_layout_mirror_prepare_stats(struct pnfs_layout_hdr *lo,
 			       NFS4_DEVICEID4_SIZE);
 			devinfo->offset = 0;
 			devinfo->length = NFS4_MAX_UINT64;
-			spin_lock(&dss_info->read_stat.lock);
-			devinfo->read_count =
-			    dss_info->read_stat.io_stat.ops_completed;
-			devinfo->read_bytes =
-			    dss_info->read_stat.io_stat.bytes_completed;
-			spin_unlock(&dss_info->read_stat.lock);
-			spin_lock(&dss_info->write_stat.lock);
-			devinfo->write_count =
-			    dss_info->write_stat.io_stat.ops_completed;
-			devinfo->write_bytes =
-			    dss_info->write_stat.io_stat.bytes_completed;
-			spin_unlock(&dss_info->write_stat.lock);
+			devinfo->read_count = read_count;
+			devinfo->read_bytes = read_bytes;
+			devinfo->write_count = write_count;
+			devinfo->write_bytes = write_bytes;
 			devinfo->layout_type = LAYOUT_FLEX_FILES;
 			devinfo->ld_private.ops = &layoutstat_ops;
 			priv->dss_info = dss_info;
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h
index 876fdb6999473..743957cd12300 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.h
+++ b/fs/nfs/flexfilelayout/flexfilelayout.h
@@ -113,6 +113,15 @@ struct nfs4_ff_layout_ds_stripe {
 	struct nfs_file_localio		nfl;
 	/* Published once by the first I/O; see nfs4_ff_layoutstat_set_start_time() */
 	ktime_t				start_time;
+	/*
+	 * Completed ops, both directions, as of this stripe's last
+	 * LAYOUTSTATS report.  Touched only by
+	 * ff_layout_mirror_prepare_stats(), so roughly once per report
+	 * interval, and serialised by the inode's i_lock, which both of its
+	 * callers hold.  Deliberately out here rather than in either
+	 * nfs4_ff_layoutstat: it must not cost the I/O paths a cacheline.
+	 */
+	__u64				last_reported_ops;
 	/*
 	 * A line each, and each carrying its own lock, so that a read and a
 	 * write to this stripe neither serialise against each other nor
@@ -131,12 +140,9 @@ struct nfs4_ff_layout_mirror {
 	u32				dss_count;
 	struct nfs4_ff_layout_ds_stripe *dss;
 	refcount_t			ref;
-	unsigned long			flags;
 	u32				report_interval;
 };
 
-#define NFS4_FF_MIRROR_STAT_AVAIL	(0)
-
 struct nfs4_ff_layout_segment {
 	struct pnfs_layout_segment	generic_hdr;
 	u64				stripe_unit;
-- 
2.52.0

[PATCH 10/11] NFSv4/flexfiles: rotate LAYOUTSTATS reporting across a mirror's stripes

[Mike Snitzer]

From: Mike Snitzer <snitzer@hammerspace.com>

Both callers of ff_layout_mirror_prepare_stats() cap it at four
ff_layoutupdate4 entries -- PNFS_LAYOUTSTATS_MAXDEV for LAYOUTSTATS and
ARRAY_SIZE(ff_args->devinfo), which is FF_LAYOUTSTATS_MAXDEV, for
LAYOUTRETURN.  The scan walks mirror->dss[] from index 0 every time and
stops once the budget is spent.

  What the server sees without this patch: for a mirror with more than
  four busy stripes, the same low dss_ids every report, forever.  The
  previous patch widened that from one stripe to four -- it did not stop
  it being the same four.  On the 32-stripe RAID0 layout this series
  targets, stripes 4..31 are reported only when the layout is returned,
  so a data server that is only ever backing a high stripe index stays
  invisible to the server's placement decisions no matter how much I/O it
  does.

Remember where each mirror's scan stopped and resume there.  The cursor
advances past each stripe that actually fills a slot, so a report picks
up where its predecessor left off and idle stripes, which are skipped
without consuming a slot, do not stall the rotation.

  What the server sees with this patch: still up to four entries per
  report, but rotating, so every busy stripe of a mirror is reported
  within ceil(busy_stripes / 4) report intervals -- eight intervals, or
  about eight minutes at the default 60 second interval, for 32 busy
  stripes.  A given stripe's numbers therefore arrive less often than
  before but, unlike before, they arrive.  The values are unchanged:
  cumulative ops_completed and bytes_completed since the layout was
  granted, so a server differencing successive reports for a stripe still
  gets a correct delta, just over a longer window.  Nothing about the
  encoding, the deviceid or the ordering within a report changes.

The mirror loop gets the same dev_limit test as the stripe loop, so a
full report stops scanning instead of walking the remaining mirrors to
break immediately on each.

[jonathan.flynn: read dss_report_start once per mirror, before the stripe
 loop.  The loop advances it after each stripe it reports, so re-reading
 it moved the index twice: stripes were skipped, and on a mirror with
 stripes not yet written (and so skipped) the index wrapped back onto a
 stripe already reported.  That put the same deviceid in one ff_iostats4
 array twice with identical counters.]
Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
Signed-off-by: Mike Snitzer <snitzer@hammerspace.com>
---
 fs/nfs/flexfilelayout/flexfilelayout.c | 22 +++++++++++++++++++---
 fs/nfs/flexfilelayout/flexfilelayout.h |  7 +++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index 670780bb3d353..591363e2cf460 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -3153,14 +3153,28 @@ ff_layout_mirror_prepare_stats(struct pnfs_layout_hdr *lo,
 	struct nfs4_ff_layout_ds_stripe *dss_info;
 	struct nfs4_ff_layout_ds *mirror_ds;
 	__u64 read_count, read_bytes, write_count, write_bytes, ops;
-	int i = 0, dss_id;
+	u32 dss_id, scanned, base;
+	int i = 0;
 
 	rcu_read_lock();
 	list_for_each_entry(mirror, &ff_layout->mirrors, mirrors) {
-		for (dss_id = 0; dss_id < mirror->dss_count; ++dss_id) {
-			dss_info = &mirror->dss[dss_id];
+		if (i >= dev_limit)
+			break;
+		base = mirror->dss_report_start;
+		for (scanned = 0; scanned < mirror->dss_count; ++scanned) {
 			if (i >= dev_limit)
 				break;
+			/* Resume where the last report of this mirror left
+			 * off; one subtraction suffices to wrap, as both
+			 * terms are below dss_count.  Use the start as it
+			 * was on entry: this loop advances it for the next
+			 * report, and re-reading it would skip stripes and
+			 * wrap back onto one already reported.
+			 */
+			dss_id = base + scanned;
+			if (dss_id >= mirror->dss_count)
+				dss_id -= mirror->dss_count;
+			dss_info = &mirror->dss[dss_id];
 			mirror_ds = rcu_dereference(dss_info->mirror_ds);
 			if (IS_ERR_OR_NULL(mirror_ds))
 				continue;
@@ -3189,6 +3203,8 @@ ff_layout_mirror_prepare_stats(struct pnfs_layout_hdr *lo,
 			if (!refcount_inc_not_zero(&mirror->ref))
 				continue;
 			dss_info->last_reported_ops = ops;
+			mirror->dss_report_start =
+				dss_id + 1 < mirror->dss_count ? dss_id + 1 : 0;
 			/* The pin holds a reference; it is exchanged out only
 			 * under i_lock.  Put in ff_layout_free_layoutstats().
 			 */
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h
index 743957cd12300..33724f2db5d62 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.h
+++ b/fs/nfs/flexfilelayout/flexfilelayout.h
@@ -141,6 +141,13 @@ struct nfs4_ff_layout_mirror {
 	struct nfs4_ff_layout_ds_stripe *dss;
 	refcount_t			ref;
 	u32				report_interval;
+	/*
+	 * dss[] index to begin the next LAYOUTSTATS scan of this mirror at,
+	 * so that a mirror with more stripes than a report has room for
+	 * rotates through them rather than always reporting the lowest.
+	 * Serialised by the inode's i_lock, like last_reported_ops.
+	 */
+	u32				dss_report_start;
 };
 
 struct nfs4_ff_layout_segment {
-- 
2.52.0

[PATCH 11/11] NFSv4/flexfiles: take the report decision out of the critical section

[Mike Snitzer]

From: Mike Snitzer <snitzer@hammerspace.com>

Now that each direction's statistics are one cacheline behind their own
lock, what is left inside that lock matters.  On the requested side it was
not just the counters:

  - dss_info->mirror and mirror->layout, two pointer chases into other
    objects
  - mirror->report_interval, on the mirror's cacheline
  - the layoutstats_timer module parameter
  - ktime_to_ms(), a division
  - ffl->last_report_time, on the layout header's cacheline, plus a
    try_cmpxchg64() on it once per report interval
  - dss_info->start_time, on the stripe's read-mostly cacheline

None of that is covered by the lock, and none of it touches the data the
lock guards.  mirror->report_interval is written once when the layout
segment is decoded and read-only afterwards; ffl->last_report_time is
shared by every mirror and stripe of the layout and has carried its own
cmpxchg since the statistics moved to the stripe; start_time has been
write-once, published by cmpxchg, since "NFSv4/flexfiles: make
dss_info->start_time write-once".  So none of them were being protected,
they were only being waited for.

Split the report decision out into nfs4_ff_layoutstat_report_due() and
call it, and nfs4_ff_layoutstat_set_start_time(), after the unlock.  The
critical section becomes what it should be: bump the in-flight count,
add to bytes_requested.  Two writes, one cacheline, no dependent loads
into other objects and no division.

The reads themselves are unchanged and still happen once per RPC.  They
are on read-mostly lines, so they share rather than bounce and are not
worth eliminating without a profile that says otherwise; the point here
is only that an I/O no longer waits behind them.

Ordering is unchanged: the statistics are updated before the decision is
taken, as before, and pnfs_report_layoutstat() was already called after
the unlock.

nfs4_ff_layoutstat_end_io() loses its dss_info argument, unused since the
lock moved into nfs4_ff_layoutstat.

Signed-off-by: Mike Snitzer <snitzer@hammerspace.com>
---
 fs/nfs/flexfilelayout/flexfilelayout.c | 60 +++++++++++++++-----------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index 591363e2cf460..90b3be4a94c80 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -745,39 +745,48 @@ nfs4_ff_layoutstat_set_start_time(struct nfs4_ff_layout_ds_stripe *dss_info,
 }
 
 /*
- * Account for an I/O about to be sent to this stripe, and return true if a
- * LAYOUTSTATS report is due.
+ * Account for an I/O about to be sent to this stripe.
  *
  * The in-flight count and the requested-side counters are bumped here
  * together, so a new requested-side path cannot update one and miss the
- * other.
+ * other.  Everything this touches is in the cacheline the caller's lock
+ * guards; anything else belongs outside that lock.
  */
-static bool
-nfs4_ff_layoutstat_start_io(struct nfs4_ff_layout_ds_stripe *dss_info,
-			    struct nfs4_ff_layoutstat *layoutstat,
+static void
+nfs4_ff_layoutstat_start_io(struct nfs4_ff_layoutstat *layoutstat,
 			    __u64 requested, ktime_t now)
+{
+	lockdep_assert_held(&layoutstat->lock);
+
+	nfs4_ff_start_busy_timer(layoutstat, now);
+	layoutstat->io_stat.bytes_requested += requested;
+}
+
+/*
+ * Is a LAYOUTSTATS report due?  Deliberately outside the statistics locks:
+ * none of this is covered by them, and all of it lives in other objects --
+ * mirror->report_interval and ffl->last_report_time, plus a division -- so
+ * running it under a lock would only lengthen a critical section that is
+ * otherwise two writes to one cacheline.
+ *
+ * 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.
+ */
+static bool
+nfs4_ff_layoutstat_report_due(struct nfs4_ff_layout_ds_stripe *dss_info,
+			      ktime_t now)
 {
 	struct nfs4_ff_layout_mirror *mirror = dss_info->mirror;
 	struct nfs4_flexfile_layout *ffl = FF_LAYOUT_FROM_HDR(mirror->layout);
 	s64 report_interval = FF_LAYOUTSTATS_REPORT_INTERVAL;
 	ktime_t last_report;
 
-	lockdep_assert_held(&layoutstat->lock);
-
-	nfs4_ff_start_busy_timer(layoutstat, now);
-	layoutstat->io_stat.bytes_requested += requested;
-
-	nfs4_ff_layoutstat_set_start_time(dss_info, now);
 	if (mirror->report_interval != 0)
 		report_interval = (s64)mirror->report_interval * 1000LL;
 	else if (layoutstats_timer != 0)
 		report_interval = (s64)layoutstats_timer * 1000LL;
 
-	/*
-	 * 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;
@@ -791,8 +800,7 @@ nfs4_ff_layoutstat_start_io(struct nfs4_ff_layout_ds_stripe *dss_info,
  * it is bumped in nfs4_ff_layoutstat_start_io().
  */
 static void
-nfs4_ff_layoutstat_end_io(struct nfs4_ff_layout_ds_stripe *dss_info,
-			  struct nfs4_ff_layoutstat *layoutstat,
+nfs4_ff_layoutstat_end_io(struct nfs4_ff_layoutstat *layoutstat,
 			  __u64 requested,
 			  __u64 completed,
 			  ktime_t time_completed,
@@ -826,10 +834,11 @@ nfs4_ff_layout_stat_io_start_read(struct inode *inode,
 	bool report;
 
 	spin_lock(&dss_info->read_stat.lock);
-	report = nfs4_ff_layoutstat_start_io(dss_info, &dss_info->read_stat,
-					     requested, now);
+	nfs4_ff_layoutstat_start_io(&dss_info->read_stat, requested, now);
 	spin_unlock(&dss_info->read_stat.lock);
 
+	nfs4_ff_layoutstat_set_start_time(dss_info, now);
+	report = nfs4_ff_layoutstat_report_due(dss_info, now);
 	if (report)
 		pnfs_report_layoutstat(inode, nfs_io_gfp_mask());
 }
@@ -844,7 +853,7 @@ nfs4_ff_layout_stat_io_end_read(struct rpc_task *task,
 	struct nfs4_ff_layout_ds_stripe *dss_info = &mirror->dss[dss_id];
 
 	spin_lock(&dss_info->read_stat.lock);
-	nfs4_ff_layoutstat_end_io(dss_info, &dss_info->read_stat,
+	nfs4_ff_layoutstat_end_io(&dss_info->read_stat,
 				  requested, completed,
 				  ktime_get(), task->tk_start);
 	spin_unlock(&dss_info->read_stat.lock);
@@ -860,10 +869,11 @@ nfs4_ff_layout_stat_io_start_write(struct inode *inode,
 	bool report;
 
 	spin_lock(&dss_info->write_stat.lock);
-	report = nfs4_ff_layoutstat_start_io(dss_info, &dss_info->write_stat,
-					     requested, now);
+	nfs4_ff_layoutstat_start_io(&dss_info->write_stat, requested, now);
 	spin_unlock(&dss_info->write_stat.lock);
 
+	nfs4_ff_layoutstat_set_start_time(dss_info, now);
+	report = nfs4_ff_layoutstat_report_due(dss_info, now);
 	if (report)
 		pnfs_report_layoutstat(inode, nfs_io_gfp_mask());
 }
@@ -882,7 +892,7 @@ nfs4_ff_layout_stat_io_end_write(struct rpc_task *task,
 		requested = completed = 0;
 
 	spin_lock(&dss_info->write_stat.lock);
-	nfs4_ff_layoutstat_end_io(dss_info, &dss_info->write_stat,
+	nfs4_ff_layoutstat_end_io(&dss_info->write_stat,
 				  requested, completed,
 				  ktime_get(), task->tk_start);
 	spin_unlock(&dss_info->write_stat.lock);
-- 
2.52.0