[PATCH 0/7] Misc btrfs-progs cleanups/fixes

[PATCH 0/7] Misc btrfs-progs cleanups/fixes

[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

 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

-- 
2.7.4

[PATCH 1/7] btrfs-progs: Explictly state test.sh must be executable

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 tests/README.md | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/README.md b/tests/README.md
index 04d2ce2ab40d..c456018554cc 100644
--- a/tests/README.md
+++ b/tests/README.md
@@ -164,7 +164,9 @@ how to do mkfs, mount, unmount, check, etc.
 and join by dashes `-`. This will become the directory name, eg. `012-subvolume-sync-must-wait`.
 
 3. Write a short description of the bug and how it's tested to the comment at the
-begining of `test.sh`. You don't need to add the file to git yet.
+begining of `test.sh`. You don't need to add the file to git yet. Don't forget
+to make the file executable, otherwise it's not going to be executed by the
+infrastructure.
 
 4. Write the test commands, comment anything that's not obvious.
 
-- 
2.7.4

[PATCH 2/7] btrfs-progs: Factor out common print_device_info

[Nikolay Borisov]

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 <nborisov@suse.com>
---
 chunk-recover.c | 18 ------------------
 super-recover.c | 13 -------------
 utils.c         | 18 ++++++++++++++++++
 utils.h         |  3 +++
 4 files changed, 21 insertions(+), 31 deletions(-)

diff --git a/chunk-recover.c b/chunk-recover.c
index 4a6d7141ae80..705bcf52379d 100644
--- a/chunk-recover.c
+++ b/chunk-recover.c
@@ -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)
diff --git a/super-recover.c b/super-recover.c
index 6b80416f976a..6a13d81b0b29 100644
--- a/super-recover.c
+++ b/super-recover.c
@@ -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);
diff --git a/utils.c b/utils.c
index 524f463d3140..6c0d9fc1bebf 100644
--- a/utils.c
+++ b/utils.c
@@ -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");
+}
diff --git a/utils.h b/utils.h
index a82d46f6d7cc..e31ed34426c4 100644
--- a/utils.h
+++ b/utils.h
@@ -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.
-- 
2.7.4

[PATCH 3/7] btrfs-progs: Remove recover_get_good_super

[Nikolay Borisov]

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 <nborisov@suse.com>
---
 super-recover.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/super-recover.c b/super-recover.c
