Re: [PATCH 3/3] btrfs: disguise single-data-RAID56 as RAID1/RAID1C3

[Qu Wenruo <wqu@suse.com>]

在 2026/5/25 19:42, David Sterba 写道:
> On Sun, May 24, 2026 at 01:43:54PM +0930, Qu Wenruo wrote:
>> I tried the idea, and it doesn't look elegant at all, we need a lot of
>> if () checks around every xor_gen()/raid6_call.
>>
>> Sure the end result is not that huge (and I have only compile tested
>> it), but if we had some parameters passed wrongly, or some corner case
>> missed, it can be very hard to notice.
>>
>> Just paste the adhoc fix for reference:
> 
> I ended up with something similar, but replaced the open coded ifs with
> a helper so there's not that much churn at the call sites.
> 
>> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
>> index 08ee8f316d96..50fefe33bbe3 100644
>> --- a/fs/btrfs/raid56.c
>> +++ b/fs/btrfs/raid56.c
>> @@ -1410,11 +1410,17 @@ static void generate_pq_vertical_step(struct
>> btrfs_raid_bio *rbio, unsigned int
>>    				rbio_qstripe_paddr(rbio, sector_nr, step_nr));
>>
>>    		assert_rbio(rbio);
>> -		raid6_call.gen_syndrome(rbio->real_stripes, step, pointers);
>> +		if (rbio->nr_data > 1) {
>> +			raid6_call.gen_syndrome(rbio->real_stripes, step, pointers);
>> +		} else {
>> +			memcpy(pointers[1], pointers[0], step);
>> +			memcpy(pointers[2], pointers[0], step);
>> +		}
>>    	} else {
>>    		/* raid5 */
>>    		memcpy(pointers[rbio->nr_data], pointers[0], step);
>> -		xor_gen(pointers[rbio->nr_data], pointers + 1, rbio->nr_data - 1,
>> +		if (rbio->nr_data > 1)
>> +			xor_gen(pointers[rbio->nr_data], pointers + 1, rbio->nr_data - 1,
>>    				step);
>>    	}
>>    	for (stripe = stripe - 1; stripe >= 0; stripe--)
>> @@ -1987,11 +1993,17 @@ static void recover_vertical_step(struct
>> btrfs_raid_bio *rbio,
>>    		}
>>
>>    		if (failb == rbio->real_stripes - 2) {
>> -			raid6_datap_recov(rbio->real_stripes, step,
>> -					  faila, pointers);
>> +			if (unlikely(rbio->nr_data == 1))
>> +				memcpy(pointers[0], pointers[rbio->real_stripes - 1], step);
>> +			else
>> +				raid6_datap_recov(rbio->real_stripes, step,
>> +						  faila, pointers);
>>    		} else {
>> -			raid6_2data_recov(rbio->real_stripes, step,
>> -					  faila, failb, pointers);
>> +			if (unlikely(rbio->nr_data == 1))
>> +				memcpy(pointers[0], pointers[rbio->real_stripes - 2], step);
> 
> The 2data recovery needs to do 2 memcpy

Nope. The recovery is only to repair the data stripe.
We do not need to bother the failed P or Q stripe here.

And I think this explains exactly why doing adhoc fix is a stupid idea 
in the first place.

We're wasting time on useless corner cases, and corner cases inside 
corner cases.
It makes no sense to waste all the time reviewing those useless details, 
when we have a much better solution to avoid all those hassles.