Re: [PATCH 8/8] Add raid6 recovery for a btrfs filesystem.

[Goffredo Baroncelli]

On 05/14/2018 09:06 PM, Daniel Kiper wrote:
> On Fri, May 11, 2018 at 09:24:46PM +0200, Goffredo Baroncelli wrote:
>> Add the raid6 recovery, in order to use a raid6 filesystem even if some
>> disks (up to two) are missing.
>> This code use the old md raid6 code already present in grub.
> 
> Please fix the commit message formating.
> 
>> Signed-off-by: Goffredo Baroncelli <address@hidden>
>> ---
>>  grub-core/fs/btrfs.c | 43 +++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 39 insertions(+), 4 deletions(-)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index 8d72607d1..07e9db910 100644
>> --- a/grub-core/fs/btrfs.c
>> +++ b/grub-core/fs/btrfs.c
>> @@ -30,6 +30,7 @@
>>  #include <grub/i18n.h>
>>  #include <grub/btrfs.h>
>>  #include <grub/crypto.h>
>> +#include <grub/diskfilter.h>
>>
>>  GRUB_MOD_LICENSE ("GPLv3+");
>>
>> @@ -696,11 +697,36 @@ rebuild_raid5 (struct raid56_buffer *buffers, 
>> grub_uint64_t nstripes,
>>                         csize);
>>  }
>>
>> +static grub_err_t
>> +raid_recover_read_raid56_buffer (void *data, int disk_nr, grub_uint64_t 
>> addr,
>> +                                 void *dest, grub_size_t size)
> 
> s/raid_recover_read_raid56_buffer/raid6_recover_read_node/
I renamed this as raid6_recover_read_buffer
It is not a node, because it is different handler than the one of patch #7


> 
>> +{
>> +    struct raid56_buffer *buffers = data;
>> +
>> +    (void)addr;
> 
> I do not like this. grub_uint64_t addr __attribute__ ((unused))" in
> prototype definition please.
> 
>> +    grub_memcpy(dest, buffers[disk_nr].buf, size);
>> +
>> +    grub_errno = buffers[disk_nr].data_is_valid ? GRUB_ERR_NONE :
>> +             GRUB_ERR_READ_ERROR;
>> +    return grub_errno;
>> +}
>> +
>> +static void
>> +rebuild_raid6 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
>> +               grub_uint64_t csize, grub_uint64_t parities_pos, void *dest,
>> +               grub_uint64_t stripen)
> 
> struct as a argument?

I thought a bit about that; however I think that the there too few place where 
grub_raid6_recover_gen() is called, so my feeling is that the effort is more
than the gain.
> 
>> +
>> +{
>> +  grub_raid6_recover_generic (buffers, nstripes, stripen, parities_pos,
>> +                              dest, 0, csize,
>> +                              raid_recover_read_raid56_buffer, 0);
>> +}
>> +
>>  static grub_err_t
>>  raid56_read_retry (struct grub_btrfs_data *data,
>>                 struct grub_btrfs_chunk_item *chunk,
>>                 grub_uint64_t stripe_offset, grub_uint64_t stripen,
>> -               grub_uint64_t csize, void *buf)
>> +               grub_uint64_t csize, void *buf, grub_uint64_t parities_pos)
> 
> struct as a argument?
> 
> 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