diff mbox series

[11/11] btrfs-progs: tune: fix incomplete fsid_change

Message ID 24bef15af8c65da69ab8a3b574e0da7b71295008.1688724045.git.anand.jain@oracle.com (mailing list archive)
State New, archived
Headers show
Series btrfs-progs: fix bugs and CHANGING_FSID_V2 flag | expand

Commit Message

Anand Jain July 7, 2023, 3:52 p.m. UTC
An incomplete fsid state occurs when devices have two or more fsids
associated with the same metadata_uuid. As it can be confusing to
determine which devices should be assembled together, the fix only
works when both the --noscan and --device options are used. This means
the user will have to manually select and assemble the devices with the
same metadata_uuids.

With this fix, the fsid can continue to be changed in the user-spce using
either a new fsid or a user-provided fsid.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tune/change-metadata-uuid.c | 4 +++-
 tune/main.c                 | 6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

Comments

David Sterba July 13, 2023, 6:57 p.m. UTC | #1
On Fri, Jul 07, 2023 at 11:52:41PM +0800, Anand Jain wrote:
> An incomplete fsid state occurs when devices have two or more fsids
> associated with the same metadata_uuid. As it can be confusing to
> determine which devices should be assembled together, the fix only
> works when both the --noscan and --device options are used. This means
> the user will have to manually select and assemble the devices with the
> same metadata_uuids.

This is not a good usability. If the user can determine which devices
should be in the --noscan and --device options why can't we do that
programaticaly? If the found devices can be reliably identified as part
of the filesystem (and there are e.g. no ambiguities due to duplicated
devices) then the command should work it out by itself.

The btrfstune commands are supposed to be restartable, namely the uuid
conversions, basically with the same command that was used to begin the
conversion. If there's a problem that needs user interactions then there
should be specific instructions what to check and how to resolve it
manually.
Anand Jain July 17, 2023, 7:25 p.m. UTC | #2
On 14/07/2023 02:57, David Sterba wrote:
> On Fri, Jul 07, 2023 at 11:52:41PM +0800, Anand Jain wrote:
>> An incomplete fsid state occurs when devices have two or more fsids
>> associated with the same metadata_uuid. As it can be confusing to
>> determine which devices should be assembled together, the fix only
>> works when both the --noscan and --device options are used. This means
>> the user will have to manually select and assemble the devices with the
>> same metadata_uuids.
> 
> This is not a good usability. If the user can determine which devices
> should be in the --noscan and --device options why can't we do that
> programaticaly?

Technically, there can be many fsids with the same metadata_uuid.
I'm afraid that in all split-brain scenarios, we may not be able
to successfully identify device partners, and assembling the
filesystem with the wrong device could be disastrous.

> If the found devices can be reliably identified as part
> of the filesystem (and there are e.g. no ambiguities due to duplicated
> devices) then the command should work it out by itself.

I expect btrfstune -m to be used more for modifying btrfs file images
rather than block devices. Users can simply copy the image and change
the fsid, which the system won't be aware of unless the user informs
us.

> The btrfstune commands are supposed to be restartable, namely the uuid
> conversions, basically with the same command that was used to begin the
> conversion. If there's a problem that needs user interactions then there
> should be specific instructions what to check and how to resolve it
> manually.

I was trying to avoid yet another cmd option. If the command is
restarted and the superblock has the CHANGING_FSID flag set, then
the command will fail. In this state, we can add specific
instructions on how to continue (i.e., --noscan and --device options
are mandatory). Or any suggestion?

Thanks.
Anand Jain July 20, 2023, 2:33 p.m. UTC | #3
On 18/07/2023 03:25, Anand Jain wrote:
> 
> 
> On 14/07/2023 02:57, David Sterba wrote:
>> On Fri, Jul 07, 2023 at 11:52:41PM +0800, Anand Jain wrote:
>>> An incomplete fsid state occurs when devices have two or more fsids
>>> associated with the same metadata_uuid. As it can be confusing to
>>> determine which devices should be assembled together, the fix only
>>> works when both the --noscan and --device options are used. This means
>>> the user will have to manually select and assemble the devices with the
>>> same metadata_uuids.
>>
>> This is not a good usability. If the user can determine which devices
>> should be in the --noscan and --device options why can't we do that
>> programaticaly?
> 
> Technically, there can be many fsids with the same metadata_uuid.
> I'm afraid that in all split-brain scenarios, we may not be able
> to successfully identify device partners, and assembling the
> filesystem with the wrong device could be disastrous.
> 
>> If the found devices can be reliably identified as part
>> of the filesystem (and there are e.g. no ambiguities due to duplicated
>> devices) then the command should work it out by itself.
> 
> I expect btrfstune -m to be used more for modifying btrfs file images
> rather than block devices. Users can simply copy the image and change
> the fsid, which the system won't be aware of unless the user informs
> us.
> 
>> The btrfstune commands are supposed to be restartable, namely the uuid
>> conversions, basically with the same command that was used to begin the
>> conversion. If there's a problem that needs user interactions then there
>> should be specific instructions what to check and how to resolve it
>> manually.
> 
> I was trying to avoid yet another cmd option. If the command is
> restarted and the superblock has the CHANGING_FSID flag set, then
> the command will fail. In this state, we can add specific
> instructions on how to continue (i.e., --noscan and --device options
> are mandatory). Or any suggestion?
> 

  OR I think it would be better to combine the functionality of both
  --noscan and --device into one option, such as --reunite.
  Better? What do  you think?

> Thanks.
diff mbox series

Patch

diff --git a/tune/change-metadata-uuid.c b/tune/change-metadata-uuid.c
index 295308f299aa..c291324ca4c4 100644
--- a/tune/change-metadata-uuid.c
+++ b/tune/change-metadata-uuid.c
@@ -21,6 +21,7 @@ 
 #include "kernel-shared/uapi/btrfs.h"
 #include "kernel-shared/ctree.h"
 #include "kernel-shared/transaction.h"
+#include "kernel-shared/volumes.h"
 #include "common/messages.h"
 #include "tune/tune.h"
 
@@ -45,7 +46,8 @@  int set_metadata_uuid(struct btrfs_root *root, const char *uuid_string)
 		return 1;
 	}
 
-	if (check_unfinished_fsid_change(root->fs_info, unused1, unused2)) {
+	if (!root->fs_info->fs_devices->sanitized &&
+	    check_unfinished_fsid_change(root->fs_info, unused1, unused2)) {
 		error("UUID rewrite in progress, cannot change metadata_uuid");
 		return 1;
 	}
diff --git a/tune/main.c b/tune/main.c
index 3ca9c5716573..f6d56af80dbf 100644
--- a/tune/main.c
+++ b/tune/main.c
@@ -323,6 +323,12 @@  int BOX_MAIN(btrfstune)(int argc, char *argv[])
 			goto free_out;
 	}
 
+	if (argv_devices != NULL && noscan) {
+		ret = scan_reunite_fs_devices(device);
+		if (ret)
+			goto free_out;
+	}
+
 	root = open_ctree_fd(fd, device, 0, ctree_flags);
 	if (!root) {
 		error("open ctree failed");