| Message ID | 20180830174225.2200-30-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | My current patch queue | expand |
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; >
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
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 >
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;
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(-)