btrfs: disguise single-data-RAID56 as RAID1/RAID1C3

Recently kernel RAID56 lib is trying to remove the unexpected single-data-RAID56 (2 disks RAID5 or 3 disk RAID5) support, meanwhile btrfs still supports such setup, which means in the long run btrfs has to handle such corner case by ourselves.

Thankfully single-data-RAID56 is really RAID1/RAID1C3, since data and P/Q stripes all match each other, rotation also makes no difference.

This patch will disguise those single-data-RAID56 chunks as RAID1/RAID1C3 chunks.

Quoted 10 lines · Qu Wenruo

Recently kernel RAID56 lib is trying to remove the unexpected single-data-RAID56 (2 disks RAID5 or 3 disk RAID5) support, meanwhile btrfs still supports such setup, which means in the long run btrfs has to handle such corner case by ourselves.

Thankfully single-data-RAID56 is really RAID1/RAID1C3, since data and P/Q stripes all match each other, rotation also makes no difference.

This patch will disguise those single-data-RAID56 chunks as RAID1/RAID1C3 chunks.

I don't think this is the right way to fix it. Calculations are an implementation detail and should be done at the time of the xor_gen or raid6_call, not touching the upper level structures related to format.

Instead, please add helpers that take the same parameters and do the switch if there's the edge case of raid5 or raid6, using memcpy. Thanks.

Quoted 12 lines · David Sterba

Quoted 10 lines · Qu Wenruo

Recently kernel RAID56 lib is trying to remove the unexpected single-data-RAID56 (2 disks RAID5 or 3 disk RAID5) support, meanwhile btrfs still supports such setup, which means in the long run btrfs has to handle such corner case by ourselves.

Thankfully single-data-RAID56 is really RAID1/RAID1C3, since data and P/Q stripes all match each other, rotation also makes no difference.

This patch will disguise those single-data-RAID56 chunks as RAID1/RAID1C3 chunks.

Quoted 6 lines

I don't think this is the right way to fix it. Calculations are an implementation detail and should be done at the time of the xor_gen or raid6_call, not touching the upper level structures related to format.

Instead, please add helpers that take the same parameters and do the switch if there's the edge case of raid5 or raid6, using memcpy. Thanks.

I'd say going dedicated memcpy() is an adhoc fix, and not any easier to maintain.

Now you have two paths, one from the raid56 lib, one from the btrfs specific handling for the single-data specific corner case, and that no one else is going to review/utilize that corner case.

From this point of view, reusing the existing RAID1/RAID1C3 is way more
reliable and sane.

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:

fs/btrfs/raid56.c +0 -0

index 08ee8f316d96..50fefe33bbe3 100644
@@ -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);

fs/btrfs/raid56.c +8 -4

-		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);

fs/btrfs/raid56.c +3 -0

