Skip to contentSkip to conversation

Btrfs: RAID 5/6 missing device replace

PATCH In review Omar Sandoval 3 participants 12 messages May 11, 2015 – Jun 12, 2015 Btrfs file system

lore
Share & tools
lore

Omar Sandoval

author
#0 lore raw

cover Btrfs: RAID 5/6 missing device replace

A user reported on Bugzilla that they were seeing kernel BUGs when attempting to replace a missing device on a RAID 6 array. After identifying the apparent cause of the BUG, I reached the conclusion that there wasn't a quick fix. Maybe Miao Xie can point something out that I missed, as he originally implemented device replace on RAID 5/6 :)

Patch 4 has the details, but the main problem is that we can't create bios for a missing device, so the main scrub code path isn't very useful. On RAID 5/6, since we only have one mirror for any piece of data, the missing device is the only mirror we can use. Clearly, (unless I missed something), this case needs to be handled differently.

These patches are on top of v4.1-rc2. I ran the scrub and replace xfstests and the script below, which also reproduces the original BUG.

Thanks!

Omar Sandoval (4):
Btrfs: remove misleading handling of missing device scrub
Btrfs: count devices correctly in readahead during RAID 5/6 replace
Btrfs: add RAID 5/6 BTRFS_RBIO_REBUILD_MISSING operation
Btrfs: fix device replace of a missing RAID 5/6 device

 fs/btrfs/raid56.c |  87 +++++++++++++++++++++++++----
 fs/btrfs/raid56.h |  10 +++-
 fs/btrfs/reada.c  |   4 +-
 fs/btrfs/scrub.c  | 164 +++++++++++++++++++++++++++++++++++++++++++++---------
 4 files changed, 225 insertions(+), 40 deletions(-)

Testing script:

----
#!/bin/bash

USAGE="Usage: $0 [eio|missing] [raid0|raid1|raid5|raid6]"

if [ "$1" = "-h" ]; then
echo "$USAGE"
exit
fi

MODE="${1:-missing}"
RAID="${2:-raid5}"

case "$MODE" in

	eio|missing)
		;;
	*)
		echo "$USAGE" >&2
		exit 1
		;;

esac

case "$RAID" in

	raid[0156])
		;;
	*)
		echo "$USAGE" >&2
		exit 1
		;;

esac

NUM_DISKS=4
NUM_RAID_DISKS=3
SRC_DISK=1
TARGET_DISK=3
NUM_SECTORS=$((1024 * 1024))
LOOP_DEVICES=()
DM_DEVICES=()

cleanup () {
	echo "Done. Press enter to cleanup..."
	read
	if findmnt /mnt; then
		umount /mnt
	fi
	for DM in "${DM_DEVICES[@]}"; do
		dmsetup remove "$DM"
	done
	for LOOP in "${LOOP_DEVICES[@]}"; do
		losetup --detach "$LOOP"
	done
	for ((i = 0; i < NUM_DISKS; i++)); do
		rm -f disk${i}.img
	done
}

trap 'cleanup; exit 1' ERR

echo "Creating disk images..."
for ((i = 0; i < NUM_DISKS; i++)); do

	rm -f disk${i}.img
	dd if=/dev/zero of=disk${i}.img bs=512 seek=$NUM_SECTORS count=0
	LOOP_DEVICES+=("$(losetup --find --show disk${i}.img)")

done

echo "Creating loopback devices..."
for LOOP in "${LOOP_DEVICES[@]}"; do

	DM="${LOOP/\/dev\/loop/dm}"
	dmsetup create "$DM" --table "0 $NUM_SECTORS linear $LOOP 0"
	DM_DEVICES+=("$DM")

done

echo "Creating filesystem..." FS_DEVICES=("${DM_DEVICES[@]:0:$NUM_RAID_DISKS}") FS_DEVICES=("${FS_DEVICES[@]/#/\/dev\/mapper\/}") MOUNT_DEVICE="${FS_DEVICES[$(((SRC_DISK + 1) % NUM_RAID_DISKS))]}" mkfs.btrfs -d "$RAID" -m "$RAID" "${FS_DEVICES[@]}" mount "$MOUNT_DEVICE" /mnt
cp -r ~/xfstests /mnt
sync

case "$MODE" in

	eio)
		echo "Killing disk..."
		dmsetup suspend "${DM_DEVICES[$SRC_DISK]}"
		dmsetup reload "${DM_DEVICES[$SRC_DISK]}" --table "0 $NUM_SECTORS error"
		dmsetup resume "${DM_DEVICES[$SRC_DISK]}"
		;;
	missing)
		echo "Removing disk and remounting degraded..."
		umount /mnt
		dmsetup remove "${DM_DEVICES[$SRC_DISK]}"
		unset DM_DEVICES[$SRC_DISK]
		mount -o degraded "$MOUNT_DEVICE" /mnt
		;;

esac

echo "Replacing disk..."
btrfs replace start -B $((SRC_DISK + 1)) /dev/mapper/"${DM_DEVICES[$TARGET_DISK]}" /mnt

echo "Scrubbing to double-check..."
btrfs scrub start -Br /mnt

cleanup
----

Signature · 1 line
2.4.0
author
#1 lore raw

PATCH 1/4 Btrfs: remove misleading handling of missing device scrub

scrub_submit() claims that it can handle a bio with a NULL block device, but this is misleading, as calling bio_add_page() on a bio with a NULL ->bi_bdev would've already crashed. Delete this, as we're about to properly handle a missing block device.

Signed-off-by Omar Sandoval <[email protected]>
---
 fs/btrfs/scrub.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)
fs/btrfs/scrub.c +1 -15
index ab58115..633fa7b 100644
@@ -2087,21 +2087,7 @@ static void scrub_submit(struct scrub_ctx *sctx)
 	sbio = sctx->bios[sctx->curr];
 	sctx->curr = -1;
 	scrub_pending_bio_inc(sctx);
