Snapshot deletion vs send (for 3.15)
cover Snapshot deletion vs send (for 3.15)
I lost track of these patches at 3.14 development cycle, they accompany the subvolume RO protections during send. Would be good to get them to 3.15 but I understand this may be too late.
David Sterba (2):
btrfs: protect snapshots from deleting during send
btrfs: assert that send is not in progres before root deletion
fs/btrfs/ctree.h | 11 +++++++++++ fs/btrfs/ioctl.c | 31 +++++++++++++++++++++++++++++++ fs/btrfs/send.c | 14 ++++++++++++-- fs/btrfs/transaction.c | 13 ------------- 4 files changed, 54 insertions(+), 15 deletions(-)
Signature · 1 line
1.7.9
PATCH 1/2 btrfs: protect snapshots from deleting during send
The patch "Btrfs: fix protection between send and root deletion" (18f687d538449373c37c) does not actually prevent to delete the snapshot and just takes care during background cleaning, but this seems rather user unfriendly, this patch implements the idea presented in
http://www.spinics.net/lists/linux-btrfs/msg30813.html
- add an internal root_item flag to denote a dead root
- check if the send_in_progress is set and refuse to delete, otherwise set the flag and proceed
- check the flag in send similar to the btrfs_root_readonly checks, for all involved roots
The root lookup in send via btrfs_read_fs_root_no_name will check if the root is really dead or not. If it is, ENOENT, aborted send. If it's alive, it's protected by send_in_progress, send can continue.
--- fs/btrfs/ctree.h | 11 +++++++++++ fs/btrfs/ioctl.c | 30 ++++++++++++++++++++++++++++++ fs/btrfs/send.c | 14 ++++++++++++-- 3 files changed, 53 insertions(+), 2 deletions(-)
fs/btrfs/ctree.h +11 -0
@@ -754,6 +754,12 @@ struct btrfs_dir_item {#define BTRFS_ROOT_SUBVOL_RDONLY (1ULL << 0) +/* +* Internal in-memory flag that a subvolume has been marked for deletion but +* still visible as a directory +*/ +#define BTRFS_ROOT_SUBVOL_DEAD (1ULL << 48) +struct btrfs_root_item {struct btrfs_inode_item inode;__le64 generation;
@@ -2749,6 +2755,11 @@ static inline bool btrfs_root_readonly(struct btrfs_root *root)return (root->root_item.flags &cpu_to_le64 (BTRFS_ROOT_SUBVOL_RDONLY )) !=0 ; } +static inline bool btrfs_root_dead (struct btrfs_root *root) +{ +return (root->root_item.flags &cpu_to_le64 (BTRFS_ROOT_SUBVOL_DEAD )) !=0 ; +} +/* struct btrfs_root_backup */ BTRFS_SETGET_STACK_FUNCS(backup_tree_root, struct btrfs_root_backup, tree_root,64 );
fs/btrfs/ioctl.c +30 -0
@@ -2147,6 +2147,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,struct btrfs_ioctl_vol_args *vol_args;struct btrfs_trans_handle *trans;struct btrfs_block_rsv block_rsv; +u64 root_flags;u64 qgroup_reserved;int namelen;int ret;
@@ -2168,6 +2169,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,if (err) goto out; + err =mutex_lock_killable_nested (&dir->i_mutex,I_MUTEX_PARENT );if (err == -EINTR ) goto out_drop_write;
@@ -2229,6 +2231,27 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, }mutex_lock (&inode->i_mutex); + +/* +* Don't allow to delete a subvolume with send in progress. This is +* inside the i_mutex so the error handling that has to drop the bit +* again is not run concurrently. +*/ +spin_lock (&dest->root_item_lock); + root_flags =btrfs_root_flags (&root->root_item); +if (root->send_in_progress ==0 ) { +btrfs_set_root_flags (&root->root_item, + root_flags |BTRFS_ROOT_SUBVOL_DEAD ); +spin_unlock (&dest->root_item_lock); + }else { +spin_unlock (&dest->root_item_lock); +btrfs_warn (root->fs_info, +"Attempt to delete subvolume %llu during send" , + root->root_key.objectid); + err = -EPERM ; + goto out_dput; + } + err =d_invalidate (dentry);if (err) goto out_unlock;
@@ -2317,6 +2340,13 @@ out_release: out_up_write:up_write (&root->fs_info->subvol_sem); out_unlock: +if (err) { +spin_lock (&dest->root_item_lock); + root_flags =btrfs_root_flags (&root->root_item); +btrfs_set_root_flags (&root->root_item, + root_flags & ~BTRFS_ROOT_SUBVOL_DEAD ); +spin_unlock (&dest->root_item_lock); + }mutex_unlock (&inode->i_mutex);if (!err) {shrink_dcache_sb (root->fs_info->sb);
fs/btrfs/send.c +12 -2
@@ -5263,7 +5263,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)/* * The subvolume must remain read-only during send, protect against -* making it RW. +* making it RW. This also protects against deletion. */ spin_lock (&send_root->root_item_lock); send_root->send_in_progress++;
@@ -5284,6 +5284,15 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) goto out; } +/* +* Unlikely but possible, if the subvolume is marked for deletion but +* is slow to remove the directory entry, send can still be started +*/ +if (btrfs_root_dead (sctx->parent_root)) { + ret = -EPERM ; + goto out; + } + arg =memdup_user (arg_,sizeof (*arg));if (IS_ERR (arg)) { ret =PTR_ERR (arg);
@@ -5411,7 +5420,8 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)spin_lock (&sctx->parent_root->root_item_lock); sctx->parent_root->send_in_progress++; -if (!btrfs_root_readonly (sctx->parent_root)) { +if (!btrfs_root_readonly (sctx->parent_root) || +btrfs_root_dead (sctx->parent_root)) {spin_unlock (&sctx->parent_root->root_item_lock);srcu_read_unlock (&fs_info->subvol_srcu, index); ret = -EPERM ;
Signature · 1 line
1.7.9
PATCH 2/2 btrfs: assert that send is not in progres before root deletion
In reply to David Sterba--- fs/btrfs/ioctl.c | 1 + fs/btrfs/transaction.c | 13 ------------- 2 files changed, 1 insertions(+), 13 deletions(-)
fs/btrfs/ioctl.c +1 -0
@@ -2352,6 +2352,7 @@ out_unlock:shrink_dcache_sb (root->fs_info->sb);btrfs_invalidate_inodes (dest);d_delete (dentry); +ASSERT (dest->send_in_progress ==0 );/* the last ref */ if (dest->cache_inode) {
fs/btrfs/transaction.c +0 -13
@@ -1977,19 +1977,6 @@ 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);
Signature · 1 line
1.7.9
Re: [PATCH 1/2] btrfs: protect snapshots from deleting during send
In reply to David SterbaQuoted 13 lines · David Sterba
The patch "Btrfs: fix protection between send and root deletion" (
18f687d538449373c37c) does not actually prevent to delete the snapshot and just takes care during background cleaning, but this seems rather user unfriendly, this patch implements the idea presented inhttp://www.spinics.net/lists/linux-btrfs/msg30813.html
- add an internal
root_itemflag to denote a dead root
- check if thesend_in_progressis set and refuse to delete, otherwise set the flag and proceed
- check the flag in send similar to thebtrfs_root_readonlychecks, for all involved roots
Quoted 3 lines
The root lookup in send via
btrfs_read_fs_root_no_namewill check if the root is really dead or not. If it is, ENOENT, aborted send. If it's alive, it's protected bysend_in_progress, send can continue.
I'm worried about the use case where we have:
* periodic automated snapshots
* periodic automated deletion of old snapshots
* periodic send for backup
The automated deletion doesn't want to error out if send is in progress, it just wants the deletion to happen in the background.
-chris
Re: [PATCH 1/2] btrfs: protect snapshots from deleting during send
Quoted 18 lines · Chris Mason
Quoted 13 lines · David Sterba
The patch "Btrfs: fix protection between send and root deletion" (
18f687d538449373c37c) does not actually prevent to delete the snapshot and just takes care during background cleaning, but this seems rather user unfriendly, this patch implements the idea presented inhttp://www.spinics.net/lists/linux-btrfs/msg30813.html
- add an internal
root_itemflag to denote a dead root
- check if thesend_in_progressis set and refuse to delete, otherwise set the flag and proceed
- check the flag in send similar to thebtrfs_root_readonlychecks, for all involved rootsQuoted 3 lines
The root lookup in send via
btrfs_read_fs_root_no_namewill check if the root is really dead or not. If it is, ENOENT, aborted send. If it's alive, it's protected bysend_in_progress, send can continue.
Quoted 8 lines
I'm worried about the use case where we have:
* periodic automated snapshots
* periodic automated deletion of old snapshots
* periodic send for backupThe automated deletion doesn't want to error out if send is in progress, it just wants the deletion to happen in the background.
I'd give the precedence to the 'backup' process before the 'clean old snapshots', because it can do more harm if the snapshot is removed meanwhile without any possibility to recover.
I understand that send does not have to be done only for the backup purposes, the snapshots can be recreated in case of error, etc.
Adding more tunables would lead to confusion and usability mess, eg. somehow mark the snapshot as disposable, or not, wrt the send-in-progress status. I don't want to go that way.
The automatic deletion process is external to btrfs and has more context of what to do if the subvolume deletion fails, for example schedule another deletion attempt.
I don't think this would cause severe problems if the the snapshots live for a bit longer, but yes it needs more work on the user's side.
Re: [PATCH 1/2] btrfs: protect snapshots from deleting during send
Quoted 25 lines · David Sterba
Quoted 24 lines · Chris Mason
Quoted 13 lines · David Sterba
The patch "Btrfs: fix protection between send and root deletion" (
18f687d538449373c37c) does not actually prevent to delete the snapshot and just takes care during background cleaning, but this seems rather user unfriendly, this patch implements the idea presented in- add an internal
root_itemflag to denote a dead root
- check if thesend_in_progressis set and refuse to delete, otherwise set the flag and proceed
- check the flag in send similar to thebtrfs_root_readonlychecks, for all involved rootsQuoted 3 lines
The root lookup in send via
btrfs_read_fs_root_no_namewill check if the root is really dead or not. If it is, ENOENT, aborted send. If it's alive, it's protected bysend_in_progress, send can continue.I'm worried about the use case where we have:
* periodic automated snapshots
* periodic automated deletion of old snapshots
* periodic send for backup
Quoted 6 lines
Quoted 2 lines
The automated deletion doesn't want to error out if send is in progress, it just wants the deletion to happen in the background.
I'd give the precedence to the 'backup' process before the 'clean old snapshots', because it can do more harm if the snapshot is removed meanwhile without any possibility to recover.
Right, we don't want either process to stop with an error. We just want them to continue happily and do the right thing...
-chris
Re: [PATCH 1/2] btrfs: protect snapshots from deleting during send
Quoted 9 lines · Chris Mason
Quoted 9 lines
Quoted 8 lines
I'm worried about the use case where we have:
* periodic automated snapshots
* periodic automated deletion of old snapshots
* periodic send for backupThe automated deletion doesn't want to error out if send is in progress, it just wants the deletion to happen in the background.
Quoted 6 lines
Quoted 3 lines
I'd give the precedence to the 'backup' process before the 'clean old snapshots', because it can do more harm if the snapshot is removed meanwhile without any possibility to recover.
Right, we don't want either process to stop with an error. We just want them to continue happily and do the right thing...
... if everything goes without errors. Not like send going out of memory, send through network has a glitch, send to a file runs out of space, and has to be restarted. Is this too unrealistic to happen?
Re: [PATCH 1/2] btrfs: protect snapshots from deleting during send
Quoted 14 lines · David Sterba
Quoted 13 lines · Chris Mason
Quoted 12 lines
Quoted 8 lines
I'm worried about the use case where we have:
* periodic automated snapshots
* periodic automated deletion of old snapshots
* periodic send for backupThe automated deletion doesn't want to error out if send is in progress, it just wants the deletion to happen in the background.
I'd give the precedence to the 'backup' process before the 'clean old snapshots', because it can do more harm if the snapshot is removed meanwhile without any possibility to recover.
Quoted 7 lines
Quoted 2 lines
Right, we don't want either process to stop with an error. We just want them to continue happily and do the right thing...
... if everything goes without errors. Not like send going out of memory, send through network has a glitch, send to a file runs out of space, and has to be restarted. Is this too unrealistic to happen?
It's a good point, a better way to say what I have in mind is that we shouldn't be adding new transient errors to the send process (on purpose ;)
-chris
Re: [PATCH 1/2] btrfs: protect snapshots from deleting during send
Quoted 20 lines · Chris Mason
Quoted 17 lines · David Sterba
Quoted 15 lines · Chris Mason
Quoted 6 lines
Quoted 6 lines
I'm worried about the use case where we have:
* periodic automated snapshots
* periodic automated deletion of old snapshots
* periodic send for backupQuoted 6 lines
Quoted 2 lines
The automated deletion doesn't want to error out if send is in progress, it just wants the deletion to happen in the background.
I'd give the precedence to the 'backup' process before the 'clean old snapshots', because it can do more harm if the snapshot is removed meanwhile without any possibility to recover.
Right, we don't want either process to stop with an error. We just want them to continue happily and do the right thing...
Quoted 6 lines
Quoted 3 lines
... if everything goes without errors. Not like send going out of memory, send through network has a glitch, send to a file runs out of space, and has to be restarted. Is this too unrealistic to happen?
It's a good point, a better way to say what I have in mind is that we shouldn't be adding new transient errors to the send process (on purpose ;)
Ok, I got a wrong understanding from your previous reply.
So the next thing in the list to try is to add tunables affecting delete vs send. Something like
$ btrfs send --protect-subvols -c clone1 -c clone2 source
that would disallow to delete clone1, clone2 and source, passed to the ioctl as a flag and internally adding another refcount for 'how many times it has been protected'. Sounds ugly, but would cover all possible combinations of sending with or without deletion protection.
Re: [PATCH 1/2] btrfs: protect snapshots from deleting during send
Quoted 33 lines · David Sterba
Quoted 20 lines · Chris Mason
Quoted 17 lines · David Sterba
Quoted 15 lines · Chris Mason
Quoted 6 lines
Quoted 6 lines
I'm worried about the use case where we have:
* periodic automated snapshots
* periodic automated deletion of old snapshots
* periodic send for backupQuoted 6 lines
Quoted 2 lines
The automated deletion doesn't want to error out if send is in progress, it just wants the deletion to happen in the background.
I'd give the precedence to the 'backup' process before the 'clean old snapshots', because it can do more harm if the snapshot is removed meanwhile without any possibility to recover.
Right, we don't want either process to stop with an error. We just want them to continue happily and do the right thing...
Quoted 6 lines
Quoted 3 lines
... if everything goes without errors. Not like send going out of memory, send through network has a glitch, send to a file runs out of space, and has to be restarted. Is this too unrealistic to happen?
It's a good point, a better way to say what I have in mind is that we shouldn't be adding new transient errors to the send process (on purpose ;)
Ok, I got a wrong understanding from your previous reply.
So the next thing in the list to try is to add tunables affecting delete vs send. Something like
Quoted 7 lines
$ btrfs send --protect-subvols -c clone1 -c clone2 sourcethat would disallow to delete clone1, clone2 and source, passed to the ioctl as a flag and internally adding another refcount for 'how many times it has been protected'. Sounds ugly, but would cover all possible combinations of sending with or without deletion protection.
Ok, I reread the patch and your original point about dealing with a send + delete + network interruption. That's the part I didn't catch the first time around.
So in my example with the automated tool, the tool really shouldn't be deleting a snapshot where send is in progress. The tool should be told that snapshot is busy and try to delete it again later.
It makes more sense now, 'll queue this up for 3.16 and we can try it out in -next.
-chris
Re: [PATCH 1/2] btrfs: protect snapshots from deleting during send
Quoted 2 lines · Chris Mason
It makes more sense now, 'll queue this up for 3.16 and we can try it out in -next.
Thanks. 3.16 is fine for me.
Re: [PATCH 1/2] btrfs: protect snapshots from deleting during send
In reply to Chris MasonQuoted 5 lines · Chris Mason
So in my example with the automated tool, the tool really shouldn't be deleting a snapshot where send is in progress. The tool should be told that snapshot is busy and try to delete it again later.
It makes more sense now, 'll queue this up for 3.16 and we can try it out in -next.
-chris
So ... does this mean the plan is to a) have userland tool give an error; or b) a deletion would be "scheduled" in the background for as soon as the send has completed?
Signature · 4 lines
__________ Brendan Hide http://swiftspirit.co.za/ http://www.webafrica.co.za/?AFF1E97
Re: [PATCH 1/2] btrfs: protect snapshots from deleting during send
Quoted 5 lines · Brendan Hide
Quoted 4 lines · Chris Mason
So in my example with the automated tool, the tool really shouldn't be deleting a snapshot where send is in progress. The tool should be told that snapshot is busy and try to delete it again later.
Quoted 7 lines
Quoted 4 lines
It makes more sense now, 'll queue this up for 3.16 and we can try it out in -next.
-chris
So ... does this mean the plan is to a) have userland tool give an error; or
b) a deletion would be "scheduled" in the background for as soon as the send has completed?
b) is current state, a) is the plan
with the patch, 'btrfs subvol delete' would return EPERM/EBUSY
Re: [PATCH 1/2] btrfs: protect snapshots from deleting during send
Quoted 9 lines · David Sterba
Quoted 8 lines · Brendan Hide
Quoted 7 lines · Chris Mason
So in my example with the automated tool, the tool really shouldn't be deleting a snapshot where send is in progress. The tool should be told that snapshot is busy and try to delete it again later.
It makes more sense now, 'll queue this up for 3.16 and we can try it out in -next.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
-chris
So ... does this mean the plan is to a) have userland tool give an error; or
b) a deletion would be "scheduled" in the background for as soon as the send has completed?b) is current state, a) is the plan
with the patch, 'btrfs subvol delete' would return EPERM/EBUSY
My apologies, I should have followed up on this a while ago already. :-/
Would having something closer to b) be more desirable if the resource simply disappears but continues in the background? This would be as in a lazy umount, where presently-open files are left open and writable but the directory tree has "disappeared".
I submit that, with a), the actual status is more obvious/concrete whereas with b+lazy), current issues would flow smoothly with no errors and no foreseeable future issues.
I reserve the right to be wrong, of course. ;)
Signature · 4 lines
__________ Brendan Hide http://swiftspirit.co.za/ http://www.webafrica.co.za/?AFF1E97
Re: [PATCH 1/2] btrfs: protect snapshots from deleting during send
In reply to David SterbaQuoted 34 lines · David Sterba
The patch "Btrfs: fix protection between send and root deletion" (
18f687d538449373c37c) does not actually prevent to delete the snapshot and just takes care during background cleaning, but this seems rather user unfriendly, this patch implements the idea presented in- add an internal
root_itemflag to denote a dead root
- check if thesend_in_progressis set and refuse to delete, otherwise set the flag and proceed
- check the flag in send similar to thebtrfs_root_readonlychecks, for all involved rootsThe root lookup in send via
btrfs_read_fs_root_no_namewill check if the root is really dead or not. If it is, ENOENT, aborted send. If it's alive, it's protected bysend_in_progress, send can continue.fs/btrfs/send.c +1
-1@@ -5263,7 +5263,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)/* * The subvolume must remain read-only during send, protect against -* making it RW. +* making it RW. This also protects against deletion. */ spin_lock (&send_root->root_item_lock); send_root->send_in_progress++;@@ -5284,6 +5284,15 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) goto out; }
fs/btrfs/send.c long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) · line 5295
Quoted 10 lines
fs/btrfs/send.c +9
-0+/* +* Unlikely but possible, if the subvolume is marked for deletion but +* is slow to remove the directory entry, send can still be started +*/ +if (btrfs_root_dead (sctx->parent_root)) { + ret = -EPERM ; + goto out; + } +
This should be sctx->send_root and it should be lower right? At this point the sctx hasn't been allocated yet.
I've changed it here and it'll go into integration shortly, but I can rebase in something different if required.
-chris