diff mbox series

[2/2] btrfs-progs: set subvol uuids when converting

Message ID 20240719150343.3992904-2-maharmstone@fb.com (mailing list archive)
State New, archived
Headers show
Series [1/2] btrfs-progs: add set_uuid param to btrfs_make_subvolume | expand

Commit Message

Mark Harmstone July 19, 2024, 3:03 p.m. UTC 
Currently when using btrfs-convert, neither the main subvolume nor the
image subvolume get uuids assigned, nor is the uuid tree created.

Signed-off-by: Mark Harmstone <maharmstone@fb.com>
---
 common/root-tree-utils.c | 29 +++++++++++++++++++++++++++++
 common/root-tree-utils.h |  1 +
 convert/common.c         | 14 +++++++++-----
 convert/main.c           |  8 +++++++-
 mkfs/main.c              | 29 -----------------------------
 5 files changed, 46 insertions(+), 35 deletions(-)

Comments

Josef Bacik July 19, 2024, 8:20 p.m. UTC | #1 
On Fri, Jul 19, 2024 at 04:03:24PM +0100, Mark Harmstone wrote:
> Currently when using btrfs-convert, neither the main subvolume nor the
> image subvolume get uuids assigned, nor is the uuid tree created.
> 
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> ---
>  common/root-tree-utils.c | 29 +++++++++++++++++++++++++++++
>  common/root-tree-utils.h |  1 +
>  convert/common.c         | 14 +++++++++-----
>  convert/main.c           |  8 +++++++-
>  mkfs/main.c              | 29 -----------------------------
>  5 files changed, 46 insertions(+), 35 deletions(-)
> 
> diff --git a/common/root-tree-utils.c b/common/root-tree-utils.c
> index f9343304..9495178c 100644
> --- a/common/root-tree-utils.c
> +++ b/common/root-tree-utils.c
> @@ -59,6 +59,35 @@ error:
>  	return ret;
>  }
>  
> +int create_uuid_tree(struct btrfs_trans_handle *trans)

If we're exporting a new thing then it needs to be renamed to
btrfs_create_uuid_tree.

Also a convert test for this would be good, to validate we're getting the UUID
tree on convert.  Thanks,

