[PATCH 00/10 v2] fixes and preparatory related to metadata_uuid

[PATCH 00/10 v2] fixes and preparatory related to metadata_uuid

[Anand Jain]

v2:
Consolidating preparatory, cleanups and bug fixes, these patches can be
merged independently.

Addressed the comments received on some of the patches, relevant changes
are mentioned in the individual patch.

The patches here were part of an earlier patchset...
  [PATCH 0/2] btrfs-progs: fix dump-super metadata_uuid
  [PATCH 00/11] btrfs-progs: fix bugs and CHANGING_FSID_V2 flag
  [PATCH 00/10] btrfs-progs: check and tune: add device and noscan options

Anand Jain (10):
  btrfs-progs: dump-super print actual metadata_uuid value
  btrfs-progs: tests: return metadata_uuid or fsid as per METADATA_UUID
    flag
  btrfs-progs: fix duplicate missing device
  btrfs-progs: track missing device counter
  btrfs-progs: tune: check for missing device
  btrfs-progs: track changing_fsid flag in fs_devices
  btrfs-progs: track num_devices per fs_devices
  btrfs-progs: track total_devs in fs devices
  btrfs-progs: track active metadata_uuid per fs_devices
  btrfs-progs: tune: consolidate return goto free-out

 kernel-shared/print-tree.c                 |  8 +---
 kernel-shared/volumes.c                    | 23 +++++++++--
 kernel-shared/volumes.h                    |  6 +++
 tests/misc-tests/034-metadata-uuid/test.sh | 24 +++++++++--
 tune/change-uuid.c                         |  4 +-
 tune/main.c                                | 47 +++++++++++++++++-----
 6 files changed, 86 insertions(+), 26 deletions(-)

-- 
2.38.1

[PATCH 01/10 v2] btrfs-progs: dump-super print actual metadata_uuid value

[Anand Jain]

The function btrfs_print_superblock() prints all members of the
superblock as they are, except for the superblock::metadata_uuid.
If the METADATA_UUID flag is unset, it prints the fsid instead of
zero as in the superblock::metadata_uuid.

Perhaps this was done because to match with the kernel
btrfs_fs_devices::metadata_uuid value as it also sets fsid if
METADATA_UUID flag is unset.

However, the actual superblock::metadata_uuid is always zero if the
METADATA_UUID flag is unset. Just to mention the kernel does not alter
the superblock::metadata_uuid value any time.

The dump-super printing fsid instead of zero, is confusing because we
generally expect dump_super to print the superblock value in the raw
formet without modification.

Fix this by printing the actual metadata_uuid value instead of fsid.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 kernel-shared/print-tree.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
index f97148c47b5a..1c398c3dacab 100644
--- a/kernel-shared/print-tree.c
+++ b/kernel-shared/print-tree.c
@@ -2006,12 +2006,8 @@ void btrfs_print_superblock(struct btrfs_super_block *sb, int full)
 
 	uuid_unparse(sb->fsid, buf);
 	printf("fsid\t\t\t%s\n", buf);
-	if (metadata_uuid_present) {
-		uuid_unparse(sb->metadata_uuid, buf);
-		printf("metadata_uuid\t\t%s\n", buf);
-	} else {
-		printf("metadata_uuid\t\t%s\n", buf);
-	}
+	uuid_unparse(sb->metadata_uuid, buf);
+	printf("metadata_uuid\t\t%s\n", buf);
 
 	printf("label\t\t\t");
 	s = sb->label;
-- 
2.38.1

[PATCH 02/10] btrfs-progs: tests: return metadata_uuid or fsid as per METADATA_UUID flag

[Anand Jain]

Commit
  btrfs-progs: dump-super print actual metadata_uuid value

fixed the value of the super_block::metadata_uuid to be printed as it
is, without tweaking it depending on the METADATA_UUID flag.

Apply similar counter tweak in the common helper functions used to read
the metadata_uuid so that test-cases still be successful.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tests/misc-tests/034-metadata-uuid/test.sh | 24 +++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/tests/misc-tests/034-metadata-uuid/test.sh b/tests/misc-tests/034-metadata-uuid/test.sh
index 88e468d96748..ab1e24637d1a 100755
--- a/tests/misc-tests/034-metadata-uuid/test.sh
+++ b/tests/misc-tests/034-metadata-uuid/test.sh
@@ -15,6 +15,19 @@ if [ ! -f /sys/fs/btrfs/features/metadata_uuid ] ; then
 	_not_run "METADATA_UUID feature not supported"
 fi
 
