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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/9/15 19:25, Johannes Thumshirn wrote:
> On 15.09.23 02:27, Qu Wenruo wrote:
>>>>     /*
>>>>      * 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?
>
> Nope. The length in the key is the stripe length. The length in the
> stride is the stride length.
>
> Here's an example for why this is needed:
>
> wrote 32768/32768 bytes at offset 0
> XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> wrote 131072/131072 bytes at offset 0
> XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> wrote 8192/8192 bytes at offset 65536
> XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>
> [snip]
>
>           item 0 key (XXXXXX RAID_STRIPE_KEY 32768) itemoff XXXXX itemsize 32
>                           encoding: RAID0
>                           stripe 0 devid 1 physical XXXXXXXXX length 32768
>           item 1 key (XXXXXX RAID_STRIPE_KEY 131072) itemoff XXXXX
> itemsize 80

Maybe you want to put the whole RAID_STRIPE_KEY definition into the headers.

In fact my initial assumption of such case would be something like this:

            item 0 key (X+0 RAID_STRIPE 32K)
		stripe 0 devid 1 physical XXXXX len 32K
	   item 1 key (X+32K RAID_STRIPE 32K)
		stripe 0 devid 1 physical XXXXX + 32K len 32K
	   item 2 key (X+64K RAID_STRIPE 64K)
		stripe 0 devid 2 physical YYYYY len 64K
	   item 3 key (X+128K RAID_STRIPE 32K)
		stripe 0 devid 1 physical XXXXX + 64K len 32K
            ...

AKA, each RAID_STRIPE_KEY would only contain a continous physical stripe.
And in above case, item 0 and item 1 can be easily merged, also length
can be removed.

And this explains why the lookup code is more complex than I initially
thought.

BTW, would the above layout make the code a little easier?
Or is there any special reason for the existing one layout?

Thank,
Qu


>                           encoding: RAID0
>                           stripe 0 devid 1 physical XXXXXXXXX length 32768
>                           stripe 1 devid 2 physical XXXXXXXXX length 65536
>                           stripe 2 devid 1 physical XXXXXXXXX length 32768

This current layout has another problem.
For RAID10 the interpretation of the RAID_STRIPE item can be very complex.
While

>           item 2 key (XXXXXX RAID_STRIPE_KEY 8192) itemoff XXXXX itemsize 32
>                           encoding: RAID0
>                           stripe 0 devid 1 physical XXXXXXXXX length 8192
>
> Without the length in the stride, we don't know when to select the next
> stride in item 1 above.