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

[Mike Snitzer <snitzer@kernel.org>]

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