[RFC PATCH v2 0/4] undelete subvolume online version

[RFC PATCH v2 0/4] undelete subvolume online version

[Lu Fengqi]

This patchset will add the BTRFS_IOC_SUBVOL_UNDELETE ioctl for online
btrfs subvolume undelete.

And using the online_undelete version of btrfs-progs, user can recover
the subvolume given by <subvol_id> to the directory given by <dest>. The
optional parameter [-n <name>] can be used to set the name of the
recovered subvolume.

# btrfs subvolume undelete [-n <name>] <subvol_id> <dest>

btrfs online undelete version:
https://github.com/littleroad/linux.git undelete

btrfs-progs online undelete version:
https://github.com/littleroad/btrfs-progs.git online_undelete

Issue: #82

Lu Fengqi (4):
  btrfs: factor out btrfs_link_subvol from create_subvol
  btrfs: don't BUG_ON() in btrfs_link_subvol()
  btrfs: undelete: introduce btrfs_undelete_subvolume
  btrfs: undelete: Add BTRFS_IOCTL_SUBVOL_UNDELETE ioctl

 fs/btrfs/ioctl.c           | 247 +++++++++++++++++++++++++++++++++----
 include/uapi/linux/btrfs.h |   7 ++
 2 files changed, 231 insertions(+), 23 deletions(-)

-- 
2.18.0

[RFC PATCH v2 1/4] btrfs: factor out btrfs_link_subvol from create_subvol

[Lu Fengqi]

The function btrfs_link_subvol is responsible to link the subvolume to
the specified directory, which is the opposite of what
btrfs_unlink_subvol does.

No functional change.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c | 64 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 4905d13dee0a..1b03d07acde2 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -542,6 +542,45 @@ int btrfs_is_empty_uuid(u8 *uuid)
 	return 1;
 }
 
+static int btrfs_link_subvol(struct btrfs_trans_handle *trans,
+			     struct inode *dir, u64 objectid, const char *name,
+			     int namelen)
+{
+	struct btrfs_root *root = BTRFS_I(dir)->root;
+	struct btrfs_key key;
+	u64 index = 0;
+	int ret;
+
+	/*
+	 * insert the directory item
+	 */
+	ret = btrfs_set_inode_index(BTRFS_I(dir), &index);
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		return ret;
+	}
+
+	key.objectid = objectid;
+	key.type = BTRFS_ROOT_ITEM_KEY;
+	key.offset = -1;
+	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
+				    BTRFS_FT_DIR, index);
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		return ret;
+	}
+
+	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
+	ret = btrfs_update_inode(trans, root, dir);
+	BUG_ON(ret);
+
+	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
+				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
+	BUG_ON(ret);
+
+	return ret;
+}
+
 static noinline int create_subvol(struct inode *dir,
 				  struct dentry *dentry,
 				  const char *name, int namelen,
@@ -563,7 +602,6 @@ static noinline int create_subvol(struct inode *dir,
 	int err;
 	u64 objectid;
 	u64 new_dirid = BTRFS_FIRST_FREE_OBJECTID;
-	u64 index = 0;
 	uuid_le new_uuid;
 
 	root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
@@ -677,29 +715,9 @@ static noinline int create_subvol(struct inode *dir,
 	new_root->highest_objectid = new_dirid;
 	mutex_unlock(&new_root->objectid_mutex);
 
-	/*
-	 * insert the directory item
-	 */
-	ret = btrfs_set_inode_index(BTRFS_I(dir), &index);
-	if (ret) {
-		btrfs_abort_transaction(trans, ret);
-		goto fail;
-	}
-
-	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
-				    BTRFS_FT_DIR, index);
-	if (ret) {
-		btrfs_abort_transaction(trans, ret);
+	ret = btrfs_link_subvol(trans, dir, objectid, name, namelen);
+	if (ret)
 		goto fail;
-	}
-
-	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
-	ret = btrfs_update_inode(trans, root, dir);
-	BUG_ON(ret);
-
-	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
-				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
-	BUG_ON(ret);
 
 	ret = btrfs_uuid_tree_add(trans, root_item->uuid,
 				  BTRFS_UUID_KEY_SUBVOL, objectid);
-- 
2.18.0

[RFC PATCH v2 2/4] btrfs: don't BUG_ON() in btrfs_link_subvol()

[Lu Fengqi]

Both of btrfs_update_inode() and btrfs_add_root_ref() may fail because
of ENOMEM. So there's no reason to panic here, we can replace BUG_ON()
with btrfs_abort_transaction() here.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 1b03d07acde2..f6173d4e7ced 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -572,11 +572,17 @@ static int btrfs_link_subvol(struct btrfs_trans_handle *trans,
 
 	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
 	ret = btrfs_update_inode(trans, root, dir);
-	BUG_ON(ret);
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		return ret;
+	}
 
 	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
 				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
-	BUG_ON(ret);
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		return ret;
+	}
 
 	return ret;
 }
