Re: [PATCH v2] btrfs: use dynamic allocation for root item in create_subvol

[Tsutomu Itoh <t-itoh@jp.fujitsu.com>]

On 2016/04/25 20:18, David Sterba wrote:
> The size of root item is more than 400 bytes, which is quite a lot of
> stack space. As we do IO from inside the subvolume ioctls, we should
> keep the stack usage low in case the filesystem is on top of other
> layers (NFS, device mapper, iscsi, etc).
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

Looks good to me.

Reviewed-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>

> ---
>   fs/btrfs/ioctl.c | 65 ++++++++++++++++++++++++++++++++------------------------
>   1 file changed, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 053e677839fe..9a63fe07bc2e 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -439,7 +439,7 @@ static noinline int create_subvol(struct inode *dir,
>   {
>   	struct btrfs_trans_handle *trans;
>   	struct btrfs_key key;
> -	struct btrfs_root_item root_item;
> +	struct btrfs_root_item *root_item;
>   	struct btrfs_inode_item *inode_item;
>   	struct extent_buffer *leaf;
>   	struct btrfs_root *root = BTRFS_I(dir)->root;
> @@ -455,16 +455,22 @@ static noinline int create_subvol(struct inode *dir,
>   	u64 qgroup_reserved;
>   	uuid_le new_uuid;
>   
> +	root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
> +	if (!root_item)
> +		return -ENOMEM;
> +
>   	ret = btrfs_find_free_objectid(root->fs_info->tree_root, &objectid);
>   	if (ret)
> -		return ret;
> +		goto fail_free;
>   
>   	/*
>   	 * Don't create subvolume whose level is not zero. Or qgroup will be
>   	 * screwed up since it assume subvolme qgroup's level to be 0.
>   	 */
> -	if (btrfs_qgroup_level(objectid))
> -		return -ENOSPC;
> +	if (btrfs_qgroup_level(objectid)) {
> +		ret = -ENOSPC;
> +		goto fail_free;
> +	}
>   
>   	btrfs_init_block_rsv(&block_rsv, BTRFS_BLOCK_RSV_TEMP);
>   	/*
> @@ -474,14 +480,14 @@ static noinline int create_subvol(struct inode *dir,
>   	ret = btrfs_subvolume_reserve_metadata(root, &block_rsv,
>   					       8, &qgroup_reserved, false);
>   	if (ret)
> -		return ret;
> +		goto fail_free;
>   
>   	trans = btrfs_start_transaction(root, 0);
>   	if (IS_ERR(trans)) {
>   		ret = PTR_ERR(trans);
>   		btrfs_subvolume_release_metadata(root, &block_rsv,
>   						 qgroup_reserved);
> -		return ret;
> +		goto fail_free;
>   	}
>   	trans->block_rsv = &block_rsv;
>   	trans->bytes_reserved = block_rsv.size;
> @@ -509,47 +515,45 @@ static noinline int create_subvol(struct inode *dir,
>   			    BTRFS_UUID_SIZE);
>   	btrfs_mark_buffer_dirty(leaf);
>   
> -	memset(&root_item, 0, sizeof(root_item));
> -
> -	inode_item = &root_item.inode;
> +	inode_item = &root_item->inode;
>   	btrfs_set_stack_inode_generation(inode_item, 1);
>   	btrfs_set_stack_inode_size(inode_item, 3);
>   	btrfs_set_stack_inode_nlink(inode_item, 1);
>   	btrfs_set_stack_inode_nbytes(inode_item, root->nodesize);
>   	btrfs_set_stack_inode_mode(inode_item, S_IFDIR | 0755);
>   
> -	btrfs_set_root_flags(&root_item, 0);
> -	btrfs_set_root_limit(&root_item, 0);
> +	btrfs_set_root_flags(root_item, 0);
> +	btrfs_set_root_limit(root_item, 0);
>   	btrfs_set_stack_inode_flags(inode_item, BTRFS_INODE_ROOT_ITEM_INIT);
>   
> -	btrfs_set_root_bytenr(&root_item, leaf->start);
> -	btrfs_set_root_generation(&root_item, trans->transid);
> -	btrfs_set_root_level(&root_item, 0);
> -	btrfs_set_root_refs(&root_item, 1);
> -	btrfs_set_root_used(&root_item, leaf->len);
> -	btrfs_set_root_last_snapshot(&root_item, 0);
> +	btrfs_set_root_bytenr(root_item, leaf->start);
> +	btrfs_set_root_generation(root_item, trans->transid);
> +	btrfs_set_root_level(root_item, 0);
> +	btrfs_set_root_refs(root_item, 1);
> +	btrfs_set_root_used(root_item, leaf->len);
> +	btrfs_set_root_last_snapshot(root_item, 0);
>   
> -	btrfs_set_root_generation_v2(&root_item,
> -			btrfs_root_generation(&root_item));
> +	btrfs_set_root_generation_v2(root_item,
> +			btrfs_root_generation(root_item));
>   	uuid_le_gen(&new_uuid);
> -	memcpy(root_item.uuid, new_uuid.b, BTRFS_UUID_SIZE);
> -	btrfs_set_stack_timespec_sec(&root_item.otime, cur_time.tv_sec);
> -	btrfs_set_stack_timespec_nsec(&root_item.otime, cur_time.tv_nsec);
> -	root_item.ctime = root_item.otime;
> -	btrfs_set_root_ctransid(&root_item, trans->transid);
> -	btrfs_set_root_otransid(&root_item, trans->transid);
> +	memcpy(root_item->uuid, new_uuid.b, BTRFS_UUID_SIZE);
> +	btrfs_set_stack_timespec_sec(&root_item->otime, cur_time.tv_sec);
> +	btrfs_set_stack_timespec_nsec(&root_item->otime, cur_time.tv_nsec);
> +	root_item->ctime = root_item->otime;
> +	btrfs_set_root_ctransid(root_item, trans->transid);
> +	btrfs_set_root_otransid(root_item, trans->transid);
>   
>   	btrfs_tree_unlock(leaf);
>   	free_extent_buffer(leaf);
>   	leaf = NULL;
>   
> -	btrfs_set_root_dirid(&root_item, new_dirid);
> +	btrfs_set_root_dirid(root_item, new_dirid);
>   
>   	key.objectid = objectid;
>   	key.offset = 0;
>   	key.type = BTRFS_ROOT_ITEM_KEY;
>   	ret = btrfs_insert_root(trans, root->fs_info->tree_root, &key,
> -				&root_item);
> +				root_item);
>   	if (ret)
>   		goto fail;
>   
> @@ -601,12 +605,13 @@ static noinline int create_subvol(struct inode *dir,
>   	BUG_ON(ret);
>   
>   	ret = btrfs_uuid_tree_add(trans, root->fs_info->uuid_root,
> -				  root_item.uuid, BTRFS_UUID_KEY_SUBVOL,
> +				  root_item->uuid, BTRFS_UUID_KEY_SUBVOL,
>   				  objectid);
>   	if (ret)
>   		btrfs_abort_transaction(trans, root, ret);
>   
>   fail:
> +	kfree(root_item);
>   	trans->block_rsv = NULL;
>   	trans->bytes_reserved = 0;
>   	btrfs_subvolume_release_metadata(root, &block_rsv, qgroup_reserved);
> @@ -629,6 +634,10 @@ static noinline int create_subvol(struct inode *dir,
>   		d_instantiate(dentry, inode);
>   	}
>   	return ret;
> +
> +fail_free:
> +	kfree(root_item);
> +	return ret;
>   }
>   
>   static void btrfs_wait_for_no_snapshoting_writes(struct btrfs_root *root)
>