[PATCH 0/4] Cleanup alloc_reserved_tree_block signature

[PATCH 0/4] Cleanup alloc_reserved_tree_block signature

[Nikolay Borisov]

At the moment alloc_reserved_tree_block takes 8 friggin arguments. The irony 
is that all of those are really derived from 3 structures. This series ends up 
reducing the arguments to 3. As a result some code, private to alloc_reserved_tree_block
is moved from run_delayed_tree_ref. Patch 1 removes the fs_info argument, 
Patch 2 replaces all arguments derived from btrfs_delayed_ref_node with the 
nodes itself, Patch 3 replaces the last 2 arguments with btrfs_delayed_extent_op
and finally, Patch 4 simplifies the code a bit by removing one extra check.

All in all this doesn't provide any functional changes but makes the code 
easier to follow. Also it has survived 2 xfstest runs. 

Nikolay Borisov (4):
  btrfs: Remove fs_info argument from alloc_reserved_tree_block
  btrfs: Simplify alloc_reserved_tree_block interface
  btrfs: Pass btrfs_delayed_extent_op to alloc_reserved_tree_block
  btrfs: Streamline shared ref check in alloc_reserved_tree_block

 fs/btrfs/extent-tree.c | 76 ++++++++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 39 deletions(-)

-- 
2.7.4

[PATCH 1/4] btrfs: Remove fs_info argument from alloc_reserved_tree_block

[Nikolay Borisov]

This function already takes a transaction handle which contains a
reference to the fs_info. So use this and remove the extra argument.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index ff26fed6196a..e80fbfa11a89 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -66,7 +66,6 @@ static int alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
 				      u64 flags, u64 owner, u64 offset,
 				      struct btrfs_key *ins, int ref_mod);
 static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
-				     struct btrfs_fs_info *fs_info,
 				     u64 parent, u64 root_objectid,
 				     u64 flags, struct btrfs_disk_key *key,
 				     int level, struct btrfs_key *ins);
@@ -2461,8 +2460,7 @@ static int run_delayed_tree_ref(struct btrfs_trans_handle *trans,
 	}
 	if (node->action == BTRFS_ADD_DELAYED_REF && insert_reserved) {
 		BUG_ON(!extent_op || !extent_op->update_flags);
-		ret = alloc_reserved_tree_block(trans, fs_info,
-						parent, ref_root,
+		ret = alloc_reserved_tree_block(trans, parent, ref_root,
 						extent_op->flags_to_set,
 						&extent_op->key,
 						ref->level, &ins);
@@ -8145,11 +8143,11 @@ static int alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
 }
 
 static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
-				     struct btrfs_fs_info *fs_info,
 				     u64 parent, u64 root_objectid,
 				     u64 flags, struct btrfs_disk_key *key,
 				     int level, struct btrfs_key *ins)
 {
+	struct btrfs_fs_info *fs_info = trans->fs_info;
 	int ret;
 	struct btrfs_extent_item *extent_item;
 	struct btrfs_tree_block_info *block_info;
-- 
2.7.4

Re: [PATCH 1/4] btrfs: Remove fs_info argument from alloc_reserved_tree_block

[David Sterba]

On Mon, May 21, 2018 at 12:27:20PM +0300, Nikolay Borisov wrote:
> This function already takes a transaction handle which contains a
> reference to the fs_info. So use this and remove the extra argument.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>

[PATCH 2/4] btrfs: Simplify alloc_reserved_tree_block interface

[Nikolay Borisov]

This function currently takes 7 parameters, most of which are
proxies for values from btrfs_delayed_ref_node struct which is not
passed. This patch simplifies the interface of the function by
simply passing said delayed ref node struct to the function. This
enables us to:

1. Move locals variables and init code related to them from
run_delayed_tree_ref which should only be used inside
alloc_reserved_tree_block, such as skinny_metadata and the btrfs_key,
representing the extent being inserted. This removes the need for the
"ins" argument. Instead, it's replaced by a local var with a more
verbose name - extent_key.

2. Now that we have a reference to the node in alloc_reserved_tree_block
the delayed_tree_ref struct can be referenced inside the function and
this enable removing the "ref->level", "parent" and "ref_root"
arguments.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c | 71 +++++++++++++++++++++++++++-----------------------
 1 file changed, 39 insertions(+), 32 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index e80fbfa11a89..fe7f7f5d99dc 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -66,9 +66,8 @@ static int alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
 				      u64 flags, u64 owner, u64 offset,
 				      struct btrfs_key *ins, int ref_mod);
 static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
