btrfs: raid56: always verify the P/Q contents for scrub

Commit Message

Qu Wenruo June 30, 2023, 12:56 a.m. UTC

[REGRESSION]
Commit 75b470332965 ("btrfs: raid56: migrate recovery and scrub recovery
path to use error_bitmap") changed the behavior of scrub_rbio().

Initially if we have no error reading the raid bio, we will assign
@need_check to true, then finish_parity_scrub() would later verify the
content of P/Q stripes before writeback.

But after that commit we never verify the content of P/Q stripes and
just writeback them.

This can lead to unrepaired P/Q stripes during scrub, or already
corrupted P/Q copied to the dev-replace target.

[FIX]
The situation is more complex than the regression, in fact the initial
behavior is not 100% correct either.

If we have the following rare case, it can still lead to the same
problem using the old behavior:

		0	16K	32K	48K	64K
	Data 1:	|IIIIIII|                       |
	Data 2:	|				|
	Parity:	|	|CCCCCCC|		|

Where "I" means IO error, "C" means corruption.

In the above case, we're scrubbing the parity stripe, then read out all
the contents of Data 1, Data 2, Parity stripes.

But found IO error in Data 1, which leads to rebuild using Data 2 and
Parity and got the correct data.

In that case, we would not verify if the Pairty is correct for range
[16K, 32K).

So here we have to always verify the content of Parity no matter if we
did recovery or not.

This patch would remove the @need_check parameter of
finish_parity_scrub() completely, and would always do the P/Q
verification before writeback.

Fixes: 75b470332965 ("btrfs: raid56: migrate recovery and scrub recovery path to use error_bitmap")
Signed-off-by: Qu Wenruo <[e-mail]>
---
 fs/btrfs/raid56.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Comments

David Sterba July 11, 2023, 8:41 p.m. UTC | #1

On Fri, Jun 30, 2023 at 08:56:40AM +0800, Qu Wenruo wrote:
> [REGRESSION]
> Commit 75b470332965 ("btrfs: raid56: migrate recovery and scrub recovery
> path to use error_bitmap") changed the behavior of scrub_rbio().
> 
> Initially if we have no error reading the raid bio, we will assign
> @need_check to true, then finish_parity_scrub() would later verify the
> content of P/Q stripes before writeback.
> 
> But after that commit we never verify the content of P/Q stripes and
> just writeback them.
> 
> This can lead to unrepaired P/Q stripes during scrub, or already
> corrupted P/Q copied to the dev-replace target.
> 
> [FIX]
> The situation is more complex than the regression, in fact the initial
> behavior is not 100% correct either.
> 
> If we have the following rare case, it can still lead to the same
> problem using the old behavior:
> 
> 		0	16K	32K	48K	64K
> 	Data 1:	|IIIIIII|                       |
> 	Data 2:	|				|
> 	Parity:	|	|CCCCCCC|		|
> 
> Where "I" means IO error, "C" means corruption.
> 
> In the above case, we're scrubbing the parity stripe, then read out all
> the contents of Data 1, Data 2, Parity stripes.
> 
> But found IO error in Data 1, which leads to rebuild using Data 2 and
> Parity and got the correct data.
> 
> In that case, we would not verify if the Pairty is correct for range
> [16K, 32K).
> 
> So here we have to always verify the content of Parity no matter if we
> did recovery or not.
> 
> This patch would remove the @need_check parameter of
> finish_parity_scrub() completely, and would always do the P/Q
> verification before writeback.
> 
> Fixes: 75b470332965 ("btrfs: raid56: migrate recovery and scrub recovery path to use error_bitmap")
> Signed-off-by: Qu Wenruo <[e-mail]>

Added to misc-next, thanks.

Patch

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index ed7235a73485..f61f82febe98 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -71,7 +71,7 @@  static void rmw_rbio_work_locked(struct work_struct *work);
 static void index_rbio_pages(struct btrfs_raid_bio *rbio);
 static int alloc_rbio_pages(struct btrfs_raid_bio *rbio);
 
-static int finish_parity_scrub(struct btrfs_raid_bio *rbio, int need_check);
+static int finish_parity_scrub(struct btrfs_raid_bio *rbio);
 static void scrub_rbio_work_locked(struct work_struct *work);
 
 static void free_raid_bio_pointers(struct btrfs_raid_bio *rbio)
@@ -2429,7 +2429,7 @@  static int alloc_rbio_essential_pages(struct btrfs_raid_bio *rbio)
 	return 0;
 }
 
-static int finish_parity_scrub(struct btrfs_raid_bio *rbio, int need_check)
+static int finish_parity_scrub(struct btrfs_raid_bio *rbio)
 {
 	struct btrfs_io_context *bioc = rbio->bioc;
 	const u32 sectorsize = bioc->fs_info->sectorsize;
@@ -2470,9 +2470,6 @@  static int finish_parity_scrub(struct btrfs_raid_bio *rbio, int need_check)
 	 */
 	clear_bit(RBIO_CACHE_READY_BIT, &rbio->flags);
 
-	if (!need_check)
-		goto writeback;
-
 	p_sector.page = alloc_page(GFP_NOFS);
 	if (!p_sector.page)
 		return -ENOMEM;
@@ -2541,7 +2538,6 @@  static int finish_parity_scrub(struct btrfs_raid_bio *rbio, int need_check)
 		q_sector.page = NULL;
 	}
 
-writeback:
 	/*
 	 * time to start writing.  Make bios for everything from the
 	 * higher layers (the bio_list in our rbio) and our p/q.  Ignore
@@ -2724,7 +2720,6 @@  static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio)
 
 static void scrub_rbio(struct btrfs_raid_bio *rbio)
 {
-	bool need_check = false;
 	int sector_nr;
 	int ret;
 
@@ -2747,7 +2742,7 @@  static void scrub_rbio(struct btrfs_raid_bio *rbio)
 	 * We have every sector properly prepared. Can finish the scrub
 	 * and writeback the good content.
 	 */
-	ret = finish_parity_scrub(rbio, need_check);
+	ret = finish_parity_scrub(rbio);
 	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
 	for (sector_nr = 0; sector_nr < rbio->stripe_nsectors; sector_nr++) {
 		int found_errors;