[RFC PATCH 0/4] undelete subvolume online version

[RFC PATCH 0/4] undelete subvolume online version

[Lu Fengqi]

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

And btrfs subvolume undelete subcommand was added to btrfs-progs.

So user can use the following command to recover all the subolume that
is left on the device. The recovered subvolume will be link to <dest> dir
named to <name_prefix><subvol_id>.

# btrfs subvolume undelete [-p <name_prefix>] <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 the btrfs_ioctl_undelete

 fs/btrfs/ioctl.c           | 270 +++++++++++++++++++++++++++++++++----
 include/uapi/linux/btrfs.h |   9 ++
 2 files changed, 255 insertions(+), 24 deletions(-)

-- 
2.18.0

[RFC PATCH 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 | 65 ++++++++++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d3a5d2a41e5f..d37c26f69112 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, root, 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,30 +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, root,
-				    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 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 d37c26f69112..e0b5a8fb15e7 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 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 | 116 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e0b5a8fb15e7..7a11c4f8e450 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1864,6 +1864,122 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
 	return ret;
 }
 
+static int btrfs_undelete_subvolume(const struct path *parent,
+				    struct btrfs_root *root,
+				    const char *name, int namelen)
+{
+	struct inode *dir = d_inode(parent->dentry);
+	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;
+
+	/* root_refs of destination parent root must not be 0 */
+	if (btrfs_root_refs(&BTRFS_I(dir)->root->root_item) == 0)
+		return -ENOENT;
+
+	ret = down_write_killable_nested(&dir->i_rwsem, I_MUTEX_PARENT);
+	if (ret == -EINTR)
+		return ret;
+
+	dentry = lookup_one_len(name, parent->dentry, 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(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 4/4] btrfs: undelete: Add the btrfs_ioctl_undelete

[Lu Fengqi]

The function will traverse the root from the fs_info->dead_roots and try
to call btrfs_undelete_subvolume() to recover them.

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           | 83 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/btrfs.h |  9 +++++
 2 files changed, 92 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7a11c4f8e450..83b9839799d0 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1980,6 +1980,87 @@ static int btrfs_undelete_subvolume(const struct path *parent,
 	return ret;
 }
 
+static int btrfs_ioctl_undelete(struct file *file, void __user *argp)
+{
+	struct btrfs_ioctl_undelete_args __user *uarg;
+	struct btrfs_ioctl_undelete_args *args;
+	struct inode *inode = file_inode(file);
+	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
+	struct btrfs_root *root, *tmp;
+	char *name;
+	u64 count = 0;
+	u64 objectid;
+	int err = 0, ret;
+
+	/* copy search header and buffer size */
+	uarg = (struct btrfs_ioctl_undelete_args __user *)argp;
+	args = memdup_user(uarg, sizeof(*args));
+	if (IS_ERR(args))
+		return PTR_ERR(args);
+	args->name[BTRFS_PATH_NAME_MAX] = '\0';
+
+	name = kzalloc(BTRFS_PATH_NAME_MAX + 1, GFP_KERNEL);
+	if (IS_ERR(name)) {
+		err = PTR_ERR(name);
+		goto free_args;
+	}
+
+	if (!capable(CAP_SYS_ADMIN)) {
+		err = -EPERM;
+		goto free;
+	}
+
+	err = mnt_want_write_file(file);
+	if (err)
+		goto free;
+
+	/* 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);
+
+	list_for_each_entry_safe(root, tmp, &fs_info->dead_roots, root_list) {
+		objectid = root->root_key.objectid;
+		snprintf(name, BTRFS_PATH_NAME_MAX, "%s%llu", args->name,
+				objectid);
+		ret = btrfs_undelete_subvolume(&file->f_path, root, name,
+					       strlen(name));
+		if (ret)
+			continue;
+
+		/*
+		 * Feel free to remove this root from dead_root list since we
+		 * have recover it successfully.
+		 */
+		spin_lock(&fs_info->trans_lock);
+		list_del_init(&root->root_list);
+		spin_unlock(&fs_info->trans_lock);
+
+		if ((count + 1) * sizeof(objectid) > args->buf_size)
+			continue;
+
+		/* copy the subvolume id to user space */
+		ret = copy_to_user(&uarg->buf[count], &objectid,
+				   sizeof(objectid));
+		if (ret)
+			err = -EFAULT;
+		count++;
+	}
+
+	mutex_unlock(&fs_info->cleaner_mutex);
+	mnt_drop_write_file(file);
+
+	/* copy the count to user space */
+	if (copy_to_user(&uarg->count, &count, sizeof(count)))
+		err = -EFAULT;
+free:
+	kfree(name);
+free_args:
+	kfree(args);
+	return err;
+}
+
 static noinline int btrfs_ioctl_subvol_getflags(struct file *file,
 						void __user *arg)
 {
@@ -6089,6 +6170,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..25d030687b27 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -816,6 +816,13 @@ struct btrfs_ioctl_get_subvol_rootref_args {
 		__u8 align[7];
 };
 
+struct btrfs_ioctl_undelete_args {
+	char name[BTRFS_PATH_NAME_MAX + 1];	/* in - subvolume name prefix */
+	__u64 buf_size;				/* in - size of buffer */
+	__u64 count;		/* out - store number of recoverd subvolumes */
+	__u64 buf[0];		/* out - store ids of recoverd subolumes */
+};
+
 /* Error codes as returned by the kernel */
 enum btrfs_err_code {
 	BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET = 1,
@@ -940,5 +947,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_undelete_args)
 
 #endif /* _UAPI_LINUX_BTRFS_H */
-- 
2.18.0

Re: [RFC PATCH 0/4] undelete subvolume online version

[David Sterba]

On Sun, Aug 05, 2018 at 06:39:57PM +0800, Lu Fengqi wrote:
> This patchset will add the BTRFS_IOC_SUBVOL_UNDELETE ioctl for online
> btrfs subvolume undelete.
> 
> And btrfs subvolume undelete subcommand was added to btrfs-progs.
> 
> So user can use the following command to recover all the subolume that
> is left on the device. The recovered subvolume will be link to <dest> dir
> named to <name_prefix><subvol_id>.

Hm, I don't agree with the proposed interface - to recover all deleted
subvolumes. IMO this should recover just one subvolume of a given id a
to given directory.

The ioctl structure has to be reworked, I've skimmed the code and saw
some suspicious things but will have a look after the interface is
settled.

Re: [RFC PATCH 0/4] undelete subvolume online version

[Lu Fengqi]

On Tue, Aug 07, 2018 at 06:39:50PM +0200, David Sterba wrote:
>On Sun, Aug 05, 2018 at 06:39:57PM +0800, Lu Fengqi wrote:
>> This patchset will add the BTRFS_IOC_SUBVOL_UNDELETE ioctl for online
>> btrfs subvolume undelete.
>> 
>> And btrfs subvolume undelete subcommand was added to btrfs-progs.
>> 
>> So user can use the following command to recover all the subolume that
>> is left on the device. The recovered subvolume will be link to <dest> dir
>> named to <name_prefix><subvol_id>.
>
>Hm, I don't agree with the proposed interface - to recover all deleted
>subvolumes. IMO this should recover just one subvolume of a given id a
>to given directory.

Thank you for taking the time to respond. I may have thought too much about
the interface before. In my imagination, the cleaner kthread is like a
monster that devours user data at any time, so the user must perform an
online undelete operation as soon as possible, so there is no time to
determine the subvol_id that should be passed. However, I have to admit
that I don't know much about the user's actual usage scenarios, I will
accept the interface you provided. Of course, I really like this because it
greatly simplifies the ioctl structure.

>
>The ioctl structure has to be reworked, I've skimmed the code and saw
>some suspicious things but will have a look after the interface is
>settled.

When I rework the ioctl structure, I will carefully recheck the
incorrect place in the code.

-- 
Thanks,
Lu

Re: [RFC PATCH 0/4] undelete subvolume online version

[Qu Wenruo]

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



On 2018年08月08日 00:39, David Sterba wrote:
> On Sun, Aug 05, 2018 at 06:39:57PM +0800, Lu Fengqi wrote:
>> This patchset will add the BTRFS_IOC_SUBVOL_UNDELETE ioctl for online
>> btrfs subvolume undelete.
>>
>> And btrfs subvolume undelete subcommand was added to btrfs-progs.
>>
>> So user can use the following command to recover all the subolume that
>> is left on the device. The recovered subvolume will be link to <dest> dir
>> named to <name_prefix><subvol_id>.
> 
> Hm, I don't agree with the proposed interface - to recover all deleted
> subvolumes. IMO this should recover just one subvolume of a given id a
> to given directory.
> 
> The ioctl structure has to be reworked, I've skimmed the code and saw
> some suspicious things but will have a look after the interface is
> settled.

My concern is, is such purpose really needed?

Yes, it's possible user made some mistake and want to get back the data.
But putting an ioctl for 'undelete', then user may consider btrfs is so
powerful that can undelete everything.
In short, this undelete feature gives user too high expectation.

And don't expect user really to read man pages. There are already tons
of reports where user execute btrfs check --repair without realizing
--repair is pretty dangerous (and thanks to the work done to repair, it
normally doesn't cause catastrophic result, but sometimes it indeed
causes extra damage)

And when user tried and failed due to deleted tree blocks, they will get
even more frustrated or even betrayed.

I prefer to put such undelete as an off-line rescue tool, instead of
making it online with an ioctl interface.

Thanks,
Qu

> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


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

Re: [RFC PATCH 0/4] undelete subvolume online version

[Lu Fengqi]

On Wed, Aug 08, 2018 at 02:11:24PM +0800, Qu Wenruo wrote:
>
>
>On 2018年08月08日 00:39, David Sterba wrote:
>> On Sun, Aug 05, 2018 at 06:39:57PM +0800, Lu Fengqi wrote:
>>> This patchset will add the BTRFS_IOC_SUBVOL_UNDELETE ioctl for online
>>> btrfs subvolume undelete.
>>>
>>> And btrfs subvolume undelete subcommand was added to btrfs-progs.
>>>
>>> So user can use the following command to recover all the subolume that
>>> is left on the device. The recovered subvolume will be link to <dest> dir
>>> named to <name_prefix><subvol_id>.
>> 
>> Hm, I don't agree with the proposed interface - to recover all deleted
>> subvolumes. IMO this should recover just one subvolume of a given id a
>> to given directory.
>> 
>> The ioctl structure has to be reworked, I've skimmed the code and saw
>> some suspicious things but will have a look after the interface is
>> settled.
>
>My concern is, is such purpose really needed?
>
>Yes, it's possible user made some mistake and want to get back the data.
>But putting an ioctl for 'undelete', then user may consider btrfs is so
>powerful that can undelete everything.
>In short, this undelete feature gives user too high expectation.
>
>And don't expect user really to read man pages. There are already tons

There is no more way about the too high expectation of users. If we provide
a feature with a sufficiently detailed man page, but users do not read the
man page when using this feature, I can only think that they are not
responsible for their own data. So, this seems to be a problem they need to
consider.

>of reports where user execute btrfs check --repair without realizing
>--repair is pretty dangerous (and thanks to the work done to repair, it
>normally doesn't cause catastrophic result, but sometimes it indeed
>causes extra damage)

The good news is that online undelete is not as dangerous as btrfs check
--repair. In fact, I think it is safe enough.

>
>And when user tried and failed due to deleted tree blocks, they will get
>even more frustrated or even betrayed.

As mentioned previous, maybe we should do what we think is right, such as
give the user more abilities to protect/recover their data, not to take
care of any sensitive users?

>
>I prefer to put such undelete as an off-line rescue tool, instead of
>making it online with an ioctl interface.

I also think that the offline undelete is more useful. After all, umount
immediately to prevent further data loss is always the most effective after
a mistake. However, since we can give the ability of online undelete to a
user which couldn't umount the filesystem easily, and don't have any side
effect on existing features. IMHO, there is no reason to reject this.

-- 
Thanks,
Lu

Re: [RFC PATCH 0/4] undelete subvolume online version

[Qu Wenruo]

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



On 2018年08月08日 14:53, Lu Fengqi wrote:
> On Wed, Aug 08, 2018 at 02:11:24PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2018年08月08日 00:39, David Sterba wrote:
>>> On Sun, Aug 05, 2018 at 06:39:57PM +0800, Lu Fengqi wrote:
>>>> This patchset will add the BTRFS_IOC_SUBVOL_UNDELETE ioctl for online
>>>> btrfs subvolume undelete.
>>>>
>>>> And btrfs subvolume undelete subcommand was added to btrfs-progs.
>>>>
>>>> So user can use the following command to recover all the subolume that
>>>> is left on the device. The recovered subvolume will be link to <dest> dir
>>>> named to <name_prefix><subvol_id>.
>>>
>>> Hm, I don't agree with the proposed interface - to recover all deleted
>>> subvolumes. IMO this should recover just one subvolume of a given id a
>>> to given directory.
>>>
>>> The ioctl structure has to be reworked, I've skimmed the code and saw
>>> some suspicious things but will have a look after the interface is
>>> settled.
>>
>> My concern is, is such purpose really needed?
>>
>> Yes, it's possible user made some mistake and want to get back the data.
>> But putting an ioctl for 'undelete', then user may consider btrfs is so
>> powerful that can undelete everything.
>> In short, this undelete feature gives user too high expectation.
>>
>> And don't expect user really to read man pages. There are already tons
> 
> There is no more way about the too high expectation of users. If we provide
> a feature with a sufficiently detailed man page, but users do not read the
> man page when using this feature, I can only think that they are not
> responsible for their own data. So, this seems to be a problem they need to
> consider.

IMHO this ioctl is design only for a pretty niche use case.
As long as it's a designed ioctl, it's some kind of a "main" feature.

Even it's users' response to read the man page, we still need to bring
the surprise to minimal.
By all mean, undelete is a minor use case and should be prevented from
the very beginning.

> 
>> of reports where user execute btrfs check --repair without realizing
>> --repair is pretty dangerous (and thanks to the work done to repair, it
>> normally doesn't cause catastrophic result, but sometimes it indeed
>> causes extra damage)
> 
> The good news is that online undelete is not as dangerous as btrfs check
> --repair. In fact, I think it is safe enough.

Yes, for the safety part I completely agree.

> 
>>
>> And when user tried and failed due to deleted tree blocks, they will get
>> even more frustrated or even betrayed.
> 
> As mentioned previous, maybe we should do what we think is right, such as
> give the user more abilities to protect/recover their data, not to take
> care of any sensitive users?

From my personal experience, adding a new ioctl means a lot of new
maintenance burden, especially when the need is small (and sometimes
raises user expectation).

Personally speaking, we already have a lot of so-called "optimization"
which makes development pretty tricky.
We should pay more attention when introduce a new feature, evaluating
the user-case and possible complexity carefully, since removing a
feature is way more complex than adding a new one.

(Although thankfully, current undelete is pretty simple, but I'm not
sure if it will become more and more complex)

> 
>>
>> I prefer to put such undelete as an off-line rescue tool, instead of
>> making it online with an ioctl interface.
> 
> I also think that the offline undelete is more useful. After all, umount
> immediately to prevent further data loss is always the most effective after
> a mistake. However, since we can give the ability of online undelete to a
> user which couldn't umount the filesystem easily,

Well, when the deletion happens, there is no much difference to do
online or offline rescue.
Either way, sane user should already stop its business and check the
system carefully.

> and don't have any side
> effect on existing features. IMHO, there is no reason to reject this.

I just don't want to regret several years later.
Currently for undelete, we already need to take a lot of factors into
consideration, (although most of them are not affected) qgroup, balance,
pending snapshot (and I may miss a lot of more).

I just don't want later development to take undelete into consideration
for such a niche use-case.

Thanks,
Qu


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