-
-	if (!sbio->bio->bi_bdev) {
-		/*
-		 * this case should not happen. If btrfs_map_block() is
-		 * wrong, it could happen for dev-replace operations on
-		 * missing devices when no mirrors are available, but in
-		 * this case it should already fail the mount.
-		 * This case is handled correctly (but _very_ slowly).
-		 */
-		printk_ratelimited(KERN_WARNING
-			"BTRFS: scrub_submit(bio bdev == NULL) is unexpected!\n");
-		bio_endio(sbio->bio, -EIO);
-	} else {
-		btrfsic_submit_bio(READ, sbio->bio);
-	}
+	btrfsic_submit_bio(READ, sbio->bio);
 }
 
 static int scrub_add_page_to_rd_bio(struct scrub_ctx *sctx,
Signature · 1 line
2.4.0
author
#2 lore raw

PATCH 2/4 Btrfs: count devices correctly in readahead during RAID 5/6 replace

In reply to Omar Sandoval

Commit 5fbc7c59fd22 ("Btrfs: fix unfinished readahead thread for raid5/6 degraded mounting") fixed a problem where we would skip a missing device when we shouldn't have because there are no other mirrors to read from in RAID 5/6. After commit 2c8cdd6ee4e7 ("Btrfs, replace: write dirty pages into the replace target device"), the fix doesn't work when we're doing a missing device replace on RAID 5/6 because the replace device is counted as a mirror so we're tricked into thinking we can safely skip the missing device. The fix is to count only the real stripes and decide based on that.

Signed-off-by Omar Sandoval <[email protected]>
---
 fs/btrfs/reada.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/btrfs/reada.c +3 -1
index 0e7beea..4645cd1 100644
@@ -328,6 +328,7 @@ static struct reada_extent *reada_find_extent(struct btrfs_root *root,
 	struct btrfs_device *prev_dev;
 	u32 blocksize;
 	u64 length;
+	int real_stripes;
 	int nzones = 0;
 	int i;
 	unsigned long index = logical >> PAGE_CACHE_SHIFT;

@@ -369,7 +370,8 @@ static struct reada_extent *reada_find_extent(struct btrfs_root *root,
 		goto error;
 	}
 
-	for (nzones = 0; nzones < bbio->num_stripes; ++nzones) {
+	real_stripes = bbio->num_stripes - bbio->num_tgtdevs;
+	for (nzones = 0; nzones < real_stripes; ++nzones) {
 		struct reada_zone *zone;
 
 		dev = bbio->stripes[nzones].dev;
Signature · 1 line
2.4.0
author
#3 lore raw

PATCH 3/4 Btrfs: add RAID 5/6 BTRFS_RBIO_REBUILD_MISSING operation

In reply to Omar Sandoval

The current RAID 5/6 recovery code isn't quite prepared to handle missing devices. In particular, it expects a bio that we previously attempted to use in the read path, meaning that it has valid pages allocated. However, missing devices have a NULL blkdev, and we can't call bio_add_page() on a bio with a NULL blkdev. We could do manual manipulation of bio->bi_io_vec, but that's pretty gross. So instead, add a separate path that allows us to manually add pages to the rbio.

Signed-off-by Omar Sandoval <[email protected]>
---
 fs/btrfs/raid56.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 fs/btrfs/raid56.h | 10 +++++--
 fs/btrfs/scrub.c  |  3 +-
 3 files changed, 86 insertions(+), 14 deletions(-)
fs/btrfs/raid56.c +77 -10
index fa72068..6fe2613 100644
@@ -61,9 +61,10 @@
 #define RBIO_CACHE_SIZE 1024
 
 enum btrfs_rbio_ops {
-	BTRFS_RBIO_WRITE	= 0,
-	BTRFS_RBIO_READ_REBUILD	= 1,
-	BTRFS_RBIO_PARITY_SCRUB	= 2,
+	BTRFS_RBIO_WRITE,
+	BTRFS_RBIO_READ_REBUILD,
+	BTRFS_RBIO_PARITY_SCRUB,
+	BTRFS_RBIO_REBUILD_MISSING,
 };
 
 struct btrfs_raid_bio {

@@ -602,6 +603,10 @@ static int rbio_can_merge(struct btrfs_raid_bio *last,
 	    cur->operation == BTRFS_RBIO_PARITY_SCRUB)
 		return 0;
 
+	if (last->operation == BTRFS_RBIO_REBUILD_MISSING ||
+	    cur->operation == BTRFS_RBIO_REBUILD_MISSING)
+		return 0;
+
 	return 1;
 }
 

@@ -793,7 +798,10 @@ static noinline void unlock_stripe(struct btrfs_raid_bio *rbio)
 
 			if (next->operation == BTRFS_RBIO_READ_REBUILD)
 				async_read_rebuild(next);
-			else if (next->operation == BTRFS_RBIO_WRITE) {
+			else if (next->operation == BTRFS_RBIO_REBUILD_MISSING) {
+				steal_rbio(rbio, next);
+				async_read_rebuild(next);
+			} else if (next->operation == BTRFS_RBIO_WRITE) {
 				steal_rbio(rbio, next);
 				async_rmw_stripe(next);
 			} else if (next->operation == BTRFS_RBIO_PARITY_SCRUB) {

@@ -1809,7 +1817,8 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
 	faila = rbio->faila;
 	failb = rbio->failb;
 
-	if (rbio->operation == BTRFS_RBIO_READ_REBUILD) {
+	if (rbio->operation == BTRFS_RBIO_READ_REBUILD ||
+	    rbio->operation == BTRFS_RBIO_REBUILD_MISSING) {
 		spin_lock_irq(&rbio->bio_list_lock);
 		set_bit(RBIO_RMW_LOCKED_BIT, &rbio->flags);
 		spin_unlock_irq(&rbio->bio_list_lock);

@@ -1834,7 +1843,8 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
 			 * if we're rebuilding a read, we have to use
 			 * pages from the bio list
 			 */
-			if (rbio->operation == BTRFS_RBIO_READ_REBUILD &&
+			if ((rbio->operation == BTRFS_RBIO_READ_REBUILD ||
+			     rbio->operation == BTRFS_RBIO_REBUILD_MISSING) &&
 			    (stripe == faila || stripe == failb)) {
 				page = page_in_rbio(rbio, stripe, pagenr, 0);
 			} else {

@@ -1943,7 +1953,8 @@ pstripe:
 			 * if we're rebuilding a read, we have to use
 			 * pages from the bio list
 			 */
-			if (rbio->operation == BTRFS_RBIO_READ_REBUILD &&
+			if ((rbio->operation == BTRFS_RBIO_READ_REBUILD ||
+			     rbio->operation == BTRFS_RBIO_REBUILD_MISSING) &&
 			    (stripe == faila || stripe == failb)) {
 				page = page_in_rbio(rbio, stripe, pagenr, 0);
 			} else {

@@ -1965,6 +1976,8 @@ cleanup_io:
 			clear_bit(RBIO_CACHE_READY_BIT, &rbio->flags);
 
 		rbio_orig_end_io(rbio, err, err == 0);
+	} else if (rbio->operation == BTRFS_RBIO_REBUILD_MISSING) {
+		rbio_orig_end_io(rbio, err, err == 0);
 	} else if (err == 0) {
 		rbio->faila = -1;
 		rbio->failb = -1;

@@ -2101,7 +2114,8 @@ out:
 	return 0;
 
 cleanup:
-	if (rbio->operation == BTRFS_RBIO_READ_REBUILD)
+	if (rbio->operation == BTRFS_RBIO_READ_REBUILD ||
+	    rbio->operation == BTRFS_RBIO_REBUILD_MISSING)
 		rbio_orig_end_io(rbio, -EIO, 0);
 	return -EIO;
 }

@@ -2232,8 +2246,9 @@ raid56_parity_alloc_scrub_rbio(struct btrfs_root *root, struct bio *bio,
 	return rbio;
 }
 
-void raid56_parity_add_scrub_pages(struct btrfs_raid_bio *rbio,
-				   struct page *page, u64 logical)
+/* Used for both parity scrub and missing. */
+void raid56_add_scrub_pages(struct btrfs_raid_bio *rbio, struct page *page,
+			    u64 logical)
 {
 	int stripe_offset;
 	int index;

@@ -2668,3 +2683,55 @@ void raid56_parity_submit_scrub_rbio(struct btrfs_raid_bio *rbio)
 	if (!lock_stripe_add(rbio))
 		async_scrub_parity(rbio);
 }
+
+/* The following code is used for dev replace of a missing RAID 5/6 device. */
+
+struct btrfs_raid_bio *
+raid56_alloc_missing_rbio(struct btrfs_root *root, struct bio *bio,
+			  struct btrfs_bio *bbio, u64 length)
+{
+	struct btrfs_raid_bio *rbio;
+
+	rbio = alloc_rbio(root, bbio, length);
+	if (IS_ERR(rbio))
+		return NULL;
+
+	rbio->operation = BTRFS_RBIO_REBUILD_MISSING;
+	bio_list_add(&rbio->bio_list, bio);
+	/*
+	 * This is a special bio which is used to hold the completion handler
+	 * and make the scrub rbio is similar to the other types
+	 */
+	ASSERT(!bio->bi_iter.bi_size);
+
+	rbio->faila = find_logical_bio_stripe(rbio, bio);
+	if (rbio->faila == -1) {
+		BUG();
+		kfree(rbio);
+		return NULL;
+	}
+
+	return rbio;
+}
+
+static void missing_raid56_work(struct btrfs_work *work)
+{
+	struct btrfs_raid_bio *rbio;
+
+	rbio = container_of(work, struct btrfs_raid_bio, work);
+	__raid56_parity_recover(rbio);
+}
+
+static void async_missing_raid56(struct btrfs_raid_bio *rbio)
+{
+	btrfs_init_work(&rbio->work, btrfs_rmw_helper,
+			missing_raid56_work, NULL, NULL);
+
+	btrfs_queue_work(rbio->fs_info->rmw_workers, &rbio->work);
+}
+
+void raid56_submit_missing_rbio(struct btrfs_raid_bio *rbio)
+{
+	if (!lock_stripe_add(rbio))
+		async_missing_raid56(rbio);
+}
fs/btrfs/raid56.h +8 -2
index 2b5d797..8b69469 100644
@@ -48,15 +48,21 @@ int raid56_parity_recover(struct btrfs_root *root, struct bio *bio,
 int raid56_parity_write(struct btrfs_root *root, struct bio *bio,
 			       struct btrfs_bio *bbio, u64 stripe_len);
 
+void raid56_add_scrub_pages(struct btrfs_raid_bio *rbio, struct page *page,
+			    u64 logical);
+
 struct btrfs_raid_bio *
 raid56_parity_alloc_scrub_rbio(struct btrfs_root *root, struct bio *bio,
 			       struct btrfs_bio *bbio, u64 stripe_len,
 			       struct btrfs_device *scrub_dev,
 			       unsigned long *dbitmap, int stripe_nsectors);
-void raid56_parity_add_scrub_pages(struct btrfs_raid_bio *rbio,
-				   struct page *page, u64 logical);
 void raid56_parity_submit_scrub_rbio(struct btrfs_raid_bio *rbio);
 
+struct btrfs_raid_bio *
+raid56_alloc_missing_rbio(struct btrfs_root *root, struct bio *bio,
+			  struct btrfs_bio *bbio, u64 length);
+void raid56_submit_missing_rbio(struct btrfs_raid_bio *rbio);
+
 int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info);
 void btrfs_free_stripe_hash_table(struct btrfs_fs_info *info);
 #endif
fs/btrfs/scrub.c +1 -2
index 633fa7b..b94694d 100644
@@ -2699,8 +2699,7 @@ static void scrub_parity_check_and_repair(struct scrub_parity *sparity)
 		goto rbio_out;
 
 	list_for_each_entry(spage, &sparity->spages, list)
-		raid56_parity_add_scrub_pages(rbio, spage->page,
-					      spage->logical);
+		raid56_add_scrub_pages(rbio, spage->page, spage->logical);
 
 	scrub_pending_bio_inc(sctx);
 	raid56_parity_submit_scrub_rbio(rbio);
Signature · 1 line
2.4.0
author
#4 lore raw

PATCH 4/4 Btrfs: fix device replace of a missing RAID 5/6 device

In reply to Omar Sandoval

The original implementation of device replace on RAID 5/6 seems to have missed support for replacing a missing device. When this is attempted, we end up calling bio_add_page() on a bio with a NULL ->bi_bdev, which crashes when we try to dereference it. This happens because btrfs_map_block() has no choice but to return us the missing device because RAID 5/6 don't have any alternate mirrors to read from, and a missing device has a NULL bdev.

The idea implemented here is to handle the missing device case separately, which better only happen when we're replacing a missing RAID 5/6 device. We use the new BTRFS_RBIO_REBUILD_MISSING operation to reconstruct the data from parity, check it with scrub_recheck_block_checksum(), and write it out with scrub_write_block_to_dev_replace().

Reported-by Philip <[email protected]>

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=96141

Signed-off-by Omar Sandoval <[email protected]>
---
 fs/btrfs/scrub.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 135 insertions(+), 10 deletions(-)
fs/btrfs/scrub.c +135 -10
index b94694d..a13f91a 100644
@@ -125,6 +125,7 @@ struct scrub_block {
 		/* It is for the data with checksum */
 		unsigned int	data_corrected:1;
 	};
+	struct btrfs_work	work;
 };
 
 /* Used for the chunks with parity stripe such RAID5/6 */

@@ -2164,6 +2165,126 @@ again:
 	return 0;
 }
 
+static void scrub_missing_raid56_end_io(struct bio *bio, int error)
+{
+	struct scrub_block *sblock = bio->bi_private;
+	struct btrfs_fs_info *fs_info = sblock->sctx->dev_root->fs_info;
+
+	if (error)
+		sblock->no_io_error_seen = 0;
+
+	btrfs_queue_work(fs_info->scrub_workers, &sblock->work);
+}
+
+static void scrub_missing_raid56_worker(struct btrfs_work *work)
+{
+	struct scrub_block *sblock = container_of(work, struct scrub_block, work);
+	struct scrub_ctx *sctx = sblock->sctx;
+	struct btrfs_fs_info *fs_info = sctx->dev_root->fs_info;
+	unsigned int is_metadata;
+	unsigned int have_csum;
+	u8 *csum;
+	u64 generation;
+	u64 logical;
+	struct btrfs_device *dev;
+
+	is_metadata = !(sblock->pagev[0]->flags & BTRFS_EXTENT_FLAG_DATA);
+	have_csum = sblock->pagev[0]->have_csum;
+	csum = sblock->pagev[0]->csum;
+	generation = sblock->pagev[0]->generation;
+	logical = sblock->pagev[0]->logical;
+	dev = sblock->pagev[0]->dev;
+
+	if (sblock->no_io_error_seen) {
+		scrub_recheck_block_checksum(fs_info, sblock, is_metadata,
+					     have_csum, csum, generation,
+					     sctx->csum_size);
+	}
+
+	if (!sblock->no_io_error_seen) {
+		spin_lock(&sctx->stat_lock);
+		sctx->stat.read_errors++;
+		spin_unlock(&sctx->stat_lock);
+		printk_ratelimited_in_rcu(KERN_ERR
+			"BTRFS: I/O error rebulding logical %llu for dev %s\n",
+			logical, rcu_str_deref(dev->name));
+	} else if (sblock->header_error || sblock->checksum_error) {
+		spin_lock(&sctx->stat_lock);
+		sctx->stat.uncorrectable_errors++;
+		spin_unlock(&sctx->stat_lock);
+		printk_ratelimited_in_rcu(KERN_ERR
+			"BTRFS: failed to rebuild valid logical %llu for dev %s\n",
+			logical, rcu_str_deref(dev->name));
+	} else {
+		scrub_write_block_to_dev_replace(sblock);
+	}
+
+	scrub_block_put(sblock);
+	scrub_pending_bio_dec(sctx);
+}
+
+static void scrub_missing_raid56_pages(struct scrub_block *sblock)
+{
+	struct scrub_ctx *sctx = sblock->sctx;
+	struct btrfs_fs_info *fs_info = sctx->dev_root->fs_info;
+	u64 length = sblock->page_count * PAGE_SIZE;
+	u64 logical = sblock->pagev[0]->logical;
+	struct btrfs_bio *bbio;
+	struct bio *bio;
+	struct btrfs_raid_bio *rbio;
+	int ret;
+	int i;
+
+	ret = btrfs_map_sblock(fs_info, REQ_GET_READ_MIRRORS, logical, &length,
+			       &bbio, 0, 1);
+	if (ret || !bbio || !bbio->raid_map)
+		goto bbio_out;
+
+	if (WARN_ON(!sctx->is_dev_replace ||
+		    !(bbio->map_type & BTRFS_BLOCK_GROUP_RAID56_MASK))) {
+		/*
+		 * We shouldn't be scrubbing a missing device. Even for dev
+		 * replace, we should only get here for RAID 5/6. We either
+		 * managed to mount something with no mirrors remaining or
+		 * there's a bug in scrub_remap_extent()/btrfs_map_block().
+		 */
+		goto bbio_out;
+	}
+
+	bio = btrfs_io_bio_alloc(GFP_NOFS, 0);
+	if (!bio)
+		goto bbio_out;
+
+	bio->bi_iter.bi_sector = logical >> 9;
+	bio->bi_private = sblock;
+	bio->bi_end_io = scrub_missing_raid56_end_io;
+
+	rbio = raid56_alloc_missing_rbio(sctx->dev_root, bio, bbio, length);
+	if (!rbio)
+		goto rbio_out;
+
+	for (i = 0; i < sblock->page_count; i++) {
+		struct scrub_page *spage = sblock->pagev[i];
+
+		raid56_add_scrub_pages(rbio, spage->page, spage->logical);
+	}
+
+	btrfs_init_work(&sblock->work, btrfs_scrub_helper,
+			scrub_missing_raid56_worker, NULL, NULL);
+	scrub_block_get(sblock);
+	scrub_pending_bio_inc(sctx);
+	raid56_submit_missing_rbio(rbio);
+	return;
+
+rbio_out:
+	bio_put(bio);
+bbio_out:
+	btrfs_put_bbio(bbio);
+	spin_lock(&sctx->stat_lock);
+	sctx->stat.malloc_errors++;
+	spin_unlock(&sctx->stat_lock);
+}
+
 static int scrub_pages(struct scrub_ctx *sctx, u64 logical, u64 len,
 		       u64 physical, struct btrfs_device *dev, u64 flags,
 		       u64 gen, int mirror_num, u8 *csum, int force,

@@ -2227,19 +2348,23 @@ leave_nomem:
 	}
 
 	WARN_ON(sblock->page_count == 0);
-	for (index = 0; index < sblock->page_count; index++) {
-		struct scrub_page *spage = sblock->pagev[index];
-		int ret;
+	if (dev->missing) {
+		scrub_missing_raid56_pages(sblock);
+	} else {
+		for (index = 0; index < sblock->page_count; index++) {
+			struct scrub_page *spage = sblock->pagev[index];
+			int ret;
 
-		ret = scrub_add_page_to_rd_bio(sctx, spage);
-		if (ret) {
-			scrub_block_put(sblock);
-			return ret;
+			ret = scrub_add_page_to_rd_bio(sctx, spage);
+			if (ret) {
+				scrub_block_put(sblock);
+				return ret;
+			}
 		}
-	}
 
-	if (force)
-		scrub_submit(sctx);
+		if (force)
+			scrub_submit(sctx);
+	}
 
 	/* last one frees, either here or in bio completion for last page */
 	scrub_block_put(sblock);
Signature · 1 line
2.4.0
#5 lore raw

RE: [PATCH 4/4] Btrfs: fix device replace of a missing RAID 5/6 device

Hi, Omar Sandoval

Quoted 165 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Omar Sandoval
Sent: Monday, May 11, 2015 3:58 PM
To: [email protected]
Cc: Miao Xie; Philip; Omar Sandoval
Subject: [PATCH 4/4] Btrfs: fix device replace of a missing RAID 5/6 device

The original implementation of device replace on RAID 5/6 seems to have missed support for replacing a missing device. When this is attempted, we end up calling bio_add_page() on a bio with a NULL ->bi_bdev, which crashes when we try to dereference it. This happens because
btrfs_map_block() has no choice but to return us the missing device because RAID 5/6 don't have any alternate mirrors to read from, and a missing device has a NULL bdev.

The idea implemented here is to handle the missing device case separately, which better only happen when we're replacing a missing RAID 5/6 device. We use the new BTRFS_RBIO_REBUILD_MISSING operation to reconstruct the data from parity, check it with scrub_recheck_block_checksum(), and write it out with scrub_write_block_to_dev_replace().

Reported-by Philip <[email protected]>

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=96141

Signed-off-by Omar Sandoval <[email protected]>
---
 fs/btrfs/scrub.c | 145
+++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 135 insertions(+), 10 deletions(-)
fs/btrfs/scrub.c +69 -0
@@ -125,6 +125,7 @@ struct scrub_block {
 		/* It is for the data with checksum */
 		unsigned int	data_corrected:1;
 	};
+	struct btrfs_work	work;
 };
 
 /* Used for the chunks with parity stripe such RAID5/6 */ @@ -2164,6
+2165,126 @@ again:
 	return 0;
 }
 
+static void scrub_missing_raid56_end_io(struct bio *bio, int error) {
+	struct scrub_block *sblock = bio->bi_private;
+	struct btrfs_fs_info *fs_info = sblock->sctx->dev_root->fs_info;
+
+	if (error)
+		sblock->no_io_error_seen = 0;
+
+	btrfs_queue_work(fs_info->scrub_workers, &sblock->work); }
+
+static void scrub_missing_raid56_worker(struct btrfs_work *work) {
+	struct scrub_block *sblock = container_of(work, struct scrub_block, work);
+	struct scrub_ctx *sctx = sblock->sctx;
+	struct btrfs_fs_info *fs_info = sctx->dev_root->fs_info;
+	unsigned int is_metadata;
+	unsigned int have_csum;
+	u8 *csum;
+	u64 generation;
+	u64 logical;
+	struct btrfs_device *dev;
+
+	is_metadata = !(sblock->pagev[0]->flags & BTRFS_EXTENT_FLAG_DATA);
+	have_csum = sblock->pagev[0]->have_csum;
+	csum = sblock->pagev[0]->csum;
+	generation = sblock->pagev[0]->generation;
+	logical = sblock->pagev[0]->logical;
+	dev = sblock->pagev[0]->dev;
+
+	if (sblock->no_io_error_seen) {
+		scrub_recheck_block_checksum(fs_info, sblock, is_metadata,
+					     have_csum, csum, generation,
+					     sctx->csum_size);
+	}
+
+	if (!sblock->no_io_error_seen) {
+		spin_lock(&sctx->stat_lock);
+		sctx->stat.read_errors++;
+		spin_unlock(&sctx->stat_lock);
+		printk_ratelimited_in_rcu(KERN_ERR
+			"BTRFS: I/O error rebulding logical %llu for dev %s\n",
+			logical, rcu_str_deref(dev->name));
+	} else if (sblock->header_error || sblock->checksum_error) {
+		spin_lock(&sctx->stat_lock);
+		sctx->stat.uncorrectable_errors++;
+		spin_unlock(&sctx->stat_lock);
+		printk_ratelimited_in_rcu(KERN_ERR
+			"BTRFS: failed to rebuild valid logical %llu for dev %s\n",
+			logical, rcu_str_deref(dev->name));
+	} else {
+		scrub_write_block_to_dev_replace(sblock);
+	}
+
+	scrub_block_put(sblock);
+	scrub_pending_bio_dec(sctx);
+}
+
+static void scrub_missing_raid56_pages(struct scrub_block *sblock) {
+	struct scrub_ctx *sctx = sblock->sctx;
+	struct btrfs_fs_info *fs_info = sctx->dev_root->fs_info;
+	u64 length = sblock->page_count * PAGE_SIZE;
+	u64 logical = sblock->pagev[0]->logical;
+	struct btrfs_bio *bbio;
+	struct bio *bio;
+	struct btrfs_raid_bio *rbio;
+	int ret;
+	int i;
+
+	ret = btrfs_map_sblock(fs_info, REQ_GET_READ_MIRRORS, logical,

&length,

fs/btrfs/scrub.c +49 -0
+			       &bbio, 0, 1);
+	if (ret || !bbio || !bbio->raid_map)
+		goto bbio_out;
+
+	if (WARN_ON(!sctx->is_dev_replace ||
+		    !(bbio->map_type & BTRFS_BLOCK_GROUP_RAID56_MASK))) {
+		/*
+		 * We shouldn't be scrubbing a missing device. Even for dev
+		 * replace, we should only get here for RAID 5/6. We either
+		 * managed to mount something with no mirrors remaining or
+		 * there's a bug in scrub_remap_extent()/btrfs_map_block().
+		 */
+		goto bbio_out;
+	}
+
+	bio = btrfs_io_bio_alloc(GFP_NOFS, 0);
+	if (!bio)
+		goto bbio_out;
+
+	bio->bi_iter.bi_sector = logical >> 9;
+	bio->bi_private = sblock;
+	bio->bi_end_io = scrub_missing_raid56_end_io;
+
+	rbio = raid56_alloc_missing_rbio(sctx->dev_root, bio, bbio, length);
+	if (!rbio)
+		goto rbio_out;
+
+	for (i = 0; i < sblock->page_count; i++) {
+		struct scrub_page *spage = sblock->pagev[i];
+
+		raid56_add_scrub_pages(rbio, spage->page, spage->logical);
+	}
+
+	btrfs_init_work(&sblock->work, btrfs_scrub_helper,
+			scrub_missing_raid56_worker, NULL, NULL);
+	scrub_block_get(sblock);
+	scrub_pending_bio_inc(sctx);
+	raid56_submit_missing_rbio(rbio);
+	return;
+
+rbio_out:
+	bio_put(bio);
+bbio_out:
+	btrfs_put_bbio(bbio);
+	spin_lock(&sctx->stat_lock);
+	sctx->stat.malloc_errors++;
+	spin_unlock(&sctx->stat_lock);
+}
+
 static int scrub_pages(struct scrub_ctx *sctx, u64 logical, u64 len,
 		       u64 physical, struct btrfs_device *dev, u64 flags,

fs/btrfs/scrub.c again:

Quoted 10 lines
fs/btrfs/scrub.c +3 -3
 		       u64 gen, int mirror_num, u8 *csum, int force, @@ -2227,19
+2348,23 @@ leave_nomem:
 	}
 
 	WARN_ON(sblock->page_count == 0);
-	for (index = 0; index < sblock->page_count; index++) {
-		struct scrub_page *spage = sblock->pagev[index];
-		int ret;
+	if (dev->missing) {
+		scrub_missing_raid56_pages(sblock);

Both non-raid56 and raid56 case have possibility run to here.

If it is a bad non-raid56 device, call scrub_missing_raid56_pages() for a non-raid56 device seems not suitable.

Since I hadn't read this patch carefully, please ignore if it is not a problem

Thanks
Zhaolei

Quoted 32 lines
fs/btrfs/scrub.c +12 -8
+	} else {
+		for (index = 0; index < sblock->page_count; index++) {
+			struct scrub_page *spage = sblock->pagev[index];
+			int ret;
 
-		ret = scrub_add_page_to_rd_bio(sctx, spage);
-		if (ret) {
-			scrub_block_put(sblock);
-			return ret;
+			ret = scrub_add_page_to_rd_bio(sctx, spage);
+			if (ret) {
+				scrub_block_put(sblock);
+				return ret;
+			}
 		}
-	}
 
-	if (force)
-		scrub_submit(sctx);
+		if (force)
+			scrub_submit(sctx);
+	}
 
 	/* last one frees, either here or in bio completion for last page */
 	scrub_block_put(sblock);
--

2.4.0

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

author
#6 lore raw

Re: [PATCH 4/4] Btrfs: fix device replace of a missing RAID 5/6 device

Hi, Zhaolei,

Quoted 92 lines · Zhao Lei

Hi, Omar Sandoval

Quoted 90 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Omar Sandoval
Sent: Monday, May 11, 2015 3:58 PM
To: [email protected]
Cc: Miao Xie; Philip; Omar Sandoval
Subject: [PATCH 4/4] Btrfs: fix device replace of a missing RAID 5/6 device

The original implementation of device replace on RAID 5/6 seems to have missed support for replacing a missing device. When this is attempted, we end up calling bio_add_page() on a bio with a NULL ->bi_bdev, which crashes when we try to dereference it. This happens because
btrfs_map_block() has no choice but to return us the missing device because RAID 5/6 don't have any alternate mirrors to read from, and a missing device has a NULL bdev.

The idea implemented here is to handle the missing device case separately, which better only happen when we're replacing a missing RAID 5/6 device. We use the new BTRFS_RBIO_REBUILD_MISSING operation to reconstruct the data from parity, check it with scrub_recheck_block_checksum(), and write it out with scrub_write_block_to_dev_replace().

Reported-by Philip <[email protected]>

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=96141

Signed-off-by Omar Sandoval <[email protected]>
---
 fs/btrfs/scrub.c | 145
+++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 135 insertions(+), 10 deletions(-)
fs/btrfs/scrub.c +46 -0
@@ -125,6 +125,7 @@ struct scrub_block {
 		/* It is for the data with checksum */
 		unsigned int	data_corrected:1;
 	};
+	struct btrfs_work	work;
 };
 
 /* Used for the chunks with parity stripe such RAID5/6 */ @@ -2164,6
+2165,126 @@ again:
 	return 0;
 }
 
+static void scrub_missing_raid56_end_io(struct bio *bio, int error) {
+	struct scrub_block *sblock = bio->bi_private;
+	struct btrfs_fs_info *fs_info = sblock->sctx->dev_root->fs_info;
+
+	if (error)
+		sblock->no_io_error_seen = 0;
+
+	btrfs_queue_work(fs_info->scrub_workers, &sblock->work); }
+
+static void scrub_missing_raid56_worker(struct btrfs_work *work) {
+	struct scrub_block *sblock = container_of(work, struct scrub_block, work);
+	struct scrub_ctx *sctx = sblock->sctx;
+	struct btrfs_fs_info *fs_info = sctx->dev_root->fs_info;
+	unsigned int is_metadata;
+	unsigned int have_csum;
+	u8 *csum;
+	u64 generation;
+	u64 logical;
+	struct btrfs_device *dev;
+
+	is_metadata = !(sblock->pagev[0]->flags & BTRFS_EXTENT_FLAG_DATA);
+	have_csum = sblock->pagev[0]->have_csum;
+	csum = sblock->pagev[0]->csum;
+	generation = sblock->pagev[0]->generation;
+	logical = sblock->pagev[0]->logical;
+	dev = sblock->pagev[0]->dev;
+
+	if (sblock->no_io_error_seen) {
+		scrub_recheck_block_checksum(fs_info, sblock, is_metadata,
+					     have_csum, csum, generation,
+					     sctx->csum_size);
+	}
+
+	if (!sblock->no_io_error_seen) {
+		spin_lock(&sctx->stat_lock);
+		sctx->stat.read_errors++;
+		spin_unlock(&sctx->stat_lock);
+		printk_ratelimited_in_rcu(KERN_ERR
+			"BTRFS: I/O error rebulding logical %llu for dev %s\n",
+			logical, rcu_str_deref(dev->name));
+	} else if (sblock->header_error || sblock->checksum_error) {
+		spin_lock(&sctx->stat_lock);
+		sctx->stat.uncorrectable_errors++;
+		spin_unlock(&sctx->stat_lock);
Quoted 8 lines

fs/btrfs/scrub.c again: · line 2222

Quoted 8 lines
fs/btrfs/scrub.c +8 -0
+		printk_ratelimited_in_rcu(KERN_ERR
+			"BTRFS: failed to rebuild valid logical %llu for dev %s\n",
+			logical, rcu_str_deref(dev->name));
+	} else {
+		scrub_write_block_to_dev_replace(sblock);
+	}
+
+	scrub_block_put(sblock);

First of all, I tested a bit more and it looks like I need this here as well:

+
+ if (sctx->is_dev_replace &&
+ atomic_read(&sctx->wr_ctx.flush_all_writes)) {
+ mutex_lock(&sctx->wr_ctx.wr_lock);
+ scrub_wr_submit(sctx);
+ mutex_unlock(&sctx->wr_ctx.wr_lock);
+ }
+

I'll resend with this added once I get to the other bug you reported.

Quoted 80 lines
Quoted 67 lines
fs/btrfs/scrub.c +15 -0
+	scrub_pending_bio_dec(sctx);
+}
+
+static void scrub_missing_raid56_pages(struct scrub_block *sblock) {
+	struct scrub_ctx *sctx = sblock->sctx;
+	struct btrfs_fs_info *fs_info = sctx->dev_root->fs_info;
+	u64 length = sblock->page_count * PAGE_SIZE;
+	u64 logical = sblock->pagev[0]->logical;
+	struct btrfs_bio *bbio;
+	struct bio *bio;
+	struct btrfs_raid_bio *rbio;
+	int ret;
+	int i;
+
+	ret = btrfs_map_sblock(fs_info, REQ_GET_READ_MIRRORS, logical,

&length,

fs/btrfs/scrub.c +49 -0
+			       &bbio, 0, 1);
+	if (ret || !bbio || !bbio->raid_map)
+		goto bbio_out;
+
+	if (WARN_ON(!sctx->is_dev_replace ||
+		    !(bbio->map_type & BTRFS_BLOCK_GROUP_RAID56_MASK))) {
+		/*
+		 * We shouldn't be scrubbing a missing device. Even for dev
+		 * replace, we should only get here for RAID 5/6. We either
+		 * managed to mount something with no mirrors remaining or
+		 * there's a bug in scrub_remap_extent()/btrfs_map_block().
+		 */
+		goto bbio_out;
+	}
+
+	bio = btrfs_io_bio_alloc(GFP_NOFS, 0);
+	if (!bio)
+		goto bbio_out;
+
+	bio->bi_iter.bi_sector = logical >> 9;
+	bio->bi_private = sblock;
+	bio->bi_end_io = scrub_missing_raid56_end_io;
+
+	rbio = raid56_alloc_missing_rbio(sctx->dev_root, bio, bbio, length);
+	if (!rbio)
+		goto rbio_out;
+
+	for (i = 0; i < sblock->page_count; i++) {
+		struct scrub_page *spage = sblock->pagev[i];
+
+		raid56_add_scrub_pages(rbio, spage->page, spage->logical);
+	}
+
+	btrfs_init_work(&sblock->work, btrfs_scrub_helper,
+			scrub_missing_raid56_worker, NULL, NULL);
+	scrub_block_get(sblock);
+	scrub_pending_bio_inc(sctx);
+	raid56_submit_missing_rbio(rbio);
+	return;
+
+rbio_out:
+	bio_put(bio);
+bbio_out:
+	btrfs_put_bbio(bbio);
+	spin_lock(&sctx->stat_lock);
+	sctx->stat.malloc_errors++;
+	spin_unlock(&sctx->stat_lock);
+}
+
 static int scrub_pages(struct scrub_ctx *sctx, u64 logical, u64 len,
 		       u64 physical, struct btrfs_device *dev, u64 flags,

fs/btrfs/scrub.c again:

Quoted 10 lines
fs/btrfs/scrub.c +3 -3
 		       u64 gen, int mirror_num, u8 *csum, int force, @@ -2227,19
+2348,23 @@ leave_nomem:
 	}
 
 	WARN_ON(sblock->page_count == 0);
-	for (index = 0; index < sblock->page_count; index++) {
-		struct scrub_page *spage = sblock->pagev[index];
-		int ret;
+	if (dev->missing) {
+		scrub_missing_raid56_pages(sblock);

Both non-raid56 and raid56 case have possibility run to here.

Quoted 5 lines

If it is a bad non-raid56 device, call scrub_missing_raid56_pages() for a non-raid56 device seems not suitable.

Since I hadn't read this patch carefully, please ignore if it is not a problem

I think the chunk from above should clarify:

+ if (WARN_ON(!sctx->is_dev_replace ||
+ !(bbio->map_type & BTRFS_BLOCK_GROUP_RAID56_MASK))) {
+ /*
+ * We shouldn't be scrubbing a missing device. Even for dev
+ * replace, we should only get here for RAID 5/6. We either
+ * managed to mount something with no mirrors remaining or
+ * there's a bug in scrub_remap_extent()/btrfs_map_block().
+ */
+ goto bbio_out;
+ }

btrfs_scrub_dev() errors out when you attempt to scrub a missing device:

	mutex_lock(&fs_info->fs_devices->device_list_mutex);
	dev = btrfs_find_device(fs_info, devid, NULL, NULL);
	if (!dev || (dev->missing && !is_dev_replace)) {
		mutex_unlock(&fs_info->fs_devices->device_list_mutex);
		return -ENODEV;
	}

So we can't get here for scrub. Now for replace, in scrub_stripe(), we try to remap the block from the missing device:

			if (is_dev_replace)
				scrub_remap_extent(fs_info, extent_logical,
						   extent_len, &extent_physical,
						   &extent_dev,
						   &extent_mirror_num);

So now let's consider what will happen with replace on the different RAID levels:

- For RAID 0, the filesystem can't even be mounted
- For RAID 1 and RAID 10, we can always remap the block to another mirror
- For RAID 5 and 6, this won't actually remap anything because there isn't another mirror! And that's what causes the bug that this patch series addresses

Also, consider that before this patch series, if we were to end up in scrub_stripe() with a missing device, we would crash later when we do a bio_add_page() on it, and I haven't seen any reports of that.

So I think that this is right, but please correct me if I'm wrong!

Quoted 2 lines

Thanks
Zhaolei

Thanks,

Signature · 1 line
Omar
#7 lore raw

RE: [PATCH 4/4] Btrfs: fix device replace of a missing RAID 5/6 device

Hi, Omar Sandoval

Quoted 131 lines

-----Original Message-----
From: Omar Sandoval [mailto:[email protected]]
Sent: Friday, June 12, 2015 4:12 PM
To: Zhao Lei
Cc: [email protected]; 'Miao Xie'; 'Philip'
Subject: Re: [PATCH 4/4] Btrfs: fix device replace of a missing RAID 5/6 device

Hi, Zhaolei,

Quoted 54 lines · Zhao Lei

Hi, Omar Sandoval

Quoted 52 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Omar Sandoval
Sent: Monday, May 11, 2015 3:58 PM
To: [email protected]
Cc: Miao Xie; Philip; Omar Sandoval
Subject: [PATCH 4/4] Btrfs: fix device replace of a missing RAID 5/6 device

The original implementation of device replace on RAID 5/6 seems to have missed support for replacing a missing device. When this is attempted, we end up calling bio_add_page() on a bio with a NULL ->bi_bdev, which crashes when we try to dereference it. This happens because
btrfs_map_block() has no choice but to return us the missing device because RAID 5/6 don't have any alternate mirrors to read from, and a missing device has a NULL bdev.

The idea implemented here is to handle the missing device case separately, which better only happen when we're replacing a missing RAID
5/6 device. We use the new BTRFS_RBIO_REBUILD_MISSING operation to reconstruct the data from parity, check it with scrub_recheck_block_checksum(), and write it out with scrub_write_block_to_dev_replace().

Reported-by Philip <[email protected]>

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=96141

Signed-off-by Omar Sandoval <[email protected]>
---
 fs/btrfs/scrub.c | 145
+++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 135 insertions(+), 10 deletions(-)
fs/btrfs/scrub.c index renamed from fs/btrfs/scrub.c +0 -0

b94694d..a13f91a 100644

fs/btrfs/scrub.c +2 -1
@@ -125,6 +125,7 @@ struct scrub_block {
 		/* It is for the data with checksum */
 		unsigned int	data_corrected:1;
 	};
+	struct btrfs_work	work;
 };
 
 /* Used for the chunks with parity stripe such RAID5/6 */ @@
-2164,6
+2165,126 @@ again:
 	return 0;
 }
Quoted 11 lines

fs/btrfs/scrub.c again:

Quoted 11 lines
fs/btrfs/scrub.c +11 -0
+static void scrub_missing_raid56_end_io(struct bio *bio, int error) {
+	struct scrub_block *sblock = bio->bi_private;
+	struct btrfs_fs_info *fs_info = sblock->sctx->dev_root->fs_info;
+
+	if (error)
+		sblock->no_io_error_seen = 0;
+
+	btrfs_queue_work(fs_info->scrub_workers, &sblock->work); }
+
+static void scrub_missing_raid56_worker(struct btrfs_work *work) {
+	struct scrub_block *sblock = container_of(work, struct scrub_block,

work);

Quoted 10 lines

fs/btrfs/scrub.c again:

Quoted 10 lines
fs/btrfs/scrub.c +10 -0
+	struct scrub_ctx *sctx = sblock->sctx;
+	struct btrfs_fs_info *fs_info = sctx->dev_root->fs_info;
+	unsigned int is_metadata;
+	unsigned int have_csum;
+	u8 *csum;
+	u64 generation;
+	u64 logical;
+	struct btrfs_device *dev;
+
+	is_metadata = !(sblock->pagev[0]->flags &

BTRFS_EXTENT_FLAG_DATA);

Quoted 23 lines
Quoted 23 lines
fs/btrfs/scrub.c +23 -0
+	have_csum = sblock->pagev[0]->have_csum;
+	csum = sblock->pagev[0]->csum;
+	generation = sblock->pagev[0]->generation;
+	logical = sblock->pagev[0]->logical;
+	dev = sblock->pagev[0]->dev;
+
+	if (sblock->no_io_error_seen) {
+		scrub_recheck_block_checksum(fs_info, sblock, is_metadata,
+					     have_csum, csum, generation,
+					     sctx->csum_size);
+	}
+
+	if (!sblock->no_io_error_seen) {
+		spin_lock(&sctx->stat_lock);
+		sctx->stat.read_errors++;
+		spin_unlock(&sctx->stat_lock);
+		printk_ratelimited_in_rcu(KERN_ERR
+			"BTRFS: I/O error rebulding logical %llu for dev %s\n",
+			logical, rcu_str_deref(dev->name));
+	} else if (sblock->header_error || sblock->checksum_error) {
+		spin_lock(&sctx->stat_lock);
+		sctx->stat.uncorrectable_errors++;
+		spin_unlock(&sctx->stat_lock);
Quoted 8 lines

fs/btrfs/scrub.c again: · line 2222

Quoted 8 lines
fs/btrfs/scrub.c +8 -0
+		printk_ratelimited_in_rcu(KERN_ERR
+			"BTRFS: failed to rebuild valid logical %llu for dev %s\n",
+			logical, rcu_str_deref(dev->name));
+	} else {
+		scrub_write_block_to_dev_replace(sblock);
+	}
+
+	scrub_block_put(sblock);

First of all, I tested a bit more and it looks like I need this here as well:

(quoted diff) +8 -0
+
+	if (sctx->is_dev_replace &&
+	    atomic_read(&sctx->wr_ctx.flush_all_writes)) {
+		mutex_lock(&sctx->wr_ctx.wr_lock);
+		scrub_wr_submit(sctx);
+		mutex_unlock(&sctx->wr_ctx.wr_lock);
+	}
+
Quoted 2 lines

I'll resend with this added once I get to the other bug you reported.

Great, I'll test after you send new version.

Quoted 129 lines
Quoted 14 lines
Quoted 14 lines
fs/btrfs/scrub.c +14 -0
+	scrub_pending_bio_dec(sctx);
+}
+
+static void scrub_missing_raid56_pages(struct scrub_block *sblock) {
+	struct scrub_ctx *sctx = sblock->sctx;
+	struct btrfs_fs_info *fs_info = sctx->dev_root->fs_info;
+	u64 length = sblock->page_count * PAGE_SIZE;
+	u64 logical = sblock->pagev[0]->logical;
+	struct btrfs_bio *bbio;
+	struct bio *bio;
+	struct btrfs_raid_bio *rbio;
+	int ret;
+	int i;
+
Quoted 8 lines

fs/btrfs/scrub.c again:

Quoted 8 lines
fs/btrfs/scrub.c +1 -0
+	ret = btrfs_map_sblock(fs_info, REQ_GET_READ_MIRRORS, logical,

&length,

fs/btrfs/scrub.c +6 -0
+			       &bbio, 0, 1);
+	if (ret || !bbio || !bbio->raid_map)
+		goto bbio_out;
+
+	if (WARN_ON(!sctx->is_dev_replace ||
+		    !(bbio->map_type &

BTRFS_BLOCK_GROUP_RAID56_MASK))) {

Quoted 38 lines
Quoted 38 lines
fs/btrfs/scrub.c +38 -0
+		/*
+		 * We shouldn't be scrubbing a missing device. Even for dev
+		 * replace, we should only get here for RAID 5/6. We either
+		 * managed to mount something with no mirrors remaining or
+		 * there's a bug in scrub_remap_extent()/btrfs_map_block().
+		 */
+		goto bbio_out;
+	}
+
+	bio = btrfs_io_bio_alloc(GFP_NOFS, 0);
+	if (!bio)
+		goto bbio_out;
+
+	bio->bi_iter.bi_sector = logical >> 9;
+	bio->bi_private = sblock;
+	bio->bi_end_io = scrub_missing_raid56_end_io;
+
+	rbio = raid56_alloc_missing_rbio(sctx->dev_root, bio, bbio, length);
+	if (!rbio)
+		goto rbio_out;
+
+	for (i = 0; i < sblock->page_count; i++) {
+		struct scrub_page *spage = sblock->pagev[i];
+
+		raid56_add_scrub_pages(rbio, spage->page, spage->logical);
+	}
+
+	btrfs_init_work(&sblock->work, btrfs_scrub_helper,
+			scrub_missing_raid56_worker, NULL, NULL);
+	scrub_block_get(sblock);
+	scrub_pending_bio_inc(sctx);
+	raid56_submit_missing_rbio(rbio);
+	return;
+
+rbio_out:
+	bio_put(bio);
+bbio_out:
+	btrfs_put_bbio(bbio);
Quoted 8 lines

fs/btrfs/scrub.c again:

Quoted 8 lines
fs/btrfs/scrub.c +5 -0
+	spin_lock(&sctx->stat_lock);
+	sctx->stat.malloc_errors++;
+	spin_unlock(&sctx->stat_lock);
+}
+
 static int scrub_pages(struct scrub_ctx *sctx, u64 logical, u64 len,
 		       u64 physical, struct btrfs_device *dev, u64 flags,
 		       u64 gen, int mirror_num, u8 *csum, int force, @@

-2227,19

Quoted 12 lines

fs/btrfs/scrub.c leave_nomem: · line 2352

Quoted 9 lines

+2348,23 @@ leave_nomem:

fs/btrfs/scrub.c +2 -3
 	}
 
 	WARN_ON(sblock->page_count == 0);
-	for (index = 0; index < sblock->page_count; index++) {
-		struct scrub_page *spage = sblock->pagev[index];
-		int ret;
+	if (dev->missing) {
+		scrub_missing_raid56_pages(sblock);

Both non-raid56 and raid56 case have possibility run to here.

Quoted 5 lines

If it is a bad non-raid56 device, call scrub_missing_raid56_pages() for a non-raid56 device seems not suitable.

Since I hadn't read this patch carefully, please ignore if it is not a problem

I think the chunk from above should clarify:

fs/btrfs/scrub.c +10 -0
+	if (WARN_ON(!sctx->is_dev_replace ||
+		    !(bbio->map_type & BTRFS_BLOCK_GROUP_RAID56_MASK))) {
+		/*
+		 * We shouldn't be scrubbing a missing device. Even for dev
+		 * replace, we should only get here for RAID 5/6. We either
+		 * managed to mount something with no mirrors remaining or
+		 * there's a bug in scrub_remap_extent()/btrfs_map_block().
+		 */
+		goto bbio_out;
+	}

btrfs_scrub_dev() errors out when you attempt to scrub a missing device:

	mutex_lock(&fs_info->fs_devices->device_list_mutex);
	dev = btrfs_find_device(fs_info, devid, NULL, NULL);
	if (!dev || (dev->missing && !is_dev_replace)) {
		mutex_unlock(&fs_info->fs_devices->device_list_mutex);
		return -ENODEV;
	}

So we can't get here for scrub. Now for replace, in scrub_stripe(), we try to remap the block from the missing device:

			if (is_dev_replace)
				scrub_remap_extent(fs_info, extent_logical,
						   extent_len, &extent_physical,
						   &extent_dev,
						   &extent_mirror_num);

So now let's consider what will happen with replace on the different RAID levels:

- For RAID 0, the filesystem can't even be mounted
- For RAID 1 and RAID 10, we can always remap the block to another mirror
- For RAID 5 and 6, this won't actually remap anything because there isn't another mirror! And that's what causes the bug that this patch series addresses

Quoted 6 lines

Also, consider that before this patch series, if we were to end up in scrub_stripe() with a missing device, we would crash later when we do a bio_add_page() on it, and I haven't seen any reports of that.

So I think that this is right, but please correct me if I'm wrong!

Thanks for your detailed explanation.
I accept your view that the code can ONLY run to above line in raid5/6.

How about add a comment like this?

if (dev->missing) {
    /*
     * this case only exist in raid5/6,
     * see comment in scrub_missing_raid56_pages() for detail.
     */
    scrub_missing_raid56_pages(sblock);
}

Thanks
Zhaolei

Quoted 6 lines
Quoted 2 lines

Thanks
Zhaolei

Thanks,
--
Omar

author
#8 lore raw
In reply to Omar Sandoval
Quoted 23 lines · Omar Sandoval

A user reported on Bugzilla that they were seeing kernel BUGs when attempting to replace a missing device on a RAID 6 array. After identifying the apparent cause of the BUG, I reached the conclusion that there wasn't a quick fix. Maybe Miao Xie can point something out that I missed, as he originally implemented device replace on RAID 5/6 :)

Patch 4 has the details, but the main problem is that we can't create bios for a missing device, so the main scrub code path isn't very useful. On RAID 5/6, since we only have one mirror for any piece of data, the missing device is the only mirror we can use. Clearly, (unless I missed something), this case needs to be handled differently.

These patches are on top of v4.1-rc2. I ran the scrub and replace xfstests and the script below, which also reproduces the original BUG.

Thanks!

Omar Sandoval (4):
Btrfs: remove misleading handling of missing device scrub
Btrfs: count devices correctly in readahead during RAID 5/6 replace
Btrfs: add RAID 5/6 BTRFS_RBIO_REBUILD_MISSING operation
Btrfs: fix device replace of a missing RAID 5/6 device

Quoted 6 lines
 fs/btrfs/raid56.c |  87 +++++++++++++++++++++++++----
 fs/btrfs/raid56.h |  10 +++-
 fs/btrfs/reada.c  |   4 +-
 fs/btrfs/scrub.c  | 164 +++++++++++++++++++++++++++++++++++++++++++++---------
 4 files changed, 225 insertions(+), 40 deletions(-)

Ping for review.

Signature · 1 line
Omar
#9 lore raw

Hi, Omar Sandoval

I tested this patchset with my script, but see general protection fault again.
NODE: kvm with virtio disk
ROOTFS: RHEL6 with btrfs-progs v4.0
KERNEL: v4.1-rc6 with 4 patchs in this patchset

Maybe my test have small different with yours,
but it seems is similar bug, could you check it?

My script:
****************
*** NOTE: update FS_DEVS in your own env before run ***
****************

#!/bin/bash

FS_DEVS=(/dev/vdd /dev/vde /dev/vdf)
PRUNE_DEV=/dev/vde
MNT=/mnt/btrfs_test_for_raid56_strip

do_cmd()

{
	echo "   $*"
	local output
	local ret
	output=$("$@" 2>&1)
	ret="$?"
	[[ "$ret" != 0 ]] && {
		echo "$output"
	}
	return "$ret"
}

mkdir -p "$MNT"
for ((i = 0; i < 10; i++)); do

	umount "$MNT" &>/dev/null

done
dmesg -c >/dev/null

echo "1: Creating filesystem"
do_cmd mkfs.btrfs -f -d raid5 -m raid5 "${FS_DEVS[@]}" || exit 1 do_cmd mount "$FS_DEVS" "$MNT" || exit 1

echo "2: Write some data"
DATA_CNT=4
for ((i = 0; i < DATA_CNT; i++)); do

	size_m="$((1<<i))"
	do_cmd dd bs=1M if=/dev/urandom of="$MNT"/file_"$i" count="$size_m" || exit 1

done

echo "3: Prune a disk in fs"
do_cmd umount "$MNT" || exit 1
do_cmd dd bs=1M if=/dev/zero of="$PRUNE_DEV" count=1 do_cmd mount -o "degraded" "$FS_DEVS" "$MNT" || exit 1

echo "4: Do scrub"
do_cmd btrfs scrub start -B "$MNT"

echo "5: Checking result"
if dmesg | grep -q 'general protection fault'; then
echo "Result Fail"
dmesg | grep -A10000 'general protection fault'
else
echo "Result OK"
fi

exit 0

Test result:

# ./bug.sh
1: Creating filesystem
   mkfs.btrfs -f -d raid5 -m raid5 /dev/vdd /dev/vde /dev/vdf
   mount /dev/vdd /mnt/btrfs_test_for_raid56_strip
2: Write some data
   dd bs=1M if=/dev/urandom of=/mnt/btrfs_test_for_raid56_strip/file_0 count=1
   dd bs=1M if=/dev/urandom of=/mnt/btrfs_test_for_raid56_strip/file_1 count=2
   dd bs=1M if=/dev/urandom of=/mnt/btrfs_test_for_raid56_strip/file_2 count=4
   dd bs=1M if=/dev/urandom of=/mnt/btrfs_test_for_raid56_strip/file_3 count=8
3: Prune a disk in fs
   umount /mnt/btrfs_test_for_raid56_strip
   dd bs=1M if=/dev/zero of=/dev/vde count=1
   mount -o degraded /dev/vdd /mnt/btrfs_test_for_raid56_strip
4: Do scrub
   btrfs scrub start -B /mnt/btrfs_test_for_raid56_strip
5: Checking result
Result Fail
[   48.899408] general protection fault: 0000 [#1] SMP
[   48.900021] Modules linked in:
[   48.900021] CPU: 0 PID: 2102 Comm: btrfs Not tainted 4.1.0-rc6_HEAD_c65b99f046843d2455aa231747b5a07a999a9f3d_ #53
[   48.900021] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5.1-0-g8936dbb-20141113_115728-nilsson.home.kraxel.org 04/01/2014
[   48.900021] task: ffff88003f6f5500 ti: ffff88003c954000 task.ti: ffff88003c954000
[   48.900021] RIP: 0010:[<ffffffff81441395>]  [<ffffffff81441395>] bio_add_page+0x15/0x70
[   48.900021] RSP: 0018:ffff88003c957718  EFLAGS: 00010246
[   48.900021] RAX: ffff88003b3ac6a8 RBX: ffff88003cb19000 RCX: 007fffc4001e58ca
[   48.900021] RDX: 000210868b41ffca RSI: ffffea0001273b60 RDI: ffff88003b3ac6a8
...

Thanks
Zhaolei

Quoted 48 lines

From: [email protected] [mailto:[email protected]] On Behalf Of Omar Sandoval
Sent: Wednesday, May 27, 2015 1:06 AM
To: [email protected]
Cc: Miao Xie; Philip
Subject: Re: [PATCH 0/4] Btrfs: RAID 5/6 missing device replace

Quoted 24 lines · Omar Sandoval

A user reported on Bugzilla that they were seeing kernel BUGs when attempting to replace a missing device on a RAID 6 array. After identifying the apparent cause of the BUG, I reached the conclusion that there wasn't a quick fix. Maybe Miao Xie can point something out that I missed, as he originally implemented device replace on RAID 5/6 :)

Patch 4 has the details, but the main problem is that we can't create bios for a missing device, so the main scrub code path isn't very useful. On RAID 5/6, since we only have one mirror for any piece of data, the missing device is the only mirror we can use. Clearly, (unless I missed something), this case needs to be handled differently.

These patches are on top of v4.1-rc2. I ran the scrub and replace xfstests and the script below, which also reproduces the original BUG.

Thanks!

Omar Sandoval (4):
Btrfs: remove misleading handling of missing device scrub
Btrfs: count devices correctly in readahead during RAID 5/6 replace
Btrfs: add RAID 5/6 BTRFS_RBIO_REBUILD_MISSING operation
Btrfs: fix device replace of a missing RAID 5/6 device

Quoted 7 lines
 fs/btrfs/raid56.c |  87 +++++++++++++++++++++++++----
fs/btrfs/raid56.h |  10 +++-
 fs/btrfs/reada.c  |   4 +-
 fs/btrfs/scrub.c  | 164
+++++++++++++++++++++++++++++++++++++++++++++---------
 4 files changed, 225 insertions(+), 40 deletions(-)

Ping for review.

--
Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

author
#10 lore raw
Quoted 11 lines · Zhao Lei

Hi, Omar Sandoval

I tested this patchset with my script, but see general protection fault again.
NODE: kvm with virtio disk
ROOTFS: RHEL6 with btrfs-progs v4.0
KERNEL: v4.1-rc6 with 4 patchs in this patchset

Maybe my test have small different with yours, but it seems is similar bug, could you check it?

Hi, Zhao Lei,

Thanks for taking a look! I was able to reproduce this and it looks like a similar bug, this time coming from the parity scrubbing code instead of the data scrubbing code. I didn't test scrubbing a degraded filesystem, only replacing a missing device, so that's probably why I didn't run into it. I'll take a closer look.

Thanks,

Signature · 1 line
Omar
#11 lore raw

Hi. I have tested this patchset in Virtual Machine.

Environment:

        Oracle VirtualBox 4.3.10 + Ubuntu 14.10 server + LVM 2.02.98
Kernel:
        4.1.0-rc7 (12 June, 2015 / Today) with your 4 patches.

Btrfs-progs: 4.0

Generic Test Procedure:
mkfs.btrfs -f -m $RAID -d $RAID && mount
cp some data
lvremove a logical volume
mount -o degraded
btrfs replace with a new device
btrfs scrub /mnt

without patches, raid 1, raid 10 are both OK, raid 5/6 cause NULL pointer dereference
and when shudown -h , get BTRFS info：
suspending dev_replace for umount
with the patches, raid 1/10/5/6 are all okey.

Quoted 21 lines · Omar Sandoval
Quoted 11 lines · Zhao Lei

Hi, Omar Sandoval

I tested this patchset with my script, but see general protection fault again.
NODE: kvm with virtio disk
ROOTFS: RHEL6 with btrfs-progs v4.0
KERNEL: v4.1-rc6 with 4 patchs in this patchset

Maybe my test have small different with yours, but it seems is similar bug, could you check it?

Hi, Zhao Lei,

Thanks for taking a look! I was able to reproduce this and it looks like a similar bug, this time coming from the parity scrubbing code instead of the data scrubbing code. I didn't test scrubbing a degraded filesystem, only replacing a missing device, so that's probably why I didn't run into it. I'll take a closer look.

Thanks,