index 6a13d81b0b29..e12513100f17 100644
--- a/super-recover.c
+++ b/super-recover.c
@@ -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) {
-- 
2.7.4

[PATCH 4/7] btrfs-progs: Use list_for_each_entry in write_dev_all_supers

[Nikolay Borisov]

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

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 disk-io.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/disk-io.c b/disk-io.c
index f5edc4796619..3d8785d5bb37 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -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;
 
-- 
2.7.4

[PATCH 5/7] btrfs-progs: Document logic of btrfs_read_dev_super

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 disk-io.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/disk-io.c b/disk-io.c
index 3d8785d5bb37..40077d4919c6 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -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.
+ *
+ * This function is used by various btrfs comands to obtain a valid superblock.
+ *
+ * It's mode of operation is controlled by the @sb_bytenr and @sbdflags
+ * parameters. If SBREAD_RECOVER flag is set and @sb_bytenr is
+ * BTRFS_SUPER_INFO_OFFSET then the function reads all 3 superblock copies and
+ * returns the newest one. If SBREAD_RECOVER is not set then only a single
+ * copy is read, which one is decided by @sb_bytenr. If @sb_bytenr !=
+ * BTRFS_SUPER_INFO_OFFSET then the sbflags is effectively ignored and only a
+ * single copy is read.
+ */
 int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 			 unsigned sbflags)
 {
-- 
2.7.4

[PATCH 6/7] btrfs-progs: Add test for super block recovery

[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 <nborisov@suse.com>
---
 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

diff --git a/tests/fsck-tests/029-superblock-recovery/test.sh b/tests/fsck-tests/029-superblock-recovery/test.sh
new file mode 100755
index 000000000000..beb78d6ccc22
--- /dev/null
+++ b/tests/fsck-tests/029-superblock-recovery/test.sh
@@ -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
+run_check truncate -s 260G dev1
+loop=$(run_check_stdout $SUDO_HELPER losetup --find --show dev1)
+
+# 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"
+	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
+
+	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
+
+	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
-- 
2.7.4

[PATCH 7/7] btrfs-progs: Fix super-recovery

[Nikolay Borisov]

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 <nborisov@suse.com>
---
 super-recover.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/super-recover.c b/super-recover.c
index e12513100f17..880fd7712546 100644
--- a/super-recover.c
+++ b/super-recover.c
@@ -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
-- 
2.7.4

Re: [PATCH 1/7] btrfs-progs: Explictly state test.sh must be executable

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1067 bytes --]



On 2017年12月05日 16:39, Nikolay Borisov wrote:
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  tests/README.md | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/README.md b/tests/README.md
> index 04d2ce2ab40d..c456018554cc 100644
> --- a/tests/README.md
> +++ b/tests/README.md
> @@ -164,7 +164,9 @@ how to do mkfs, mount, unmount, check, etc.
>  and join by dashes `-`. This will become the directory name, eg. `012-subvolume-sync-must-wait`.
>  
>  3. Write a short description of the bug and how it's tested to the comment at the
> -begining of `test.sh`. You don't need to add the file to git yet.
> +begining of `test.sh`. You don't need to add the file to git yet. Don't forget
> +to make the file executable, otherwise it's not going to be executed by the
> +infrastructure.

This comment is really needed, as myself also sometimes forget to do that.

Reviewed-by: <wqu@suse.com>

Thanks,
Qu

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 2/7] btrfs-progs: Factor out common print_device_info

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3297 bytes --]



On 2017年12月05日 16:39, Nikolay Borisov wrote:
> 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 <nborisov@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  chunk-recover.c | 18 ------------------
>  super-recover.c | 13 -------------
>  utils.c         | 18 ++++++++++++++++++
>  utils.h         |  3 +++
>  4 files changed, 21 insertions(+), 31 deletions(-)
> 
> diff --git a/chunk-recover.c b/chunk-recover.c
> index 4a6d7141ae80..705bcf52379d 100644
> --- a/chunk-recover.c
> +++ b/chunk-recover.c
> @@ -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)
> diff --git a/super-recover.c b/super-recover.c
> index 6b80416f976a..6a13d81b0b29 100644
> --- a/super-recover.c
> +++ b/super-recover.c
> @@ -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);
> diff --git a/utils.c b/utils.c
> index 524f463d3140..6c0d9fc1bebf 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -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");
> +}
> diff --git a/utils.h b/utils.h
> index a82d46f6d7cc..e31ed34426c4 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -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.
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 3/7] btrfs-progs: Remove recover_get_good_super

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1572 bytes --]



On 2017年12月05日 16:39, Nikolay Borisov wrote:
> 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 <nborisov@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  super-recover.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/super-recover.c b/super-recover.c
> index 6a13d81b0b29..e12513100f17 100644
> --- a/super-recover.c
> +++ b/super-recover.c
> @@ -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) {
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 4/7] btrfs-progs: Use list_for_each_entry in write_dev_all_supers

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1259 bytes --]



On 2017年12月05日 16:39, Nikolay Borisov wrote:
> No need to use extra variable and 2 macros when we can succintly use 1.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

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
> ---
>  disk-io.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/disk-io.c b/disk-io.c
> index f5edc4796619..3d8785d5bb37 100644
> --- a/disk-io.c
> +++ b/disk-io.c
> @@ -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;
>  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 4/7] btrfs-progs: Use list_for_each_entry in write_dev_all_supers

[Nikolay Borisov]

On  5.12.2017 11:14, Qu Wenruo wrote:
> 
> 
> On 2017年12月05日 16:39, Nikolay Borisov wrote:
>> No need to use extra variable and 2 macros when we can succintly use 1.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> 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)

I will fold them then and resend this patch.