-- 
2.18.0

[RFC PATCH v2 3/4] btrfs: undelete: introduce btrfs_undelete_subvolume

[Lu Fengqi]

The function will do the following things which are almost the opposite
of what btrfs_delete_subvolume() does:

1. link the subvolume to the parent specified;
2. clear root flag and set root_refs to 1;
3. add the subvol to the uuid_tree;
4. delete the orphan_item.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index f6173d4e7ced..f088dea53c16 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1880,6 +1880,119 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
 	return ret;
 }
 
+static int btrfs_undelete_subvolume(struct btrfs_root *root,
+				    struct dentry *parent, const char *name,
+				    int namelen)
+{
+	struct inode *dir = d_inode(parent);
+	struct btrfs_fs_info *fs_info = btrfs_sb(dir->i_sb);
+	struct btrfs_root_item *root_item = &root->root_item;
+	struct btrfs_trans_handle *trans;
+	struct btrfs_block_rsv block_rsv;
+	struct dentry *dentry;
+	struct inode *inode;
+	u64 root_flags;
+	int ret;
+
+	btrfs_debug(fs_info, "Undelete subvolume %llu",
+				root->root_key.objectid);
+
+	/* only care about the intact subvolume */
+	if (btrfs_disk_key_objectid(&root_item->drop_progress) != 0)
+		return 0;
+
+	ret = down_write_killable_nested(&dir->i_rwsem, I_MUTEX_PARENT);
+	if (ret == -EINTR)
+		return ret;
+
+	dentry = lookup_one_len(name, parent, namelen);
+	if (IS_ERR(dentry)) {
+		ret = PTR_ERR(dentry);
+		goto out_unlock;
+	}
+
+	down_write(&fs_info->subvol_sem);
+
+	ret = btrfs_may_create(dir, dentry);
+	if (ret)
+		goto out_up_write;
+
+	ret = btrfs_check_dir_item_collision(BTRFS_I(dir)->root, dir->i_ino,
+					     name, namelen);
+	if (ret)
+		goto out_up_write;
+
+	btrfs_init_block_rsv(&block_rsv, BTRFS_BLOCK_RSV_TEMP);
+	/*
+	 * 1 - parent dir inode
+	 * 2 - dir entries
+	 * 2 - root ref/backref
+	 * 1 - UUID item
+	 */
+	ret = btrfs_subvolume_reserve_metadata(root, &block_rsv, 6, false);
+	if (ret)
+		goto out_up_write;
+
+	trans = btrfs_start_transaction(BTRFS_I(dir)->root, 0);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		btrfs_subvolume_release_metadata(fs_info, &block_rsv);
+		goto out_up_write;
+	}
+
+	trans->block_rsv = &block_rsv;
+	trans->bytes_reserved = block_rsv.size;
+
+	ret = btrfs_link_subvol(trans, dir, root->root_key.objectid, name,
+				namelen);
+	if (ret)
+		goto fail;
+
+	/* clear BTRFS_ROOT_SUBVOL_DEAD root flag and set root_refs to 1*/
+	root_flags = btrfs_root_flags(root_item);
+	btrfs_set_root_flags(root_item,
+			     root_flags & ~BTRFS_ROOT_SUBVOL_DEAD);
+	btrfs_set_root_refs(root_item, 1);
+	ret = btrfs_update_root(trans, fs_info->tree_root,
+				&root->root_key, &root->root_item);
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		goto fail;
+	}
+
+	ret = btrfs_uuid_tree_add(trans, root_item->uuid, BTRFS_UUID_KEY_SUBVOL,
+				  root->root_key.objectid);
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		goto fail;
+	}
+
+	ret = btrfs_del_orphan_item(trans, fs_info->tree_root,
+				    root->root_key.objectid);
+	if (ret && ret != -ENOENT) {
+		btrfs_abort_transaction(trans, ret);
+		goto fail;
+	}
+fail:
+	trans->block_rsv = NULL;
+	trans->bytes_reserved = 0;
+	btrfs_subvolume_release_metadata(fs_info, &block_rsv);
+	ret = btrfs_commit_transaction(trans);
+	if (!ret) {
+		inode = btrfs_lookup_dentry(dir, dentry);
+		if (IS_ERR(inode))
+			return PTR_ERR(inode);
+		d_instantiate(dentry, inode);
+		fsnotify_mkdir(dir, dentry);
+	}
+out_up_write:
+	up_write(&fs_info->subvol_sem);
+	dput(dentry);
+out_unlock:
+	inode_unlock(dir);
+	return ret;
+}
+
 static noinline int btrfs_ioctl_subvol_getflags(struct file *file,
 						void __user *arg)
 {
-- 
2.18.0

[RFC PATCH v2 4/4] btrfs: undelete: Add BTRFS_IOCTL_SUBVOL_UNDELETE ioctl

[Lu Fengqi]

This ioctl will provide user the ability to recover the subvolume of the
given id to the given directory.

Note: It will lock fs_info->cleaner_mutex to keep the cleaner kthread
from deleting the subvolume which we want to recover.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c           | 64 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/btrfs.h |  7 +++++
 2 files changed, 71 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index f088dea53c16..3ddf6e1c117b 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1993,6 +1993,68 @@ static int btrfs_undelete_subvolume(struct btrfs_root *root,
 	return ret;
 }
 
+static int btrfs_ioctl_undelete(struct file *file, void __user *argp)
+{
+	struct btrfs_ioctl_subvol_undelete_args *args;
+	struct inode *inode = file_inode(file);
+	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
+	struct btrfs_root *root;
+	int ret = 0;
+
+	if (!S_ISDIR(inode->i_mode))
+		return -ENOTDIR;
+
+	args = memdup_user(argp, sizeof(*args));
+	if (IS_ERR(args))
+		return PTR_ERR(args);
+
+	args->name[BTRFS_PATH_NAME_MAX] = '\0';
+
+	if (!capable(CAP_SYS_ADMIN)) {
+		ret = -EPERM;
+		goto free;
+	}
+
+	ret = mnt_want_write_file(file);
+	if (ret)
+		goto free;
+
+	ret = -ENOENT;
+	spin_lock(&fs_info->trans_lock);
+	list_for_each_entry(root, &fs_info->dead_roots, root_list) {
+		if (root->root_key.objectid == args->subvol_id) {
+			list_del_init(&root->root_list);
+			ret = 0;
+			break;
+		}
+	}
+	spin_unlock(&fs_info->trans_lock);
+	if (ret)
+		goto drop_write;
+
+	/*
+	 * Lock cleaner_mutex to prevent the cleaner kthread from deleting the
+	 * subvolume we want to recover so that we can perform the next rescue
+	 * in a relaxed manner.
+	 */
+	mutex_lock(&fs_info->cleaner_mutex);
+
+	ret = btrfs_undelete_subvolume(root, file->f_path.dentry, args->name,
+				       strlen(args->name));
+	if (ret) {
+		btrfs_add_dead_root(root);
+		goto unlock;
+	}
+
+unlock:
+	mutex_unlock(&fs_info->cleaner_mutex);
+drop_write:
+	mnt_drop_write_file(file);
+free:
+	kfree(args);
+	return ret;
+}
+
 static noinline int btrfs_ioctl_subvol_getflags(struct file *file,
 						void __user *arg)
 {
@@ -6118,6 +6180,8 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_get_subvol_rootref(file, argp);
 	case BTRFS_IOC_INO_LOOKUP_USER:
 		return btrfs_ioctl_ino_lookup_user(file, argp);
+	case BTRFS_IOC_SUBVOL_UNDELETE:
+		return btrfs_ioctl_undelete(file, argp);
 	}
 
 	return -ENOTTY;
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index 5ca1d21fc4a7..e6d3c8e24bb8 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -816,6 +816,11 @@ struct btrfs_ioctl_get_subvol_rootref_args {
 		__u8 align[7];
 };
 
+struct btrfs_ioctl_subvol_undelete_args {
+	__u64 subvol_id;
+	char name[BTRFS_PATH_NAME_MAX + 1];
+};
+
 /* Error codes as returned by the kernel */
 enum btrfs_err_code {
 	BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET = 1,
@@ -940,5 +945,7 @@ enum btrfs_err_code {
 				struct btrfs_ioctl_get_subvol_rootref_args)
 #define BTRFS_IOC_INO_LOOKUP_USER _IOWR(BTRFS_IOCTL_MAGIC, 62, \
 				struct btrfs_ioctl_ino_lookup_user_args)
+#define BTRFS_IOC_SUBVOL_UNDELETE _IOWR(BTRFS_IOCTL_MAGIC, 63, \
+				struct btrfs_ioctl_subvol_undelete_args)
 
 #endif /* _UAPI_LINUX_BTRFS_H */
