[PATCH v2 0/8] Delayed refs cleanups/streamlining

[PATCH v2 0/8] Delayed refs cleanups/streamlining

[Nikolay Borisov]

This patchkit aims to simplify and streamline the logic involved in 
initialising and adding delayed refs/delayed head structures which deal with 
modification of the extent tree. Currently the logic for init and add was 
contained in one function for each type of structure. This resulted in very 
awkward interface with shitloads of arguments, this in turn made it really hard
to understand the gist of the code. This series rectifies the situation by 
extracting common parts and using those rather than open coding duplicated
logic for every type of delayed ref (tree or data ref). 

The first 5 patches deal with the delayed_ref structs. Each patch is 
incremental and makes the code bisectable. The last tree factor out the init 
code for delayed_ref_head into a separate function and begin to use it. The 
final completely splits the two. 

The net result is both a cleaner interface as well as somewhat reduced 
critical section under delayed_refs->lock spinlock. 

V2: Rebased the patches on latest misc-next.

Nikolay Borisov (8):
  btrfs: Factor out common delayed refs init code
  btrfs: Use init_delayed_ref_common in add_delayed_tree_ref
  btrfs: Use init_delayed_ref_common in add_delayed_data_ref
  btrfs: Open-code add_delayed_tree_ref
  btrfs: Open-code add_delayed_data_ref
  btrfs: Introduce init_delayed_ref_head
  btrfs: Use init_delayed_ref_head in add_delayed_ref_head
  btrfs: split delayed ref head initialization and addition

 fs/btrfs/delayed-ref.c | 248 ++++++++++++++++++++++++-------------------------
 1 file changed, 121 insertions(+), 127 deletions(-)

-- 
2.7.4

[PATCH v2 1/8] btrfs: Factor out common delayed refs init code

[Nikolay Borisov]

THe majority of the init code for struct btrfs_delayed_ref_node is
duplicated in add_delayed_data_ref and add_delayed_tree_ref. Factor
out the common bits in init_delayed_ref_common. This function is
going to be used in future patches to clean that up. No functional
changes

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/delayed-ref.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 4fb041e14742..a0dc255792c7 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -645,6 +645,57 @@ add_delayed_ref_head(struct btrfs_trans_handle *trans,
 }
 
 /*
+ * init_delayed_ref_common - Initialize the structure which represents a
+ *			     modification to a an extent.
+ *
+ * @fs_info:    Internal to the mounted filesystem mount structure.
+ *
+ * @ref:	The structure which is going to be initialized.
+ *
+ * @bytenr:	The logical address of the extent for which a modification is
+ *		going to be recorded.
+ *
+ * @num_bytes:  Size of the extent whose modification is being recorded.
+ *
+ * @ref_root:	The id of the root where this modification has originated, this
+ *		can be either one of the well-known metadata trees or the
+ *		subvolume id which references this extent.
+ *
+ * @action:	Can be one of BTRFS_ADD_DELAYED_REF/BTRFS_DROP_DELAYED_REF or
+ *		BTRFS_ADD_DELAYED_EXTENT
+ *
+ * @ref_type:	Holds the type of the extent which is being recorded, can be
+ *		one of BTRFS_SHARED_BLOCK_REF_KEY/BTRFS_TREE_BLOCK_REF_KEY
+ *		when recording a metadata extent or BTRFS_SHARED_DATA_REF_KEY/
+ *		BTRFS_EXTENT_DATA_REF_KEY when recording data extent
+ */
+static void init_delayed_ref_common(struct btrfs_fs_info *fs_info,
+				    struct btrfs_delayed_ref_node *ref,
+				    u64 bytenr, u64 num_bytes, u64 ref_root,
+				    int action, u8 ref_type)
+{
+	u64 seq = 0;
+
+	if (action == BTRFS_ADD_DELAYED_EXTENT)
+		action = BTRFS_ADD_DELAYED_REF;
+
+	if (is_fstree(ref_root))
+		seq = atomic64_read(&fs_info->tree_mod_seq);
+
+	refcount_set(&ref->refs, 1);
+	ref->bytenr = bytenr;
+	ref->num_bytes = num_bytes;
+	ref->ref_mod = 1;
+	ref->action = action;
+	ref->is_head = 0;
+	ref->in_tree = 1;
+	ref->seq = seq;
+	ref->type = ref_type;
+	RB_CLEAR_NODE(&ref->ref_node);
+	INIT_LIST_HEAD(&ref->add_list);
+}
+
+/*
  * helper to insert a delayed tree ref into the rbtree.
  */
 static noinline void
-- 
2.7.4

[PATCH v2 2/8] btrfs: Use init_delayed_ref_common in add_delayed_tree_ref

[Nikolay Borisov]