> 
> Thanks,
> Qu
>> ---
>>  disk-io.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/disk-io.c b/disk-io.c
>> index f5edc4796619..3d8785d5bb37 100644
>> --- a/disk-io.c
>> +++ b/disk-io.c
>> @@ -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;
>>  
>>
> 

Re: [PATCH 5/7] btrfs-progs: Document logic of btrfs_read_dev_super

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1646 bytes --]



On 2017年12月05日 16:39, Nikolay Borisov wrote:
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  disk-io.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/disk-io.c b/disk-io.c
> index 3d8785d5bb37..40077d4919c6 100644
> --- a/disk-io.c
> +++ b/disk-io.c
> @@ -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.
> + *
> + * This function is used by various btrfs comands to obtain a valid superblock.
> + *
> + * It's mode of operation is controlled by the @sb_bytenr and @sbdflags
> + * parameters. If SBREAD_RECOVER flag is set and @sb_bytenr is
> + * BTRFS_SUPER_INFO_OFFSET then the function reads all 3 superblock copies and
> + * returns the newest one. If SBREAD_RECOVER is not set then only a single
> + * copy is read, which one is decided by @sb_bytenr. If @sb_bytenr !=
> + * BTRFS_SUPER_INFO_OFFSET then the sbflags is effectively ignored and only a
> + * single copy is read.

Although the logic is not as straightforward, it's still acceptable and
the comment does makes it clearer.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 6/7] btrfs-progs: Add test for super block recovery

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3530 bytes --]



On 2017年12月05日 16:39, Nikolay Borisov wrote:
> 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 <nborisov@suse.com>

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

> ---
>  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
> 
> diff --git a/tests/fsck-tests/029-superblock-recovery/test.sh b/tests/fsck-tests/029-superblock-recovery/test.sh
> new file mode 100755
> index 000000000000..beb78d6ccc22
> --- /dev/null
> +++ b/tests/fsck-tests/029-superblock-recovery/test.sh
> @@ -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
> +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.

> +
> +# 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"
> +	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

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 7/7] btrfs-progs: Fix super-recovery

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1551 bytes --]



On 2017年12月05日 16:39, Nikolay Borisov wrote:
> Commit 3296d058b7ce ("btrfs-progs: super-recover: Reuse
>  btrfs_read_dev_super function")

Oh my fault.

> 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 <nborisov@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>  super-recover.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/super-recover.c b/super-recover.c
> index e12513100f17..880fd7712546 100644
> --- a/super-recover.c
> +++ b/super-recover.c
> @@ -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
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 6/7] btrfs-progs: Add test for super block recovery

[Nikolay Borisov]

On  5.12.2017 11:33, Qu Wenruo wrote:
> 
> 
> On 2017年12月05日 16:39, Nikolay Borisov wrote:
>> 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 <nborisov@suse.com>
> 
> One nitpick for the patch sequence, normally we put fix before test
> case, to avoid breaking bisect.
> 
>> ---
>>  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
>>
>> diff --git a/tests/fsck-tests/029-superblock-recovery/test.sh b/tests/fsck-tests/029-superblock-recovery/test.sh
>> new file mode 100755
>> index 000000000000..beb78d6ccc22
>> --- /dev/null
>> +++ b/tests/fsck-tests/029-superblock-recovery/test.sh
>> @@ -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
>> +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.

> 
>> +
>> +# 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"
>> +	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
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.

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


> 
> Thanks,
> Qu
> 
>> +
>> +	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
>>
> 

Re: [PATCH 6/7] btrfs-progs: Add test for super block recovery

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 5101 bytes --]



On 2017年12月05日 18:04, Nikolay Borisov wrote:
> 
> 
> On  5.12.2017 11:33, Qu Wenruo wrote:
>>
>>
>> On 2017年12月05日 16:39, Nikolay Borisov wrote:
>>> 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 <nborisov@suse.com>
>>
>> One nitpick for the patch sequence, normally we put fix before test
>> case, to avoid breaking bisect.
>>
>>> ---
>>>  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
>>>
>>> diff --git a/tests/fsck-tests/029-superblock-recovery/test.sh b/tests/fsck-tests/029-superblock-recovery/test.sh
>>> new file mode 100755
>>> index 000000000000..beb78d6ccc22
>>> --- /dev/null
>>> +++ b/tests/fsck-tests/029-superblock-recovery/test.sh
>>> @@ -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
>>> +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.

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