-- 
2.18.0

[RFC PATCH v2 1/2] btrfs-progs: ioctl: add BTRFS_IOC_SUBVOL_UNDELETE to ioctl.h

[Lu Fengqi]

Copied from uapi/linux/btrfs.h.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 ioctl.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/ioctl.h b/ioctl.h
index 709e996f401c..75978a4e8265 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -670,6 +670,11 @@ struct btrfs_ioctl_send_args_64 {
 } __attribute__((packed));
 BUILD_ASSERT(sizeof(struct btrfs_ioctl_send_args_64) == 72);
 
+struct btrfs_ioctl_subvol_undelete_args {
+	__u64 subvol_id;
+	char name[BTRFS_PATH_NAME_MAX + 1];
+};
+
 #define BTRFS_IOC_SEND_64_COMPAT_DEFINED 1
 
 /* Error codes as returned by the kernel */
@@ -828,6 +833,8 @@ static inline char *btrfs_err_str(enum btrfs_err_code err_code)
                                   struct btrfs_ioctl_feature_flags[3])
 #define BTRFS_IOC_RM_DEV_V2	_IOW(BTRFS_IOCTL_MAGIC, 58, \
 				   struct btrfs_ioctl_vol_args_v2)
+#define BTRFS_IOC_SUBVOL_UNDELETE _IOWR(BTRFS_IOCTL_MAGIC, 63, \
+				struct btrfs_ioctl_subvol_undelete_args)
 #ifdef __cplusplus
 }
 #endif