Use the newly introduced common helper.  No functional changes

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/delayed-ref.c | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index a0dc255792c7..1c27d3322198 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -708,38 +708,25 @@ add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
 {
 	struct btrfs_delayed_tree_ref *full_ref;
 	struct btrfs_delayed_ref_root *delayed_refs;
-	u64 seq = 0;
+	u8 ref_type;
 	int ret;
 
-	if (action == BTRFS_ADD_DELAYED_EXTENT)
-		action = BTRFS_ADD_DELAYED_REF;
-
-	if (is_fstree(ref_root))
-		seq = atomic64_read(&fs_info->tree_mod_seq);
 	delayed_refs = &trans->transaction->delayed_refs;
-
-	/* first set the basic ref node struct up */
-	refcount_set(&ref->refs, 1);
-	ref->bytenr = bytenr;
-	ref->num_bytes = num_bytes;
-	ref->ref_mod = 1;
-	ref->action = action;
-	ref->is_head = 0;
-	ref->in_tree = 1;
-	ref->seq = seq;
-	RB_CLEAR_NODE(&ref->ref_node);
-	INIT_LIST_HEAD(&ref->add_list);
-
 	full_ref = btrfs_delayed_node_to_tree_ref(ref);
-	full_ref->parent = parent;
-	full_ref->root = ref_root;
 	if (parent)
-		ref->type = BTRFS_SHARED_BLOCK_REF_KEY;
+	        ref_type = BTRFS_SHARED_BLOCK_REF_KEY;
 	else
-		ref->type = BTRFS_TREE_BLOCK_REF_KEY;
+	        ref_type = BTRFS_TREE_BLOCK_REF_KEY;
+
+	init_delayed_ref_common(fs_info, ref, bytenr, num_bytes, ref_root,
+				action, ref_type);
+	full_ref->root = ref_root;
+	full_ref->parent = parent;
 	full_ref->level = level;
 
-	trace_add_delayed_tree_ref(fs_info, ref, full_ref, action);
+	trace_add_delayed_tree_ref(fs_info, ref, full_ref,
+				   action == BTRFS_ADD_DELAYED_EXTENT ?
+				   BTRFS_ADD_DELAYED_REF : action);
 
 	ret = insert_delayed_ref(trans, delayed_refs, head_ref, ref);
 
-- 
2.7.4

[PATCH v2 3/8] btrfs: Use init_delayed_ref_common in add_delayed_data_ref

[Nikolay Borisov]

Use the newly introduced helper and remove the duplicate code.
No functional changes

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/delayed-ref.c | 34 ++++++++++------------------------
 1 file changed, 10 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 1c27d3322198..3ab2ba94d6f4 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -750,41 +750,27 @@ add_delayed_data_ref(struct btrfs_trans_handle *trans,
 {
 	struct btrfs_delayed_data_ref *full_ref;
 	struct btrfs_delayed_ref_root *delayed_refs;
-	u64 seq = 0;
+	u8 ref_type;
 	int ret;
 
-	if (action == BTRFS_ADD_DELAYED_EXTENT)
-		action = BTRFS_ADD_DELAYED_REF;
 
 	delayed_refs = &trans->transaction->delayed_refs;
-
-	if (is_fstree(ref_root))
-		seq = atomic64_read(&trans->fs_info->tree_mod_seq);
-
-	/* first set the basic ref node struct up */
-	refcount_set(&ref->refs, 1);
-	ref->bytenr = bytenr;
-	ref->num_bytes = num_bytes;
-	ref->ref_mod = 1;
-	ref->action = action;
-	ref->is_head = 0;
-	ref->in_tree = 1;
-	ref->seq = seq;
-	RB_CLEAR_NODE(&ref->ref_node);
-	INIT_LIST_HEAD(&ref->add_list);
-
 	full_ref = btrfs_delayed_node_to_data_ref(ref);
-	full_ref->parent = parent;
-	full_ref->root = ref_root;
 	if (parent)
-		ref->type = BTRFS_SHARED_DATA_REF_KEY;
+	        ref_type = BTRFS_SHARED_DATA_REF_KEY;
 	else
-		ref->type = BTRFS_EXTENT_DATA_REF_KEY;
+	        ref_type = BTRFS_EXTENT_DATA_REF_KEY;
 
+	init_delayed_ref_common(trans->fs_info, ref, bytenr, num_bytes,
+				ref_root, action, ref_type);
+	full_ref->root = ref_root;
+	full_ref->parent = parent;
 	full_ref->objectid = owner;
 	full_ref->offset = offset;
 
-	trace_add_delayed_data_ref(trans->fs_info, ref, full_ref, action);
+	trace_add_delayed_data_ref(trans->fs_info, ref, full_ref,
+				   action == BTRFS_ADD_DELAYED_EXTENT ?
+				   BTRFS_ADD_DELAYED_REF : action);
 
 	ret = insert_delayed_ref(trans, delayed_refs, head_ref, ref);
 	if (ret > 0)
-- 
2.7.4

[PATCH v2 4/8] btrfs: Open-code add_delayed_tree_ref

[Nikolay Borisov]

Now that the initialization part and the critical section code have
been split it's a lot easier to open code add_delayed_tree_ref. Do
so in the following manner:

1. The commin init code is put immediately after memory-to-be-init is
allocate, followed by the ref-specific member initialization.

2. The only piece of code that remains in the critical section is
insert_delayed_ref call.

3. Tracing and memory freeing code is put outside of the critical
section as well.

The only real change here is an overall shorter critical section when
dealing with delayed tree refs. From functional point of view - the
code is unchanged.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/delayed-ref.c | 66 ++++++++++++++++----------------------------------
 1 file changed, 21 insertions(+), 45 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 3ab2ba94d6f4..827ca01cf5af 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -696,49 +696,6 @@ static void init_delayed_ref_common(struct btrfs_fs_info *fs_info,
 }
 
 /*
- * helper to insert a delayed tree ref into the rbtree.
- */
-static noinline void
-add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
-		     struct btrfs_trans_handle *trans,
-		     struct btrfs_delayed_ref_head *head_ref,
-		     struct btrfs_delayed_ref_node *ref, u64 bytenr,
-		     u64 num_bytes, u64 parent, u64 ref_root, int level,
-		     int action)
-{
-	struct btrfs_delayed_tree_ref *full_ref;
-	struct btrfs_delayed_ref_root *delayed_refs;
-	u8 ref_type;
-	int ret;
-
-	delayed_refs = &trans->transaction->delayed_refs;
-	full_ref = btrfs_delayed_node_to_tree_ref(ref);
-	if (parent)
-	        ref_type = BTRFS_SHARED_BLOCK_REF_KEY;
-	else
-	        ref_type = BTRFS_TREE_BLOCK_REF_KEY;
-
-	init_delayed_ref_common(fs_info, ref, bytenr, num_bytes, ref_root,
-				action, ref_type);
-	full_ref->root = ref_root;
-	full_ref->parent = parent;
-	full_ref->level = level;
-
-	trace_add_delayed_tree_ref(fs_info, ref, full_ref,
-				   action == BTRFS_ADD_DELAYED_EXTENT ?
-				   BTRFS_ADD_DELAYED_REF : action);
-
-	ret = insert_delayed_ref(trans, delayed_refs, head_ref, ref);
-
-	/*
-	 * XXX: memory should be freed at the same level allocated.
-	 * But bad practice is anywhere... Follow it now. Need cleanup.
-	 */
-	if (ret > 0)
-		kmem_cache_free(btrfs_delayed_tree_ref_cachep, full_ref);
-}
-
-/*
  * helper to insert a delayed data ref into the rbtree.
  */
 static noinline void
@@ -795,12 +752,25 @@ int btrfs_add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
 	struct btrfs_qgroup_extent_record *record = NULL;
 	int qrecord_inserted;
 	int is_system = (ref_root == BTRFS_CHUNK_TREE_OBJECTID);
+	int ret;
+	u8 ref_type = parent ? BTRFS_SHARED_BLOCK_REF_KEY :
+		BTRFS_TREE_BLOCK_REF_KEY;
 
 	BUG_ON(extent_op && extent_op->is_data);
 	ref = kmem_cache_alloc(btrfs_delayed_tree_ref_cachep, GFP_NOFS);
 	if (!ref)
 		return -ENOMEM;
 
+	if (parent)
+		ref_type = BTRFS_SHARED_BLOCK_REF_KEY;
+	else
+		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;
+	ref->parent = parent;
+	ref->level = level;
+
 	head_ref = kmem_cache_alloc(btrfs_delayed_ref_head_cachep, GFP_NOFS);
 	if (!head_ref)
 		goto free_ref;
@@ -826,10 +796,16 @@ int btrfs_add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
 					is_system, &qrecord_inserted,
 					old_ref_mod, new_ref_mod);
 
-	add_delayed_tree_ref(fs_info, trans, head_ref, &ref->node, bytenr,
-			     num_bytes, parent, ref_root, level, action);
+
+	ret = insert_delayed_ref(trans, delayed_refs, head_ref, &ref->node);
 	spin_unlock(&delayed_refs->lock);
 
+	trace_add_delayed_tree_ref(fs_info, &ref->node, ref,
+				   action == BTRFS_ADD_DELAYED_EXTENT ?
+				   BTRFS_ADD_DELAYED_REF : action);
+	if (ret > 0)
+		kmem_cache_free(btrfs_delayed_tree_ref_cachep, ref);
+
 	if (qrecord_inserted)
 		btrfs_qgroup_trace_extent_post(fs_info, record);
 
-- 
2.7.4

[PATCH v2 5/8] btrfs: Open-code add_delayed_data_ref

[Nikolay Borisov]

Now that the initialization part and the critical section code have
been split it's a lot easier to open code add_delayed_data_ref. Do
so in the following manner:

1. The common init function is put immediately after memory-to-be-init
is allocated, followed by the specific data ref initialization.

2. The only piece of code that remains in the critical section is
insert_delayed_ref call.

3. Tracing and memory freeing code is moved outside of the critical
section.

No functional changes, just an overall shorter critical section.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/delayed-ref.c | 63 ++++++++++++++++++--------------------------------
 1 file changed, 22 insertions(+), 41 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 827ca01cf5af..2d1375739b3d 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -696,45 +696,6 @@ static void init_delayed_ref_common(struct btrfs_fs_info *fs_info,
 }
 
 /*
- * helper to insert a delayed data ref into the rbtree.
- */
-static noinline void
-add_delayed_data_ref(struct btrfs_trans_handle *trans,
-		     struct btrfs_delayed_ref_head *head_ref,
-		     struct btrfs_delayed_ref_node *ref, u64 bytenr,
-		     u64 num_bytes, u64 parent, u64 ref_root, u64 owner,
-		     u64 offset, int action)
-{
-	struct btrfs_delayed_data_ref *full_ref;
-	struct btrfs_delayed_ref_root *delayed_refs;
-	u8 ref_type;
-	int ret;
-
-
-	delayed_refs = &trans->transaction->delayed_refs;
-	full_ref = btrfs_delayed_node_to_data_ref(ref);
-	if (parent)
-	        ref_type = BTRFS_SHARED_DATA_REF_KEY;
-	else
-	        ref_type = BTRFS_EXTENT_DATA_REF_KEY;
-
-	init_delayed_ref_common(trans->fs_info, ref, bytenr, num_bytes,
-				ref_root, action, ref_type);
-	full_ref->root = ref_root;
-	full_ref->parent = parent;
-	full_ref->objectid = owner;
-	full_ref->offset = offset;
-
-	trace_add_delayed_data_ref(trans->fs_info, ref, full_ref,
-				   action == BTRFS_ADD_DELAYED_EXTENT ?
-				   BTRFS_ADD_DELAYED_REF : action);
-
-	ret = insert_delayed_ref(trans, delayed_refs, head_ref, ref);
-	if (ret > 0)
-		kmem_cache_free(btrfs_delayed_data_ref_cachep, full_ref);
-}
-
-/*
  * add a delayed tree ref.  This does all of the accounting required
  * to make sure the delayed ref is eventually processed before this
  * transaction commits.
@@ -834,11 +795,25 @@ int btrfs_add_delayed_data_ref(struct btrfs_fs_info *fs_info,
 	struct btrfs_delayed_ref_root *delayed_refs;
 	struct btrfs_qgroup_extent_record *record = NULL;
 	int qrecord_inserted;
+	int ret;
+	u8 ref_type;
 
 	ref = kmem_cache_alloc(btrfs_delayed_data_ref_cachep, GFP_NOFS);
 	if (!ref)
 		return -ENOMEM;
 
+	if (parent)
+	        ref_type = BTRFS_SHARED_DATA_REF_KEY;
+	else
+	        ref_type = BTRFS_EXTENT_DATA_REF_KEY;
+	init_delayed_ref_common(fs_info, &ref->node, bytenr, num_bytes,
+				ref_root, action, ref_type);
+	ref->root = ref_root;
+	ref->parent = parent;
+	ref->objectid = owner;
+	ref->offset = offset;
+
+
 	head_ref = kmem_cache_alloc(btrfs_delayed_ref_head_cachep, GFP_NOFS);
 	if (!head_ref) {
 		kmem_cache_free(btrfs_delayed_data_ref_cachep, ref);
@@ -870,10 +845,16 @@ int btrfs_add_delayed_data_ref(struct btrfs_fs_info *fs_info,
 					action, 1, 0, &qrecord_inserted,
 					old_ref_mod, new_ref_mod);
 
-	add_delayed_data_ref(trans, head_ref, &ref->node, bytenr, num_bytes,
-			     parent, ref_root, owner, offset, action);
+	ret = insert_delayed_ref(trans, delayed_refs, head_ref, &ref->node);
 	spin_unlock(&delayed_refs->lock);
 
+	trace_add_delayed_data_ref(trans->fs_info, &ref->node, ref,
+				   action == BTRFS_ADD_DELAYED_EXTENT ?
+				   BTRFS_ADD_DELAYED_REF : action);
+	if (ret > 0)
+		kmem_cache_free(btrfs_delayed_data_ref_cachep, ref);
+
+
 	if (qrecord_inserted)
 		return btrfs_qgroup_trace_extent_post(fs_info, record);
 	return 0;
-- 
2.7.4

[PATCH v2 6/8] btrfs: Introduce init_delayed_ref_head

[Nikolay Borisov]

add_delayed_ref_head implements the logic to both initialize a head_ref
structure as well as perform the necessary operations to add it to the
delayed ref machinery. This has resulted in a very cumebrsome interface
with loads of parameters and code, which at first glance, looks very
unwieldy. Begin untangling it by first extracting the initialization
only code in its own function. It's more or less verbatim copy of the
first part of add_delayed_ref_head.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/delayed-ref.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 2d1375739b3d..39481a0f6834 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -526,6 +526,76 @@ update_existing_head_ref(struct btrfs_delayed_ref_root *delayed_refs,
 	spin_unlock(&existing->lock);
 }
 
+
+static void init_delayed_ref_head(struct btrfs_delayed_ref_head *head_ref,
+				  struct btrfs_qgroup_extent_record *qrecord,
+				  u64 bytenr, u64 num_bytes, u64 ref_root,
+				  u64 reserved, int action, bool is_data,
+				  bool is_system)
+{
+
+	int count_mod = 1;
+	int must_insert_reserved = 0;
+
+	/* If reserved is provided, it must be a data extent. */
+	BUG_ON(!is_data && reserved);
+
+	/*
+	 * the head node stores the sum of all the mods, so dropping a ref
+	 * should drop the sum in the head node by one.
+	 */
+	if (action == BTRFS_UPDATE_DELAYED_HEAD)
+		count_mod = 0;
+	else if (action == BTRFS_DROP_DELAYED_REF)
+		count_mod = -1;
+
+	/*
+	 * BTRFS_ADD_DELAYED_EXTENT means that we need to update
+	 * the reserved accounting when the extent is finally added, or
+	 * if a later modification deletes the delayed ref without ever
+	 * inserting the extent into the extent allocation tree.
+	 * ref->must_insert_reserved is the flag used to record
+	 * that accounting mods are required.
+	 *
+	 * Once we record must_insert_reserved, switch the action to
+	 * BTRFS_ADD_DELAYED_REF because other special casing is not required.
+	 */
+	if (action == BTRFS_ADD_DELAYED_EXTENT)
+		must_insert_reserved = 1;
+	else
+		must_insert_reserved = 0;
+
+
+	refcount_set(&head_ref->refs, 1);
+	head_ref->bytenr = bytenr;
+	head_ref->num_bytes = num_bytes;
+	head_ref->ref_mod = count_mod;
+	head_ref->must_insert_reserved = must_insert_reserved;
+	head_ref->is_data = is_data;
+	head_ref->is_system = is_system;
+	head_ref->ref_tree = RB_ROOT;
+	INIT_LIST_HEAD(&head_ref->ref_add_list);
+	RB_CLEAR_NODE(&head_ref->href_node);
+	head_ref->processing = 0;
+	head_ref->total_ref_mod = count_mod;
+	head_ref->qgroup_reserved = 0;
+	head_ref->qgroup_ref_root = 0;
+	spin_lock_init(&head_ref->lock);
+	mutex_init(&head_ref->mutex);
+
+
+	if (qrecord) {
+		if (ref_root && reserved) {
+			head_ref->qgroup_ref_root = ref_root;
+			head_ref->qgroup_reserved = reserved;
+		}
+
+		qrecord->bytenr = bytenr;
+		qrecord->num_bytes = num_bytes;
+		qrecord->old_roots = NULL;
+	}
+}
+
 /*
  * helper function to actually insert a head node into the rbtree.
  * this does all the dirty work in terms of maintaining the correct
-- 
2.7.4

[PATCH v2 7/8] btrfs: Use init_delayed_ref_head in add_delayed_ref_head

[Nikolay Borisov]

Use the newly introduced function when initialising the head_ref in
add_delayed_ref_head. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/delayed-ref.c | 63 ++++----------------------------------------------
 1 file changed, 4 insertions(+), 59 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 39481a0f6834..b4b809511fce 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -613,69 +613,14 @@ add_delayed_ref_head(struct btrfs_trans_handle *trans,
 {
 	struct btrfs_delayed_ref_head *existing;
 	struct btrfs_delayed_ref_root *delayed_refs;
-	int count_mod = 1;
-	int must_insert_reserved = 0;
 	int qrecord_inserted = 0;
 
-	/* If reserved is provided, it must be a data extent. */
-	BUG_ON(!is_data && reserved);
-
-	/*
-	 * the head node stores the sum of all the mods, so dropping a ref
-	 * should drop the sum in the head node by one.
-	 */
-	if (action == BTRFS_UPDATE_DELAYED_HEAD)
-		count_mod = 0;
-	else if (action == BTRFS_DROP_DELAYED_REF)
-		count_mod = -1;
-
-	/*
-	 * BTRFS_ADD_DELAYED_EXTENT means that we need to update
-	 * the reserved accounting when the extent is finally added, or
-	 * if a later modification deletes the delayed ref without ever
-	 * inserting the extent into the extent allocation tree.
-	 * ref->must_insert_reserved is the flag used to record
-	 * that accounting mods are required.
-	 *
-	 * Once we record must_insert_reserved, switch the action to
-	 * BTRFS_ADD_DELAYED_REF because other special casing is not required.
-	 */
-	if (action == BTRFS_ADD_DELAYED_EXTENT)
-		must_insert_reserved = 1;
-	else
-		must_insert_reserved = 0;
-
 	delayed_refs = &trans->transaction->delayed_refs;