Some of them can be cleaned up later.

> 
>>
>>> +
>>> +# 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"
>>> +	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
> 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.
> 
>>
>>> +
>>> +	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

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

> 
> 
>>
>> Thanks,
>> Qu
>>
>>> +
>>> +	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
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 6/7] btrfs-progs: Add test for super block recovery

[Nikolay Borisov]

On  5.12.2017 13:12, Qu Wenruo wrote:
> 
> 
> On 2017年12月05日 18:04, Nikolay Borisov wrote:
>>
>>
>> On  5.12.2017 11:33, Qu Wenruo wrote:
>>>
>>>
>>> On 2017年12月05日 16:39, Nikolay Borisov wrote:
>>>> 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 <nborisov@suse.com>
>>>
>>> One nitpick for the patch sequence, normally we put fix before test
>>> case, to avoid breaking bisect.
>>>
>>>> ---
>>>>  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
>>>>
>>>> diff --git a/tests/fsck-tests/029-superblock-recovery/test.sh b/tests/fsck-tests/029-superblock-recovery/test.sh
>>>> new file mode 100755
>>>> index 000000000000..beb78d6ccc22
>>>> --- /dev/null
>>>> +++ b/tests/fsck-tests/029-superblock-recovery/test.sh
>>>> @@ -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
>>>> +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.
> 
> The same feeling when I am pointed that something can be replaced by
> wrappers in fstests.
> 
> Some of them can be cleaned up later.
> 
>>
>>>
>>>> +
>>>> +# 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"
>>>> +	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
>> 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.
>>
>>>
>>>> +
>>>> +	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
> 
> 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.

> 
> Thanks,
> Qu
> 
>>
>>
>>>
>>> Thanks,
>>> Qu
>>>
>>>> +
>>>> +	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
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

Re: [PATCH 6/7] btrfs-progs: Add test for super block recovery

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 5877 bytes --]



On 2017年12月05日 19:26, Nikolay Borisov wrote:
> 
> 
> On  5.12.2017 13:12, Qu Wenruo wrote:
>>
>>
>> On 2017年12月05日 18:04, Nikolay Borisov wrote:
>>>
>>>
>>> On  5.12.2017 11:33, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2017年12月05日 16:39, Nikolay Borisov wrote:
>>>>> 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 <nborisov@suse.com>
>>>>
>>>> One nitpick for the patch sequence, normally we put fix before test
>>>> case, to avoid breaking bisect.
>>>>
>>>>> ---
>>>>>  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
>>>>>
>>>>> diff --git a/tests/fsck-tests/029-superblock-recovery/test.sh b/tests/fsck-tests/029-superblock-recovery/test.sh
>>>>> new file mode 100755
>>>>> index 000000000000..beb78d6ccc22
>>>>> --- /dev/null
>>>>> +++ b/tests/fsck-tests/029-superblock-recovery/test.sh
>>>>> @@ -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
>>>>> +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.
>>
>> The same feeling when I am pointed that something can be replaced by
>> wrappers in fstests.
>>
>> Some of them can be cleaned up later.
>>
>>>
>>>>
>>>>> +
>>>>> +# 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"
>>>>> +	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
>>> 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.
>>>
>>>>
>>>>> +
>>>>> +	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
>>
>> 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.

Yep, could be done as another patchset.

Thanks,
Qu
> 
>>
>> Thanks,
>> Qu
>>
>>>
>>>
>>>>
>>>> Thanks,
>>>> Qu
>>>>
>>>>> +
>>>>> +	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
>>>>>
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

[PATCH v2] btrfs-progs: Replace usage of list_for_each with list_for_each_entry

