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

[Daniel Kiper]

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.

> +            *  chunk_stripe_length -> size of a stripe (typically 64k)

I assume that stripe does not cover P1 and Q1 (parity disks)?

> +            *  nstripes -> number of disks
> +            *
> +            */
> +           stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);

What is the low?

> +           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?

Daniel