[PATCH v2 0/3] btrfs-progs: small cleanups related to subvolume creation

[PATCH v2 0/3] btrfs-progs: small cleanups related to subvolume creation

[Qu Wenruo]

[CHANGELOG]
v2:
- A new patch to cleanup btrfs_mksubvol()
  We have a kernel function with the same name, but those two are doing
  completely different works.

  The progs version is more like linking a subvolume to a destination.

  The new patch removes btrfs_mksubvol(), and move the convert specific
  code to convert code basis.

Thanks to Mark's new effort to introduce subvolume creation ability, the
long existing duplicated subvolume creation problem is exposed again.

The first patch to do a small cleanup for btrfs_create_tree() so that
the parameter list matches the kernel one.

The second one is the main dish to fully merge the different functions
to create a subvolume.

We have btrfs_create_tree() to properly create an empty tree, and
btrfs_make_root_dir() to create the initial root dir.

So use them to create btrfs_make_subvolume():

- Calls btrfs_create_tree() to properly create an empty tree
  Unlike btrfs_copy_root() used in create_subvol(), which can be unsafe
  if the source subvolume is not empty.

- Calls btrfs_read_fs_root() to setup the cache and tracking
  Inside create_data_reloc_tree() we directly added the root to
  fs_root_tree, which is only safe for data reloc tree.

  As we didn't properly set the correct tracking flags.

- Calls btrfs_make_root_dir() to setup the root directory

And finally cleanup a confusing function, btrfs_mksubvol(), which is not
really creating a subvolume, but linking a subvolume to a parent inode.

Instead of the confusing name, create a new helper,
btrfs_link_subvolume() to do the linkage, and move the convert specific
retry behavior into convert code.

Qu Wenruo (3):
  btrfs-progs: remove fs_info parameter from btrfs_create_tree()
  btrfs-progs: introduce btrfs_make_subvolume()
  btrfs-progs: use btrfs_link_subvolume() to replace btrfs_mksubvol()

 Makefile                        |   1 +
 check/main.c                    |   9 +-
 common/root-tree-utils.c        | 215 ++++++++++++++++++++++++++++++++
 common/root-tree-utils.h        |  30 +++++
 convert/main.c                  | 107 +++++++++-------
 kernel-shared/ctree.h           |   8 +-
 kernel-shared/disk-io.c         |   4 +-
 kernel-shared/disk-io.h         |   1 -
 kernel-shared/free-space-tree.c |   2 +-
 kernel-shared/inode.c           | 140 +--------------------
 mkfs/common.c                   |  39 ------
 mkfs/common.h                   |   2 -
 mkfs/main.c                     |  78 +-----------
 13 files changed, 331 insertions(+), 305 deletions(-)
 create mode 100644 common/root-tree-utils.c
 create mode 100644 common/root-tree-utils.h

--
2.45.2

[PATCH v2 1/3] btrfs-progs: remove fs_info parameter from btrfs_create_tree()

[Qu Wenruo]

The @fs_info parameter can be easily extracted from @trans, and kernel
has already remove the parameter.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/disk-io.c         | 4 ++--
 kernel-shared/disk-io.h         | 1 -
 kernel-shared/free-space-tree.c | 2 +-
 mkfs/main.c                     | 8 ++++----
 4 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
