Skip to contentSkip to conversation

btrfs: delayed-inode: Remove wrong qgroup meta reservation calls

PATCH In review Qu Wenruo 2 participants 4 messages Apr 17, 2018 – Apr 18, 2018 Btrfs file system

lore
Share & tools
lore

Qu Wenruo

author
#0 lore raw

PATCH 1/1 btrfs: delayed-inode: Remove wrong qgroup meta reservation calls

Commit 4f5427ccce5d ("btrfs: delayed-inode: Use new qgroup meta rsv for delayed inode and item") merged into mainline is not the updated version submitted to the mail list in Dec 2017.

Which lacks the following fixes:

1) Remove btrfs_qgroup_convert_reserved_meta() call in btrfs_delayed_item_release_metadata()
2) Remove btrfs_qgroup_reserve_meta_prealloc() call in btrfs_delayed_inode_reserve_metadata()

Those fixes will resolve unexpected EDQUOT problems.

Fixes 4f5427ccce5d ("btrfs: delayed-inode: Use new qgroup meta rsv for delayed inode and item")
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/delayed-inode.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)
fs/btrfs/delayed-inode.c +17 -5
index 86ec2edc05e8..ac4fa7218c57 100644
@@ -569,6 +569,12 @@ static int btrfs_delayed_item_reserve_metadata(struct btrfs_trans_handle *trans,
 	dst_rsv = &fs_info->delayed_block_rsv;
 
 	num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1);
