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

[Goffredo Baroncelli]

On 05/14/2018 07:52 PM, Daniel Kiper wrote:
> On Fri, May 11, 2018 at 09:24:39PM +0200, Goffredo Baroncelli wrote:
>> Signed-off-by: Goffredo Baroncelli <address@hidden>
>> ---
>>  grub-core/fs/btrfs.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 61 insertions(+)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index be195448d..7e287d0ec 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
>>    grub_uint8_t dummy2[0xc];
>>    grub_uint16_t nstripes;
>>    grub_uint16_t nsubstripes;
>> @@ -764,6 +766,65 @@ 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, stripe_nr, high, low;
>> +
>> +          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;
>> +            }
>> +
>> +          /*
>> +           * A raid 6 layout consists in several stripes spread
> 
> This line is confusing if you compare it with both cases a few lines above.
> I think that you should explain somewhere why this math works for RAID 5
> and RAID 6.
> 
>> +           * 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
> 
> s/by the/on/?
> 
>> +           *  index;
>> +           *  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)
> 
> s/number of disk/number of disk in row/?
> 
>> +           *  off -> logical address to read (from teh beginning of the
> 
> What is "teh"? nth? And it seems to me that off is the offset from the
> beginning of the array.
> 
>> +           *         chunk space)
> 
> I am not sure what you mean by "chunk space" here.

In BTRFS the logical space where are stored the data and the metadata is 
divided in chunks (or block group). Each chunk maps its "logical space" to the
disk(s) space; how this "logical space" is arranged on the disk(s) depends by
the chunk profile. Eg:
- RAID1: the logical space is mapped on two disks, where each disk is the mirror
  of the other.
- RAID5: the logical space is mapped on (n-1) disks, and the Nth disk is used
  as parity (where n is the number of available disks).

Each chunks maps a logical space that has a size of few GB. This means that
a btrfs filesystem has several chunks. And each chunks may have different 
profiles.
So it is possible that inside a btrfs filesystem there are area where a RAID1 
profile is applied, other where a RAID5 is applied and so on...


> 
>> +           *  chunk_stripe_length -> size of a stripe (typically 64k)
> 
> I assume that stripe does not cover P1 and Q1 (parity disks)?

a stripe is a slice of a "row" (e.g. A1, B2, Q3...)
> 
>> +           *  nstripes -> number of disks
>> +           *
>> +           */
>> +          stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);
> 
> What is the low?

"low", is the offset where the data is placed inside a "stripe"


> 
>> +          high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);
>> +
>> +          /*
>> +           * 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
> 
> May I ask you to use proper English sentences in the comments?

I rearranged the comments; see V4 patches set.

> 
> Daniel
> 


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