-- 
2.18.0

[RFC PATCH v2 2/2] btrfs-progs: subvolume: undelete: add btrfs subvolume undelete subcommand

[Lu Fengqi]

Add the undelete subcommand, this is depend on the
BTRFS_IOC_SUBVOL_UNDELETE ioctl.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 btrfs-completion |  2 +-
 cmds-subvolume.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/btrfs-completion b/btrfs-completion
index ae683f4ecf61..2b43fbd63023 100644
--- a/btrfs-completion
+++ b/btrfs-completion
@@ -30,7 +30,7 @@ _btrfs()
 	local cmd=${words[1]}
 
 	commands='subvolume filesystem balance device scrub check rescue restore inspect-internal property send receive quota qgroup replace help version'
-	commands_subvolume='create delete list snapshot find-new get-default set-default show sync'
+	commands_subvolume='create delete list snapshot find-new get-default set-default show sync undelete'
 	commands_filesystem='defragment sync resize show df du label usage'
 	commands_balance='start pause cancel resume status'
 	commands_device='scan add delete remove ready stats usage'
diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index e7a884af1f5d..228d0b9e9b34 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -1219,6 +1219,74 @@ out:
 	return !!ret;
 }
 
+static const char * const cmd_subvol_undelete_usage[] = {
+	"btrfs subvolume undelete [-n <name>] <subvol_id> <dest>",
+	"Undelete the subvolume of the given <subvol_id> to <dest>.",
+	"",
+	"-n <name>  recover the subvolume with <name>.",
+	NULL
+};
+
+static int cmd_subvol_undelete(int argc, char **argv)
+{
+	struct btrfs_ioctl_subvol_undelete_args args;
+	bool need_assign_name = true;
+	DIR *dirstream = NULL;
+	char *dest;
+	int fd = -1;
+	int ret;
+
+	memset(&args, 0, sizeof(args));
+
+	while (1) {
+		int c = getopt(argc, argv, "n:");
+
+		if (c < 0)
+			break;
+
+		switch (c) {
+		case 'n':
+			strncpy_null(args.name, optarg);
+			need_assign_name = false;
+			break;
+		default:
+			usage(cmd_subvol_undelete_usage);
+		}
+	}
+	if (!need_assign_name) {
+		if (!test_issubvolname(args.name)) {
+			error("invalid subvolume name: %s", args.name);
+			return -EINVAL;
+		} else if (strlen(args.name) > BTRFS_VOL_NAME_MAX) {
+			error("subvolume name too long: %s", args.name);
+			return -EINVAL;
+		}
+	}
+
+	if (check_argc_exact(argc - optind, 2))
+		usage(cmd_subvol_undelete_usage);
+
+	args.subvol_id = arg_strtou64(argv[optind]);
+	if (need_assign_name)
+		snprintf(args.name, BTRFS_VOL_NAME_MAX, "sub_%llu",
+				args.subvol_id);
+
+	dest = argv[optind + 1];
+	fd = btrfs_open_dir(dest, &dirstream, 1);
+	if (fd < 0) {
+		error("can't access '%s'", dest);
+		return -1;
+	}
+
+	ret = ioctl(fd, BTRFS_IOC_SUBVOL_UNDELETE, &args);
+	if (ret)
+		perror("BTRFS_IOC_SUBVOL_UNDELETE");
+
+	close_file_or_dir(fd, dirstream);
+
+	return ret;
+}
+
 static const char subvolume_cmd_group_info[] =
 "manage subvolumes: create, delete, list, etc";
 