+
+	/*
+	 * Here we migrate space rsv from transaction rsv, since have
+	 * already reserved space when starting a transaction.
+	 * So no need to reserve qgroup space here.
+	 */
 	ret = btrfs_block_rsv_migrate(src_rsv, dst_rsv, num_bytes, 1);
 	if (!ret) {
 		trace_btrfs_space_reservation(fs_info, "delayed_item",

@@ -590,7 +596,10 @@ static void btrfs_delayed_item_release_metadata(struct btrfs_root *root,
 		return;
 
 	rsv = &fs_info->delayed_block_rsv;
-	btrfs_qgroup_convert_reserved_meta(root, item->bytes_reserved);
+	/*
+	 * Check btrfs_delayed_item_reserve_metadata() to see why we don't need
+	 * to release/reserve qgroup space.
+	 */
 	trace_btrfs_space_reservation(fs_info, "delayed_item",
 				      item->key.objectid, item->bytes_reserved,
 				      0);

@@ -615,9 +624,6 @@ static int btrfs_delayed_inode_reserve_metadata(
 
 	num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1);
 
-	ret = btrfs_qgroup_reserve_meta_prealloc(root, num_bytes, true);
-	if (ret < 0)
-		return ret;
 	/*
 	 * btrfs_dirty_inode will update the inode under btrfs_join_transaction
 	 * which doesn't reserve space for speed.  This is a problem since we

@@ -629,6 +635,10 @@ static int btrfs_delayed_inode_reserve_metadata(
 	 */
 	if (!src_rsv || (!trans->bytes_reserved &&
 			 src_rsv->type != BTRFS_BLOCK_RSV_DELALLOC)) {
+		ret = btrfs_qgroup_reserve_meta_prealloc(root,
+				fs_info->nodesize, true);
+		if (ret < 0)
+			return ret;
 		ret = btrfs_block_rsv_add(root, dst_rsv, num_bytes,
 					  BTRFS_RESERVE_NO_FLUSH);
 		/*

@@ -647,7 +657,9 @@ static int btrfs_delayed_inode_reserve_metadata(
 						      "delayed_inode",
 						      btrfs_ino(inode),
 						      num_bytes, 1);
-		}
+		} else
+			btrfs_qgroup_free_meta_prealloc(root,
+							fs_info->nodesize);
 		return ret;
 	}
Signature · 1 line
2.17.0
#1 lore raw
Quoted 11 lines · Qu Wenruo

Commit 4f5427ccce5d ("btrfs: delayed-inode: Use new qgroup meta rsv for delayed inode and item") merged into mainline is not the updated version submitted to the mail list in Dec 2017.

Which lacks the following fixes:

1) Remove btrfs_qgroup_convert_reserved_meta() call in btrfs_delayed_item_release_metadata()
2) Remove btrfs_qgroup_reserve_meta_prealloc() call in btrfs_delayed_inode_reserve_metadata()

Quoted 4 lines

Those fixes will resolve unexpected EDQUOT problems.

Fixes 4f5427ccce5d ("btrfs: delayed-inode: Use new qgroup meta rsv for delayed inode and item")
Signed-off-by Qu Wenruo <[email protected]>

Added to 4.17 queue, thanks.

fs/btrfs/delayed-inode.c static int btrfs_delayed_item_reserve_metadata(struct btrfs_trans_handle *trans, · line 577

Quoted 10 lines
fs/btrfs/delayed-inode.c +6 -0
@@ -569,6 +569,12 @@ static int btrfs_delayed_item_reserve_metadata(struct btrfs_trans_handle *trans,
 	dst_rsv = &fs_info->delayed_block_rsv;
 
 	num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1);
+
+	/*
+	 * Here we migrate space rsv from transaction rsv, since have
+	 * already reserved space when starting a transaction.
+	 * So no need to reserve qgroup space here.
+	 */

Please format the comments to the full line width.

fs/btrfs/delayed-inode.c static int btrfs_delayed_inode_reserve_metadata( · line 662

Quoted 8 lines
fs/btrfs/delayed-inode.c +3 -1
@@ -647,7 +657,9 @@ static int btrfs_delayed_inode_reserve_metadata(
 						      "delayed_inode",
 						      btrfs_ino(inode),
 						      num_bytes, 1);
-		}
+		} else
+			btrfs_qgroup_free_meta_prealloc(root,
+							fs_info->nodesize);

Please don't diverge from the coding style, I'm fixing such issues but, you know.

author
#2 lore raw
Quoted 22 lines · David Sterba
Quoted 11 lines · Qu Wenruo

Commit 4f5427ccce5d ("btrfs: delayed-inode: Use new qgroup meta rsv for delayed inode and item") merged into mainline is not the updated version submitted to the mail list in Dec 2017.

Which lacks the following fixes:

1) Remove btrfs_qgroup_convert_reserved_meta() call in btrfs_delayed_item_release_metadata()
2) Remove btrfs_qgroup_reserve_meta_prealloc() call in btrfs_delayed_inode_reserve_metadata()

Quoted 4 lines

Those fixes will resolve unexpected EDQUOT problems.

Fixes 4f5427ccce5d ("btrfs: delayed-inode: Use new qgroup meta rsv for delayed inode and item")
Signed-off-by Qu Wenruo <[email protected]>

Added to 4.17 queue, thanks.

Quoted 3 lines
fs/btrfs/delayed-inode.c +0 -0
@@ -569,6 +569,12 @@ static int btrfs_delayed_item_reserve_metadata(struct btrfs_trans_handle *trans,
 	dst_rsv = &fs_info->delayed_block_rsv;
Quoted 9 lines

fs/btrfs/delayed-inode.c static int btrfs_delayed_item_reserve_metadata(struct btrfs_trans_handle *trans, · line 577

Quoted 7 lines
fs/btrfs/delayed-inode.c +6 -0
 	num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1);
+
+	/*
+	 * Here we migrate space rsv from transaction rsv, since have
+	 * already reserved space when starting a transaction.
+	 * So no need to reserve qgroup space here.
+	 */

Please format the comments to the full line width.

Right, the already can go previous line without exceeding 80 chars.

But the "So no need to..." line is a new line so it will not take up any space of previous line anyway.

Quoted 12 lines

fs/btrfs/delayed-inode.c static int btrfs_delayed_inode_reserve_metadata( · line 662

Quoted 9 lines
fs/btrfs/delayed-inode.c +3 -1
@@ -647,7 +657,9 @@ static int btrfs_delayed_inode_reserve_metadata(
 						      "delayed_inode",
 						      btrfs_ino(inode),
 						      num_bytes, 1);
-		}
+		} else
+			btrfs_qgroup_free_meta_prealloc(root,
+							fs_info->nodesize);

Please don't diverge from the coding style, I'm fixing such issues but, you know.

For this, did you mean the bracket for else branch?

Thanks,
Qu

#3 lore raw
Quoted 12 lines · Qu Wenruo
Quoted 11 lines

fs/btrfs/delayed-inode.c static int btrfs_delayed_item_reserve_metadata(struct btrfs_trans_handle *trans, · line 577

Quoted 9 lines
fs/btrfs/delayed-inode.c +6 -0
 	dst_rsv = &fs_info->delayed_block_rsv;
 
 	num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1);
+
+	/*
+	 * Here we migrate space rsv from transaction rsv, since have
+	 * already reserved space when starting a transaction.
+	 * So no need to reserve qgroup space here.
+	 */

Please format the comments to the full line width.

Quoted 4 lines

Right, the already can go previous line without exceeding 80 chars.

But the "So no need to..." line is a new line so it will not take up any space of previous line anyway.

You can split the loosely related parts by an empty line, ie. paragraphs, but I don't tend to like if the new sentence on a new line when it logically follows the previous one.

Quoted 12 lines
Quoted 11 lines

fs/btrfs/delayed-inode.c static int btrfs_delayed_inode_reserve_metadata( · line 662

Quoted 8 lines
fs/btrfs/delayed-inode.c +3 -1
@@ -647,7 +657,9 @@ static int btrfs_delayed_inode_reserve_metadata(
 						      "delayed_inode",
 						      btrfs_ino(inode),
 						      num_bytes, 1);
-		}
+		} else
+			btrfs_qgroup_free_meta_prealloc(root,
+							fs_info->nodesize);

Please don't diverge from the coding style, I'm fixing such issues but, you know.

Quoted 1 line

For this, did you mean the bracket for else branch?

Yes.

	if () {
	...
	} else {
	...
	}