btrfs: Refactor delayed ref parameter list
cover btrfs: Refactor delayed ref parameter list
Current delayed ref interface has several problems:
- Longer and longer parameter lists
bytenr
num_bytes
parent
---- So far so good
ref_root
owner
offset
---- I don't feel well now
for_reloc
^^^^ This parameter only makes sense for qgroup code, but we need
to pass the parameter a long way.
This makes later expand on parameter list more and more tricky.
- Different interpretation for the same parameter
Above @owner for data ref is ino who owns this extent, while for tree ref, it's level. They are even in different size range.
For level we only need 0~8, while for ino it's BTRFS_FIRST_FREE_OBJECTID~BTRFS_LAST_FREE_OBJECTID, so it's still possible to distinguish them, but it's never a straight-forward thing to grasp.
And @offset doesn't even makes sense for tree ref.
Such parameter reuse may look clever as an hidden union, but it destroys code readability.
This patchset will change the way how we pass parameters for delayed ref.
Instead of calling delayed ref interface like:
ret = btrfs_inc_extent_ref(trans, root, bytenr, num_bytes, parent,
ref_root, owner, offset); Or ret = btrfs_inc_extent_ref(trans, root, bytenr, nodesize, parent, level, ref_root, 0);
We now call like:
btrfs_init_generic_ref(&ref, bytenr, num_bytes,
root->root_key.objectid, parent);
btrfs_init_data_ref(&ref, ref_root, owner, offset); ret = btrfs_inc_extent_ref(trans, &ref);
Or
btrfs_init_generic_ref(&ref, bytenr, num_bytes,
root->root_key.objectid, parent);
btrfs_init_tree_ref(&ref, level, ref_root);
ret = btrfs_inc_extent_ref(trans, &ref);
To determine if a ref is tree or data, instead of calling like:
if (owner < BTRFS_FIRST_FREE_OBJECTID) {
} else {
}
We do it straight-forward:
if (ref->type == BTRFS_REF_METADATA) {
} else {
}
And for newer and minor new members, we don't need to add a new parameter to btrfs_add_delayed_tree|data_ref() or btrfs_inc_extent_ref(), just assign them after generic/data/tree init:
btrfs_init_generic_ref(&ref, bytenr, num_bytes,
root->root_key.objectid, parent);
btrfs_init_data_ref(&ref, ref_root, owner, offset); ref->skip_qgroup = true; /* @skip_qgroup is default to false, so new
code doesn't need to care */
ret = btrfs_inc_extent_ref(trans, &ref);
This should improve the code readability and make later code easier to write.
Qu Wenruo (8):
btrfs: delayed-ref: Introduce better documented delayed ref structures
btrfs: extent-tree: Open-code process_func in __btrfs_mod_ref
btrfs: delayed-ref: Use btrfs_ref to refactor
btrfs_add_delayed_tree_ref()
btrfs: delayed-ref: Use btrfs_ref to refactor
btrfs_add_delayed_data_ref()
btrfs: ref-verify: Use btrfs_ref to refactor btrfs_ref_tree_mod()
btrfs: extent-tree: Use btrfs_ref to refactor add_pinned_bytes()
btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref()
btrfs: extent-tree: Use btrfs_ref to refactor btrfs_free_extent()
fs/btrfs/ctree.h | 11 +-- fs/btrfs/delayed-ref.c | 43 ++++++--- fs/btrfs/delayed-ref.h | 121 +++++++++++++++++++++++-- fs/btrfs/extent-tree.c | 195 +++++++++++++++++++---------------------- fs/btrfs/file.c | 43 +++++---- fs/btrfs/inode.c | 23 +++-- fs/btrfs/ioctl.c | 17 ++-- fs/btrfs/ref-verify.c | 53 ++++++----- fs/btrfs/ref-verify.h | 10 +-- fs/btrfs/relocation.c | 70 +++++++++------ fs/btrfs/tree-log.c | 12 ++- 11 files changed, 375 insertions(+), 223 deletions(-)
Signature · 1 line
2.19.2
PATCH 1/8 btrfs: delayed-ref: Introduce better documented delayed ref structures
Current delayed ref interface has several problems:
- Longer and longer parameter lists
bytenr
num_bytes
parent
ref_root
owner
offset
for_reloc << Only qgroup code cares.
- Different interpretation for the same parameter
Above @owner for data ref is ino owning this extent,
while for tree ref, it's level. They are even in different size range. For level we only need 0~8, while for ino it's BTRFS_FIRST_FREE_OBJECTID~BTRFS_LAST_FREE_OBJECTID.
And @offset doesn't even makes sense for tree ref.
Such parameter reuse may look clever as an hidden union, but it destroys code readability.
To solve both problems, we introduce a new structure, btrfs_ref to solve them:
- Structure instead of long parameter list
This makes later expansion easier, and better documented.
- Use btrfs_ref::type to distinguish data and tree ref
- Use proper union to store data/tree ref specific structures.
- Use separate functions to fill data/tree ref data, with a common generic function to fill common bytenr/num_bytes members.
All parameters will find its place in btrfs_ref, and an extra member, real_root, inspired by ref-verify code, is newly introduced for later qgroup code, to record which tree is triggered this extent modification.
This patch doesn't touch any code, but provides the basis for incoming refactors.
--- fs/btrfs/delayed-ref.h | 109 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+)
fs/btrfs/delayed-ref.h +109 -0
@@ -176,6 +176,81 @@ struct btrfs_delayed_ref_root {u64 qgroup_to_skip; }; +enum btrfs_ref_type { +BTRFS_REF_NOT_SET =0 , +BTRFS_REF_DATA , +BTRFS_REF_METADATA , +BTRFS_REF_LAST , +}; + +struct btrfs_data_ref { +/* +* For EXTENT_DATA_REF +* +* @ref_root: current owner of the extent. +* may differ from btrfs_ref::real_root. +* @ino: inode number of the owner. +* @offset: *CALCULATED* offset. Not EXTENT_DATA key offset. +* +*/ +u64 ref_root; +u64 ino; +u64 offset; +}; + +struct btrfs_tree_ref { +/* Common for all sub types and skinny combination */ +int level; + +/* +* For TREE_BLOCK_REF (skinny metadata, either inline or keyed) +* +* root here may differ from btrfs_ref::real_root. +*/ +u64 root; + +/* For non-skinny metadata, no special member needed */ +}; + +struct btrfs_ref { +enum btrfs_ref_type type; +int action; + +/* +* Use full backref(SHARED_BLOCK_REF or SHARED_DATA_REF) for this +* extent and its children. +* Set for reloc trees. +*/ +unsigned int use_fullback:1 ; + +/* +* Whether this extent should go through qgroup record. +* Normally false, but for certain case like delayed subtree scan, +* this can hugely reduce qgroup overhead. +*/ +unsigned int skip_qgroup:1 ; + +/* +* Who owns this reference modification, optional. +* +* One example: +* When creating reloc tree for source fs, it will increase tree block +* ref for children tree blocks. +* In that case, btrfs_ref::real_root = reloc tree, +* while btrfs_ref::tree_ref::root = fs tree. +*/ +u64 real_root; +u64 bytenr; +u64 len; + +/* Common @parent for SHARED_DATA_REF/SHARED_BLOCK_REF */ +u64 parent; +union { +struct btrfs_data_ref data_ref; +struct btrfs_tree_ref tree_ref; + }; +}; +extern struct kmem_cache *btrfs_delayed_ref_head_cachep;extern struct kmem_cache *btrfs_delayed_tree_ref_cachep;extern struct kmem_cache *btrfs_delayed_data_ref_cachep;
@@ -184,6 +259,40 @@ extern struct kmem_cache *btrfs_delayed_extent_op_cachep;int __init btrfs_delayed_ref_init (void );void __cold btrfs_delayed_ref_exit (void ); +static inline void btrfs_init_generic_ref (struct btrfs_ref *generic_ref, +int action,u64 bytenr,u64 len,u64 real_root, +u64 parent) +{ + generic_ref->action = action; + generic_ref->bytenr = bytenr; + generic_ref->len = len; + generic_ref->real_root = real_root; + generic_ref->parent = parent; +} + +static inline void btrfs_init_tree_ref (struct btrfs_ref *generic_ref, +int level,u64 root) +{ +/* If @real_root not set, use @root as fallback */ +if (!generic_ref->real_root) + generic_ref->real_root = root; + generic_ref->tree_ref.level = level; + generic_ref->tree_ref.root = root; + generic_ref->type =BTRFS_REF_METADATA ; +} + +static inline void btrfs_init_data_ref (struct btrfs_ref *generic_ref, +u64 ref_root,u64 ino,u64 offset) +{ +/* If @real_root not set, use @root as fallback */ +if (!generic_ref->real_root) + generic_ref->real_root = ref_root; + generic_ref->data_ref.ref_root = ref_root; + generic_ref->data_ref.ino = ino; + generic_ref->data_ref.offset = offset; + generic_ref->type =BTRFS_REF_DATA ; +} +static inline struct btrfs_delayed_extent_op *btrfs_alloc_delayed_extent_op (void ) {
Signature · 1 line
2.19.2
Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures
Quoted 51 lines · Qu Wenruo
Current delayed ref interface has several problems:
- Longer and longer parameter lists
bytenr
num_bytes
parent
ref_root
owner
offset
for_reloc<< Only qgroup code cares.- Different interpretation for the same parameter
Above @owner for data ref is ino owning this extent,
while for tree ref, it's level. They are even in different size range. For level we only need 0~8, while for ino it's BTRFS_FIRST_FREE_OBJECTID~BTRFS_LAST_FREE_OBJECTID.And @offset doesn't even makes sense for tree ref.
Such parameter reuse may look clever as an hidden union, but it destroys code readability.
To solve both problems, we introduce a new structure,
btrfs_refto solve them:- Structure instead of long parameter list
This makes later expansion easier, and better documented.- Use
btrfs_ref::typeto distinguish data and tree ref- Use proper union to store data/tree ref specific structures.
- Use separate functions to fill data/tree ref data, with a common generic function to fill common bytenr/num_bytes members.
All parameters will find its place in
btrfs_ref, and an extra member,real_root, inspired by ref-verify code, is newly introduced for later qgroup code, to record which tree is triggered this extent modification.This patch doesn't touch any code, but provides the basis for incoming refactors.
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/delayed-ref.h | 109 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+)fs/btrfs/delayed-ref.h +0
-0
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 180
Quoted 5 lines
fs/btrfs/delayed-ref.h +2
-0@@ -176,6 +176,81 @@ struct btrfs_delayed_ref_root {u64 qgroup_to_skip; }; +enum btrfs_ref_type { +BTRFS_REF_NOT_SET =0 ,
No need for explicit initialisation when your enums start from 0.
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 191
Quoted 11 lines
fs/btrfs/delayed-ref.h +11
-0+BTRFS_REF_DATA , +BTRFS_REF_METADATA , + BTRFS_REF_LAST, +}; + +struct btrfs_data_ref { +/* +* For EXTENT_DATA_REF +* +* @ref_root: current owner of the extent. +* may differ from btrfs_ref::real_root.
Here 'owner' is a bit ambiguous. Isn't this the root of the owner
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 192
Quoted 1 line
fs/btrfs/delayed-ref.h +1
-0+* @ino: inode number of the owner.
And the owner is really the owning inode?
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 193
Quoted 1 line
fs/btrfs/delayed-ref.h +1
-0+* @offset: *CALCULATED* offset. Not EXTENT_DATA key offset.
What does calculated mean here?
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 195
Quoted 2 lines
fs/btrfs/delayed-ref.h +2
-0+* +*/
Ugh, this is ugly, why not have a single /* */ line above each member and document them like that?
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 203
Quoted 8 lines
fs/btrfs/delayed-ref.h +8
-0+u64 ref_root; +u64 ino; +u64 offset; +}; + +struct btrfs_tree_ref { +/* Common for all sub types and skinny combination */ +int level;
Like you've done here. Mention that this is the level in the tree that this reference is located at.
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 209
Quoted 6 lines
fs/btrfs/delayed-ref.h +6
-0+ +/* +* For TREE_BLOCK_REF (skinny metadata, either inline or keyed) +* +* root here may differ from btrfs_ref::real_root. +*/
Make it refer to the documentation of the generic btrfs_ref structure because if someone reads this comment and doesn't read the one in btrfs_ref then it's not clear under what conditions they might differ.
Quoted 5 lines
fs/btrfs/delayed-ref.h +5
-0+u64 root; + +/* For non-skinny metadata, no special member needed */ +}; +
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 224
Quoted 10 lines
fs/btrfs/delayed-ref.h +10
-0+struct btrfs_ref { +enum btrfs_ref_type type; +int action; + +/* +* Use full backref(SHARED_BLOCK_REF or SHARED_DATA_REF) for this +* extent and its children. +* Set for reloc trees. +*/ +unsigned int use_fullback:1 ;
'fullback' is too terse, use_backreferences or something conveying more information? Also please use explicit bool type:
bool xxxx:1
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 231
Quoted 7 lines
fs/btrfs/delayed-ref.h +7
-0+ +/* +* Whether this extent should go through qgroup record. +* Normally false, but for certain case like delayed subtree scan, +* this can hugely reduce qgroup overhead. +*/ + unsigned int skip_qgroup:1;
again, explicit bool type please.
Quoted 5 lines
fs/btrfs/delayed-ref.h +5
-0+ +/* +* Who owns this reference modification, optional. +* +* One example:
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 246
Quoted 10 lines
fs/btrfs/delayed-ref.h +10
-0+* When creating reloc tree for source fs, it will increase tree block +* ref for children tree blocks. +* In that case, btrfs_ref::real_root = reloc tree, +* while btrfs_ref::tree_ref::root = fs tree. +*/ +u64 real_root; +u64 bytenr; +u64 len; + +/* Common @parent for SHARED_DATA_REF/SHARED_BLOCK_REF */
OK, it's common but what does it really contain? Either document it or rename it to parent_block or something.
Quoted 11 lines
fs/btrfs/delayed-ref.h +7
-0+u64 parent; +union { +struct btrfs_data_ref data_ref; +struct btrfs_tree_ref tree_ref; + }; +}; +extern struct kmem_cache *btrfs_delayed_ref_head_cachep;extern struct kmem_cache *btrfs_delayed_tree_ref_cachep;extern struct kmem_cache *btrfs_delayed_data_ref_cachep;
fs/btrfs/delayed-ref.h extern struct kmem_cache *btrfs_delayed_extent_op_cachep; · line 266
Quoted 8 lines
fs/btrfs/delayed-ref.h +5
-0@@ -184,6 +259,40 @@ extern struct kmem_cache *btrfs_delayed_extent_op_cachep;int __init btrfs_delayed_ref_init (void );void __cold btrfs_delayed_ref_exit (void ); +static inline void btrfs_init_generic_ref (struct btrfs_ref *generic_ref, +int action,u64 bytenr,u64 len,u64 real_root, +u64 parent) +{ + generic_ref->action = action;
IMO it makes sense in this series to have a patch which converts the action defines to an enum and subsequently modify functions/structs to actually be of enum type.
Quoted 33 lines
fs/btrfs/delayed-ref.h +29
-0+ generic_ref->bytenr = bytenr; + generic_ref->len = len; + generic_ref->real_root = real_root; + generic_ref->parent = parent; +} + +static inline void btrfs_init_tree_ref (struct btrfs_ref *generic_ref, +int level,u64 root) +{ +/* If @real_root not set, use @root as fallback */ +if (!generic_ref->real_root) + generic_ref->real_root = root; + generic_ref->tree_ref.level = level; + generic_ref->tree_ref.root = root; + generic_ref->type =BTRFS_REF_METADATA ; +} + +static inline void btrfs_init_data_ref (struct btrfs_ref *generic_ref, +u64 ref_root,u64 ino,u64 offset) +{ +/* If @real_root not set, use @root as fallback */ +if (!generic_ref->real_root) + generic_ref->real_root = ref_root; + generic_ref->data_ref.ref_root = ref_root; + generic_ref->data_ref.ino = ino; + generic_ref->data_ref.offset = offset; + generic_ref->type =BTRFS_REF_DATA ; +} +static inline struct btrfs_delayed_extent_op *btrfs_alloc_delayed_extent_op (void ) {
Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures
Quoted 57 lines · Nikolay Borisov
Quoted 54 lines · Qu Wenruo
Current delayed ref interface has several problems:
- Longer and longer parameter lists
bytenr
num_bytes
parent
ref_root
owner
offset
for_reloc<< Only qgroup code cares.- Different interpretation for the same parameter
Above @owner for data ref is ino owning this extent,
while for tree ref, it's level. They are even in different size range. For level we only need 0~8, while for ino it's BTRFS_FIRST_FREE_OBJECTID~BTRFS_LAST_FREE_OBJECTID.And @offset doesn't even makes sense for tree ref.
Such parameter reuse may look clever as an hidden union, but it destroys code readability.
To solve both problems, we introduce a new structure,
btrfs_refto solve them:- Structure instead of long parameter list
This makes later expansion easier, and better documented.- Use
btrfs_ref::typeto distinguish data and tree ref- Use proper union to store data/tree ref specific structures.
- Use separate functions to fill data/tree ref data, with a common generic function to fill common bytenr/num_bytes members.
All parameters will find its place in
btrfs_ref, and an extra member,real_root, inspired by ref-verify code, is newly introduced for later qgroup code, to record which tree is triggered this extent modification.This patch doesn't touch any code, but provides the basis for incoming refactors.
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/delayed-ref.h | 109 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+)fs/btrfs/delayed-ref.h +0
-0@@ -176,6 +176,81 @@ struct btrfs_delayed_ref_root {u64 qgroup_to_skip; };
Quoted 4 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 180
Quoted 2 lines
fs/btrfs/delayed-ref.h +2
-0+enum btrfs_ref_type { +BTRFS_REF_NOT_SET =0 ,No need for explicit initialisation when your enums start from 0.
Forgot that.
Quoted 14 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 191
Quoted 12 lines
fs/btrfs/delayed-ref.h +11
-0+BTRFS_REF_DATA , +BTRFS_REF_METADATA , + BTRFS_REF_LAST, +}; + +struct btrfs_data_ref { +/* +* For EXTENT_DATA_REF +* +* @ref_root: current owner of the extent. +* may differ from btrfs_ref::real_root. Here 'owner' is a bit ambiguous. Isn't this the root of the owner
Well, the @ref_root is a little confusing.
Here it could mean "the current btrfs_header_owner(eb)".
I don't really know the correct term here.
Quoted 4 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 192
Quoted 2 lines
fs/btrfs/delayed-ref.h +1
-0+* @ino: inode number of the owner. And the owner is really the owning inode?
Yes, the owning inode.
Quoted 4 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 193
Quoted 2 lines
fs/btrfs/delayed-ref.h +1
-0+* @offset: *CALCULATED* offset. Not EXTENT_DATA key offset. What does calculated mean here?
Because btrfs data backref instead of using the offset of the owning EXTENT_DATA key, it uses key->offset - extent_offset.
Quoted 6 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 195
Quoted 3 lines
fs/btrfs/delayed-ref.h +2
-0+* +*/ Ugh, this is ugly, why not have a single /* */ line above each member and document them like that?
Because the complexity of @ref_root can't really go one line.
Quoted 34 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 203
Quoted 9 lines
fs/btrfs/delayed-ref.h +8
-0+u64 ref_root; +u64 ino; +u64 offset; +}; + +struct btrfs_tree_ref { +/* Common for all sub types and skinny combination */ +int level;Like you've done here. Mention that this is the level in the tree that this reference is located at.
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 209
Quoted 6 lines
fs/btrfs/delayed-ref.h +6
-0+ +/* +* For TREE_BLOCK_REF (skinny metadata, either inline or keyed) +* +* root here may differ from btrfs_ref::real_root. +*/ Make it refer to the documentation of the generic
btrfs_refstructure because if someone reads this comment and doesn't read the one inbtrfs_refthen it's not clear under what conditions they might differ.Quoted 10 lines
fs/btrfs/delayed-ref.h +10
-0+u64 root; + +/* For non-skinny metadata, no special member needed */ +}; + +struct btrfs_ref { +enum btrfs_ref_type type; +int action; + +/*
Quoted 8 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 224
Quoted 5 lines
fs/btrfs/delayed-ref.h +5
-0+* Use full backref(SHARED_BLOCK_REF or SHARED_DATA_REF) for this +* extent and its children. +* Set for reloc trees. +*/ + unsigned int use_fullback:1;'fullback' is too terse,
use_backreferencesor something conveying more information?
@use_backreferences looks even stranger to me.
Here the point is, if this bit set, all backref will use SHARED_BLOCK_REF or SHARED_DATA_REF subtype, and just leave a pointer to its parent.
Any good idea for explaining this?
Quoted 3 lines
Also please use explicit bool type:
bool xxxx:1
Is this valid? Haven't seen such usage in kernel code IIRC.
Quoted 44 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 231
Quoted 8 lines
fs/btrfs/delayed-ref.h +7
-0+ +/* +* Whether this extent should go through qgroup record. +* Normally false, but for certain case like delayed subtree scan, +* this can hugely reduce qgroup overhead. +*/ + unsigned int skip_qgroup:1;again, explicit bool type please.
Quoted 5 lines
fs/btrfs/delayed-ref.h +5
-0+ +/* +* Who owns this reference modification, optional. +* +* One example: fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 246
Quoted 10 lines
fs/btrfs/delayed-ref.h +10
-0+* When creating reloc tree for source fs, it will increase tree block +* ref for children tree blocks. +* In that case, btrfs_ref::real_root = reloc tree, +* while btrfs_ref::tree_ref::root = fs tree. +*/ +u64 real_root; +u64 bytenr; +u64 len; + +/* Common @parent for SHARED_DATA_REF/SHARED_BLOCK_REF */ OK, it's common but what does it really contain? Either document it or rename it to
parent_blockor something.Quoted 14 lines
fs/btrfs/delayed-ref.h +7
-0+u64 parent; +union { +struct btrfs_data_ref data_ref; +struct btrfs_tree_ref tree_ref; + }; +}; +extern struct kmem_cache *btrfs_delayed_ref_head_cachep;extern struct kmem_cache *btrfs_delayed_tree_ref_cachep;extern struct kmem_cache *btrfs_delayed_data_ref_cachep;@@ -184,6 +259,40 @@ extern struct kmem_cache *btrfs_delayed_extent_op_cachep;int __init btrfs_delayed_ref_init (void );void __cold btrfs_delayed_ref_exit (void );
Quoted 9 lines
fs/btrfs/delayed-ref.h extern struct kmem_cache *btrfs_delayed_extent_op_cachep; · line 266
Quoted 5 lines
fs/btrfs/delayed-ref.h +5
-0+static inline void btrfs_init_generic_ref (struct btrfs_ref *generic_ref, +int action,u64 bytenr,u64 len,u64 real_root, +u64 parent) +{ + generic_ref->action = action;IMO it makes sense in this series to have a patch which converts the action defines to an enum and subsequently modify functions/structs to actually be of enum type.
Makes sense.
Thanks,
Qu
Quoted 34 lines
Quoted 34 lines
fs/btrfs/delayed-ref.h +29
-0+ generic_ref->bytenr = bytenr; + generic_ref->len = len; + generic_ref->real_root = real_root; + generic_ref->parent = parent; +} + +static inline void btrfs_init_tree_ref (struct btrfs_ref *generic_ref, +int level,u64 root) +{ +/* If @real_root not set, use @root as fallback */ +if (!generic_ref->real_root) + generic_ref->real_root = root; + generic_ref->tree_ref.level = level; + generic_ref->tree_ref.root = root; + generic_ref->type =BTRFS_REF_METADATA ; +} + +static inline void btrfs_init_data_ref (struct btrfs_ref *generic_ref, +u64 ref_root,u64 ino,u64 offset) +{ +/* If @real_root not set, use @root as fallback */ +if (!generic_ref->real_root) + generic_ref->real_root = ref_root; + generic_ref->data_ref.ref_root = ref_root; + generic_ref->data_ref.ino = ino; + generic_ref->data_ref.offset = offset; + generic_ref->type =BTRFS_REF_DATA ; +} +static inline struct btrfs_delayed_extent_op *btrfs_alloc_delayed_extent_op (void ) {
Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures
Quoted 16 lines · Qu Wenruo
Quoted 13 lines · Nikolay Borisov
Quoted 10 lines · Qu Wenruo
Current delayed ref interface has several problems:
- Longer and longer parameter lists
bytenr
num_bytes
parent
ref_root
owner
offset
for_reloc<< Only qgroup code cares.
<snip>
Quoted 9 lines
Quoted 8 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 191
Quoted 6 lines
fs/btrfs/delayed-ref.h +6
-0+struct btrfs_data_ref { +/* +* For EXTENT_DATA_REF +* +* @ref_root: current owner of the extent. +* may differ from btrfs_ref::real_root. Here 'owner' is a bit ambiguous. Isn't this the root of the owner
Quoted 4 lines
Well, the @ref_root is a little confusing.
Here it could mean "the currentbtrfs_header_owner(eb)".I don't really know the correct term here.
My point was that the word owner is being overloaded. I guess the whole sentence can be rephrased as:
"The root this reference originates from. For cases where ref_root differs from btrfs_ref::real_root consult btrfs_ref comments". Or something along those lines.
Quoted 9 lines
Quoted 5 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 192
Quoted 3 lines
fs/btrfs/delayed-ref.h +1
-0+* @ino: inode number of the owner. And the owner is really the owning inode?
Yes, the owning inode.
Quoted 1 line
Quoted 6 lines
Quoted 3 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 193
Quoted 1 line
fs/btrfs/delayed-ref.h +1
-0+* @offset: *CALCULATED* offset. Not EXTENT_DATA key offset. What does calculated mean here?
Because btrfs data backref instead of using the offset of the owning
EXTENT_DATAkey, it useskey->offset-extent_offset.
There's no way anyone reading: *CALCULATED* offset could infer that. Please reword. Even this sentence is a bit terse. When writing documentation assume people who are going to read it will have *very* little knowledge of the internal structure of the code.
Quoted 9 lines
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +2
-0+ * + */Ugh, this is ugly, why not have a single /* */ line above each member and document them like that?
Because the complexity of @ref_root can't really go one line.
What I meant is to have the documentation for every member right above the respective member, rather than having the documentation in one place, followed by the variables. If it takes more than a single line to document a member so be it.
<snip>
Quoted 10 lines
Quoted 9 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 224
Quoted 6 lines
fs/btrfs/delayed-ref.h +6
-0+/* +* Use full backref(SHARED_BLOCK_REF or SHARED_DATA_REF) for this +* extent and its children. +* Set for reloc trees. +*/ + unsigned int use_fullback:1;'fullback' is too terse,
use_backreferencesor something conveying more information?
Quoted 7 lines
@use_backreferences looks even stranger to me.
Here the point is, if this bit set, all backref will use
SHARED_BLOCK_REForSHARED_DATA_REFsubtype, and just leave a pointer to its parent.Any good idea for explaining this?
What's the alternative if this bit is not set, how would the tree look like?
Quoted 6 lines
Quoted 4 lines
Also please use explicit bool type:
bool xxxx:1
Is this valid? Haven't seen such usage in kernel code IIRC.
git grep 'bool .*:1' | wc -l
417
<snip>
Quoted 1 line
Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures
Quoted 72 lines · Nikolay Borisov
Quoted 16 lines · Qu Wenruo
Quoted 13 lines · Nikolay Borisov
Quoted 10 lines · Qu Wenruo
Current delayed ref interface has several problems:
- Longer and longer parameter lists
bytenr
num_bytes
parent
ref_root
owner
offset
for_reloc<< Only qgroup code cares.<snip>
Quoted 9 lines
Quoted 8 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 191
Quoted 6 lines
fs/btrfs/delayed-ref.h +6
-0+struct btrfs_data_ref { +/* +* For EXTENT_DATA_REF +* +* @ref_root: current owner of the extent. +* may differ from btrfs_ref::real_root. Here 'owner' is a bit ambiguous. Isn't this the root of the owner
Quoted 4 lines
Well, the @ref_root is a little confusing.
Here it could mean "the currentbtrfs_header_owner(eb)".I don't really know the correct term here.
My point was that the word owner is being overloaded. I guess the whole sentence can be rephrased as:
"The root this reference originates from. For cases where
ref_rootdiffers frombtrfs_ref::real_rootconsultbtrfs_refcomments". Or something along those lines.Quoted 9 lines
Quoted 5 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 192
Quoted 3 lines
fs/btrfs/delayed-ref.h +1
-0+* @ino: inode number of the owner. And the owner is really the owning inode?
Yes, the owning inode.
Quoted 1 line
Quoted 6 lines
Quoted 3 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 193
Quoted 1 line
fs/btrfs/delayed-ref.h +1
-0+* @offset: *CALCULATED* offset. Not EXTENT_DATA key offset. What does calculated mean here?
Because btrfs data backref instead of using the offset of the owning
EXTENT_DATAkey, it useskey->offset-extent_offset.There's no way anyone reading: *CALCULATED* offset could infer that. Please reword. Even this sentence is a bit terse. When writing documentation assume people who are going to read it will have *very* little knowledge of the internal structure of the code.
Quoted 8 lines
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +2
-0+ * + */Ugh, this is ugly, why not have a single /* */ line above each member and document them like that?
Quoted 6 lines
Quoted 1 line
Because the complexity of @ref_root can't really go one line.
What I meant is to have the documentation for every member right above the respective member, rather than having the documentation in one place, followed by the variables. If it takes more than a single line to document a member so be it.
Understood.
Quoted 15 lines
<snip>
Quoted 12 lines
Quoted 9 lines
fs/btrfs/delayed-ref.h struct btrfs_delayed_ref_root { · line 224
Quoted 6 lines
fs/btrfs/delayed-ref.h +6
-0+/* +* Use full backref(SHARED_BLOCK_REF or SHARED_DATA_REF) for this +* extent and its children. +* Set for reloc trees. +*/ + unsigned int use_fullback:1;'fullback' is too terse,
use_backreferencesor something conveying more information?@use_backreferences looks even stranger to me.
Quoted 7 lines
Quoted 5 lines
Here the point is, if this bit set, all backref will use
SHARED_BLOCK_REForSHARED_DATA_REFsubtype, and just leave a pointer to its parent.Any good idea for explaining this?
What's the alternative if this bit is not set, how would the tree look like?
If not set and @parent is 0, it will goes normal backref like EXTENT_DATA_REF (root, ino, offset), or TREE_BLOCK_REF (root, level).
If not set and @parent is not 0, it will go SHARED_BLOCK_REF or SHARED_DATA_REF (parent).
Quoted 11 lines
Quoted 8 lines
Quoted 6 lines
Also please use explicit bool type:
bool xxxx:1
Is this valid? Haven't seen such usage in kernel code IIRC.
git grep 'bool .*:1' | wc -l
417
grep -IR 'bool .*:1' fs/btrfs/ | wc -l
0
So I guess another cleanup?
Thanks,
Qu
Quoted 3 lines
<snip>
Quoted 1 line
Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures
Quoted 6 lines · Qu Wenruo
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines
Also please use explicit bool type:
bool xxxx:1
Is this valid? Haven't seen such usage in kernel code IIRC.
Quoted 7 lines
Quoted 2 lines
git grep 'bool .*:1' | wc -l
417grep -IR 'bool .*:1'
fs/btrfs/| wc -l
0So I guess another cleanup?
No bool bitfields unless there's a space saved in the structure, please. If there are less than 4 bool types in a structure, it's same as 4 bits in u32 and is ok. The bit manipulation needs more instructions, checking zero/non-zero of a byte is typically cheaper.
Looking at the structure, the 'int action' can be reduced and the hole to the next u64 has enough bytes to squeeze a number of bools if needed.
About enum convert (Old "Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures")
In reply to Nikolay BorisovQuoted 2 lines · Nikolay Borisov
[snip]
Quoted 5 lines
IMO it makes sense in this series to have a patch which converts the action defines to an enum and subsequently modify functions/structs to actually be of enum type.
I'm completely OK to convert it to enum, for its conflict free nature.
However I'm not sure to which degree we should go.
For current David's enum cleanup, we all go anonymous enum, which just acts as an auto-increasing #define.
However AFAIK compiler can further compress the size of named enum if needed, and it provides much better type check to prevent us to do stupid type convert.
I'm wondering which way should we go. The named enum (which needs to modify all parameters) or anonymous enum for less code modification?
Thanks,
Qu
PATCH 2/8 btrfs: extent-tree: Open-code process_func in __btrfs_mod_ref
In reply to Qu WenruoThe process_func is never a function hook used anywhere else.
Open code it to make later delayed ref refactor easier, so we can refactor btrfs_inc_extent_ref() and btrfs_free_extent() in different patches.
--- fs/btrfs/extent-tree.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-)
fs/btrfs/extent-tree.c +18 -15
@@ -3220,10 +3220,6 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,int i;int level;int ret =0 ; -int (*process_func)(struct btrfs_trans_handle *, -struct btrfs_root *, -u64 ,u64 ,u64 ,u64 ,u64 ,u64 ,bool ); -if (btrfs_is_testing (fs_info))return 0 ;
@@ -3235,11 +3231,6 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,if (!test_bit (BTRFS_ROOT_REF_COWS , &root->state) && level ==0 )return 0 ; -if (inc) - process_func = btrfs_inc_extent_ref; -else - process_func = btrfs_free_extent; -if (full_backref) parent = buf->start;else
@@ -3261,17 +3252,29 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, num_bytes =btrfs_file_extent_disk_num_bytes (buf, fi); key.offset -=btrfs_file_extent_offset (buf, fi); - ret =process_func (trans, root, bytenr, num_bytes, - parent, ref_root, key.objectid, - key.offset, for_reloc); +if (inc) + ret =btrfs_inc_extent_ref (trans, root, bytenr, + num_bytes, parent, ref_root, + key.objectid, key.offset, + for_reloc); +else + ret =btrfs_free_extent (trans, root, bytenr, + num_bytes, parent, ref_root, + key.objectid, key.offset, + for_reloc);if (ret) goto fail; }else { bytenr =btrfs_node_blockptr (buf, i); num_bytes = fs_info->nodesize; - ret =process_func (trans, root, bytenr, num_bytes, - parent, ref_root, level -1 ,0 , - for_reloc); +if (inc) + ret =btrfs_inc_extent_ref (trans, root, bytenr, + num_bytes, parent, ref_root, + level -1 ,0 , for_reloc); +else + ret =btrfs_free_extent (trans, root, bytenr, + num_bytes, parent, ref_root, + level -1 ,0 , for_reloc);if (ret) goto fail; }
Signature · 1 line
2.19.2
Re: [PATCH 2/8] btrfs: extent-tree: Open-code process_func in __btrfs_mod_ref
Quoted 7 lines · Qu Wenruo
The
process_funcis never a function hook used anywhere else.Open code it to make later delayed ref refactor easier, so we can refactor
btrfs_inc_extent_ref()andbtrfs_free_extent()in different patches.Signed-off-by Qu Wenruo <[email protected]>
Quoted 68 lines
--- fs/btrfs/extent-tree.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-)fs/btrfs/extent-tree.c +18
-15@@ -3220,10 +3220,6 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,int i;int level;int ret =0 ; -int (*process_func)(struct btrfs_trans_handle *, -struct btrfs_root *, -u64 ,u64 ,u64 ,u64 ,u64 ,u64 ,bool ); -if (btrfs_is_testing (fs_info))return 0 ;@@ -3235,11 +3231,6 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,if (!test_bit (BTRFS_ROOT_REF_COWS , &root->state) && level ==0 )return 0 ; -if (inc) - process_func = btrfs_inc_extent_ref; -else - process_func = btrfs_free_extent; -if (full_backref) parent = buf->start;else @@ -3261,17 +3252,29 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, num_bytes =btrfs_file_extent_disk_num_bytes (buf, fi); key.offset -=btrfs_file_extent_offset (buf, fi); - ret =process_func (trans, root, bytenr, num_bytes, - parent, ref_root, key.objectid, - key.offset, for_reloc); +if (inc) + ret =btrfs_inc_extent_ref (trans, root, bytenr, + num_bytes, parent, ref_root, + key.objectid, key.offset, + for_reloc); +else + ret =btrfs_free_extent (trans, root, bytenr, + num_bytes, parent, ref_root, + key.objectid, key.offset, + for_reloc);if (ret) goto fail; }else { bytenr =btrfs_node_blockptr (buf, i); num_bytes = fs_info->nodesize; - ret =process_func (trans, root, bytenr, num_bytes, - parent, ref_root, level -1 ,0 , - for_reloc); +if (inc) + ret =btrfs_inc_extent_ref (trans, root, bytenr, + num_bytes, parent, ref_root, + level -1 ,0 , for_reloc); +else + ret =btrfs_free_extent (trans, root, bytenr, + num_bytes, parent, ref_root, + level -1 ,0 , for_reloc);if (ret) goto fail; }
PATCH 3/8 btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_tree_ref()
In reply to Qu Wenruobtrfs_add_delayed_tree_ref() has a longer and longer parameter list, and some caller like btrfs_inc_extent_ref() are using @owner as level for delayed tree ref.
Instead of making the parameter list longer and longer, use btrfs_ref to refactor it, so each parameter assignment should be self-explaining without dirty level/owner trick, and provides the basis for later refactor.
--- fs/btrfs/delayed-ref.c | 24 ++++++++++++++------- fs/btrfs/delayed-ref.h | 4 +--- fs/btrfs/extent-tree.c | 48 ++++++++++++++++++++++++------------------ 3 files changed, 44 insertions(+), 32 deletions(-)
fs/btrfs/delayed-ref.c +16 -8
@@ -710,9 +710,7 @@ static void init_delayed_ref_common(struct btrfs_fs_info *fs_info,* transaction commits. */ int btrfs_add_delayed_tree_ref (struct btrfs_trans_handle *trans, -u64 bytenr,u64 num_bytes,u64 parent, -u64 ref_root,int level,bool for_reloc, -int action, +struct btrfs_ref *generic_ref,struct btrfs_delayed_extent_op *extent_op,int *old_ref_mod,int *new_ref_mod) {
@@ -722,10 +720,17 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,struct btrfs_delayed_ref_root *delayed_refs;struct btrfs_qgroup_extent_record *record =NULL ;int qrecord_inserted; -bool is_system = (ref_root ==BTRFS_CHUNK_TREE_OBJECTID ); +bool is_system = (generic_ref->real_root ==BTRFS_CHUNK_TREE_OBJECTID ); +int action = generic_ref->action; +int level = generic_ref->tree_ref.level;int ret; +u64 bytenr = generic_ref->bytenr; +u64 num_bytes = generic_ref->len; +u64 parent = generic_ref->parent;u8 ref_type; +ASSERT (generic_ref && generic_ref->type ==BTRFS_REF_METADATA && + generic_ref->action);BUG_ON (extent_op && extent_op->is_data); ref =kmem_cache_alloc (btrfs_delayed_tree_ref_cachep,GFP_NOFS );if (!ref)
@@ -738,7 +743,9 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans, }if (test_bit (BTRFS_FS_QUOTA_ENABLED , &fs_info->flags) && -is_fstree (ref_root) && !for_reloc) { +is_fstree (generic_ref->real_root) && +is_fstree (generic_ref->tree_ref.root) && + !generic_ref->skip_qgroup) { record =kzalloc (sizeof (*record),GFP_NOFS );if (!record) {kmem_cache_free (btrfs_delayed_tree_ref_cachep, ref);
@@ -753,13 +760,14 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans, ref_type =BTRFS_TREE_BLOCK_REF_KEY ;init_delayed_ref_common (fs_info, &ref->node, bytenr, num_bytes, - ref_root, action, ref_type); - ref->root = ref_root; + generic_ref->tree_ref.root, action, ref_type); + ref->root = generic_ref->tree_ref.root; ref->parent = parent; ref->level = level;init_delayed_ref_head (head_ref, record, bytenr, num_bytes, - ref_root,0 , action, false, is_system); + generic_ref->tree_ref.root,0 , action, false, + is_system); head_ref->extent_op = extent_op; delayed_refs = &trans->transaction->delayed_refs;
fs/btrfs/delayed-ref.h +1 -3
@@ -333,9 +333,7 @@ static inline void btrfs_put_delayed_ref_head(struct btrfs_delayed_ref_head *hea }int btrfs_add_delayed_tree_ref (struct btrfs_trans_handle *trans, -u64 bytenr,u64 num_bytes,u64 parent, -u64 ref_root,int level,bool for_reloc, -int action, +struct btrfs_ref *generic_ref,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,
fs/btrfs/extent-tree.c +27 -21
106 lines not on this page · raw message
Signature · 1 line
2.19.2
Re: [PATCH 3/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_tree_ref()
Quoted 36 lines · Qu Wenruo
btrfs_add_delayed_tree_ref()has a longer and longer parameter list, and some caller likebtrfs_inc_extent_ref()are using @owner as level for delayed tree ref.Instead of making the parameter list longer and longer, use
btrfs_refto refactor it, so each parameter assignment should be self-explaining without dirty level/owner trick, and provides the basis for later refactor.Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/delayed-ref.c | 24 ++++++++++++++------- fs/btrfs/delayed-ref.h | 4 +--- fs/btrfs/extent-tree.c | 48 ++++++++++++++++++++++++------------------ 3 files changed, 44 insertions(+), 32 deletions(-)fs/btrfs/delayed-ref.c +2
-4@@ -710,9 +710,7 @@ static void init_delayed_ref_common(struct btrfs_fs_info *fs_info,* transaction commits. */ int btrfs_add_delayed_tree_ref (struct btrfs_trans_handle *trans, -u64 bytenr,u64 num_bytes,u64 parent, -u64 ref_root,int level,bool for_reloc, -int action, +struct btrfs_ref *generic_ref,struct btrfs_delayed_extent_op *extent_op,int *old_ref_mod,int *new_ref_mod) {@@ -722,10 +720,17 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,struct btrfs_delayed_ref_root *delayed_refs;struct btrfs_qgroup_extent_record *record =NULL ;int qrecord_inserted; -bool is_system = (ref_root ==BTRFS_CHUNK_TREE_OBJECTID ); +bool is_system = (generic_ref->real_root ==BTRFS_CHUNK_TREE_OBJECTID );
fs/btrfs/delayed-ref.c int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans, · line 733
Quoted 10 lines
fs/btrfs/delayed-ref.c +7
-0+int action = generic_ref->action; +int level = generic_ref->tree_ref.level;int ret; +u64 bytenr = generic_ref->bytenr; +u64 num_bytes = generic_ref->len; +u64 parent = generic_ref->parent;u8 ref_type; +ASSERT (generic_ref && generic_ref->type ==BTRFS_REF_METADATA && + generic_ref->action);
You have already dereferenced generic_ref by the time you ASSERT and would have crashed. I guess the first condition in the assert can be removed.
Quoted 3 lines
fs/btrfs/delayed-ref.c +0
-0BUG_ON (extent_op && extent_op->is_data); ref =kmem_cache_alloc (btrfs_delayed_tree_ref_cachep,GFP_NOFS );if (!ref)
<snip>
Quoted 15 lines
fs/btrfs/extent-tree.c +2
-0@@ -2031,6 +2031,7 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans, bool for_reloc) {struct btrfs_fs_info *fs_info = root->fs_info; +struct btrfs_ref generic_ref = {0 };int old_ref_mod, new_ref_mod;int ret;@@ -2040,13 +2041,13 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,btrfs_ref_tree_mod (root, bytenr, num_bytes, parent, root_objectid, owner, offset,BTRFS_ADD_DELAYED_REF ); + btrfs_init_generic_ref(&generic_ref, BTRFS_ADD_DELAYED_REF, bytenr,
fs/btrfs/extent-tree.c int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans, · line 2048
Quoted 10 lines
fs/btrfs/extent-tree.c +3
-6+ num_bytes, root->root_key.objectid, parent); + generic_ref.skip_qgroup = for_reloc;if (owner <BTRFS_FIRST_FREE_OBJECTID ) { - ret =btrfs_add_delayed_tree_ref (trans, bytenr, - num_bytes, parent, - root_objectid, (int )owner, - for_reloc, -BTRFS_ADD_DELAYED_REF ,NULL , - &old_ref_mod, &new_ref_mod); +btrfs_init_tree_ref (&generic_ref, (int )owner, root_objectid);
The cast is now redundant since the parameter definition of btrfs_init_tree_ref is an int.
Quoted 5 lines
fs/btrfs/extent-tree.c +2
-0+ ret =btrfs_add_delayed_tree_ref (trans, &generic_ref, +NULL , &old_ref_mod, &new_ref_mod); }else { ret = btrfs_add_delayed_data_ref(trans, root, bytenr, num_bytes, parent,
<snip>
fs/btrfs/extent-tree.c int btrfs_free_extent(struct btrfs_trans_handle *trans, · line 7113
Quoted 10 lines
fs/btrfs/extent-tree.c +1
-5@@ -7102,11 +7110,8 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans, old_ref_mod = new_ref_mod =0 ; ret =0 ; }else if (owner <BTRFS_FIRST_FREE_OBJECTID ) { - ret = btrfs_add_delayed_tree_ref(trans, bytenr, - num_bytes, parent, - root_objectid, (int)owner, - for_reloc, - BTRFS_DROP_DELAYED_REF, NULL, +btrfs_init_tree_ref (&generic_ref, (int )owner, root_objectid);
ditto about the cast
fs/btrfs/extent-tree.c int btrfs_free_extent(struct btrfs_trans_handle *trans, · line 7117
Quoted 4 lines
fs/btrfs/extent-tree.c +1
-0+ ret =btrfs_add_delayed_tree_ref (trans, &generic_ref,NULL , &old_ref_mod, &new_ref_mod); }else { ret = btrfs_add_delayed_data_ref(trans, root, bytenr,
<sniop>
Re: [PATCH 3/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_tree_ref()
Quoted 74 lines · Nikolay Borisov
Quoted 36 lines · Qu Wenruo
btrfs_add_delayed_tree_ref()has a longer and longer parameter list, and some caller likebtrfs_inc_extent_ref()are using @owner as level for delayed tree ref.Instead of making the parameter list longer and longer, use
btrfs_refto refactor it, so each parameter assignment should be self-explaining without dirty level/owner trick, and provides the basis for later refactor.Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/delayed-ref.c | 24 ++++++++++++++------- fs/btrfs/delayed-ref.h | 4 +--- fs/btrfs/extent-tree.c | 48 ++++++++++++++++++++++++------------------ 3 files changed, 44 insertions(+), 32 deletions(-)fs/btrfs/delayed-ref.c +2
-4@@ -710,9 +710,7 @@ static void init_delayed_ref_common(struct btrfs_fs_info *fs_info,* transaction commits. */ int btrfs_add_delayed_tree_ref (struct btrfs_trans_handle *trans, -u64 bytenr,u64 num_bytes,u64 parent, -u64 ref_root,int level,bool for_reloc, -int action, +struct btrfs_ref *generic_ref,struct btrfs_delayed_extent_op *extent_op,int *old_ref_mod,int *new_ref_mod) {@@ -722,10 +720,17 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,struct btrfs_delayed_ref_root *delayed_refs;struct btrfs_qgroup_extent_record *record =NULL ;int qrecord_inserted; -bool is_system = (ref_root ==BTRFS_CHUNK_TREE_OBJECTID ); +bool is_system = (generic_ref->real_root ==BTRFS_CHUNK_TREE_OBJECTID );fs/btrfs/delayed-ref.c int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans, · line 733
Quoted 10 lines
fs/btrfs/delayed-ref.c +7
-0+int action = generic_ref->action; +int level = generic_ref->tree_ref.level;int ret; +u64 bytenr = generic_ref->bytenr; +u64 num_bytes = generic_ref->len; +u64 parent = generic_ref->parent;u8 ref_type; +ASSERT (generic_ref && generic_ref->type ==BTRFS_REF_METADATA && + generic_ref->action);You have already dereferenced
generic_refby the time you ASSERT and would have crashed. I guess the first condition in the assert can be removed.Quoted 3 lines
fs/btrfs/delayed-ref.c +0
-0BUG_ON (extent_op && extent_op->is_data); ref =kmem_cache_alloc (btrfs_delayed_tree_ref_cachep,GFP_NOFS );if (!ref)<snip>
Quoted 14 lines
fs/btrfs/extent-tree.c +1
-0@@ -2031,6 +2031,7 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans, bool for_reloc) {struct btrfs_fs_info *fs_info = root->fs_info; +struct btrfs_ref generic_ref = {0 };int old_ref_mod, new_ref_mod;int ret;@@ -2040,13 +2041,13 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,btrfs_ref_tree_mod (root, bytenr, num_bytes, parent, root_objectid, owner, offset,BTRFS_ADD_DELAYED_REF );
Quoted 14 lines
fs/btrfs/extent-tree.c int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans, · line 2048
Quoted 11 lines
fs/btrfs/extent-tree.c +4
-6+btrfs_init_generic_ref (&generic_ref,BTRFS_ADD_DELAYED_REF , bytenr, + num_bytes, root->root_key.objectid, parent); + generic_ref.skip_qgroup = for_reloc;if (owner <BTRFS_FIRST_FREE_OBJECTID ) { - ret =btrfs_add_delayed_tree_ref (trans, bytenr, - num_bytes, parent, - root_objectid, (int )owner, - for_reloc, -BTRFS_ADD_DELAYED_REF ,NULL , - &old_ref_mod, &new_ref_mod); +btrfs_init_tree_ref (&generic_ref, (int )owner, root_objectid);The cast is now redundant since the parameter definition of
btrfs_init_tree_refis an int.
It will be redundant when btrfs_free_extent() is converted to use btrfs_ref. But not now.
As @owner is still u64, while parameter of btrfs_init_tree_ref() is still using int for @level.
Thanks,
Qu
Quoted 27 lines
Quoted 5 lines
fs/btrfs/extent-tree.c +2
-0+ ret =btrfs_add_delayed_tree_ref (trans, &generic_ref, +NULL , &old_ref_mod, &new_ref_mod); }else { ret = btrfs_add_delayed_data_ref(trans, root, bytenr, num_bytes, parent,<snip>
fs/btrfs/extent-tree.c int btrfs_free_extent(struct btrfs_trans_handle *trans, · line 7113
Quoted 10 lines
fs/btrfs/extent-tree.c +1
-5@@ -7102,11 +7110,8 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans, old_ref_mod = new_ref_mod =0 ; ret =0 ; }else if (owner <BTRFS_FIRST_FREE_OBJECTID ) { - ret = btrfs_add_delayed_tree_ref(trans, bytenr, - num_bytes, parent, - root_objectid, (int)owner, - for_reloc, - BTRFS_DROP_DELAYED_REF, NULL, +btrfs_init_tree_ref (&generic_ref, (int )owner, root_objectid);ditto about the cast
fs/btrfs/extent-tree.c int btrfs_free_extent(struct btrfs_trans_handle *trans, · line 7117
Quoted 4 lines
fs/btrfs/extent-tree.c +1
-0+ ret =btrfs_add_delayed_tree_ref (trans, &generic_ref,NULL , &old_ref_mod, &new_ref_mod); }else { ret = btrfs_add_delayed_data_ref(trans, root, bytenr,<sniop>
PATCH 4/8 btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_data_ref()
In reply to Qu WenruoJust like btrfs_add_delayed_tree_ref(), use btrfs_ref to refactor btrfs_add_delayed_data_ref().
--- fs/btrfs/delayed-ref.c | 19 +++++++++++++------ fs/btrfs/delayed-ref.h | 8 +++----- fs/btrfs/extent-tree.c | 24 +++++++++++------------- 3 files changed, 27 insertions(+), 24 deletions(-)
fs/btrfs/delayed-ref.c +13 -6
@@ -800,21 +800,27 @@ 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, -int *old_ref_mod,int *new_ref_mod) +struct btrfs_ref *generic_ref, +u64 reserved,int *old_ref_mod, +int *new_ref_mod) {struct btrfs_fs_info *fs_info = trans->fs_info;struct btrfs_delayed_data_ref *ref;struct btrfs_delayed_ref_head *head_ref;struct btrfs_delayed_ref_root *delayed_refs;struct btrfs_qgroup_extent_record *record =NULL ; +int action = generic_ref->action;int qrecord_inserted;int ret; +u64 bytenr = generic_ref->bytenr; +u64 num_bytes = generic_ref->len; +u64 parent = generic_ref->parent; +u64 ref_root = generic_ref->data_ref.ref_root; +u64 owner = generic_ref->data_ref.ino; +u64 offset = generic_ref->data_ref.offset;u8 ref_type; +ASSERT (generic_ref && generic_ref->type ==BTRFS_REF_DATA && action); ref =kmem_cache_alloc (btrfs_delayed_data_ref_cachep,GFP_NOFS );if (!ref)return -ENOMEM ;
@@ -838,7 +844,8 @@ 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 (root->root_key.objectid)) { +is_fstree (ref_root) &&is_fstree (generic_ref->real_root) && + !generic_ref->skip_qgroup) { record =kzalloc (sizeof (*record),GFP_NOFS );if (!record) {kmem_cache_free (btrfs_delayed_data_ref_cachep, ref);
fs/btrfs/delayed-ref.h +3 -5
@@ -337,11 +337,9 @@ 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, -int *old_ref_mod,int *new_ref_mod); +struct btrfs_ref *generic_ref, +u64 reserved,int *old_ref_mod, +int *new_ref_mod); int btrfs_add_delayed_extent_op(struct btrfs_fs_info *fs_info, struct btrfs_trans_handle *trans, u64 bytenr, u64 num_bytes,
fs/btrfs/extent-tree.c +11 -13
@@ -2049,10 +2049,8 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans, ret =btrfs_add_delayed_tree_ref (trans, &generic_ref,NULL , &old_ref_mod, &new_ref_mod); }else { - ret =btrfs_add_delayed_data_ref (trans, root, bytenr, - num_bytes, parent, - root_objectid, owner, offset, -0 ,BTRFS_ADD_DELAYED_REF , +btrfs_init_data_ref (&generic_ref, root_objectid, owner, offset); + ret =btrfs_add_delayed_data_ref (trans, &generic_ref,0 , &old_ref_mod, &new_ref_mod); }
@@ -7114,10 +7112,8 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans, ret =btrfs_add_delayed_tree_ref (trans, &generic_ref,NULL , &old_ref_mod, &new_ref_mod); }else { - ret =btrfs_add_delayed_data_ref (trans, root, bytenr, - num_bytes, parent, - root_objectid, owner, offset, -0 ,BTRFS_DROP_DELAYED_REF , +btrfs_init_data_ref (&generic_ref, root_objectid, owner, offset); + ret =btrfs_add_delayed_data_ref (trans, &generic_ref,0 , &old_ref_mod, &new_ref_mod); }
@@ -8082,6 +8078,7 @@ int btrfs_alloc_reserved_file_extent(struct btrfs_trans_handle *trans, u64 offset, u64 ram_bytes, struct btrfs_key *ins) { + struct btrfs_ref generic_ref = { 0 };int ret;BUG_ON (root->root_key.objectid ==BTRFS_TREE_LOG_OBJECTID );
@@ -8090,11 +8087,12 @@ 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, root, ins->objectid, - ins->offset,0 , - root->root_key.objectid, owner, - offset, ram_bytes, -BTRFS_ADD_DELAYED_EXTENT ,NULL ,NULL ); +btrfs_init_generic_ref (&generic_ref,BTRFS_ADD_DELAYED_EXTENT , + ins->objectid, ins->offset, + root->root_key.objectid,0 ); +btrfs_init_data_ref (&generic_ref, root->root_key.objectid, owner, offset); + ret =btrfs_add_delayed_data_ref (trans, &generic_ref, + ram_bytes,NULL ,NULL );return ret; }
Signature · 1 line
2.19.2
Re: [PATCH 4/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_data_ref()
Quoted 34 lines · Qu Wenruo
Just like
btrfs_add_delayed_tree_ref(), usebtrfs_refto refactorbtrfs_add_delayed_data_ref().Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/delayed-ref.c | 19 +++++++++++++------ fs/btrfs/delayed-ref.h | 8 +++----- fs/btrfs/extent-tree.c | 24 +++++++++++------------- 3 files changed, 27 insertions(+), 24 deletions(-)fs/btrfs/delayed-ref.c +4
-5@@ -800,21 +800,27 @@ 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, -int *old_ref_mod,int *new_ref_mod) +struct btrfs_ref *generic_ref, +u64 reserved,int *old_ref_mod, +int *new_ref_mod) {struct btrfs_fs_info *fs_info = trans->fs_info;struct btrfs_delayed_data_ref *ref;struct btrfs_delayed_ref_head *head_ref;struct btrfs_delayed_ref_root *delayed_refs;struct btrfs_qgroup_extent_record *record =NULL ; +int action = generic_ref->action;int qrecord_inserted;
fs/btrfs/delayed-ref.c int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans, · line 823
Quoted 10 lines
fs/btrfs/delayed-ref.c +7
-0int ret; +u64 bytenr = generic_ref->bytenr; +u64 num_bytes = generic_ref->len; +u64 parent = generic_ref->parent; +u64 ref_root = generic_ref->data_ref.ref_root; +u64 owner = generic_ref->data_ref.ino; +u64 offset = generic_ref->data_ref.offset;u8 ref_type; +ASSERT (generic_ref && generic_ref->type ==BTRFS_REF_DATA && action);
generic_ref already dereferenced, no need to assert it.
Quoted 89 lines
fs/btrfs/delayed-ref.c +2
-1ref =kmem_cache_alloc (btrfs_delayed_data_ref_cachep,GFP_NOFS );if (!ref)return -ENOMEM ;@@ -838,7 +844,8 @@ 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 (root->root_key.objectid)) { +is_fstree (ref_root) &&is_fstree (generic_ref->real_root) && + !generic_ref->skip_qgroup) { record =kzalloc (sizeof (*record),GFP_NOFS );if (!record) {kmem_cache_free (btrfs_delayed_data_ref_cachep, ref);fs/btrfs/delayed-ref.h +3
-5@@ -337,11 +337,9 @@ 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, -int *old_ref_mod,int *new_ref_mod); +struct btrfs_ref *generic_ref, +u64 reserved,int *old_ref_mod, +int *new_ref_mod); int btrfs_add_delayed_extent_op(struct btrfs_fs_info *fs_info, struct btrfs_trans_handle *trans, u64 bytenr, u64 num_bytes,fs/btrfs/extent-tree.c +11
-13@@ -2049,10 +2049,8 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans, ret =btrfs_add_delayed_tree_ref (trans, &generic_ref,NULL , &old_ref_mod, &new_ref_mod); }else { - ret =btrfs_add_delayed_data_ref (trans, root, bytenr, - num_bytes, parent, - root_objectid, owner, offset, -0 ,BTRFS_ADD_DELAYED_REF , +btrfs_init_data_ref (&generic_ref, root_objectid, owner, offset); + ret =btrfs_add_delayed_data_ref (trans, &generic_ref,0 , &old_ref_mod, &new_ref_mod); }@@ -7114,10 +7112,8 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans, ret =btrfs_add_delayed_tree_ref (trans, &generic_ref,NULL , &old_ref_mod, &new_ref_mod); }else { - ret =btrfs_add_delayed_data_ref (trans, root, bytenr, - num_bytes, parent, - root_objectid, owner, offset, -0 ,BTRFS_DROP_DELAYED_REF , +btrfs_init_data_ref (&generic_ref, root_objectid, owner, offset); + ret =btrfs_add_delayed_data_ref (trans, &generic_ref,0 , &old_ref_mod, &new_ref_mod); }@@ -8082,6 +8078,7 @@ int btrfs_alloc_reserved_file_extent(struct btrfs_trans_handle *trans, u64 offset, u64 ram_bytes, struct btrfs_key *ins) { + struct btrfs_ref generic_ref = { 0 };int ret;BUG_ON (root->root_key.objectid ==BTRFS_TREE_LOG_OBJECTID );@@ -8090,11 +8087,12 @@ 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, root, ins->objectid, - ins->offset,0 , - root->root_key.objectid, owner, - offset, ram_bytes, -BTRFS_ADD_DELAYED_EXTENT ,NULL ,NULL ); +btrfs_init_generic_ref (&generic_ref,BTRFS_ADD_DELAYED_EXTENT , + ins->objectid, ins->offset, + root->root_key.objectid,0 ); +btrfs_init_data_ref (&generic_ref, root->root_key.objectid, owner, offset); + ret =btrfs_add_delayed_data_ref (trans, &generic_ref, + ram_bytes,NULL ,NULL );return ret; }
PATCH 5/8 btrfs: ref-verify: Use btrfs_ref to refactor btrfs_ref_tree_mod()
In reply to Qu WenruoIt's a perfect match for btrfs_ref_tree_mod() to use btrfs_ref, as btrfs_ref describes a metadata/data reference update comprehensively.
Now we have one less function use confusing owner/level trick.
--- fs/btrfs/extent-tree.c | 27 +++++++-------------- fs/btrfs/ref-verify.c | 53 ++++++++++++++++++++++++------------------ fs/btrfs/ref-verify.h | 10 ++++---- 3 files changed, 42 insertions(+), 48 deletions(-)
fs/btrfs/extent-tree.c +8 -19
@@ -2038,9 +2038,6 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,BUG_ON (owner <BTRFS_FIRST_FREE_OBJECTID && root_objectid ==BTRFS_TREE_LOG_OBJECTID ); -btrfs_ref_tree_mod (root, bytenr, num_bytes, parent, root_objectid, - owner, offset,BTRFS_ADD_DELAYED_REF ); -btrfs_init_generic_ref (&generic_ref,BTRFS_ADD_DELAYED_REF , bytenr, num_bytes, root->root_key.objectid, parent); generic_ref.skip_qgroup = for_reloc;
@@ -2054,6 +2051,8 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans, &old_ref_mod, &new_ref_mod); } +btrfs_ref_tree_mod (fs_info, &generic_ref); +if (ret ==0 && old_ref_mod <0 && new_ref_mod >=0 ) {bool metadata = owner <BTRFS_FIRST_FREE_OBJECTID ;
@@ -7025,10 +7024,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,if (root->root_key.objectid !=BTRFS_TREE_LOG_OBJECTID ) {int old_ref_mod, new_ref_mod; -btrfs_ref_tree_mod (root, buf->start, buf->len, parent, - root->root_key.objectid, -btrfs_header_level (buf),0 , -BTRFS_DROP_DELAYED_REF ); +btrfs_ref_tree_mod (fs_info, &generic_ref); ret =btrfs_add_delayed_tree_ref (trans, &generic_ref,NULL , &old_ref_mod, &new_ref_mod);BUG_ON (ret);/* -ENOMEM */
@@ -7089,11 +7085,6 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans,if (btrfs_is_testing (fs_info))return 0 ; -if (root_objectid !=BTRFS_TREE_LOG_OBJECTID ) -btrfs_ref_tree_mod (root, bytenr, num_bytes, parent, - root_objectid, owner, offset, -BTRFS_DROP_DELAYED_REF ); -btrfs_init_generic_ref (&generic_ref,BTRFS_DROP_DELAYED_REF , bytenr, num_bytes, root->root_key.objectid, parent); generic_ref.skip_qgroup = for_reloc;
@@ -7117,6 +7108,9 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans, &old_ref_mod, &new_ref_mod); } +if (root_objectid !=BTRFS_TREE_LOG_OBJECTID ) +btrfs_ref_tree_mod (fs_info, &generic_ref); +if (ret ==0 && old_ref_mod >=0 && new_ref_mod <0 ) {bool metadata = owner <BTRFS_FIRST_FREE_OBJECTID ;
@@ -8083,14 +8077,11 @@ int btrfs_alloc_reserved_file_extent(struct btrfs_trans_handle *trans,BUG_ON (root->root_key.objectid ==BTRFS_TREE_LOG_OBJECTID ); -btrfs_ref_tree_mod (root, ins->objectid, ins->offset,0 , - root->root_key.objectid, owner, offset, -BTRFS_ADD_DELAYED_EXTENT ); -btrfs_init_generic_ref (&generic_ref,BTRFS_ADD_DELAYED_EXTENT , ins->objectid, ins->offset, root->root_key.objectid,0 );btrfs_init_data_ref (&generic_ref, root->root_key.objectid, owner, offset); +btrfs_ref_tree_mod (root->fs_info, &generic_ref); ret =btrfs_add_delayed_data_ref (trans, &generic_ref, ram_bytes,NULL ,NULL );return ret;
@@ -8338,13 +8329,11 @@ struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans, extent_op->is_data = false; extent_op->level = level; -btrfs_ref_tree_mod (root, ins.objectid, ins.offset, parent, - root_objectid, level,0 , -BTRFS_ADD_DELAYED_EXTENT );btrfs_init_generic_ref (&generic_ref,BTRFS_ADD_DELAYED_EXTENT , ins.objectid, ins.offset, root->root_key.objectid, parent);btrfs_init_tree_ref (&generic_ref, level, root_objectid); +btrfs_ref_tree_mod (fs_info, &generic_ref); ret =btrfs_add_delayed_tree_ref (trans, &generic_ref, extent_op,NULL ,NULL );if (ret)
fs/btrfs/ref-verify.c +30 -23
107 lines not on this page · raw message
fs/btrfs/ref-verify.h +4 -6
@@ -9,9 +9,8 @@#ifdef CONFIG_BTRFS_FS_REF_VERIFY int btrfs_build_ref_tree (struct btrfs_fs_info *fs_info);void btrfs_free_ref_cache (struct btrfs_fs_info *fs_info); -int btrfs_ref_tree_mod (struct btrfs_root *root,u64 bytenr,u64 num_bytes, -u64 parent,u64 ref_root,u64 owner,u64 offset, -int action); +int btrfs_ref_tree_mod (struct btrfs_fs_info *fs_info, +struct btrfs_ref *generic_ref);void btrfs_free_ref_tree_range (struct btrfs_fs_info *fs_info,u64 start,u64 len);
@@ -30,9 +29,8 @@ static inline void btrfs_free_ref_cache(struct btrfs_fs_info *fs_info) { } -static inline int btrfs_ref_tree_mod (struct btrfs_root *root,u64 bytenr, -u64 num_bytes,u64 parent,u64 ref_root, -u64 owner,u64 offset,int action) +static inline int btrfs_ref_tree_mod (struct btrfs_fs_info *fs_info, +struct btrfs_ref *generic_ref) {return 0 ; }
Signature · 1 line
2.19.2
PATCH 6/8 btrfs: extent-tree: Use btrfs_ref to refactor add_pinned_bytes()
In reply to Qu WenruoSince add_pinned_bytes() only needs to know if the extent is metadata and if it's a chunk tree extent, btrfs_ref is a perfect match for it, as we don't need various owner/level trick to determine extent type.
--- fs/btrfs/extent-tree.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)
fs/btrfs/extent-tree.c +10 -16
@@ -738,14 +738,15 @@ static struct btrfs_space_info *__find_space_info(struct btrfs_fs_info *info,return NULL ; } -static void add_pinned_bytes (struct btrfs_fs_info *fs_info,s64 num_bytes, -bool metadata,u64 root_objectid) +static void add_pinned_bytes (struct btrfs_fs_info *fs_info, +struct btrfs_ref *ref) {struct btrfs_space_info *space_info; +s64 num_bytes = -ref->len;u64 flags; -if (metadata) { -if (root_objectid ==BTRFS_CHUNK_TREE_OBJECTID ) +if (ref->type ==BTRFS_REF_METADATA ) { +if (ref->tree_ref.root ==BTRFS_CHUNK_TREE_OBJECTID ) flags =BTRFS_BLOCK_GROUP_SYSTEM ;else flags =BTRFS_BLOCK_GROUP_METADATA ;
@@ -2053,11 +2054,8 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,btrfs_ref_tree_mod (fs_info, &generic_ref); -if (ret ==0 && old_ref_mod <0 && new_ref_mod >=0 ) { -bool metadata = owner <BTRFS_FIRST_FREE_OBJECTID ; - -add_pinned_bytes (fs_info, -num_bytes, metadata, root_objectid); - } +if (ret ==0 && old_ref_mod <0 && new_ref_mod >=0 ) +add_pinned_bytes (fs_info, &generic_ref);return ret; }
@@ -7059,8 +7057,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans, } out:if (pin) -add_pinned_bytes (fs_info, buf->len, true, - root->root_key.objectid); +add_pinned_bytes (fs_info, &generic_ref);if (last_ref) {/*
@@ -7111,11 +7108,8 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans,if (root_objectid !=BTRFS_TREE_LOG_OBJECTID )btrfs_ref_tree_mod (fs_info, &generic_ref); -if (ret ==0 && old_ref_mod >=0 && new_ref_mod <0 ) { -bool metadata = owner <BTRFS_FIRST_FREE_OBJECTID ; - -add_pinned_bytes (fs_info, num_bytes, metadata, root_objectid); - } +if (ret ==0 && old_ref_mod >=0 && new_ref_mod <0 ) +add_pinned_bytes (fs_info, &generic_ref);return ret; }
Signature · 1 line
2.19.2
Re: [PATCH 6/8] btrfs: extent-tree: Use btrfs_ref to refactor add_pinned_bytes()
Quoted 5 lines · Qu Wenruo
Since
add_pinned_bytes()only needs to know if the extent is metadata and if it's a chunk tree extent,btrfs_refis a perfect match for it, as we don't need various owner/level trick to determine extent type.Signed-off-by Qu Wenruo <[email protected]>
Quoted 67 lines
--- fs/btrfs/extent-tree.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)fs/btrfs/extent-tree.c +10
-16@@ -738,14 +738,15 @@ static struct btrfs_space_info *__find_space_info(struct btrfs_fs_info *info,return NULL ; } -static void add_pinned_bytes (struct btrfs_fs_info *fs_info,s64 num_bytes, -bool metadata,u64 root_objectid) +static void add_pinned_bytes (struct btrfs_fs_info *fs_info, +struct btrfs_ref *ref) {struct btrfs_space_info *space_info; +s64 num_bytes = -ref->len;u64 flags; -if (metadata) { -if (root_objectid ==BTRFS_CHUNK_TREE_OBJECTID ) +if (ref->type ==BTRFS_REF_METADATA ) { +if (ref->tree_ref.root ==BTRFS_CHUNK_TREE_OBJECTID ) flags =BTRFS_BLOCK_GROUP_SYSTEM ;else flags =BTRFS_BLOCK_GROUP_METADATA ;@@ -2053,11 +2054,8 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,btrfs_ref_tree_mod (fs_info, &generic_ref); -if (ret ==0 && old_ref_mod <0 && new_ref_mod >=0 ) { -bool metadata = owner <BTRFS_FIRST_FREE_OBJECTID ; - -add_pinned_bytes (fs_info, -num_bytes, metadata, root_objectid); - } +if (ret ==0 && old_ref_mod <0 && new_ref_mod >=0 ) +add_pinned_bytes (fs_info, &generic_ref);return ret; }@@ -7059,8 +7057,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans, } out:if (pin) -add_pinned_bytes (fs_info, buf->len, true, - root->root_key.objectid); +add_pinned_bytes (fs_info, &generic_ref);if (last_ref) {/* @@ -7111,11 +7108,8 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans,if (root_objectid !=BTRFS_TREE_LOG_OBJECTID )btrfs_ref_tree_mod (fs_info, &generic_ref); -if (ret ==0 && old_ref_mod >=0 && new_ref_mod <0 ) { -bool metadata = owner <BTRFS_FIRST_FREE_OBJECTID ; - -add_pinned_bytes (fs_info, num_bytes, metadata, root_objectid); - } +if (ret ==0 && old_ref_mod >=0 && new_ref_mod <0 ) +add_pinned_bytes (fs_info, &generic_ref);return ret; }
PATCH 7/8 btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref()
In reply to Qu WenruoNow we don't need to play the dirty game of reusing @owner for tree block level.
--- fs/btrfs/ctree.h | 6 ++--- fs/btrfs/extent-tree.c | 58 ++++++++++++++++++++++-------------------- fs/btrfs/file.c | 20 ++++++++++----- fs/btrfs/inode.c | 10 +++++--- fs/btrfs/ioctl.c | 17 ++++++++----- fs/btrfs/relocation.c | 44 ++++++++++++++++++++------------ fs/btrfs/tree-log.c | 12 ++++++--- 7 files changed, 100 insertions(+), 67 deletions(-)
fs/btrfs/ctree.h +2 -4
@@ -40,6 +40,7 @@ extern struct kmem_cache *btrfs_bit_radix_cachep;extern struct kmem_cache *btrfs_path_cachep;extern struct kmem_cache *btrfs_free_space_cachep;struct btrfs_ordered_sum ; +struct btrfs_ref ;#define BTRFS_MAGIC 0x4D5F53665248425FULL/* ascii _BHRfS_M, no null */
@@ -2682,10 +2683,7 @@ int btrfs_free_and_pin_reserved_extent(struct btrfs_fs_info *fs_info,void btrfs_prepare_extent_commit (struct btrfs_fs_info *fs_info);int btrfs_finish_extent_commit (struct btrfs_trans_handle *trans);int btrfs_inc_extent_ref (struct btrfs_trans_handle *trans, -struct btrfs_root *root, -u64 bytenr,u64 num_bytes,u64 parent, -u64 root_objectid,u64 owner,u64 offset, -bool for_reloc); +struct btrfs_ref *generic_ref);int btrfs_start_dirty_block_groups (struct btrfs_trans_handle *trans); int btrfs_write_dirty_block_groups(struct btrfs_trans_handle *trans,
fs/btrfs/extent-tree.c +31 -27
102 lines not on this page · raw message
fs/btrfs/file.c +14 -6
@@ -754,6 +754,7 @@ int __btrfs_drop_extents(struct btrfs_trans_handle *trans,struct btrfs_fs_info *fs_info = root->fs_info;struct extent_buffer *leaf;struct btrfs_file_extent_item *fi; +struct btrfs_ref ref = {0 };struct btrfs_key key;struct btrfs_key new_key;u64 ino =btrfs_ino (BTRFS_I (inode));
@@ -909,11 +910,15 @@ int __btrfs_drop_extents(struct btrfs_trans_handle *trans,btrfs_mark_buffer_dirty (leaf);if (update_refs && disk_bytenr >0 ) { - ret =btrfs_inc_extent_ref (trans, root, - disk_bytenr, num_bytes,0 , +btrfs_init_generic_ref (&ref, +BTRFS_ADD_DELAYED_REF , + disk_bytenr, num_bytes, + root->root_key.objectid,0 ); +btrfs_init_data_ref (&ref, root->root_key.objectid, new_key.objectid, - start - extent_offset, false); + start - extent_offset); + ret =btrfs_inc_extent_ref (trans, &ref);BUG_ON (ret);/* -ENOMEM */ } key.offset = start;
@@ -1142,6 +1147,7 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,struct extent_buffer *leaf;struct btrfs_path *path;struct btrfs_file_extent_item *fi; +struct btrfs_ref ref = {0 };struct btrfs_key key;struct btrfs_key new_key;u64 bytenr;
@@ -1287,9 +1293,11 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans, extent_end - split);btrfs_mark_buffer_dirty (leaf); - ret =btrfs_inc_extent_ref (trans, root, bytenr, num_bytes, -0 , root->root_key.objectid, - ino, orig_offset, false); +btrfs_init_generic_ref (&ref,BTRFS_ADD_DELAYED_REF , bytenr, + num_bytes, root->root_key.objectid,0 ); +btrfs_init_data_ref (&ref, root->root_key.objectid, ino, + orig_offset); + ret =btrfs_inc_extent_ref (trans, &ref);if (ret) {btrfs_abort_transaction (trans, ret); goto out;
fs/btrfs/inode.c +6 -4
@@ -2540,6 +2540,7 @@ static noinline int relink_extent_backref(struct btrfs_path *path,struct btrfs_file_extent_item *item;struct btrfs_ordered_extent *ordered;struct btrfs_trans_handle *trans; +struct btrfs_ref ref = {0 };struct btrfs_root *root;struct btrfs_key key;struct extent_buffer *leaf;
@@ -2710,10 +2711,11 @@ static noinline int relink_extent_backref(struct btrfs_path *path,inode_add_bytes (inode, len);btrfs_release_path (path); - ret =btrfs_inc_extent_ref (trans, root, new->bytenr, - new->disk_len,0 , - backref->root_id, backref->inum, - new->file_pos, false);/* start - extent_offset */ +btrfs_init_generic_ref (&ref,BTRFS_ADD_DELAYED_REF , new->bytenr, + new->disk_len, backref->root_id,0 ); +btrfs_init_data_ref (&ref, backref->root_id, backref->inum, + new->file_pos);/* start - extent_offset */ + ret =btrfs_inc_extent_ref (trans, &ref);if (ret) {btrfs_abort_transaction (trans, ret); goto out_free_path;
fs/btrfs/ioctl.c +10 -7
@@ -4083,14 +4083,17 @@ static int btrfs_clone(struct inode *src, struct inode *inode, datal);if (disko) { +struct btrfs_ref ref = {0 };inode_add_bytes (inode, datal); - ret =btrfs_inc_extent_ref (trans, - root, - disko, diskl,0 , - root->root_key.objectid, -btrfs_ino (BTRFS_I (inode)), - new_key.offset - datao, - false); +btrfs_init_generic_ref (&ref, +BTRFS_ADD_DELAYED_REF , disko, + diskl, root->root_key.objectid, +0 ); +btrfs_init_data_ref (&ref, + root->root_key.objectid, +btrfs_ino (BTRFS_I (inode)), + new_key.offset - datao); + ret =btrfs_inc_extent_ref (trans, &ref);if (ret) {btrfs_abort_transaction (trans, ret);
fs/btrfs/relocation.c +28 -16
81 lines not on this page · raw message
fs/btrfs/tree-log.c +9 -3
@@ -693,9 +693,11 @@ static noinline int replay_one_extent(struct btrfs_trans_handle *trans, goto out;if (ins.objectid >0 ) { +struct btrfs_ref ref = {0 };u64 csum_start;u64 csum_end;LIST_HEAD (ordered_sums); +/* * is this extent already allocated in the extent * allocation tree? If so, just add a reference
@@ -703,10 +705,14 @@ static noinline int replay_one_extent(struct btrfs_trans_handle *trans, ret =btrfs_lookup_data_extent (fs_info, ins.objectid, ins.offset);if (ret ==0 ) { - ret =btrfs_inc_extent_ref (trans, root, +btrfs_init_generic_ref (&ref, +BTRFS_ADD_DELAYED_REF , ins.objectid, ins.offset, -0 , root->root_key.objectid, - key->objectid, offset, false); + root->root_key.objectid,0 ); +btrfs_init_data_ref (&ref, + root->root_key.objectid, + key->objectid, offset); + ret =btrfs_inc_extent_ref (trans, &ref);if (ret) goto out; }else {
Signature · 1 line
2.19.2
Re: [PATCH 7/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref()
Quoted 52 lines · Qu Wenruo
Now we don't need to play the dirty game of reusing @owner for tree block level.
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/ctree.h | 6 ++--- fs/btrfs/extent-tree.c | 58 ++++++++++++++++++++++-------------------- fs/btrfs/file.c | 20 ++++++++++----- fs/btrfs/inode.c | 10 +++++--- fs/btrfs/ioctl.c | 17 ++++++++----- fs/btrfs/relocation.c | 44 ++++++++++++++++++++------------ fs/btrfs/tree-log.c | 12 ++++++--- 7 files changed, 100 insertions(+), 67 deletions(-)fs/btrfs/ctree.h +2
-4@@ -40,6 +40,7 @@ extern struct kmem_cache *btrfs_bit_radix_cachep;extern struct kmem_cache *btrfs_path_cachep;extern struct kmem_cache *btrfs_free_space_cachep;struct btrfs_ordered_sum ; +struct btrfs_ref ;#define BTRFS_MAGIC 0x4D5F53665248425FULL/* ascii _BHRfS_M, no null */ @@ -2682,10 +2683,7 @@ int btrfs_free_and_pin_reserved_extent(struct btrfs_fs_info *fs_info,void btrfs_prepare_extent_commit (struct btrfs_fs_info *fs_info);int btrfs_finish_extent_commit (struct btrfs_trans_handle *trans);int btrfs_inc_extent_ref (struct btrfs_trans_handle *trans, -struct btrfs_root *root, -u64 bytenr,u64 num_bytes,u64 parent, -u64 root_objectid,u64 owner,u64 offset, -bool for_reloc); +struct btrfs_ref *generic_ref);int btrfs_start_dirty_block_groups (struct btrfs_trans_handle *trans); int btrfs_write_dirty_block_groups(struct btrfs_trans_handle *trans,fs/btrfs/extent-tree.c +1
-4@@ -2026,36 +2026,28 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,/* Can return -ENOMEM */ int btrfs_inc_extent_ref (struct btrfs_trans_handle *trans, -struct btrfs_root *root, -u64 bytenr,u64 num_bytes,u64 parent, -u64 root_objectid,u64 owner,u64 offset, -bool for_reloc) +struct btrfs_ref *generic_ref) {
fs/btrfs/extent-tree.c int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr, · line 2036
Quoted 10 lines
fs/btrfs/extent-tree.c +3
-4-struct btrfs_fs_info *fs_info = root->fs_info; -struct btrfs_ref generic_ref = {0 }; +struct btrfs_fs_info *fs_info = trans->fs_info;int old_ref_mod, new_ref_mod;int ret; -BUG_ON (owner <BTRFS_FIRST_FREE_OBJECTID && - root_objectid ==BTRFS_TREE_LOG_OBJECTID ); +BUG_ON (generic_ref->type ==BTRFS_REF_NOT_SET || + !generic_ref->action);
Ouch, we should be removing BUG_ONs and not introducing new ones. Make it an assert
fs/btrfs/extent-tree.c int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr, · line 2038
Quoted 2 lines
fs/btrfs/extent-tree.c +2
-0+BUG_ON (generic_ref->type ==BTRFS_REF_METADATA && + generic_ref->tree_ref.root ==BTRFS_TREE_LOG_OBJECTID );
Ideally this should also be converted to an assert. I guess it could only happen if the filesystem is corrupted so perhaps is redundant now?
Quoted 27 lines
fs/btrfs/extent-tree.c +6
-12-btrfs_init_generic_ref (&generic_ref,BTRFS_ADD_DELAYED_REF , bytenr, - num_bytes, root->root_key.objectid, parent); - generic_ref.skip_qgroup = for_reloc; -if (owner <BTRFS_FIRST_FREE_OBJECTID ) { -btrfs_init_tree_ref (&generic_ref, (int )owner, root_objectid); - ret =btrfs_add_delayed_tree_ref (trans, &generic_ref, +if (generic_ref->type ==BTRFS_REF_METADATA ) + ret =btrfs_add_delayed_tree_ref (trans, generic_ref,NULL , &old_ref_mod, &new_ref_mod); - }else { -btrfs_init_data_ref (&generic_ref, root_objectid, owner, offset); - ret =btrfs_add_delayed_data_ref (trans, &generic_ref,0 , +else + ret =btrfs_add_delayed_data_ref (trans, generic_ref,0 , &old_ref_mod, &new_ref_mod); - } -btrfs_ref_tree_mod (fs_info, &generic_ref); +btrfs_ref_tree_mod (fs_info, generic_ref);if (ret ==0 && old_ref_mod <0 && new_ref_mod >=0 ) -add_pinned_bytes (fs_info, &generic_ref); +add_pinned_bytes (fs_info, generic_ref);return ret; }
<snip>
Re: [PATCH 7/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref()
Quoted 67 lines · Nikolay Borisov
Quoted 52 lines · Qu Wenruo
Now we don't need to play the dirty game of reusing @owner for tree block level.
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/ctree.h | 6 ++--- fs/btrfs/extent-tree.c | 58 ++++++++++++++++++++++-------------------- fs/btrfs/file.c | 20 ++++++++++----- fs/btrfs/inode.c | 10 +++++--- fs/btrfs/ioctl.c | 17 ++++++++----- fs/btrfs/relocation.c | 44 ++++++++++++++++++++------------ fs/btrfs/tree-log.c | 12 ++++++--- 7 files changed, 100 insertions(+), 67 deletions(-)fs/btrfs/ctree.h +2
-4@@ -40,6 +40,7 @@ extern struct kmem_cache *btrfs_bit_radix_cachep;extern struct kmem_cache *btrfs_path_cachep;extern struct kmem_cache *btrfs_free_space_cachep;struct btrfs_ordered_sum ; +struct btrfs_ref ;#define BTRFS_MAGIC 0x4D5F53665248425FULL/* ascii _BHRfS_M, no null */ @@ -2682,10 +2683,7 @@ int btrfs_free_and_pin_reserved_extent(struct btrfs_fs_info *fs_info,void btrfs_prepare_extent_commit (struct btrfs_fs_info *fs_info);int btrfs_finish_extent_commit (struct btrfs_trans_handle *trans);int btrfs_inc_extent_ref (struct btrfs_trans_handle *trans, -struct btrfs_root *root, -u64 bytenr,u64 num_bytes,u64 parent, -u64 root_objectid,u64 owner,u64 offset, -bool for_reloc); +struct btrfs_ref *generic_ref);int btrfs_start_dirty_block_groups (struct btrfs_trans_handle *trans); int btrfs_write_dirty_block_groups(struct btrfs_trans_handle *trans,fs/btrfs/extent-tree.c +1
-4@@ -2026,36 +2026,28 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,/* Can return -ENOMEM */ int btrfs_inc_extent_ref (struct btrfs_trans_handle *trans, -struct btrfs_root *root, -u64 bytenr,u64 num_bytes,u64 parent, -u64 root_objectid,u64 owner,u64 offset, -bool for_reloc) +struct btrfs_ref *generic_ref) {fs/btrfs/extent-tree.c int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr, · line 2036
Quoted 10 lines
fs/btrfs/extent-tree.c +3
-4-struct btrfs_fs_info *fs_info = root->fs_info; -struct btrfs_ref generic_ref = {0 }; +struct btrfs_fs_info *fs_info = trans->fs_info;int old_ref_mod, new_ref_mod;int ret; -BUG_ON (owner <BTRFS_FIRST_FREE_OBJECTID && - root_objectid ==BTRFS_TREE_LOG_OBJECTID ); +BUG_ON (generic_ref->type ==BTRFS_REF_NOT_SET || + !generic_ref->action);Ouch, we should be removing
BUG_ONsand not introducing new ones. Make it an assert
Quoted 5 lines
fs/btrfs/extent-tree.c int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr, · line 2038
Quoted 2 lines
fs/btrfs/extent-tree.c +2
-0+BUG_ON (generic_ref->type ==BTRFS_REF_METADATA && + generic_ref->tree_ref.root ==BTRFS_TREE_LOG_OBJECTID );Ideally this should also be converted to an assert. I guess it could only happen if the filesystem is corrupted so perhaps is redundant now?
I think we need more types of assert, not all BUG_ONs can be replaced by ASSERT as behaviour would change depending ond CONFIG_BTRFS_ASSERTS. IMO the asserts are best suited for development-time checks and cannot replace runtime-checks where the BUG_ON is a big hammer to stop execution otherwise it would cause worse things later.
This was mentioned in the past a few times, what we really don't want are new BUG_ONs instead of proper error handling. The rest would be nice to annotate by comments why it's there and that there's really no other way around that.
I don't recall all the ideas so am not saying either way for the asserts or bugons. Both would be ok here and full audit of BUG_ONs is one of my long-term projects so it would get sorted eventually.
Re: [PATCH 7/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref()
Quoted 60 lines · David Sterba
Quoted 59 lines · Nikolay Borisov
Quoted 58 lines · Qu Wenruo
Now we don't need to play the dirty game of reusing @owner for tree block level.
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/ctree.h | 6 ++--- fs/btrfs/extent-tree.c | 58 ++++++++++++++++++++++-------------------- fs/btrfs/file.c | 20 ++++++++++----- fs/btrfs/inode.c | 10 +++++--- fs/btrfs/ioctl.c | 17 ++++++++----- fs/btrfs/relocation.c | 44 ++++++++++++++++++++------------ fs/btrfs/tree-log.c | 12 ++++++--- 7 files changed, 100 insertions(+), 67 deletions(-)fs/btrfs/ctree.h +2
-4@@ -40,6 +40,7 @@ extern struct kmem_cache *btrfs_bit_radix_cachep;extern struct kmem_cache *btrfs_path_cachep;extern struct kmem_cache *btrfs_free_space_cachep;struct btrfs_ordered_sum ; +struct btrfs_ref ;#define BTRFS_MAGIC 0x4D5F53665248425FULL/* ascii _BHRfS_M, no null */ @@ -2682,10 +2683,7 @@ int btrfs_free_and_pin_reserved_extent(struct btrfs_fs_info *fs_info,void btrfs_prepare_extent_commit (struct btrfs_fs_info *fs_info);int btrfs_finish_extent_commit (struct btrfs_trans_handle *trans);int btrfs_inc_extent_ref (struct btrfs_trans_handle *trans, -struct btrfs_root *root, -u64 bytenr,u64 num_bytes,u64 parent, -u64 root_objectid,u64 owner,u64 offset, -bool for_reloc); +struct btrfs_ref *generic_ref);int btrfs_start_dirty_block_groups (struct btrfs_trans_handle *trans); int btrfs_write_dirty_block_groups(struct btrfs_trans_handle *trans,fs/btrfs/extent-tree.c +2
-6@@ -2026,36 +2026,28 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,/* Can return -ENOMEM */ int btrfs_inc_extent_ref (struct btrfs_trans_handle *trans, -struct btrfs_root *root, -u64 bytenr,u64 num_bytes,u64 parent, -u64 root_objectid,u64 owner,u64 offset, -bool for_reloc) +struct btrfs_ref *generic_ref) { -struct btrfs_fs_info *fs_info = root->fs_info; -struct btrfs_ref generic_ref = {0 }; +struct btrfs_fs_info *fs_info = trans->fs_info;int old_ref_mod, new_ref_mod;int ret;
Quoted 7 lines
Quoted 7 lines
fs/btrfs/extent-tree.c int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr, · line 2036
Quoted 4 lines
fs/btrfs/extent-tree.c +2
-2-BUG_ON (owner <BTRFS_FIRST_FREE_OBJECTID && - root_objectid ==BTRFS_TREE_LOG_OBJECTID ); +BUG_ON (generic_ref->type ==BTRFS_REF_NOT_SET || + !generic_ref->action);Ouch, we should be removing
BUG_ONsand not introducing new ones. Make it an assert
Oh I forgot this one, it should be ASSERT().
Quoted 6 lines
Quoted 6 lines
fs/btrfs/extent-tree.c int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr, · line 2038
Quoted 3 lines
fs/btrfs/extent-tree.c +2
-0+BUG_ON (generic_ref->type ==BTRFS_REF_METADATA && + generic_ref->tree_ref.root ==BTRFS_TREE_LOG_OBJECTID );Ideally this should also be converted to an assert. I guess it could only happen if the filesystem is corrupted so perhaps is redundant now?
This is from the original code, just changed to btrfs_ref interface.
Unlike previous easy check, this one could happen by careless caller, just one wrong @ref_root could easily lead to this BUG_ON().
Quoted 6 lines
I think we need more types of assert, not all
BUG_ONscan be replaced by ASSERT as behaviour would change depending ondCONFIG_BTRFS_ASSERTS. IMO the asserts are best suited for development-time checks and cannot replace runtime-checks where theBUG_ONis a big hammer to stop execution otherwise it would cause worse things later.
Yep, so I kept the original BUG_ON(). (Although forgot to use ASSERT() for the new check)
Quoted 5 lines
This was mentioned in the past a few times, what we really don't want are new
BUG_ONsinstead of proper error handling. The rest would be nice to annotate by comments why it's there and that there's really no other way around that.
Then I'd go back to my old practice, return EINVAL/EUCLEAN with proper error message.
By that way, we won't continue under all case, and with proper error message to info developer to simplify user debug.
The only reason for me not to do this practice this time is, I didn't see much adoption except me.
So if there is some positive feedback, I'd like to use such practice more.
Thanks,
Qu
Quoted 5 lines
I don't recall all the ideas so am not saying either way for the asserts or bugons. Both would be ok here and full audit of
BUG_ONsis one of my long-term projects so it would get sorted eventually.
PATCH 8/8 btrfs: extent-tree: Use btrfs_ref to refactor btrfs_free_extent()
In reply to Qu WenruoSimilar to btrfs_inc_extent_ref(), just use btrfs_ref to replace the long parameter list and the confusing @owner parameter.
--- fs/btrfs/ctree.h | 5 +--- fs/btrfs/extent-tree.c | 53 ++++++++++++++++++------------------------ fs/btrfs/file.c | 23 ++++++++++-------- fs/btrfs/inode.c | 13 +++++++---- fs/btrfs/relocation.c | 26 +++++++++++++-------- 5 files changed, 62 insertions(+), 58 deletions(-)
fs/btrfs/ctree.h +1 -4
@@ -2671,10 +2671,7 @@ int btrfs_set_disk_extent_flags(struct btrfs_trans_handle *trans,struct btrfs_fs_info *fs_info, u64 bytenr, u64 num_bytes, u64 flags, int level, int is_data); -int btrfs_free_extent (struct btrfs_trans_handle *trans, -struct btrfs_root *root, -u64 bytenr,u64 num_bytes,u64 parent,u64 root_objectid, -u64 owner,u64 offset,bool for_reloc); +int btrfs_free_extent (struct btrfs_trans_handle *trans,struct btrfs_ref *ref);int btrfs_free_reserved_extent (struct btrfs_fs_info *fs_info,u64 start,u64 len,int delalloc);
fs/btrfs/extent-tree.c +23 -30
104 lines not on this page · raw message
fs/btrfs/file.c +13 -10
@@ -998,11 +998,15 @@ int __btrfs_drop_extents(struct btrfs_trans_handle *trans, extent_end =ALIGN (extent_end, fs_info->sectorsize); }else if (update_refs && disk_bytenr >0 ) { - ret =btrfs_free_extent (trans, root, - disk_bytenr, num_bytes,0 , +btrfs_init_generic_ref (&ref, +BTRFS_DROP_DELAYED_REF , + disk_bytenr, num_bytes, + root->root_key.objectid,0 ); +btrfs_init_data_ref (&ref, root->root_key.objectid, - key.objectid, key.offset - - extent_offset, false); + key.objectid, + key.offset - extent_offset); + ret =btrfs_free_extent (trans, &ref);BUG_ON (ret);/* -ENOMEM */ inode_sub_bytes (inode, extent_end - key.offset);
@@ -1319,6 +1323,9 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans, other_start = end; other_end =0 ; +btrfs_init_generic_ref (&ref,BTRFS_DROP_DELAYED_REF , bytenr, + num_bytes, root->root_key.objectid,0 ); +btrfs_init_data_ref (&ref, root->root_key.objectid, ino, orig_offset);if (extent_mergeable (leaf, path->slots[0 ] +1 , ino, bytenr, orig_offset, &other_start, &other_end)) {
@@ -1329,9 +1336,7 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans, extent_end = other_end; del_slot = path->slots[0 ] +1 ; del_nr++; - ret =btrfs_free_extent (trans, root, bytenr, num_bytes, -0 , root->root_key.objectid, - ino, orig_offset, false); + ret =btrfs_free_extent (trans, &ref);if (ret) {btrfs_abort_transaction (trans, ret); goto out;
@@ -1349,9 +1354,7 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans, key.offset = other_start; del_slot = path->slots[0 ]; del_nr++; - ret =btrfs_free_extent (trans, root, bytenr, num_bytes, -0 , root->root_key.objectid, - ino, orig_offset, false); + ret =btrfs_free_extent (trans, &ref);if (ret) {btrfs_abort_transaction (trans, ret); goto out;
fs/btrfs/inode.c +9 -4
@@ -4714,12 +4714,17 @@ int btrfs_truncate_inode_items(struct btrfs_trans_handle *trans,if (found_extent && (test_bit (BTRFS_ROOT_REF_COWS , &root->state) || root == fs_info->tree_root)) { +struct btrfs_ref ref = {0 }; +btrfs_set_path_blocking (path); bytes_deleted += extent_num_bytes; - ret =btrfs_free_extent (trans, root, extent_start, - extent_num_bytes,0 , -btrfs_header_owner (leaf), - ino, extent_offset, false); + +btrfs_init_generic_ref (&ref,BTRFS_DROP_DELAYED_REF , + extent_start, extent_num_bytes, + root->root_key.objectid,0 ); +btrfs_init_data_ref (&ref,btrfs_header_owner (leaf), + ino, extent_offset); + ret =btrfs_free_extent (trans, &ref);if (ret) {btrfs_abort_transaction (trans, ret);break ;
fs/btrfs/relocation.c +16 -10
@@ -1731,9 +1731,11 @@ int replace_file_extents(struct btrfs_trans_handle *trans,break ; } - ret =btrfs_free_extent (trans, root, bytenr, num_bytes, - parent,btrfs_header_owner (leaf), - key.objectid, key.offset, false); +btrfs_init_generic_ref (&ref,BTRFS_DROP_DELAYED_REF , bytenr, + num_bytes, root->root_key.objectid, parent); +btrfs_init_data_ref (&ref,btrfs_header_owner (leaf), + key.objectid, key.offset); + ret =btrfs_free_extent (trans, &ref);if (ret) {btrfs_abort_transaction (trans, ret);break ;
@@ -1948,15 +1950,19 @@ int replace_path(struct btrfs_trans_handle *trans, struct reloc_control *rc, ret =btrfs_inc_extent_ref (trans, &ref);BUG_ON (ret); - ret =btrfs_free_extent (trans, src, new_bytenr, blocksize, - path->nodes[level]->start, - src->root_key.objectid, level -1 ,0 , - true); +btrfs_init_generic_ref (&ref,BTRFS_DROP_DELAYED_REF , new_bytenr, + blocksize, src->root_key.objectid, + path->nodes[level]->start); +btrfs_init_tree_ref (&ref, level -1 , src->root_key.objectid); + ref.skip_qgroup = true; + ret =btrfs_free_extent (trans, &ref);BUG_ON (ret); - ret =btrfs_free_extent (trans, dest, old_bytenr, blocksize, -0 , dest->root_key.objectid, level -1 , -0 , true); +btrfs_init_generic_ref (&ref,BTRFS_DROP_DELAYED_REF , old_bytenr, + blocksize, dest->root_key.objectid,0 ); +btrfs_init_tree_ref (&ref, level -1 , dest->root_key.objectid); + ref.skip_qgroup = true; + ret =btrfs_free_extent (trans, &ref);BUG_ON (ret);btrfs_unlock_up_safe (path,0 );
Signature · 1 line
2.19.2