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

[Goffredo Baroncelli]

On 09/10/2018 20.20, Daniel Kiper wrote:
> On Thu, Sep 27, 2018 at 08:35:02PM +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>
>> ---
>>  grub-core/fs/btrfs.c | 160 +++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 155 insertions(+), 5 deletions(-)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index 554f350c5..db8df0eea 100644
>> --- a/grub-core/fs/btrfs.c
>> +++ b/grub-core/fs/btrfs.c
>> @@ -29,6 +29,7 @@
[...]

>> +
>> +      stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1) + i;
> 
> I think that "chunk + 1" requires short comment why...

This is a quite common pattern (a struct followed by an array of struct). 
However I added a short comment like

      /* after the struct grub_btrfs_chunk_item, there is an array of 
         struct grub_btrfs_chunk_stripe */
      stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1) + i;


[...]
>> -        for (i = 0; i < redundancy; i++)
>> +        if (!is_raid56)
> 
> Why not "if (is_raid56)"? This looks more natural here.

I think that it was due to the fact than before it was only the not "raid5/6" 
profiles.
Then it was added the "raid5/6" profiles, so I added this case after.

What I dislike is not the order, but the fact that in one case (not raid5), the 
same function is called several time until it found valid data. In the other 
case, it is called only the first time, then a completely different path is 
called....
I am trying a different solution, but in any case the result is a bit ugly....


> 
>> +          for (i = 0; i < redundancy; i++)
>> +            {
>> +              err = btrfs_read_from_chunk (data, chunk, stripen,
>> +                                           stripe_offset,
>> +                                           i,     /* redundancy */
>> +                                           csize, buf);
>> +              if (!err)
>> +                break;
>> +              grub_errno = GRUB_ERR_NONE;
>> +            }
>> +        else
>>            {
>>              err = btrfs_read_from_chunk (data, chunk, stripen,
>>                                           stripe_offset,
>> -                                         i,     /* redundancy */
>> +                                         0,     /* no mirror */
>>                                           csize, buf);
>> -            if (!err)
>> -              break;
>>              grub_errno = GRUB_ERR_NONE;
>> +            if (err != GRUB_ERR_NONE)
> 
> Please be consistent and use "if (err)" here.
ok
> 
>> +              err = raid56_read_retry (data, chunk, stripe_offset,
>> +                                       csize, buf);
>>            }
>> -        if (i != redundancy)
>> +        if (err == GRUB_ERR_NONE)
> 
> if (!err) please...
ok
> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 


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