btrfs: qgroup: Skip delayed data ref for reloc trees

PATCH In review Qu Wenruo 3 participants 5 messages Nov 20, 2018 Btrfs file system

Share & tools

Qu Wenruo

author

2018-11-20 08:46 UTC #0 lore raw

PATCH 1/1 btrfs: qgroup: Skip delayed data ref for reloc trees

Currently we only check @ref_root in btrfs_add_delayed_data_ref() to determine whether a data delayed ref is for reloc tree.

Such check is insufficient as for relocation we could pass @ref_root as the source file tree, causing qgroup to trace unchanged data extents even we're only relocating metadata chunks.

We could insert qgroup extent record for the following call trace even we're only relocating metadata block group:

And another case when dropping reloc tree:

This patch will introduce @root parameter for btrfs_add_delayed_data_ref(), so that we could determine if this data extent belongs to reloc tree or not.

This could skip data extents which aren't really modified during relocation.

For the same real world 4G data 16 snapshots 4K nodesize metadata balance test:

                     | v4.20-rc1 + delaye*  | w/ patch       | diff
-----------------------------------------------------------------------
relocated extents    | 22773                | 22656          | -0.1%
qgroup dirty extents | 122879               | 74316          | -39.5%
time (real)          | 23.073s              | 14.971         | -35.1%

*: v4.20-rc1 + delayed subtree scan patchset

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/delayed-ref.c | 3 ++-
 fs/btrfs/delayed-ref.h | 1 +
 fs/btrfs/extent-tree.c | 6 +++---
 3 files changed, 6 insertions(+), 4 deletions(-)

fs/btrfs/delayed-ref.c +2 -1

index 9301b3ad9217..269bd6ecb8f3 100644
@@ -798,6 +798,7 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,
  * add a delayed data ref. it's similar to btrfs_add_delayed_tree_ref.
  */
 int btrfs_add_delayed_data_ref(struct btrfs_trans_handle *trans,
+			       struct btrfs_root *root,
 			       u64 bytenr, u64 num_bytes,
 			       u64 parent, u64 ref_root,
 			       u64 owner, u64 offset, u64 reserved, int action,

@@ -835,7 +836,7 @@ int btrfs_add_delayed_data_ref(struct btrfs_trans_handle *trans,
 	}
 
 	if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) &&