-				     u64 parent, u64 root_objectid,
-				     u64 flags, struct btrfs_disk_key *key,
-				     int level, struct btrfs_key *ins);
+				     struct btrfs_delayed_ref_node *node,
+				     u64 flags, struct btrfs_disk_key *key);
 static int do_chunk_alloc(struct btrfs_trans_handle *trans,
 			  struct btrfs_fs_info *fs_info, u64 flags,
 			  int force);
@@ -2430,10 +2429,8 @@ static int run_delayed_tree_ref(struct btrfs_trans_handle *trans,
 {
 	int ret = 0;
 	struct btrfs_delayed_tree_ref *ref;
-	struct btrfs_key ins;
 	u64 parent = 0;
 	u64 ref_root = 0;
-	bool skinny_metadata = btrfs_fs_incompat(fs_info, SKINNY_METADATA);
 
 	ref = btrfs_delayed_node_to_tree_ref(node);
 	trace_run_delayed_tree_ref(fs_info, node, ref, node->action);
@@ -2442,15 +2439,6 @@ static int run_delayed_tree_ref(struct btrfs_trans_handle *trans,
 		parent = ref->parent;
 	ref_root = ref->root;
 
-	ins.objectid = node->bytenr;
-	if (skinny_metadata) {
-		ins.offset = ref->level;
-		ins.type = BTRFS_METADATA_ITEM_KEY;
-	} else {
-		ins.offset = node->num_bytes;
-		ins.type = BTRFS_EXTENT_ITEM_KEY;
-	}
-
 	if (node->ref_mod != 1) {
 		btrfs_err(fs_info,
 	"btree block(%llu) has %d references rather than 1: action %d ref_root %llu parent %llu",
@@ -2460,10 +2448,9 @@ static int run_delayed_tree_ref(struct btrfs_trans_handle *trans,
 	}
 	if (node->action == BTRFS_ADD_DELAYED_REF && insert_reserved) {
 		BUG_ON(!extent_op || !extent_op->update_flags);
-		ret = alloc_reserved_tree_block(trans, parent, ref_root,
+		ret = alloc_reserved_tree_block(trans, node,
 						extent_op->flags_to_set,
-						&extent_op->key,
-						ref->level, &ins);
+						&extent_op->key);
 	} else if (node->action == BTRFS_ADD_DELAYED_REF) {
 		ret = __btrfs_inc_extent_ref(trans, fs_info, node,
 					     parent, ref_root,
@@ -8143,37 +8130,57 @@ static int alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
 }
 
 static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
-				     u64 parent, u64 root_objectid,
-				     u64 flags, struct btrfs_disk_key *key,
-				     int level, struct btrfs_key *ins)
+				     struct btrfs_delayed_ref_node *node,
+				     u64 flags, struct btrfs_disk_key *key)
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
 	int ret;
 	struct btrfs_extent_item *extent_item;
+	struct btrfs_key extent_key;
 	struct btrfs_tree_block_info *block_info;
 	struct btrfs_extent_inline_ref *iref;
 	struct btrfs_path *path;
 	struct extent_buffer *leaf;
+	struct btrfs_delayed_tree_ref *ref;
 	u32 size = sizeof(*extent_item) + sizeof(*iref);
-	u64 num_bytes = ins->offset;
+	u64 num_bytes;
+	u64 parent;
 	bool skinny_metadata = btrfs_fs_incompat(fs_info, SKINNY_METADATA);
 
-	if (!skinny_metadata)
+	ref = btrfs_delayed_node_to_tree_ref(node);
+
+	if (node->type == BTRFS_SHARED_BLOCK_REF_KEY)
+		parent = ref->parent;
+	else
+		parent = 0;
+
+	extent_key.objectid = node->bytenr;
+	if (skinny_metadata) {
+		extent_key.offset = ref->level;
+		extent_key.type = BTRFS_METADATA_ITEM_KEY;
+		num_bytes = fs_info->nodesize;
+	} else {
+		extent_key.offset = node->num_bytes;
+		extent_key.type = BTRFS_EXTENT_ITEM_KEY;
 		size += sizeof(*block_info);
+		num_bytes = node->num_bytes;
+	}
 
 	path = btrfs_alloc_path();
 	if (!path) {
-		btrfs_free_and_pin_reserved_extent(fs_info, ins->objectid,
+		btrfs_free_and_pin_reserved_extent(fs_info,
+						   extent_key.objectid,
 						   fs_info->nodesize);
 		return -ENOMEM;
 	}
 
 	path->leave_spinning = 1;
 	ret = btrfs_insert_empty_item(trans, fs_info->extent_root, path,
-				      ins, size);
+				      &extent_key, size);
 	if (ret) {
 		btrfs_free_path(path);
-		btrfs_free_and_pin_reserved_extent(fs_info, ins->objectid,
+		btrfs_free_and_pin_reserved_extent(fs_info,
+						   extent_key.objectid,
 						   fs_info->nodesize);
 		return ret;
 	}
@@ -8188,11 +8195,10 @@ static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
 
 	if (skinny_metadata) {
 		iref = (struct btrfs_extent_inline_ref *)(extent_item + 1);
-		num_bytes = fs_info->nodesize;
 	} else {
 		block_info = (struct btrfs_tree_block_info *)(extent_item + 1);
 		btrfs_set_tree_block_key(leaf, block_info, key);
-		btrfs_set_tree_block_level(leaf, block_info, level);
+		btrfs_set_tree_block_level(leaf, block_info, ref->level);
 		iref = (struct btrfs_extent_inline_ref *)(block_info + 1);
 	}
 
@@ -8204,25 +8210,26 @@ static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
 	} else {
 		btrfs_set_extent_inline_ref_type(leaf, iref,
 						 BTRFS_TREE_BLOCK_REF_KEY);
-		btrfs_set_extent_inline_ref_offset(leaf, iref, root_objectid);
+		btrfs_set_extent_inline_ref_offset(leaf, iref, ref->root);
 	}
 
 	btrfs_mark_buffer_dirty(leaf);
 	btrfs_free_path(path);
 
-	ret = remove_from_free_space_tree(trans, ins->objectid, num_bytes);
+	ret = remove_from_free_space_tree(trans, extent_key.objectid,
+					  num_bytes);
 	if (ret)
 		return ret;
 
-	ret = update_block_group(trans, fs_info, ins->objectid,
+	ret = update_block_group(trans, fs_info, extent_key.objectid,
 				 fs_info->nodesize, 1);
 	if (ret) { /* -ENOENT, logic error */
 		btrfs_err(fs_info, "update block group failed for %llu %llu",
-			ins->objectid, ins->offset);
+			extent_key.objectid, extent_key.offset);
 		BUG();
 	}
 
-	trace_btrfs_reserved_extent_alloc(fs_info, ins->objectid,
+	trace_btrfs_reserved_extent_alloc(fs_info, extent_key.objectid,
 					  fs_info->nodesize);
 	return ret;
 }
-- 
2.7.4

Re: [PATCH 2/4] btrfs: Simplify alloc_reserved_tree_block interface

[David Sterba]

On Mon, May 21, 2018 at 12:27:21PM +0300, Nikolay Borisov wrote:
> This function currently takes 7 parameters, most of which are
> proxies for values from btrfs_delayed_ref_node struct which is not
> passed. This patch simplifies the interface of the function by
> simply passing said delayed ref node struct to the function. This
> enables us to:
> 
> 1. Move locals variables and init code related to them from
> run_delayed_tree_ref which should only be used inside
> alloc_reserved_tree_block, such as skinny_metadata and the btrfs_key,
> representing the extent being inserted. This removes the need for the
> "ins" argument. Instead, it's replaced by a local var with a more
> verbose name - extent_key.
> 
> 2. Now that we have a reference to the node in alloc_reserved_tree_block
> the delayed_tree_ref struct can be referenced inside the function and
> this enable removing the "ref->level", "parent" and "ref_root"
> arguments.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

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

[PATCH 3/4] btrfs: Pass btrfs_delayed_extent_op to alloc_reserved_tree_block

[Nikolay Borisov]

Instead of taking only specific member of this structure, which results
in 2 extra arguments, just take the delayed_extent_op struct and
reference the arguments inside the functions. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index fe7f7f5d99dc..4d092d14cbe9 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -67,7 +67,7 @@ static int alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
 				      struct btrfs_key *ins, int ref_mod);
 static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
 				     struct btrfs_delayed_ref_node *node,
-				     u64 flags, struct btrfs_disk_key *key);
+				     struct btrfs_delayed_extent_op *extent_op);
 static int do_chunk_alloc(struct btrfs_trans_handle *trans,
 			  struct btrfs_fs_info *fs_info, u64 flags,
 			  int force);
@@ -2448,9 +2448,7 @@ static int run_delayed_tree_ref(struct btrfs_trans_handle *trans,
 	}
 	if (node->action == BTRFS_ADD_DELAYED_REF && insert_reserved) {
 		BUG_ON(!extent_op || !extent_op->update_flags);
-		ret = alloc_reserved_tree_block(trans, node,
-						extent_op->flags_to_set,
-						&extent_op->key);
+		ret = alloc_reserved_tree_block(trans, node, extent_op);
 	} else if (node->action == BTRFS_ADD_DELAYED_REF) {
 		ret = __btrfs_inc_extent_ref(trans, fs_info, node,
 					     parent, ref_root,
@@ -8131,7 +8129,7 @@ static int alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
 
 static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
 				     struct btrfs_delayed_ref_node *node,
-				     u64 flags, struct btrfs_disk_key *key)
+				     struct btrfs_delayed_extent_op *extent_op)
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
 	int ret;
@@ -8145,6 +8143,7 @@ static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
 	u32 size = sizeof(*extent_item) + sizeof(*iref);
 	u64 num_bytes;
 	u64 parent;
+	u64 flags = extent_op->flags_to_set;
 	bool skinny_metadata = btrfs_fs_incompat(fs_info, SKINNY_METADATA);
 
 	ref = btrfs_delayed_node_to_tree_ref(node);
@@ -8197,7 +8196,7 @@ static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
 		iref = (struct btrfs_extent_inline_ref *)(extent_item + 1);
 	} else {
 		block_info = (struct btrfs_tree_block_info *)(extent_item + 1);
-		btrfs_set_tree_block_key(leaf, block_info, key);
+		btrfs_set_tree_block_key(leaf, block_info, &extent_op->key);
 		btrfs_set_tree_block_level(leaf, block_info, ref->level);
 		iref = (struct btrfs_extent_inline_ref *)(block_info + 1);
 	}
