Re: [PATCH 1/7] Add support for reading a filesystem with a raid5 or raid6 profile.

[Goffredo Baroncelli]

On 05/09/2018 03:46 PM, Daniel Kiper wrote:
> On Tue, Apr 24, 2018 at 09:13:10PM +0200, Goffredo Baroncelli wrote:
>> Signed-off-by: Goffredo Baroncelli <address@hidden>
>> ---
>>  grub-core/fs/btrfs.c | 32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index be195448d..b0032ea46 100644
>> --- a/grub-core/fs/btrfs.c
>> +++ b/grub-core/fs/btrfs.c
>> @@ -119,6 +119,8 @@ struct grub_btrfs_chunk_item
>>  #define GRUB_BTRFS_CHUNK_TYPE_RAID1         0x10
>>  #define GRUB_BTRFS_CHUNK_TYPE_DUPLICATED    0x20
>>  #define GRUB_BTRFS_CHUNK_TYPE_RAID10        0x40
>> +#define GRUB_BTRFS_CHUNK_TYPE_RAID5         0x80
>> +#define GRUB_BTRFS_CHUNK_TYPE_RAID6        0x100
> 
> #define GRUB_BTRFS_CHUNK_TYPE_RAID5         0x80
> #define GRUB_BTRFS_CHUNK_TYPE_RAID6         0x100
>  Please start in one column...     ---->    ^^^^^
OK
> 
>>    grub_uint8_t dummy2[0xc];
>>    grub_uint16_t nstripes;
>>    grub_uint16_t nsubstripes;
>> @@ -764,6 +766,36 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
>> grub_disk_addr_t addr,
>>            stripe_offset = low + chunk_stripe_length
>>              * high;
>>            csize = chunk_stripe_length - low;
>> +          break;
>> +        }
>> +      case GRUB_BTRFS_CHUNK_TYPE_RAID5:
>> +      case GRUB_BTRFS_CHUNK_TYPE_RAID6:
>> +        {
>> +          grub_uint64_t nparities;
>> +          grub_uint64_t stripe_nr, high;
>> +          grub_uint64_t low;
> 
> Could you define all in one line?
Ok
> 
>> +          redundancy = 1;   /* no redundancy for now */
>> +
>> +          if (grub_le_to_cpu64 (chunk->type) & GRUB_BTRFS_CHUNK_TYPE_RAID5)
>> +            {
>> +              grub_dprintf ("btrfs", "RAID5\n");
>> +              nparities = 1;
>> +            }
>> +          else
>> +            {
>> +              grub_dprintf ("btrfs", "RAID6\n");
>> +              nparities = 2;
>> +            }
>> +
>> +          stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);
> 
> This is clear for me...
I want to put this comment before the line above
              /*
               * A raid 6 layout consists in several stripes spread
               * on the disks, following a layout like the one below
               *
               *   Disk1  Disk2  Disk3  Ddisk4
               *
               *    A1     B1     P1     Q1
               *    Q2     A2     B2     P2  
               *    P3     Q3     A3     B3
               *  [...]
               *
               *  Note that the placement of the parities depends by the row
               *  In the code below:
               *  stripe_nr -> is the stripe number not considering the parities
               *               (A1=0, B1=1, A2 = 2, B2 = 3, ...)
               *  high -> is the row number (0 for A1...Q1, 1 for Q2..P2, ...)
               *  stripen -> is the column number (or number of disk)
               *  off -> logical address to read (from teh beginning of the 
               *         chunk space)
               *  chunk_stripe_length -> size of a stripe (typically 64k)
               *  nstripes -> number of disks
               * 
               */

> 
>> +          high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);
then I want to put here

              /*
               * until now stripen is evaluated without the parities (0 for A1,
               * A2, A3... 1 for B1, B2...); now consider also the parities (0
               * for A1, 1 for A2, 2 for A3....); the math is done "modulo"
               * number of disk
               */


>> +          grub_divmod64 (high + stripen, nstripes, &stripen);
> 
> ... but not these two. Could you enlighten me?

It is more clear ? To understand this I spent a lot of time dumping the raw 
data on the disks; this because in wikipedia there is another stripe layout 
(A1, B1, B2 always on the first column; however btrfs does this different)
>> +          stripe_offset = low + chunk_stripe_length * high;
> 
> ???
This is like the other RAID profile: high is the "row number" of the stripe 
matrix; so stripe_offset is the start of the data from the begin of the chunk 
(on the disk)
> 
>> +          csize = chunk_stripe_length - low;
> 
> Chunk size to read?
This is like the other code. We can read until the end of the stripe. Then the 
disks will change.
> 
> Daniel
> 


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