+has_metadata_uuid_flag() {
+	local dev="$1"
+
+	run_check_stdout $SUDO_HELPER "$TOP/btrfs" inspect-internal \
+		dump-super "$dev" | egrep -q METADATA_UUID
+
+	if [ $? -eq 0 ]; then
+		echo true
+	else
+		echo false
+	fi
+}
+
 read_fsid() {
 	local dev="$1"
 
@@ -24,9 +37,14 @@ read_fsid() {
 
 read_metadata_uuid() {
 	local dev="$1"
-
-	echo $(run_check_stdout $SUDO_HELPER "$TOP/btrfs" inspect-internal \
-		dump-super "$dev" | awk '/metadata_uuid/ {print $2}')
+	local flag=$(has_metadata_uuid_flag $dev)
+
+	if [ "$flag" == "true" ]; then
+		echo $(run_check_stdout $SUDO_HELPER "$TOP/btrfs" inspect-internal \
+			dump-super "$dev" | awk '/metadata_uuid/ {print $2}')
+	else
+		read_fsid $dev
+	fi
 }
 
 check_btrfstune() {
-- 
2.38.1

[PATCH 03/10] btrfs-progs: fix duplicate missing device

[Anand Jain]

btrfs_read_sys_array() adds a missing device with its devid only.
So, any subsequent btrfs_find_device(..devid, uuid) call in the
open_ctree_fd() still fails resulting in addition of duplicate
struct btrfs_device to the list, as shown below.

open_ctree_fd()
::
 btrfs_setup_chunk_tree_and_device_map()
  btrfs_read_sys_array()
   read_one_chunk()
    btrfs_find_device()
    fill_missing_device() <--- dev_uuid wasn't updated
    list_add()
  btrfs_read_chunk_tree
    read_one_dev()
       btrfs_find_device(..,devid, dev_uuid,..); <-- fails
       list_add
       fill_device_from_item(leaf, dev_item, device);

This ends up having two btrfs_device for the same missing devid.

For example:

(btrfstune is compiled with a boilerplate code to dump the device list,
as in the mailing list).

Before:

There are two device with devid 1.

$ btrfstune -m ./disk4.raw
warning, device 1 is missing
[fsid: 95bbc163-671a-4a0a-bd34-03a65e4b338c]
	size:			120
	metadata_uuid:		95bbc163-671a-4a0a-bd34-03a65e4b338c
	fs_devs_addr:		0xdb64e0
	total_rw_bytes:		1048576000
	[[UUID: 703a4cac-bca0-47e4-98f6-55e530800172]]
		sb_flags: 0x0
		sb_incompact_flags: 0x0
		dev_addr:	0xdb69a0
		device:		(null)
		devid:		1
		generation:	0
		total_bytes:	524288000
		bytes_used:	127926272
		type:		0
		io_align:	4096
		io_width:	4096
		sector_size:	4096
	[[UUID: 00000000-0000-0000-0000-000000000000]]
		sb_flags: 0x0
		sb_incompact_flags: 0x0
		dev_addr:	0xdb3060
		device:		(null)
		devid:		1
		generation:	0
		total_bytes:	0
		bytes_used:	0
		type:		0
		io_align:	0
		io_width:	0
		sector_size:	0
	[[UUID: 1db7564f-e53b-46ff-8a33-a8b2d00d86d1]]
		sb_flags: 0x1000000001
		sb_incompact_flags: 0x141
		dev_addr:	0xdb6e90
		device:		/tdev/disk4.raw
		devid:		2
		generation:	6
		total_bytes:	524288000
		bytes_used:	127926272
		type:		0
		io_align:	4096
		io_width:	4096
		sector_size:	4096

Fix this issue by adding the UUID to the missing device created in
fill_missing_device().

After:

$ btrfstune -m /tdev/disk4.raw
warning, device 1 is missing
[fsid: 95bbc163-671a-4a0a-bd34-03a65e4b338c]
        size:                   120
        metadata_uuid:          95bbc163-671a-4a0a-bd34-03a65e4b338c
        fs_devs_addr:           0x161f380
        total_rw_bytes:         1048576000
        [[UUID: 703a4cac-bca0-47e4-98f6-55e530800172]]
                sb_flags: 0x0
                sb_incompact_flags: 0x0
                dev_addr:       0x161c060
                device:         (null)
                devid:          1
                generation:     0
                total_bytes:    524288000
                bytes_used:     127926272
                type:           0
                io_align:       4096
                io_width:       4096
                sector_size:    4096
        [[UUID: 1db7564f-e53b-46ff-8a33-a8b2d00d86d1]]
                sb_flags: 0x1000000001
                sb_incompact_flags: 0x141
                dev_addr:       0x161fe90
                device:         /tdev/disk4.raw
                devid:          2
                generation:     6
                total_bytes:    524288000
                bytes_used:     127926272
                type:           0
                io_align:       4096
                io_width:       4096
                sector_size:    4096

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: Update change log

 kernel-shared/volumes.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
index 81abda3f7d1c..92282524867d 100644
--- a/kernel-shared/volumes.c
+++ b/kernel-shared/volumes.c
@@ -2077,12 +2077,13 @@ int btrfs_chunk_readonly(struct btrfs_fs_info *fs_info, u64 chunk_offset)
 	return readonly;
 }
 
-static struct btrfs_device *fill_missing_device(u64 devid)
+static struct btrfs_device *fill_missing_device(u64 devid, u8 *uuid)
 {
 	struct btrfs_device *device;
 
 	device = kzalloc(sizeof(*device), GFP_NOFS);
 	device->devid = devid;
+	memcpy(device->uuid, uuid, BTRFS_UUID_SIZE);
 	device->fd = -1;
 	return device;
 }
@@ -2150,7 +2151,7 @@ static int read_one_chunk(struct btrfs_fs_info *fs_info, struct btrfs_key *key,
 		map->stripes[i].dev = btrfs_find_device(fs_info, devid, uuid,
 							NULL);
 		if (!map->stripes[i].dev) {
-			map->stripes[i].dev = fill_missing_device(devid);
+			map->stripes[i].dev = fill_missing_device(devid, uuid);
 			printf("warning, device %llu is missing\n",
 			       (unsigned long long)devid);
 			list_add(&map->stripes[i].dev->dev_list,
-- 
2.38.1

[PATCH 04/10] btrfs-progs: track missing device counter

[Anand Jain]

Maintain the btrfs_fs_devices::missing counter to track the number of
missing devices.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: Maintains parity with the kernel.

 kernel-shared/volumes.c | 2 ++
 kernel-shared/volumes.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
index 92282524867d..54ef553e8230 100644
--- a/kernel-shared/volumes.c
+++ b/kernel-shared/volumes.c
@@ -2156,6 +2156,7 @@ static int read_one_chunk(struct btrfs_fs_info *fs_info, struct btrfs_key *key,
 			       (unsigned long long)devid);
 			list_add(&map->stripes[i].dev->dev_list,
 				 &fs_info->fs_devices->devices);
+			fs_info->fs_devices->missing_devices++;
 		}
 
 	}
@@ -2258,6 +2259,7 @@ static int read_one_dev(struct btrfs_fs_info *fs_info,
 		device->fd = -1;
 		list_add(&device->dev_list,
 			 &fs_info->fs_devices->devices);
+		fs_info->fs_devices->missing_devices++;
 	}
 
 	fill_device_from_item(leaf, dev_item, device);
diff --git a/kernel-shared/volumes.h b/kernel-shared/volumes.h
index ab5ac40269bb..24b52c86562d 100644
--- a/kernel-shared/volumes.h
+++ b/kernel-shared/volumes.h
@@ -88,6 +88,7 @@ struct btrfs_fs_devices {
 	u64 latest_trans;
 	u64 lowest_devid;
 
+	u64 missing_devices;
 	u64 total_rw_bytes;
 
 	int latest_bdev;
-- 
2.38.1

[PATCH 05/10] btrfs-progs: tune: check for missing device

[Anand Jain]

If btrfstune is executed on a filesystem that contains a missing device,
the command will now fail.

It is ok fail when any of the options supported by btrfstune are used.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: adds comment

 tune/main.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tune/main.c b/tune/main.c
index 73d09c34a897..8febd0d6479c 100644
--- a/tune/main.c
+++ b/tune/main.c
@@ -287,6 +287,23 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[])
 		return 1;
 	}
 
+	/*
+	 * As we increment the generation number here, it is unlikely that the
+	 * missing device will have a higher generation number, and the kernel
+	 * won't use its sb for any further commits, even if it is not missing
+	 * during mount. So, we allow all operations except for -m, -M, -u, and
+	 * -U, as these operations also change the fsid/metadata_uuid, which are
+	 *  key parameters for assembling the devices and need to be consistent
+	 *  on all the partner devices.
+	 */
+	if ((change_metadata_uuid || random_fsid || new_fsid_str) &&
+	     root->fs_info->fs_devices->missing_devices) {
+		error("missing %lld device(s), failing the command",
+		       root->fs_info->fs_devices->missing_devices);
+		ret = 1;
+		goto out;
+	}
+
  	if (to_bg_tree) {
 		if (to_extent_tree) {
 			error("option --convert-to-block-group-tree conflicts with --convert-from-block-group-tree");
-- 
2.38.1

[PATCH 06/10] btrfs-progs: track changing_fsid flag in fs_devices

[Anand Jain]

To prepare for reuniting separated devices due to an incomplete
fsid change task, consolidate and monitor the per device's
changing_fsid flag in the struct btrfs_fs_devices.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 kernel-shared/volumes.c | 9 +++++++++
 kernel-shared/volumes.h | 2 ++
 tune/change-uuid.c      | 4 +---
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
index 54ef553e8230..375ceaa93de4 100644
--- a/kernel-shared/volumes.c
+++ b/kernel-shared/volumes.c
@@ -342,6 +342,9 @@ static int device_list_add(const char *path,
 	u64 devid = btrfs_stack_device_id(&disk_super->dev_item);
 	bool metadata_uuid = (btrfs_super_incompat_flags(disk_super) &
 		BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
+	bool changing_fsid = (btrfs_super_flags(disk_super) &
+			      (BTRFS_SUPER_FLAG_CHANGING_FSID |
+			       BTRFS_SUPER_FLAG_CHANGING_FSID_V2));
 
 	if (metadata_uuid)
 		fs_devices = find_fsid(disk_super->fsid,
@@ -424,6 +427,12 @@ static int device_list_add(const char *path,
                 device->name = name;
         }
 
+	/*
+	 * If changing_fsid the fs_devices will still hold the status from
+	 * the other devices.
+	 */
+	if (changing_fsid)
+		fs_devices->changing_fsid = true;
 
 	if (found_transid > fs_devices->latest_trans) {
 		fs_devices->latest_devid = devid;
diff --git a/kernel-shared/volumes.h b/kernel-shared/volumes.h
index 24b52c86562d..786add2c879d 100644
--- a/kernel-shared/volumes.h
+++ b/kernel-shared/volumes.h
@@ -99,6 +99,8 @@ struct btrfs_fs_devices {
 	struct btrfs_fs_devices *seed;
 
 	enum btrfs_chunk_allocation_policy chunk_alloc_policy;
+
+	bool changing_fsid;
 };
 
 struct btrfs_bio_stripe {
diff --git a/tune/change-uuid.c b/tune/change-uuid.c
index cbfc8634168b..30cfb145459f 100644
--- a/tune/change-uuid.c
+++ b/tune/change-uuid.c
@@ -214,10 +214,8 @@ int check_unfinished_fsid_change(struct btrfs_fs_info *fs_info,
 				 uuid_t fsid_ret, uuid_t chunk_id_ret)
 {
 	struct btrfs_root *tree_root = fs_info->tree_root;
-	u64 flags = btrfs_super_flags(fs_info->super_copy);
 
-	if (flags & (BTRFS_SUPER_FLAG_CHANGING_FSID |
-		     BTRFS_SUPER_FLAG_CHANGING_FSID_V2)) {
+	if (fs_info->fs_devices->changing_fsid) {
 		memcpy(fsid_ret, fs_info->super_copy->fsid, BTRFS_FSID_SIZE);
 		read_extent_buffer(tree_root->node, chunk_id_ret,
 				btrfs_header_chunk_tree_uuid(tree_root->node),
-- 
2.38.1

[PATCH 07/10] btrfs-progs: track num_devices per fs_devices

[Anand Jain]

Similar to the kernel we need to track the number of devices scanned
per fs_devices. A preparation patch to fix incomplete fsid changing.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: Maintains parity with the kernel.

 kernel-shared/volumes.c | 1 +
 kernel-shared/volumes.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
index 375ceaa93de4..88d6c51e3e7e 100644
--- a/kernel-shared/volumes.c
+++ b/kernel-shared/volumes.c
@@ -405,6 +405,7 @@ static int device_list_add(const char *path,
 			btrfs_stack_device_bytes_used(&disk_super->dev_item);
 		list_add(&device->dev_list, &fs_devices->devices);
 		device->fs_devices = fs_devices;
+		fs_devices->num_devices++;
 	} else if (!device->name || strcmp(device->name, path)) {
 		char *name;
 
diff --git a/kernel-shared/volumes.h b/kernel-shared/volumes.h
index 786add2c879d..fe8802a9fb38 100644
--- a/kernel-shared/volumes.h
+++ b/kernel-shared/volumes.h
@@ -88,6 +88,7 @@ struct btrfs_fs_devices {
 	u64 latest_trans;
 	u64 lowest_devid;
 
+	u64 num_devices;
 	u64 missing_devices;
 	u64 total_rw_bytes;
 
-- 
2.38.1

[PATCH 08/10] btrfs-progs: track total_devs in fs devices

[Anand Jain]

Similar to the kernel, introduce the btrfs_fs_devices::total_devs
attribute to know the overall count of devices that are expected
to be present per filesystem.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: Maintains parity with the kernel.

 kernel-shared/volumes.c | 4 +++-
 kernel-shared/volumes.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
index 88d6c51e3e7e..1954bc230462 100644
--- a/kernel-shared/volumes.c
+++ b/kernel-shared/volumes.c
@@ -367,7 +367,8 @@ static int device_list_add(const char *path,
 			       BTRFS_FSID_SIZE);
 
 		fs_devices->latest_devid = devid;
-		fs_devices->latest_trans = found_transid;
+		/* Below we would set this to found_transid */
+		fs_devices->latest_trans = 0;
 		fs_devices->lowest_devid = (u64)-1;
 		fs_devices->chunk_alloc_policy = BTRFS_CHUNK_ALLOC_REGULAR;
 		device = NULL;
@@ -438,6 +439,7 @@ static int device_list_add(const char *path,
 	if (found_transid > fs_devices->latest_trans) {
 		fs_devices->latest_devid = devid;
 		fs_devices->latest_trans = found_transid;
+		fs_devices->total_devices = device->total_devs;
 	}
 	if (fs_devices->lowest_devid > devid) {
 		fs_devices->lowest_devid = devid;
diff --git a/kernel-shared/volumes.h b/kernel-shared/volumes.h
index fe8802a9fb38..088cb62c3c32 100644
--- a/kernel-shared/volumes.h
+++ b/kernel-shared/volumes.h
@@ -92,6 +92,7 @@ struct btrfs_fs_devices {
 	u64 missing_devices;
 	u64 total_rw_bytes;
 
+	u64 total_devices;
 	int latest_bdev;
 	int lowest_bdev;
 	struct list_head devices;
-- 
2.38.1

[PATCH 09/10] btrfs-progs: track active metadata_uuid per fs_devices

[Anand Jain]

When the fsid does not match the metadata_uuid, the METADATA_UUID flag is
set in the superblock.

Changing the fsid using the btrfstune -U|-u option is not possible on a
filesystem with the METADATA_UUID flag set. But we are checking the
METADATA_UUID only from the super_copy, and not from the other scanned
device.

To fix this bug, track the metadata_uuid at the fs_devices level instead
of checking it only on the specified device in the argument, and use it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 kernel-shared/volumes.c | 2 ++
 kernel-shared/volumes.h | 1 +
 tune/main.c             | 3 ++-
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
index 1954bc230462..ea7bfc66a5e4 100644
--- a/kernel-shared/volumes.c
+++ b/kernel-shared/volumes.c
@@ -435,6 +435,8 @@ static int device_list_add(const char *path,
 	 */
 	if (changing_fsid)
 		fs_devices->changing_fsid = true;
+	if (metadata_uuid)
+		fs_devices->active_metadata_uuid = true;
 
 	if (found_transid > fs_devices->latest_trans) {
 		fs_devices->latest_devid = devid;
diff --git a/kernel-shared/volumes.h b/kernel-shared/volumes.h
index 088cb62c3c32..23559b43e749 100644
--- a/kernel-shared/volumes.h
+++ b/kernel-shared/volumes.h
@@ -103,6 +103,7 @@ struct btrfs_fs_devices {
 	enum btrfs_chunk_allocation_policy chunk_alloc_policy;
 
 	bool changing_fsid;
+	bool active_metadata_uuid;
 };
 
 struct btrfs_bio_stripe {
diff --git a/tune/main.c b/tune/main.c
index 8febd0d6479c..d6c01bb75261 100644
--- a/tune/main.c
+++ b/tune/main.c
@@ -412,7 +412,8 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[])
 	}
 
 	if (random_fsid || (new_fsid_str && !change_metadata_uuid)) {
-		if (btrfs_fs_incompat(root->fs_info, METADATA_UUID)) {
+		if (btrfs_fs_incompat(root->fs_info, METADATA_UUID) ||
+		    root->fs_info->fs_devices->active_metadata_uuid) {
 			error(
 		"Cannot rewrite fsid while METADATA_UUID flag is active. \n"
 		"Ensure fsid and metadata_uuid match before retrying.");
-- 
2.38.1

[PATCH 10/10] btrfs-progs: tune: consolidate return goto free-out

[Anand Jain]

The upcoming "--device" option requires memory to parse devices, which
should be freed before returning from the main() function. As a
preparation for adding the "--device" option to the "btrfstune" command,
provide a consolidated error return exit from the main function with a
"goto" labeled "free_out". The label "free_out" may not make sense
currently, but it will when the "--device" option is added.

There are several return statements within the main function, and
changing all of them in the main "--device" feature patch would deviate
from the actual for the feature changes. Hence, it made sense to create
a preparatory patch.

The return value for any failure remains the same as in the original code.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tune/main.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/tune/main.c b/tune/main.c
index d6c01bb75261..7951fa15b59d 100644
--- a/tune/main.c
+++ b/tune/main.c
@@ -145,7 +145,7 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[])
 	bool to_fst = false;
 	int csum_type = -1;
 	char *new_fsid_str = NULL;
-	int ret;
+	int ret = 1;
 	u64 super_flags = 0;
 	int fd = -1;
 
@@ -233,18 +233,18 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[])
 	set_argv0(argv);
 	device = argv[optind];
 	if (check_argc_exact(argc - optind, 1))
-		return 1;
+		goto free_out;
 
 	if (random_fsid && new_fsid_str) {
 		error("random fsid can't be used with specified fsid");
-		return 1;
+		goto free_out;
 	}
 	if (!super_flags && !seeding_flag && !(random_fsid || new_fsid_str) &&
 	    !change_metadata_uuid && csum_type == -1 && !to_bg_tree &&
 	    !to_extent_tree && !to_fst) {
 		error("at least one option should be specified");
 		usage(&tune_cmd, 1);
-		return 1;
+		goto free_out;
 	}
 
 	if (new_fsid_str) {
@@ -253,18 +253,21 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[])
 		ret = uuid_parse(new_fsid_str, tmp);
 		if (ret < 0) {
 			error("could not parse UUID: %s", new_fsid_str);
-			return 1;
+			ret = 1;
+			goto free_out;
 		}
 		if (!test_uuid_unique(new_fsid_str)) {
 			error("fsid %s is not unique", new_fsid_str);
-			return 1;
+			ret = 1;
+			goto free_out;
 		}
 	}
 
 	fd = open(device, O_RDWR);
 	if (fd < 0) {
 		error("mount check: cannot open %s: %m", device);
-		return 1;
+		ret = 1;
+		goto free_out;
 	}
 
 	ret = check_mounted_where(fd, device, NULL, 0, NULL,
@@ -273,18 +276,21 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[])
 		errno = -ret;
 		error("could not check mount status of %s: %m", device);
 		close(fd);
-		return 1;
+		ret = 1;
+		goto free_out;
 	} else if (ret) {
 		error("%s is mounted", device);
 		close(fd);
-		return 1;
+		ret = 1;
+		goto free_out;
 	}
 
 	root = open_ctree_fd(fd, device, 0, ctree_flags);
 
 	if (!root) {
 		error("open ctree failed");
-		return 1;
+		ret = 1;
+		goto free_out;
 	}
 
 	/*
@@ -450,5 +456,6 @@ out:
 	close_ctree(root);
 	btrfs_close_all_devices();
 
+free_out:
 	return ret;
 }
-- 
2.38.1

Re: [PATCH 01/10 v2] btrfs-progs: dump-super print actual metadata_uuid value

[Anand Jain]

On 03/08/2023 07:29, Anand Jain wrote:
> The function btrfs_print_superblock() prints all members of the
> superblock as they are, except for the superblock::metadata_uuid.
> If the METADATA_UUID flag is unset, it prints the fsid instead of
> zero as in the superblock::metadata_uuid.
> 
> Perhaps this was done because to match with the kernel
> btrfs_fs_devices::metadata_uuid value as it also sets fsid if
> METADATA_UUID flag is unset.
> 
> However, the actual superblock::metadata_uuid is always zero if the
> METADATA_UUID flag is unset. Just to mention the kernel does not alter
> the superblock::metadata_uuid value any time.
> 
> The dump-super printing fsid instead of zero, is confusing because we
> generally expect dump_super to print the superblock value in the raw
> formet without modification.
> 
> Fix this by printing the actual metadata_uuid value instead of fsid.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
V2: none. pls ignore the v2 in the subject.

>   kernel-shared/print-tree.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
> index f97148c47b5a..1c398c3dacab 100644
> --- a/kernel-shared/print-tree.c
> +++ b/kernel-shared/print-tree.c
> @@ -2006,12 +2006,8 @@ void btrfs_print_superblock(struct btrfs_super_block *sb, int full)
>   
>   	uuid_unparse(sb->fsid, buf);
>   	printf("fsid\t\t\t%s\n", buf);
> -	if (metadata_uuid_present) {
> -		uuid_unparse(sb->metadata_uuid, buf);
> -		printf("metadata_uuid\t\t%s\n", buf);
> -	} else {
> -		printf("metadata_uuid\t\t%s\n", buf);
> -	}
> +	uuid_unparse(sb->metadata_uuid, buf);
> +	printf("metadata_uuid\t\t%s\n", buf);
>   
>   	printf("label\t\t\t");
>   	s = sb->label;

Re: [PATCH 07/10] btrfs-progs: track num_devices per fs_devices

[David Sterba]

On Thu, Aug 03, 2023 at 07:29:43AM +0800, Anand Jain wrote:
> Similar to the kernel we need to track the number of devices scanned
> per fs_devices. A preparation patch to fix incomplete fsid changing.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Patch looks ok and makes sense but it crashes on segfault with test 001
in misc-tests. I haven't analyzed what exactly it is but given that
there's only one pointer dereference it must be it. Reverting the patch
makes it work (with the whole series applied), so I'll drop it for now
so you can have a look.

Re: [PATCH 00/10 v2] fixes and preparatory related to metadata_uuid

[David Sterba]

On Thu, Aug 03, 2023 at 07:29:36AM +0800, Anand Jain wrote:
> v2:
> Consolidating preparatory, cleanups and bug fixes, these patches can be
> merged independently.
> 
> Addressed the comments received on some of the patches, relevant changes
> are mentioned in the individual patch.
> 
> The patches here were part of an earlier patchset...
>   [PATCH 0/2] btrfs-progs: fix dump-super metadata_uuid
>   [PATCH 00/11] btrfs-progs: fix bugs and CHANGING_FSID_V2 flag
>   [PATCH 00/10] btrfs-progs: check and tune: add device and noscan options
> 
> Anand Jain (10):
>   btrfs-progs: dump-super print actual metadata_uuid value
>   btrfs-progs: tests: return metadata_uuid or fsid as per METADATA_UUID
>     flag
>   btrfs-progs: fix duplicate missing device
>   btrfs-progs: track missing device counter
>   btrfs-progs: tune: check for missing device
>   btrfs-progs: track changing_fsid flag in fs_devices
>   btrfs-progs: track num_devices per fs_devices
>   btrfs-progs: track total_devs in fs devices
>   btrfs-progs: track active metadata_uuid per fs_devices
>   btrfs-progs: tune: consolidate return goto free-out

Except the num_devices one the rest applied to devel, thanks.

Re: [PATCH 10/10] btrfs-progs: tune: consolidate return goto free-out

[David Sterba]

On Thu, Aug 03, 2023 at 07:29:46AM +0800, Anand Jain wrote:
> The upcoming "--device" option requires memory to parse devices, which
> should be freed before returning from the main() function. As a
> preparation for adding the "--device" option to the "btrfstune" command,
> provide a consolidated error return exit from the main function with a
> "goto" labeled "free_out". The label "free_out" may not make sense
> currently, but it will when the "--device" option is added.
> 
> There are several return statements within the main function, and
> changing all of them in the main "--device" feature patch would deviate
> from the actual for the feature changes. Hence, it made sense to create
> a preparatory patch.
> 
> The return value for any failure remains the same as in the original code.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  tune/main.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/tune/main.c b/tune/main.c
> index d6c01bb75261..7951fa15b59d 100644
> --- a/tune/main.c
> +++ b/tune/main.c
> @@ -145,7 +145,7 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[])
>  	bool to_fst = false;
>  	int csum_type = -1;
>  	char *new_fsid_str = NULL;
> -	int ret;
> +	int ret = 1;

The pattern I'd like to use is to keep ret either uninitialized or 0 and
let each jump to error set the value explicitly, so it's clear without
going back to the initialization.

>  	u64 super_flags = 0;
>  	int fd = -1;
>  
> @@ -233,18 +233,18 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[])
>  	set_argv0(argv);
>  	device = argv[optind];
>  	if (check_argc_exact(argc - optind, 1))
> -		return 1;
> +		goto free_out;
>  
>  	if (random_fsid && new_fsid_str) {
>  		error("random fsid can't be used with specified fsid");
> -		return 1;

ie. ret = 1;

> +		goto free_out;
>  	}
>  	if (!super_flags && !seeding_flag && !(random_fsid || new_fsid_str) &&
>  	    !change_metadata_uuid && csum_type == -1 && !to_bg_tree &&
>  	    !to_extent_tree && !to_fst) {
>  		error("at least one option should be specified");
>  		usage(&tune_cmd, 1);
> -		return 1;
> +		goto free_out;

Re: [PATCH 07/10] btrfs-progs: track num_devices per fs_devices

[Anand Jain]

On 12/8/23 01:41, David Sterba wrote:
> On Thu, Aug 03, 2023 at 07:29:43AM +0800, Anand Jain wrote:
>> Similar to the kernel we need to track the number of devices scanned
>> per fs_devices. A preparation patch to fix incomplete fsid changing.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> Patch looks ok and makes sense but it crashes on segfault with test 001
> in misc-tests. I haven't analyzed what exactly it is but given that
> there's only one pointer dereference it must be it. Reverting the patch
> makes it work (with the whole series applied), so I'll drop it for now
> so you can have a look.


Run the command: $ make clean-all to fix it.

It appears that the struct btrfs_fs_devices is used somewhere
statically?, and it gets updated only with a clean compile.

Since this has not been integrated yet, would it be better for
me to include it in the upcoming btrfs-progs patchset?

Thanks, Anand

Re: [PATCH 10/10] btrfs-progs: tune: consolidate return goto free-out

[Anand Jain]

On 12/8/23 01:47, David Sterba wrote:
> On Thu, Aug 03, 2023 at 07:29:46AM +0800, Anand Jain wrote:
>> The upcoming "--device" option requires memory to parse devices, which
>> should be freed before returning from the main() function. As a
>> preparation for adding the "--device" option to the "btrfstune" command,
>> provide a consolidated error return exit from the main function with a
>> "goto" labeled "free_out". The label "free_out" may not make sense
>> currently, but it will when the "--device" option is added.
>>
>> There are several return statements within the main function, and
>> changing all of them in the main "--device" feature patch would deviate
>> from the actual for the feature changes. Hence, it made sense to create
>> a preparatory patch.
>>
>> The return value for any failure remains the same as in the original code.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   tune/main.c | 27 +++++++++++++++++----------
>>   1 file changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/tune/main.c b/tune/main.c
>> index d6c01bb75261..7951fa15b59d 100644
>> --- a/tune/main.c
>> +++ b/tune/main.c
>> @@ -145,7 +145,7 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[])
>>   	bool to_fst = false;
>>   	int csum_type = -1;
>>   	char *new_fsid_str = NULL;
>> -	int ret;
>> +	int ret = 1;
> 
> The pattern I'd like to use is to keep ret either uninitialized or 0 and
> let each jump to error set the value explicitly, so it's clear without
> going back to the initialization.

  Understood. However, if uninitialized, older GCC versions might give
  false positive warnings for uninitialized variable usage, though
  it's not an issue here.

> 
>>   	u64 super_flags = 0;
>>   	int fd = -1;
>>   
>> @@ -233,18 +233,18 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[])
>>   	set_argv0(argv);
>>   	device = argv[optind];
>>   	if (check_argc_exact(argc - optind, 1))
>> -		return 1;
>> +		goto free_out;
>>   
>>   	if (random_fsid && new_fsid_str) {
>>   		error("random fsid can't be used with specified fsid");
>> -		return 1;
> 
> ie. ret = 1;

   Code in the devel looks good.

Thanks.

> 
>> +		goto free_out;
>>   	}
>>   	if (!super_flags && !seeding_flag && !(random_fsid || new_fsid_str) &&
>>   	    !change_metadata_uuid && csum_type == -1 && !to_bg_tree &&
>>   	    !to_extent_tree && !to_fst) {
>>   		error("at least one option should be specified");
>>   		usage(&tune_cmd, 1);
>> -		return 1;
>> +		goto free_out;

Re: [PATCH 07/10] btrfs-progs: track num_devices per fs_devices

[Anand Jain]

On 14/08/2023 19:16, Anand Jain wrote:
> 
> 
> On 12/8/23 01:41, David Sterba wrote:
>> On Thu, Aug 03, 2023 at 07:29:43AM +0800, Anand Jain wrote:
>>> Similar to the kernel we need to track the number of devices scanned
>>> per fs_devices. A preparation patch to fix incomplete fsid changing.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>
>> Patch looks ok and makes sense but it crashes on segfault with test 001
>> in misc-tests. I haven't analyzed what exactly it is but given that
>> there's only one pointer dereference it must be it. Reverting the patch
>> makes it work (with the whole series applied), so I'll drop it for now
>> so you can have a look.
> 
> 
> Run the command: $ make clean-all to fix it.
> 
> It appears that the struct btrfs_fs_devices is used somewhere
> statically?, and it gets updated only with a clean compile.
> 
> Since this has not been integrated yet, would it be better for
> me to include it in the upcoming btrfs-progs patchset?
> 

This patch is now part of the new patchset in the ML.

  [PATCH 00/16] btrfs-progs: recover from failed metadata_uuid
port kernel



> Thanks, Anand