Misc btrfs-progs cleanups/fixes

Signed-off-by Nikolay Borisov <[email protected]>

---
 tests/README.md | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

tests/README.md +0 -0

index 04d2ce2ab40d..c456018554cc 100644

tests/README.md +0 -0

 4. Write the test commands, comment anything that's not obvious.
 
 

This function has been copied twice in chunk-recover and super-recover. Factor it out into utils.c/h and use it. No functional changes.

Signed-off-by Nikolay Borisov <[email protected]>

---
 chunk-recover.c | 18 ------------------
 super-recover.c | 13 -------------
 utils.c         | 18 ++++++++++++++++++
 utils.h         |  3 +++
 4 files changed, 21 insertions(+), 31 deletions(-)

chunk-recover.c +0 -18

index 4a6d7141ae80..705bcf52379d 100644
@@ -451,24 +451,6 @@ static void print_device_extent_tree(struct device_extent_tree *tree)
 	printf("\n");
 }
 
-static void print_device_info(struct btrfs_device *device, char *prefix)
-{
-	if (prefix)
-		printf("%s", prefix);
-	printf("Device: id = %llu, name = %s\n",
-	       device->devid, device->name);
-}
-
-static void print_all_devices(struct list_head *devices)
-{
-	struct btrfs_device *dev;
-
-	printf("All Devices:\n");
-	list_for_each_entry(dev, devices, dev_list)
-		print_device_info(dev, "\t");
-	printf("\n");
-}
-
 static void print_scan_result(struct recover_control *rc)
 {
 	if (!rc->verbose)

super-recover.c +0 -13

index 6b80416f976a..6a13d81b0b29 100644
@@ -186,19 +186,6 @@ static struct super_block_record *recover_get_good_super(
 	return record;
 }
 
-static void print_all_devices(struct list_head *devices)
-{
-	struct btrfs_device *dev;
-
-	printf("All Devices:\n");
-	list_for_each_entry(dev, devices, dev_list) {
-		printf("\t");
-		printf("Device: id = %llu, name = %s\n",
-			dev->devid, dev->name);
-	}
-	printf("\n");
-}
-
 static void print_super_info(struct super_block_record *record)
 {
 	printf("\t\tdevice name = %s\n", record->device_name);

utils.c +18 -0

index 524f463d3140..6c0d9fc1bebf 100644
@@ -2701,3 +2701,21 @@ unsigned long total_memory(void)
         }
         return si.totalram * si.mem_unit;       /* bytes */
 }
+
+void print_device_info(struct btrfs_device *device, char *prefix)
+{
+	if (prefix)
+		printf("%s", prefix);
+	printf("Device: id = %llu, name = %s\n",
+	       device->devid, device->name);
+}
+
+void print_all_devices(struct list_head *devices)
+{
+	struct btrfs_device *dev;
+
+	printf("All Devices:\n");
+	list_for_each_entry(dev, devices, dev_list)
+		print_device_info(dev, "\t");
+	printf("\n");
+}

utils.h +3 -0

index a82d46f6d7cc..e31ed34426c4 100644
@@ -171,6 +171,9 @@ int prefixcmp(const char *str, const char *prefix);
 
 unsigned long total_memory(void);
 
