Re: [PATCH v2 2/4] Btrfs: fix protection between send and root deletion
Hi David, On 01/22/2014 02:16 AM, David Sterba wrote: On Thu, Jan 16, 2014 at 10:32:38AM +0800, Miao Xie wrote: Your fix makes sure that the deleted root will not get cleaned and stays during the send. Only after it finishes it will be cleaned. Now, what if send fails or is interrupted? There's no way to redo it. Yes the user can be blamed for the mistake, or the tools will prevent him to do it. I don't think so. The users should be responsible for their behavior if they destroy the subvolume. Right now it's not possible to determine if a subvolume is involved in a send (other than the user knows by himself that he started send). Send or subvolume cleaning can be performed on the background. Although the user is responsible for his actions, the consequence here is not obvious, silent and irreversible. I see the latter as more user-friendly. Doing a 'send and forget' where I don't care if the data will be sent properly does not fit the primary purpose of send/receive with backups. My idea to fix that: - add an internal root_item flag to denote a dead root - set this flag in btrfs_add_dead_root() - check the flag in send similar to the btrfs_root_readonly checks, for all involved roots - in 'destroy subvolume, check if the send_in_progress is set and refuse to delete It is similar to our approach. But I think our idea is better because - we needn't add a new flag Adding the flag is cheap. - The subvolumes are special directory, the most operations of them should be similar to the common directory. Since we can remove a directory while someone is accessing it, it is better that we can destroy a subvolume while we are using it as a send parent. Yes they're similar, but subvolumes have additional features that need to be handled appropriately. One cannot send a directory. So we disagree, I see a reason for the deletion protection and will do the patch myself. Let's see if we can get more user feedback then. I'm NAKing this patch in current state, if it helps anything. Both ways are ok for me actually, don't be annoyed anyway, You and Miao are really doing a good job to Btrfs, just go ahead, i am ok with dropping this patch.^_^ Thanks, Wang david -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] Btrfs: fix protection between send and root deletion
On Wed, Jan 22, 2014 at 04:44:14PM +0800, Wang Shilong wrote: So we disagree, I see a reason for the deletion protection and will do the patch myself. Let's see if we can get more user feedback then. I'm NAKing this patch in current state, if it helps anything. Both ways are ok for me actually, don't be annoyed anyway, Nah, this is a message to maintainers that the patch discussion has reached some conclusion and should help deciding if the patch should be merged or not. We've seen in the past that after a moderate discussion against patch inclusion, the patch ended up merged as if nothing happend. _This_ can be annoying. You and Miao are really doing a good job to Btrfs, just go ahead, i am ok with dropping this patch.^_^ Ok, thanks. -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] Btrfs: fix protection between send and root deletion
On Thu, Jan 16, 2014 at 10:32:38AM +0800, Miao Xie wrote: Your fix makes sure that the deleted root will not get cleaned and stays during the send. Only after it finishes it will be cleaned. Now, what if send fails or is interrupted? There's no way to redo it. Yes the user can be blamed for the mistake, or the tools will prevent him to do it. I don't think so. The users should be responsible for their behavior if they destroy the subvolume. Right now it's not possible to determine if a subvolume is involved in a send (other than the user knows by himself that he started send). Send or subvolume cleaning can be performed on the background. Although the user is responsible for his actions, the consequence here is not obvious, silent and irreversible. I see the latter as more user-friendly. Doing a 'send and forget' where I don't care if the data will be sent properly does not fit the primary purpose of send/receive with backups. My idea to fix that: - add an internal root_item flag to denote a dead root - set this flag in btrfs_add_dead_root() - check the flag in send similar to the btrfs_root_readonly checks, for all involved roots - in 'destroy subvolume, check if the send_in_progress is set and refuse to delete It is similar to our approach. But I think our idea is better because - we needn't add a new flag Adding the flag is cheap. - The subvolumes are special directory, the most operations of them should be similar to the common directory. Since we can remove a directory while someone is accessing it, it is better that we can destroy a subvolume while we are using it as a send parent. Yes they're similar, but subvolumes have additional features that need to be handled appropriately. One cannot send a directory. So we disagree, I see a reason for the deletion protection and will do the patch myself. Let's see if we can get more user feedback then. I'm NAKing this patch in current state, if it helps anything. david -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] Btrfs: fix protection between send and root deletion
On Tue, Jan 14, 2014 at 10:22:56AM +0800, Miao Xie wrote: On mon, 13 Jan 2014 19:27:45 +0100, David Sterba wrote: The root what is about to be sent out may be deleted after we get it, if there is no in-memory i-node in it, it will be inserted into the dead_roots list immediately, then the cleaner thread will drop it. But the sender doesn't know, the sender will access a broken tree. Thanks, I see the race now, but I think that we should prevent the subvolume deletion at a higher level, similar to the RO-RW protection. Your fix makes sure that the deleted root will not get cleaned and stays during the send. Only after it finishes it will be cleaned. Now, what if send fails or is interrupted? There's no way to redo it. Yes the user can be blamed for the mistake, or the tools will prevent him to do it. I see the latter as more user-friendly. Doing a 'send and forget' where I don't care if the data will be sent properly does not fit the primary purpose of send/receive with backups. My idea to fix that: - add an internal root_item flag to denote a dead root - set this flag in btrfs_add_dead_root() - check the flag in send similar to the btrfs_root_readonly checks, for all involved roots - in 'destroy subvolume, check if the send_in_progress is set and refuse to delete The root lookup in send via btrfs_read_fs_root_no_name will check if the root is dead or not. If it is, ENOENT, aborted send. If it's alive, it's protected by send_in_progress, send can continue. This has become more complex than the original patch, I'll try to think about it again later if it still makes sense. Comments welcome of course. david -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] Btrfs: fix protection between send and root deletion
On wed, 15 Jan 2014 17:40:38 +0100, David Sterba wrote: On Tue, Jan 14, 2014 at 10:22:56AM +0800, Miao Xie wrote: On mon, 13 Jan 2014 19:27:45 +0100, David Sterba wrote: The root what is about to be sent out may be deleted after we get it, if there is no in-memory i-node in it, it will be inserted into the dead_roots list immediately, then the cleaner thread will drop it. But the sender doesn't know, the sender will access a broken tree. Thanks, I see the race now, but I think that we should prevent the subvolume deletion at a higher level, similar to the RO-RW protection. Your fix makes sure that the deleted root will not get cleaned and stays during the send. Only after it finishes it will be cleaned. Now, what if send fails or is interrupted? There's no way to redo it. Yes the user can be blamed for the mistake, or the tools will prevent him to do it. I don't think so. The users should be responsible for their behavior if they destroy the subvolume. I see the latter as more user-friendly. Doing a 'send and forget' where I don't care if the data will be sent properly does not fit the primary purpose of send/receive with backups. My idea to fix that: - add an internal root_item flag to denote a dead root - set this flag in btrfs_add_dead_root() - check the flag in send similar to the btrfs_root_readonly checks, for all involved roots - in 'destroy subvolume, check if the send_in_progress is set and refuse to delete It is similar to our approach. But I think our idea is better because - we needn't add a new flag - The subvolumes are special directory, the most operations of them should be similar to the common directory. Since we can remove a directory while someone is accessing it, it is better that we can destroy a subvolume while we are using it as a send parent. Thanks Miao The root lookup in send via btrfs_read_fs_root_no_name will check if the root is dead or not. If it is, ENOENT, aborted send. If it's alive, it's protected by send_in_progress, send can continue. This has become more complex than the original patch, I'll try to think about it again later if it still makes sense. Comments welcome of course. david -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] Btrfs: fix protection between send and root deletion
On Tue, Jan 07, 2014 at 05:25:19PM +0800, Wang Shilong wrote:
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1971,6 +1971,19 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_root
*root)
}
root = list_first_entry(fs_info-dead_roots,
struct btrfs_root, root_list);
You're pulling the root from dead_roots here ...
+ /*
+ * Make sure root is not involved in send,
... and there's no way to send such a thing.
+ * if we fail with first root, we return
+ * directly rather than continue.
+ */
+ spin_lock(root-root_item_lock);
+ if (root-send_in_progress) {
+ spin_unlock(fs_info-trans_lock);
+ spin_unlock(root-root_item_lock);
+ return 0;
+ }
+ spin_unlock(root-root_item_lock);
+
list_del_init(root-root_list);
spin_unlock(fs_info-trans_lock);
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] Btrfs: fix protection between send and root deletion
On mon, 13 Jan 2014 19:27:45 +0100, David Sterba wrote:
On Tue, Jan 07, 2014 at 05:25:19PM +0800, Wang Shilong wrote:
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1971,6 +1971,19 @@ int btrfs_clean_one_deleted_snapshot(struct
btrfs_root *root)
}
root = list_first_entry(fs_info-dead_roots,
struct btrfs_root, root_list);
You're pulling the root from dead_roots here ...
+/*
+ * Make sure root is not involved in send,
... and there's no way to send such a thing.
The root what is about to be sent out may be deleted after we get it, if
there is no in-memory i-node in it, it will be inserted into the dead_roots list
immediately, then the cleaner thread will drop it. But the sender doesn't know,
the sender will access a broken tree.
Thanks
Miao
+ * if we fail with first root, we return
+ * directly rather than continue.
+ */
+spin_lock(root-root_item_lock);
+if (root-send_in_progress) {
+spin_unlock(fs_info-trans_lock);
+spin_unlock(root-root_item_lock);
+return 0;
+}
+spin_unlock(root-root_item_lock);
+
list_del_init(root-root_list);
spin_unlock(fs_info-trans_lock);
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/4] Btrfs: fix protection between send and root deletion
We should gurantee that parent and clone roots can not be destroyed
during send, for this we have two ideas.
1.by holding @subvol_sem, this might be a nightmare, because it will
block all subvolumes deletion for a long time.
2.Miao pointed out we can reuse @send_in_progress, that mean we will
skip snapshot deletion if root sending is in progress.
Here we adopt the second approach since it won't block other subvolumes
deletion for a long time.
Besides in btrfs_clean_one_deleted_snapshot(), we only check first root
, if this root is involved in send, we return directly rather than
continue to check.There are several reasons about it:
1.this case happen seldomly.
2.after sending,cleaner thread can continue to drop that root.
3.make code simple
Cc: David Sterba dste...@suse.cz
Signed-off-by: Wang Shilong wangsl.f...@cn.fujitsu.com
Reviewed-by: Miao Xie mi...@cn.fujitsu.com
---
Changelog v1-v2:
use right root to check(pointed by David)
---
fs/btrfs/send.c| 16
fs/btrfs/transaction.c | 13 +
2 files changed, 29 insertions(+)
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 5b69785..4e2461b 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -4753,6 +4753,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user
*arg_)
u64 *clone_sources_tmp = NULL;
int clone_sources_to_rollback = 0;
int sort_clone_roots = 0;
+ int index;
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
@@ -4893,8 +4894,12 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user
*arg_)
key.objectid = clone_sources_tmp[i];
key.type = BTRFS_ROOT_ITEM_KEY;
key.offset = (u64)-1;
+
+ index = srcu_read_lock(fs_info-subvol_srcu);
+
clone_root = btrfs_read_fs_root_no_name(fs_info, key);
if (IS_ERR(clone_root)) {
+ srcu_read_unlock(fs_info-subvol_srcu, index);
ret = PTR_ERR(clone_root);
goto out;
}
@@ -4903,10 +4908,13 @@ long btrfs_ioctl_send(struct file *mnt_file, void
__user *arg_)
clone_root-send_in_progress++;
if (!btrfs_root_readonly(clone_root)) {
spin_unlock(clone_root-root_item_lock);
+ srcu_read_unlock(fs_info-subvol_srcu, index);
ret = -EPERM;
goto out;
}
spin_unlock(clone_root-root_item_lock);
+ srcu_read_unlock(fs_info-subvol_srcu, index);
+
sctx-clone_roots[i].root = clone_root;
}
vfree(clone_sources_tmp);
@@ -4917,19 +4925,27 @@ long btrfs_ioctl_send(struct file *mnt_file, void
__user *arg_)
key.objectid = arg-parent_root;
key.type = BTRFS_ROOT_ITEM_KEY;
key.offset = (u64)-1;
+
+ index = srcu_read_lock(fs_info-subvol_srcu);
+
sctx-parent_root = btrfs_read_fs_root_no_name(fs_info, key);
if (IS_ERR(sctx-parent_root)) {
+ srcu_read_unlock(fs_info-subvol_srcu, index);
ret = PTR_ERR(sctx-parent_root);
goto out;
}
+
spin_lock(sctx-parent_root-root_item_lock);
sctx-parent_root-send_in_progress++;
if (!btrfs_root_readonly(sctx-parent_root)) {
spin_unlock(sctx-parent_root-root_item_lock);
+ srcu_read_unlock(fs_info-subvol_srcu, index);
ret = -EPERM;
goto out;
}
spin_unlock(sctx-parent_root-root_item_lock);
+
+ srcu_read_unlock(fs_info-subvol_srcu, index);
}
/*
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 30d11bb..1ef8e28 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1971,6 +1971,19 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_root
*root)
}
root = list_first_entry(fs_info-dead_roots,
struct btrfs_root, root_list);
+ /*
+* Make sure root is not involved in send,
+* if we fail with first root, we return
+* directly rather than continue.
+*/
+ spin_lock(root-root_item_lock);
+ if (root-send_in_progress) {
+ spin_unlock(fs_info-trans_lock);
+ spin_unlock(root-root_item_lock);
+ return 0;
+ }
+ spin_unlock(root-root_item_lock);
+
list_del_init(root-root_list);
spin_unlock(fs_info-trans_lock);
--
1.8.3.1
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a
