* [PATCH] btrfs: qgroup: Skip delayed data ref for reloc trees
@ 2018-11-20 8:46 Qu Wenruo
0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2018-11-20 8:46 UTC (permalink / raw)
To: linux-btrfs
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 <wqu@suse.com>
---
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(-)
diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 9301b3ad9217..269bd6ecb8f3 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -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);
diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
index 8e20c5cb5404..6c60737e55d6 100644
--- a/fs/btrfs/delayed-ref.h
+++ b/fs/btrfs/delayed-ref.h
@@ -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,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a1febf155747..0554d2cc2ea1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -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,
--
2.19.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] btrfs: qgroup: Skip delayed data ref for reloc trees
@ 2018-11-20 8:46 Qu Wenruo
2018-11-20 8:51 ` Nikolay Borisov
0 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2018-11-20 8:46 UTC (permalink / raw)
To: linux-btrfs
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 <wqu@suse.com>
---
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(-)
diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 9301b3ad9217..269bd6ecb8f3 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -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);
diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
index 8e20c5cb5404..6c60737e55d6 100644
--- a/fs/btrfs/delayed-ref.h
+++ b/fs/btrfs/delayed-ref.h
@@ -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,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a1febf155747..0554d2cc2ea1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -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,
--
2.19.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: qgroup: Skip delayed data ref for reloc trees
2018-11-20 8:46 Qu Wenruo
@ 2018-11-20 8:51 ` Nikolay Borisov
2018-11-20 9:07 ` Qu Wenruo
0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Borisov @ 2018-11-20 8:51 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
On 20.11.18 г. 10:46 ч., Qu Wenruo wrote:
> 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 <wqu@suse.com>
> ---
> 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(-)
>
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index 9301b3ad9217..269bd6ecb8f3 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -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.
> 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);
> diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
> index 8e20c5cb5404..6c60737e55d6 100644
> --- a/fs/btrfs/delayed-ref.h
> +++ b/fs/btrfs/delayed-ref.h
> @@ -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,
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index a1febf155747..0554d2cc2ea1 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: qgroup: Skip delayed data ref for reloc trees
2018-11-20 8:51 ` Nikolay Borisov
@ 2018-11-20 9:07 ` Qu Wenruo
2018-11-20 9:11 ` Nikolay Borisov
0 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2018-11-20 9:07 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 2018/11/20 下午4:51, Nikolay Borisov wrote:
>
>
> On 20.11.18 г. 10:46 ч., Qu Wenruo wrote:
>> 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 <wqu@suse.com>
>> ---
>> 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(-)
>>
>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>> index 9301b3ad9217..269bd6ecb8f3 100644
>> --- a/fs/btrfs/delayed-ref.c
>> +++ b/fs/btrfs/delayed-ref.c
>> @@ -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
>
>> 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);
>> diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
>> index 8e20c5cb5404..6c60737e55d6 100644
>> --- a/fs/btrfs/delayed-ref.h
>> +++ b/fs/btrfs/delayed-ref.h
>> @@ -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,
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index a1febf155747..0554d2cc2ea1 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -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,
>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: qgroup: Skip delayed data ref for reloc trees
2018-11-20 9:07 ` Qu Wenruo
@ 2018-11-20 9:11 ` Nikolay Borisov
2018-11-20 9:17 ` Qu Wenruo
0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Borisov @ 2018-11-20 9:11 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
On 20.11.18 г. 11:07 ч., Qu Wenruo wrote:
>
>
> On 2018/11/20 下午4:51, Nikolay Borisov wrote:
<snip>
>> 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.
>
> So I think we may need to document at least the difference.
>
> Thanks,
> Qu
>
>>
<snip>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: qgroup: Skip delayed data ref for reloc trees
2018-11-20 9:11 ` Nikolay Borisov
@ 2018-11-20 9:17 ` Qu Wenruo
0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2018-11-20 9:17 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 2018/11/20 下午5:11, Nikolay Borisov wrote:
>
>
> On 20.11.18 г. 11:07 ч., Qu Wenruo wrote:
>>
>>
>> On 2018/11/20 下午4:51, Nikolay Borisov wrote:
> <snip>
>>> 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.
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
>
>
>>
>> So I think we may need to document at least the difference.
>>
>> Thanks,
>> Qu
>>
>>>
> <snip>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-20 9:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-20 8:46 [PATCH] btrfs: qgroup: Skip delayed data ref for reloc trees Qu Wenruo
-- strict thread matches above, loose matches on Subject: below --
2018-11-20 8:46 Qu Wenruo
2018-11-20 8:51 ` Nikolay Borisov
2018-11-20 9:07 ` Qu Wenruo
2018-11-20 9:11 ` Nikolay Borisov
2018-11-20 9:17 ` Qu Wenruo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).