Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

On Thu, Oct 11, 2018 at 08:51:01PM +0200, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli <address@hidden>
>
> Add support for recovery for a RAID 5 btrfs profile. In addition
> it is added some code as preparatory work for RAID 6 recovery code.
>
> Signed-off-by: Goffredo Baroncelli <address@hidden>
> ---
>  grub-core/fs/btrfs.c | 162 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 157 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 899dc32b7..d066d54cc 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -29,6 +29,7 @@
>  #include <minilzo.h>
>  #include <grub/i18n.h>
>  #include <grub/btrfs.h>
> +#include <grub/crypto.h>
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -665,6 +666,141 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>      return err;
>  }
>
> +struct raid56_buffer {
> +  void *buf;
> +  int  data_is_valid;
> +};
> +
> +static void
> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
> +            grub_uint64_t nstripes, grub_uint64_t csize)
> +{
> +  grub_uint64_t i;
> +  int first;
> +
> +  for(i = 0; buffers[i].data_is_valid && i < nstripes; i++);
> +
> +  if (i == nstripes)
> +    {
> +      grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
> OK\n");
> +      return;
> +    }
> +
> +  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T 
> "\n", i);
> +
> +  for (i = 0, first = 1; i < nstripes; i++)
> +    {
> +      if (!buffers[i].data_is_valid)
> +     continue;
> +
> +      if (first) {
> +     grub_memcpy(dest, buffers[i].buf, csize);
> +     first = 0;
> +      } else
> +     grub_crypto_xor (dest, dest, buffers[i].buf, csize);
> +

Please drop this empty line.

> +    }
> +}
> +
> +static grub_err_t
> +raid56_read_retry (struct grub_btrfs_data *data,
> +                struct grub_btrfs_chunk_item *chunk,
> +                grub_uint64_t stripe_offset,
> +                grub_uint64_t csize, void *buf)
> +{
> +  struct raid56_buffer *buffers;
> +  grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes);
> +  grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type);
> +  grub_err_t ret = GRUB_ERR_OUT_OF_MEMORY;
> +  grub_uint64_t i, failed_devices;
> +
> +  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
> +  if (!buffers)
> +    goto cleanup;
> +
> +  for (i = 0; i < nstripes; i++)
> +    {
> +      buffers[i].buf = grub_zalloc (csize);
> +      if (!buffers[i].buf)
> +     goto cleanup;
> +    }
> +
> +  for (failed_devices = 0, i = 0; i < nstripes; i++)
> +    {
> +      struct grub_btrfs_chunk_stripe *stripe;
> +      grub_disk_addr_t paddr;
> +      grub_device_t dev;
> +      grub_err_t err;
> +
> +      /* after the struct grub_btrfs_chunk_item, there is an array of
> +         struct grub_btrfs_chunk_stripe */

/* Struct grub_btrfs_chunk_stripe lives behind struct grub_btrfs_chunk_item. */

> +      stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1) + i;
> +
> +      paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> +      grub_dprintf ("btrfs", "reading paddr %" PRIxGRUB_UINT64_T
> +                    " from stripe ID %" PRIxGRUB_UINT64_T "\n", paddr,
> +                    stripe->device_id);
> +
> +      dev = find_device (data, stripe->device_id);
> +      if (!dev)
> +     {
> +       buffers[i].data_is_valid = 0;
> +       grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID 
> %"
> +                     PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
> +       failed_devices++;
> +       continue;
> +     }
> +
> +      err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> +                         paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> +                         csize, buffers[i].buf);
> +      if (err == GRUB_ERR_NONE)
> +     {
> +       buffers[i].data_is_valid = 1;
> +       grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %"

s/Ok/OK/

> +                     PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
> +     }
> +      else
> +     {
> +       buffers[i].data_is_valid = 0;
> +       grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T
> +                     " FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i,
> +                     stripe->device_id);
> +       failed_devices++;
> +     }
> +    }
> +
> +  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
> +    {
> +      grub_dprintf ("btrfs",
> +                 "not enough disks for RAID 5: total %" PRIuGRUB_UINT64_T
> +                 ", missing %" PRIuGRUB_UINT64_T "\n",
> +                 nstripes, failed_devices);
> +      ret = GRUB_ERR_READ_ERROR;
> +      goto cleanup;
> +    }
> +  else
> +    grub_dprintf ("btrfs",
> +               "enough disks for RAID 5 rebuilding: total %"

s/enough disks for RAID 5 rebuilding/enough disks for RAID 5/

> +               PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n",
> +               nstripes, failed_devices);
> +
> +  /* if these are enough, try to rebuild the data */

/* We have enough disks. So, rebuild the data. */

> +  if (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5)
> +    rebuild_raid5 (buf, buffers, nstripes, csize);
> +  else
> +    grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
> +
> +  ret = GRUB_ERR_NONE;
> + cleanup:
> +  if (buffers)
> +    for (i = 0; i < nstripes; i++)
> +     grub_free(buffers[i].buf);

Lack of space between function name and "(".

> +  grub_free(buffers);

Ditto.

> +  return ret;
> +}
> +
>  static grub_err_t
>  grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>                        void *buf, grub_size_t size, int recursion_depth)
> @@ -742,6 +878,10 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>       grub_uint16_t nstripes;
>       unsigned redundancy = 1;
>       unsigned i, j;
> +     int is_raid56;
> +
> +     is_raid56 = !!(grub_le_to_cpu64 (chunk->type) &
> +                    GRUB_BTRFS_CHUNK_TYPE_RAID5);
>
>       if (grub_le_to_cpu64 (chunk->size) <= off)
>         {
> @@ -921,17 +1061,29 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>           grub_dprintf ("btrfs", "reading laddr 0x%" PRIxGRUB_UINT64_T "\n",
>                         addr);
>
> -         for (i = 0; i < redundancy; i++)
> +         if (!is_raid56)

Why not "if (is_raid56)"? I asked about that earlier. Please change
this if and of course code below. It will be much easier to read. And
you do not need curly brackets for for loop after else.

> +           for (i = 0; i < redundancy; i++)
> +             {
> +               err = btrfs_read_from_chunk (data, chunk, stripen,
> +                                            stripe_offset,
> +                                            i,     /* redundancy */
> +                                            csize, buf);
> +               if (!err)
> +                 break;
> +               grub_errno = GRUB_ERR_NONE;
> +             }
> +         else
>             {
>               err = btrfs_read_from_chunk (data, chunk, stripen,
>                                            stripe_offset,
> -                                          i,     /* redundancy */
> +                                          0,     /* no mirror */
>                                            csize, buf);
> -             if (!err)
> -               break;
>               grub_errno = GRUB_ERR_NONE;
> +             if (err)
> +               err = raid56_read_retry (data, chunk, stripe_offset,
> +                                        csize, buf);
>             }
> -         if (i != redundancy)
> +         if (!err)
>             break;
>         }
>       if (err)

Daniel