[PATCH] btrfs: commit transaction after deleting a subvolume

[PATCH] btrfs: commit transaction after deleting a subvolume

[David Sterba]

Alex pointed out the consequences after a transaction is not committed
when a subvolume is deleted, so in case of a crash before an actual
commit happens will let the subvolume reappear.

Original post:
http://www.spinics.net/lists/linux-btrfs/msg22088.html

Josef's objections:
http://www.spinics.net/lists/linux-btrfs/msg22256.html

While there's no need to do a full commit for regular files, a subvolume
may get a different treatment.

http://www.spinics.net/lists/linux-btrfs/msg23087.html:

"That a subvol/snapshot may appear after crash if transation commit did
not happen does not feel so good. We know that the subvol is only
scheduled for deletion and needs to be processed by cleaner.

>From that point I'd rather see the commit to happen to avoid any
unexpected surprises.  A subvolume that re-appears still holds the data
references and consumes space although the user does not assume that.

Automated snapshotting and deleting needs some guarantees about the
behaviour and what to do after a crash. So now it has to process the
backlog of previously deleted snapshots and verify that they're not
there, compared to "deleted -> will never appear, can forget about it".
"

There is a performance penalty incured by the change, but deleting a
subvolume is not a frequent operation and the tradeoff seems justified
by getting the guarantee stated above.

CC: Alex Lyakas <alex.btrfs@zadarastorage.com>
CC: Josef Bacik <jbacik@fusionio.com>
Signed-off-by: David Sterba <dsterba@suse.cz>
---
 fs/btrfs/ioctl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e407f75..4394632 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2268,7 +2268,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 out_end_trans:
 	trans->block_rsv = NULL;
 	trans->bytes_reserved = 0;
-	ret = btrfs_end_transaction(trans, root);
+	ret = btrfs_commit_transaction(trans, root);
 	if (ret && !err)
 		err = ret;
 	inode->i_flags |= S_DEAD;
-- 
1.7.9

Re: [PATCH] btrfs: commit transaction after deleting a subvolume

[Alex Lyakas]

 Thank you for addressing this, David.

On Sat, Aug 31, 2013 at 1:25 AM, David Sterba <dsterba@suse.cz> wrote:
> Alex pointed out the consequences after a transaction is not committed
> when a subvolume is deleted, so in case of a crash before an actual
> commit happens will let the subvolume reappear.
>
> Original post:
> http://www.spinics.net/lists/linux-btrfs/msg22088.html
>
> Josef's objections:
> http://www.spinics.net/lists/linux-btrfs/msg22256.html
>
> While there's no need to do a full commit for regular files, a subvolume
> may get a different treatment.
>
> http://www.spinics.net/lists/linux-btrfs/msg23087.html:
>
> "That a subvol/snapshot may appear after crash if transation commit did
> not happen does not feel so good. We know that the subvol is only
> scheduled for deletion and needs to be processed by cleaner.
>
> From that point I'd rather see the commit to happen to avoid any
> unexpected surprises.  A subvolume that re-appears still holds the data
> references and consumes space although the user does not assume that.
>
> Automated snapshotting and deleting needs some guarantees about the
> behaviour and what to do after a crash. So now it has to process the
> backlog of previously deleted snapshots and verify that they're not
> there, compared to "deleted -> will never appear, can forget about it".
> "
>
> There is a performance penalty incured by the change, but deleting a
> subvolume is not a frequent operation and the tradeoff seems justified
> by getting the guarantee stated above.
>
> CC: Alex Lyakas <alex.btrfs@zadarastorage.com>
> CC: Josef Bacik <jbacik@fusionio.com>
> Signed-off-by: David Sterba <dsterba@suse.cz>
> ---
>  fs/btrfs/ioctl.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index e407f75..4394632 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2268,7 +2268,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>  out_end_trans:
>         trans->block_rsv = NULL;
>         trans->bytes_reserved = 0;
> -       ret = btrfs_end_transaction(trans, root);
> +       ret = btrfs_commit_transaction(trans, root);
>         if (ret && !err)
>                 err = ret;
>         inode->i_flags |= S_DEAD;
> --
> 1.7.9
>

Re: [PATCH] btrfs: commit transaction after deleting a subvolume

[Chris Mason]

Quoting David Sterba (2013-08-30 18:25:46)
> Alex pointed out the consequences after a transaction is not committed
> when a subvolume is deleted, so in case of a crash before an actual
> commit happens will let the subvolume reappear.
> 
> Original post:
> http://www.spinics.net/lists/linux-btrfs/msg22088.html
> 
> Josef's objections:
> http://www.spinics.net/lists/linux-btrfs/msg22256.html
> 
> While there's no need to do a full commit for regular files, a subvolume
> may get a different treatment.
> 
> http://www.spinics.net/lists/linux-btrfs/msg23087.html:
> 
> "That a subvol/snapshot may appear after crash if transation commit did
> not happen does not feel so good. We know that the subvol is only
> scheduled for deletion and needs to be processed by cleaner.
> 
> From that point I'd rather see the commit to happen to avoid any
> unexpected surprises.  A subvolume that re-appears still holds the data
> references and consumes space although the user does not assume that.
> 
> Automated snapshotting and deleting needs some guarantees about the
> behaviour and what to do after a crash. So now it has to process the
> backlog of previously deleted snapshots and verify that they're not
> there, compared to "deleted -> will never appear, can forget about it".
> "

My objections are pretty similar to Josef's.  But, there's no reason we
can't change the progs to optionally trigger a commit.

What I want to avoid is bulk snapshot deletion triggering a commit for
each individual snapshot.  

-chris

Re: [PATCH] btrfs: commit transaction after deleting a subvolume

[David Sterba]

On Sun, Oct 20, 2013 at 08:19:59AM -0400, Chris Mason wrote:
> My objections are pretty similar to Josef's.  But, there's no reason we
> can't change the progs to optionally trigger a commit.

Works for me, though I'm not clear what should be the default.

> What I want to avoid is bulk snapshot deletion triggering a commit for
> each individual snapshot.  

I agree and came to the same conclusion later on.

david