Re: [PATCH v9 01/11] btrfs: add raid stripe tree definitions

[Qu Wenruo]

On 2023/9/15 09:52, Qu Wenruo wrote:
> On 2023/9/15 01:36, Johannes Thumshirn wrote:
> > Add definitions for the raid stripe tree. This tree will hold information
> > about the on-disk layout of the stripes in a RAID set.
> >
> > Each stripe extent has a 1:1 relationship with an on-disk extent item and
> > is doing the logical to per-drive physical address translation for the
> > extent item in question.
> >
> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
> > ---
> >   fs/btrfs/accessors.h            | 10 ++++++++++
> >   fs/btrfs/locking.c              |  1 +
> >   include/uapi/linux/btrfs_tree.h | 31 +++++++++++++++++++++++++++++++
> >   3 files changed, 42 insertions(+)
> >
> > diff --git a/fs/btrfs/accessors.h b/fs/btrfs/accessors.h
> > index f958eccff477..977ff160a024 100644
> > --- a/fs/btrfs/accessors.h
> > +++ b/fs/btrfs/accessors.h
> > @@ -306,6 +306,16 @@ BTRFS_SETGET_FUNCS(timespec_nsec, struct 
> > btrfs_timespec, nsec, 32);
> >   BTRFS_SETGET_STACK_FUNCS(stack_timespec_sec, struct btrfs_timespec, 
> > sec, 64);
> >   BTRFS_SETGET_STACK_FUNCS(stack_timespec_nsec, struct btrfs_timespec, 
> > nsec, 32);
> > +BTRFS_SETGET_FUNCS(stripe_extent_encoding, struct 
> > btrfs_stripe_extent, encoding, 8);
> > +BTRFS_SETGET_FUNCS(raid_stride_devid, struct btrfs_raid_stride, 
> > devid, 64);
> > +BTRFS_SETGET_FUNCS(raid_stride_physical, struct btrfs_raid_stride, 
> > physical, 64);
> > +BTRFS_SETGET_FUNCS(raid_stride_length, struct btrfs_raid_stride, 
> > length, 64);
> > +BTRFS_SETGET_STACK_FUNCS(stack_stripe_extent_encoding,
> > +             struct btrfs_stripe_extent, encoding, 8);
> > +BTRFS_SETGET_STACK_FUNCS(stack_raid_stride_devid, struct 
> > btrfs_raid_stride, devid, 64);
> > +BTRFS_SETGET_STACK_FUNCS(stack_raid_stride_physical, struct 
> > btrfs_raid_stride, physical, 64);
> > +BTRFS_SETGET_STACK_FUNCS(stack_raid_stride_length, struct 
> > btrfs_raid_stride, length, 64);
> > +
> >   /* struct btrfs_dev_extent */
> >   BTRFS_SETGET_FUNCS(dev_extent_chunk_tree, struct btrfs_dev_extent, 
> > chunk_tree, 64);
> >   BTRFS_SETGET_FUNCS(dev_extent_chunk_objectid, struct btrfs_dev_extent,
> > diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
> > index 6ac4fd8cc8dc..74d8e2003f58 100644
> > --- a/fs/btrfs/locking.c
> > +++ b/fs/btrfs/locking.c
> > @@ -74,6 +74,7 @@ static struct btrfs_lockdep_keyset {
> >       { .id = BTRFS_UUID_TREE_OBJECTID,    DEFINE_NAME("uuid")    },
> >       { .id = BTRFS_FREE_SPACE_TREE_OBJECTID,    
> > DEFINE_NAME("free-space") },
> >       { .id = BTRFS_BLOCK_GROUP_TREE_OBJECTID, 
> > DEFINE_NAME("block-group") },
> > +    { .id = BTRFS_RAID_STRIPE_TREE_OBJECTID, 
> > DEFINE_NAME("raid-stripe") },
> >       { .id = 0,                DEFINE_NAME("tree")    },
> >   };
> > diff --git a/include/uapi/linux/btrfs_tree.h 
> > b/include/uapi/linux/btrfs_tree.h
> > index fc3c32186d7e..6d9c43416b6e 100644
> > --- a/include/uapi/linux/btrfs_tree.h
> > +++ b/include/uapi/linux/btrfs_tree.h
> > @@ -73,6 +73,9 @@
> >   /* Holds the block group items for extent tree v2. */
> >   #define BTRFS_BLOCK_GROUP_TREE_OBJECTID 11ULL
> > +/* Tracks RAID stripes in block groups. */
> > +#define BTRFS_RAID_STRIPE_TREE_OBJECTID 12ULL
> > +
> >   /* device stats in the device tree */
> >   #define BTRFS_DEV_STATS_OBJECTID 0ULL
> > @@ -261,6 +264,8 @@
> >   #define BTRFS_DEV_ITEM_KEY    216
> >   #define BTRFS_CHUNK_ITEM_KEY    228
> > +#define BTRFS_RAID_STRIPE_KEY    230
> > +
> >   /*
> >    * Records the overall state of the qgroups.
> >    * There's only one instance of this key present,
> > @@ -719,6 +724,32 @@ struct btrfs_free_space_header {
> >       __le64 num_bitmaps;
> >   } __attribute__ ((__packed__));
> > +struct btrfs_raid_stride {
> > +    /* The btrfs device-id this raid extent lives on */
> > +    __le64 devid;
> > +    /* The physical location on disk */
> > +    __le64 physical;
> > +    /* The length of stride on this disk */
> > +    __le64 length;

Forgot to mention, for btrfs_stripe_extent structure, its key is 
(PHYSICAL, RAID_STRIPE_KEY, LENGTH) right?

So is the length in the btrfs_raid_stride duplicated and we can save 8 
bytes?

Thanks,
Qu
> > +} __attribute__ ((__packed__));
> > +
> > +/* The stripe_extent::encoding, 1:1 mapping of enum btrfs_raid_types */
> > +#define BTRFS_STRIPE_RAID0    1
> > +#define BTRFS_STRIPE_RAID1    2
> > +#define BTRFS_STRIPE_DUP    3
> > +#define BTRFS_STRIPE_RAID10    4
> > +#define BTRFS_STRIPE_RAID5    5
> > +#define BTRFS_STRIPE_RAID6    6
> > +#define BTRFS_STRIPE_RAID1C3    7
> > +#define BTRFS_STRIPE_RAID1C4    8
> > +
> > +struct btrfs_stripe_extent {
> > +    __u8 encoding;
>
> Considerng the encoding for now is 1:1 map of btrfs_raid_types, and 
> normally we use variable like @raid_index for such usage, have 
> considered rename it to "raid_index" or "profile_index" instead?
>
> Another thing is, you may want to add extra tree-checker code to verify 
> the btrfs_stripe_extent members.
>
> For encoding, it should be all be the known numbers, and item size for 
> alignment.
>
> The same for physical/length alignment checks.
>
> Thanks,
> Qu
> > +    __u8 reserved[7];
> > +    /* An array of raid strides this stripe is composed of */
> > +    struct btrfs_raid_stride strides[];
> > +} __attribute__ ((__packed__));
> > +
> >   #define BTRFS_HEADER_FLAG_WRITTEN    (1ULL << 0)
> >   #define BTRFS_HEADER_FLAG_RELOC        (1ULL << 1)