Re: [PATCH 9/9] btrfs: Add RAID 6 recovery for a btrfs filesystem.

[Daniel Kiper <dkiper@net-space.pl>]

On Wed, Sep 26, 2018 at 09:56:07PM +0200, Goffredo Baroncelli wrote:
> On 25/09/2018 21.20, Daniel Kiper wrote:
> > On Wed, Sep 19, 2018 at 08:40:40PM +0200, Goffredo Baroncelli wrote:
> >> From: Goffredo Baroncelli <kreijack@inwind.it>
> >>
> [....]
> >>  	       *  - stripe_offset is the disk offset,
> >>  	       *  - csize is the "potential" data to read. It will be reduced to
> >>  	       *    size if the latter is smaller.
> >> +	       *  - parities_pos is the position of the parity inside a row (
> >
> > s/inside/in/>
> >> +	       *    2 for P1, 3 for P2...)
>
> +              *  - nparities is the number of parities (1 for RAID5, 2 for RAID6);
> +              *    used only in RAID5/6 code.
>
> >>  	       */
> >>  	      block_nr = grub_divmod64 (off, chunk_stripe_length, &low);
> >>
> >> @@ -1030,6 +1069,9 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
> >>  	       */
> >>  	      grub_divmod64 (high + stripen, nstripes, &stripen);
> >>
> >> +	      grub_divmod64 (high + nstripes - nparities, nstripes,
> >> +			     &parities_pos);
> >
> > I think that this math requires a bit of explanation in the comment
> > before grub_divmod64(). Especially I am interested in why high +
> > nstripes - nparities works as expected.
>
>
> What about
>
> /*
>  * parities_pos is equal to "(high - nparities) % nstripes" (see the diagram above).
>  * However "high - nparities" might be negative (eg when high == 0) leading to an
>  * incorrect computation.
>  * Instead "high + nstripes - nparities" is always positive and in modulo nstripes is
>  * equal to "(high - nparities) % nstripes
>  */

LGTM.

Daniel