Re: [PATCH 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile.

[Daniel Kiper]

On Wed, Sep 19, 2018 at 08:40:32PM +0200, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli <address@hidden>
>
> Signed-off-by: Goffredo Baroncelli <address@hidden>
> ---
>  grub-core/fs/btrfs.c | 66 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index be195448d..56c42746d 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,70 @@ 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, block_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 of several blocks spread on the disks.
> +            * The raid terminology is used to call all the blocks of a row
> +            * "stripe". Unfortunately the BTRFS terminology confuses block

Stripe is data set or parity (parity stripe) on one disk. Block has
different meaning. Please stick to btrfs terminology and say it clearly
in the comment. And even add a link to btrfs wiki page to ease reading.

I think about this one:
  
https://btrfs.wiki.kernel.org/index.php/Manpage/mkfs.btrfs#BLOCK_GROUPS.2C_CHUNKS.2C_RAID

> +            * and stripe.

I do not think so. Or at least not so much...

> +            *
> +            *   Disk0  Disk1  Disk2  Disk3
> +            *
> +            *    A1     B1     P1     Q1
> +            *    Q2     A2     B2     P2
> +            *    P3     Q3     A3     B3
> +            *  [...]
> +            *
> +            *  Note that the placement of the parities depends on row index.
> +            *  In the code below:
> +            *  - block_nr is the block number without the parities

Well, it seems to me that the btrfs code introduces confusion not the
spec itself. I would leave code as is but s/block number/stripe number/.

> +            *    (A1 = 0, B1 = 1, A2 = 2, B2 = 3, ...),
> +            *  - high is the row number (0 for A1...Q1, 1 for Q2...P2, ...),
> +            *  - stripen is the disk number (0 for A1,Q2,P3, 1 for B1...),

s/disk number/disk number in a row/

> +            *  - off is the logical address to read
> +            *  - chunk_stripe_length is the size of a block (typically 64k),

s/a block/a stripe/

> +            *  - nstripes is the number of disks,

s/number of disks/number of disks in a row/

I miss the description of nparities here...

> +            *  - low is the offset of the data inside a stripe,
> +            *  - stripe_offset is the disk offset,

s/the disk offset/the data offset in an array/?

> +            *  - csize is the "potential" data to read. It will be reduced to
> +            *    size if the latter is smaller.
> +            */
> +           block_nr = grub_divmod64 (off, chunk_stripe_length, &low);
> +
> +           /*
> +            * stripen is computed without the parities (0 for A1, A2, A3...
> +            * 1 for B1, B2...).
> +            */
> +           high = grub_divmod64 (block_nr, nstripes - nparities, &stripen);

This is clear...

> +           /*
> +            * stripen is recomputed considering the parities (0 for A1, 1 for
> +            * A2, 2 for A3....).
> +            */
> +           grub_divmod64 (high + stripen, nstripes, &stripen);

... but this looks strange... You add disk number to row number. Hmmm...
It looks that it works but this is not obvious at first sight. Could you
explain that?

> +           stripe_offset = low + chunk_stripe_length * high;
> +           csize = chunk_stripe_length - low;
> +
>             break;
>           }
>         default:

Daniel