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

[Daniel Kiper]

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/

> 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/

> +                                 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.

>  {
>    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.

> +  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.

Daniel