Re: [PATCH 5/8] Refactor the code that read from disk

[Daniel Kiper]

It looks that patch 4 is missing in this series.

On Fri, May 11, 2018 at 09:24:43PM +0200, Goffredo Baroncelli wrote:
> 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.

s/raid/RAID/ in all patches please. And use "RAID 5" and "RAID 6" (with
space between RAID and number) respectively.

> Signed-off-by: Goffredo Baroncelli <address@hidden>
> ---
>  grub-core/fs/btrfs.c | 106 +++++++++++++++++++++++++------------------
>  1 file changed, 61 insertions(+), 45 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 32c4382e5..fc4198e39 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -625,6 +625,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)
> @@ -638,7 +679,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;
> @@ -858,53 +898,29 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>
>       for (j = 0; j < 2; j++)
>         {
> +         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);
> +
>           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;
> +             err = btrfs_read_from_chunk (data, chunk, stripen,
> +                                          stripe_offset,
> +                                          i,     /* redundancy */
> +                                          csize, buf);
> +             if (err == GRUB_ERR_NONE)
> +                 break;
>             }
> -         if (i != redundancy)
> -           break;
> +         if (err == GRUB_ERR_NONE)
> +             break;
>         }
>       if (err)
>         return grub_errno = err;

One logic change per patch please. First put the code in the function
then change the log message or vice versa. This is important to ease
bisection later.

Daniel