[2/5] btrfs: use a separate variable to store the num_bytes of the qgroup_reserve
diff mbox series

Message ID 20180804131057.9967-3-lufq.fnst@cn.fujitsu.com
State New
Headers show
Series
  • some trivial cleanup about btrfs_delete_subvolume
Related show

Commit Message

Lu Fengqi Aug. 4, 2018, 1:10 p.m. UTC
After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned
again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv
fails, we cannot properly free the num_bytes of the previous
qgroup_reserve.

Delete the comment for the qgroup_reserved that does not exist and add a
comment about use_global_rsv.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 fs/btrfs/extent-tree.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

Comments

David Sterba Aug. 7, 2018, 4:19 p.m. UTC | #1
On Sat, Aug 04, 2018 at 09:10:54PM +0800, Lu Fengqi wrote:
> After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned
> again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv
> fails, we cannot properly free the num_bytes of the previous
> qgroup_reserve.

This does not look like a trivial cleanup at all. There was an unused
parameter, removed in c4c129db5da8f070147f175 ("btrfs: drop unused
parameter qgroup_reserved"), that introduced the bug.  This was in this
rc1 so it's a regression and I'll consider pushing it to the 4.18 final.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lu Fengqi Aug. 8, 2018, 3:04 a.m. UTC | #2
On Tue, Aug 07, 2018 at 06:19:12PM +0200, David Sterba wrote:
>On Sat, Aug 04, 2018 at 09:10:54PM +0800, Lu Fengqi wrote:
>> After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned
>> again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv
>> fails, we cannot properly free the num_bytes of the previous
>> qgroup_reserve.
>
>This does not look like a trivial cleanup at all. There was an unused
>parameter, removed in c4c129db5da8f070147f175 ("btrfs: drop unused
>parameter qgroup_reserved"), that introduced the bug.  This was in this
>rc1 so it's a regression and I'll consider pushing it to the 4.18 final.
>
>

I apologize for the inconvenience. I should add the Fixes tag, and really
shouldn't mix it into the trivial cleanup patch set.
David Sterba Aug. 8, 2018, 1:56 p.m. UTC | #3
On Wed, Aug 08, 2018 at 11:04:37AM +0800, Lu Fengqi wrote:
> On Tue, Aug 07, 2018 at 06:19:12PM +0200, David Sterba wrote:
> >On Sat, Aug 04, 2018 at 09:10:54PM +0800, Lu Fengqi wrote:
> >> After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned
> >> again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv
> >> fails, we cannot properly free the num_bytes of the previous
> >> qgroup_reserve.
> >
> >This does not look like a trivial cleanup at all. There was an unused
> >parameter, removed in c4c129db5da8f070147f175 ("btrfs: drop unused
> >parameter qgroup_reserved"), that introduced the bug.  This was in this
> >rc1 so it's a regression and I'll consider pushing it to the 4.18 final.
> 
> I apologize for the inconvenience. I should add the Fixes tag, and really
> shouldn't mix it into the trivial cleanup patch set.

As the bug does not qualify as urgent regression, I'm not going to
forward it to 4.18. Please update the subject and changelog so it
reflects that's an actual fix. I'll add it to the 4.19 queue then.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lu Fengqi Aug. 8, 2018, 2:33 p.m. UTC | #4
David Sterba <dsterba@suse.cz> 于2018年8月8日周三 下午9:57写道:
>
> On Wed, Aug 08, 2018 at 11:04:37AM +0800, Lu Fengqi wrote:
> > On Tue, Aug 07, 2018 at 06:19:12PM +0200, David Sterba wrote:
> > >On Sat, Aug 04, 2018 at 09:10:54PM +0800, Lu Fengqi wrote:
> > >> After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned
> > >> again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv
> > >> fails, we cannot properly free the num_bytes of the previous
> > >> qgroup_reserve.
> > >
> > >This does not look like a trivial cleanup at all. There was an unused
> > >parameter, removed in c4c129db5da8f070147f175 ("btrfs: drop unused
> > >parameter qgroup_reserved"), that introduced the bug.  This was in this
> > >rc1 so it's a regression and I'll consider pushing it to the 4.18 final.
> >
> > I apologize for the inconvenience. I should add the Fixes tag, and really
> > shouldn't mix it into the trivial cleanup patch set.
>
> As the bug does not qualify as urgent regression, I'm not going to
> forward it to 4.18. Please update the subject and changelog so it
> reflects that's an actual fix. I'll add it to the 4.19 queue then.
> Thanks.

No problem. I will send it tomorrow.

-------------------------------------------------------------
Thanks,
Lu

> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox series

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index de6f75f5547b..8b009ae964cc 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5800,7 +5800,7 @@  void btrfs_trans_release_chunk_metadata(struct btrfs_trans_handle *trans)
  * root: the root of the parent directory
  * rsv: block reservation
  * items: the number of items that we need do reservation
- * qgroup_reserved: used to return the reserved size in qgroup
+ * use_global_rsv: allow fallback to the global block reservation
  *
  * This function is used to reserve the space for snapshot/subvolume
  * creation and deletion. Those operations are different with the
@@ -5810,10 +5810,10 @@  void btrfs_trans_release_chunk_metadata(struct btrfs_trans_handle *trans)
  * the space reservation mechanism in start_transaction().
  */
 int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
-				     struct btrfs_block_rsv *rsv,
-				     int items,
+				     struct btrfs_block_rsv *rsv, int items,
 				     bool use_global_rsv)
 {
+	u64 qgroup_num_bytes = 0;
 	u64 num_bytes;
 	int ret;
 	struct btrfs_fs_info *fs_info = root->fs_info;
@@ -5821,12 +5821,10 @@  int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
 
 	if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags)) {
 		/* One for parent inode, two for dir entries */
-		num_bytes = 3 * fs_info->nodesize;
-		ret = btrfs_qgroup_reserve_meta_prealloc(root, num_bytes, true);
+		qgroup_num_bytes = 3 * fs_info->nodesize;
+		ret = btrfs_qgroup_reserve_meta_prealloc(root, qgroup_num_bytes, true);
 		if (ret)
 			return ret;
-	} else {
-		num_bytes = 0;
 	}
 
 	num_bytes = btrfs_calc_trans_metadata_size(fs_info, items);
@@ -5838,8 +5836,8 @@  int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
 	if (ret == -ENOSPC && use_global_rsv)
 		ret = btrfs_block_rsv_migrate(global_rsv, rsv, num_bytes, 1);
 
-	if (ret && num_bytes)
-		btrfs_qgroup_free_meta_prealloc(root, num_bytes);
+	if (ret && qgroup_num_bytes)
+		btrfs_qgroup_free_meta_prealloc(root, qgroup_num_bytes);
 
 	return ret;
 }