Re: [PATCH 8/9] btrfs: make more generic the code for RAID 6 rebuilding

[Goffredo Baroncelli]

On 05/30/2018 02:05 PM, Daniel Kiper wrote:
> On Wed, May 16, 2018 at 08:48:18PM +0200, Goffredo Baroncelli wrote:
>> The original code which handles the recovery of a RAID 6 disks array
>> assumes that all reads are multiple of 1 << GRUB_DISK_SECTOR_BITS and it
>> assumes that all the I/O is done via the struct grub_diskfilter_segment.
>> This is not true for the btrfs code. In order to reuse the native
>> grub_raid6_recover() code, it is modified to not call
>> grub_diskfilter_read_node() directly, but to call an handler passed
>> as argument.
> 
> s/as argument/as an argument/
ok
> 
>> Signed-off-by: Goffredo Baroncelli <address@hidden>
>> ---
>>  grub-core/disk/raid6_recover.c | 53 ++++++++++++++++++++++------------
>>  include/grub/diskfilter.h      |  9 ++++++
>>  2 files changed, 44 insertions(+), 18 deletions(-)
>>
>> diff --git a/grub-core/disk/raid6_recover.c b/grub-core/disk/raid6_recover.c
>> index aa674f6ca..10ee495e2 100644
>> --- a/grub-core/disk/raid6_recover.c
>> +++ b/grub-core/disk/raid6_recover.c
>> @@ -74,14 +74,25 @@ mod_255 (unsigned x)
>>  }
>>
>>  static grub_err_t
>> -grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int 
>> p,
>> -                    char *buf, grub_disk_addr_t sector, grub_size_t size)
>> +raid6_recover_read_node (void *data, int disk_nr,
> 
> s/disk_nr/disknr/

OK

>> +                                grub_uint64_t sector,
>> +                                void *buf, grub_size_t size)
>> +{
>> +    struct grub_diskfilter_segment *array = data;
> 
> Please add one empty line here.
> 
>> +    return grub_diskfilter_read_node (&array->nodes[disk_nr],
>> +                                  (grub_disk_addr_t)sector,
>> +                                  size >> GRUB_DISK_SECTOR_BITS, buf);
>> +}
>> +
>> +grub_err_t
>> +grub_raid6_recover_gen (void *data, grub_uint64_t nstripes, int disknr, int 
>> p,
>> +                        char *buf, grub_uint64_t sector, grub_size_t size,
>> +                        raid_recover_read_t read_func, int layout)
> 
> Could you change the order of read_func and layout arguments?
> I mean make read_func the last one and put the layout before it.
OK
> 
>>  {
>>    int i, q, pos;
>>    int bad1 = -1, bad2 = -1;
>>    char *pbuf = 0, *qbuf = 0;
>>
>> -  size <<= GRUB_DISK_SECTOR_BITS;
>>    pbuf = grub_zalloc (size);
>>    if (!pbuf)
>>      goto quit;
>> @@ -91,17 +102,17 @@ grub_raid6_recover (struct grub_diskfilter_segment 
>> *array, int disknr, int p,
>>      goto quit;
>>
>>    q = p + 1;
>> -  if (q == (int) array->node_count)
>> +  if (q == (int) nstripes)
>>      q = 0;
>>
>>    pos = q + 1;
>> -  if (pos == (int) array->node_count)
>> +  if (pos == (int) nstripes)
>>      pos = 0;
>>
>> -  for (i = 0; i < (int) array->node_count - 2; i++)
>> +  for (i = 0; i < (int) nstripes - 2; i++)
>>      {
>>        int c;
>> -      if (array->layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
>> +      if (layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
>>      c = pos;
>>        else
>>      c = i;
>> @@ -109,8 +120,7 @@ grub_raid6_recover (struct grub_diskfilter_segment 
>> *array, int disknr, int p,
>>          bad1 = c;
>>        else
>>          {
>> -          if (! grub_diskfilter_read_node (&array->nodes[pos], sector,
>> -                                       size >> GRUB_DISK_SECTOR_BITS, buf))
>> +      if (!read_func(data, pos, sector, buf, size))
>>              {
>>                grub_crypto_xor (pbuf, pbuf, buf, size);
>>                grub_raid_block_mulx (c, buf, size);
>> @@ -128,7 +138,7 @@ grub_raid6_recover (struct grub_diskfilter_segment 
>> *array, int disknr, int p,
>>          }
>>
>>        pos++;
>> -      if (pos == (int) array->node_count)
>> +      if (pos == (int) nstripes)
>>          pos = 0;
>>      }
>>
>> @@ -139,16 +149,14 @@ grub_raid6_recover (struct grub_diskfilter_segment 
>> *array, int disknr, int p,
>>    if (bad2 < 0)
>>      {
>>        /* One bad device */
>> -      if ((! grub_diskfilter_read_node (&array->nodes[p], sector,
>> -                                    size >> GRUB_DISK_SECTOR_BITS, buf)))
>> +      if (!read_func(data, p, sector, buf, size))
>>          {
>>            grub_crypto_xor (buf, buf, pbuf, size);
>>            goto quit;
>>          }
>>
>>        grub_errno = GRUB_ERR_NONE;
>> -      if (grub_diskfilter_read_node (&array->nodes[q], sector,
>> -                                 size >> GRUB_DISK_SECTOR_BITS, buf))
>> +      if (read_func(data, q, sector, buf, size))
>>          goto quit;
>>
>>        grub_crypto_xor (buf, buf, qbuf, size);
>> @@ -160,14 +168,12 @@ grub_raid6_recover (struct grub_diskfilter_segment 
>> *array, int disknr, int p,
>>        /* Two bad devices */
>>        unsigned c;
>>
>> -      if (grub_diskfilter_read_node (&array->nodes[p], sector,
>> -                                 size >> GRUB_DISK_SECTOR_BITS, buf))
>> +      if (read_func(data, p, sector, buf, size))
>>          goto quit;
>>
>>        grub_crypto_xor (pbuf, pbuf, buf, size);
>>
>> -      if (grub_diskfilter_read_node (&array->nodes[q], sector,
>> -                                 size >> GRUB_DISK_SECTOR_BITS, buf))
>> +      if (read_func(data, q, sector, buf, size))
>>          goto quit;
>>
>>        grub_crypto_xor (qbuf, qbuf, buf, size);
>> @@ -190,6 +196,17 @@ quit:
>>    return grub_errno;
>>  }
>>
>> +static grub_err_t
>> +grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int 
>> p,
>> +                    char *buf, grub_disk_addr_t sector, grub_size_t size)
>> +{
>> +
> 
> Drop this empty line.
ok
> 
>> +  return grub_raid6_recover_gen (array, array->node_count, disknr, p, buf,
>> +                                 sector, size << GRUB_DISK_SECTOR_BITS,
>> +                                 raid6_recover_read_node,
>> +                                 array->layout);
>> +}
>> +
>>  GRUB_MOD_INIT(raid6rec)
>>  {
>>    grub_raid6_init_table ();
>> diff --git a/include/grub/diskfilter.h b/include/grub/diskfilter.h
>> index d89273c1b..911888e33 100644
>> --- a/include/grub/diskfilter.h
>> +++ b/include/grub/diskfilter.h
>> @@ -189,6 +189,15 @@ typedef grub_err_t (*grub_raid6_recover_func_t) (struct 
>> grub_diskfilter_segment
>>  extern grub_raid5_recover_func_t grub_raid5_recover_func;
>>  extern grub_raid6_recover_func_t grub_raid6_recover_func;
>>
>> +typedef grub_err_t (* raid_recover_read_t)(void *data, int disk_nr,
>> +                                       grub_uint64_t addr, void *dest,
>> +                                       grub_size_t size);
>> +
>> +grub_err_t
>> +grub_raid6_recover_gen (void *data, grub_uint64_t nstripes, int disknr, int 
>> p,
>> +                        char *buf, grub_uint64_t sector, grub_size_t size,
>> +                        raid_recover_read_t read_func, int layout);
> 
> Please add extern here.
ok
> 
> Daniel
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5