Linux-BTRFS Archive on lore.kernel.org
 help / Atom feed
* [PATCH 0/8] btrfs: Refactor delayed ref parameter list
@ 2018-12-06  6:58 Qu Wenruo
  2018-12-06  6:58 ` [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures Qu Wenruo
                   ` (7 more replies)
  0 siblings, 8 replies; 23+ messages in thread
From: Qu Wenruo @ 2018-12-06  6:58 UTC (permalink / raw)
  To: linux-btrfs

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(-)

-- 
2.19.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures
  2018-12-06  6:58 [PATCH 0/8] btrfs: Refactor delayed ref parameter list Qu Wenruo
@ 2018-12-06  6:58 ` Qu Wenruo
  2018-12-10  9:48   ` Nikolay Borisov
  2018-12-06  6:58 ` [PATCH 2/8] btrfs: extent-tree: Open-code process_func in __btrfs_mod_ref Qu Wenruo
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 23+ messages in thread
From: Qu Wenruo @ 2018-12-06  6:58 UTC (permalink / raw)
  To: linux-btrfs

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.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/delayed-ref.h | 109 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
index d8fa12d3f2cc..e36d6b05d85e 100644
--- a/fs/btrfs/delayed-ref.h
+++ b/fs/btrfs/delayed-ref.h
@@ -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)
 {
-- 
2.19.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 2/8] btrfs: extent-tree: Open-code process_func in __btrfs_mod_ref
  2018-12-06  6:58 [PATCH 0/8] btrfs: Refactor delayed ref parameter list Qu Wenruo
  2018-12-06  6:58 ` [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures Qu Wenruo
@ 2018-12-06  6:58 ` Qu Wenruo
  2018-12-07 17:39   ` Nikolay Borisov
  2018-12-06  6:58 ` [PATCH 3/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_tree_ref() Qu Wenruo
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 23+ messages in thread
From: Qu Wenruo @ 2018-12-06  6:58 UTC (permalink / raw)
  To: linux-btrfs

The 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.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent-tree.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index ea2c3d5220f0..ea68d288d761 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -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;
 		}
-- 
2.19.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 3/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_tree_ref()
  2018-12-06  6:58 [PATCH 0/8] btrfs: Refactor delayed ref parameter list Qu Wenruo
  2018-12-06  6:58 ` [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures Qu Wenruo
  2018-12-06  6:58 ` [PATCH 2/8] btrfs: extent-tree: Open-code process_func in __btrfs_mod_ref Qu Wenruo
@ 2018-12-06  6:58 ` Qu Wenruo
  2018-12-10  9:21   ` Nikolay Borisov
  2018-12-06  6:58 ` [PATCH 4/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_data_ref() Qu Wenruo
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 23+ messages in thread
From: Qu Wenruo @ 2018-12-06  6:58 UTC (permalink / raw)
  To: linux-btrfs

btrfs_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.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 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(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 11dd46be4017..c42b8ade7b07 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -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;
diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
index e36d6b05d85e..dbe029c4e01b 100644
--- a/fs/btrfs/delayed-ref.h
+++ b/fs/btrfs/delayed-ref.h
@@ -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,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index ea68d288d761..ecfa0234863b 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -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,
+			       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);
+		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,
@@ -7013,9 +7014,16 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
 			   u64 parent, int last_ref)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
+	struct btrfs_ref generic_ref = { 0 };
 	int pin = 1;
 	int ret;
 
+	btrfs_init_generic_ref(&generic_ref, BTRFS_DROP_DELAYED_REF,
+			       buf->start, buf->len, root->root_key.objectid,
+			       parent);
+	btrfs_init_tree_ref(&generic_ref, btrfs_header_level(buf),
+			    root->root_key.objectid);
+
 	if (root->root_key.objectid != BTRFS_TREE_LOG_OBJECTID) {
 		int old_ref_mod, new_ref_mod;
 
@@ -7023,11 +7031,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
 				   root->root_key.objectid,
 				   btrfs_header_level(buf), 0,
 				   BTRFS_DROP_DELAYED_REF);
-		ret = btrfs_add_delayed_tree_ref(trans, buf->start,
-						 buf->len, parent,
-						 root->root_key.objectid,
-						 btrfs_header_level(buf), false,
-						 BTRFS_DROP_DELAYED_REF, NULL,
+		ret = btrfs_add_delayed_tree_ref(trans, &generic_ref, NULL,
 						 &old_ref_mod, &new_ref_mod);
 		BUG_ON(ret); /* -ENOMEM */
 		pin = old_ref_mod >= 0 && new_ref_mod < 0;
@@ -7080,6 +7084,7 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans,
 		      u64 owner, u64 offset, 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;
 
@@ -7091,6 +7096,9 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans,
 				   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;
 	/*
 	 * tree log blocks never actually go into the extent allocation
 	 * tree, just update pinning info and exit early.
@@ -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);
+		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,
@@ -8276,6 +8281,7 @@ struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
 	struct btrfs_block_rsv *block_rsv;
 	struct extent_buffer *buf;
 	struct btrfs_delayed_extent_op *extent_op;
+	struct btrfs_ref generic_ref = { 0 };
 	u64 flags = 0;
 	int ret;
 	bool for_reloc = false;
@@ -8337,11 +8343,11 @@ struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
 		btrfs_ref_tree_mod(root, ins.objectid, ins.offset, parent,
 				   root_objectid, level, 0,
 				   BTRFS_ADD_DELAYED_EXTENT);
-		ret = btrfs_add_delayed_tree_ref(trans, ins.objectid,
-						 ins.offset, parent,
-						 root_objectid, level,
-						 for_reloc,
-						 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);
+		ret = btrfs_add_delayed_tree_ref(trans, &generic_ref,
 						 extent_op, NULL, NULL);
 		if (ret)
 			goto out_free_delayed;
-- 
2.19.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 4/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_data_ref()
  2018-12-06  6:58 [PATCH 0/8] btrfs: Refactor delayed ref parameter list Qu Wenruo
                   ` (2 preceding siblings ...)
  2018-12-06  6:58 ` [PATCH 3/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_tree_ref() Qu Wenruo
@ 2018-12-06  6:58 ` Qu Wenruo
  2018-12-10  9:23   ` Nikolay Borisov
  2018-12-06  6:59 ` [PATCH 5/8] btrfs: ref-verify: Use btrfs_ref to refactor btrfs_ref_tree_mod() Qu Wenruo
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 23+ messages in thread
From: Qu Wenruo @ 2018-12-06  6:58 UTC (permalink / raw)
  To: linux-btrfs

Just like btrfs_add_delayed_tree_ref(), use btrfs_ref to refactor
btrfs_add_delayed_data_ref().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 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(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index c42b8ade7b07..09caf1e6fc22 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -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);
diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
index dbe029c4e01b..a8fde33b43fd 100644
--- a/fs/btrfs/delayed-ref.h
+++ b/fs/btrfs/delayed-ref.h
@@ -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,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index ecfa0234863b..fa5dd3dfe2e7 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -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;
 }
 
-- 
2.19.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 5/8] btrfs: ref-verify: Use btrfs_ref to refactor btrfs_ref_tree_mod()
  2018-12-06  6:58 [PATCH 0/8] btrfs: Refactor delayed ref parameter list Qu Wenruo
                   ` (3 preceding siblings ...)
  2018-12-06  6:58 ` [PATCH 4/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_data_ref() Qu Wenruo
@ 2018-12-06  6:59 ` Qu Wenruo
  2018-12-06  6:59 ` [PATCH 6/8] btrfs: extent-tree: Use btrfs_ref to refactor add_pinned_bytes() Qu Wenruo
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 23+ messages in thread
From: Qu Wenruo @ 2018-12-06  6:59 UTC (permalink / raw)
  To: linux-btrfs

It'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.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 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(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index fa5dd3dfe2e7..1d812bc2c7fc 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -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)
diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c
index d69fbfb30aa9..f6c15b612570 100644
--- a/fs/btrfs/ref-verify.c
+++ b/fs/btrfs/ref-verify.c
@@ -670,36 +670,43 @@ static void dump_block_entry(struct btrfs_fs_info *fs_info,
 
 /*
  * btrfs_ref_tree_mod: called when we modify a ref for a bytenr
- * @root: the root we are making this modification from.
- * @bytenr: the bytenr we are modifying.
- * @num_bytes: number of bytes.
- * @parent: the parent bytenr.
- * @ref_root: the original root owner of the bytenr.
- * @owner: level in the case of metadata, inode in the case of data.
- * @offset: 0 for metadata, file offset for data.
- * @action: the action that we are doing, this is the same as the delayed ref
- *	action.
  *
  * This will add an action item to the given bytenr and do sanity checks to make
  * sure we haven't messed something up.  If we are making a new allocation and
  * this block entry has history we will delete all previous actions as long as
  * our sanity checks pass as they are no longer needed.
  */
-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)
 {
-	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct ref_entry *ref = NULL, *exist;
 	struct ref_action *ra = NULL;
 	struct block_entry *be = NULL;
 	struct root_entry *re = NULL;
+	int action = generic_ref->action;
 	int ret = 0;
-	bool metadata = owner < BTRFS_FIRST_FREE_OBJECTID;
+	bool metadata;
+	u64 bytenr = generic_ref->bytenr;
+	u64 num_bytes = generic_ref->len;
+	u64 parent = generic_ref->parent;
+	u64 ref_root;
+	u64 owner;
+	u64 offset;
 
-	if (!btrfs_test_opt(root->fs_info, REF_VERIFY))
+	if (!btrfs_test_opt(fs_info, REF_VERIFY))
 		return 0;
 
+	if (generic_ref->type == BTRFS_REF_METADATA) {
+		ref_root = generic_ref->tree_ref.root;
+		owner = generic_ref->tree_ref.level;
+		offset = 0;
+	} else {
+		ref_root = generic_ref->data_ref.ref_root;
+		owner = generic_ref->data_ref.ino;
+		offset = generic_ref->data_ref.offset;
+	}
+	metadata = owner < BTRFS_FIRST_FREE_OBJECTID;
+
 	ref = kzalloc(sizeof(struct ref_entry), GFP_NOFS);
 	ra = kmalloc(sizeof(struct ref_action), GFP_NOFS);
 	if (!ra || !ref) {
@@ -732,7 +739,7 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
 
 	INIT_LIST_HEAD(&ra->list);
 	ra->action = action;
-	ra->root = root->root_key.objectid;
+	ra->root = generic_ref->real_root;
 
 	/*
 	 * This is an allocation, preallocate the block_entry in case we haven't
@@ -745,7 +752,7 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
 		 * is and the new root objectid, so let's not treat the passed
 		 * in root as if it really has a ref for this bytenr.
 		 */
-		be = add_block_entry(root->fs_info, bytenr, num_bytes, ref_root);
+		be = add_block_entry(fs_info, bytenr, num_bytes, ref_root);
 		if (IS_ERR(be)) {
 			kfree(ra);
 			ret = PTR_ERR(be);
@@ -787,13 +794,13 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
 			 * one we want to lookup below when we modify the
 			 * re->num_refs.
 			 */
-			ref_root = root->root_key.objectid;
-			re->root_objectid = root->root_key.objectid;
+			ref_root = generic_ref->real_root;
+			re->root_objectid = generic_ref->real_root;
 			re->num_refs = 0;
 		}
 
-		spin_lock(&root->fs_info->ref_verify_lock);
-		be = lookup_block_entry(&root->fs_info->block_tree, bytenr);
+		spin_lock(&fs_info->ref_verify_lock);
+		be = lookup_block_entry(&fs_info->block_tree, bytenr);
 		if (!be) {
 			btrfs_err(fs_info,
 "trying to do action %d to bytenr %llu num_bytes %llu but there is no existing entry!",
@@ -862,7 +869,7 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
 			 * didn't thik of some other corner case.
 			 */
 			btrfs_err(fs_info, "failed to find root %llu for %llu",
-				  root->root_key.objectid, be->bytenr);
+				  generic_ref->real_root, be->bytenr);
 			dump_block_entry(fs_info, be);
 			dump_ref_action(fs_info, ra);
 			kfree(ra);
@@ -881,7 +888,7 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
 	list_add_tail(&ra->list, &be->actions);
 	ret = 0;
 out_unlock:
-	spin_unlock(&root->fs_info->ref_verify_lock);
+	spin_unlock(&fs_info->ref_verify_lock);
 out:
 	if (ret)
 		btrfs_clear_opt(fs_info->mount_opt, REF_VERIFY);
diff --git a/fs/btrfs/ref-verify.h b/fs/btrfs/ref-verify.h
index b7d2a4edfdb7..855de37719b5 100644
--- a/fs/btrfs/ref-verify.h
+++ b/fs/btrfs/ref-verify.h
@@ -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;
 }
-- 
2.19.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 6/8] btrfs: extent-tree: Use btrfs_ref to refactor add_pinned_bytes()
  2018-12-06  6:58 [PATCH 0/8] btrfs: Refactor delayed ref parameter list Qu Wenruo
                   ` (4 preceding siblings ...)
  2018-12-06  6:59 ` [PATCH 5/8] btrfs: ref-verify: Use btrfs_ref to refactor btrfs_ref_tree_mod() Qu Wenruo
@ 2018-12-06  6:59 ` Qu Wenruo
  2018-12-10  9:45   ` Nikolay Borisov
  2018-12-06  6:59 ` [PATCH 7/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref() Qu Wenruo
  2018-12-06  6:59 ` [PATCH 8/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_free_extent() Qu Wenruo
  7 siblings, 1 reply; 23+ messages in thread
From: Qu Wenruo @ 2018-12-06  6:59 UTC (permalink / raw)
  To: linux-btrfs

Since 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.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent-tree.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 1d812bc2c7fc..70c05ca30d9a 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -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;
 }
-- 
2.19.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 7/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref()
  2018-12-06  6:58 [PATCH 0/8] btrfs: Refactor delayed ref parameter list Qu Wenruo
                   ` (5 preceding siblings ...)
  2018-12-06  6:59 ` [PATCH 6/8] btrfs: extent-tree: Use btrfs_ref to refactor add_pinned_bytes() Qu Wenruo
@ 2018-12-06  6:59 ` Qu Wenruo
  2018-12-10  9:52   ` Nikolay Borisov
  2018-12-06  6:59 ` [PATCH 8/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_free_extent() Qu Wenruo
  7 siblings, 1 reply; 23+ messages in thread
From: Qu Wenruo @ 2018-12-06  6:59 UTC (permalink / raw)
  To: linux-btrfs

Now we don't need to play the dirty game of reusing @owner for tree block
level.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 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(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 6f4b1e605736..db3df5ce6087 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -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,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 70c05ca30d9a..ff60091aef6b 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -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;
 
-	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);
+	BUG_ON(generic_ref->type == BTRFS_REF_METADATA &&
+	       generic_ref->tree_ref.root == BTRFS_TREE_LOG_OBJECTID);
 
-	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;
 }
@@ -3212,8 +3204,10 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
 	u32 nritems;
 	struct btrfs_key key;
 	struct btrfs_file_extent_item *fi;
+	struct btrfs_ref generic_ref = { 0 };
 	bool for_reloc = btrfs_header_flag(buf, BTRFS_HEADER_FLAG_RELOC);
 	int i;
+	int action;
 	int level;
 	int ret = 0;
 
@@ -3231,6 +3225,10 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
 		parent = buf->start;
 	else
 		parent = 0;
+	if (inc)
+		action = BTRFS_ADD_DELAYED_REF;
+	else
+		action = BTRFS_DROP_DELAYED_REF;
 
 	for (i = 0; i < nritems; i++) {
 		if (level == 0) {
@@ -3248,11 +3246,14 @@ 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);
+			btrfs_init_generic_ref(&generic_ref, action, bytenr,
+					num_bytes, root->root_key.objectid,
+					parent);
+			btrfs_init_data_ref(&generic_ref, ref_root, key.objectid,
+					    key.offset);
+			generic_ref.skip_qgroup = for_reloc;
 			if (inc)
-				ret = btrfs_inc_extent_ref(trans, root, bytenr,
-						num_bytes, parent, ref_root,
-						key.objectid, key.offset,
-						for_reloc);
+				ret = btrfs_inc_extent_ref(trans, &generic_ref);
 			else
 				ret = btrfs_free_extent(trans, root, bytenr,
 						num_bytes, parent, ref_root,
@@ -3263,10 +3264,13 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
 		} else {
 			bytenr = btrfs_node_blockptr(buf, i);
 			num_bytes = fs_info->nodesize;
+			btrfs_init_generic_ref(&generic_ref, action, bytenr,
+					num_bytes, root->root_key.objectid,
+					parent);
+			btrfs_init_tree_ref(&generic_ref, level - 1, ref_root);
+			generic_ref.skip_qgroup = for_reloc;
 			if (inc)
-				ret = btrfs_inc_extent_ref(trans, root, bytenr,
-						num_bytes, parent, ref_root,
-						level - 1, 0, for_reloc);
+				ret = btrfs_inc_extent_ref(trans, &generic_ref);
 			else
 				ret = btrfs_free_extent(trans, root, bytenr,
 						num_bytes, parent, ref_root,
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 56414c2ebf1d..35c786334972 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -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;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 539dd3f7f1bd..aa83f68e22ee 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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;
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ab9e87eb9a4e..d6115b04134f 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -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);
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 80550c4464f3..7aa240d1f361 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1658,6 +1658,8 @@ int replace_file_extents(struct btrfs_trans_handle *trans,
 
 	nritems = btrfs_header_nritems(leaf);
 	for (i = 0; i < nritems; i++) {
+		struct btrfs_ref ref = { 0 };
+
 		cond_resched();
 		btrfs_item_key_to_cpu(leaf, &key, i);
 		if (key.type != BTRFS_EXTENT_DATA_KEY)
@@ -1718,10 +1720,12 @@ int replace_file_extents(struct btrfs_trans_handle *trans,
 		dirty = 1;
 
 		key.offset -= btrfs_file_extent_offset(leaf, fi);
-		ret = btrfs_inc_extent_ref(trans, root, new_bytenr,
-					   num_bytes, parent,
-					   btrfs_header_owner(leaf),
-					   key.objectid, key.offset, false);
+
+		btrfs_init_generic_ref(&ref, BTRFS_ADD_DELAYED_REF, new_bytenr,
+				num_bytes, root->root_key.objectid, parent);
+		btrfs_init_data_ref(&ref, btrfs_header_owner(leaf),
+				    key.objectid, key.offset);
+		ret = btrfs_inc_extent_ref(trans, &ref);
 		if (ret) {
 			btrfs_abort_transaction(trans, ret);
 			break;
@@ -1771,6 +1775,7 @@ int replace_path(struct btrfs_trans_handle *trans, struct reloc_control *rc,
 	struct btrfs_fs_info *fs_info = dest->fs_info;
 	struct extent_buffer *eb;
 	struct extent_buffer *parent;
+	struct btrfs_ref ref = { 0 };
 	struct btrfs_key key;
 	u64 old_bytenr;
 	u64 new_bytenr;
@@ -1929,14 +1934,18 @@ int replace_path(struct btrfs_trans_handle *trans, struct reloc_control *rc,
 					      path->slots[level], old_ptr_gen);
 		btrfs_mark_buffer_dirty(path->nodes[level]);
 
-		ret = btrfs_inc_extent_ref(trans, src, old_bytenr,
-					blocksize, path->nodes[level]->start,
-					src->root_key.objectid, level - 1, 0,
-					true);
+		btrfs_init_generic_ref(&ref, BTRFS_ADD_DELAYED_REF, old_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_inc_extent_ref(trans, &ref);
 		BUG_ON(ret);
-		ret = btrfs_inc_extent_ref(trans, dest, new_bytenr,
-					blocksize, 0, dest->root_key.objectid,
-					level - 1, 0, true);
+		btrfs_init_generic_ref(&ref, BTRFS_ADD_DELAYED_REF, new_bytenr,
+				       blocksize, dest->root_key.objectid, 0);
+		btrfs_init_tree_ref(&ref, level - 1, dest->root_key.objectid);
+		ref.skip_qgroup = true;
+		ret = btrfs_inc_extent_ref(trans, &ref);
 		BUG_ON(ret);
 
 		ret = btrfs_free_extent(trans, src, new_bytenr, blocksize,
@@ -2763,6 +2772,7 @@ static int do_relocation(struct btrfs_trans_handle *trans,
 	rc->backref_cache.path[node->level] = node;
 	list_for_each_entry(edge, &node->upper, list[LOWER]) {
 		struct btrfs_key first_key;
+		struct btrfs_ref ref = { 0 };
 
 		cond_resched();
 
@@ -2860,11 +2870,13 @@ static int do_relocation(struct btrfs_trans_handle *trans,
 						      trans->transid);
 			btrfs_mark_buffer_dirty(upper->eb);
 
-			ret = btrfs_inc_extent_ref(trans, root,
-						node->eb->start, blocksize,
-						upper->eb->start,
-						btrfs_header_owner(upper->eb),
-						node->level, 0, false);
+			btrfs_init_generic_ref(&ref, BTRFS_ADD_DELAYED_REF,
+					       node->eb->start, blocksize,
+					       root->root_key.objectid,
+					       upper->eb->start);
+			btrfs_init_tree_ref(&ref, node->level,
+					    btrfs_header_owner(upper->eb));
+			ret = btrfs_inc_extent_ref(trans, &ref);
 			BUG_ON(ret);
 
 			ret = btrfs_drop_subtree(trans, root, eb, upper->eb);
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 9e4d274902b8..e4da64088679 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -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 {
-- 
2.19.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 8/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_free_extent()
  2018-12-06  6:58 [PATCH 0/8] btrfs: Refactor delayed ref parameter list Qu Wenruo
                   ` (6 preceding siblings ...)
  2018-12-06  6:59 ` [PATCH 7/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref() Qu Wenruo
@ 2018-12-06  6:59 ` Qu Wenruo
  7 siblings, 0 replies; 23+ messages in thread
From: Qu Wenruo @ 2018-12-06  6:59 UTC (permalink / raw)
  To: linux-btrfs

Similar to btrfs_inc_extent_ref(), just use btrfs_ref to replace the
long parameter list and the confusing @owner parameter.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 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(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index db3df5ce6087..9ed55a29993d 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -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);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index ff60091aef6b..8a6a73006dc4 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3255,10 +3255,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
 			if (inc)
 				ret = btrfs_inc_extent_ref(trans, &generic_ref);
 			else
-				ret = btrfs_free_extent(trans, root, bytenr,
-						num_bytes, parent, ref_root,
-						key.objectid, key.offset,
-						for_reloc);
+				ret = btrfs_free_extent(trans, &generic_ref);
 			if (ret)
 				goto fail;
 		} else {
@@ -3272,9 +3269,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
 			if (inc)
 				ret = btrfs_inc_extent_ref(trans, &generic_ref);
 			else
-				ret = btrfs_free_extent(trans, root, bytenr,
-						num_bytes, parent, ref_root,
-						level - 1, 0, for_reloc);
+				ret = btrfs_free_extent(trans, &generic_ref);
 			if (ret)
 				goto fail;
 		}
@@ -7073,47 +7068,43 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
 }
 
 /* Can return -ENOMEM */
-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)
 {
-	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;
 
 	if (btrfs_is_testing(fs_info))
 		return 0;
 
-	btrfs_init_generic_ref(&generic_ref, BTRFS_DROP_DELAYED_REF, bytenr,
-			       num_bytes, root->root_key.objectid, parent);
-	generic_ref.skip_qgroup = for_reloc;
 	/*
 	 * tree log blocks never actually go into the extent allocation
 	 * tree, just update pinning info and exit early.
 	 */
-	if (root_objectid == BTRFS_TREE_LOG_OBJECTID) {
-		WARN_ON(owner >= BTRFS_FIRST_FREE_OBJECTID);
+	if ((ref->type == BTRFS_REF_METADATA &&
+	     ref->tree_ref.root == BTRFS_TREE_LOG_OBJECTID) ||
+	    (ref->type == BTRFS_REF_DATA &&
+	     ref->data_ref.ref_root == BTRFS_TREE_LOG_OBJECTID)) {
 		/* unlocks the pinned mutex */
-		btrfs_pin_extent(fs_info, bytenr, num_bytes, 1);
+		btrfs_pin_extent(fs_info, ref->bytenr, ref->len, 1);
 		old_ref_mod = new_ref_mod = 0;
 		ret = 0;
-	} else 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, NULL,
+	} else if (ref->type == BTRFS_REF_METADATA) {
+		ret = btrfs_add_delayed_tree_ref(trans, 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,
+		ret = btrfs_add_delayed_data_ref(trans, ref, 0,
 						 &old_ref_mod, &new_ref_mod);
 	}
 
-	if (root_objectid != BTRFS_TREE_LOG_OBJECTID)
-		btrfs_ref_tree_mod(fs_info, &generic_ref);
+	if (!((ref->type == BTRFS_REF_METADATA &&
+	       ref->tree_ref.root == BTRFS_TREE_LOG_OBJECTID) ||
+	      (ref->type == BTRFS_REF_DATA &&
+	       ref->data_ref.ref_root == BTRFS_TREE_LOG_OBJECTID)))
+		btrfs_ref_tree_mod(fs_info, ref);
 
 	if (ret == 0 && old_ref_mod >= 0 && new_ref_mod < 0)
-		add_pinned_bytes(fs_info, &generic_ref);
+		add_pinned_bytes(fs_info, ref);
 
 	return ret;
 }
@@ -8548,6 +8539,7 @@ static noinline int do_walk_down(struct btrfs_trans_handle *trans,
 	u32 blocksize;
 	struct btrfs_key key;
 	struct btrfs_key first_key;
+	struct btrfs_ref ref = { 0 };
 	struct extent_buffer *next;
 	int level = wc->level;
 	int reada = 0;
@@ -8694,9 +8686,10 @@ static noinline int do_walk_down(struct btrfs_trans_handle *trans,
 					     ret);
 			}
 		}