-
-	refcount_set(&head_ref->refs, 1);
-	head_ref->bytenr = bytenr;
-	head_ref->num_bytes = num_bytes;
-	head_ref->ref_mod = count_mod;
-	head_ref->must_insert_reserved = must_insert_reserved;
-	head_ref->is_data = is_data;
-	head_ref->is_system = is_system;
-	head_ref->ref_tree = RB_ROOT;
-	INIT_LIST_HEAD(&head_ref->ref_add_list);
-	RB_CLEAR_NODE(&head_ref->href_node);
-	head_ref->processing = 0;
-	head_ref->total_ref_mod = count_mod;
-	head_ref->qgroup_reserved = 0;
-	head_ref->qgroup_ref_root = 0;
-	spin_lock_init(&head_ref->lock);
-	mutex_init(&head_ref->mutex);
-
+	init_delayed_ref_head(head_ref, qrecord, bytenr, num_bytes, ref_root,
+			      reserved, action, is_data, is_system);
 	/* Record qgroup extent info if provided */
 	if (qrecord) {
-		if (ref_root && reserved) {
-			head_ref->qgroup_ref_root = ref_root;
-			head_ref->qgroup_reserved = reserved;
-		}
-
-		qrecord->bytenr = bytenr;
-		qrecord->num_bytes = num_bytes;
-		qrecord->old_roots = NULL;
-
-		if(btrfs_qgroup_trace_extent_nolock(trans->fs_info,
+		if (btrfs_qgroup_trace_extent_nolock(trans->fs_info,
 					delayed_refs, qrecord))
 			kfree(qrecord);
 		else
@@ -700,7 +645,7 @@ add_delayed_ref_head(struct btrfs_trans_handle *trans,
 	} else {
 		if (old_ref_mod)
 			*old_ref_mod = 0;
-		if (is_data && count_mod < 0)
+		if (is_data && head_ref->ref_mod < 0)
 			delayed_refs->pending_csums += num_bytes;
 		delayed_refs->num_heads++;
 		delayed_refs->num_heads_ready++;
-- 
2.7.4

[PATCH v2 8/8] btrfs: split delayed ref head initialization and addition

[Nikolay Borisov]

add_delayed_ref_head really performed 2 independent operations -
initialisting the ref head and adding it to a list. Now that the init
part is in a separate function let's complete the separation between
both operations. This results in a lot simpler interface for
add_delayed_ref_head since the function now deals solely with either
adding the newly initialised delayed ref head or merging it into an
existing delayed ref head. This results in vastly simplified function
signature since 5 arguments are dropped. The only other thing worth
mentioning is that due to this split the WARN_ON catching reinit of
existing. In this patch the condition is extended such that:

  qrecord && head_ref->qgroup_ref_root && head_ref->qgroup_reserved

is added. This is done because the two qgroup_* prefixed member are
set only if both ref_root and reserved are passed. So functionally
it's equivalent to the old WARN_ON and allows to remove the two args
from add_delayed_ref_head.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/delayed-ref.c | 46 ++++++++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index b4b809511fce..8f799d18315b 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -605,9 +605,7 @@ static noinline struct btrfs_delayed_ref_head *
 add_delayed_ref_head(struct btrfs_trans_handle *trans,
 		     struct btrfs_delayed_ref_head *head_ref,
 		     struct btrfs_qgroup_extent_record *qrecord,
-		     u64 bytenr, u64 num_bytes, u64 ref_root, u64 reserved,
-		     int action, int is_data, int is_system,
-		     int *qrecord_inserted_ret,
+		     int action, int *qrecord_inserted_ret,
 		     int *old_ref_mod, int *new_ref_mod)
 
 {
@@ -616,8 +614,7 @@ add_delayed_ref_head(struct btrfs_trans_handle *trans,
 	int qrecord_inserted = 0;
 
 	delayed_refs = &trans->transaction->delayed_refs;
-	init_delayed_ref_head(head_ref, qrecord, bytenr, num_bytes, ref_root,
-			      reserved, action, is_data, is_system);
+
 	/* Record qgroup extent info if provided */
 	if (qrecord) {
 		if (btrfs_qgroup_trace_extent_nolock(trans->fs_info,
@@ -632,7 +629,9 @@ add_delayed_ref_head(struct btrfs_trans_handle *trans,
 	existing = htree_insert(&delayed_refs->href_root,
 				&head_ref->href_node);
 	if (existing) {
-		WARN_ON(ref_root && reserved && existing->qgroup_ref_root
+		WARN_ON(qrecord && head_ref->qgroup_ref_root
+			&& head_ref->qgroup_reserved
+			&& existing->qgroup_ref_root
 			&& existing->qgroup_reserved);
 		update_existing_head_ref(delayed_refs, existing, head_ref,
 					 old_ref_mod);
@@ -645,8 +644,8 @@ add_delayed_ref_head(struct btrfs_trans_handle *trans,
 	} else {
 		if (old_ref_mod)
 			*old_ref_mod = 0;
-		if (is_data && head_ref->ref_mod < 0)
-			delayed_refs->pending_csums += num_bytes;
+		if (head_ref->is_data && head_ref->ref_mod < 0)
+			delayed_refs->pending_csums += head_ref->num_bytes;
 		delayed_refs->num_heads++;
 		delayed_refs->num_heads_ready++;
 		atomic_inc(&delayed_refs->num_entries);
@@ -656,6 +655,7 @@ add_delayed_ref_head(struct btrfs_trans_handle *trans,
 		*qrecord_inserted_ret = qrecord_inserted;
 	if (new_ref_mod)
 		*new_ref_mod = head_ref->total_ref_mod;
+
 	return head_ref;
 }
 
@@ -727,7 +727,7 @@ int btrfs_add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
 	struct btrfs_delayed_ref_root *delayed_refs;
 	struct btrfs_qgroup_extent_record *record = NULL;
 	int qrecord_inserted;
-	int is_system = (ref_root == BTRFS_CHUNK_TREE_OBJECTID);
+	bool is_system = (ref_root == BTRFS_CHUNK_TREE_OBJECTID);
 	int ret;
 	u8 ref_type = parent ? BTRFS_SHARED_BLOCK_REF_KEY :
 		BTRFS_TREE_BLOCK_REF_KEY;
@@ -758,6 +758,8 @@ int btrfs_add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
 			goto free_head_ref;
 	}
 
+	init_delayed_ref_head(head_ref, record, bytenr, num_bytes,
+			      ref_root, 0, action, false, is_system);
 	head_ref->extent_op = extent_op;
 
 	delayed_refs = &trans->transaction->delayed_refs;
@@ -767,12 +769,10 @@ int btrfs_add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
 	 * insert both the head node and the new ref without dropping
 	 * the spin lock
 	 */
-	head_ref = add_delayed_ref_head(trans, head_ref, record, bytenr,
-					num_bytes, 0, 0, action, 0,
-					is_system, &qrecord_inserted,
+	head_ref = add_delayed_ref_head(trans, head_ref, record,
+					action, &qrecord_inserted,
 					old_ref_mod, new_ref_mod);
 
-
 	ret = insert_delayed_ref(trans, delayed_refs, head_ref, &ref->node);
 	spin_unlock(&delayed_refs->lock);
 
@@ -846,6 +846,8 @@ int btrfs_add_delayed_data_ref(struct btrfs_fs_info *fs_info,
 		}
 	}
 
+	init_delayed_ref_head(head_ref, record, bytenr, num_bytes, ref_root,
+			      reserved, action, true, false);
 	head_ref->extent_op = NULL;
 
 	delayed_refs = &trans->transaction->delayed_refs;
@@ -855,9 +857,8 @@ int btrfs_add_delayed_data_ref(struct btrfs_fs_info *fs_info,
 	 * insert both the head node and the new ref without dropping
 	 * the spin lock
 	 */
-	head_ref = add_delayed_ref_head(trans, head_ref, record, bytenr,
-					num_bytes, ref_root, reserved,
-					action, 1, 0, &qrecord_inserted,
+	head_ref = add_delayed_ref_head(trans, head_ref, record,
+					action, &qrecord_inserted,
 					old_ref_mod, new_ref_mod);
 
 	ret = insert_delayed_ref(trans, delayed_refs, head_ref, &ref->node);
@@ -887,19 +888,16 @@ int btrfs_add_delayed_extent_op(struct btrfs_fs_info *fs_info,
 	if (!head_ref)
 		return -ENOMEM;
 
+	init_delayed_ref_head(head_ref, NULL, bytenr, num_bytes, 0, 0,
+			      BTRFS_UPDATE_DELAYED_HEAD, extent_op->is_data,
+			      false);
 	head_ref->extent_op = extent_op;
 
 	delayed_refs = &trans->transaction->delayed_refs;
 	spin_lock(&delayed_refs->lock);
 
-	/*
-	 * extent_ops just modify the flags of an extent and they don't result
-	 * in ref count changes, hence it's safe to pass false/0 for is_system
-	 * argument
-	 */
-	add_delayed_ref_head(trans, head_ref, NULL, bytenr, num_bytes, 0, 0,
-			     BTRFS_UPDATE_DELAYED_HEAD, extent_op->is_data,
-			     0, NULL, NULL, NULL);
+	add_delayed_ref_head(trans, head_ref, NULL, BTRFS_UPDATE_DELAYED_HEAD,
+			     NULL, NULL, NULL);
 
 	spin_unlock(&delayed_refs->lock);
 	return 0;
-- 
2.7.4

Re: [PATCH v2 0/8] Delayed refs cleanups/streamlining

[David Sterba]

On Tue, Apr 24, 2018 at 05:18:16PM +0300, Nikolay Borisov wrote:
> This patchkit aims to simplify and streamline the logic involved in 
> initialising and adding delayed refs/delayed head structures which deal with 
> modification of the extent tree. Currently the logic for init and add was 
> contained in one function for each type of structure. This resulted in very 
> awkward interface with shitloads of arguments, this in turn made it really hard
> to understand the gist of the code. This series rectifies the situation by 
> extracting common parts and using those rather than open coding duplicated
> logic for every type of delayed ref (tree or data ref). 
> 
> The first 5 patches deal with the delayed_ref structs. Each patch is 
> incremental and makes the code bisectable. The last tree factor out the init 
> code for delayed_ref_head into a separate function and begin to use it. The 
> final completely splits the two. 
> 
> The net result is both a cleaner interface as well as somewhat reduced 
> critical section under delayed_refs->lock spinlock. 
> 
> V2: Rebased the patches on latest misc-next.
> 
> Nikolay Borisov (8):
>   btrfs: Factor out common delayed refs init code
>   btrfs: Use init_delayed_ref_common in add_delayed_tree_ref
>   btrfs: Use init_delayed_ref_common in add_delayed_data_ref
>   btrfs: Open-code add_delayed_tree_ref
>   btrfs: Open-code add_delayed_data_ref
>   btrfs: Introduce init_delayed_ref_head
>   btrfs: Use init_delayed_ref_head in add_delayed_ref_head
>   btrfs: split delayed ref head initialization and addition

Reviewed-by: David Sterba <dsterba@suse.com>

On the way to misc-next, it's been in next for quite some time.

Re: [PATCH v2 4/8] btrfs: Open-code add_delayed_tree_ref

[Nikolay Borisov]

On 24.04.2018 17:18, Nikolay Borisov wrote:
> Now that the initialization part and the critical section code have
> been split it's a lot easier to open code add_delayed_tree_ref. Do
> so in the following manner:
> 
> 1. The commin init code is put immediately after memory-to-be-init is
> allocate, followed by the ref-specific member initialization.
> 
> 2. The only piece of code that remains in the critical section is
> insert_delayed_ref call.
> 
> 3. Tracing and memory freeing code is put outside of the critical
> section as well.
> 
> The only real change here is an overall shorter critical section when
> dealing with delayed tree refs. From functional point of view - the
> code is unchanged.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/delayed-ref.c | 66 ++++++++++++++++----------------------------------
>  1 file changed, 21 insertions(+), 45 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index 3ab2ba94d6f4..827ca01cf5af 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -696,49 +696,6 @@ static void init_delayed_ref_common(struct btrfs_fs_info *fs_info,
>  }
>  
>  /*
> - * helper to insert a delayed tree ref into the rbtree.
> - */
> -static noinline void
> -add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
> -		     struct btrfs_trans_handle *trans,
> -		     struct btrfs_delayed_ref_head *head_ref,
> -		     struct btrfs_delayed_ref_node *ref, u64 bytenr,
> -		     u64 num_bytes, u64 parent, u64 ref_root, int level,
> -		     int action)
> -{
> -	struct btrfs_delayed_tree_ref *full_ref;
> -	struct btrfs_delayed_ref_root *delayed_refs;
> -	u8 ref_type;
> -	int ret;
> -
> -	delayed_refs = &trans->transaction->delayed_refs;
> -	full_ref = btrfs_delayed_node_to_tree_ref(ref);
> -	if (parent)
> -	        ref_type = BTRFS_SHARED_BLOCK_REF_KEY;
> -	else
> -	        ref_type = BTRFS_TREE_BLOCK_REF_KEY;
> -
> -	init_delayed_ref_common(fs_info, ref, bytenr, num_bytes, ref_root,
> -				action, ref_type);
> -	full_ref->root = ref_root;
> -	full_ref->parent = parent;
> -	full_ref->level = level;
> -
> -	trace_add_delayed_tree_ref(fs_info, ref, full_ref,
> -				   action == BTRFS_ADD_DELAYED_EXTENT ?
> -				   BTRFS_ADD_DELAYED_REF : action);
> -
> -	ret = insert_delayed_ref(trans, delayed_refs, head_ref, ref);
> -
> -	/*
> -	 * XXX: memory should be freed at the same level allocated.
> -	 * But bad practice is anywhere... Follow it now. Need cleanup.
> -	 */
> -	if (ret > 0)
> -		kmem_cache_free(btrfs_delayed_tree_ref_cachep, full_ref);
> -}
> -
> -/*
>   * helper to insert a delayed data ref into the rbtree.
>   */
>  static noinline void
> @@ -795,12 +752,25 @@ int btrfs_add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
>  	struct btrfs_qgroup_extent_record *record = NULL;
>  	int qrecord_inserted;
>  	int is_system = (ref_root == BTRFS_CHUNK_TREE_OBJECTID);
> +	int ret;
> +	u8 ref_type = parent ? BTRFS_SHARED_BLOCK_REF_KEY :
> +		BTRFS_TREE_BLOCK_REF_KEY;
>  
>  	BUG_ON(extent_op && extent_op->is_data);
>  	ref = kmem_cache_alloc(btrfs_delayed_tree_ref_cachep, GFP_NOFS);
>  	if (!ref)
>  		return -ENOMEM;
>  
> +	if (parent)
> +		ref_type = BTRFS_SHARED_BLOCK_REF_KEY;
> +	else
> +		ref_type = BTRFS_TREE_BLOCK_REF_KEY;

Ooops, this if (parent) check is duplicated with the "parent ? " at the
beginning of the function. Would you care to delete either of them -
perhaps this one, or should I send a fixlet for you to squash ?

> +	init_delayed_ref_common(fs_info, &ref->node, bytenr, num_bytes,
> +				ref_root, action, ref_type);
> +	ref->root = ref_root;
> +	ref->parent = parent;
> +	ref->level = level;
> +
>  	head_ref = kmem_cache_alloc(btrfs_delayed_ref_head_cachep, GFP_NOFS);
>  	if (!head_ref)
>  		goto free_ref;
> @@ -826,10 +796,16 @@ int btrfs_add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
>  					is_system, &qrecord_inserted,
>  					old_ref_mod, new_ref_mod);
>  
> -	add_delayed_tree_ref(fs_info, trans, head_ref, &ref->node, bytenr,
> -			     num_bytes, parent, ref_root, level, action);
> +
> +	ret = insert_delayed_ref(trans, delayed_refs, head_ref, &ref->node);
>  	spin_unlock(&delayed_refs->lock);
>  
> +	trace_add_delayed_tree_ref(fs_info, &ref->node, ref,
> +				   action == BTRFS_ADD_DELAYED_EXTENT ?
> +				   BTRFS_ADD_DELAYED_REF : action);
> +	if (ret > 0)
> +		kmem_cache_free(btrfs_delayed_tree_ref_cachep, ref);
> +
>  	if (qrecord_inserted)
>  		btrfs_qgroup_trace_extent_post(fs_info, record);
>  
> 

Re: [PATCH v2 5/8] btrfs: Open-code add_delayed_data_ref

[Nikolay Borisov]

On 24.04.2018 17:18, Nikolay Borisov wrote:
> Now that the initialization part and the critical section code have
> been split it's a lot easier to open code add_delayed_data_ref. Do
> so in the following manner:
> 
> 1. The common init function is put immediately after memory-to-be-init
> is allocated, followed by the specific data ref initialization.
> 
> 2. The only piece of code that remains in the critical section is
> insert_delayed_ref call.
> 
> 3. Tracing and memory freeing code is moved outside of the critical
> section.
> 
> No functional changes, just an overall shorter critical section.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/delayed-ref.c | 63 ++++++++++++++++++--------------------------------
>  1 file changed, 22 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index 827ca01cf5af..2d1375739b3d 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -696,45 +696,6 @@ static void init_delayed_ref_common(struct btrfs_fs_info *fs_info,
>  }
>  
>  /*
> - * helper to insert a delayed data ref into the rbtree.
> - */
> -static noinline void
> -add_delayed_data_ref(struct btrfs_trans_handle *trans,
> -		     struct btrfs_delayed_ref_head *head_ref,
> -		     struct btrfs_delayed_ref_node *ref, u64 bytenr,
> -		     u64 num_bytes, u64 parent, u64 ref_root, u64 owner,
> -		     u64 offset, int action)
> -{
> -	struct btrfs_delayed_data_ref *full_ref;
> -	struct btrfs_delayed_ref_root *delayed_refs;
> -	u8 ref_type;
> -	int ret;
> -
> -
> -	delayed_refs = &trans->transaction->delayed_refs;
> -	full_ref = btrfs_delayed_node_to_data_ref(ref);
> -	if (parent)
> -	        ref_type = BTRFS_SHARED_DATA_REF_KEY;
> -	else
> -	        ref_type = BTRFS_EXTENT_DATA_REF_KEY;
> -
> -	init_delayed_ref_common(trans->fs_info, ref, bytenr, num_bytes,
> -				ref_root, action, ref_type);
> -	full_ref->root = ref_root;
> -	full_ref->parent = parent;
> -	full_ref->objectid = owner;
> -	full_ref->offset = offset;
> -
> -	trace_add_delayed_data_ref(trans->fs_info, ref, full_ref,
> -				   action == BTRFS_ADD_DELAYED_EXTENT ?
> -				   BTRFS_ADD_DELAYED_REF : action);
> -
> -	ret = insert_delayed_ref(trans, delayed_refs, head_ref, ref);
> -	if (ret > 0)
> -		kmem_cache_free(btrfs_delayed_data_ref_cachep, full_ref);
> -}
> -
> -/*
>   * add a delayed tree ref.  This does all of the accounting required
>   * to make sure the delayed ref is eventually processed before this
>   * transaction commits.
> @@ -834,11 +795,25 @@ int btrfs_add_delayed_data_ref(struct btrfs_fs_info *fs_info,
>  	struct btrfs_delayed_ref_root *delayed_refs;
>  	struct btrfs_qgroup_extent_record *record = NULL;
>  	int qrecord_inserted;
> +	int ret;
> +	u8 ref_type;
>  
>  	ref = kmem_cache_alloc(btrfs_delayed_data_ref_cachep, GFP_NOFS);
>  	if (!ref)
>  		return -ENOMEM;
>  
> +	if (parent)
> +	        ref_type = BTRFS_SHARED_DATA_REF_KEY;
> +	else
> +	        ref_type = BTRFS_EXTENT_DATA_REF_KEY;

Argh, here the check style is different than in add_delayed_tree_ref...
I guess in the previous patch it makes sense to delete the check at the
definition of ref_type.
> +	init_delayed_ref_common(fs_info, &ref->node, bytenr, num_bytes,
> +				ref_root, action, ref_type);
> +	ref->root = ref_root;
> +	ref->parent = parent;
> +	ref->objectid = owner;
> +	ref->offset = offset;
> +
> +
>  	head_ref = kmem_cache_alloc(btrfs_delayed_ref_head_cachep, GFP_NOFS);
>  	if (!head_ref) {
>  		kmem_cache_free(btrfs_delayed_data_ref_cachep, ref);
> @@ -870,10 +845,16 @@ int btrfs_add_delayed_data_ref(struct btrfs_fs_info *fs_info,
>  					action, 1, 0, &qrecord_inserted,
>  					old_ref_mod, new_ref_mod);
>  
> -	add_delayed_data_ref(trans, head_ref, &ref->node, bytenr, num_bytes,
> -			     parent, ref_root, owner, offset, action);
> +	ret = insert_delayed_ref(trans, delayed_refs, head_ref, &ref->node);
>  	spin_unlock(&delayed_refs->lock);
>  
> +	trace_add_delayed_data_ref(trans->fs_info, &ref->node, ref,
> +				   action == BTRFS_ADD_DELAYED_EXTENT ?
> +				   BTRFS_ADD_DELAYED_REF : action);
> +	if (ret > 0)
> +		kmem_cache_free(btrfs_delayed_data_ref_cachep, ref);
> +
> +
>  	if (qrecord_inserted)
>  		return btrfs_qgroup_trace_extent_post(fs_info, record);
>  	return 0;
> 

Re: [PATCH v2 4/8] btrfs: Open-code add_delayed_tree_ref

[David Sterba]

On Thu, May 10, 2018 at 04:55:24PM +0300, Nikolay Borisov wrote:
> > @@ -795,12 +752,25 @@ int btrfs_add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
> >  	struct btrfs_qgroup_extent_record *record = NULL;
> >  	int qrecord_inserted;
> >  	int is_system = (ref_root == BTRFS_CHUNK_TREE_OBJECTID);
> > +	int ret;
> > +	u8 ref_type = parent ? BTRFS_SHARED_BLOCK_REF_KEY :
> > +		BTRFS_TREE_BLOCK_REF_KEY;
> >  
> >  	BUG_ON(extent_op && extent_op->is_data);
> >  	ref = kmem_cache_alloc(btrfs_delayed_tree_ref_cachep, GFP_NOFS);
> >  	if (!ref)
> >  		return -ENOMEM;
> >  
> > +	if (parent)
> > +		ref_type = BTRFS_SHARED_BLOCK_REF_KEY;
> > +	else
> > +		ref_type = BTRFS_TREE_BLOCK_REF_KEY;
> 
> Ooops, this if (parent) check is duplicated with the "parent ? " at the
> beginning of the function. Would you care to delete either of them -
> perhaps this one, or should I send a fixlet for you to squash ?

I removed it.