-	    is_fstree(ref_root)) {
+	    is_fstree(ref_root) && is_fstree(root->root_key.objectid)) {
 		record = kmalloc(sizeof(*record), GFP_NOFS);
 		if (!record) {
 			kmem_cache_free(btrfs_delayed_data_ref_cachep, ref);

fs/btrfs/delayed-ref.h +1 -0

index 8e20c5cb5404..6c60737e55d6 100644
@@ -240,6 +240,7 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,
 			       struct btrfs_delayed_extent_op *extent_op,
 			       int *old_ref_mod, int *new_ref_mod);
 int btrfs_add_delayed_data_ref(struct btrfs_trans_handle *trans,
+			       struct btrfs_root *root,
 			       u64 bytenr, u64 num_bytes,
 			       u64 parent, u64 ref_root,
 			       u64 owner, u64 offset, u64 reserved, int action,

fs/btrfs/extent-tree.c +3 -3

index a1febf155747..0554d2cc2ea1 100644
@@ -2046,7 +2046,7 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
 						 BTRFS_ADD_DELAYED_REF, NULL,
 						 &old_ref_mod, &new_ref_mod);
 	} else {
-		ret = btrfs_add_delayed_data_ref(trans, bytenr,
+		ret = btrfs_add_delayed_data_ref(trans, root, bytenr,
 						 num_bytes, parent,
 						 root_objectid, owner, offset,
 						 0, BTRFS_ADD_DELAYED_REF,

@@ -7104,7 +7104,7 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans,
 						 BTRFS_DROP_DELAYED_REF, NULL,
 						 &old_ref_mod, &new_ref_mod);
 	} else {
-		ret = btrfs_add_delayed_data_ref(trans, bytenr,
+		ret = btrfs_add_delayed_data_ref(trans, root, bytenr,
 						 num_bytes, parent,
 						 root_objectid, owner, offset,
 						 0, BTRFS_DROP_DELAYED_REF,

@@ -8080,7 +8080,7 @@ int btrfs_alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
 			   root->root_key.objectid, owner, offset,
 			   BTRFS_ADD_DELAYED_EXTENT);
 
-	ret = btrfs_add_delayed_data_ref(trans, ins->objectid,
+	ret = btrfs_add_delayed_data_ref(trans, root, ins->objectid,
 					 ins->offset, 0,
 					 root->root_key.objectid, owner,
 					 offset, ram_bytes,

Signature · 1 line

2.19.1

Nikolay Borisov

2018-11-20 08:52 UTC #1 lore raw

Quoted 57 lines · Qu Wenruo

Currently we only check @ref_root in btrfs_add_delayed_data_ref() to determine whether a data delayed ref is for reloc tree.
Such check is insufficient as for relocation we could pass @ref_root as the source file tree, causing qgroup to trace unchanged data extents even we're only relocating metadata chunks.
We could insert qgroup extent record for the following call trace even we're only relocating metadata block group:
do_relocation()
|- btrfs_cow_block(root=reloc_root)
|- update_ref_for_cow(root=reloc_root)
|- __btrfs_mod_ref(root=reloc_root)
|- ref_root = btrfs_header_owner()
|- btrfs_add_delayed_data_ref(ref_root=source_root)
And another case when dropping reloc tree:
clean_dirty_root()
|- btrfs_drop_snapshot(root=relocc_root)
|- walk_up_tree(root=reloc_root)
|- walk_up_proc(root=reloc_root)
|- btrfs_dec_ref(root=reloc_root)
|- __btrfs_mod_ref(root=reloc_root)
|- ref_root = btrfs_header_owner()
|- btrfs_add_delayed_data_ref(ref_root=source_root)
This patch will introduce @root parameter for btrfs_add_delayed_data_ref(), so that we could determine if this data extent belongs to reloc tree or not.
This could skip data extents which aren't really modified during relocation.
For the same real world 4G data 16 snapshots 4K nodesize metadata balance test:
                     | v4.20-rc1 + delaye*  | w/ patch       | diff
-----------------------------------------------------------------------
relocated extents    | 22773                | 22656          | -0.1%
qgroup dirty extents | 122879               | 74316          | -39.5%
time (real)          | 23.073s              | 14.971         | -35.1%
*: v4.20-rc1 + delayed subtree scan patchset
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/delayed-ref.c | 3 ++-
 fs/btrfs/delayed-ref.h | 1 +
 fs/btrfs/extent-tree.c | 6 +++---
 3 files changed, 6 insertions(+), 4 deletions(-)
fs/btrfs/delayed-ref.c +0 -0
index 9301b3ad9217..269bd6ecb8f3 100644

fs/btrfs/delayed-ref.c int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans, · line 801

Quoted 4 lines

fs/btrfs/delayed-ref.c +1 -0

@@ -798,6 +798,7 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,
  * add a delayed data ref. it's similar to btrfs_add_delayed_tree_ref.
  */
 int btrfs_add_delayed_data_ref(struct btrfs_trans_handle *trans,
+			       struct btrfs_root *root,

I'm beginning to wonder, should we document btrfs_add_delayed_data_ref/btrfs_add_tree_ref arguments separate for each function, or should only the differences be documented - in this case the newly added root parameter. The rest of the arguments are being documented at init_delayed_ref_common.

Quoted 56 lines

fs/btrfs/delayed-ref.c +1 -1

 			       u64 bytenr, u64 num_bytes,
 			       u64 parent, u64 ref_root,
 			       u64 owner, u64 offset, u64 reserved, int action,

@@ -835,7 +836,7 @@ int btrfs_add_delayed_data_ref(struct btrfs_trans_handle *trans,
 	}
 
 	if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) &&
-	    is_fstree(ref_root)) {
+	    is_fstree(ref_root) && is_fstree(root->root_key.objectid)) {
 		record = kmalloc(sizeof(*record), GFP_NOFS);
 		if (!record) {
 			kmem_cache_free(btrfs_delayed_data_ref_cachep, ref);

fs/btrfs/delayed-ref.h +1 -0

index 8e20c5cb5404..6c60737e55d6 100644
@@ -240,6 +240,7 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,
 			       struct btrfs_delayed_extent_op *extent_op,
 			       int *old_ref_mod, int *new_ref_mod);
 int btrfs_add_delayed_data_ref(struct btrfs_trans_handle *trans,
+			       struct btrfs_root *root,
 			       u64 bytenr, u64 num_bytes,
 			       u64 parent, u64 ref_root,
 			       u64 owner, u64 offset, u64 reserved, int action,

fs/btrfs/extent-tree.c +3 -3

index a1febf155747..0554d2cc2ea1 100644
@@ -2046,7 +2046,7 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
 						 BTRFS_ADD_DELAYED_REF, NULL,
 						 &old_ref_mod, &new_ref_mod);
 	} else {
-		ret = btrfs_add_delayed_data_ref(trans, bytenr,
+		ret = btrfs_add_delayed_data_ref(trans, root, bytenr,
 						 num_bytes, parent,
 						 root_objectid, owner, offset,
 						 0, BTRFS_ADD_DELAYED_REF,

@@ -7104,7 +7104,7 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans,
 						 BTRFS_DROP_DELAYED_REF, NULL,
 						 &old_ref_mod, &new_ref_mod);
 	} else {
-		ret = btrfs_add_delayed_data_ref(trans, bytenr,
+		ret = btrfs_add_delayed_data_ref(trans, root, bytenr,
 						 num_bytes, parent,
 						 root_objectid, owner, offset,
 						 0, BTRFS_DROP_DELAYED_REF,

@@ -8080,7 +8080,7 @@ int btrfs_alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
 			   root->root_key.objectid, owner, offset,
 			   BTRFS_ADD_DELAYED_EXTENT);
 
-	ret = btrfs_add_delayed_data_ref(trans, ins->objectid,
+	ret = btrfs_add_delayed_data_ref(trans, root, ins->objectid,
 					 ins->offset, 0,
 					 root->root_key.objectid, owner,
 					 offset, ram_bytes,

Qu Wenruo

2018-11-20 09:07 UTC #2 lore raw

Quoted 55 lines · Nikolay Borisov

Quoted 52 lines · Qu Wenruo
Currently we only check @ref_root in btrfs_add_delayed_data_ref() to determine whether a data delayed ref is for reloc tree.
Such check is insufficient as for relocation we could pass @ref_root as the source file tree, causing qgroup to trace unchanged data extents even we're only relocating metadata chunks.
We could insert qgroup extent record for the following call trace even we're only relocating metadata block group:
do_relocation()
|- btrfs_cow_block(root=reloc_root)
|- update_ref_for_cow(root=reloc_root)
|- __btrfs_mod_ref(root=reloc_root)
|- ref_root = btrfs_header_owner()
|- btrfs_add_delayed_data_ref(ref_root=source_root)
And another case when dropping reloc tree:
clean_dirty_root()
|- btrfs_drop_snapshot(root=relocc_root)
|- walk_up_tree(root=reloc_root)
|- walk_up_proc(root=reloc_root)
|- btrfs_dec_ref(root=reloc_root)
|- __btrfs_mod_ref(root=reloc_root)
|- ref_root = btrfs_header_owner()
|- btrfs_add_delayed_data_ref(ref_root=source_root)
This patch will introduce @root parameter for btrfs_add_delayed_data_ref(), so that we could determine if this data extent belongs to reloc tree or not.
This could skip data extents which aren't really modified during relocation.
For the same real world 4G data 16 snapshots 4K nodesize metadata balance test:
                     | v4.20-rc1 + delaye*  | w/ patch       | diff
-----------------------------------------------------------------------
relocated extents    | 22773                | 22656          | -0.1%
qgroup dirty extents | 122879               | 74316          | -39.5%
time (real)          | 23.073s              | 14.971         | -35.1%
*: v4.20-rc1 + delayed subtree scan patchset
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/delayed-ref.c | 3 ++-
 fs/btrfs/delayed-ref.h | 1 +
 fs/btrfs/extent-tree.c | 6 +++---
 3 files changed, 6 insertions(+), 4 deletions(-)

Quoted 15 lines

fs/btrfs/delayed-ref.c int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans, · line 801
Quoted 9 lines
fs/btrfs/delayed-ref.c +1 -0
index 9301b3ad9217..269bd6ecb8f3 100644
@@ -798,6 +798,7 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,
  * add a delayed data ref. it's similar to btrfs_add_delayed_tree_ref.
  */
 int btrfs_add_delayed_data_ref(struct btrfs_trans_handle *trans,
+			       struct btrfs_root *root,
I'm beginning to wonder, should we document btrfs_add_delayed_data_ref/btrfs_add_tree_ref arguments separate for each function, or should only the differences be documented - in this case the newly added root parameter. The rest of the arguments are being documented at init_delayed_ref_common.

You won't be happy with my later plan, it will add new parameter for btrfs_add_delayed_tree_ref(), and it may not be @root, but some bool.

So I think we may need to document at least the difference.

Thanks,
Qu

Quoted 57 lines

fs/btrfs/delayed-ref.c +1 -1

 			       u64 bytenr, u64 num_bytes,
 			       u64 parent, u64 ref_root,
 			       u64 owner, u64 offset, u64 reserved, int action,

@@ -835,7 +836,7 @@ int btrfs_add_delayed_data_ref(struct btrfs_trans_handle *trans,
 	}
 
 	if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) &&
-	    is_fstree(ref_root)) {
+	    is_fstree(ref_root) && is_fstree(root->root_key.objectid)) {
 		record = kmalloc(sizeof(*record), GFP_NOFS);
 		if (!record) {
 			kmem_cache_free(btrfs_delayed_data_ref_cachep, ref);

fs/btrfs/delayed-ref.h +1 -0

index 8e20c5cb5404..6c60737e55d6 100644
@@ -240,6 +240,7 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,
 			       struct btrfs_delayed_extent_op *extent_op,
 			       int *old_ref_mod, int *new_ref_mod);
 int btrfs_add_delayed_data_ref(struct btrfs_trans_handle *trans,
+			       struct btrfs_root *root,
 			       u64 bytenr, u64 num_bytes,
 			       u64 parent, u64 ref_root,
 			       u64 owner, u64 offset, u64 reserved, int action,

fs/btrfs/extent-tree.c +3 -3

index a1febf155747..0554d2cc2ea1 100644
@@ -2046,7 +2046,7 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
 						 BTRFS_ADD_DELAYED_REF, NULL,
 						 &old_ref_mod, &new_ref_mod);
 	} else {
-		ret = btrfs_add_delayed_data_ref(trans, bytenr,
+		ret = btrfs_add_delayed_data_ref(trans, root, bytenr,
 						 num_bytes, parent,
 						 root_objectid, owner, offset,
 						 0, BTRFS_ADD_DELAYED_REF,

@@ -7104,7 +7104,7 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans,
 						 BTRFS_DROP_DELAYED_REF, NULL,
 						 &old_ref_mod, &new_ref_mod);
 	} else {
-		ret = btrfs_add_delayed_data_ref(trans, bytenr,
+		ret = btrfs_add_delayed_data_ref(trans, root, bytenr,
 						 num_bytes, parent,
 						 root_objectid, owner, offset,
 						 0, BTRFS_DROP_DELAYED_REF,

@@ -8080,7 +8080,7 @@ int btrfs_alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
 			   root->root_key.objectid, owner, offset,
 			   BTRFS_ADD_DELAYED_EXTENT);
 
-	ret = btrfs_add_delayed_data_ref(trans, ins->objectid,
+	ret = btrfs_add_delayed_data_ref(trans, root, ins->objectid,
 					 ins->offset, 0,
 					 root->root_key.objectid, owner,
 					 offset, ram_bytes,

Nikolay Borisov

2018-11-20 09:11 UTC #3 lore raw

Quoted 3 lines · Qu Wenruo

On 2018/11/20 下午4:51, Nikolay Borisov wrote:

<snip>

Quoted 8 lines

Quoted 5 lines
I'm beginning to wonder, should we document btrfs_add_delayed_data_ref/btrfs_add_tree_ref arguments separate for each function, or should only the differences be documented - in this case the newly added root parameter. The rest of the arguments are being documented at init_delayed_ref_common.
You won't be happy with my later plan, it will add new parameter for btrfs_add_delayed_tree_ref(), and it may not be @root, but some bool.

You are right, but I'm starting to think that the interface of adding those references is wrong because we shouldn't really need adding more and more arguments. All of this feels like piling hack on top of hack for some legacy infrastructure which no one bothers fixing from a high-level.

Quoted 7 lines

So I think we may need to document at least the difference.
Thanks,
Qu
Quoted 1 line

<snip>

Qu Wenruo

2018-11-20 09:17 UTC #4 lore raw

Quoted 13 lines · Nikolay Borisov

Quoted 3 lines · Qu Wenruo
On 2018/11/20 下午4:51, Nikolay Borisov wrote:
<snip>
Quoted 6 lines
Quoted 5 lines
I'm beginning to wonder, should we document btrfs_add_delayed_data_ref/btrfs_add_tree_ref arguments separate for each function, or should only the differences be documented - in this case the newly added root parameter. The rest of the arguments are being documented at init_delayed_ref_common.

Quoted 8 lines

Quoted 2 lines
You won't be happy with my later plan, it will add new parameter for btrfs_add_delayed_tree_ref(), and it may not be @root, but some bool.
You are right, but I'm starting to think that the interface of adding those references is wrong because we shouldn't really need adding more and more arguments. All of this feels like piling hack on top of hack for some legacy infrastructure which no one bothers fixing from a high-level.

Can't agree any more!

But the problem is, such big change on the low level delayed-ref infrastructure could easily cause new bugs, thus there isn't much driving force to change it.

I'm considering to change the longer and longer parameter list into a structure as the first step to do cleanup.
(By the nature of structure and union, some parameters can easily be merged into an union, makes the parameter structure easier to read)

Feel free if you have any better suggestion.
(I also hate the current btrfs_inc_extent_ref() and btrfs_inc_ref() interfaces, but I don't have enough confidence to change them right now)

Thanks,
Qu

Quoted 11 lines

Quoted 9 lines
So I think we may need to document at least the difference.
Thanks,
Qu
Quoted 1 line
<snip>