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 <kreij...@inwind.it>
> ---
> 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
