Re: [PATCH] btrfs-progs: raid56: fix the wrong recovery condition for data and P case

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: dsterba@suse.cz, Qu Wenruo <wqu@suse.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH] btrfs-progs: raid56: fix the wrong recovery condition for data and P case
Date: Wed, 17 Nov 2021 07:09:37 +0800	[thread overview]
Message-ID: <4dbad01d-00e7-882c-6d91-c0351514df71@gmx.com> (raw)
In-Reply-To: <20211116135222.GP28560@twin.jikos.cz>

On 2021/11/16 21:52, David Sterba wrote:
> On Tue, Nov 16, 2021 at 09:10:51PM +0800, Qu Wenruo wrote:
>> There is a bug in raid56_recov() which doesn't properly repair data and
>> P case corruption:
>>
>> 	/* Data and P*/
>> 	if (dest2 == nr_devs - 1)
>> 		return raid6_recov_datap(nr_devs, stripe_len, dest1, data);
>>
>> Note that, dest1/2 is to indicate which slot has corruption.
>>
>> For RAID6 cases:
>>
>> [0, nr_devs - 2) is for data stripes,
>> @data_devs - 2 is for P,
>> @data_devs - 1 is for Q.
>>
>> For above code, the comment is correct, but the check condition is
>> wrong, and leads to the only project, btrfs-fuse, to report raid6
>> recovery error for 2 devices missing case.
>>
>> Fix it by using correct condition.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> But I'm more interested in why this function is still there, as there
>> seems to be no caller of this function in btrfs-progs anyway.
>
> The file is there from old times when the radi56 implementation landed
> and the file was a copy of something in the lib/raid6 directory, but the
> sources have diverged.
>
> The function was not used as there was no repair code in userspace, so
> the question is if w still want it there or remove it.
>
But then the problem is, how could userspace doesn't have any RAID56
recovery mechanism?

Things like btrfs check still needs RAID56 recovery to read fs with
missing devices, this doesn't make any sense to me.

The only good news is now we have another project which would do a full
coverage test for btrfs-progs...

Thanks,
Qu