Re: [PATCH 4/6] btrfs-progs: use a unified btrfs_make_subvol() implementation

[Josef Bacik <josef@toxicpanda.com>]

On Mon, Oct 16, 2023 at 03:08:50PM +1030, Qu Wenruo wrote:
> To create a subvolume there are several different helpers:
> 
> - create_subvol() from convert/main.c
>   This relies one using an empty fs_root tree to copy its root item.
>   But otherwise has a pretty well wrapper btrfs_ake_root_dir() helper to
>   handle the inode item/ref insertion.
> 
> - create_data_reloc_tree() from mkfs/main.c
>   This is already pretty usable for generic subvolume creation, the only
>   bad code is the open-coded rootdir setup.
> 
> So here this patch introduce a better version with all the benefit from
> the above implementations:
> 
> - Move btrfs_make_root_dir() into kernel-shared/root-tree.[ch]
> 
> - Implement a unified btrfs_make_subvol() to replace above two implementations
>   It would go with btrfs_create_root(), and btrfs_make_root_dir() to
>   remove duplicated code, and return a btrfs_root pointer for caller
>   to utilize.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  convert/main.c            | 55 +++++--------------------
>  kernel-shared/ctree.h     |  4 ++
>  kernel-shared/root-tree.c | 86 +++++++++++++++++++++++++++++++++++++++
>  mkfs/common.c             | 39 ------------------
>  mkfs/common.h             |  2 -
>  mkfs/main.c               | 78 +++--------------------------------
>  6 files changed, 107 insertions(+), 157 deletions(-)
> 
> diff --git a/convert/main.c b/convert/main.c
> index 73740fe26d55..453e2c003c20 100644
> --- a/convert/main.c
> +++ b/convert/main.c
> @@ -915,44 +915,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.
> @@ -1012,6 +974,7 @@ static int make_convert_data_block_groups(struct btrfs_trans_handle *trans,
>  static int init_btrfs(struct btrfs_mkfs_config *cfg, struct btrfs_root *root,
>  			 struct btrfs_convert_context *cctx, u32 convert_flags)
>  {
> +	struct btrfs_root *created_root;
>  	struct btrfs_key location;
>  	struct btrfs_trans_handle *trans;
>  	struct btrfs_fs_info *fs_info = root->fs_info;
> @@ -1057,15 +1020,19 @@ 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);
> -	if (ret < 0) {
> -		error("failed to create subvolume image root: %d", ret);
> +	created_root = btrfs_create_subvol(trans, CONV_IMAGE_SUBVOL_OBJECTID);
> +	if (IS_ERR(created_root)) {
> +		ret = PTR_ERR(created_root);
> +		errno = -ret;
> +		error("failed to create subvolume image root: %m");
>  		goto err;
>  	}
>  	/* subvol for data relocation tree */
> -	ret = create_subvol(trans, root, BTRFS_DATA_RELOC_TREE_OBJECTID);
> -	if (ret < 0) {
> -		error("failed to create DATA_RELOC root: %d", ret);
> +	created_root = btrfs_create_subvol(trans, BTRFS_DATA_RELOC_TREE_OBJECTID);
> +	if (IS_ERR(created_root)) {
> +		ret = PTR_ERR(created_root);
> +		errno = -ret;
> +		error("failed to create DATA_RELOC root: %m");
>  		goto err;
>  	}
>  
> diff --git a/kernel-shared/ctree.h b/kernel-shared/ctree.h
> index 1dda40e96a60..ea459464063d 100644
> --- a/kernel-shared/ctree.h
> +++ b/kernel-shared/ctree.h
> @@ -1134,6 +1134,10 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
>  		      *item);
>  int btrfs_find_last_root(struct btrfs_root *root, u64 objectid, struct
>  			 btrfs_root_item *item, struct btrfs_key *key);
> +int btrfs_make_root_dir(struct btrfs_trans_handle *trans,
> +			struct btrfs_root *root, u64 objectid);
> +struct btrfs_root *btrfs_create_subvol(struct btrfs_trans_handle *trans,
> +				       u64 objectid);
>  /* dir-item.c */
>  int btrfs_insert_dir_item(struct btrfs_trans_handle *trans, struct btrfs_root
>  			  *root, const char *name, int name_len, u64 dir,
> diff --git a/kernel-shared/root-tree.c b/kernel-shared/root-tree.c
> index 33f9e4697b18..1fe7d535fdc7 100644
> --- a/kernel-shared/root-tree.c
> +++ b/kernel-shared/root-tree.c

We're moving towards a world where kernel-shared will be an exact-ish copy of
the kernel code.  Please put helpers like this in common/, I did this for
several of the extent tree related helpers we need for fsck, this is a good fit
for that.  Thanks,

Josef