+
+void print_device_info(struct btrfs_device *device, char *prefix);
+void print_all_devices(struct list_head *devices);
 /*
  * Global program state, configurable by command line and available to
  * functions without extra context passing.

Currently getting the good super really consists of just getting the first entry on the linked list, since it's the one with the highest transid. So remove the function and just use list_first_entry directly.

Signed-off-by Nikolay Borisov <[email protected]>

---
 super-recover.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

super-recover.c +3 -10

index 6a13d81b0b29..e12513100f17 100644
@@ -177,15 +177,6 @@ static int read_fs_supers(struct btrfs_recover_superblock *recover)
 	return 0;
 }
 
-static struct super_block_record *recover_get_good_super(
-				struct btrfs_recover_superblock *recover)
-{
-	struct super_block_record *record;
-	record = list_entry(recover->good_supers.next,
-				struct super_block_record, list);
-	return record;
-}
-
 static void print_super_info(struct super_block_record *record)
 {
 	printf("\t\tdevice name = %s\n", record->device_name);

@@ -280,7 +271,9 @@ int btrfs_recover_superblocks(const char *dname,
 			goto no_recover;
 		}
 	}
-	record = recover_get_good_super(&recover);
+	record = list_first_entry(&recover.good_supers,
+				  struct super_block_record, list);
+
 	root = open_ctree(record->device_name, record->bytenr,
 			  OPEN_CTREE_RECOVER_SUPER | OPEN_CTREE_WRITES);
 	if (!root) {

No need to use extra variable and 2 macros when we can succintly use 1.

Signed-off-by Nikolay Borisov <[email protected]>

---
 disk-io.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

disk-io.c +1 -3

index f5edc4796619..3d8785d5bb37 100644
@@ -1556,7 +1556,6 @@ static int write_dev_supers(struct btrfs_fs_info *fs_info,
 
 int write_all_supers(struct btrfs_fs_info *fs_info)
 {
-	struct list_head *cur;
 	struct list_head *head = &fs_info->fs_devices->devices;
 	struct btrfs_device *dev;
 	struct btrfs_super_block *sb;

@@ -1566,8 +1565,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info)
 
 	sb = fs_info->super_copy;
 	dev_item = &sb->dev_item;
-	list_for_each(cur, head) {
-		dev = list_entry(cur, struct btrfs_device, dev_list);
+	list_for_each_entry(dev, head, dev_list) {
 		if (!dev->writeable)
 			continue;
 

Quoted 2 lines · Nikolay Borisov

No need to use extra variable and 2 macros when we can succintly use 1.

Quoted 1 line

Signed-off-by Nikolay Borisov <[email protected]>

Straightforward cleanup.

Although I found several other places with the same problem.

It would be better to address them all in one patch. (3 in volumes.c 1 in utils.c and 1 in cmds-filesystem.c)

Thanks,
Qu

Quoted 27 lines

---
 disk-io.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

disk-io.c +1 -3

index f5edc4796619..3d8785d5bb37 100644
@@ -1556,7 +1556,6 @@ static int write_dev_supers(struct btrfs_fs_info *fs_info,
 
 int write_all_supers(struct btrfs_fs_info *fs_info)
 {
-	struct list_head *cur;
 	struct list_head *head = &fs_info->fs_devices->devices;
 	struct btrfs_device *dev;
 	struct btrfs_super_block *sb;

@@ -1566,8 +1565,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info)
 
 	sb = fs_info->super_copy;
 	dev_item = &sb->dev_item;
-	list_for_each(cur, head) {
-		dev = list_entry(cur, struct btrfs_device, dev_list);
+	list_for_each_entry(dev, head, dev_list) {
 		if (!dev->writeable)
 			continue;
 

There are a couple of places where instead of the more succinct list_for_each_entry the code uses list_for_each. This results in slightly more code with no additional benefit as well as no coherent pattern. This patch makes the code uniform. No functional changes

Signed-off-by Nikolay Borisov <[email protected]>

---
 cmds-filesystem.c |  3 +--
 disk-io.c         |  4 +---
 utils.c           |  7 +------
 volumes.c         | 15 ++++-----------
 4 files changed, 7 insertions(+), 22 deletions(-)

cmds-filesystem.c +1 -2

index 7728430f16a1..7c154589a15f 100644
@@ -182,8 +182,7 @@ static int uuid_search(struct btrfs_fs_devices *fs_devices, const char *search)
 	if (!strncmp(uuidbuf, search, search_len))
 		return 1;
 
-	list_for_each(cur, &fs_devices->devices) {
-		device = list_entry(cur, struct btrfs_device, dev_list);
+	list_for_each_entry(device, &fs_devices->devices, dev_list) {
 		if ((device->label && strcmp(device->label, search) == 0) ||
 		    strcmp(device->name, search) == 0)
 			return 1;

disk-io.c +1 -3

index f5edc4796619..3d8785d5bb37 100644
@@ -1556,7 +1556,6 @@ static int write_dev_supers(struct btrfs_fs_info *fs_info,
 
 int write_all_supers(struct btrfs_fs_info *fs_info)
 {
-	struct list_head *cur;
 	struct list_head *head = &fs_info->fs_devices->devices;
 	struct btrfs_device *dev;
 	struct btrfs_super_block *sb;

@@ -1566,8 +1565,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info)
 
 	sb = fs_info->super_copy;
 	dev_item = &sb->dev_item;
-	list_for_each(cur, head) {
-		dev = list_entry(cur, struct btrfs_device, dev_list);
+	list_for_each_entry(dev, head, dev_list) {
 		if (!dev->writeable)
 			continue;
 

utils.c +1 -6

index 6c0d9fc1bebf..65383282b512 100644
@@ -804,14 +804,9 @@ static int blk_file_in_dev_list(struct btrfs_fs_devices* fs_devices,
 		const char* file)
 {
 	int ret;
-	struct list_head *head;
-	struct list_head *cur;
 	struct btrfs_device *device;
 
-	head = &fs_devices->devices;
-	list_for_each(cur, head) {
-		device = list_entry(cur, struct btrfs_device, dev_list);
-
+	list_for_each_entry(device, &fs_devices->devices, dev_list) {
 		if((ret = is_same_loop_file(device->name, file)))
 			return ret;
 	}

volumes.c +4 -11

index ce3a540578fd..2e1fb4a46465 100644
@@ -58,10 +58,8 @@ static struct btrfs_device *__find_device(struct list_head *head, u64 devid,
 					  u8 *uuid)
 {
 	struct btrfs_device *dev;
-	struct list_head *cur;
 
-	list_for_each(cur, head) {
-		dev = list_entry(cur, struct btrfs_device, dev_list);
+	list_for_each_entry(dev, head, dev_list) {
 		if (dev->devid == devid &&
 		    !memcmp(dev->uuid, uuid, BTRFS_UUID_SIZE)) {
 			return dev;

@@ -72,11 +70,9 @@ static struct btrfs_device *__find_device(struct list_head *head, u64 devid,
 
 static struct btrfs_fs_devices *find_fsid(u8 *fsid)
 {
-	struct list_head *cur;
 	struct btrfs_fs_devices *fs_devices;
 
-	list_for_each(cur, &fs_uuids) {
-		fs_devices = list_entry(cur, struct btrfs_fs_devices, list);
+	list_for_each_entry(fs_devices, &fs_uuids, list) {
 		if (memcmp(fsid, fs_devices->fsid, BTRFS_FSID_SIZE) == 0)
 			return fs_devices;
 	}

@@ -234,13 +230,10 @@ void btrfs_close_all_devices(void)
 int btrfs_open_devices(struct btrfs_fs_devices *fs_devices, int flags)
 {
 	int fd;
-	struct list_head *head = &fs_devices->devices;
-	struct list_head *cur;
 	struct btrfs_device *device;
 	int ret;
 
-	list_for_each(cur, head) {
-		device = list_entry(cur, struct btrfs_device, dev_list);
+	list_for_each_entry(device, &fs_devices->devices, dev_list) {
 		if (!device->name) {
 			printk("no name for device %llu, skip it now\n", device->devid);
 			continue;

@@ -1726,7 +1719,7 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 		return -EIO;
 	}
 	if (btrfs_chunk_sector_size(leaf, chunk) != sectorsize) {
-		error("invalid chunk sectorsize %llu", 
+		error("invalid chunk sectorsize %llu",
 		      (unsigned long long)btrfs_chunk_sector_size(leaf, chunk));
 		return -EIO;
 	}

Signed-off-by Nikolay Borisov <[email protected]>

---
 disk-io.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

disk-io.c +6 -0

index 3d8785d5bb37..40077d4919c6 100644
@@ -1419,6 +1419,23 @@ static int check_super(struct btrfs_super_block *sb, unsigned sbflags)
 	return -EIO;
 }
 
+/*
+ * btrfs_read_dev_super - read a valid superblock from a block device
+ * @fd:		file descrioptor of the device
+ * @sb:		buffer where the superblock is going to be read in
+ * @sb_bytenr:  offset of the particular superblock copie we want
+ * @sbflags:	flags controlling how the superblock is read.

disk-io.c +1 -0

+ */
 int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 			 unsigned sbflags)
 {

This functionality regressed some time ago and it was never caught. Seems no one complained of that, but to be sure add a regression test to prevent future regressions.

Signed-off-by Nikolay Borisov <[email protected]>

---
 tests/fsck-tests/029-superblock-recovery/test.sh | 64 ++++++++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100755 tests/fsck-tests/029-superblock-recovery/test.sh

tests/fsck-tests/029-superblock-recovery/test.sh new file +5 -0

new file mode 100755
index 000000000000..beb78d6ccc22
@@ -0,0 +1,64 @@
+#!/bin/bash
+# Test that any superblock is correctly detected
+# and fixed by btrfs rescue
+
+source "$TOP/tests/common"

tests/fsck-tests/029-superblock-recovery/test.sh new file +11 -0

+
+# Create the test file system.
+run_check $SUDO_HELPER "$TOP"/mkfs.btrfs -f "$loop"
+
+function check_corruption {
+	local sb_offset=$1
+	local source_sb=$2
+
+
+	# First we ensure we can mount it successfully
+	run_check $SUDO_HELPER mount $loop "$TEST_MNT"

tests/fsck-tests/029-superblock-recovery/test.sh new file +17 -0

+
+	run_check $SUDO_HELPER mount $loop "$TEST_MNT"
+	run_check $SUDO_HELPER umount "$TEST_MNT"
+}
+
+_log "Corrupting first superblock"
+check_corruption 64
+
+_log "Corrupting second superblock"
+check_corruption 65536 1
+
+_log "Corrupting third superblock"
+check_corruption 268435456 2
+
+# Cleanup
+run_check $SUDO_HELPER losetup -d "$loop"
+rm -f dev1

Quoted 5 lines · Nikolay Borisov

This functionality regressed some time ago and it was never caught. Seems no one complained of that, but to be sure add a regression test to prevent future regressions.

Signed-off-by Nikolay Borisov <[email protected]>

One nitpick for the patch sequence, normally we put fix before test case, to avoid breaking bisect.

Quoted 24 lines

---
 tests/fsck-tests/029-superblock-recovery/test.sh | 64 ++++++++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100755 tests/fsck-tests/029-superblock-recovery/test.sh

tests/fsck-tests/029-superblock-recovery/test.sh new file +13 -0

new file mode 100755
index 000000000000..beb78d6ccc22
@@ -0,0 +1,64 @@
+#!/bin/bash
+# Test that any superblock is correctly detected
+# and fixed by btrfs rescue
+
+source "$TOP/tests/common"
+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+check_prereq btrfs-select-super
+
+setup_root_helper
+
+rm -f dev1

tests/fsck-tests/029-superblock-recovery/test.sh line 15

Quoted 2 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +2 -0

+run_check truncate -s 260G dev1
+loop=$(run_check_stdout $SUDO_HELPER losetup --find --show dev1)

We have function to do it already.
prepare_test_dev will use loopback device as fallback if $TEST_DEV is not specified.
Tt can handle size well, and it also uses sparse file so no need to worry about disk usage.

Quoted 12 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +11 -0

+
+# Create the test file system.
+run_check $SUDO_HELPER "$TOP"/mkfs.btrfs -f "$loop"
+
+function check_corruption {
+	local sb_offset=$1
+	local source_sb=$2
+
+
+	# First we ensure we can mount it successfully
+	run_check $SUDO_HELPER mount $loop "$TEST_MNT"

tests/fsck-tests/029-superblock-recovery/test.sh line 36

Quoted 10 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +10 -0

+	run_check $SUDO_HELPER umount "$TEST_MNT"
+
+	# Now corrupt 1k of the superblock at sb_offset
+	run_check $SUDO_HELPER dd bs=1K count=1 seek=$(($sb_offset + 1)) if=/dev/zero of="$loop"
+
+	#if corrupting one of the sb copies, copy it over the initial superblock
+	if [ ! -z $source_sb ]; then
+		local shift_val=$((16 << $source_sb * 12 ))
+		run_check $SUDO_HELPER dd bs=1K count=4 seek=64 skip=$shift_val if="$loop" of="$loop"
+	fi

Personally speaking, corrupt 64K (1st super) then corrupt the desired copy could make the function easier.
Although we need to split the check part from this function, resulting something like:

corrupt_super 64k
corrupt_super 64m
check_super_recover

tests/fsck-tests/029-superblock-recovery/test.sh line 47

Quoted 12 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +11 -0

+
+	run_mustfail "Mounted fs with corrupted superblock" \
+		$SUDO_HELPER mount $loop "$TEST_MNT"
+
+	# Now run btrfs rescue which should fix the superblock. It uses 2
+	# to signal success of recovery use mayfail to ignore that retval
+	# but still log the output of the command
+	run_mayfail $SUDO_HELPER "$TOP"/btrfs rescue super-recover -yv "$loop"
+	if [ $? != 2 ]; then
+		_fail "couldn't rescue super"
+	fi

It's understandable to have return value other than 0 to distinguish health fs from repairable fs.
But at least let's also put this into man page.

Thanks,
Qu

Quoted 18 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +17 -0

+
+	run_check $SUDO_HELPER mount $loop "$TEST_MNT"
+	run_check $SUDO_HELPER umount "$TEST_MNT"
+}
+
+_log "Corrupting first superblock"
+check_corruption 64
+
+_log "Corrupting second superblock"
+check_corruption 65536 1
+
+_log "Corrupting third superblock"
+check_corruption 268435456 2
+
+# Cleanup
+run_check $SUDO_HELPER losetup -d "$loop"
+rm -f dev1

Quoted 36 lines · Qu Wenruo

Quoted 5 lines · Nikolay Borisov

This functionality regressed some time ago and it was never caught. Seems no one complained of that, but to be sure add a regression test to prevent future regressions.

Signed-off-by Nikolay Borisov <[email protected]>

One nitpick for the patch sequence, normally we put fix before test case, to avoid breaking bisect.

Quoted 24 lines

---
 tests/fsck-tests/029-superblock-recovery/test.sh | 64 ++++++++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100755 tests/fsck-tests/029-superblock-recovery/test.sh

tests/fsck-tests/029-superblock-recovery/test.sh new file +13 -0

new file mode 100755
index 000000000000..beb78d6ccc22
@@ -0,0 +1,64 @@
+#!/bin/bash
+# Test that any superblock is correctly detected
+# and fixed by btrfs rescue
+
+source "$TOP/tests/common"
+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+check_prereq btrfs-select-super
+
+setup_root_helper
+
+rm -f dev1

Quoted 8 lines

tests/fsck-tests/029-superblock-recovery/test.sh line 15

Quoted 2 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +2 -0

+run_check truncate -s 260G dev1
+loop=$(run_check_stdout $SUDO_HELPER losetup --find --show dev1)

We have function to do it already.
prepare_test_dev will use loopback device as fallback if $TEST_DEV is not specified.
Tt can handle size well, and it also uses sparse file so no need to worry about disk usage.

Then the test suite is not very consistent, since I copied this loopback handling from some other test.

Quoted 29 lines

Quoted 13 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +11 -0

+
+# Create the test file system.
+run_check $SUDO_HELPER "$TOP"/mkfs.btrfs -f "$loop"
+
+function check_corruption {
+	local sb_offset=$1
+	local source_sb=$2
+
+
+	# First we ensure we can mount it successfully
+	run_check $SUDO_HELPER mount $loop "$TEST_MNT"

tests/fsck-tests/029-superblock-recovery/test.sh line 36

Quoted 10 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +10 -0

+	run_check $SUDO_HELPER umount "$TEST_MNT"
+
+	# Now corrupt 1k of the superblock at sb_offset
+	run_check $SUDO_HELPER dd bs=1K count=1 seek=$(($sb_offset + 1)) if=/dev/zero of="$loop"
+
+	#if corrupting one of the sb copies, copy it over the initial superblock
+	if [ ! -z $source_sb ]; then
+		local shift_val=$((16 << $source_sb * 12 ))
+		run_check $SUDO_HELPER dd bs=1K count=4 seek=64 skip=$shift_val if="$loop" of="$loop"
+	fi

Personally speaking, corrupt 64K (1st super) then corrupt the desired copy could make the function easier.
Although we need to split the check part from this function, resulting something like:

Quoted 3 lines

corrupt_super 64k
corrupt_super 64m
check_super_recover

I'm reluctant to change this function any more. It has comments on all logical steps and is self-contained and I'd rather keep it that way.

Quoted 16 lines

tests/fsck-tests/029-superblock-recovery/test.sh line 47

Quoted 12 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +11 -0

+
+	run_mustfail "Mounted fs with corrupted superblock" \
+		$SUDO_HELPER mount $loop "$TEST_MNT"
+
+	# Now run btrfs rescue which should fix the superblock. It uses 2
+	# to signal success of recovery use mayfail to ignore that retval
+	# but still log the output of the command
+	run_mayfail $SUDO_HELPER "$TOP"/btrfs rescue super-recover -yv "$loop"
+	if [ $? != 2 ]; then
+		_fail "couldn't rescue super"
+	fi

It's understandable to have return value other than 0 to distinguish health fs from repairable fs.
But at least let's also put this into man page.

Yeah, tell me about it, super recovery actually has 5 return values:

7985fe64e0e2 ("Btrfs-progs: add super-recover to recover bad supers")

There will be five kinds of return values:

0: all supers are valid, no need to recover
1: usage or syntax error
2: recover all bad superblocks successfully
3: fail to recover bad superblocks
4: abort to recover bad superblocks

Quoted 16 lines

Thanks,
Qu

Quoted 10 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +10 -0

+
+	run_check $SUDO_HELPER mount $loop "$TEST_MNT"
+	run_check $SUDO_HELPER umount "$TEST_MNT"
+}
+
+_log "Corrupting first superblock"
+check_corruption 64
+
+_log "Corrupting second superblock"
+check_corruption 65536 1

Quoted 9 lines

tests/fsck-tests/029-superblock-recovery/test.sh line 64

Quoted 8 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +7 -0

+
+_log "Corrupting third superblock"
+check_corruption 268435456 2
+
+# Cleanup
+run_check $SUDO_HELPER losetup -d "$loop"
+rm -f dev1

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

Quoted 48 lines · Nikolay Borisov

Quoted 36 lines · Qu Wenruo

Quoted 5 lines · Nikolay Borisov

This functionality regressed some time ago and it was never caught. Seems no one complained of that, but to be sure add a regression test to prevent future regressions.

Signed-off-by Nikolay Borisov <[email protected]>

One nitpick for the patch sequence, normally we put fix before test case, to avoid breaking bisect.

Quoted 24 lines

---
 tests/fsck-tests/029-superblock-recovery/test.sh | 64 ++++++++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100755 tests/fsck-tests/029-superblock-recovery/test.sh

tests/fsck-tests/029-superblock-recovery/test.sh new file +13 -0

new file mode 100755
index 000000000000..beb78d6ccc22
@@ -0,0 +1,64 @@
+#!/bin/bash
+# Test that any superblock is correctly detected
+# and fixed by btrfs rescue
+
+source "$TOP/tests/common"
+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+check_prereq btrfs-select-super
+
+setup_root_helper
+
+rm -f dev1

Quoted 8 lines

tests/fsck-tests/029-superblock-recovery/test.sh line 15

Quoted 2 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +2 -0

+run_check truncate -s 260G dev1
+loop=$(run_check_stdout $SUDO_HELPER losetup --find --show dev1)

We have function to do it already.
prepare_test_dev will use loopback device as fallback if $TEST_DEV is not specified.
Tt can handle size well, and it also uses sparse file so no need to worry about disk usage.

Quoted 2 lines

Then the test suite is not very consistent, since I copied this loopback handling from some other test.

The same feeling when I am pointed that something can be replaced by wrappers in fstests.

Some of them can be cleaned up later.

Quoted 56 lines

Quoted 29 lines

Quoted 13 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +11 -0

+
+# Create the test file system.
+run_check $SUDO_HELPER "$TOP"/mkfs.btrfs -f "$loop"
+
+function check_corruption {
+	local sb_offset=$1
+	local source_sb=$2
+
+
+	# First we ensure we can mount it successfully
+	run_check $SUDO_HELPER mount $loop "$TEST_MNT"

tests/fsck-tests/029-superblock-recovery/test.sh line 36

Quoted 10 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +10 -0

+	run_check $SUDO_HELPER umount "$TEST_MNT"
+
+	# Now corrupt 1k of the superblock at sb_offset
+	run_check $SUDO_HELPER dd bs=1K count=1 seek=$(($sb_offset + 1)) if=/dev/zero of="$loop"
+
+	#if corrupting one of the sb copies, copy it over the initial superblock
+	if [ ! -z $source_sb ]; then
+		local shift_val=$((16 << $source_sb * 12 ))
+		run_check $SUDO_HELPER dd bs=1K count=4 seek=64 skip=$shift_val if="$loop" of="$loop"
+	fi

Personally speaking, corrupt 64K (1st super) then corrupt the desired copy could make the function easier.
Although we need to split the check part from this function, resulting something like:

Quoted 3 lines

corrupt_super 64k
corrupt_super 64m
check_super_recover

I'm reluctant to change this function any more. It has comments on all logical steps and is self-contained and I'd rather keep it that way.

Quoted 16 lines

tests/fsck-tests/029-superblock-recovery/test.sh line 47

Quoted 12 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +11 -0

+
+	run_mustfail "Mounted fs with corrupted superblock" \
+		$SUDO_HELPER mount $loop "$TEST_MNT"
+
+	# Now run btrfs rescue which should fix the superblock. It uses 2
+	# to signal success of recovery use mayfail to ignore that retval
+	# but still log the output of the command
+	run_mayfail $SUDO_HELPER "$TOP"/btrfs rescue super-recover -yv "$loop"
+	if [ $? != 2 ]; then
+		_fail "couldn't rescue super"
+	fi

It's understandable to have return value other than 0 to distinguish health fs from repairable fs.
But at least let's also put this into man page.

Yeah, tell me about it, super recovery actually has 5 return values:

7985fe64e0e2 ("Btrfs-progs: add super-recover to recover bad supers")

Quoted 7 lines

There will be five kinds of return values:

0: all supers are valid, no need to recover
1: usage or syntax error
2: recover all bad superblocks successfully
3: fail to recover bad superblocks
4: abort to recover bad superblocks

Since we all agree that the return value is a messy,
maybe we could just simplify it to 0 (all valid or successful recover) and 1 (the rest)?

Thanks,
Qu

Quoted 30 lines

Quoted 16 lines

Thanks,
Qu

Quoted 10 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +10 -0

+
+	run_check $SUDO_HELPER mount $loop "$TEST_MNT"
+	run_check $SUDO_HELPER umount "$TEST_MNT"
+}
+
+_log "Corrupting first superblock"
+check_corruption 64
+
+_log "Corrupting second superblock"
+check_corruption 65536 1

Quoted 9 lines

tests/fsck-tests/029-superblock-recovery/test.sh line 64

Quoted 8 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +7 -0

+
+_log "Corrupting third superblock"
+check_corruption 268435456 2
+
+# Cleanup
+run_check $SUDO_HELPER losetup -d "$loop"
+rm -f dev1

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

Quoted 123 lines · Qu Wenruo

Quoted 48 lines · Nikolay Borisov

Quoted 36 lines · Qu Wenruo

Quoted 5 lines · Nikolay Borisov

This functionality regressed some time ago and it was never caught. Seems no one complained of that, but to be sure add a regression test to prevent future regressions.

Signed-off-by Nikolay Borisov <[email protected]>

One nitpick for the patch sequence, normally we put fix before test case, to avoid breaking bisect.

Quoted 24 lines

---
 tests/fsck-tests/029-superblock-recovery/test.sh | 64 ++++++++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100755 tests/fsck-tests/029-superblock-recovery/test.sh

tests/fsck-tests/029-superblock-recovery/test.sh new file +13 -0

new file mode 100755
index 000000000000..beb78d6ccc22
@@ -0,0 +1,64 @@
+#!/bin/bash
+# Test that any superblock is correctly detected
+# and fixed by btrfs rescue
+
+source "$TOP/tests/common"
+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+check_prereq btrfs-select-super
+
+setup_root_helper
+
+rm -f dev1

Quoted 8 lines

tests/fsck-tests/029-superblock-recovery/test.sh line 15

Quoted 2 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +2 -0

+run_check truncate -s 260G dev1
+loop=$(run_check_stdout $SUDO_HELPER losetup --find --show dev1)

We have function to do it already.
prepare_test_dev will use loopback device as fallback if $TEST_DEV is not specified.
Tt can handle size well, and it also uses sparse file so no need to worry about disk usage.

Quoted 2 lines

Then the test suite is not very consistent, since I copied this loopback handling from some other test.

The same feeling when I am pointed that something can be replaced by wrappers in fstests.

Some of them can be cleaned up later.

Quoted 63 lines

Quoted 29 lines

Quoted 13 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +11 -0

+
+# Create the test file system.
+run_check $SUDO_HELPER "$TOP"/mkfs.btrfs -f "$loop"
+
+function check_corruption {
+	local sb_offset=$1
+	local source_sb=$2
+
+
+	# First we ensure we can mount it successfully
+	run_check $SUDO_HELPER mount $loop "$TEST_MNT"

tests/fsck-tests/029-superblock-recovery/test.sh line 36

Quoted 10 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +10 -0

+	run_check $SUDO_HELPER umount "$TEST_MNT"
+
+	# Now corrupt 1k of the superblock at sb_offset
+	run_check $SUDO_HELPER dd bs=1K count=1 seek=$(($sb_offset + 1)) if=/dev/zero of="$loop"
+
+	#if corrupting one of the sb copies, copy it over the initial superblock
+	if [ ! -z $source_sb ]; then
+		local shift_val=$((16 << $source_sb * 12 ))
+		run_check $SUDO_HELPER dd bs=1K count=4 seek=64 skip=$shift_val if="$loop" of="$loop"
+	fi

Personally speaking, corrupt 64K (1st super) then corrupt the desired copy could make the function easier.
Although we need to split the check part from this function, resulting something like:

Quoted 3 lines

corrupt_super 64k
corrupt_super 64m
check_super_recover

I'm reluctant to change this function any more. It has comments on all logical steps and is self-contained and I'd rather keep it that way.

Quoted 16 lines

tests/fsck-tests/029-superblock-recovery/test.sh line 47

Quoted 12 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +11 -0

+
+	run_mustfail "Mounted fs with corrupted superblock" \
+		$SUDO_HELPER mount $loop "$TEST_MNT"
+
+	# Now run btrfs rescue which should fix the superblock. It uses 2
+	# to signal success of recovery use mayfail to ignore that retval
+	# but still log the output of the command
+	run_mayfail $SUDO_HELPER "$TOP"/btrfs rescue super-recover -yv "$loop"
+	if [ $? != 2 ]; then
+		_fail "couldn't rescue super"
+	fi

It's understandable to have return value other than 0 to distinguish health fs from repairable fs.
But at least let's also put this into man page.

Yeah, tell me about it, super recovery actually has 5 return values:

7985fe64e0e2 ("Btrfs-progs: add super-recover to recover bad supers")

There will be five kinds of return values:

0: all supers are valid, no need to recover
1: usage or syntax error
2: recover all bad superblocks successfully
3: fail to recover bad superblocks
4: abort to recover bad superblocks

Quoted 3 lines

Since we all agree that the return value is a messy,
maybe we could just simplify it to 0 (all valid or successful recover) and 1 (the rest)?

I have no objection, but it's out of the scope of the current series.

Quoted 36 lines

Thanks,
Qu

Quoted 30 lines

Quoted 16 lines

Thanks,
Qu

Quoted 10 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +10 -0

+
+	run_check $SUDO_HELPER mount $loop "$TEST_MNT"
+	run_check $SUDO_HELPER umount "$TEST_MNT"
+}
+
+_log "Corrupting first superblock"
+check_corruption 64
+
+_log "Corrupting second superblock"
+check_corruption 65536 1

Quoted 9 lines

tests/fsck-tests/029-superblock-recovery/test.sh line 64

Quoted 8 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +7 -0

+
+_log "Corrupting third superblock"
+check_corruption 268435456 2
+
+# Cleanup
+run_check $SUDO_HELPER losetup -d "$loop"
+rm -f dev1

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

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

Quoted 16 lines

Quoted 16 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +13 -0

@@ -0,0 +1,64 @@
+#!/bin/bash
+# Test that any superblock is correctly detected
+# and fixed by btrfs rescue
+
+source "$TOP/tests/common"
+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+check_prereq btrfs-select-super
+
+setup_root_helper
+
+rm -f dev1

Quoted 8 lines

tests/fsck-tests/029-superblock-recovery/test.sh line 15

Quoted 2 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +2 -0

+run_check truncate -s 260G dev1
+loop=$(run_check_stdout $SUDO_HELPER losetup --find --show dev1)

We have function to do it already.
prepare_test_dev will use loopback device as fallback if $TEST_DEV is not specified.
Tt can handle size well, and it also uses sparse file so no need to worry about disk usage.

Then the test suite is not very consistent, since I copied this loopback handling from some other test.

From which one? Some tests have special needs and may set up the loop

Quoted 25 lines · Nikolay Borisov

Quoted 24 lines

Quoted 8 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +5 -0

@@ -0,0 +1,64 @@
+#!/bin/bash
+# Test that any superblock is correctly detected
+# and fixed by btrfs rescue
+
+source "$TOP/tests/common"

tests/fsck-tests/029-superblock-recovery/test.sh line 15

Quoted 10 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +10 -0

+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+check_prereq btrfs-select-super
+
+setup_root_helper
+
+rm -f dev1
+run_check truncate -s 260G dev1
+loop=$(run_check_stdout $SUDO_HELPER losetup --find --show dev1)

We have function to do it already.
prepare_test_dev will use loopback device as fallback if $TEST_DEV is not specified.
Tt can handle size well, and it also uses sparse file so no need to worry about disk usage.

Quoted 2 lines

Then the test suite is not very consistent, since I copied this loopback handling from some other test.

From which one? Some tests have special needs and may set up the loop device on their own. The rest should use the helpers in tests/common and I don't see anything that would not be provided by them.

The consistency in the testsuite is being improved as we find and identify the patterns worth wrapping into helpers so you may see some code duplication.

Quoted 29 lines · David Sterba

Quoted 27 lines · Nikolay Borisov

Quoted 16 lines

Quoted 16 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +13 -0

@@ -0,0 +1,64 @@
+#!/bin/bash
+# Test that any superblock is correctly detected
+# and fixed by btrfs rescue
+
+source "$TOP/tests/common"
+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+check_prereq btrfs-select-super
+
+setup_root_helper
+
+rm -f dev1

Quoted 8 lines

tests/fsck-tests/029-superblock-recovery/test.sh line 15

Quoted 2 lines

tests/fsck-tests/029-superblock-recovery/test.sh new file +2 -0

+run_check truncate -s 260G dev1
+loop=$(run_check_stdout $SUDO_HELPER losetup --find --show dev1)

We have function to do it already.
prepare_test_dev will use loopback device as fallback if $TEST_DEV is not specified.
Tt can handle size well, and it also uses sparse file so no need to worry about disk usage.

Then the test suite is not very consistent, since I copied this loopback handling from some other test.

Quoted 3 lines

From which one? Some tests have special needs and may set up the loop device on their own. The rest should use the helpers in tests/common and I don't see anything that would not be provided by them.

Looking back at the code it seems it could have been from 021-image-multi-devices.

Anyway, I haven't resubmitted the sequence since I was waiting for feedback. Qu already mentioned this could be refactored.

Commit 3296d058b7ce ("btrfs-progs: super-recover: Reuse
btrfs_read_dev_super function")

changed the logic when a superblock
is added to the bad block list to depend on -EIO. However currently btrfs_read_dev_super doesn't return -EIO when the fist super block is broken. Instead it returns -1. This causes the super-recovery logic to miss the fact that the first super block is completely broken.

Fix this by considering any error code from btrfs_read_dev_super other than -ENOENT to mean that the super block is corrupted. -ENOENT means that the superblock copy is not part of the fs i.e. it's smaller than the offset of the block. This can only occur for the 2nd copy at 256gb mark.

Signed-off-by Nikolay Borisov <[email protected]>

---
 super-recover.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

super-recover.c +1 -1

index e12513100f17..880fd7712546 100644
@@ -136,7 +136,7 @@ read_dev_supers(char *filename, struct btrfs_recover_superblock *recover)
 			max_gen = btrfs_super_generation(sb);
 			if (max_gen > recover->max_generation)
 				recover->max_generation = max_gen;
-		} else if (ret == -EIO){
+		} else if (ret != -ENOENT){
 			/*
 			 * Skip superblock which doesn't exist, only adds
 			 * really corrupted superblock

Here is a series doing some minor code cleanups, hopefully making the code more idiomatic and easier to follow. They should be pretty low-risk and introduce no functional changes (patches 1-5).

The the last 2 patches deal with a regression of btrfs rescue super-recovery. Turns out this was broken for sometime. Patch 6 introduces a regression test which hopefully will prevent further occurences and patch 7 fixes the actual bug.

Nikolay Borisov (7):
btrfs-progs: Explictly state test.sh must be executable
btrfs-progs: Factor out common print_device_info
btrfs-progs: Remove recover_get_good_super
btrfs-progs: Use list_for_each_entry in write_dev_all_supers
btrfs-progs: Document logic of btrfs_read_dev_super
btrfs-progs: Add test for super block recovery
btrfs-progs: Fix super-recovery

 chunk-recover.c                                  | 18 -------
 disk-io.c                                        | 21 ++++++--
 super-recover.c                                  | 28 ++---------
 tests/README.md                                  |  4 +-
 tests/fsck-tests/029-superblock-recovery/test.sh | 64 ++++++++++++++++++++++++
 utils.c                                          | 18 +++++++
 utils.h                                          |  3 ++
 7 files changed, 110 insertions(+), 46 deletions(-)
 create mode 100755 tests/fsck-tests/029-superblock-recovery/test.sh

Quoted 18 lines · Nikolay Borisov

Here is a series doing some minor code cleanups, hopefully making the code more idiomatic and easier to follow. They should be pretty low-risk and introduce no functional changes (patches 1-5).

The the last 2 patches deal with a regression of btrfs rescue super-recovery. Turns out this was broken for sometime. Patch 6 introduces a regression test which hopefully will prevent further occurences and patch 7 fixes the actual bug.

Nikolay Borisov (7):
btrfs-progs: Explictly state test.sh must be executable
btrfs-progs: Factor out common print_device_info
btrfs-progs: Remove recover_get_good_super
btrfs-progs: Use list_for_each_entry in write_dev_all_supers
btrfs-progs: Document logic of btrfs_read_dev_super
btrfs-progs: Add test for super block recovery
btrfs-progs: Fix super-recovery

Quoted 9 lines

 chunk-recover.c                                  | 18 -------
 disk-io.c                                        | 21 ++++++--
 super-recover.c                                  | 28 ++---------
 tests/README.md                                  |  4 +-
 tests/fsck-tests/029-superblock-recovery/test.sh | 64 ++++++++++++++++++++++++
 utils.c                                          | 18 +++++++
 utils.h                                          |  3 ++
 7 files changed, 110 insertions(+), 46 deletions(-)
 create mode 100755 tests/fsck-tests/029-superblock-recovery/test.sh

Gentle ping since I'd like to get this into next btrfs-progs version, especially the "fix super-recovery" patch.