-		ret = btrfs_free_extent(trans, root, bytenr, blocksize,
-					parent, root->root_key.objectid,
-					level - 1, 0, false);
+		btrfs_init_generic_ref(&ref, BTRFS_DROP_DELAYED_REF, bytenr,
+				blocksize, root->root_key.objectid, parent);
+		btrfs_init_tree_ref(&ref, level - 1, root->root_key.objectid);
+		ret = btrfs_free_extent(trans, &ref);
 		if (ret)
 			goto out_unlock;
 	}
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 35c786334972..0a6793498f6c 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -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;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index aa83f68e22ee..12dd3a3f4b1c 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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;
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 7aa240d1f361..d2c3b6560a3f 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -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);
-- 
2.19.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/8] btrfs: extent-tree: Open-code process_func in __btrfs_mod_ref
  2018-12-06  6:58 ` [PATCH 2/8] btrfs: extent-tree: Open-code process_func in __btrfs_mod_ref Qu Wenruo
@ 2018-12-07 17:39   ` Nikolay Borisov
  0 siblings, 0 replies; 23+ messages in thread
From: Nikolay Borisov @ 2018-12-07 17:39 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs



On 6.12.18 г. 8:58 ч., Qu Wenruo wrote:
> The 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.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/extent-tree.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index ea2c3d5220f0..ea68d288d761 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -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;
>  		}
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_tree_ref()
  2018-12-06  6:58 ` [PATCH 3/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_tree_ref() Qu Wenruo
@ 2018-12-10  9:21   ` Nikolay Borisov
  2018-12-12  5:50     ` Qu Wenruo
  0 siblings, 1 reply; 23+ messages in thread
From: Nikolay Borisov @ 2018-12-10  9:21 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs



On 6.12.18 г. 8:58 ч., Qu Wenruo wrote:
> btrfs_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.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  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(-)
> 
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index 11dd46be4017..c42b8ade7b07 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -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);

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.

>  	BUG_ON(extent_op && extent_op->is_data);
>  	ref = kmem_cache_alloc(btrfs_delayed_tree_ref_cachep, GFP_NOFS);
>  	if (!ref)

<snip>

> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -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,
> +			       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.
> +		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>

> @@ -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
> +		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>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_data_ref()
  2018-12-06  6:58 ` [PATCH 4/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_data_ref() Qu Wenruo
@ 2018-12-10  9:23   ` Nikolay Borisov
  0 siblings, 0 replies; 23+ messages in thread
From: Nikolay Borisov @ 2018-12-10  9:23 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs



On 6.12.18 г. 8:58 ч., Qu Wenruo wrote:
> Just like btrfs_add_delayed_tree_ref(), use btrfs_ref to refactor
> btrfs_add_delayed_data_ref().
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  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(-)
> 
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index c42b8ade7b07..09caf1e6fc22 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -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);

generic_ref already dereferenced, no need to assert it.

>  	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);
> diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
> index dbe029c4e01b..a8fde33b43fd 100644
> --- a/fs/btrfs/delayed-ref.h
> +++ b/fs/btrfs/delayed-ref.h
> @@ -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,
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index ecfa0234863b..fa5dd3dfe2e7 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -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;
>  }
>  
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 6/8] btrfs: extent-tree: Use btrfs_ref to refactor add_pinned_bytes()
  2018-12-06  6:59 ` [PATCH 6/8] btrfs: extent-tree: Use btrfs_ref to refactor add_pinned_bytes() Qu Wenruo
@ 2018-12-10  9:45   ` Nikolay Borisov
  0 siblings, 0 replies; 23+ messages in thread
From: Nikolay Borisov @ 2018-12-10  9:45 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs



On 6.12.18 г. 8:59 ч., Qu Wenruo wrote:
> Since 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.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>


> ---
>  fs/btrfs/extent-tree.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 1d812bc2c7fc..70c05ca30d9a 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -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;
>  }
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures
  2018-12-06  6:58 ` [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures Qu Wenruo
@ 2018-12-10  9:48   ` Nikolay Borisov
  2018-12-10 11:20     ` Qu Wenruo
  2018-12-12  5:09     ` About enum convert (Old "Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures") Qu Wenruo
  0 siblings, 2 replies; 23+ messages in thread
From: Nikolay Borisov @ 2018-12-10  9:48 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs



On 6.12.18 г. 8:58 ч., Qu Wenruo wrote:
> 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.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/delayed-ref.h | 109 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 109 insertions(+)
> 
> diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
> index d8fa12d3f2cc..e36d6b05d85e 100644
> --- a/fs/btrfs/delayed-ref.h
> +++ b/fs/btrfs/delayed-ref.h
> @@ -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.

> +	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

> +	 * @ino: 	inode number of the owner. 

And the owner is really the owning inode?

> +	 * @offset:	*CALCULATED* offset. Not EXTENT_DATA key offset.

What does calculated mean here?

> +	 *
> +	 */

Ugh, this is ugly, why not have a single /* */ line above each member
and document them like that?

> +	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.

> +
> +	/*
> +	 * 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.

> +	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;

'fullback' is too terse, use_backreferences or something conveying more
information? Also please use explicit bool type:

bool xxxx: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;

again, explicit bool type please.

> +
> +	/*
> +	 * 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 */

OK, it's common but what does it really contain? Either document it or
rename it to parent_block or something.

> +	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;

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.

> +	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)
>  {
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 7/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref()
  2018-12-06  6:59 ` [PATCH 7/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref() Qu Wenruo
@ 2018-12-10  9:52   ` Nikolay Borisov
  2018-12-10 17:28     ` David Sterba
  0 siblings, 1 reply; 23+ messages in thread
From: Nikolay Borisov @ 2018-12-10  9:52 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs



On 6.12.18 г. 8:59 ч., Qu Wenruo wrote:
> Now we don't need to play the dirty game of reusing @owner for tree block
> level.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  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(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 6f4b1e605736..db3df5ce6087 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -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,
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 70c05ca30d9a..ff60091aef6b 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -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;
>  
> -	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

> +	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?

>  
> -	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>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures
  2018-12-10  9:48   ` Nikolay Borisov
@ 2018-12-10 11:20     ` Qu Wenruo
  2018-12-10 11:32       ` Nikolay Borisov
  2018-12-12  5:09     ` About enum convert (Old "Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures") Qu Wenruo
  1 sibling, 1 reply; 23+ messages in thread
From: Qu Wenruo @ 2018-12-10 11:20 UTC (permalink / raw)
  To: Nikolay Borisov, Qu Wenruo, linux-btrfs



On 2018/12/10 下午5:48, Nikolay Borisov wrote:
> 
> 
> On 6.12.18 г. 8:58 ч., Qu Wenruo wrote:
>> 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.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/delayed-ref.h | 109 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 109 insertions(+)
>>
>> diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
>> index d8fa12d3f2cc..e36d6b05d85e 100644
>> --- a/fs/btrfs/delayed-ref.h
>> +++ b/fs/btrfs/delayed-ref.h
>> @@ -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.

Forgot that.

> 
>> +	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.

> 
>> +	 * @ino: 	inode number of the owner. 
> 
> And the owner is really the owning inode?

Yes, the owning inode.

> 
>> +	 * @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.

> 
>> +	 *
>> +	 */
> 
> 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.

> 
>> +	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.
> 
>> +
>> +	/*
>> +	 * 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.
> 
>> +	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;
> 
> 'fullback' is too terse, use_backreferences or 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?

> Also please use explicit bool type:
> 
> bool xxxx:1

Is this valid? Haven't seen such usage in kernel code IIRC.

> 
>> +
>> +	/*
>> +	 * 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.
> 
>> +
>> +	/*
>> +	 * 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 */
> 
> OK, it's common but what does it really contain? Either document it or
> rename it to parent_block or something.
> 
>> +	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;
> 
> 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

> 
>> +	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)
>>  {
>>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures
  2018-12-10 11:20     ` Qu Wenruo
@ 2018-12-10 11:32       ` Nikolay Borisov
  2018-12-10 11:37         ` Qu Wenruo
  0 siblings, 1 reply; 23+ messages in thread
From: Nikolay Borisov @ 2018-12-10 11:32 UTC (permalink / raw)
  To: Qu Wenruo, Qu Wenruo, linux-btrfs



On 10.12.18 г. 13:20 ч., Qu Wenruo wrote:
> 
> 
> On 2018/12/10 下午5:48, Nikolay Borisov wrote:
>>
>>
>> On 6.12.18 г. 8:58 ч., Qu Wenruo wrote:
>>> 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>

>>> +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.

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.

> 
>>
>>> +	 * @ino: 	inode number of the owner. 
>>
>> And the owner is really the owning inode?
> 
> Yes, the owning inode.
> 
>>
>>> +	 * @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.

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.

> 
>>
>>> +	 *
>>> +	 */
>>
>> 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>

>>> +	/*
>>> +	 * 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?
> 
> @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?

What's the alternative if this bit is not set, how would the tree look like?


> 
>> 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>
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures
  2018-12-10 11:32       ` Nikolay Borisov
@ 2018-12-10 11:37         ` Qu Wenruo
  2018-12-10 17:03           ` David Sterba
  0 siblings, 1 reply; 23+ messages in thread
From: Qu Wenruo @ 2018-12-10 11:37 UTC (permalink / raw)
  To: Nikolay Borisov, Qu Wenruo, linux-btrfs



On 2018/12/10 下午7:32, Nikolay Borisov wrote:
> 
> 
> On 10.12.18 г. 13:20 ч., Qu Wenruo wrote:
>>
>>
>> On 2018/12/10 下午5:48, Nikolay Borisov wrote:
>>>
>>>
>>> On 6.12.18 г. 8:58 ч., Qu Wenruo wrote:
>>>> 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>
> 
>>>> +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.
> 
> 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.
> 
>>
>>>
>>>> +	 * @ino: 	inode number of the owner. 
>>>
>>> And the owner is really the owning inode?
>>
>> Yes, the owning inode.
>>
>>>
>>>> +	 * @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.
> 
> 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.
> 
>>
>>>
>>>> +	 *
>>>> +	 */
>>>
>>> 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.

Understood.

> 
> <snip>
> 
>>>> +	/*
>>>> +	 * 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?
>>
>> @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?
> 
> 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).

> 
> 
>>
>>> 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

> 
> <snip>
>>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures
  2018-12-10 11:37         ` Qu Wenruo
@ 2018-12-10 17:03           ` David Sterba
  0 siblings, 0 replies; 23+ messages in thread
From: David Sterba @ 2018-12-10 17:03 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: Nikolay Borisov, Qu Wenruo, linux-btrfs

On Mon, Dec 10, 2018 at 07:37:57PM +0800, Qu Wenruo wrote:
> >>> 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?

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.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 7/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref()
  2018-12-10  9:52   ` Nikolay Borisov
@ 2018-12-10 17:28     ` David Sterba
  2018-12-11  2:02       ` Qu Wenruo
  0 siblings, 1 reply; 23+ messages in thread
From: David Sterba @ 2018-12-10 17:28 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: Qu Wenruo, linux-btrfs

On Mon, Dec 10, 2018 at 11:52:25AM +0200, Nikolay Borisov wrote:
> On 6.12.18 г. 8:59 ч., Qu Wenruo wrote:
> > Now we don't need to play the dirty game of reusing @owner for tree block
> > level.
> > 
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > ---
> >  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(-)
> > 
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 6f4b1e605736..db3df5ce6087 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -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,
> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > index 70c05ca30d9a..ff60091aef6b 100644
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -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;
> >  
> > -	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
> 
> > +	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.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 7/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref()
  2018-12-10 17:28     ` David Sterba
@ 2018-12-11  2:02       ` Qu Wenruo
  0 siblings, 0 replies; 23+ messages in thread
From: Qu Wenruo @ 2018-12-11  2:02 UTC (permalink / raw)
  To: dsterba, Nikolay Borisov, Qu Wenruo, linux-btrfs



On 2018/12/11 上午1:28, David Sterba wrote:
> On Mon, Dec 10, 2018 at 11:52:25AM +0200, Nikolay Borisov wrote:
>> On 6.12.18 г. 8:59 ч., Qu Wenruo wrote:
>>> Now we don't need to play the dirty game of reusing @owner for tree block
>>> level.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  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(-)
>>>
>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>> index 6f4b1e605736..db3df5ce6087 100644
>>> --- a/fs/btrfs/ctree.h
>>> +++ b/fs/btrfs/ctree.h
>>> @@ -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,
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index 70c05ca30d9a..ff60091aef6b 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -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;
>>>  
>>> -	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

Oh I forgot this one, it should be ASSERT().

>>
>>> +	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().

> 
> 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.

Yep, so I kept the original BUG_ON().
(Although forgot to use ASSERT() for the new check)

> 
> 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.

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

> 
> 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.
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* About enum convert (Old "Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures")
  2018-12-10  9:48   ` Nikolay Borisov
  2018-12-10 11:20     ` Qu Wenruo
@ 2018-12-12  5:09     ` Qu Wenruo
  1 sibling, 0 replies; 23+ messages in thread
From: Qu Wenruo @ 2018-12-12  5:09 UTC (permalink / raw)
  To: Nikolay Borisov, Qu Wenruo, linux-btrfs



On 2018/12/10 下午5:48, Nikolay Borisov wrote:
> 
> 
[snip]
> 
> 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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_tree_ref()
  2018-12-10  9:21   ` Nikolay Borisov
@ 2018-12-12  5:50     ` Qu Wenruo
  0 siblings, 0 replies; 23+ messages in thread
From: Qu Wenruo @ 2018-12-12  5:50 UTC (permalink / raw)
  To: Nikolay Borisov, Qu Wenruo, linux-btrfs



On 2018/12/10 下午5:21, Nikolay Borisov wrote:
> 
> 
> On 6.12.18 г. 8:58 ч., Qu Wenruo wrote:
>> btrfs_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.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  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(-)
>>
>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>> index 11dd46be4017..c42b8ade7b07 100644
>> --- a/fs/btrfs/delayed-ref.c
>> +++ b/fs/btrfs/delayed-ref.c
>> @@ -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);
> 
> 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.
> 
>>  	BUG_ON(extent_op && extent_op->is_data);
>>  	ref = kmem_cache_alloc(btrfs_delayed_tree_ref_cachep, GFP_NOFS);
>>  	if (!ref)
> 
> <snip>
> 
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -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,
>> +			       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.

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

>> +		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>
> 
>> @@ -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
>> +		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>
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, back to index

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-06  6:58 [PATCH 0/8] btrfs: Refactor delayed ref parameter list Qu Wenruo
2018-12-06  6:58 ` [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures Qu Wenruo
2018-12-10  9:48   ` Nikolay Borisov
2018-12-10 11:20     ` Qu Wenruo
2018-12-10 11:32       ` Nikolay Borisov
2018-12-10 11:37         ` Qu Wenruo
2018-12-10 17:03           ` David Sterba
2018-12-12  5:09     ` About enum convert (Old "Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures") Qu Wenruo
2018-12-06  6:58 ` [PATCH 2/8] btrfs: extent-tree: Open-code process_func in __btrfs_mod_ref Qu Wenruo
2018-12-07 17:39   ` Nikolay Borisov
2018-12-06  6:58 ` [PATCH 3/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_tree_ref() Qu Wenruo
2018-12-10  9:21   ` Nikolay Borisov
2018-12-12  5:50     ` Qu Wenruo
2018-12-06  6:58 ` [PATCH 4/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_data_ref() Qu Wenruo
2018-12-10  9:23   ` Nikolay Borisov
2018-12-06  6:59 ` [PATCH 5/8] btrfs: ref-verify: Use btrfs_ref to refactor btrfs_ref_tree_mod() Qu Wenruo
2018-12-06  6:59 ` [PATCH 6/8] btrfs: extent-tree: Use btrfs_ref to refactor add_pinned_bytes() Qu Wenruo
2018-12-10  9:45   ` Nikolay Borisov
2018-12-06  6:59 ` [PATCH 7/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref() Qu Wenruo
2018-12-10  9:52   ` Nikolay Borisov
2018-12-10 17:28     ` David Sterba
2018-12-11  2:02       ` Qu Wenruo
2018-12-06  6:59 ` [PATCH 8/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_free_extent() Qu Wenruo

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable: git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox