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

[Daniel Kiper]

On Wed, Oct 17, 2018 at 09:03:58PM +0200, Goffredo Baroncelli wrote:
> On 17/10/2018 16.14, Daniel Kiper wrote:
> > On Thu, Oct 11, 2018 at 08:51:01PM +0200, Goffredo Baroncelli wrote:
> >> From: Goffredo Baroncelli <address@hidden>
> >>
> >> Add support for recovery for a RAID 5 btrfs profile. In addition
> >> it is added some code as preparatory work for RAID 6 recovery code.
> >>
> >> Signed-off-by: Goffredo Baroncelli <address@hidden>
> >> ---
> [...]
>
> >> +
> >> +  for (failed_devices = 0, i = 0; i < nstripes; i++)
> >> +    {
> >> +      struct grub_btrfs_chunk_stripe *stripe;
> >> +      grub_disk_addr_t paddr;
> >> +      grub_device_t dev;
> >> +      grub_err_t err;
> >> +
> >> +      /* after the struct grub_btrfs_chunk_item, there is an array of
> >> +         struct grub_btrfs_chunk_stripe */
> >
> > /* Struct grub_btrfs_chunk_stripe lives behind struct 
> > grub_btrfs_chunk_item. */
>
> What about
>
> /* The struct grub_btrfs_chunk_stripe array lives behind struct 
> grub_btrfs_chunk_item. */

Works for me.

> [...]
>
> >> @@ -921,17 +1061,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)
> >
> > Why not "if (is_raid56)"? I asked about that earlier. Please change
> > this if and of course code below. It will be much easier to read. And
> > you do not need curly brackets for for loop after else.
>
> Frankly speaking I don't see any problem having a if (!...). However I
> update the code as your request, hoping to speedup this patch set

OK, it works. However, if you have "else" below then I think that it is
more natural to drop "!" here. If you would not have else I would not
complain. Well, because it would not make sense to do so... :-)))

Daniel