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

[Johannes Thumshirn]

On 15.09.23 12:34, Qu Wenruo wrote:
> 
> 
> 
> 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.
> 


JFYI preliminary tests for your suggestion look reasonably good. I'll 
give it some more testing and code cleanup but it actually seems 
sensible to do.