-- 
2.7.4

Re: [PATCH 3/4] btrfs: Pass btrfs_delayed_extent_op to alloc_reserved_tree_block

[David Sterba]

On Mon, May 21, 2018 at 12:27:22PM +0300, Nikolay Borisov wrote:
> Instead of taking only specific member of this structure, which results
> in 2 extra arguments, just take the delayed_extent_op struct and
> reference the arguments inside the functions. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

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

[PATCH 4/4] btrfs: Streamline shared ref check in alloc_reserved_tree_block

[Nikolay Borisov]

Instead of setting "parent" to ref->parent only when dealing with
a shared ref and subsequently performing another check to see
if (parent > 0), check the "node->type" directly and act accordingly.
This makes the code more streamline. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 4d092d14cbe9..9e8786a7555f 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -8142,17 +8142,11 @@ static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
 	struct btrfs_delayed_tree_ref *ref;
 	u32 size = sizeof(*extent_item) + sizeof(*iref);
 	u64 num_bytes;
-	u64 parent;
 	u64 flags = extent_op->flags_to_set;
 	bool skinny_metadata = btrfs_fs_incompat(fs_info, SKINNY_METADATA);
 
 	ref = btrfs_delayed_node_to_tree_ref(node);
 
-	if (node->type == BTRFS_SHARED_BLOCK_REF_KEY)
-		parent = ref->parent;
-	else
-		parent = 0;
-
 	extent_key.objectid = node->bytenr;
 	if (skinny_metadata) {
 		extent_key.offset = ref->level;
@@ -8201,11 +8195,11 @@ static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
 		iref = (struct btrfs_extent_inline_ref *)(block_info + 1);
 	}
 
