Re: [PATCH 6/9] btrfs: refactor the code that read from disk

[Daniel Kiper]

On Wed, May 16, 2018 at 08:48:16PM +0200, Goffredo Baroncelli wrote:
> This is a preparatory patch, to help the adding of the RAID 5/6 recovery

Please move "This is a preparatory patch" sentence below...

> 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/readability/readability too/?

You can put "This is a preparatory patch" in separate line here.
Same for the other patches.

> Signed-off-by: Goffredo Baroncelli <address@hidden>
> ---
>  grub-core/fs/btrfs.c | 85 +++++++++++++++++++++++++-------------------
>  1 file changed, 49 insertions(+), 36 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 51f300829..63651928b 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;
> @@ -879,42 +919,15 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t 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", "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");
> -                 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 you drop this line then you change behavior of this function.
I have a feeling that this should stay as is. At least for now.
If you need this change then probably it should be a part of the
other patch.

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

Ditto.

Daniel