index 295bd50ad063..3e8cab1187f0 100644
--- a/kernel-shared/disk-io.c
+++ b/kernel-shared/disk-io.c
@@ -2369,12 +2369,12 @@ int btrfs_delete_and_free_root(struct btrfs_trans_handle *trans,
 }
 
 struct btrfs_root *btrfs_create_tree(struct btrfs_trans_handle *trans,
-				     struct btrfs_fs_info *fs_info,
 				     struct btrfs_key *key)
 {
-	struct extent_buffer *leaf;
+	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_root *tree_root = fs_info->tree_root;
 	struct btrfs_root *root;
+	struct extent_buffer *leaf;
 	int ret = 0;
 
 	root = kzalloc(sizeof(*root), GFP_KERNEL);
diff --git a/kernel-shared/disk-io.h b/kernel-shared/disk-io.h
index 9f848635fd69..0047db5e7c3e 100644
--- a/kernel-shared/disk-io.h
+++ b/kernel-shared/disk-io.h
@@ -239,7 +239,6 @@ int write_tree_block(struct btrfs_trans_handle *trans,
 		     struct extent_buffer *eb);
 int btrfs_fs_roots_compare_roots(const struct rb_node *node1, const struct rb_node *node2);
 struct btrfs_root *btrfs_create_tree(struct btrfs_trans_handle *trans,
-				     struct btrfs_fs_info *fs_info,
 				     struct btrfs_key *key);
 int btrfs_delete_and_free_root(struct btrfs_trans_handle *trans,
 			       struct btrfs_root *root);
diff --git a/kernel-shared/free-space-tree.c b/kernel-shared/free-space-tree.c
index 93806ca01162..81fd57b886d2 100644
--- a/kernel-shared/free-space-tree.c
+++ b/kernel-shared/free-space-tree.c
@@ -1516,7 +1516,7 @@ int btrfs_create_free_space_tree(struct btrfs_fs_info *fs_info)
 	if (IS_ERR(trans))
 		return PTR_ERR(trans);
 
-	free_space_root = btrfs_create_tree(trans, fs_info, &root_key);
+	free_space_root = btrfs_create_tree(trans, &root_key);
 	if (IS_ERR(free_space_root)) {
 		ret = PTR_ERR(free_space_root);
 		goto abort;
diff --git a/mkfs/main.c b/mkfs/main.c
index b40f7432bb74..077da1e33fc6 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -748,7 +748,7 @@ static int create_data_reloc_tree(struct btrfs_trans_handle *trans)
 	char *name = "..";
 	int ret;
 
-	root = btrfs_create_tree(trans, fs_info, &key);
+	root = btrfs_create_tree(trans, &key);
 	if (IS_ERR(root)) {
 		ret = PTR_ERR(root);
 		goto out;
@@ -870,7 +870,7 @@ static int create_uuid_tree(struct btrfs_trans_handle *trans)
 	int ret = 0;
 
 	UASSERT(fs_info->uuid_root == NULL);
-	root = btrfs_create_tree(trans, fs_info, &key);
+	root = btrfs_create_tree(trans, &key);
 	if (IS_ERR(root)) {
 		ret = PTR_ERR(root);
 		goto out;
@@ -900,7 +900,7 @@ static int create_global_root(struct btrfs_trans_handle *trans, u64 objectid,
 	};
 	int ret = 0;
 
-	root = btrfs_create_tree(trans, fs_info, &key);
+	root = btrfs_create_tree(trans, &key);
 	if (IS_ERR(root)) {
 		ret = PTR_ERR(root);
 		goto out;
@@ -1127,7 +1127,7 @@ static int setup_raid_stripe_tree_root(struct btrfs_fs_info *fs_info)
 		return ret;
 	}
 
-	stripe_root = btrfs_create_tree(trans, fs_info, &key);
+	stripe_root = btrfs_create_tree(trans, &key);
 	if (IS_ERR(stripe_root))  {
 		ret = PTR_ERR(stripe_root);
 		btrfs_abort_transaction(trans, ret);
-- 
2.45.2

[PATCH v2 2/3] btrfs-progs: introduce btrfs_make_subvolume()

[Qu Wenruo]

There are two different subvolume/data reloc tree creation routines:

- create_subvol() from convert/main.c
  * calls btrfs_copy_root() to create an empty root
    This is not safe, as it relies on the source root to be empty.
  * calls btrfs_read_fs_root() to add it to the cache and trace it
    properly
  * calls btrfs_make_root_dir() to initialize the empty new root

- create_data_reloc_tree() from mkfs/main.c
  * calls btrfs_create_tree() to create an empty rooo
  * Manually add the root to fs_root cache
    This is only safe for data reloc tree as it's never updated
    inside btrfs-progs.
    But not safe for other subvolume trees.
  * manually setup the root dir

Both has its good and bad aspects, so here we introduce a new helper,
btrfs_make_subvolume():

- Calls btrfs_create_tree() to create an empty root
- Calls btrfs_read_fs_root() to setup the cache and tracking properly
- Calls btrfs_make_root_dir() to initialize the root dir
- Calls btrfs_update_root() to reflect the rootdir change

So this new helper can replace both create_subvol() and
create_data_reloc_tree().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Makefile                 |   1 +
 check/main.c             |   1 +
 common/root-tree-utils.c | 108 +++++++++++++++++++++++++++++++++++++++
 common/root-tree-utils.h |  26 ++++++++++
 convert/main.c           |  43 ++--------------
 mkfs/common.c            |  39 --------------
 mkfs/common.h            |   2 -
 mkfs/main.c              |  72 +-------------------------
 8 files changed, 141 insertions(+), 151 deletions(-)
 create mode 100644 common/root-tree-utils.c
 create mode 100644 common/root-tree-utils.h

diff --git a/Makefile b/Makefile
index 82dfb1b40cde..c0428c573933 100644
--- a/Makefile
+++ b/Makefile
@@ -221,6 +221,7 @@ objects = \
 	common/device-utils.o	\
 	common/extent-cache.o	\
 	common/extent-tree-utils.o	\
+	common/root-tree-utils.o	\
 	common/filesystem-utils.o	\
 	common/format-output.o	\
 	common/fsfeatures.o	\
diff --git a/check/main.c b/check/main.c
index 693f77c3542d..8a572c22036e 100644
--- a/check/main.c
+++ b/check/main.c
@@ -59,6 +59,7 @@
 #include "common/open-utils.h"
 #include "common/string-utils.h"
 #include "common/clear-cache.h"
+#include "common/root-tree-utils.h"
 #include "cmds/commands.h"
 #include "mkfs/common.h"
 #include "check/common.h"
diff --git a/common/root-tree-utils.c b/common/root-tree-utils.c
new file mode 100644
index 000000000000..a937037ea4fd
--- /dev/null
+++ b/common/root-tree-utils.c
@@ -0,0 +1,108 @@
+/*
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 021110-1307, USA.
+ */
+
+#include <time.h>
+#include "common/root-tree-utils.h"
+#include "common/rbtree-utils.h"
+#include "common/messages.h"
+#include "kernel-shared/disk-io.h"
+
+int btrfs_make_root_dir(struct btrfs_trans_handle *trans,
+			struct btrfs_root *root, u64 objectid)
+{
+	int ret;
+	struct btrfs_inode_item inode_item;
+	time_t now = time(NULL);
+
+	memset(&inode_item, 0, sizeof(inode_item));
+	btrfs_set_stack_inode_generation(&inode_item, trans->transid);
+	btrfs_set_stack_inode_size(&inode_item, 0);
+	btrfs_set_stack_inode_nlink(&inode_item, 1);
+	btrfs_set_stack_inode_nbytes(&inode_item, root->fs_info->nodesize);
+	btrfs_set_stack_inode_mode(&inode_item, S_IFDIR | 0755);
+	btrfs_set_stack_timespec_sec(&inode_item.atime, now);
+	btrfs_set_stack_timespec_nsec(&inode_item.atime, 0);
+	btrfs_set_stack_timespec_sec(&inode_item.ctime, now);
+	btrfs_set_stack_timespec_nsec(&inode_item.ctime, 0);
+	btrfs_set_stack_timespec_sec(&inode_item.mtime, now);
+	btrfs_set_stack_timespec_nsec(&inode_item.mtime, 0);
+	btrfs_set_stack_timespec_sec(&inode_item.otime, now);
+	btrfs_set_stack_timespec_nsec(&inode_item.otime, 0);
+
+	if (root->fs_info->tree_root == root)
+		btrfs_set_super_root_dir(root->fs_info->super_copy, objectid);
+
+	ret = btrfs_insert_inode(trans, root, objectid, &inode_item);
+	if (ret)
+		goto error;
+
+	ret = btrfs_insert_inode_ref(trans, root, "..", 2, objectid, objectid, 0);
+	if (ret)
+		goto error;
+
+	btrfs_set_root_dirid(&root->root_item, objectid);
+	ret = 0;
+error:
+	return ret;
+}
+
+/*
+ * Create a subvolume and initialize its content with the top inode.
+ *
+ * The created tree root would have its root_ref as 1.
+ * Thus for subvolumes caller needs to properly add ROOT_BACKREF items.
+ */
+int btrfs_make_subvolume(struct btrfs_trans_handle *trans, u64 objectid)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	struct btrfs_root *root;
+	struct btrfs_key key = {
+		.objectid = objectid,
+		.type = BTRFS_ROOT_ITEM_KEY,
+	};
+	int ret;
+
+	/* FSTREE is different and can not be created by this function. */
+	UASSERT(objectid != BTRFS_FS_TREE_OBJECTID);
+	UASSERT(is_fstree(objectid) || objectid == BTRFS_DATA_RELOC_TREE_OBJECTID);
+
+	root = btrfs_create_tree(trans, &key);
+	if (IS_ERR(root)) {
+		ret = PTR_ERR(root);
+		goto error;
+	}
+	/*
+	 * Free it for now, and re-read it from disk to setup cache and
+	 * tracking.
+	 */
+	btrfs_free_fs_root(root);
+	root = btrfs_read_fs_root(fs_info, &key);
+	if (IS_ERR(root)) {
+		ret = PTR_ERR(root);
+		goto error;
+	}
+	ret = btrfs_make_root_dir(trans, root, BTRFS_FIRST_FREE_OBJECTID);
+	if (ret < 0)
+		goto error;
+	ret = btrfs_update_root(trans, fs_info->tree_root, &root->root_key,
+				&root->root_item);
+	if (ret < 0)
+		goto error;
+	return 0;
+error:
+	btrfs_abort_transaction(trans, ret);
+	return ret;
+}
diff --git a/common/root-tree-utils.h b/common/root-tree-utils.h
new file mode 100644
index 000000000000..f4e354c0bf3e
--- /dev/null
+++ b/common/root-tree-utils.h
@@ -0,0 +1,26 @@
+/*
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 021110-1307, USA.
+ */
+
+#ifndef _ROOT_TREE_UTILS_H_
+#define _ROOT_TREE_UTILS_H_
+
+#include "kernel-shared/transaction.h"
+
+int btrfs_make_root_dir(struct btrfs_trans_handle *trans,
+			struct btrfs_root *root, u64 objectid);
+int btrfs_make_subvolume(struct btrfs_trans_handle *trans, u64 objectid);
+
+#endif
diff --git a/convert/main.c b/convert/main.c
index 8e73aa25b1da..31fa146c86b1 100644
--- a/convert/main.c
+++ b/convert/main.c
@@ -120,6 +120,7 @@
 #include "common/box.h"
 #include "common/open-utils.h"
 #include "common/extent-tree-utils.h"
+#include "common/root-tree-utils.h"
 #include "common/clear-cache.h"
 #include "cmds/commands.h"
 #include "check/repair.h"
@@ -916,44 +917,6 @@ out:
 	return ret;
 }
 
-static int create_subvol(struct btrfs_trans_handle *trans,
-			 struct btrfs_root *root, u64 root_objectid)
-{
-	struct extent_buffer *tmp;
-	struct btrfs_root *new_root;
-	struct btrfs_key key;
-	struct btrfs_root_item root_item;
-	int ret;
-
-	ret = btrfs_copy_root(trans, root, root->node, &tmp,
-			      root_objectid);
-	if (ret)
-		return ret;
-
-	memcpy(&root_item, &root->root_item, sizeof(root_item));
-	btrfs_set_root_bytenr(&root_item, tmp->start);
-	btrfs_set_root_level(&root_item, btrfs_header_level(tmp));
-	btrfs_set_root_generation(&root_item, trans->transid);
-	free_extent_buffer(tmp);
-
-	key.objectid = root_objectid;
-	key.type = BTRFS_ROOT_ITEM_KEY;
-	key.offset = trans->transid;
-	ret = btrfs_insert_root(trans, root->fs_info->tree_root,
-				&key, &root_item);
-
-	key.offset = (u64)-1;
-	new_root = btrfs_read_fs_root(root->fs_info, &key);
-	if (!new_root || IS_ERR(new_root)) {
-		error("unable to fs read root: %lu", PTR_ERR(new_root));
-		return PTR_ERR(new_root);
-	}
-
-	ret = btrfs_make_root_dir(trans, new_root, BTRFS_FIRST_FREE_OBJECTID);
-
-	return ret;
-}
-
 /*
  * New make_btrfs() has handle system and meta chunks quite well.
  * So only need to add remaining data chunks.
@@ -1059,13 +1022,13 @@ static int init_btrfs(struct btrfs_mkfs_config *cfg, struct btrfs_root *root,
 			     BTRFS_FIRST_FREE_OBJECTID);
 
 	/* subvol for fs image file */
-	ret = create_subvol(trans, root, CONV_IMAGE_SUBVOL_OBJECTID);
+	ret = btrfs_make_subvolume(trans, CONV_IMAGE_SUBVOL_OBJECTID);
 	if (ret < 0) {
 		error("failed to create subvolume image root: %d", ret);
 		goto err;
 	}
 	/* subvol for data relocation tree */
-	ret = create_subvol(trans, root, BTRFS_DATA_RELOC_TREE_OBJECTID);
+	ret = btrfs_make_subvolume(trans, BTRFS_DATA_RELOC_TREE_OBJECTID);
 	if (ret < 0) {
 		error("failed to create DATA_RELOC root: %d", ret);
 		goto err;
diff --git a/mkfs/common.c b/mkfs/common.c
index 5de3e0b6d59c..e28b1e827ee7 100644
--- a/mkfs/common.c
+++ b/mkfs/common.c
@@ -758,45 +758,6 @@ out:
 	return ret;
 }
 
-int btrfs_make_root_dir(struct btrfs_trans_handle *trans,
-			struct btrfs_root *root, u64 objectid)
-{
-	int ret;
-	struct btrfs_inode_item inode_item;
-	time_t now = time(NULL);
-
-	memset(&inode_item, 0, sizeof(inode_item));
-	btrfs_set_stack_inode_generation(&inode_item, trans->transid);
-	btrfs_set_stack_inode_size(&inode_item, 0);
-	btrfs_set_stack_inode_nlink(&inode_item, 1);
-	btrfs_set_stack_inode_nbytes(&inode_item, root->fs_info->nodesize);
-	btrfs_set_stack_inode_mode(&inode_item, S_IFDIR | 0755);
-	btrfs_set_stack_timespec_sec(&inode_item.atime, now);
-	btrfs_set_stack_timespec_nsec(&inode_item.atime, 0);
-	btrfs_set_stack_timespec_sec(&inode_item.ctime, now);
-	btrfs_set_stack_timespec_nsec(&inode_item.ctime, 0);
-	btrfs_set_stack_timespec_sec(&inode_item.mtime, now);
-	btrfs_set_stack_timespec_nsec(&inode_item.mtime, 0);
-	btrfs_set_stack_timespec_sec(&inode_item.otime, now);
-	btrfs_set_stack_timespec_nsec(&inode_item.otime, 0);
-
-	if (root->fs_info->tree_root == root)
-		btrfs_set_super_root_dir(root->fs_info->super_copy, objectid);
-
-	ret = btrfs_insert_inode(trans, root, objectid, &inode_item);
-	if (ret)
-		goto error;
-
-	ret = btrfs_insert_inode_ref(trans, root, "..", 2, objectid, objectid, 0);
-	if (ret)
-		goto error;
-
-	btrfs_set_root_dirid(&root->root_item, objectid);
-	ret = 0;
-error:
-	return ret;
-}
-
 /*
  * Btrfs minimum size calculation is complicated, it should include at least:
  * 1. system group size
diff --git a/mkfs/common.h b/mkfs/common.h
index de0ff57beee8..c600c16622fa 100644
--- a/mkfs/common.h
+++ b/mkfs/common.h
@@ -103,8 +103,6 @@ struct btrfs_mkfs_config {
 };
 
 int make_btrfs(int fd, struct btrfs_mkfs_config *cfg);
-int btrfs_make_root_dir(struct btrfs_trans_handle *trans,
-			struct btrfs_root *root, u64 objectid);
 u64 btrfs_min_dev_size(u32 nodesize, bool mixed, u64 zone_size, u64 meta_profile,
 		       u64 data_profile);
 int test_minimum_size(const char *file, u64 min_dev_size);
diff --git a/mkfs/main.c b/mkfs/main.c
index 077da1e33fc6..cf5cae45d02a 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -58,6 +58,7 @@
 #include "common/units.h"
 #include "common/string-utils.h"
 #include "common/string-table.h"
+#include "common/root-tree-utils.h"
 #include "cmds/commands.h"
 #include "check/qgroup-verify.h"
 #include "mkfs/common.h"
@@ -734,75 +735,6 @@ static void update_chunk_allocation(struct btrfs_fs_info *fs_info,
 	}
 }
 
-static int create_data_reloc_tree(struct btrfs_trans_handle *trans)
-{
-	struct btrfs_fs_info *fs_info = trans->fs_info;
-	struct btrfs_inode_item *inode;
-	struct btrfs_root *root;
-	struct btrfs_path path = { 0 };
-	struct btrfs_key key = {
-		.objectid = BTRFS_DATA_RELOC_TREE_OBJECTID,
-		.type = BTRFS_ROOT_ITEM_KEY,
-	};
-	u64 ino = BTRFS_FIRST_FREE_OBJECTID;
-	char *name = "..";
-	int ret;
-
-	root = btrfs_create_tree(trans, &key);
-	if (IS_ERR(root)) {
-		ret = PTR_ERR(root);
-		goto out;
-	}
-	/* Update dirid as created tree has default dirid 0 */
-	btrfs_set_root_dirid(&root->root_item, ino);
-	ret = btrfs_update_root(trans, fs_info->tree_root, &root->root_key,
-				&root->root_item);
-	if (ret < 0)
-		goto out;
-
-	/* Cache this tree so it can be cleaned up at close_ctree() */
-	ret = rb_insert(&fs_info->fs_root_tree, &root->rb_node,
-			btrfs_fs_roots_compare_roots);
-	if (ret < 0)
-		goto out;
-
-	/* Insert INODE_ITEM */
-	ret = btrfs_new_inode(trans, root, ino, 0755 | S_IFDIR);
-	if (ret < 0)
-		goto out;
-
-	/* then INODE_REF */
-	ret = btrfs_insert_inode_ref(trans, root, name, strlen(name), ino, ino,
-				     0);
-	if (ret < 0)
-		goto out;
-
-	/* Update nlink of that inode item */
-	key.objectid = ino;
-	key.type = BTRFS_INODE_ITEM_KEY;
-	key.offset = 0;
-
-	ret = btrfs_search_slot(trans, root, &key, &path, 0, 1);
-	if (ret > 0) {
-		ret = -ENOENT;
-		btrfs_release_path(&path);
-		goto out;
-	}
-	if (ret < 0) {
-		btrfs_release_path(&path);
-		goto out;
-	}
-	inode = btrfs_item_ptr(path.nodes[0], path.slots[0],
-			       struct btrfs_inode_item);
-	btrfs_set_inode_nlink(path.nodes[0], inode, 1);
-	btrfs_mark_buffer_dirty(path.nodes[0]);
-	btrfs_release_path(&path);
-	return 0;
-out:
-	btrfs_abort_transaction(trans, ret);
-	return ret;
-}
-
 static int btrfs_uuid_tree_add(struct btrfs_trans_handle *trans, u8 *uuid,
 			       u8 type, u64 subvol_id_cpu)
 {
@@ -1939,7 +1871,7 @@ raid_groups:
 		goto out;
 	}
 
-	ret = create_data_reloc_tree(trans);
+	ret = btrfs_make_subvolume(trans, BTRFS_DATA_RELOC_TREE_OBJECTID);
 	if (ret) {
 		error("unable to create data reloc tree: %d", ret);
 		goto out;
-- 
2.45.2

[PATCH v2 3/3] btrfs-progs: use btrfs_link_subvolume() to replace btrfs_mksubvol()

[Qu Wenruo]

The function btrfs_mksubvol() is very different between btrfs-progs and
kernel, the former version is really just linking a subvolume to another
directory inode, but the kernel version is really to make a completely
new subvolume.

Instead of same-named function, introduce btrfs_link_subvolume() and use
it to replace the old btrfs_mksubvol().

This is done by:

- Introduce btrfs_link_subvolume()
  Which would do extra checks before doing any modification:
  * Make sure the target inode is a directory
  * Make sure no filename conflict

  Then do the linkage:
  * Add the dir_item/dir_index into the parent inode
  * Add the forward and backward root refs into tree root

- Introduce link_image_subvolume() helper
  Currently btrfs_mksubvol() has a dedicated convert filename retry
  behavior, which is unnecessary and should be done by the convert code.

  Now move the filename retry behavior into the helper.

- Remove btrfs_mksubvol()
  Since there is only one caller utilizing btrfs_mksubvol(), and it's
  now gone, we can remove the old btrfs_mksubvol().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c             |   8 +--
 common/root-tree-utils.c | 107 ++++++++++++++++++++++++++++++
 common/root-tree-utils.h |   4 ++
 convert/main.c           |  64 ++++++++++++++++--
 kernel-shared/ctree.h    |   8 +--
 kernel-shared/inode.c    | 140 ++-------------------------------------
 6 files changed, 184 insertions(+), 147 deletions(-)

diff --git a/check/main.c b/check/main.c
index 8a572c22036e..205bbb4a3c73 100644
--- a/check/main.c
+++ b/check/main.c
@@ -2361,10 +2361,10 @@ static int repair_inode_backrefs(struct btrfs_root *root,
 			struct btrfs_trans_handle *trans;
 			struct btrfs_key location;
 
-			ret = check_dir_conflict(root, backref->name,
-						 backref->namelen,
-						 backref->dir,
-						 backref->index);
+			ret = btrfs_check_dir_conflict(root, backref->name,
+						       backref->namelen,
+						       backref->dir,
+						       backref->index);
 			if (ret) {
 				/*
 				 * let nlink fixing routine to handle it,
diff --git a/common/root-tree-utils.c b/common/root-tree-utils.c
index a937037ea4fd..39e4a7be9498 100644
--- a/common/root-tree-utils.c
+++ b/common/root-tree-utils.c
@@ -106,3 +106,110 @@ error:
 	btrfs_abort_transaction(trans, ret);
 	return ret;
 }
+
+/*
+ * Link subvoume @subvol as @name under directory inode @parent_dir of
+ * subvolume @parent_root.
+ */
+int btrfs_link_subvolume(struct btrfs_trans_handle *trans,
+			 struct btrfs_root *parent_root,
+			 u64 parent_dir, const char *name,
+			 int namelen, struct btrfs_root *subvol)
+{
+	struct btrfs_root *tree_root = trans->fs_info->tree_root;
+	struct btrfs_path path = { 0 };
+	struct btrfs_key key;
+	struct btrfs_inode_item *ii;
+	u64 index;
+	u64 isize;
+	u32 imode;
+	int ret;
+
+	UASSERT(namelen && namelen <= BTRFS_NAME_LEN);
+
+	/* Make sure @parent_dir is a directory. */
+	key.objectid = parent_dir;
+	key.type = BTRFS_INODE_ITEM_KEY;
+	key.offset = 0;
+	ret = btrfs_search_slot(NULL, parent_root, &key, &path, 0, 0);
+	if (ret > 0)
+		ret = -ENOENT;
+	if (ret < 0) {
+		btrfs_release_path(&path);
+		return ret;
+	}
+	ii = btrfs_item_ptr(path.nodes[0], path.slots[0], struct btrfs_inode_item);
+	imode = btrfs_inode_mode(path.nodes[0], ii);
+	btrfs_release_path(&path);
+
+	if (!S_ISDIR(imode)) {
+		ret = -EUCLEAN;
+		error("%s: inode %llu of subvolume %llu is not a directory",
+		      __func__, parent_dir, parent_root->root_key.objectid);
+		return ret;
+	}
+
+	ret = btrfs_find_free_dir_index(parent_root, parent_dir, &index);
+	if (ret < 0)
+		return ret;
+
+	/* Filename conflicts check. */
+	ret = btrfs_check_dir_conflict(parent_root, name, namelen, parent_dir,
+				       index);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Now everything is fine, add the link.
+	 * From now on, every error would lead to transaction abort.
+	 *
+	 * Add the dir_item/index first.
+	 */
+	ret = btrfs_insert_dir_item(trans, parent_root, name, namelen,
+				    parent_dir, &subvol->root_key,
+				    BTRFS_FT_DIR, index);
+	if (ret < 0)
+		goto abort;
+
+	/* Update inode size of the parent inode */
+	key.objectid = parent_dir;
+	key.type = BTRFS_INODE_ITEM_KEY;
+	key.offset = 0;
+	ret = btrfs_search_slot(trans, parent_root, &key, &path, 1, 1);
+	if (ret > 0)
+		ret = -ENOENT;
+	if (ret < 0) {
+		btrfs_release_path(&path);
+		goto abort;
+	}
+	ii = btrfs_item_ptr(path.nodes[0], path.slots[0],
+			    struct btrfs_inode_item);
+	isize = btrfs_inode_size(path.nodes[0], ii);
+	isize += namelen * 2;
+	btrfs_set_inode_size(path.nodes[0], ii, isize);
+	btrfs_mark_buffer_dirty(path.nodes[0]);
+	btrfs_release_path(&path);
+
+	/* Add the root backref. */
+	ret = btrfs_add_root_ref(trans, tree_root, subvol->root_key.objectid,
+				 BTRFS_ROOT_BACKREF_KEY,
+				 parent_root->root_key.objectid, parent_dir,
+				 index, name, namelen);
+	if (ret < 0)
+		goto abort;
+
+	/* Then forward ref*/
+	ret = btrfs_add_root_ref(trans, tree_root,
+				 parent_root->root_key.objectid,
+				 BTRFS_ROOT_REF_KEY, subvol->root_key.objectid,
+				 parent_dir, index, name, namelen);
+	if (ret < 0)
+		goto abort;
+	/* For now, all root should have its refs == 1 already.
+	 * So no need to update the root refs. */
+	UASSERT(btrfs_root_refs(&subvol->root_item) == 1);
+	return 0;
+abort:
+	btrfs_abort_transaction(trans, ret);
+	return ret;
+}
diff --git a/common/root-tree-utils.h b/common/root-tree-utils.h
index f4e354c0bf3e..c5318bd9b8db 100644
--- a/common/root-tree-utils.h
+++ b/common/root-tree-utils.h
@@ -22,5 +22,9 @@
 int btrfs_make_root_dir(struct btrfs_trans_handle *trans,
 			struct btrfs_root *root, u64 objectid);
 int btrfs_make_subvolume(struct btrfs_trans_handle *trans, u64 objectid);
+int btrfs_link_subvolume(struct btrfs_trans_handle *trans,
+			 struct btrfs_root *parent_root,
+			 u64 parent_dir, const char *name,
+			 int namelen, struct btrfs_root *subvol);
 
 #endif
diff --git a/convert/main.c b/convert/main.c
index 31fa146c86b1..b2d0d5666737 100644
--- a/convert/main.c
+++ b/convert/main.c
@@ -1110,6 +1110,62 @@ static int convert_open_fs(const char *devname,
 	return -1;
 }
 
+static int link_image_subvolume(struct btrfs_root *image_root, char *name)
+{
+	struct btrfs_fs_info *fs_info = image_root->fs_info;
+	struct btrfs_root *fs_root = fs_info->fs_root;
+	struct btrfs_trans_handle *trans;
+	char buf[BTRFS_NAME_LEN + 1]; /* for snprintf, null terminated. */
+	int ret;
+
+	/*
+	 * 1 root backref 1 root forward ref, 1 dir item 1 dir index,
+	 * and finally one root item.
+	 */
+	trans = btrfs_start_transaction(fs_root, 5);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		errno = -ret;
+		error("failed to start transaction to link subvolume: %m");
+		return PTR_ERR(trans);
+	}
+	ret = btrfs_link_subvolume(trans, fs_root, btrfs_root_dirid(&fs_root->root_item),
+			name, strlen(name), image_root);
+	/* No filename conflicts, all done. */
+	if (!ret)
+		goto commit;
+	/* Other unexpected errors. */
+	if (ret != -EEXIST) {
+		btrfs_abort_transaction(trans, ret);
+		return ret;
+	}
+
+	/* Filename conflicts detected, try different names. */
+	for (int i = 0; i < 1024; i++) {
+		int len;
+
+		len = snprintf(buf, ARRAY_SIZE(buf), "%s%d", name, i);
+		if (len == 0 || len > BTRFS_NAME_LEN) {
+			error("failed to find an alternative name for image_root");
+			ret = -EINVAL;
+			goto abort;
+		}
+		ret = btrfs_link_subvolume(trans, fs_root,
+					   btrfs_root_dirid(&fs_root->root_item),
+					   buf, len, image_root);
+		if (!ret)
+			break;
+		if (ret != -EEXIST)
+			goto abort;
+	}
+commit:
+	ret = btrfs_commit_transaction(trans, fs_info->tree_root);
+	return ret;
+abort:
+	btrfs_abort_transaction(trans, ret);
+	return ret;
+}
+
 static int do_convert(const char *devname, u32 convert_flags, u32 nodesize,
 		const char *fslabel, int progress,
 		struct btrfs_mkfs_features *features, u16 csum_type,
@@ -1276,10 +1332,10 @@ static int do_convert(const char *devname, u32 convert_flags, u32 nodesize,
 		task_deinit(ctx.info);
 	}
 
-	image_root = btrfs_mksubvol(root, subvol_name,
-				    CONV_IMAGE_SUBVOL_OBJECTID, true);
-	if (!image_root) {
-		error("unable to link subvolume %s", subvol_name);
+	ret = link_image_subvolume(image_root, subvol_name);
+	if (ret < 0) {
+		errno = -ret;
+		error("unable to link subvolume %s: %m", subvol_name);
 		goto fail;
 	}
 
diff --git a/kernel-shared/ctree.h b/kernel-shared/ctree.h
index 1341a418726b..2388879d1db3 100644
--- a/kernel-shared/ctree.h
+++ b/kernel-shared/ctree.h
@@ -1212,8 +1212,10 @@ static inline int is_fstree(u64 rootid)
 void btrfs_uuid_to_key(const u8 *uuid, u8 type, struct btrfs_key *key);
 
 /* inode.c */
-int check_dir_conflict(struct btrfs_root *root, char *name, int namelen,
-		u64 dir, u64 index);
+int btrfs_find_free_dir_index(struct btrfs_root *root, u64 dir_ino,
+			      u64 *ret_ino);
+int btrfs_check_dir_conflict(struct btrfs_root *root, const char *name,
+			     int namelen, u64 dir, u64 index);
 int btrfs_new_inode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		u64 ino, u32 mode);
 int btrfs_change_inode_flags(struct btrfs_trans_handle *trans,
@@ -1229,8 +1231,6 @@ int btrfs_add_orphan_item(struct btrfs_trans_handle *trans,
 			  u64 ino);
 int btrfs_mkdir(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		char *name, int namelen, u64 parent_ino, u64 *ino, int mode);
-struct btrfs_root *btrfs_mksubvol(struct btrfs_root *root, const char *base,
-				  u64 root_objectid, bool convert);
 int btrfs_find_free_objectid(struct btrfs_trans_handle *trans,
 			     struct btrfs_root *fs_root,
 			     u64 dirid, u64 *objectid);
diff --git a/kernel-shared/inode.c b/kernel-shared/inode.c
index 91b4f629f3ac..5927af041dbf 100644
--- a/kernel-shared/inode.c
+++ b/kernel-shared/inode.c
@@ -40,8 +40,8 @@
  * Find a free inode index for later btrfs_add_link().
  * Currently just search from the largest dir_index and +1.
  */
-static int btrfs_find_free_dir_index(struct btrfs_root *root, u64 dir_ino,
-				     u64 *ret_ino)
+int btrfs_find_free_dir_index(struct btrfs_root *root, u64 dir_ino,
+			      u64 *ret_ino)
 {
 	struct btrfs_path *path;
 	struct btrfs_key key;
@@ -93,8 +93,8 @@ out:
 }
 
 /* Check the dir_item/index conflicts before insert */
-int check_dir_conflict(struct btrfs_root *root, char *name, int namelen,
-		       u64 dir, u64 index)
+int btrfs_check_dir_conflict(struct btrfs_root *root, const char *name,
+			     int namelen, u64 dir, u64 index)
 {
 	struct btrfs_path *path;
 	struct btrfs_key key;
@@ -190,7 +190,7 @@ int btrfs_add_link(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 			goto out;
 	}
 
-	ret = check_dir_conflict(root, name, namelen, parent_ino, ret_index);
+	ret = btrfs_check_dir_conflict(root, name, namelen, parent_ino, ret_index);
 	if (ret < 0 && !(ignore_existed && ret == -EEXIST))
 		goto out;
 
@@ -582,136 +582,6 @@ out:
 	return ret;
 }
 
-struct btrfs_root *btrfs_mksubvol(struct btrfs_root *root,
-				  const char *base, u64 root_objectid,
-				  bool convert)
-{
-	struct btrfs_trans_handle *trans;
-	struct btrfs_fs_info *fs_info = root->fs_info;
-	struct btrfs_root *tree_root = fs_info->tree_root;
-	struct btrfs_root *new_root = NULL;
-	struct btrfs_path path = { 0 };
-	struct btrfs_inode_item *inode_item;
-	struct extent_buffer *leaf;
-	struct btrfs_key key;
-	u64 dirid = btrfs_root_dirid(&root->root_item);
-	u64 index = 2;
-	char buf[BTRFS_NAME_LEN + 1]; /* for snprintf null */
-	int len;
-	int i;
-	int ret;
-
-	len = strlen(base);
-	if (len == 0 || len > BTRFS_NAME_LEN)
-		return NULL;
-
-	key.objectid = dirid;
-	key.type = BTRFS_DIR_INDEX_KEY;
-	key.offset = (u64)-1;
-
-	ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0);
-	if (ret <= 0) {
-		error("search for DIR_INDEX dirid %llu failed: %d",
-				(unsigned long long)dirid, ret);
-		goto fail;
-	}
-
-	if (path.slots[0] > 0) {
-		path.slots[0]--;
-		btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]);
-		if (key.objectid == dirid && key.type == BTRFS_DIR_INDEX_KEY)
-			index = key.offset + 1;
-	}
-	btrfs_release_path(&path);
-
-	trans = btrfs_start_transaction(root, 1);
-	if (IS_ERR(trans)) {
-		ret = PTR_ERR(trans);
-		errno = -ret;
-		error_msg(ERROR_MSG_START_TRANS, "%m");
-		goto fail;
-	}
-
-	key.objectid = dirid;
-	key.type =  BTRFS_INODE_ITEM_KEY;
-	key.offset = 0;
-
-	ret = btrfs_lookup_inode(trans, root, &path, &key, 1);
-	if (ret) {
-		error("search for INODE_ITEM %llu failed: %d",
-				(unsigned long long)dirid, ret);
-		goto fail;
-	}
-	leaf = path.nodes[0];
-	inode_item = btrfs_item_ptr(leaf, path.slots[0],
-				    struct btrfs_inode_item);
-
-	key.objectid = root_objectid;
-	key.type = BTRFS_ROOT_ITEM_KEY;
-	key.offset = (u64)-1;
-
-	memcpy(buf, base, len);
-	if (convert) {
-		for (i = 0; i < 1024; i++) {
-			ret = btrfs_insert_dir_item(trans, root, buf, len,
-					dirid, &key, BTRFS_FT_DIR, index);
-			if (ret != -EEXIST)
-				break;
-			len = snprintf(buf, ARRAY_SIZE(buf), "%s%d", base, i);
-			if (len < 1 || len > BTRFS_NAME_LEN) {
-				ret = -EINVAL;
-				break;
-			}
-		}
-	} else {
-		ret = btrfs_insert_dir_item(trans, root, buf, len, dirid, &key,
-					    BTRFS_FT_DIR, index);
-	}
-	if (ret)
-		goto fail;
-
-	btrfs_set_inode_size(leaf, inode_item, len * 2 +
-			     btrfs_inode_size(leaf, inode_item));
-	btrfs_mark_buffer_dirty(leaf);
-	btrfs_release_path(&path);
-
-	/* add the backref first */
-	ret = btrfs_add_root_ref(trans, tree_root, root_objectid,
-				 BTRFS_ROOT_BACKREF_KEY,
-				 root->root_key.objectid,
-				 dirid, index, buf, len);
-	if (ret) {
-		error("unable to add root backref for %llu: %d",
-				root->root_key.objectid, ret);
-		goto fail;
-	}
-
-	/* now add the forward ref */
-	ret = btrfs_add_root_ref(trans, tree_root, root->root_key.objectid,
-				 BTRFS_ROOT_REF_KEY, root_objectid,
-				 dirid, index, buf, len);
-	if (ret) {
-		error("unable to add root ref for %llu: %d",
-				root->root_key.objectid, ret);
-		goto fail;
-	}
-
-	ret = btrfs_commit_transaction(trans, root);
-	if (ret) {
-		errno = -ret;
-		error_msg(ERROR_MSG_COMMIT_TRANS, "%m");
-		goto fail;
-	}
-
-	new_root = btrfs_read_fs_root(fs_info, &key);
-	if (IS_ERR(new_root)) {
-		error("unable to fs read root: %lu", PTR_ERR(new_root));
-		new_root = NULL;
-	}
-fail:
-	return new_root;
-}
-
 /*
  * Walk the tree of allocated inodes and find a hole.
  */
-- 
2.45.2