@@ -1237,6 +1305,8 @@ const struct cmd_group subvolume_cmd_group = {
 			NULL, 0 },
 		{ "show", cmd_subvol_show, cmd_subvol_show_usage, NULL, 0 },
 		{ "sync", cmd_subvol_sync, cmd_subvol_sync_usage, NULL, 0 },
+		{ "undelete", cmd_subvol_undelete, cmd_subvol_undelete_usage,
+			NULL, 0 },
 		NULL_CMD_STRUCT
 	}
 };
-- 
2.18.0

Re: [RFC PATCH v2 2/4] btrfs: don't BUG_ON() in btrfs_link_subvol()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1274 bytes --]



On 2018/9/11 下午7:29, Lu Fengqi wrote:
> Both of btrfs_update_inode() and btrfs_add_root_ref() may fail because
> of ENOMEM.

Not only ENOMEM, but also EEXIST/EIO from btrfs_insert_empty_item().

> So there's no reason to panic here, we can replace BUG_ON()
> with btrfs_abort_transaction() here.

Valid cleanup.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> 
> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
> ---
>  fs/btrfs/ioctl.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 1b03d07acde2..f6173d4e7ced 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -572,11 +572,17 @@ static int btrfs_link_subvol(struct btrfs_trans_handle *trans,
>  
>  	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
>  	ret = btrfs_update_inode(trans, root, dir);
> -	BUG_ON(ret);
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
> +		return ret;
> +	}
>  
>  	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
>  				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
> -	BUG_ON(ret);
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
> +		return ret;
> +	}
>  
>  	return ret;
>  }
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH v2 1/4] btrfs: factor out btrfs_link_subvol from create_subvol

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3322 bytes --]



