[29/35] btrfs: just delete pending bgs if we are aborted
diff mbox series

Message ID 20180830174225.2200-30-josef@toxicpanda.com
State New
Headers show
Series
  • My current patch queue
Related show

Commit Message

Josef Bacik Aug. 30, 2018, 5:42 p.m. UTC
We still need to do all of the accounting cleanup for pending block
groups if we abort.  So set the ret to trans->aborted so if we aborted
the cleanup happens and everybody is happy.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nikolay Borisov Aug. 31, 2018, 7:46 a.m. UTC | #1
On 30.08.2018 20:42, Josef Bacik wrote:
> We still need to do all of the accounting cleanup for pending block
> groups if we abort.  So set the ret to trans->aborted so if we aborted
> the cleanup happens and everybody is happy.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/extent-tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 90f267f4dd0f..132a1157982c 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -10333,7 +10333,7 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans)
>  	struct btrfs_root *extent_root = fs_info->extent_root;
>  	struct btrfs_block_group_item item;
>  	struct btrfs_key key;
> -	int ret = 0;
> +	int ret = trans->aborted;

This is really subtle and magical and not obvious from the context of
the patch, but if the transaction is aborted this will change the loop
to actually just delete all block groups in ->new_bgs. I'd rather have
an explicit loop for that honestly.

>  	bool can_flush_pending_bgs = trans->can_flush_pending_bgs;
>  
>  	trans->can_flush_pending_bgs = false;
>
Josef Bacik Aug. 31, 2018, 2:05 p.m. UTC | #2
On Fri, Aug 31, 2018 at 10:46:36AM +0300, Nikolay Borisov wrote:
> 
> 
> On 30.08.2018 20:42, Josef Bacik wrote:
> > We still need to do all of the accounting cleanup for pending block
> > groups if we abort.  So set the ret to trans->aborted so if we aborted
> > the cleanup happens and everybody is happy.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/extent-tree.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > index 90f267f4dd0f..132a1157982c 100644
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -10333,7 +10333,7 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans)
> >  	struct btrfs_root *extent_root = fs_info->extent_root;
> >  	struct btrfs_block_group_item item;
> >  	struct btrfs_key key;
> > -	int ret = 0;
> > +	int ret = trans->aborted;
> 
> This is really subtle and magical and not obvious from the context of
> the patch, but if the transaction is aborted this will change the loop
> to actually just delete all block groups in ->new_bgs. I'd rather have
> an explicit loop for that honestly.

We need it this way in case creating the bg's errors out anyway, there's no
sense in adding a bunch of code to do something we have to handle already
anyway.  Thanks,

Josef
Omar Sandoval Sept. 1, 2018, 12:33 a.m. UTC | #3
On Thu, Aug 30, 2018 at 01:42:19PM -0400, Josef Bacik wrote:
> We still need to do all of the accounting cleanup for pending block
> groups if we abort.  So set the ret to trans->aborted so if we aborted
> the cleanup happens and everybody is happy.

Reviewed-by: Omar Sandoval <osandov@fb.com>

Reusing the loop is fine IMO, but a comment would be appreciated.

> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/extent-tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 90f267f4dd0f..132a1157982c 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -10333,7 +10333,7 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans)
>  	struct btrfs_root *extent_root = fs_info->extent_root;
>  	struct btrfs_block_group_item item;
>  	struct btrfs_key key;
> -	int ret = 0;
> +	int ret = trans->aborted;
>  	bool can_flush_pending_bgs = trans->can_flush_pending_bgs;
>  
>  	trans->can_flush_pending_bgs = false;
> -- 
> 2.14.3
>

Patch
diff mbox series

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 90f267f4dd0f..132a1157982c 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -10333,7 +10333,7 @@  void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans)
 	struct btrfs_root *extent_root = fs_info->extent_root;
 	struct btrfs_block_group_item item;
 	struct btrfs_key key;
-	int ret = 0;
+	int ret = trans->aborted;
 	bool can_flush_pending_bgs = trans->can_flush_pending_bgs;
 
 	trans->can_flush_pending_bgs = false;