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

[Goffredo Baroncelli]

On 05/30/2018 01:30 PM, Daniel Kiper wrote:
> On Wed, May 16, 2018 at 08:48:17PM +0200, Goffredo Baroncelli wrote:
>> Signed-off-by: Goffredo Baroncelli <address@hidden>
>> ---
>>  grub-core/fs/btrfs.c | 174 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 170 insertions(+), 4 deletions(-)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index 63651928b..5fcaad86f 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+");
>>
>> @@ -666,6 +667,150 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>>      return err;
>>  }
>>
>> +struct raid56_buffer {
>> +  void *buf;
>> +  int  data_is_valid;
>> +};
>> +
>> +static void
>> +rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
>> +               grub_uint64_t csize)
>> +{
>> +  grub_uint64_t target = 0, i;
>> +
>> +  while (buffers[target].data_is_valid && target < nstripes)
>> +    ++target;
>> +
>> +  if (target == nstripes)
>> +    {
>> +      grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
>> OK\n");
>> +      return;
>> +    }
>> +
>> +  grub_dprintf ("btrfs", "rebuilding raid5 stripe #%" PRIuGRUB_UINT64_T 
>> "\n",
>> +            target);
>> +  for (i = 0; i < nstripes; i++)
>> +    if (i != target)
>> +      grub_crypto_xor (buffers[target].buf, buffers[target].buf, 
>> buffers[i].buf,
>> +                       csize);
>> +}
>> +
>> +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 stripen,
>> +               grub_uint64_t csize, void *buf)
>> +{
>> +
>> +  struct raid56_buffer *buffers = NULL;
>> +  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_NONE;
>> +  grub_uint64_t i, failed_devices;
>> +
>> +  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
>> +  if (!buffers)
>> +    {
>> +      ret = GRUB_ERR_OUT_OF_MEMORY;
>> +      goto cleanup;
>> +    }
>> +
>> +  for (i = 0; i < nstripes; i++)
>> +    {
>> +      buffers[i].buf = grub_zalloc (csize);
>> +      if (!buffers[i].buf)
>> +    {
>> +      ret = GRUB_ERR_OUT_OF_MEMORY;
>> +      goto cleanup;
>> +    }
>> +    }
>> +
>> +  for (i = 0; i < nstripes; i++)
>> +    {
>> +      struct grub_btrfs_chunk_stripe *stripe;
>> +      grub_disk_addr_t paddr;
>> +      grub_device_t dev;
>> +      grub_err_t err2;
>> +
>> +      stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
>> +      stripe += 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);
>> +      continue;
> 
> What will happen if more than one stripe is broken?
See below
> 
>> +    }
>> +
>> +      err2 = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
>> +                         paddr & (GRUB_DISK_SECTOR_SIZE - 1),
>> +                         csize, buffers[i].buf);
>> +      if (err2 == GRUB_ERR_NONE)
>> +    {
>> +      buffers[i].data_is_valid = 1;
>> +      grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %"
>> +                    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);
> 
> Ditto?
See below
> 
>> +    }
>> +    }
>> +
>> +  failed_devices = 0;
>> +  for (i = 0; i < nstripes; i++)
>> +    if (!buffers[i].data_is_valid)
>> +      ++failed_devices;
>> +  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
>> +    {
>> +      grub_dprintf ("btrfs",
>> +                "not enough disks for raid5: total %" PRIuGRUB_UINT64_T
> 
> s/raid5/RAID5/ Please fix this and the rest of messages in the other patches.
OK
> 
>> +                ", missing %" PRIuGRUB_UINT64_T "\n",
>> +                nstripes, failed_devices);
>> +      ret = GRUB_ERR_READ_ERROR;
>> +      goto cleanup;
> 
> Ahhh... Here it is! Perfect!
Right !
> 
>> +    }
>> +  else
>> +    {
>> +      grub_dprintf ("btrfs",
>> +                    "enough disks for raid5/6 rebuilding: total %"
> 
> s#raid5/6#RAID5# This is the patch for RAID 5. Right?
> Please do not mix RAID 5 changes with RAID 6 changes.
ok
> 
>> +                PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n",
>> +                    nstripes, failed_devices);
>> +    }
>> +
>> +  /* if these are enough, try to rebuild the data */
>> +  if (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5)
>> +    {
>> +      rebuild_raid5 (buffers, nstripes, csize);
>> +      grub_memcpy (buf, buffers[stripen].buf, csize);
> 
> Hmmm... Do we need this grub_memcpy()? Why rebuild_raid5() could
> not fill buf directly? If it is not possible then I think that
> grub_memcpy() should be called from rebuild_raid5().
I removed it
> 
>> +    }
>> +  else
>> +    {
>> +      grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
>> +    }
> 
> Please drop this curly brackets.
Ok, 
> 
>> +
>> +cleanup:
> 
> Space before the label please.
ok
> 
>> +  if (buffers)
>> +    {
>> +      for (i = 0; i < nstripes; i++)
>> +    if (buffers[i].buf)
>> +      grub_free(buffers[i].buf);
>> +      grub_free(buffers);
>> +    }
>> +
>> +  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)
>> @@ -743,6 +888,11 @@ 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;
>> +    grub_uint64_t parities_pos = 0;
>> +
>> +    is_raid56 = !!(grub_le_to_cpu64 (chunk->type) &
>> +                   GRUB_BTRFS_CHUNK_TYPE_RAID5);
> 
> OK, I would leave this as is. However, I would mention
> in the commit message that this patch also do some minor
> preparatory work for RAID 6 support.
OK
> 
>>
>>      if (grub_le_to_cpu64 (chunk->size) <= off)
>>        {
>> @@ -885,6 +1035,8 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
>> grub_disk_addr_t addr,
>>             * number of disks
>>             */
>>            grub_divmod64 (high + stripen, nstripes, &stripen);
>> +          grub_divmod64 (high + nstripes - nparities, nstripes,
>> +                         &parities_pos);
>>
>>            stripe_offset = low + chunk_stripe_length * high;
>>            csize = chunk_stripe_length - low;
>> @@ -917,15 +1069,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)
>> +          {
>> +            for (i = 0; i < redundancy; i++)
>> +              {
>> +                err = btrfs_read_from_chunk (data, chunk, stripen,
>> +                                             stripe_offset,
>> +                                             i,     /* redundancy */
>> +                                             csize, buf);
>> +                if (err == GRUB_ERR_NONE)
>> +                    break;
>> +              }
>> +          }
>> +        else
>>            {
>>              err = btrfs_read_from_chunk (data, chunk, stripen,
>>                                           stripe_offset,
>> -                                         i,     /* redundancy */
>> +                                         0,     /* no mirror */
>>                                           csize, buf);
> 
> I do not understand this change... Why?
In RAID1, RAID10 you have several disks from which you can read without further 
effort. In raid5 and 6, you must read from a specific disk, otherwise you need 
to rebuild the stripe. Is very different.
Pay attention that the diff output is very confusing, because the old code is 
shifted to left and diff think that it is a new code....
> 
>> -            if (err == GRUB_ERR_NONE)
>> -                break;
>> +            if (err != GRUB_ERR_NONE)
>> +              err = raid56_read_retry (data, chunk, stripe_offset,
>> +                                       stripen, csize, buf);
> 
> I have a feeling that this change is somehow related to my comment
> for patch #6.
IS very different, the former is "exit from the retry-from-another-mirror loop 
if the read is ok", the latter is "in case of error rebuild the stripe". In the 
first case it is possible to check the iterator index; in the second one I can 
check only the exit code.
> 
>>            }
>> +
> 
> Drop this change please.
ok
> 
>>          if (err == GRUB_ERR_NONE)
>>              break;
>>        }
> 
> Daniel
> 


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