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

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Nov 05, 2026 at 10:34:56AM -0500, Anna Schumaker wrote:
> Hi Mike,
> 
> On Mon, Nov 2, 2026, at 2:11 PM, Mike Snitzer wrote:
> > 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.
> 
> This commit message is very wordy, and can probably be edited down.

OK, will fix. (I'll go over the entire series on this point.)

> >
> > 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.
> 
> Let's leave the original comment alone, since most of this information
> is  duplicated in the commit message and can be easily found with
> `git blame`.

Sure.

> > +	 */
> > +	spinlock_t			lock ____cacheline_aligned_in_smp;
> 
> Making this cacheline aligned seems like a good idea!

Yes, thanks.

Mike