Re: [PATCH] btrfs: remove extent buffer's redundant `len` member field

[David Sterba <dsterba@suse.cz>]

On Fri, May 02, 2025 at 02:03:55PM +0200, Daniel Vacek wrote:
> > Yeah, 256 is a nice number because it aligns with cachelines on multiple
> > architectures, this is useful for splitting the structure to the "data
> > accessed together" and locking/refcounting. It's a tentative goal, we
> > used to have larger eb size due to own locking implementation but with
> > rwsems it got close/under 256.
> >
> > The current size 240 is 1/4 of cacheline shifted so it's not all clean
> > but whe have some wiggle room for adding new members or cached values,
> > like folio_size/folio_shift/addr.
> 
> Sounds like we could force align to cacheline or explicitly pad to
> 256B? The later could be a bit tricky though.

We could and we also have conflicting goals:

- alignment (with some waste)

- more objects packed into the slab which is 8K

More objects mean better chance to satisfy allocations, that are right
now NOFAIL, so blocking metadata operations with worse consequences.

> > >         struct btrfs_fs_info *fs_info;
> > >
> > >         /*
> > > @@ -94,9 +97,6 @@ struct extent_buffer {
> > >         spinlock_t refs_lock;
> > >         atomic_t refs;
> > >         int read_mirror;
> > > -       /* >= 0 if eb belongs to a log tree, -1 otherwise */
> > > -       s8 log_index;
> > > -       u8 folio_shift;
> > >         struct rcu_head rcu_head;
> > >
> > >         struct rw_semaphore lock;
> > >
> > > you're down to 256 even on -rt. And the great part is I don't see any
> > > sacrifices (other than accessing a cacheline in fs_info). We're only
> > > using 8 flags now, so there is still some room left for another 8 if
> > > needed in the future.
> >
> > Which means that the size on non-rt would be something like 228, roughly
> > calculating the savings and the increase due to spinloct_t going from
> > 4 -> 32 bytes. Also I'd like to see the generated assembly after the
> > suggested reordering.
> 
> If I see correctly the non-rt will not change when I keep ulong
> bflags. The -rt build goes down to 264 bytes. That's a bit better for
> free but still not ideal from alignment POV.
> 
> > The eb may not be perfect, I think there could be false sharing of
> > refs_lock and refs but this is a wild guess and based only on code
> 
> refs_lock and refs look like they share the same cacheline in every
> case. At least on x86.
> But still, the slab object is not aligned in the first place. Luckily
> the two fields roam together.
> 
> Out of curiosity, is there any past experience where this kind of
> optimizations make a difference within a filesystem code?
> I can imagine perhaps for fast devices like NVDIMM or DAX the CPU may
> become the bottleneck? Or are nowadays NVMe devices already fast
> enough to saturate the CPU?

I don't have a specific example. This is tricky to measure and
historically the devices were slow so any IO and waiting made the cache
effects irrelevant. With NVMe, modern CPUs we might start seeing that.
Instrumentation or profiling of the structures can do that, there are
tools for that but the variability of the hardware combinations and
runtime conditions makes it hard so I'm resorting to more "static"
approach and go after known good patterns like alignment or placement
related to cachelines (manually or with ____cacheline_aligned_in_smp).

> I faintly recall one issue where I debugged a CPU which could not keep
> up with handling the interrupts of finished IO on NVMe submitted by
> other CPUs. Though that was on xfs (or maybe ext4) not on btrfs. But
> that was a power-play of one against the rest as the interrupt was not
> balanced or spread to more CPUs.