On 2018/9/11 下午7:29, Lu Fengqi wrote:
> The function btrfs_link_subvol is responsible to link the subvolume to
> the specified directory, which is the opposite of what
> btrfs_unlink_subvol does.
> 
> No functional change.
> 
> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>

The patch itself is OK.

Just small nitpicks inlined below.

> ---
>  fs/btrfs/ioctl.c | 64 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 4905d13dee0a..1b03d07acde2 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -542,6 +542,45 @@ int btrfs_is_empty_uuid(u8 *uuid)
>  	return 1;
>  }
>  
> +static int btrfs_link_subvol(struct btrfs_trans_handle *trans,
> +			     struct inode *dir, u64 objectid, const char *name,
> +			     int namelen)
> +{
> +	struct btrfs_root *root = BTRFS_I(dir)->root;
> +	struct btrfs_key key;
> +	u64 index = 0;
> +	int ret;
> +
> +	/*
> +	 * insert the directory item
> +	 */
> +	ret = btrfs_set_inode_index(BTRFS_I(dir), &index);
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
> +		return ret;
> +	}
> +
> +	key.objectid = objectid;
> +	key.type = BTRFS_ROOT_ITEM_KEY;
> +	key.offset = -1;
> +	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
> +				    BTRFS_FT_DIR, index);
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
> +		return ret;
> +	}
> +
> +	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
> +	ret = btrfs_update_inode(trans, root, dir);
> +	BUG_ON(ret);

What about clean up this BUG_ON()?

> +
> +	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
> +				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
> +	BUG_ON(ret);

And this one?

Thanks,
Qu