[Nikolay Borisov]

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 <nborisov@suse.com>
---
 cmds-filesystem.c |  3 +--
 disk-io.c         |  4 +---
 utils.c           |  7 +------
 volumes.c         | 15 ++++-----------
 4 files changed, 7 insertions(+), 22 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 7728430f16a1..7c154589a15f 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -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;
diff --git a/disk-io.c b/disk-io.c
index f5edc4796619..3d8785d5bb37 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -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;
 
diff --git a/utils.c b/utils.c
index 6c0d9fc1bebf..65383282b512 100644
--- a/utils.c
+++ b/utils.c
@@ -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;
 	}
diff --git a/volumes.c b/volumes.c
index ce3a540578fd..2e1fb4a46465 100644
--- a/volumes.c
+++ b/volumes.c
@@ -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;
 	}
-- 
2.7.4

Re: [PATCH v2] btrfs-progs: Replace usage of list_for_each with list_for_each_entry

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4658 bytes --]



On 2017年12月07日 17:10, Nikolay Borisov wrote:
> 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 <nborisov@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  cmds-filesystem.c |  3 +--
>  disk-io.c         |  4 +---
>  utils.c           |  7 +------
>  volumes.c         | 15 ++++-----------
>  4 files changed, 7 insertions(+), 22 deletions(-)
> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 7728430f16a1..7c154589a15f 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -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;
> diff --git a/disk-io.c b/disk-io.c
> index f5edc4796619..3d8785d5bb37 100644
> --- a/disk-io.c
> +++ b/disk-io.c
> @@ -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;
>  
> diff --git a/utils.c b/utils.c
> index 6c0d9fc1bebf..65383282b512 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -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;
>  	}
> diff --git a/volumes.c b/volumes.c
> index ce3a540578fd..2e1fb4a46465 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -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;
>  	}
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 0/7] Misc btrfs-progs cleanups/fixes

[Nikolay Borisov]

On  5.12.2017 10:39, Nikolay Borisov wrote:
> 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


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

> 

Re: [PATCH 6/7] btrfs-progs: Add test for super block recovery

[David Sterba]

On Tue, Dec 05, 2017 at 12:04:49PM +0200, Nikolay Borisov wrote:
> >> --- /dev/null
> >> +++ b/tests/fsck-tests/029-superblock-recovery/test.sh
> >> @@ -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
> >> +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
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.

Re: [PATCH 6/7] btrfs-progs: Add test for super block recovery

[Nikolay Borisov]

On 23.01.2018 17:07, David Sterba wrote:
> On Tue, Dec 05, 2017 at 12:04:49PM +0200, Nikolay Borisov wrote:
>>>> --- /dev/null
>>>> +++ b/tests/fsck-tests/029-superblock-recovery/test.sh
>>>> @@ -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
>>>> +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
> 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.


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

Re: [PATCH 6/7] btrfs-progs: Add test for super block recovery

[David Sterba]

On Tue, Jan 23, 2018 at 05:29:57PM +0200, Nikolay Borisov wrote:
> On 23.01.2018 17:07, David Sterba wrote:
> > On Tue, Dec 05, 2017 at 12:04:49PM +0200, Nikolay Borisov wrote:
> >>>> --- /dev/null
> >>>> +++ b/tests/fsck-tests/029-superblock-recovery/test.sh
> >>>> @@ -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
> >>>> +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
> > 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.

That was my guess too. There are now multi-loop device helpers, this
test has been added before that.

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

I've applied your patch without changes and updated it in another commit
as it changed more than just some trivial bits.

Re: [PATCH 0/7] Misc btrfs-progs cleanups/fixes

[David Sterba]

On Mon, Jan 15, 2018 at 11:17:33AM +0200, Nikolay Borisov wrote:
> On  5.12.2017 10:39, Nikolay Borisov wrote:
> > 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
> 
> 
> Gentle ping since I'd like to get this into next btrfs-progs version,
> especially the  "fix super-recovery" patch.

Series applied, thanks. I've swapped the order of test and fix as Qu
pointed out and made some other minor fixups to the patches.