+			else
+				raid6_2data_recov(rbio->real_stripes, step,
+						  faila, failb, pointers);
  		}
  	} else {

void *p;

fs/btrfs/raid56.c +3 -0

@@ -2002,6 +2014,9 @@ static void recover_vertical_step(struct 
 btrfs_raid_bio *rbio,
  		/* Copy parity block into failed block to start with */
  		memcpy(pointers[faila], pointers[rbio->nr_data], step);
 
+		if (rbio->nr_data == 1)
+			goto cleanup;
+
  		/* Rearrange the pointer array */
  		p = pointers[faila];

  		for (stripe_nr = faila; stripe_nr < rbio->nr_data - 1;

fs/btrfs/raid56.c +8 -2

@@ -2644,11 +2659,17 @@ static bool verify_one_parity_step(struct 
 btrfs_raid_bio *rbio,
  	if (has_qstripe) {
  		assert_rbio(rbio);
  		/* RAID6, call the library function to fill in our P/Q. */
-		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[nr_data], pointers[0], step);
-		xor_gen(pointers[nr_data], pointers + 1, nr_data - 1, step);
+		if (rbio->nr_data > 1)
+			xor_gen(pointers[nr_data], pointers + 1, nr_data - 1, step);
  	}
 

/* Check scrubbing parity and repair it. */
--
2.54.0

Quoted 8 lines · Qu Wenruo

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.

Quoted 29 lines

fs/btrfs/raid56.c +0 -0

index 08ee8f316d96..50fefe33bbe3 100644
@@ -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);

fs/btrfs/raid56.c +8 -2

-		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,
   		}
 

fs/btrfs/raid56.c static void recover_vertical_step(struct btrfs_raid_bio *rbio, · line 2003

Quoted 13 lines

fs/btrfs/raid56.c +7 -4

   		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

Quoted 8 lines · Qu Wenruo

Recently kernel RAID56 lib is trying to remove the unexpected single-data-RAID56 (2 disks RAID5 or 3 disk RAID5) support, meanwhile btrfs still supports such setup, which means in the long run btrfs has to handle such corner case by ourselves.

Thankfully single-data-RAID56 is really RAID1/RAID1C3, since data and P/Q stripes all match each other, rotation also makes no difference.

Quoted 2 lines

This patch will disguise those single-data-RAID56 chunks as RAID1/RAID1C3 chunks.

I don't think this is the right way to fix it. Calculations are an implementation detail and should be done at the time of the xor_gen or raid6_call, not touching the upper level structures related to format.

Quoted 12 lines · David Sterba

Quoted 10 lines · Qu Wenruo

Recently kernel RAID56 lib is trying to remove the unexpected single-data-RAID56 (2 disks RAID5 or 3 disk RAID5) support, meanwhile btrfs still supports such setup, which means in the long run btrfs has to handle such corner case by ourselves.

Thankfully single-data-RAID56 is really RAID1/RAID1C3, since data and P/Q stripes all match each other, rotation also makes no difference.

This patch will disguise those single-data-RAID56 chunks as RAID1/RAID1C3 chunks.

Quoted 3 lines

I don't think this is the right way to fix it. Calculations are an implementation detail and should be done at the time of the xor_gen or raid6_call, not touching the upper level structures related to format.

It absolutely is. Adding fast path workarounds for this is completely stupid when it can be trivially handled on the mount side.

Quoted 17 lines · Christoph Hellwig

Quoted 15 lines · David Sterba

Quoted 10 lines · Qu Wenruo

Recently kernel RAID56 lib is trying to remove the unexpected single-data-RAID56 (2 disks RAID5 or 3 disk RAID5) support, meanwhile btrfs still supports such setup, which means in the long run btrfs has to handle such corner case by ourselves.

Thankfully single-data-RAID56 is really RAID1/RAID1C3, since data and P/Q stripes all match each other, rotation also makes no difference.

This patch will disguise those single-data-RAID56 chunks as RAID1/RAID1C3 chunks.

I don't think this is the right way to fix it. Calculations are an implementation detail and should be done at the time of the xor_gen or raid6_call, not touching the upper level structures related to format.

Quoted 2 lines

It absolutely is. Adding fast path workarounds for this is completely stupid when it can be trivially handled on the mount side.

It's a lazy hack at best and fix on absolutely wrong layer. The library should provide the support for the edge case. But we disagree on that.

Quoted 2 lines

It's a lazy hack at best and fix on absolutely wrong layer. The library should provide the support for the edge case. But we disagree on that.

I strongly disagree.

Firstly on the layer to fix, let me be clear, if something writes like RAID1, reads like RAID1, repairs like RAID1, then it's RAID1.

With that mindset, changing chunk type at read/add time is exactly the correct layer to fix.

Quoted 2 lines

It absolutely is. Adding fast path workarounds for this is completely stupid when it can be trivially handled on the mount side.

It's a lazy hack at best and fix on absolutely wrong layer. The library should provide the support for the edge case. But we disagree on that.

[CHANGELOG]
RFC->v1:
- Remove unused btrfs_chunk_map::io_(align|width) members To make space for the new member.

- Fix a duplicated bg->flags assignment in fill_dummy_bgs() Just exposed by the last patch which also touches that code.

- Use a dedicated member to record the on-disk chunk type So it's less fragile than the RFC patch.

The single-data-RAID56 (2 disks raid5 or 3 disks raid6) is not a feature that should be supported in the first place, thus raid56 is going to remove such support.

Meanwhile rejecting single-data-RAID56 will cause existing btrfs users unable to mount their fs, even if there is only a single empty chunk with that flag, mostly caused by some degraded mount.

So to avoid impacting existing users, implement an internal single-data-RAID56 to RAID1/RAID1C3 mapping, to co-operate with the upstream raid56 lib changes.

Qu Wenruo (3):
btrfs: remove btrfs_chunk_map::io_(align|width) members
btrfs: remove duplicated block group type assignment
btrfs: disguise single-data-RAID56 as RAID1/RAID1C3

  fs/btrfs/block-group.c |  3 +--
  fs/btrfs/volumes.c     | 25 ++++++++++++++++++-------
  fs/btrfs/volumes.h     | 10 ++++++++--
  3 files changed, 27 insertions(+), 11 deletions(-)