-	if (parent > 0) {
+	if (node->type == BTRFS_SHARED_BLOCK_REF_KEY) {
 		BUG_ON(!(flags & BTRFS_BLOCK_FLAG_FULL_BACKREF));
 		btrfs_set_extent_inline_ref_type(leaf, iref,
 						 BTRFS_SHARED_BLOCK_REF_KEY);
-		btrfs_set_extent_inline_ref_offset(leaf, iref, parent);
+		btrfs_set_extent_inline_ref_offset(leaf, iref, ref->parent);
 	} else {
 		btrfs_set_extent_inline_ref_type(leaf, iref,
 						 BTRFS_TREE_BLOCK_REF_KEY);
-- 
2.7.4

Re: [PATCH 4/4] btrfs: Streamline shared ref check in alloc_reserved_tree_block

[David Sterba]

On Mon, May 21, 2018 at 12:27:23PM +0300, Nikolay Borisov wrote:
> Instead of setting "parent" to ref->parent only when dealing with
> a shared ref and subsequently performing another check to see
> if (parent > 0), check the "node->type" directly and act accordingly.
> This makes the code more streamline. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

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

Re: [PATCH 0/4] Cleanup alloc_reserved_tree_block signature

[David Sterba]

On Mon, May 21, 2018 at 12:27:19PM +0300, Nikolay Borisov wrote:
> At the moment alloc_reserved_tree_block takes 8 friggin arguments. The irony 
> is that all of those are really derived from 3 structures. This series ends up 
> reducing the arguments to 3. As a result some code, private to alloc_reserved_tree_block
> is moved from run_delayed_tree_ref. Patch 1 removes the fs_info argument, 
> Patch 2 replaces all arguments derived from btrfs_delayed_ref_node with the 
> nodes itself, Patch 3 replaces the last 2 arguments with btrfs_delayed_extent_op
> and finally, Patch 4 simplifies the code a bit by removing one extra check.
> 
> All in all this doesn't provide any functional changes but makes the code 
> easier to follow. Also it has survived 2 xfstest runs. 
> 
> Nikolay Borisov (4):
>   btrfs: Remove fs_info argument from alloc_reserved_tree_block
>   btrfs: Simplify alloc_reserved_tree_block interface
>   btrfs: Pass btrfs_delayed_extent_op to alloc_reserved_tree_block
>   btrfs: Streamline shared ref check in alloc_reserved_tree_block

Passed fstests and added to misc-next, thanks.