Josef
Qu Wenruo July 19, 2024, 10:28 p.m. UTC | #2 
在 2024/7/20 00:33, Mark Harmstone 写道:
> Currently when using btrfs-convert, neither the main subvolume nor the
> image subvolume get uuids assigned, nor is the uuid tree created.
>
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> ---
>   common/root-tree-utils.c | 29 +++++++++++++++++++++++++++++
>   common/root-tree-utils.h |  1 +
>   convert/common.c         | 14 +++++++++-----
>   convert/main.c           |  8 +++++++-
>   mkfs/main.c              | 29 -----------------------------
>   5 files changed, 46 insertions(+), 35 deletions(-)
>
> diff --git a/common/root-tree-utils.c b/common/root-tree-utils.c
> index f9343304..9495178c 100644
> --- a/common/root-tree-utils.c
> +++ b/common/root-tree-utils.c
> @@ -59,6 +59,35 @@ error:
>   	return ret;
>   }
>
> +int create_uuid_tree(struct btrfs_trans_handle *trans)
> +{
> +	struct btrfs_fs_info *fs_info = trans->fs_info;
> +	struct btrfs_root *root;
> +	struct btrfs_key key = {
> +		.objectid = BTRFS_UUID_TREE_OBJECTID,
> +		.type = BTRFS_ROOT_ITEM_KEY,
> +	};
> +	int ret = 0;
> +
> +	UASSERT(fs_info->uuid_root == NULL);
> +	root = btrfs_create_tree(trans, &key);
> +	if (IS_ERR(root)) {
> +		ret = PTR_ERR(root);
> +		goto out;
> +	}
> +
> +	add_root_to_dirty_list(root);
> +	fs_info->uuid_root = root;
> +	ret = btrfs_uuid_tree_add(trans, fs_info->fs_root->root_item.uuid,
> +				  BTRFS_UUID_KEY_SUBVOL,
> +				  fs_info->fs_root->root_key.objectid);
> +	if (ret < 0)
> +		btrfs_abort_transaction(trans, ret);
> +
> +out:
> +	return ret;
> +}
> +
>   int btrfs_uuid_tree_add(struct btrfs_trans_handle *trans, u8 *uuid, u8 type,
>   			u64 subvol_id_cpu)
>   {
> diff --git a/common/root-tree-utils.h b/common/root-tree-utils.h
> index 78731dd5..aec1849b 100644
> --- a/common/root-tree-utils.h
> +++ b/common/root-tree-utils.h
> @@ -29,5 +29,6 @@ int btrfs_link_subvolume(struct btrfs_trans_handle *trans,
>   			 int namelen, struct btrfs_root *subvol);
>   int btrfs_uuid_tree_add(struct btrfs_trans_handle *trans, u8 *uuid, u8 type,
>   			u64 subvol_id_cpu);
> +int create_uuid_tree(struct btrfs_trans_handle *trans);
>
>   #endif
> diff --git a/convert/common.c b/convert/common.c
> index b093fdb5..667f38a4 100644
> --- a/convert/common.c
> +++ b/convert/common.c
> @@ -190,7 +190,7 @@ static int setup_temp_extent_buffer(struct extent_buffer *buf,
>   static void insert_temp_root_item(struct extent_buffer *buf,
>   				  struct btrfs_mkfs_config *cfg,
>   				  int *slot, u32 *itemoff, u64 objectid,
> -				  u64 bytenr)
> +				  u64 bytenr, bool set_uuid)

Considering this is really a temporary root item, I believe we can skip
it the UUID step for now.
(Only one of the 4 callers are passing true for it).


I'm wondering if a kernel like uuid tree generation would be more
concentrated and easier to implement.

E.g. after the fs is fully converted, generate the uuid tree then update
the UUID for the involved subvolumes.

Thanks,
Qu
>   {
>   	struct btrfs_root_item root_item;
>   	struct btrfs_inode_item *inode_item;
> @@ -210,6 +210,9 @@ static void insert_temp_root_item(struct extent_buffer *buf,
>   	btrfs_set_root_generation(&root_item, 1);
>   	btrfs_set_root_bytenr(&root_item, bytenr);
>
> +	if (set_uuid)
> +		uuid_generate(root_item.uuid);
> +
>   	memset(&disk_key, 0, sizeof(disk_key));
>   	btrfs_set_disk_key_type(&disk_key, BTRFS_ROOT_ITEM_KEY);
>   	btrfs_set_disk_key_objectid(&disk_key, objectid);
> @@ -281,13 +284,14 @@ static int setup_temp_root_tree(int fd, struct btrfs_mkfs_config *cfg,
>   		goto out;
>
>   	insert_temp_root_item(buf, cfg, &slot, &itemoff,
> -			      BTRFS_EXTENT_TREE_OBJECTID, extent_bytenr);
> +			      BTRFS_EXTENT_TREE_OBJECTID, extent_bytenr,
> +			      false);
>   	insert_temp_root_item(buf, cfg, &slot, &itemoff,
> -			      BTRFS_DEV_TREE_OBJECTID, dev_bytenr);
> +			      BTRFS_DEV_TREE_OBJECTID, dev_bytenr, false);
>   	insert_temp_root_item(buf, cfg, &slot, &itemoff,
> -			      BTRFS_FS_TREE_OBJECTID, fs_bytenr);
> +			      BTRFS_FS_TREE_OBJECTID, fs_bytenr, true);
>   	insert_temp_root_item(buf, cfg, &slot, &itemoff,
> -			      BTRFS_CSUM_TREE_OBJECTID, csum_bytenr);
> +			      BTRFS_CSUM_TREE_OBJECTID, csum_bytenr, false);
>
>   	ret = write_temp_extent_buffer(fd, buf, root_bytenr, cfg);
>   out:
> diff --git a/convert/main.c b/convert/main.c
> index c863ce91..8cfd9407 100644
> --- a/convert/main.c
> +++ b/convert/main.c
> @@ -1021,8 +1021,14 @@ static int init_btrfs(struct btrfs_mkfs_config *cfg, struct btrfs_root *root,
>   	btrfs_set_root_dirid(&fs_info->fs_root->root_item,
>   			     BTRFS_FIRST_FREE_OBJECTID);
>
> +	ret = create_uuid_tree(trans);
> +	if (ret) {
> +		error("failed to create uuid tree: %d", ret);
> +		goto err;
> +	}
> +
>   	/* subvol for fs image file */
> -	ret = btrfs_make_subvolume(trans, CONV_IMAGE_SUBVOL_OBJECTID, false);
> +	ret = btrfs_make_subvolume(trans, CONV_IMAGE_SUBVOL_OBJECTID, true);
>   	if (ret < 0) {
>   		error("failed to create subvolume image root: %d", ret);
>   		goto err;
> diff --git a/mkfs/main.c b/mkfs/main.c
> index 0bdb414a..a69aa24b 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -735,35 +735,6 @@ static void update_chunk_allocation(struct btrfs_fs_info *fs_info,
>   	}
>   }
>
> -static int create_uuid_tree(struct btrfs_trans_handle *trans)
> -{
> -	struct btrfs_fs_info *fs_info = trans->fs_info;
> -	struct btrfs_root *root;
> -	struct btrfs_key key = {
> -		.objectid = BTRFS_UUID_TREE_OBJECTID,
> -		.type = BTRFS_ROOT_ITEM_KEY,
> -	};
> -	int ret = 0;
> -
> -	UASSERT(fs_info->uuid_root == NULL);
> -	root = btrfs_create_tree(trans, &key);
> -	if (IS_ERR(root)) {
> -		ret = PTR_ERR(root);
> -		goto out;
> -	}
> -
> -	add_root_to_dirty_list(root);
> -	fs_info->uuid_root = root;
> -	ret = btrfs_uuid_tree_add(trans, fs_info->fs_root->root_item.uuid,
> -				  BTRFS_UUID_KEY_SUBVOL,
> -				  fs_info->fs_root->root_key.objectid);
> -	if (ret < 0)
> -		btrfs_abort_transaction(trans, ret);
> -
> -out:
> -	return ret;
> -}
> -
>   static int create_global_root(struct btrfs_trans_handle *trans, u64 objectid,
>   			      int root_id)
>   {
David Sterba July 26, 2024, 5:30 p.m. UTC | #3 
On Sat, Jul 20, 2024 at 07:58:30AM +0930, Qu Wenruo wrote:
> 在 2024/7/20 00:33, Mark Harmstone 写道:
> > Currently when using btrfs-convert, neither the main subvolume nor the
> > image subvolume get uuids assigned, nor is the uuid tree created.
> >
> > Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> > ---
> >   common/root-tree-utils.c | 29 +++++++++++++++++++++++++++++
> >   common/root-tree-utils.h |  1 +
> >   convert/common.c         | 14 +++++++++-----
> >   convert/main.c           |  8 +++++++-
> >   mkfs/main.c              | 29 -----------------------------
> >   5 files changed, 46 insertions(+), 35 deletions(-)
> >
> > diff --git a/common/root-tree-utils.c b/common/root-tree-utils.c
> > index f9343304..9495178c 100644
> > --- a/common/root-tree-utils.c
> > +++ b/common/root-tree-utils.c
> > @@ -59,6 +59,35 @@ error:
> >   	return ret;
> >   }
> >
> > +int create_uuid_tree(struct btrfs_trans_handle *trans)
> > +{
> > +	struct btrfs_fs_info *fs_info = trans->fs_info;
> > +	struct btrfs_root *root;
> > +	struct btrfs_key key = {
> > +		.objectid = BTRFS_UUID_TREE_OBJECTID,
> > +		.type = BTRFS_ROOT_ITEM_KEY,
> > +	};
> > +	int ret = 0;
> > +
> > +	UASSERT(fs_info->uuid_root == NULL);
> > +	root = btrfs_create_tree(trans, &key);
> > +	if (IS_ERR(root)) {
> > +		ret = PTR_ERR(root);
> > +		goto out;
> > +	}
> > +
> > +	add_root_to_dirty_list(root);
> > +	fs_info->uuid_root = root;
> > +	ret = btrfs_uuid_tree_add(trans, fs_info->fs_root->root_item.uuid,
> > +				  BTRFS_UUID_KEY_SUBVOL,
> > +				  fs_info->fs_root->root_key.objectid);
> > +	if (ret < 0)
> > +		btrfs_abort_transaction(trans, ret);
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> >   int btrfs_uuid_tree_add(struct btrfs_trans_handle *trans, u8 *uuid, u8 type,
> >   			u64 subvol_id_cpu)
> >   {
> > diff --git a/common/root-tree-utils.h b/common/root-tree-utils.h
> > index 78731dd5..aec1849b 100644
> > --- a/common/root-tree-utils.h
> > +++ b/common/root-tree-utils.h
> > @@ -29,5 +29,6 @@ int btrfs_link_subvolume(struct btrfs_trans_handle *trans,
> >   			 int namelen, struct btrfs_root *subvol);
> >   int btrfs_uuid_tree_add(struct btrfs_trans_handle *trans, u8 *uuid, u8 type,
> >   			u64 subvol_id_cpu);
> > +int create_uuid_tree(struct btrfs_trans_handle *trans);
> >
> >   #endif
> > diff --git a/convert/common.c b/convert/common.c
> > index b093fdb5..667f38a4 100644
> > --- a/convert/common.c
> > +++ b/convert/common.c
> > @@ -190,7 +190,7 @@ static int setup_temp_extent_buffer(struct extent_buffer *buf,
> >   static void insert_temp_root_item(struct extent_buffer *buf,
> >   				  struct btrfs_mkfs_config *cfg,
> >   				  int *slot, u32 *itemoff, u64 objectid,
> > -				  u64 bytenr)
> > +				  u64 bytenr, bool set_uuid)
> 
> Considering this is really a temporary root item, I believe we can skip
> it the UUID step for now.
> (Only one of the 4 callers are passing true for it).
> 
> 
> I'm wondering if a kernel like uuid tree generation would be more
> concentrated and easier to implement.
> 
> E.g. after the fs is fully converted, generate the uuid tree then update
> the UUID for the involved subvolumes.

Agreed, this sounds like a better option.
diff mbox series

Patch

diff --git a/common/root-tree-utils.c b/common/root-tree-utils.c
index f9343304..9495178c 100644
--- a/common/root-tree-utils.c
+++ b/common/root-tree-utils.c
@@ -59,6 +59,35 @@  error:
 	return ret;
 }
 
+int create_uuid_tree(struct btrfs_trans_handle *trans)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	struct btrfs_root *root;
+	struct btrfs_key key = {
+		.objectid = BTRFS_UUID_TREE_OBJECTID,
+		.type = BTRFS_ROOT_ITEM_KEY,
+	};
+	int ret = 0;
+
+	UASSERT(fs_info->uuid_root == NULL);
+	root = btrfs_create_tree(trans, &key);
+	if (IS_ERR(root)) {
+		ret = PTR_ERR(root);
+		goto out;
+	}
+
+	add_root_to_dirty_list(root);
+	fs_info->uuid_root = root;
+	ret = btrfs_uuid_tree_add(trans, fs_info->fs_root->root_item.uuid,
+				  BTRFS_UUID_KEY_SUBVOL,
+				  fs_info->fs_root->root_key.objectid);
+	if (ret < 0)
+		btrfs_abort_transaction(trans, ret);
+
+out:
+	return ret;
+}
+
 int btrfs_uuid_tree_add(struct btrfs_trans_handle *trans, u8 *uuid, u8 type,
 			u64 subvol_id_cpu)
 {
diff --git a/common/root-tree-utils.h b/common/root-tree-utils.h
index 78731dd5..aec1849b 100644
--- a/common/root-tree-utils.h
+++ b/common/root-tree-utils.h
@@ -29,5 +29,6 @@  int btrfs_link_subvolume(struct btrfs_trans_handle *trans,
 			 int namelen, struct btrfs_root *subvol);
 int btrfs_uuid_tree_add(struct btrfs_trans_handle *trans, u8 *uuid, u8 type,
 			u64 subvol_id_cpu);
+int create_uuid_tree(struct btrfs_trans_handle *trans);
 
 #endif
diff --git a/convert/common.c b/convert/common.c
index b093fdb5..667f38a4 100644
--- a/convert/common.c
+++ b/convert/common.c
@@ -190,7 +190,7 @@  static int setup_temp_extent_buffer(struct extent_buffer *buf,
 static void insert_temp_root_item(struct extent_buffer *buf,
 				  struct btrfs_mkfs_config *cfg,
 				  int *slot, u32 *itemoff, u64 objectid,
-				  u64 bytenr)
+				  u64 bytenr, bool set_uuid)
 {
 	struct btrfs_root_item root_item;
 	struct btrfs_inode_item *inode_item;
@@ -210,6 +210,9 @@  static void insert_temp_root_item(struct extent_buffer *buf,
 	btrfs_set_root_generation(&root_item, 1);
 	btrfs_set_root_bytenr(&root_item, bytenr);
 
+	if (set_uuid)
+		uuid_generate(root_item.uuid);
+
 	memset(&disk_key, 0, sizeof(disk_key));
 	btrfs_set_disk_key_type(&disk_key, BTRFS_ROOT_ITEM_KEY);
 	btrfs_set_disk_key_objectid(&disk_key, objectid);
@@ -281,13 +284,14 @@  static int setup_temp_root_tree(int fd, struct btrfs_mkfs_config *cfg,
 		goto out;
 
 	insert_temp_root_item(buf, cfg, &slot, &itemoff,
-			      BTRFS_EXTENT_TREE_OBJECTID, extent_bytenr);
+			      BTRFS_EXTENT_TREE_OBJECTID, extent_bytenr,
+			      false);
 	insert_temp_root_item(buf, cfg, &slot, &itemoff,
-			      BTRFS_DEV_TREE_OBJECTID, dev_bytenr);
+			      BTRFS_DEV_TREE_OBJECTID, dev_bytenr, false);
 	insert_temp_root_item(buf, cfg, &slot, &itemoff,
-			      BTRFS_FS_TREE_OBJECTID, fs_bytenr);
+			      BTRFS_FS_TREE_OBJECTID, fs_bytenr, true);
 	insert_temp_root_item(buf, cfg, &slot, &itemoff,
-			      BTRFS_CSUM_TREE_OBJECTID, csum_bytenr);
+			      BTRFS_CSUM_TREE_OBJECTID, csum_bytenr, false);
 
 	ret = write_temp_extent_buffer(fd, buf, root_bytenr, cfg);
 out:
diff --git a/convert/main.c b/convert/main.c
index c863ce91..8cfd9407 100644
--- a/convert/main.c
+++ b/convert/main.c
@@ -1021,8 +1021,14 @@  static int init_btrfs(struct btrfs_mkfs_config *cfg, struct btrfs_root *root,
 	btrfs_set_root_dirid(&fs_info->fs_root->root_item,
 			     BTRFS_FIRST_FREE_OBJECTID);
 
+	ret = create_uuid_tree(trans);
+	if (ret) {
+		error("failed to create uuid tree: %d", ret);
+		goto err;
+	}
+
 	/* subvol for fs image file */
-	ret = btrfs_make_subvolume(trans, CONV_IMAGE_SUBVOL_OBJECTID, false);
+	ret = btrfs_make_subvolume(trans, CONV_IMAGE_SUBVOL_OBJECTID, true);
 	if (ret < 0) {
 		error("failed to create subvolume image root: %d", ret);
 		goto err;
diff --git a/mkfs/main.c b/mkfs/main.c
index 0bdb414a..a69aa24b 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -735,35 +735,6 @@  static void update_chunk_allocation(struct btrfs_fs_info *fs_info,
 	}
 }
 
-static int create_uuid_tree(struct btrfs_trans_handle *trans)
-{
-	struct btrfs_fs_info *fs_info = trans->fs_info;
-	struct btrfs_root *root;
-	struct btrfs_key key = {
-		.objectid = BTRFS_UUID_TREE_OBJECTID,
-		.type = BTRFS_ROOT_ITEM_KEY,
-	};
-	int ret = 0;
-
-	UASSERT(fs_info->uuid_root == NULL);
-	root = btrfs_create_tree(trans, &key);
-	if (IS_ERR(root)) {
-		ret = PTR_ERR(root);
-		goto out;
-	}
-
-	add_root_to_dirty_list(root);
-	fs_info->uuid_root = root;
-	ret = btrfs_uuid_tree_add(trans, fs_info->fs_root->root_item.uuid,
-				  BTRFS_UUID_KEY_SUBVOL,
-				  fs_info->fs_root->root_key.objectid);
-	if (ret < 0)
-		btrfs_abort_transaction(trans, ret);
-
-out:
-	return ret;
-}
-
 static int create_global_root(struct btrfs_trans_handle *trans, u64 objectid,
 			      int root_id)
 {