grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 5/7] Refactor the code of read from disk


From: Goffredo Baroncelli
Subject: Re: [PATCH 5/7] Refactor the code of read from disk
Date: Wed, 9 May 2018 21:37:05 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0

On 05/09/2018 04:26 PM, Daniel Kiper wrote:
> On Tue, Apr 24, 2018 at 09:13:14PM +0200, Goffredo Baroncelli wrote:
>> This is a preparatory patch, to help the adding of the raid5/6 recovery code
> 
> OK, but please tell us why this patch is needed?

I will put this commit message:

  Refactor the code that read from disk

  This is a preparatory patch, to help the adding of the raid5/6 recovery code.
  In case of availability of all disks, this code is good for all the RAID
  profiles. However in case of failure, the error handling is quite different.
  Refactoring this code increases the general readability.

> 
>> Signed-off-by: Goffredo Baroncelli <address@hidden>
>> ---
>>  grub-core/fs/btrfs.c | 111 ++++++++++++++++++++++++-------------------
>>  1 file changed, 62 insertions(+), 49 deletions(-)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index d6c3adbe4..697322125 100644
>> --- a/grub-core/fs/btrfs.c
>> +++ b/grub-core/fs/btrfs.c
>> @@ -623,6 +623,47 @@ find_device (struct grub_btrfs_data *data, 
>> grub_uint64_t id)
>>    return ctx.dev_found;
>>  }
>>
>> +static grub_err_t
>> +btrfs_read_from_chunk (struct grub_btrfs_data *data,
>> +                   struct grub_btrfs_chunk_item *chunk,
>> +                   grub_uint64_t stripen, grub_uint64_t stripe_offset,
>> +                   int redundancy, grub_uint64_t csize,
>> +                   void *buf)
>> +{
>> +
>> +    struct grub_btrfs_chunk_stripe *stripe;
>> +    grub_disk_addr_t paddr;
>> +    grub_device_t dev;
>> +    grub_err_t err;
>> +
>> +    stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
>> +    /* Right now the redundancy handling is easy.
>> +       With RAID5-like it will be more difficult.  */
>> +    stripe += stripen + redundancy;
>> +
>> +    paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
>> +
>> +    grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T
>> +              " maps to 0x%" PRIxGRUB_UINT64_T "\n",
>> +              stripen, stripe->offset);
>> +    grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", 
>> paddr);
>> +
>> +    dev = find_device (data, stripe->device_id);
>> +    if (!dev)
>> +      {
>> +    grub_dprintf ("btrfs",
>> +                  "couldn't find a necessary member device "
>> +                  "of multi-device filesystem\n");
>> +    grub_errno = GRUB_ERR_NONE;
>> +    return GRUB_ERR_READ_ERROR;
>> +      }
>> +
>> +    err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
>> +                      paddr & (GRUB_DISK_SECTOR_SIZE - 1),
>> +                      csize, buf);
>> +    return err;
>> +}
>> +
>>  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)
>> @@ -636,7 +677,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
>> grub_disk_addr_t addr,
>>        grub_err_t err = 0;
>>        struct grub_btrfs_key key_out;
>>        int challoc = 0;
>> -      grub_device_t dev;
>>        struct grub_btrfs_key key_in;
>>        grub_size_t chsize;
>>        grub_disk_addr_t chaddr;
>> @@ -825,55 +865,28 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
>> grub_disk_addr_t addr,
>>      if (csize > (grub_uint64_t) size)
>>        csize = size;
>>
>> -    for (j = 0; j < 2; j++)
>> +    err = GRUB_ERR_NONE + 1;
>> +
>> +    for (j = 0; j < 2 && err != GRUB_ERR_NONE; j++)
>>        {
>> -        for (i = 0; i < redundancy; i++)
>> -          {
>> -            struct grub_btrfs_chunk_stripe *stripe;
>> -            grub_disk_addr_t paddr;
>> -
>> -            stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
>> -            /* Right now the redundancy handling is easy.
>> -               With RAID5-like it will be more difficult.  */
>> -            stripe += stripen + i;
>> -
>> -            paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
>> -
>> -            grub_dprintf ("btrfs", "chunk 0x%" PRIxGRUB_UINT64_T
>> -                          "+0x%" PRIxGRUB_UINT64_T
>> -                          " (%d stripes (%d substripes) of %"
>> -                          PRIxGRUB_UINT64_T ") stripe %" PRIxGRUB_UINT64_T
>> -                          " maps to 0x%" PRIxGRUB_UINT64_T "\n",
>> -                          grub_le_to_cpu64 (key->offset),
>> -                          grub_le_to_cpu64 (chunk->size),
>> -                          grub_le_to_cpu16 (chunk->nstripes),
>> -                          grub_le_to_cpu16 (chunk->nsubstripes),
>> -                          grub_le_to_cpu64 (chunk->stripe_length),
>> -                          stripen, stripe->offset);
>> -            grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T
>> -                          " for laddr 0x%" PRIxGRUB_UINT64_T "\n", paddr,
>> -                          addr);
>> -
>> -            dev = find_device (data, stripe->device_id);
>> -            if (!dev)
>> -              {
>> -                grub_dprintf ("btrfs",
>> -                              "couldn't find a necessary member device "
>> -                              "of multi-device filesystem\n");
>> -                err = grub_errno;
>> -                grub_errno = GRUB_ERR_NONE;
>> -                continue;
>> -              }
>> -
>> -            err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
>> -                                  paddr & (GRUB_DISK_SECTOR_SIZE - 1),
>> -                                  csize, buf);
>> -            if (!err)
>> -              break;
>> -            grub_errno = GRUB_ERR_NONE;
>> -          }
>> -        if (i != redundancy)
>> -          break;
>> +        grub_dprintf ("btrfs", "chunk 0x%" PRIxGRUB_UINT64_T
>> +                      "+0x%" PRIxGRUB_UINT64_T
>> +                      " (%d stripes (%d substripes) of %"
>> +                      PRIxGRUB_UINT64_T ") \n",
>> +                      grub_le_to_cpu64 (key->offset),
>> +                      grub_le_to_cpu64 (chunk->size),
>> +                      grub_le_to_cpu16 (chunk->nstripes),
>> +                      grub_le_to_cpu16 (chunk->nsubstripes),
>> +                      grub_le_to_cpu64 (chunk->stripe_length));
>> +        grub_dprintf ("btrfs", "reading laddr 0x%" PRIxGRUB_UINT64_T "\n",
>> +                      addr);
>> +
>> +        err = GRUB_ERR_NONE + 1;
>> +        for (i = 0; i < redundancy && err != GRUB_ERR_NONE; i++)
>> +             err = btrfs_read_from_chunk (data, chunk, stripen,
>> +                                          stripe_offset,
>> +                                          i,     /* redundancy */
>> +                                          csize, buf);
> 
> It seems to me that this patch does more than it is told in commit message.
> At least it plays some games with debug messages? Could you split this
> patch into logical parts or if it is not possible could you say in the
> commit message why are you doing this and that?

I had some trouble in understanding the original code, because the mixing of
the "retry" logic and the "reading logic". So I refactored the "reading logic",
which helped also to implement the retry logic for the RAID5/6.

The log is the same, however the caller and the called doesn't have the
same information (the caller has access to the "key" variable, and the
called has access to the "stripe" variable). So I split the log depending
by the information available.

> 
> Daniel
> 


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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]