> +
> +	return ret;
> +}
> +
>  static noinline int create_subvol(struct inode *dir,
>  				  struct dentry *dentry,
>  				  const char *name, int namelen,
> @@ -563,7 +602,6 @@ static noinline int create_subvol(struct inode *dir,
>  	int err;
>  	u64 objectid;
>  	u64 new_dirid = BTRFS_FIRST_FREE_OBJECTID;
> -	u64 index = 0;
>  	uuid_le new_uuid;
>  
>  	root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
> @@ -677,29 +715,9 @@ static noinline int create_subvol(struct inode *dir,
>  	new_root->highest_objectid = new_dirid;
>  	mutex_unlock(&new_root->objectid_mutex);
>  
> -	/*
> -	 * insert the directory item
> -	 */
> -	ret = btrfs_set_inode_index(BTRFS_I(dir), &index);
> -	if (ret) {
> -		btrfs_abort_transaction(trans, ret);
> -		goto fail;
> -	}
> -
> -	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
> -				    BTRFS_FT_DIR, index);
> -	if (ret) {
> -		btrfs_abort_transaction(trans, ret);
> +	ret = btrfs_link_subvol(trans, dir, objectid, name, namelen);
> +	if (ret)
>  		goto fail;
> -	}
> -
> -	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
> -	ret = btrfs_update_inode(trans, root, dir);
> -	BUG_ON(ret);
> -
> -	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
> -				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
> -	BUG_ON(ret);
>  
>  	ret = btrfs_uuid_tree_add(trans, root_item->uuid,
>  				  BTRFS_UUID_KEY_SUBVOL, objectid);
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH v2 1/4] btrfs: factor out btrfs_link_subvol from create_subvol

[Lu Fengqi]

On Tue, Sep 11, 2018 at 07:57:03PM +0800, Qu Wenruo wrote:
>
>
>On 2018/9/11 下午7:29, Lu Fengqi wrote:
>> The function btrfs_link_subvol is responsible to link the subvolume to
>> the specified directory, which is the opposite of what
>> btrfs_unlink_subvol does.
>> 
>> No functional change.
>> 
>> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
>
>The patch itself is OK.
>
>Just small nitpicks inlined below.
>
>> ---
>>  fs/btrfs/ioctl.c | 64 +++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 41 insertions(+), 23 deletions(-)
>> 
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 4905d13dee0a..1b03d07acde2 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -542,6 +542,45 @@ int btrfs_is_empty_uuid(u8 *uuid)
>>  	return 1;
>>  }
>>  
>> +static int btrfs_link_subvol(struct btrfs_trans_handle *trans,
>> +			     struct inode *dir, u64 objectid, const char *name,
>> +			     int namelen)
>> +{
>> +	struct btrfs_root *root = BTRFS_I(dir)->root;
>> +	struct btrfs_key key;
>> +	u64 index = 0;
>> +	int ret;
>> +
>> +	/*
>> +	 * insert the directory item
>> +	 */
>> +	ret = btrfs_set_inode_index(BTRFS_I(dir), &index);
>> +	if (ret) {
>> +		btrfs_abort_transaction(trans, ret);
>> +		return ret;
>> +	}
>> +
>> +	key.objectid = objectid;
>> +	key.type = BTRFS_ROOT_ITEM_KEY;
>> +	key.offset = -1;
>> +	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
>> +				    BTRFS_FT_DIR, index);
>> +	if (ret) {
>> +		btrfs_abort_transaction(trans, ret);
>> +		return ret;
>> +	}
>> +
>> +	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
>> +	ret = btrfs_update_inode(trans, root, dir);
>> +	BUG_ON(ret);
>
>What about clean up this BUG_ON()?
>
>> +
>> +	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
>> +				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
>> +	BUG_ON(ret);
>
>And this one?

Sorry, this makes you confused. This is exactly the cleanup done by Patch 2, because
I want to just move the code in Patch 1.

Thanks,
Lu

>
>Thanks,
>Qu
>
>> +
>> +	return ret;
>> +}
>> +
>>  static noinline int create_subvol(struct inode *dir,
>>  				  struct dentry *dentry,
>>  				  const char *name, int namelen,
>> @@ -563,7 +602,6 @@ static noinline int create_subvol(struct inode *dir,
>>  	int err;
>>  	u64 objectid;
>>  	u64 new_dirid = BTRFS_FIRST_FREE_OBJECTID;
>> -	u64 index = 0;
>>  	uuid_le new_uuid;
>>  
>>  	root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
>> @@ -677,29 +715,9 @@ static noinline int create_subvol(struct inode *dir,
>>  	new_root->highest_objectid = new_dirid;
>>  	mutex_unlock(&new_root->objectid_mutex);
>>  
>> -	/*
>> -	 * insert the directory item
>> -	 */
>> -	ret = btrfs_set_inode_index(BTRFS_I(dir), &index);
>> -	if (ret) {
>> -		btrfs_abort_transaction(trans, ret);
>> -		goto fail;
>> -	}
>> -
>> -	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
>> -				    BTRFS_FT_DIR, index);
>> -	if (ret) {
>> -		btrfs_abort_transaction(trans, ret);
>> +	ret = btrfs_link_subvol(trans, dir, objectid, name, namelen);
>> +	if (ret)
>>  		goto fail;
>> -	}
>> -
>> -	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
>> -	ret = btrfs_update_inode(trans, root, dir);
>> -	BUG_ON(ret);
>> -
>> -	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
>> -				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
>> -	BUG_ON(ret);
>>  
>>  	ret = btrfs_uuid_tree_add(trans, root_item->uuid,
>>  				  BTRFS_UUID_KEY_SUBVOL, objectid);
>> 
>