Re: [PATCH 7/8] btrfs: use a dedicated data structure for chunk maps

[David Sterba <dsterba@suse.cz>]

On Wed, Nov 22, 2023 at 11:32:33AM +0000, Filipe Manana wrote:
> On Tue, Nov 21, 2023 at 6:30 PM David Sterba <dsterba@suse.cz> wrote:
> > On Tue, Nov 21, 2023 at 01:38:38PM +0000, fdmanana@kernel.org wrote:
> >
> > struct btrfs_chunk_map {
> >         struct rb_node             rb_node __attribute__((__aligned__(8))); /*     0    24 */
> >         refcount_t                 refs;                 /*    24     4 */
> >
> >         /* XXX 4 bytes hole, try to pack */
> >
> >         u64                        start;                /*    32     8 */
> >         u64                        chunk_len;            /*    40     8 */
> >         u64                        stripe_size;          /*    48     8 */
> >         u64                        type;                 /*    56     8 */
> >         /* --- cacheline 1 boundary (64 bytes) --- */
> >         int                        io_align;             /*    64     4 */
> >         int                        io_width;             /*    68     4 */
> >         int                        num_stripes;          /*    72     4 */
> >         int                        sub_stripes;          /*    76     4 */
> >         int                        verified_stripes;     /*    80     4 */
> >
> >         /* XXX 4 bytes hole, try to pack */
> >
> >         struct btrfs_io_stripe     stripes[];            /*    88     0 */
> >
> >         /* size: 88, cachelines: 2, members: 12 */
> >         /* sum members: 80, holes: 2, sum holes: 8 */
> >         /* forced alignments: 1 */
> >         /* last cacheline: 24 bytes */
> > } __attribute__((__aligned__(8)));
> >
> > I could move verify_stripes after refs or move refs to start of the
> > second cacheline between type and io_align. I suspect some cache
> > bouncing could happen with refcount updates and tree traversal but it's
> > a speculation and I don't think the effects would be measurable.
> 
> I would prefer to have verified_stripes in the first cache line, right
> below refs for example, so that
> everything needed for map lookups and insertions is in the same cache line.

Right, that makes sense.

> Do you want to squash such change in this patch? Or should I send it separately?

I'll do the change, thanks.