[PATCH v2 0/3] Qgroup fix for dirty hack routines

[PATCH v2 0/3] Qgroup fix for dirty hack routines

[Qu Wenruo]

This patchset introduce 2 fixes for data extent owner hacks.

One can be triggered by balance, another one can be trigged by log replay
after power loss.

Root cause are all similar: EXTENT_DATA owner is changed by dirty
hacks, from swapping tree blocks containing EXTENT_DATA to manually
update extent backref without using inc/dec_extent_ref.

The first patch introduces needed functions, then 2 fixes.

The reproducer are all merged into xfstests, btrfs/123 and btrfs/119.

3rd patch stay untouched while 2nd patch get update thanks for the report
from Goldwyn.

Changelog:
v2:
  Update 2nd patch to handle cases where the whole subtree, not only
  level 2 nodes get updated.

Qu Wenruo (3):
  btrfs: qgroup: Refactor btrfs_qgroup_insert_dirty_extent()
  btrfs: relocation: Fix leaking qgroups numbers on data extents
  btrfs: qgroup: Fix qgroup incorrectness caused by log replay

 fs/btrfs/delayed-ref.c |   5 +--
 fs/btrfs/extent-tree.c |  36 +++-------------
 fs/btrfs/qgroup.c      |  39 ++++++++++++++---
 fs/btrfs/qgroup.h      |  44 +++++++++++++++++--
 fs/btrfs/relocation.c  | 114 ++++++++++++++++++++++++++++++++++++++++++++++---
 fs/btrfs/tree-log.c    |  16 +++++++
 6 files changed, 205 insertions(+), 49 deletions(-)

-- 
2.9.2

[PATCH v2 1/3] btrfs: qgroup: Refactor btrfs_qgroup_insert_dirty_extent()

[Qu Wenruo]

Refactor btrfs_qgroup_insert_dirty_extent() function, to two functions:
1. _btrfs_qgroup_insert_dirty_extent()
   Almost the same with original code.
   For delayed_ref usage, which has delayed refs locked.

   Change the return value type to int, since caller never needs the
   pointer, but only needs to know if they need to free the allocated
   memory.

2. btrfs_qgroup_record_dirty_extent()
   The more encapsulated version.

   Will do the delayed_refs lock, memory allocation, quota enabled check
   and other misc things.

The original design is to keep exported functions to minimal, but since
more btrfs hacks exposed, like replacing path in balance, needs us to
record dirty extents manually, so we have to add such functions.

Also, add comment for both functions, to info developers how to keep
qgroup correct when doing hacks.

Cc: Mark Fasheh <mfasheh@suse.de>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/delayed-ref.c |  5 +----
 fs/btrfs/extent-tree.c | 36 +++++-------------------------------
 fs/btrfs/qgroup.c      | 39 ++++++++++++++++++++++++++++++++++-----
 fs/btrfs/qgroup.h      | 44 +++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 81 insertions(+), 43 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 430b368..5eed597 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -541,7 +541,6 @@ add_delayed_ref_head(struct btrfs_fs_info *fs_info,
 	struct btrfs_delayed_ref_head *existing;
 	struct btrfs_delayed_ref_head *head_ref = NULL;
 	struct btrfs_delayed_ref_root *delayed_refs;
-	struct btrfs_qgroup_extent_record *qexisting;
 	int count_mod = 1;
 	int must_insert_reserved = 0;
 
@@ -606,9 +605,7 @@ add_delayed_ref_head(struct btrfs_fs_info *fs_info,
 		qrecord->num_bytes = num_bytes;
 		qrecord->old_roots = NULL;
 
-		qexisting = btrfs_qgroup_insert_dirty_extent(delayed_refs,
-							     qrecord);
-		if (qexisting)
+		if(_btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
 			kfree(qrecord);
 	}
 
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 9fcb8c9..47c85ff 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -8519,34 +8519,6 @@ reada:
 	wc->reada_slot = slot;
 }
 
-/*
- * These may not be seen by the usual inc/dec ref code so we have to
- * add them here.
- */
-static int record_one_subtree_extent(struct btrfs_trans_handle *trans,
-				     struct btrfs_root *root, u64 bytenr,
-				     u64 num_bytes)
-{
-	struct btrfs_qgroup_extent_record *qrecord;
-	struct btrfs_delayed_ref_root *delayed_refs;
-
-	qrecord = kmalloc(sizeof(*qrecord), GFP_NOFS);
-	if (!qrecord)
-		return -ENOMEM;
-
-	qrecord->bytenr = bytenr;
-	qrecord->num_bytes = num_bytes;
-	qrecord->old_roots = NULL;
-
-	delayed_refs = &trans->transaction->delayed_refs;
-	spin_lock(&delayed_refs->lock);
-	if (btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
-		kfree(qrecord);
-	spin_unlock(&delayed_refs->lock);
-
-	return 0;
-}
-
 static int account_leaf_items(struct btrfs_trans_handle *trans,
 			      struct btrfs_root *root,
 			      struct extent_buffer *eb)
@@ -8580,7 +8552,8 @@ static int account_leaf_items(struct btrfs_trans_handle *trans,
 
 		num_bytes = btrfs_file_extent_disk_num_bytes(eb, fi);
 
-		ret = record_one_subtree_extent(trans, root, bytenr, num_bytes);
+		ret = btrfs_qgroup_record_dirty_extent(trans, root->fs_info,
+				bytenr, num_bytes, GFP_NOFS);
 		if (ret)
 			return ret;
 	}
@@ -8729,8 +8702,9 @@ walk_down:
 			btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
 			path->locks[level] = BTRFS_READ_LOCK_BLOCKING;
 
-			ret = record_one_subtree_extent(trans, root, child_bytenr,
-							root->nodesize);
+			ret = btrfs_qgroup_record_dirty_extent(trans,
+					root->fs_info, child_bytenr,
+					root->nodesize, GFP_NOFS);
 			if (ret)
 				goto out;
 		}
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 9d4c05b..76d4f67 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1453,9 +1453,9 @@ int btrfs_qgroup_prepare_account_extents(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-struct btrfs_qgroup_extent_record
-*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root *delayed_refs,
-				  struct btrfs_qgroup_extent_record *record)
+int _btrfs_qgroup_insert_dirty_extent(
+		struct btrfs_delayed_ref_root *delayed_refs,
+		struct btrfs_qgroup_extent_record *record)
 {
 	struct rb_node **p = &delayed_refs->dirty_extent_root.rb_node;
 	struct rb_node *parent_node = NULL;
@@ -1474,12 +1474,41 @@ struct btrfs_qgroup_extent_record
 		else if (bytenr > entry->bytenr)
 			p = &(*p)->rb_right;
 		else
-			return entry;
+			return 1;
 	}
 
 	rb_link_node(&record->node, parent_node, p);
 	rb_insert_color(&record->node, &delayed_refs->dirty_extent_root);
-	return NULL;
+	return 0;
+}
+
+int btrfs_qgroup_record_dirty_extent(struct btrfs_trans_handle *trans,
+		struct btrfs_fs_info *fs_info, u64 bytenr, u64 num_bytes,
+		gfp_t gfp_flag)
+{
+	struct btrfs_qgroup_extent_record *record;
+	struct btrfs_delayed_ref_root *delayed_refs;
+	int ret;
+
+	if (!fs_info->quota_enabled || bytenr == 0 || num_bytes == 0)
+		return 0;
+	if (WARN_ON(trans == NULL))
+		return -EINVAL;
+	record = kmalloc(sizeof(*record), gfp_flag);
+	if (!record)
+		return -ENOMEM;
+
+	delayed_refs = &trans->transaction->delayed_refs;
+	record->bytenr = bytenr;
+	record->num_bytes = num_bytes;
+	record->old_roots = NULL;
+
+	spin_lock(&delayed_refs->lock);
+	ret = _btrfs_qgroup_insert_dirty_extent(delayed_refs, record);
+	spin_unlock(&delayed_refs->lock);
+	if (ret > 0)
+		kfree(record);
+	return 0;
 }
 
 #define UPDATE_NEW	0
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index ecb2c14..f6691e3 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -63,9 +63,47 @@ void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info);
 struct btrfs_delayed_extent_op;
 int btrfs_qgroup_prepare_account_extents(struct btrfs_trans_handle *trans,
 					 struct btrfs_fs_info *fs_info);
-struct btrfs_qgroup_extent_record
-*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root *delayed_refs,
-				  struct btrfs_qgroup_extent_record *record);
+
+/*
+ * Insert one dirty extent record into delayed_refs for qgroup.
+ * Caller must ensure the delayed_refs are already locked and quota is enabled.
+ *
+ * Return 0 if there is no existing dirty extent record for that bytenr, and
+ * insert that record into delayed_refs.
+ * Return > 0 if there is already existing dirty extent record for that bytenr.
+ * Caller then can free the record structure.
+ * Error is not possible
+ *
+ * For caller needs better encapsulated interface, call
+ * btrfs_qgroup_record_dirty_extent(), which will handle locks and memory
+ * allocation.
+ */
+int _btrfs_qgroup_insert_dirty_extent(
+		struct btrfs_delayed_ref_root *delayed_refs,
+		struct btrfs_qgroup_extent_record *record);
+
+/*
+ * Info qgroup to track one extent, so at trans commit time qgroup can
+ * update qgroup accounts for that extent.
+ *
+ * That extent can be either meta or data.
+ * This function can be called several times for any extent and won't cause
+ * any qgroup incorrectness.
+ * As long as dirty extent is recorded, qgroup can hardly go wrong.
+ *
+ * This function should be called when owner of extent is changed and the
+ * code uses hack to avoid normal inc/dev_extent_ref().
+ * For example, swapping two tree blocks in balance or modifying file extent
+ * disk bytenr manually.
+ *
+ * Return 0 if the operation is done.
+ * Return <0 for error, like memory allocation failure or invalid parameter
+ * (NULL trans)
+ */
+int btrfs_qgroup_record_dirty_extent(struct btrfs_trans_handle *trans,
+		struct btrfs_fs_info *fs_info, u64 bytenr, u64 num_bytes,
+		gfp_t gfp_flag);
+
 int
 btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans,
 			    struct btrfs_fs_info *fs_info,
-- 
2.9.2

[PATCH v2 2/3] btrfs: relocation: Fix leaking qgroups numbers on data extents

[Qu Wenruo]

When balancing data extents, qgroup will leak all its numbers for
relocated data extents.

The relocation is done in the following steps for data extents:
1) Create data reloc tree and inode
2) Copy all data extents to data reloc tree
   And commit transaction
3) Create tree reloc tree(special snapshot) for any related subvolumes
4) Replace file extent in tree reloc tree with new extents in data reloc
   tree
   And commit transaction
5) Merge tree reloc tree with original fs, by swapping tree blocks

For 1)~4), since tree reloc tree and data reloc tree doesn't count to
qgroup, everything is OK.

But for 5), the swapping of tree blocks will only info qgroup to track
metadata extents.

If metadata extents contain file extents, qgroup number for file extents
will get lost, leading to corrupted qgroup accounting.

The fix is, before commit transaction of step 5), manually info qgroup to
track all file extents in data reloc tree.
Since at commit transaction time, the tree swapping is done, and qgroup
will account these data extents correctly.

Cc: Mark Fasheh <mfasheh@suse.de>
Reported-by: Mark Fasheh <mfasheh@suse.de>
Reported-by: Filipe Manana <fdmanana@gmail.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
changelog:
v2:
  Iterate all file extents in data reloc tree, instead of iterating
  leafs of a swapped level 1 tree block.
  This fixes case where a level 2 or higher tree block is merged with
  original subvolume.
---
 fs/btrfs/relocation.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 108 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index fc067b0..def7c9c 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -31,6 +31,7 @@
 #include "async-thread.h"
 #include "free-space-cache.h"
 #include "inode-map.h"
+#include "qgroup.h"
 
 /*
  * backref_node, mapping_node and tree_block start with this
@@ -3912,6 +3913,95 @@ int prepare_to_relocate(struct reloc_control *rc)
 	return 0;
 }
 
+/*
+ * Qgroup fixer for data chunk relocation.
+ * The data relocation is done in the following steps
+ * 1) Copy data extents into data reloc tree
+ * 2) Create tree reloc tree(special snapshot) for related subvolumes
+ * 3) Modify file extents in tree reloc tree
+ * 4) Merge tree reloc tree with original fs tree, by swapping tree blocks
+ *
+ * The problem is, data and tree reloc tree are not accounted to qgroup,
+ * and 4) will only info qgroup to track tree blocks change, not file extents
+ * in the tree blocks.
+ *
+ * The good news is, related data extents are all in data reloc tree, so we
+ * only need to info qgroup to track all file extents in data reloc tree
+ * before commit trans.
+ */
+static int qgroup_fix_relocated_data_extents(struct btrfs_trans_handle *trans,
+					     struct reloc_control *rc)
+{
+	struct btrfs_fs_info *fs_info = rc->extent_root->fs_info;
+	struct inode *inode = rc->data_inode;
+	struct btrfs_root *data_reloc_root = BTRFS_I(inode)->root;
+	struct btrfs_path *path;
+	struct btrfs_key key;
+	int ret = 0;
+
+	if (!fs_info->quota_enabled)
+		return 0;
+
+	/*
+	 * Only for stage where we update data pointers the qgroup fix is
+	 * valid.
+	 * For MOVING_DATA stage, we will miss the timing of swapping tree
+	 * blocks, and won't fix it.
+	 */
+	if (!(rc->stage == UPDATE_DATA_PTRS && rc->extents_found))
+		return 0;
+
+	path = btrfs_alloc_path();
+	if (!path)
+		return -ENOMEM;
+	key.objectid = btrfs_ino(inode);
+	key.type = BTRFS_EXTENT_DATA_KEY;
+	key.offset = 0;
+
+	ret = btrfs_search_slot(NULL, data_reloc_root, &key, path, 0, 0);
+	if (ret < 0)
+		goto out;
+
+	lock_extent(&BTRFS_I(inode)->io_tree, 0, (u64)-1);
+	while (1) {
+		struct btrfs_file_extent_item *fi;
+
+		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
+		if (key.objectid > btrfs_ino(inode))
+			break;
+		if (key.type != BTRFS_EXTENT_DATA_KEY)
+			goto next;
+		fi = btrfs_item_ptr(path->nodes[0], path->slots[0],
+				    struct btrfs_file_extent_item);
+		if (btrfs_file_extent_type(path->nodes[0], fi) !=
+				BTRFS_FILE_EXTENT_REG)
+			goto next;
+		/*
+		pr_info("disk bytenr: %llu, num_bytes: %llu\n",
+			btrfs_file_extent_disk_bytenr(path->nodes[0], fi),
+			btrfs_file_extent_disk_num_bytes(path->nodes[0], fi));
+			*/
+		ret = btrfs_qgroup_record_dirty_extent(trans, fs_info,
+			btrfs_file_extent_disk_bytenr(path->nodes[0], fi),
+			btrfs_file_extent_disk_num_bytes(path->nodes[0], fi),
+			GFP_NOFS);
+		if (ret < 0)
+			break;
+next:
+		ret = btrfs_next_item(data_reloc_root, path);
+		if (ret < 0)
+			break;
+		if (ret > 0) {
+			ret = 0;
+			break;
+		}
+	}
+	unlock_extent(&BTRFS_I(inode)->io_tree, 0 , (u64)-1);
+out:
+	btrfs_free_path(path);
+	return ret;
+}
+
 static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 {
 	struct rb_root blocks = RB_ROOT;
@@ -4098,10 +4188,16 @@ restart:
 
 	/* get rid of pinned extents */
 	trans = btrfs_join_transaction(rc->extent_root);
-	if (IS_ERR(trans))
+	if (IS_ERR(trans)) {
 		err = PTR_ERR(trans);
-	else
-		btrfs_commit_transaction(trans, rc->extent_root);
+		goto out_free;
+	}
+	err = qgroup_fix_relocated_data_extents(trans, rc);
+	if (err < 0) {
+		btrfs_abort_transaction(trans, rc->extent_root, err);
+		goto out_free;
+	}
+	btrfs_commit_transaction(trans, rc->extent_root);
 out_free:
 	btrfs_free_block_rsv(rc->extent_root, rc->block_rsv);
 	btrfs_free_path(path);
@@ -4464,10 +4560,16 @@ int btrfs_recover_relocation(struct btrfs_root *root)
 	unset_reloc_control(rc);
 
 	trans = btrfs_join_transaction(rc->extent_root);
-	if (IS_ERR(trans))
+	if (IS_ERR(trans)) {
 		err = PTR_ERR(trans);
-	else
-		err = btrfs_commit_transaction(trans, rc->extent_root);
+		goto out_free;
+	}
+	err = qgroup_fix_relocated_data_extents(trans, rc);
+	if (err < 0) {
+		btrfs_abort_transaction(trans, rc->extent_root, err);
+		goto out_free;
+	}
+	err = btrfs_commit_transaction(trans, rc->extent_root);
 out_free:
 	kfree(rc);
 out:
-- 
2.9.2

[PATCH v2 3/3] btrfs: qgroup: Fix qgroup incorrectness caused by log replay

[Qu Wenruo]

When doing log replay at mount time(after power loss), qgroup will leak
numbers of replayed data extents.

The cause is almost the same of balance.
So fix it by manually informing qgroup for owner changed extents.

The bug can be detected by btrfs/119 test case.

Cc: Mark Fasheh <mfasheh@suse.de>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/tree-log.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index c05f69a..80f8345 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -27,6 +27,7 @@
 #include "backref.h"
 #include "hash.h"
 #include "compression.h"
+#include "qgroup.h"
 
 /* magic values for the inode_only field in btrfs_log_inode:
  *
@@ -680,6 +681,21 @@ static noinline int replay_one_extent(struct btrfs_trans_handle *trans,
 		ins.type = BTRFS_EXTENT_ITEM_KEY;
 		offset = key->offset - btrfs_file_extent_offset(eb, item);
 
+		/*
+		 * Manually record dirty extent, as here we did a shallow
+		 * file extent item copy and skip normal backref update,
+		 * but modify extent tree all by ourselves.
+		 * So need to manually record dirty extent for qgroup,
+		 * as the owner of the file extent changed from log tree
+		 * (doesn't affect qgroup) to fs/file tree(affects qgroup)
+		 */
+		ret = btrfs_qgroup_record_dirty_extent(trans, root->fs_info,
+				btrfs_file_extent_disk_bytenr(eb, item),
+				btrfs_file_extent_disk_num_bytes(eb, item),
+				GFP_NOFS);
+		if (ret < 0)
+			goto out;
+
 		if (ins.objectid > 0) {
 			u64 csum_start;
 			u64 csum_end;
-- 
2.9.2

Re: [PATCH v2 1/3] btrfs: qgroup: Refactor btrfs_qgroup_insert_dirty_extent()

[Goldwyn Rodrigues]

Thanks Qu,

This patch set fixes all the reported problems. However, I have some
minor issues with coding style.


On 08/04/2016 09:29 PM, Qu Wenruo wrote:
> Refactor btrfs_qgroup_insert_dirty_extent() function, to two functions:
> 1. _btrfs_qgroup_insert_dirty_extent()
>    Almost the same with original code.
>    For delayed_ref usage, which has delayed refs locked.
> 
>    Change the return value type to int, since caller never needs the
>    pointer, but only needs to know if they need to free the allocated
>    memory.
> 
> 2. btrfs_qgroup_record_dirty_extent()
>    The more encapsulated version.
> 
>    Will do the delayed_refs lock, memory allocation, quota enabled check
>    and other misc things.
> 
> The original design is to keep exported functions to minimal, but since
> more btrfs hacks exposed, like replacing path in balance, needs us to
> record dirty extents manually, so we have to add such functions.
> 
> Also, add comment for both functions, to info developers how to keep
> qgroup correct when doing hacks.
> 
> Cc: Mark Fasheh <mfasheh@suse.de>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>  fs/btrfs/delayed-ref.c |  5 +----
>  fs/btrfs/extent-tree.c | 36 +++++-------------------------------
>  fs/btrfs/qgroup.c      | 39 ++++++++++++++++++++++++++++++++++-----
>  fs/btrfs/qgroup.h      | 44 +++++++++++++++++++++++++++++++++++++++++---
>  4 files changed, 81 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index 430b368..5eed597 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -541,7 +541,6 @@ add_delayed_ref_head(struct btrfs_fs_info *fs_info,
>  	struct btrfs_delayed_ref_head *existing;
>  	struct btrfs_delayed_ref_head *head_ref = NULL;
>  	struct btrfs_delayed_ref_root *delayed_refs;
> -	struct btrfs_qgroup_extent_record *qexisting;
>  	int count_mod = 1;
>  	int must_insert_reserved = 0;
>  
> @@ -606,9 +605,7 @@ add_delayed_ref_head(struct btrfs_fs_info *fs_info,
>  		qrecord->num_bytes = num_bytes;
>  		qrecord->old_roots = NULL;
>  
> -		qexisting = btrfs_qgroup_insert_dirty_extent(delayed_refs,
> -							     qrecord);
> -		if (qexisting)
> +		if(_btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>  			kfree(qrecord);
>  	}
>  
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 9fcb8c9..47c85ff 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -8519,34 +8519,6 @@ reada:
>  	wc->reada_slot = slot;
>  }
>  
> -/*
> - * These may not be seen by the usual inc/dec ref code so we have to
> - * add them here.
> - */
> -static int record_one_subtree_extent(struct btrfs_trans_handle *trans,
> -				     struct btrfs_root *root, u64 bytenr,
> -				     u64 num_bytes)
> -{
> -	struct btrfs_qgroup_extent_record *qrecord;
> -	struct btrfs_delayed_ref_root *delayed_refs;
> -
> -	qrecord = kmalloc(sizeof(*qrecord), GFP_NOFS);
> -	if (!qrecord)
> -		return -ENOMEM;
> -
> -	qrecord->bytenr = bytenr;
> -	qrecord->num_bytes = num_bytes;
> -	qrecord->old_roots = NULL;
> -
> -	delayed_refs = &trans->transaction->delayed_refs;
> -	spin_lock(&delayed_refs->lock);
> -	if (btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
> -		kfree(qrecord);
> -	spin_unlock(&delayed_refs->lock);
> -
> -	return 0;
> -}
> -
>  static int account_leaf_items(struct btrfs_trans_handle *trans,
>  			      struct btrfs_root *root,
>  			      struct extent_buffer *eb)
> @@ -8580,7 +8552,8 @@ static int account_leaf_items(struct btrfs_trans_handle *trans,
>  
>  		num_bytes = btrfs_file_extent_disk_num_bytes(eb, fi);
>  
> -		ret = record_one_subtree_extent(trans, root, bytenr, num_bytes);
> +		ret = btrfs_qgroup_record_dirty_extent(trans, root->fs_info,
> +				bytenr, num_bytes, GFP_NOFS);
>  		if (ret)
>  			return ret;
>  	}
> @@ -8729,8 +8702,9 @@ walk_down:
>  			btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
>  			path->locks[level] = BTRFS_READ_LOCK_BLOCKING;
>  
> -			ret = record_one_subtree_extent(trans, root, child_bytenr,
> -							root->nodesize);
> +			ret = btrfs_qgroup_record_dirty_extent(trans,
> +					root->fs_info, child_bytenr,
> +					root->nodesize, GFP_NOFS);
>  			if (ret)
>  				goto out;
>  		}
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 9d4c05b..76d4f67 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1453,9 +1453,9 @@ int btrfs_qgroup_prepare_account_extents(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> -struct btrfs_qgroup_extent_record
> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root *delayed_refs,
> -				  struct btrfs_qgroup_extent_record *record)
> +int _btrfs_qgroup_insert_dirty_extent(
> +		struct btrfs_delayed_ref_root *delayed_refs,
> +		struct btrfs_qgroup_extent_record *record)

Why do you rename the function preceding with an underscore? just leave
it as it is.

>  {
>  	struct rb_node **p = &delayed_refs->dirty_extent_root.rb_node;
>  	struct rb_node *parent_node = NULL;
> @@ -1474,12 +1474,41 @@ struct btrfs_qgroup_extent_record
>  		else if (bytenr > entry->bytenr)
>  			p = &(*p)->rb_right;
>  		else
> -			return entry;
> +			return 1;

return -EALREADY?

>  	}
>  
>  	rb_link_node(&record->node, parent_node, p);
>  	rb_insert_color(&record->node, &delayed_refs->dirty_extent_root);
> -	return NULL;
> +	return 0;
> +}
> +
> +int btrfs_qgroup_record_dirty_extent(struct btrfs_trans_handle *trans,
> +		struct btrfs_fs_info *fs_info, u64 bytenr, u64 num_bytes,
> +		gfp_t gfp_flag)
> +{
> +	struct btrfs_qgroup_extent_record *record;
> +	struct btrfs_delayed_ref_root *delayed_refs;
> +	int ret;
> +
> +	if (!fs_info->quota_enabled || bytenr == 0 || num_bytes == 0)
> +		return 0;
> +	if (WARN_ON(trans == NULL))
> +		return -EINVAL;
> +	record = kmalloc(sizeof(*record), gfp_flag);
> +	if (!record)
> +		return -ENOMEM;
> +
> +	delayed_refs = &trans->transaction->delayed_refs;
> +	record->bytenr = bytenr;
> +	record->num_bytes = num_bytes;
> +	record->old_roots = NULL;
> +
> +	spin_lock(&delayed_refs->lock);
> +	ret = _btrfs_qgroup_insert_dirty_extent(delayed_refs, record);
> +	spin_unlock(&delayed_refs->lock);
> +	if (ret > 0)
> +		kfree(record);
> +	return 0;
>  }
>  
>  #define UPDATE_NEW	0
> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
> index ecb2c14..f6691e3 100644
> --- a/fs/btrfs/qgroup.h
> +++ b/fs/btrfs/qgroup.h
> @@ -63,9 +63,47 @@ void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info);
>  struct btrfs_delayed_extent_op;
>  int btrfs_qgroup_prepare_account_extents(struct btrfs_trans_handle *trans,
>  					 struct btrfs_fs_info *fs_info);
> -struct btrfs_qgroup_extent_record
> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root *delayed_refs,
> -				  struct btrfs_qgroup_extent_record *record);
> +
> +/*
> + * Insert one dirty extent record into delayed_refs for qgroup.
> + * Caller must ensure the delayed_refs are already locked and quota is enabled.
> + *
> + * Return 0 if there is no existing dirty extent record for that bytenr, and
> + * insert that record into delayed_refs.
> + * Return > 0 if there is already existing dirty extent record for that bytenr.
> + * Caller then can free the record structure.
> + * Error is not possible

You can remove this if you intend to return EALREADY.

> + *
> + * For caller needs better encapsulated interface, call
> + * btrfs_qgroup_record_dirty_extent(), which will handle locks and memory
> + * allocation.
> + */
> +int _btrfs_qgroup_insert_dirty_extent(
> +		struct btrfs_delayed_ref_root *delayed_refs,
> +		struct btrfs_qgroup_extent_record *record);
> +
> +/*
> + * Info qgroup to track one extent, so at trans commit time qgroup can
> + * update qgroup accounts for that extent.
> + *
> + * That extent can be either meta or data.
> + * This function can be called several times for any extent and won't cause
> + * any qgroup incorrectness.
> + * As long as dirty extent is recorded, qgroup can hardly go wrong.
> + *
> + * This function should be called when owner of extent is changed and the
> + * code uses hack to avoid normal inc/dev_extent_ref().
> + * For example, swapping two tree blocks in balance or modifying file extent
> + * disk bytenr manually.
> + *
> + * Return 0 if the operation is done.
> + * Return <0 for error, like memory allocation failure or invalid parameter
> + * (NULL trans)
> + */
> +int btrfs_qgroup_record_dirty_extent(struct btrfs_trans_handle *trans,
> +		struct btrfs_fs_info *fs_info, u64 bytenr, u64 num_bytes,
> +		gfp_t gfp_flag);
> +
>  int
>  btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans,
>  			    struct btrfs_fs_info *fs_info,
> 

-- 
Goldwyn

Re: [PATCH v2 1/3] btrfs: qgroup: Refactor btrfs_qgroup_insert_dirty_extent()

[Goldwyn Rodrigues]

Thanks Qu,

This patch set fixes all the reported problems. However, I have some
minor issues with coding style.


On 08/04/2016 09:29 PM, Qu Wenruo wrote:
> Refactor btrfs_qgroup_insert_dirty_extent() function, to two functions:
> 1. _btrfs_qgroup_insert_dirty_extent()
>    Almost the same with original code.
>    For delayed_ref usage, which has delayed refs locked.
> 
>    Change the return value type to int, since caller never needs the
>    pointer, but only needs to know if they need to free the allocated
>    memory.
> 
> 2. btrfs_qgroup_record_dirty_extent()
>    The more encapsulated version.
> 
>    Will do the delayed_refs lock, memory allocation, quota enabled check
>    and other misc things.
> 
> The original design is to keep exported functions to minimal, but since
> more btrfs hacks exposed, like replacing path in balance, needs us to
> record dirty extents manually, so we have to add such functions.
> 
> Also, add comment for both functions, to info developers how to keep
> qgroup correct when doing hacks.
> 
> Cc: Mark Fasheh <mfasheh@suse.de>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>  fs/btrfs/delayed-ref.c |  5 +----
>  fs/btrfs/extent-tree.c | 36 +++++-------------------------------
>  fs/btrfs/qgroup.c      | 39 ++++++++++++++++++++++++++++++++++-----
>  fs/btrfs/qgroup.h      | 44 +++++++++++++++++++++++++++++++++++++++++---
>  4 files changed, 81 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index 430b368..5eed597 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -541,7 +541,6 @@ add_delayed_ref_head(struct btrfs_fs_info *fs_info,
>  	struct btrfs_delayed_ref_head *existing;
>  	struct btrfs_delayed_ref_head *head_ref = NULL;
>  	struct btrfs_delayed_ref_root *delayed_refs;
> -	struct btrfs_qgroup_extent_record *qexisting;
>  	int count_mod = 1;
>  	int must_insert_reserved = 0;
>  
> @@ -606,9 +605,7 @@ add_delayed_ref_head(struct btrfs_fs_info *fs_info,
>  		qrecord->num_bytes = num_bytes;
>  		qrecord->old_roots = NULL;
>  
> -		qexisting = btrfs_qgroup_insert_dirty_extent(delayed_refs,
> -							     qrecord);
> -		if (qexisting)
> +		if(_btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>  			kfree(qrecord);
>  	}
>  
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 9fcb8c9..47c85ff 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -8519,34 +8519,6 @@ reada:
>  	wc->reada_slot = slot;
>  }
>  
> -/*
> - * These may not be seen by the usual inc/dec ref code so we have to
> - * add them here.
> - */
> -static int record_one_subtree_extent(struct btrfs_trans_handle *trans,
> -				     struct btrfs_root *root, u64 bytenr,
> -				     u64 num_bytes)
> -{
> -	struct btrfs_qgroup_extent_record *qrecord;
> -	struct btrfs_delayed_ref_root *delayed_refs;
> -
> -	qrecord = kmalloc(sizeof(*qrecord), GFP_NOFS);
> -	if (!qrecord)
> -		return -ENOMEM;
> -
> -	qrecord->bytenr = bytenr;
> -	qrecord->num_bytes = num_bytes;
> -	qrecord->old_roots = NULL;
> -
> -	delayed_refs = &trans->transaction->delayed_refs;
> -	spin_lock(&delayed_refs->lock);
> -	if (btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
> -		kfree(qrecord);
> -	spin_unlock(&delayed_refs->lock);
> -
> -	return 0;
> -}
> -
>  static int account_leaf_items(struct btrfs_trans_handle *trans,
>  			      struct btrfs_root *root,
>  			      struct extent_buffer *eb)
> @@ -8580,7 +8552,8 @@ static int account_leaf_items(struct btrfs_trans_handle *trans,
>  
>  		num_bytes = btrfs_file_extent_disk_num_bytes(eb, fi);
>  
> -		ret = record_one_subtree_extent(trans, root, bytenr, num_bytes);
> +		ret = btrfs_qgroup_record_dirty_extent(trans, root->fs_info,
> +				bytenr, num_bytes, GFP_NOFS);
>  		if (ret)
>  			return ret;
>  	}
> @@ -8729,8 +8702,9 @@ walk_down:
>  			btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
>  			path->locks[level] = BTRFS_READ_LOCK_BLOCKING;
>  
> -			ret = record_one_subtree_extent(trans, root, child_bytenr,
> -							root->nodesize);
> +			ret = btrfs_qgroup_record_dirty_extent(trans,
> +					root->fs_info, child_bytenr,
> +					root->nodesize, GFP_NOFS);
>  			if (ret)
>  				goto out;
>  		}
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 9d4c05b..76d4f67 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1453,9 +1453,9 @@ int btrfs_qgroup_prepare_account_extents(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> -struct btrfs_qgroup_extent_record
> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root *delayed_refs,
> -				  struct btrfs_qgroup_extent_record *record)
> +int _btrfs_qgroup_insert_dirty_extent(
> +		struct btrfs_delayed_ref_root *delayed_refs,
> +		struct btrfs_qgroup_extent_record *record)

Why do you rename the function preceding with an underscore? just leave
it as it is.

>  {
>  	struct rb_node **p = &delayed_refs->dirty_extent_root.rb_node;
>  	struct rb_node *parent_node = NULL;
> @@ -1474,12 +1474,41 @@ struct btrfs_qgroup_extent_record
>  		else if (bytenr > entry->bytenr)
>  			p = &(*p)->rb_right;
>  		else
> -			return entry;
> +			return 1;

return -EALREADY?

>  	}
>  
>  	rb_link_node(&record->node, parent_node, p);
>  	rb_insert_color(&record->node, &delayed_refs->dirty_extent_root);
> -	return NULL;
> +	return 0;
> +}
> +
> +int btrfs_qgroup_record_dirty_extent(struct btrfs_trans_handle *trans,
> +		struct btrfs_fs_info *fs_info, u64 bytenr, u64 num_bytes,
> +		gfp_t gfp_flag)
> +{
> +	struct btrfs_qgroup_extent_record *record;
> +	struct btrfs_delayed_ref_root *delayed_refs;
> +	int ret;
> +
> +	if (!fs_info->quota_enabled || bytenr == 0 || num_bytes == 0)
> +		return 0;
> +	if (WARN_ON(trans == NULL))
> +		return -EINVAL;
> +	record = kmalloc(sizeof(*record), gfp_flag);
> +	if (!record)
> +		return -ENOMEM;
> +
> +	delayed_refs = &trans->transaction->delayed_refs;
> +	record->bytenr = bytenr;
> +	record->num_bytes = num_bytes;
> +	record->old_roots = NULL;
> +
> +	spin_lock(&delayed_refs->lock);
> +	ret = _btrfs_qgroup_insert_dirty_extent(delayed_refs, record);
> +	spin_unlock(&delayed_refs->lock);
> +	if (ret > 0)
> +		kfree(record);
> +	return 0;
>  }
>  
>  #define UPDATE_NEW	0
> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
> index ecb2c14..f6691e3 100644
> --- a/fs/btrfs/qgroup.h
> +++ b/fs/btrfs/qgroup.h
> @@ -63,9 +63,47 @@ void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info);
>  struct btrfs_delayed_extent_op;
>  int btrfs_qgroup_prepare_account_extents(struct btrfs_trans_handle *trans,
>  					 struct btrfs_fs_info *fs_info);
> -struct btrfs_qgroup_extent_record
> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root *delayed_refs,
> -				  struct btrfs_qgroup_extent_record *record);
> +
> +/*
> + * Insert one dirty extent record into delayed_refs for qgroup.
> + * Caller must ensure the delayed_refs are already locked and quota is enabled.
> + *
> + * Return 0 if there is no existing dirty extent record for that bytenr, and
> + * insert that record into delayed_refs.
> + * Return > 0 if there is already existing dirty extent record for that bytenr.
> + * Caller then can free the record structure.
> + * Error is not possible

You can remove this if you intend to return EALREADY.

> + *
> + * For caller needs better encapsulated interface, call
> + * btrfs_qgroup_record_dirty_extent(), which will handle locks and memory
> + * allocation.
> + */
> +int _btrfs_qgroup_insert_dirty_extent(
> +		struct btrfs_delayed_ref_root *delayed_refs,
> +		struct btrfs_qgroup_extent_record *record);
> +
> +/*
> + * Info qgroup to track one extent, so at trans commit time qgroup can
> + * update qgroup accounts for that extent.
> + *
> + * That extent can be either meta or data.
> + * This function can be called several times for any extent and won't cause
> + * any qgroup incorrectness.
> + * As long as dirty extent is recorded, qgroup can hardly go wrong.
> + *
> + * This function should be called when owner of extent is changed and the
> + * code uses hack to avoid normal inc/dev_extent_ref().
> + * For example, swapping two tree blocks in balance or modifying file extent
> + * disk bytenr manually.
> + *
> + * Return 0 if the operation is done.
> + * Return <0 for error, like memory allocation failure or invalid parameter
> + * (NULL trans)
> + */
> +int btrfs_qgroup_record_dirty_extent(struct btrfs_trans_handle *trans,
> +		struct btrfs_fs_info *fs_info, u64 bytenr, u64 num_bytes,
> +		gfp_t gfp_flag);
> +
>  int
>  btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans,
>  			    struct btrfs_fs_info *fs_info,
> 

-- 
Goldwyn

Re: [PATCH v2 1/3] btrfs: qgroup: Refactor btrfs_qgroup_insert_dirty_extent()

[Qu Wenruo]

At 08/07/2016 01:19 AM, Goldwyn Rodrigues wrote:
> Thanks Qu,
>
> This patch set fixes all the reported problems. However, I have some
> minor issues with coding style.
>

Thanks for the test.

>
> On 08/04/2016 09:29 PM, Qu Wenruo wrote:
>> Refactor btrfs_qgroup_insert_dirty_extent() function, to two functions:
>> 1. _btrfs_qgroup_insert_dirty_extent()
>>    Almost the same with original code.
>>    For delayed_ref usage, which has delayed refs locked.
>>
>>    Change the return value type to int, since caller never needs the
>>    pointer, but only needs to know if they need to free the allocated
>>    memory.
>>
>> 2. btrfs_qgroup_record_dirty_extent()
>>    The more encapsulated version.
>>
>>    Will do the delayed_refs lock, memory allocation, quota enabled check
>>    and other misc things.
>>
>> The original design is to keep exported functions to minimal, but since
>> more btrfs hacks exposed, like replacing path in balance, needs us to
>> record dirty extents manually, so we have to add such functions.
>>
>> Also, add comment for both functions, to info developers how to keep
>> qgroup correct when doing hacks.
>>
>> Cc: Mark Fasheh <mfasheh@suse.de>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>>  fs/btrfs/delayed-ref.c |  5 +----
>>  fs/btrfs/extent-tree.c | 36 +++++-------------------------------
>>  fs/btrfs/qgroup.c      | 39 ++++++++++++++++++++++++++++++++++-----
>>  fs/btrfs/qgroup.h      | 44 +++++++++++++++++++++++++++++++++++++++++---
>>  4 files changed, 81 insertions(+), 43 deletions(-)
>>
>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>> index 430b368..5eed597 100644
>> --- a/fs/btrfs/delayed-ref.c
>> +++ b/fs/btrfs/delayed-ref.c
>> @@ -541,7 +541,6 @@ add_delayed_ref_head(struct btrfs_fs_info *fs_info,
>>  	struct btrfs_delayed_ref_head *existing;
>>  	struct btrfs_delayed_ref_head *head_ref = NULL;
>>  	struct btrfs_delayed_ref_root *delayed_refs;
>> -	struct btrfs_qgroup_extent_record *qexisting;
>>  	int count_mod = 1;
>>  	int must_insert_reserved = 0;
>>
>> @@ -606,9 +605,7 @@ add_delayed_ref_head(struct btrfs_fs_info *fs_info,
>>  		qrecord->num_bytes = num_bytes;
>>  		qrecord->old_roots = NULL;
>>
>> -		qexisting = btrfs_qgroup_insert_dirty_extent(delayed_refs,
>> -							     qrecord);
>> -		if (qexisting)
>> +		if(_btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>>  			kfree(qrecord);
>>  	}
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 9fcb8c9..47c85ff 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -8519,34 +8519,6 @@ reada:
>>  	wc->reada_slot = slot;
>>  }
>>
>> -/*
>> - * These may not be seen by the usual inc/dec ref code so we have to
>> - * add them here.
>> - */
>> -static int record_one_subtree_extent(struct btrfs_trans_handle *trans,
>> -				     struct btrfs_root *root, u64 bytenr,
>> -				     u64 num_bytes)
>> -{
>> -	struct btrfs_qgroup_extent_record *qrecord;
>> -	struct btrfs_delayed_ref_root *delayed_refs;
>> -
>> -	qrecord = kmalloc(sizeof(*qrecord), GFP_NOFS);
>> -	if (!qrecord)
>> -		return -ENOMEM;
>> -
>> -	qrecord->bytenr = bytenr;
>> -	qrecord->num_bytes = num_bytes;
>> -	qrecord->old_roots = NULL;
>> -
>> -	delayed_refs = &trans->transaction->delayed_refs;
>> -	spin_lock(&delayed_refs->lock);
>> -	if (btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>> -		kfree(qrecord);
>> -	spin_unlock(&delayed_refs->lock);
>> -
>> -	return 0;
>> -}
>> -
>>  static int account_leaf_items(struct btrfs_trans_handle *trans,
>>  			      struct btrfs_root *root,
>>  			      struct extent_buffer *eb)
>> @@ -8580,7 +8552,8 @@ static int account_leaf_items(struct btrfs_trans_handle *trans,
>>
>>  		num_bytes = btrfs_file_extent_disk_num_bytes(eb, fi);
>>
>> -		ret = record_one_subtree_extent(trans, root, bytenr, num_bytes);
>> +		ret = btrfs_qgroup_record_dirty_extent(trans, root->fs_info,
>> +				bytenr, num_bytes, GFP_NOFS);
>>  		if (ret)
>>  			return ret;
>>  	}
>> @@ -8729,8 +8702,9 @@ walk_down:
>>  			btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
>>  			path->locks[level] = BTRFS_READ_LOCK_BLOCKING;
>>
>> -			ret = record_one_subtree_extent(trans, root, child_bytenr,
>> -							root->nodesize);
>> +			ret = btrfs_qgroup_record_dirty_extent(trans,
>> +					root->fs_info, child_bytenr,
>> +					root->nodesize, GFP_NOFS);
>>  			if (ret)
>>  				goto out;
>>  		}
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 9d4c05b..76d4f67 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -1453,9 +1453,9 @@ int btrfs_qgroup_prepare_account_extents(struct btrfs_trans_handle *trans,
>>  	return ret;
>>  }
>>
>> -struct btrfs_qgroup_extent_record
>> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root *delayed_refs,
>> -				  struct btrfs_qgroup_extent_record *record)
>> +int _btrfs_qgroup_insert_dirty_extent(
>> +		struct btrfs_delayed_ref_root *delayed_refs,
>> +		struct btrfs_qgroup_extent_record *record)
>
> Why do you rename the function preceding with an underscore? just leave
> it as it is.

Because these two functions have different usage.

the underscore one are used when caller has already acquired needed 
lock, which is used by delayed-refs code.

And the one without underscore is for other usage.

Despite the lock, these two functions are all the same.

So I prefer to use underscore.

>
>>  {
>>  	struct rb_node **p = &delayed_refs->dirty_extent_root.rb_node;
>>  	struct rb_node *parent_node = NULL;
>> @@ -1474,12 +1474,41 @@ struct btrfs_qgroup_extent_record
>>  		else if (bytenr > entry->bytenr)
>>  			p = &(*p)->rb_right;
>>  		else
>> -			return entry;
>> +			return 1;
>
> return -EALREADY?

Since the existing case is quite common, I prefer to use >=0 return 
value, other than minus return value which is more common for error case.

Thanks,
Qu

>
>>  	}
>>
>>  	rb_link_node(&record->node, parent_node, p);
>>  	rb_insert_color(&record->node, &delayed_refs->dirty_extent_root);
>> -	return NULL;
>> +	return 0;
>> +}
>> +
>> +int btrfs_qgroup_record_dirty_extent(struct btrfs_trans_handle *trans,
>> +		struct btrfs_fs_info *fs_info, u64 bytenr, u64 num_bytes,
>> +		gfp_t gfp_flag)
>> +{
>> +	struct btrfs_qgroup_extent_record *record;
>> +	struct btrfs_delayed_ref_root *delayed_refs;
>> +	int ret;
>> +
>> +	if (!fs_info->quota_enabled || bytenr == 0 || num_bytes == 0)
>> +		return 0;
>> +	if (WARN_ON(trans == NULL))
>> +		return -EINVAL;
>> +	record = kmalloc(sizeof(*record), gfp_flag);
>> +	if (!record)
>> +		return -ENOMEM;
>> +
>> +	delayed_refs = &trans->transaction->delayed_refs;
>> +	record->bytenr = bytenr;
>> +	record->num_bytes = num_bytes;
>> +	record->old_roots = NULL;
>> +
>> +	spin_lock(&delayed_refs->lock);
>> +	ret = _btrfs_qgroup_insert_dirty_extent(delayed_refs, record);
>> +	spin_unlock(&delayed_refs->lock);
>> +	if (ret > 0)
>> +		kfree(record);
>> +	return 0;
>>  }
>>
>>  #define UPDATE_NEW	0
>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>> index ecb2c14..f6691e3 100644
>> --- a/fs/btrfs/qgroup.h
>> +++ b/fs/btrfs/qgroup.h
>> @@ -63,9 +63,47 @@ void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info);
>>  struct btrfs_delayed_extent_op;
>>  int btrfs_qgroup_prepare_account_extents(struct btrfs_trans_handle *trans,
>>  					 struct btrfs_fs_info *fs_info);
>> -struct btrfs_qgroup_extent_record
>> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root *delayed_refs,
>> -				  struct btrfs_qgroup_extent_record *record);
>> +
>> +/*
>> + * Insert one dirty extent record into delayed_refs for qgroup.
>> + * Caller must ensure the delayed_refs are already locked and quota is enabled.
>> + *
>> + * Return 0 if there is no existing dirty extent record for that bytenr, and
>> + * insert that record into delayed_refs.
>> + * Return > 0 if there is already existing dirty extent record for that bytenr.
>> + * Caller then can free the record structure.
>> + * Error is not possible
>
> You can remove this if you intend to return EALREADY.
>
>> + *
>> + * For caller needs better encapsulated interface, call
>> + * btrfs_qgroup_record_dirty_extent(), which will handle locks and memory
>> + * allocation.
>> + */
>> +int _btrfs_qgroup_insert_dirty_extent(
>> +		struct btrfs_delayed_ref_root *delayed_refs,
>> +		struct btrfs_qgroup_extent_record *record);
>> +
>> +/*
>> + * Info qgroup to track one extent, so at trans commit time qgroup can
>> + * update qgroup accounts for that extent.
>> + *
>> + * That extent can be either meta or data.
>> + * This function can be called several times for any extent and won't cause
>> + * any qgroup incorrectness.
>> + * As long as dirty extent is recorded, qgroup can hardly go wrong.
>> + *
>> + * This function should be called when owner of extent is changed and the
>> + * code uses hack to avoid normal inc/dev_extent_ref().
>> + * For example, swapping two tree blocks in balance or modifying file extent
>> + * disk bytenr manually.
>> + *
>> + * Return 0 if the operation is done.
>> + * Return <0 for error, like memory allocation failure or invalid parameter
>> + * (NULL trans)
>> + */
>> +int btrfs_qgroup_record_dirty_extent(struct btrfs_trans_handle *trans,
>> +		struct btrfs_fs_info *fs_info, u64 bytenr, u64 num_bytes,
>> +		gfp_t gfp_flag);
>> +
>>  int
>>  btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans,
>>  			    struct btrfs_fs_info *fs_info,
>>
>

Re: [PATCH v2 1/3] btrfs: qgroup: Refactor btrfs_qgroup_insert_dirty_extent()

[Goldwyn Rodrigues]

On 08/07/2016 07:39 PM, Qu Wenruo wrote:
> 
> 
> At 08/07/2016 01:19 AM, Goldwyn Rodrigues wrote:
>> Thanks Qu,
>>
>> This patch set fixes all the reported problems. However, I have some
>> minor issues with coding style.
>>
> 
> Thanks for the test.
> 
>>
>> On 08/04/2016 09:29 PM, Qu Wenruo wrote:
>>> Refactor btrfs_qgroup_insert_dirty_extent() function, to two functions:
>>> 1. _btrfs_qgroup_insert_dirty_extent()
>>>    Almost the same with original code.
>>>    For delayed_ref usage, which has delayed refs locked.
>>>
>>>    Change the return value type to int, since caller never needs the
>>>    pointer, but only needs to know if they need to free the allocated
>>>    memory.
>>>
>>> 2. btrfs_qgroup_record_dirty_extent()
>>>    The more encapsulated version.
>>>
>>>    Will do the delayed_refs lock, memory allocation, quota enabled check
>>>    and other misc things.
>>>
>>> The original design is to keep exported functions to minimal, but since
>>> more btrfs hacks exposed, like replacing path in balance, needs us to
>>> record dirty extents manually, so we have to add such functions.
>>>
>>> Also, add comment for both functions, to info developers how to keep
>>> qgroup correct when doing hacks.
>>>
>>> Cc: Mark Fasheh <mfasheh@suse.de>
>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>> ---
>>>  fs/btrfs/delayed-ref.c |  5 +----
>>>  fs/btrfs/extent-tree.c | 36 +++++-------------------------------
>>>  fs/btrfs/qgroup.c      | 39 ++++++++++++++++++++++++++++++++++-----
>>>  fs/btrfs/qgroup.h      | 44
>>> +++++++++++++++++++++++++++++++++++++++++---
>>>  4 files changed, 81 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>>> index 430b368..5eed597 100644
>>> --- a/fs/btrfs/delayed-ref.c
>>> +++ b/fs/btrfs/delayed-ref.c
>>> @@ -541,7 +541,6 @@ add_delayed_ref_head(struct btrfs_fs_info *fs_info,
>>>      struct btrfs_delayed_ref_head *existing;
>>>      struct btrfs_delayed_ref_head *head_ref = NULL;
>>>      struct btrfs_delayed_ref_root *delayed_refs;
>>> -    struct btrfs_qgroup_extent_record *qexisting;
>>>      int count_mod = 1;
>>>      int must_insert_reserved = 0;
>>>
>>> @@ -606,9 +605,7 @@ add_delayed_ref_head(struct btrfs_fs_info *fs_info,
>>>          qrecord->num_bytes = num_bytes;
>>>          qrecord->old_roots = NULL;
>>>
>>> -        qexisting = btrfs_qgroup_insert_dirty_extent(delayed_refs,
>>> -                                 qrecord);
>>> -        if (qexisting)
>>> +        if(_btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>>>              kfree(qrecord);
>>>      }
>>>
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index 9fcb8c9..47c85ff 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -8519,34 +8519,6 @@ reada:
>>>      wc->reada_slot = slot;
>>>  }
>>>
>>> -/*
>>> - * These may not be seen by the usual inc/dec ref code so we have to
>>> - * add them here.
>>> - */
>>> -static int record_one_subtree_extent(struct btrfs_trans_handle *trans,
>>> -                     struct btrfs_root *root, u64 bytenr,
>>> -                     u64 num_bytes)
>>> -{
>>> -    struct btrfs_qgroup_extent_record *qrecord;
>>> -    struct btrfs_delayed_ref_root *delayed_refs;
>>> -
>>> -    qrecord = kmalloc(sizeof(*qrecord), GFP_NOFS);
>>> -    if (!qrecord)
>>> -        return -ENOMEM;
>>> -
>>> -    qrecord->bytenr = bytenr;
>>> -    qrecord->num_bytes = num_bytes;
>>> -    qrecord->old_roots = NULL;
>>> -
>>> -    delayed_refs = &trans->transaction->delayed_refs;
>>> -    spin_lock(&delayed_refs->lock);
>>> -    if (btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>>> -        kfree(qrecord);
>>> -    spin_unlock(&delayed_refs->lock);
>>> -
>>> -    return 0;
>>> -}
>>> -
>>>  static int account_leaf_items(struct btrfs_trans_handle *trans,
>>>                    struct btrfs_root *root,
>>>                    struct extent_buffer *eb)
>>> @@ -8580,7 +8552,8 @@ static int account_leaf_items(struct
>>> btrfs_trans_handle *trans,
>>>
>>>          num_bytes = btrfs_file_extent_disk_num_bytes(eb, fi);
>>>
>>> -        ret = record_one_subtree_extent(trans, root, bytenr,
>>> num_bytes);
>>> +        ret = btrfs_qgroup_record_dirty_extent(trans, root->fs_info,
>>> +                bytenr, num_bytes, GFP_NOFS);
>>>          if (ret)
>>>              return ret;
>>>      }
>>> @@ -8729,8 +8702,9 @@ walk_down:
>>>              btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
>>>              path->locks[level] = BTRFS_READ_LOCK_BLOCKING;
>>>
>>> -            ret = record_one_subtree_extent(trans, root, child_bytenr,
>>> -                            root->nodesize);
>>> +            ret = btrfs_qgroup_record_dirty_extent(trans,
>>> +                    root->fs_info, child_bytenr,
>>> +                    root->nodesize, GFP_NOFS);
>>>              if (ret)
>>>                  goto out;
>>>          }
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index 9d4c05b..76d4f67 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -1453,9 +1453,9 @@ int btrfs_qgroup_prepare_account_extents(struct
>>> btrfs_trans_handle *trans,
>>>      return ret;
>>>  }
>>>
>>> -struct btrfs_qgroup_extent_record
>>> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root
>>> *delayed_refs,
>>> -                  struct btrfs_qgroup_extent_record *record)
>>> +int _btrfs_qgroup_insert_dirty_extent(
>>> +        struct btrfs_delayed_ref_root *delayed_refs,
>>> +        struct btrfs_qgroup_extent_record *record)
>>
>> Why do you rename the function preceding with an underscore? just leave
>> it as it is.
> 
> Because these two functions have different usage.

Where is the "other" function used? AFAICS, you are replacing the
function name.

> 
> the underscore one are used when caller has already acquired needed
> lock, which is used by delayed-refs code.
> 
> And the one without underscore is for other usage.

What/Where is the "other usage"? Please don't say "previous".

> 
> Despite the lock, these two functions are all the same.
> 
> So I prefer to use underscore.

BTW, you missed include/trace/events/btrfs.h, the reference to
btrfs_qgroup_insert_dirty_extent in DEFINE_EVENT.

> 
>>
>>>  {
>>>      struct rb_node **p = &delayed_refs->dirty_extent_root.rb_node;
>>>      struct rb_node *parent_node = NULL;
>>> @@ -1474,12 +1474,41 @@ struct btrfs_qgroup_extent_record
>>>          else if (bytenr > entry->bytenr)
>>>              p = &(*p)->rb_right;
>>>          else
>>> -            return entry;
>>> +            return 1;
>>
>> return -EALREADY?
> 
> Since the existing case is quite common, I prefer to use >=0 return
> value, other than minus return value which is more common for error case.
> 
> Thanks,
> Qu
> 
>>
>>>      }
>>>
>>>      rb_link_node(&record->node, parent_node, p);
>>>      rb_insert_color(&record->node, &delayed_refs->dirty_extent_root);
>>> -    return NULL;
>>> +    return 0;
>>> +}
>>> +
>>> +int btrfs_qgroup_record_dirty_extent(struct btrfs_trans_handle *trans,
>>> +        struct btrfs_fs_info *fs_info, u64 bytenr, u64 num_bytes,
>>> +        gfp_t gfp_flag)
>>> +{
>>> +    struct btrfs_qgroup_extent_record *record;
>>> +    struct btrfs_delayed_ref_root *delayed_refs;
>>> +    int ret;
>>> +
>>> +    if (!fs_info->quota_enabled || bytenr == 0 || num_bytes == 0)
>>> +        return 0;
>>> +    if (WARN_ON(trans == NULL))
>>> +        return -EINVAL;
>>> +    record = kmalloc(sizeof(*record), gfp_flag);
>>> +    if (!record)
>>> +        return -ENOMEM;
>>> +
>>> +    delayed_refs = &trans->transaction->delayed_refs;
>>> +    record->bytenr = bytenr;
>>> +    record->num_bytes = num_bytes;
>>> +    record->old_roots = NULL;
>>> +
>>> +    spin_lock(&delayed_refs->lock);
>>> +    ret = _btrfs_qgroup_insert_dirty_extent(delayed_refs, record);
>>> +    spin_unlock(&delayed_refs->lock);
>>> +    if (ret > 0)
>>> +        kfree(record);
>>> +    return 0;
>>>  }
>>>
>>>  #define UPDATE_NEW    0
>>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>>> index ecb2c14..f6691e3 100644
>>> --- a/fs/btrfs/qgroup.h
>>> +++ b/fs/btrfs/qgroup.h
>>> @@ -63,9 +63,47 @@ void btrfs_free_qgroup_config(struct btrfs_fs_info
>>> *fs_info);
>>>  struct btrfs_delayed_extent_op;
>>>  int btrfs_qgroup_prepare_account_extents(struct btrfs_trans_handle
>>> *trans,
>>>                       struct btrfs_fs_info *fs_info);
>>> -struct btrfs_qgroup_extent_record
>>> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root
>>> *delayed_refs,
>>> -                  struct btrfs_qgroup_extent_record *record);
>>> +
>>> +/*
>>> + * Insert one dirty extent record into delayed_refs for qgroup.
>>> + * Caller must ensure the delayed_refs are already locked and quota
>>> is enabled.
>>> + *
>>> + * Return 0 if there is no existing dirty extent record for that
>>> bytenr, and
>>> + * insert that record into delayed_refs.
>>> + * Return > 0 if there is already existing dirty extent record for
>>> that bytenr.
>>> + * Caller then can free the record structure.
>>> + * Error is not possible
>>
>> You can remove this if you intend to return EALREADY.
>>
>>> + *
>>> + * For caller needs better encapsulated interface, call
>>> + * btrfs_qgroup_record_dirty_extent(), which will handle locks and
>>> memory
>>> + * allocation.
>>> + */
>>> +int _btrfs_qgroup_insert_dirty_extent(
>>> +        struct btrfs_delayed_ref_root *delayed_refs,
>>> +        struct btrfs_qgroup_extent_record *record);
>>> +
>>> +/*
>>> + * Info qgroup to track one extent, so at trans commit time qgroup can
>>> + * update qgroup accounts for that extent.
>>> + *
>>> + * That extent can be either meta or data.
>>> + * This function can be called several times for any extent and
>>> won't cause
>>> + * any qgroup incorrectness.
>>> + * As long as dirty extent is recorded, qgroup can hardly go wrong.
>>> + *
>>> + * This function should be called when owner of extent is changed
>>> and the
>>> + * code uses hack to avoid normal inc/dev_extent_ref().
>>> + * For example, swapping two tree blocks in balance or modifying
>>> file extent
>>> + * disk bytenr manually.
>>> + *
>>> + * Return 0 if the operation is done.
>>> + * Return <0 for error, like memory allocation failure or invalid
>>> parameter
>>> + * (NULL trans)
>>> + */
>>> +int btrfs_qgroup_record_dirty_extent(struct btrfs_trans_handle *trans,
>>> +        struct btrfs_fs_info *fs_info, u64 bytenr, u64 num_bytes,
>>> +        gfp_t gfp_flag);
>>> +
>>>  int
>>>  btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans,
>>>                  struct btrfs_fs_info *fs_info,
>>>
>>
> 
> 
> 

-- 
Goldwyn

Re: [PATCH v2 1/3] btrfs: qgroup: Refactor btrfs_qgroup_insert_dirty_extent()

[Qu Wenruo]

At 08/08/2016 10:54 AM, Goldwyn Rodrigues wrote:
>
>
> On 08/07/2016 07:39 PM, Qu Wenruo wrote:
>>
>>
>> At 08/07/2016 01:19 AM, Goldwyn Rodrigues wrote:
>>> Thanks Qu,
>>>
>>> This patch set fixes all the reported problems. However, I have some
>>> minor issues with coding style.
>>>
>>
>> Thanks for the test.
>>
>>>
>>> On 08/04/2016 09:29 PM, Qu Wenruo wrote:
>>>> Refactor btrfs_qgroup_insert_dirty_extent() function, to two functions:
>>>> 1. _btrfs_qgroup_insert_dirty_extent()
>>>>    Almost the same with original code.
>>>>    For delayed_ref usage, which has delayed refs locked.
>>>>
>>>>    Change the return value type to int, since caller never needs the
>>>>    pointer, but only needs to know if they need to free the allocated
>>>>    memory.
>>>>
>>>> 2. btrfs_qgroup_record_dirty_extent()
>>>>    The more encapsulated version.
>>>>
>>>>    Will do the delayed_refs lock, memory allocation, quota enabled check
>>>>    and other misc things.
>>>>
>>>> The original design is to keep exported functions to minimal, but since
>>>> more btrfs hacks exposed, like replacing path in balance, needs us to
>>>> record dirty extents manually, so we have to add such functions.
>>>>
>>>> Also, add comment for both functions, to info developers how to keep
>>>> qgroup correct when doing hacks.
>>>>
>>>> Cc: Mark Fasheh <mfasheh@suse.de>
>>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>>> ---
>>>>  fs/btrfs/delayed-ref.c |  5 +----
>>>>  fs/btrfs/extent-tree.c | 36 +++++-------------------------------
>>>>  fs/btrfs/qgroup.c      | 39 ++++++++++++++++++++++++++++++++++-----
>>>>  fs/btrfs/qgroup.h      | 44
>>>> +++++++++++++++++++++++++++++++++++++++++---
>>>>  4 files changed, 81 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>>>> index 430b368..5eed597 100644
>>>> --- a/fs/btrfs/delayed-ref.c
>>>> +++ b/fs/btrfs/delayed-ref.c
>>>> @@ -541,7 +541,6 @@ add_delayed_ref_head(struct btrfs_fs_info *fs_info,
>>>>      struct btrfs_delayed_ref_head *existing;
>>>>      struct btrfs_delayed_ref_head *head_ref = NULL;
>>>>      struct btrfs_delayed_ref_root *delayed_refs;
>>>> -    struct btrfs_qgroup_extent_record *qexisting;
>>>>      int count_mod = 1;
>>>>      int must_insert_reserved = 0;
>>>>
>>>> @@ -606,9 +605,7 @@ add_delayed_ref_head(struct btrfs_fs_info *fs_info,
>>>>          qrecord->num_bytes = num_bytes;
>>>>          qrecord->old_roots = NULL;
>>>>
>>>> -        qexisting = btrfs_qgroup_insert_dirty_extent(delayed_refs,
>>>> -                                 qrecord);
>>>> -        if (qexisting)
>>>> +        if(_btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>>>>              kfree(qrecord);
>>>>      }
>>>>
>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>> index 9fcb8c9..47c85ff 100644
>>>> --- a/fs/btrfs/extent-tree.c
>>>> +++ b/fs/btrfs/extent-tree.c
>>>> @@ -8519,34 +8519,6 @@ reada:
>>>>      wc->reada_slot = slot;
>>>>  }
>>>>
>>>> -/*
>>>> - * These may not be seen by the usual inc/dec ref code so we have to
>>>> - * add them here.
>>>> - */
>>>> -static int record_one_subtree_extent(struct btrfs_trans_handle *trans,
>>>> -                     struct btrfs_root *root, u64 bytenr,
>>>> -                     u64 num_bytes)
>>>> -{
>>>> -    struct btrfs_qgroup_extent_record *qrecord;
>>>> -    struct btrfs_delayed_ref_root *delayed_refs;
>>>> -
>>>> -    qrecord = kmalloc(sizeof(*qrecord), GFP_NOFS);
>>>> -    if (!qrecord)
>>>> -        return -ENOMEM;
>>>> -
>>>> -    qrecord->bytenr = bytenr;
>>>> -    qrecord->num_bytes = num_bytes;
>>>> -    qrecord->old_roots = NULL;
>>>> -
>>>> -    delayed_refs = &trans->transaction->delayed_refs;
>>>> -    spin_lock(&delayed_refs->lock);
>>>> -    if (btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>>>> -        kfree(qrecord);
>>>> -    spin_unlock(&delayed_refs->lock);
>>>> -
>>>> -    return 0;
>>>> -}
>>>> -
>>>>  static int account_leaf_items(struct btrfs_trans_handle *trans,
>>>>                    struct btrfs_root *root,
>>>>                    struct extent_buffer *eb)
>>>> @@ -8580,7 +8552,8 @@ static int account_leaf_items(struct
>>>> btrfs_trans_handle *trans,
>>>>
>>>>          num_bytes = btrfs_file_extent_disk_num_bytes(eb, fi);
>>>>
>>>> -        ret = record_one_subtree_extent(trans, root, bytenr,
>>>> num_bytes);
>>>> +        ret = btrfs_qgroup_record_dirty_extent(trans, root->fs_info,
>>>> +                bytenr, num_bytes, GFP_NOFS);
>>>>          if (ret)
>>>>              return ret;
>>>>      }
>>>> @@ -8729,8 +8702,9 @@ walk_down:
>>>>              btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
>>>>              path->locks[level] = BTRFS_READ_LOCK_BLOCKING;
>>>>
>>>> -            ret = record_one_subtree_extent(trans, root, child_bytenr,
>>>> -                            root->nodesize);
>>>> +            ret = btrfs_qgroup_record_dirty_extent(trans,
>>>> +                    root->fs_info, child_bytenr,
>>>> +                    root->nodesize, GFP_NOFS);
>>>>              if (ret)
>>>>                  goto out;
>>>>          }
>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>> index 9d4c05b..76d4f67 100644
>>>> --- a/fs/btrfs/qgroup.c
>>>> +++ b/fs/btrfs/qgroup.c
>>>> @@ -1453,9 +1453,9 @@ int btrfs_qgroup_prepare_account_extents(struct
>>>> btrfs_trans_handle *trans,
>>>>      return ret;
>>>>  }
>>>>
>>>> -struct btrfs_qgroup_extent_record
>>>> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root
>>>> *delayed_refs,
>>>> -                  struct btrfs_qgroup_extent_record *record)
>>>> +int _btrfs_qgroup_insert_dirty_extent(
>>>> +        struct btrfs_delayed_ref_root *delayed_refs,
>>>> +        struct btrfs_qgroup_extent_record *record)
>>>
>>> Why do you rename the function preceding with an underscore? just leave
>>> it as it is.
>>
>> Because these two functions have different usage.
>
> Where is the "other" function used? AFAICS, you are replacing the
> function name.

See 2nd and 3rd patch.
Or be more specific, dirty hack fixes, which is the case where we need 
to do manual tracking for qgroup.

>
>>
>> the underscore one are used when caller has already acquired needed
>> lock, which is used by delayed-refs code.
>>
>> And the one without underscore is for other usage.
>
> What/Where is the "other usage"? Please don't say "previous".
>

See above.

>>
>> Despite the lock, these two functions are all the same.
>>
>> So I prefer to use underscore.
>
> BTW, you missed include/trace/events/btrfs.h, the reference to
> btrfs_qgroup_insert_dirty_extent in DEFINE_EVENT.
>

Since btrfs_qgroup_insert_dirty_extent() is just calling the underscore 
one with proper lock content, so the trace is not affected.

Thanks,
Qu

>>
>>>
>>>>  {
>>>>      struct rb_node **p = &delayed_refs->dirty_extent_root.rb_node;
>>>>      struct rb_node *parent_node = NULL;
>>>> @@ -1474,12 +1474,41 @@ struct btrfs_qgroup_extent_record
>>>>          else if (bytenr > entry->bytenr)
>>>>              p = &(*p)->rb_right;
>>>>          else
>>>> -            return entry;
>>>> +            return 1;
>>>
>>> return -EALREADY?
>>
>> Since the existing case is quite common, I prefer to use >=0 return
>> value, other than minus return value which is more common for error case.
>>
>> Thanks,
>> Qu
>>
>>>
>>>>      }
>>>>
>>>>      rb_link_node(&record->node, parent_node, p);
>>>>      rb_insert_color(&record->node, &delayed_refs->dirty_extent_root);
>>>> -    return NULL;
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +int btrfs_qgroup_record_dirty_extent(struct btrfs_trans_handle *trans,
>>>> +        struct btrfs_fs_info *fs_info, u64 bytenr, u64 num_bytes,
>>>> +        gfp_t gfp_flag)
>>>> +{
>>>> +    struct btrfs_qgroup_extent_record *record;
>>>> +    struct btrfs_delayed_ref_root *delayed_refs;
>>>> +    int ret;
>>>> +
>>>> +    if (!fs_info->quota_enabled || bytenr == 0 || num_bytes == 0)
>>>> +        return 0;
>>>> +    if (WARN_ON(trans == NULL))
>>>> +        return -EINVAL;
>>>> +    record = kmalloc(sizeof(*record), gfp_flag);
>>>> +    if (!record)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    delayed_refs = &trans->transaction->delayed_refs;
>>>> +    record->bytenr = bytenr;
>>>> +    record->num_bytes = num_bytes;
>>>> +    record->old_roots = NULL;
>>>> +
>>>> +    spin_lock(&delayed_refs->lock);
>>>> +    ret = _btrfs_qgroup_insert_dirty_extent(delayed_refs, record);
>>>> +    spin_unlock(&delayed_refs->lock);
>>>> +    if (ret > 0)
>>>> +        kfree(record);
>>>> +    return 0;
>>>>  }
>>>>
>>>>  #define UPDATE_NEW    0
>>>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>>>> index ecb2c14..f6691e3 100644
>>>> --- a/fs/btrfs/qgroup.h
>>>> +++ b/fs/btrfs/qgroup.h
>>>> @@ -63,9 +63,47 @@ void btrfs_free_qgroup_config(struct btrfs_fs_info
>>>> *fs_info);
>>>>  struct btrfs_delayed_extent_op;
>>>>  int btrfs_qgroup_prepare_account_extents(struct btrfs_trans_handle
>>>> *trans,
>>>>                       struct btrfs_fs_info *fs_info);
>>>> -struct btrfs_qgroup_extent_record
>>>> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root
>>>> *delayed_refs,
>>>> -                  struct btrfs_qgroup_extent_record *record);
>>>> +
>>>> +/*
>>>> + * Insert one dirty extent record into delayed_refs for qgroup.
>>>> + * Caller must ensure the delayed_refs are already locked and quota
>>>> is enabled.
>>>> + *
>>>> + * Return 0 if there is no existing dirty extent record for that
>>>> bytenr, and
>>>> + * insert that record into delayed_refs.
>>>> + * Return > 0 if there is already existing dirty extent record for
>>>> that bytenr.
>>>> + * Caller then can free the record structure.
>>>> + * Error is not possible
>>>
>>> You can remove this if you intend to return EALREADY.
>>>
>>>> + *
>>>> + * For caller needs better encapsulated interface, call
>>>> + * btrfs_qgroup_record_dirty_extent(), which will handle locks and
>>>> memory
>>>> + * allocation.
>>>> + */
>>>> +int _btrfs_qgroup_insert_dirty_extent(
>>>> +        struct btrfs_delayed_ref_root *delayed_refs,
>>>> +        struct btrfs_qgroup_extent_record *record);
>>>> +
>>>> +/*
>>>> + * Info qgroup to track one extent, so at trans commit time qgroup can
>>>> + * update qgroup accounts for that extent.
>>>> + *
>>>> + * That extent can be either meta or data.
>>>> + * This function can be called several times for any extent and
>>>> won't cause
>>>> + * any qgroup incorrectness.
>>>> + * As long as dirty extent is recorded, qgroup can hardly go wrong.
>>>> + *
>>>> + * This function should be called when owner of extent is changed
>>>> and the
>>>> + * code uses hack to avoid normal inc/dev_extent_ref().
>>>> + * For example, swapping two tree blocks in balance or modifying
>>>> file extent
>>>> + * disk bytenr manually.
>>>> + *
>>>> + * Return 0 if the operation is done.
>>>> + * Return <0 for error, like memory allocation failure or invalid
>>>> parameter
>>>> + * (NULL trans)
>>>> + */
>>>> +int btrfs_qgroup_record_dirty_extent(struct btrfs_trans_handle *trans,
>>>> +        struct btrfs_fs_info *fs_info, u64 bytenr, u64 num_bytes,
>>>> +        gfp_t gfp_flag);
>>>> +
>>>>  int
>>>>  btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans,
>>>>                  struct btrfs_fs_info *fs_info,
>>>>
>>>
>>
>>
>>
>

Re: [PATCH v2 1/3] btrfs: qgroup: Refactor btrfs_qgroup_insert_dirty_extent()

[Goldwyn Rodrigues]

On 08/08/2016 07:26 PM, Qu Wenruo wrote:
> 
> 
> At 08/08/2016 10:54 AM, Goldwyn Rodrigues wrote:
>>
>>
>> On 08/07/2016 07:39 PM, Qu Wenruo wrote:
>>>
>>>
>>> At 08/07/2016 01:19 AM, Goldwyn Rodrigues wrote:
>>>> Thanks Qu,
>>>>
>>>> This patch set fixes all the reported problems. However, I have some
>>>> minor issues with coding style.
>>>>
>>>
>>> Thanks for the test.
>>>
>>>>
>>>> On 08/04/2016 09:29 PM, Qu Wenruo wrote:
>>>>> Refactor btrfs_qgroup_insert_dirty_extent() function, to two
>>>>> functions:
>>>>> 1. _btrfs_qgroup_insert_dirty_extent()
>>>>>    Almost the same with original code.
>>>>>    For delayed_ref usage, which has delayed refs locked.
>>>>>
>>>>>    Change the return value type to int, since caller never needs the
>>>>>    pointer, but only needs to know if they need to free the allocated
>>>>>    memory.
>>>>>
>>>>> 2. btrfs_qgroup_record_dirty_extent()
>>>>>    The more encapsulated version.
>>>>>
>>>>>    Will do the delayed_refs lock, memory allocation, quota enabled
>>>>> check
>>>>>    and other misc things.
>>>>>
>>>>> The original design is to keep exported functions to minimal, but
>>>>> since
>>>>> more btrfs hacks exposed, like replacing path in balance, needs us to
>>>>> record dirty extents manually, so we have to add such functions.
>>>>>
>>>>> Also, add comment for both functions, to info developers how to keep
>>>>> qgroup correct when doing hacks.
>>>>>
>>>>> Cc: Mark Fasheh <mfasheh@suse.de>
>>>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>>>> ---
>>>>>  fs/btrfs/delayed-ref.c |  5 +----
>>>>>  fs/btrfs/extent-tree.c | 36 +++++-------------------------------
>>>>>  fs/btrfs/qgroup.c      | 39 ++++++++++++++++++++++++++++++++++-----
>>>>>  fs/btrfs/qgroup.h      | 44
>>>>> +++++++++++++++++++++++++++++++++++++++++---
>>>>>  4 files changed, 81 insertions(+), 43 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>>>>> index 430b368..5eed597 100644
>>>>> --- a/fs/btrfs/delayed-ref.c
>>>>> +++ b/fs/btrfs/delayed-ref.c
>>>>> @@ -541,7 +541,6 @@ add_delayed_ref_head(struct btrfs_fs_info
>>>>> *fs_info,
>>>>>      struct btrfs_delayed_ref_head *existing;
>>>>>      struct btrfs_delayed_ref_head *head_ref = NULL;
>>>>>      struct btrfs_delayed_ref_root *delayed_refs;
>>>>> -    struct btrfs_qgroup_extent_record *qexisting;
>>>>>      int count_mod = 1;
>>>>>      int must_insert_reserved = 0;
>>>>>
>>>>> @@ -606,9 +605,7 @@ add_delayed_ref_head(struct btrfs_fs_info
>>>>> *fs_info,
>>>>>          qrecord->num_bytes = num_bytes;
>>>>>          qrecord->old_roots = NULL;
>>>>>
>>>>> -        qexisting = btrfs_qgroup_insert_dirty_extent(delayed_refs,
>>>>> -                                 qrecord);
>>>>> -        if (qexisting)
>>>>> +        if(_btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>>>>>              kfree(qrecord);
>>>>>      }
>>>>>
>>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>>> index 9fcb8c9..47c85ff 100644
>>>>> --- a/fs/btrfs/extent-tree.c
>>>>> +++ b/fs/btrfs/extent-tree.c
>>>>> @@ -8519,34 +8519,6 @@ reada:
>>>>>      wc->reada_slot = slot;
>>>>>  }
>>>>>
>>>>> -/*
>>>>> - * These may not be seen by the usual inc/dec ref code so we have to
>>>>> - * add them here.
>>>>> - */
>>>>> -static int record_one_subtree_extent(struct btrfs_trans_handle
>>>>> *trans,
>>>>> -                     struct btrfs_root *root, u64 bytenr,
>>>>> -                     u64 num_bytes)
>>>>> -{
>>>>> -    struct btrfs_qgroup_extent_record *qrecord;
>>>>> -    struct btrfs_delayed_ref_root *delayed_refs;
>>>>> -
>>>>> -    qrecord = kmalloc(sizeof(*qrecord), GFP_NOFS);
>>>>> -    if (!qrecord)
>>>>> -        return -ENOMEM;
>>>>> -
>>>>> -    qrecord->bytenr = bytenr;
>>>>> -    qrecord->num_bytes = num_bytes;
>>>>> -    qrecord->old_roots = NULL;
>>>>> -
>>>>> -    delayed_refs = &trans->transaction->delayed_refs;
>>>>> -    spin_lock(&delayed_refs->lock);
>>>>> -    if (btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>>>>> -        kfree(qrecord);
>>>>> -    spin_unlock(&delayed_refs->lock);
>>>>> -
>>>>> -    return 0;
>>>>> -}
>>>>> -
>>>>>  static int account_leaf_items(struct btrfs_trans_handle *trans,
>>>>>                    struct btrfs_root *root,
>>>>>                    struct extent_buffer *eb)
>>>>> @@ -8580,7 +8552,8 @@ static int account_leaf_items(struct
>>>>> btrfs_trans_handle *trans,
>>>>>
>>>>>          num_bytes = btrfs_file_extent_disk_num_bytes(eb, fi);
>>>>>
>>>>> -        ret = record_one_subtree_extent(trans, root, bytenr,
>>>>> num_bytes);
>>>>> +        ret = btrfs_qgroup_record_dirty_extent(trans, root->fs_info,
>>>>> +                bytenr, num_bytes, GFP_NOFS);
>>>>>          if (ret)
>>>>>              return ret;
>>>>>      }
>>>>> @@ -8729,8 +8702,9 @@ walk_down:
>>>>>              btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
>>>>>              path->locks[level] = BTRFS_READ_LOCK_BLOCKING;
>>>>>
>>>>> -            ret = record_one_subtree_extent(trans, root,
>>>>> child_bytenr,
>>>>> -                            root->nodesize);
>>>>> +            ret = btrfs_qgroup_record_dirty_extent(trans,
>>>>> +                    root->fs_info, child_bytenr,
>>>>> +                    root->nodesize, GFP_NOFS);
>>>>>              if (ret)
>>>>>                  goto out;
>>>>>          }
>>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>>> index 9d4c05b..76d4f67 100644
>>>>> --- a/fs/btrfs/qgroup.c
>>>>> +++ b/fs/btrfs/qgroup.c
>>>>> @@ -1453,9 +1453,9 @@ int btrfs_qgroup_prepare_account_extents(struct
>>>>> btrfs_trans_handle *trans,
>>>>>      return ret;
>>>>>  }
>>>>>
>>>>> -struct btrfs_qgroup_extent_record
>>>>> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root
>>>>> *delayed_refs,
>>>>> -                  struct btrfs_qgroup_extent_record *record)
>>>>> +int _btrfs_qgroup_insert_dirty_extent(
>>>>> +        struct btrfs_delayed_ref_root *delayed_refs,
>>>>> +        struct btrfs_qgroup_extent_record *record)
>>>>
>>>> Why do you rename the function preceding with an underscore? just leave
>>>> it as it is.
>>>
>>> Because these two functions have different usage.
>>
>> Where is the "other" function used? AFAICS, you are replacing the
>> function name.
> 
> See 2nd and 3rd patch.
> Or be more specific, dirty hack fixes, which is the case where we need
> to do manual tracking for qgroup.

I still don't see it. Can you be more specific with function names to
help me understand?

All you are doing is adding is btrfs_qgroup_record_dirty_extent() which
calls _btrfs_qgroup_insert_dirty_extent() with the locks. Is that what
you are referring to?


> 
>>
>>>
>>> the underscore one are used when caller has already acquired needed
>>> lock, which is used by delayed-refs code.
>>>
>>> And the one without underscore is for other usage.
>>
>> What/Where is the "other usage"? Please don't say "previous".
>>
> 
> See above.
> 
>>>
>>> Despite the lock, these two functions are all the same.
>>>
>>> So I prefer to use underscore.
>>
>> BTW, you missed include/trace/events/btrfs.h, the reference to
>> btrfs_qgroup_insert_dirty_extent in DEFINE_EVENT.
>>
> 
> Since btrfs_qgroup_insert_dirty_extent() is just calling the underscore
> one with proper lock content, so the trace is not affected.

There is NO btrfs_qgroup_insert_dirty_extent(). Your patches REMOVED it
and replaced it with _btrfs_qgroup_insert_dirty_extent(). Only
btrfs_qgroup_record_dirty_extent() which is not in trace.


<snipped>

-- 
Goldwyn

Re: [PATCH v2 1/3] btrfs: qgroup: Refactor btrfs_qgroup_insert_dirty_extent()

[Qu Wenruo]

At 08/09/2016 09:01 AM, Goldwyn Rodrigues wrote:
>
>
> On 08/08/2016 07:26 PM, Qu Wenruo wrote:
>>
>>
>> At 08/08/2016 10:54 AM, Goldwyn Rodrigues wrote:
>>>
>>>
>>> On 08/07/2016 07:39 PM, Qu Wenruo wrote:
>>>>
>>>>
>>>> At 08/07/2016 01:19 AM, Goldwyn Rodrigues wrote:
>>>>> Thanks Qu,
>>>>>
>>>>> This patch set fixes all the reported problems. However, I have some
>>>>> minor issues with coding style.
>>>>>
>>>>
>>>> Thanks for the test.
>>>>
>>>>>
>>>>> On 08/04/2016 09:29 PM, Qu Wenruo wrote:
>>>>>> Refactor btrfs_qgroup_insert_dirty_extent() function, to two
>>>>>> functions:
>>>>>> 1. _btrfs_qgroup_insert_dirty_extent()
>>>>>>    Almost the same with original code.
>>>>>>    For delayed_ref usage, which has delayed refs locked.
>>>>>>
>>>>>>    Change the return value type to int, since caller never needs the
>>>>>>    pointer, but only needs to know if they need to free the allocated
>>>>>>    memory.
>>>>>>
>>>>>> 2. btrfs_qgroup_record_dirty_extent()
>>>>>>    The more encapsulated version.
>>>>>>
>>>>>>    Will do the delayed_refs lock, memory allocation, quota enabled
>>>>>> check
>>>>>>    and other misc things.
>>>>>>
>>>>>> The original design is to keep exported functions to minimal, but
>>>>>> since
>>>>>> more btrfs hacks exposed, like replacing path in balance, needs us to
>>>>>> record dirty extents manually, so we have to add such functions.
>>>>>>
>>>>>> Also, add comment for both functions, to info developers how to keep
>>>>>> qgroup correct when doing hacks.
>>>>>>
>>>>>> Cc: Mark Fasheh <mfasheh@suse.de>
>>>>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>>>>> ---
>>>>>>  fs/btrfs/delayed-ref.c |  5 +----
>>>>>>  fs/btrfs/extent-tree.c | 36 +++++-------------------------------
>>>>>>  fs/btrfs/qgroup.c      | 39 ++++++++++++++++++++++++++++++++++-----
>>>>>>  fs/btrfs/qgroup.h      | 44
>>>>>> +++++++++++++++++++++++++++++++++++++++++---
>>>>>>  4 files changed, 81 insertions(+), 43 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>>>>>> index 430b368..5eed597 100644
>>>>>> --- a/fs/btrfs/delayed-ref.c
>>>>>> +++ b/fs/btrfs/delayed-ref.c
>>>>>> @@ -541,7 +541,6 @@ add_delayed_ref_head(struct btrfs_fs_info
>>>>>> *fs_info,
>>>>>>      struct btrfs_delayed_ref_head *existing;
>>>>>>      struct btrfs_delayed_ref_head *head_ref = NULL;
>>>>>>      struct btrfs_delayed_ref_root *delayed_refs;
>>>>>> -    struct btrfs_qgroup_extent_record *qexisting;
>>>>>>      int count_mod = 1;
>>>>>>      int must_insert_reserved = 0;
>>>>>>
>>>>>> @@ -606,9 +605,7 @@ add_delayed_ref_head(struct btrfs_fs_info
>>>>>> *fs_info,
>>>>>>          qrecord->num_bytes = num_bytes;
>>>>>>          qrecord->old_roots = NULL;
>>>>>>
>>>>>> -        qexisting = btrfs_qgroup_insert_dirty_extent(delayed_refs,
>>>>>> -                                 qrecord);
>>>>>> -        if (qexisting)
>>>>>> +        if(_btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>>>>>>              kfree(qrecord);
>>>>>>      }
>>>>>>
>>>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>>>> index 9fcb8c9..47c85ff 100644
>>>>>> --- a/fs/btrfs/extent-tree.c
>>>>>> +++ b/fs/btrfs/extent-tree.c
>>>>>> @@ -8519,34 +8519,6 @@ reada:
>>>>>>      wc->reada_slot = slot;
>>>>>>  }
>>>>>>
>>>>>> -/*
>>>>>> - * These may not be seen by the usual inc/dec ref code so we have to
>>>>>> - * add them here.
>>>>>> - */
>>>>>> -static int record_one_subtree_extent(struct btrfs_trans_handle
>>>>>> *trans,
>>>>>> -                     struct btrfs_root *root, u64 bytenr,
>>>>>> -                     u64 num_bytes)
>>>>>> -{
>>>>>> -    struct btrfs_qgroup_extent_record *qrecord;
>>>>>> -    struct btrfs_delayed_ref_root *delayed_refs;
>>>>>> -
>>>>>> -    qrecord = kmalloc(sizeof(*qrecord), GFP_NOFS);
>>>>>> -    if (!qrecord)
>>>>>> -        return -ENOMEM;
>>>>>> -
>>>>>> -    qrecord->bytenr = bytenr;
>>>>>> -    qrecord->num_bytes = num_bytes;
>>>>>> -    qrecord->old_roots = NULL;
>>>>>> -
>>>>>> -    delayed_refs = &trans->transaction->delayed_refs;
>>>>>> -    spin_lock(&delayed_refs->lock);
>>>>>> -    if (btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>>>>>> -        kfree(qrecord);
>>>>>> -    spin_unlock(&delayed_refs->lock);
>>>>>> -
>>>>>> -    return 0;
>>>>>> -}
>>>>>> -
>>>>>>  static int account_leaf_items(struct btrfs_trans_handle *trans,
>>>>>>                    struct btrfs_root *root,
>>>>>>                    struct extent_buffer *eb)
>>>>>> @@ -8580,7 +8552,8 @@ static int account_leaf_items(struct
>>>>>> btrfs_trans_handle *trans,
>>>>>>
>>>>>>          num_bytes = btrfs_file_extent_disk_num_bytes(eb, fi);
>>>>>>
>>>>>> -        ret = record_one_subtree_extent(trans, root, bytenr,
>>>>>> num_bytes);
>>>>>> +        ret = btrfs_qgroup_record_dirty_extent(trans, root->fs_info,
>>>>>> +                bytenr, num_bytes, GFP_NOFS);
>>>>>>          if (ret)
>>>>>>              return ret;
>>>>>>      }
>>>>>> @@ -8729,8 +8702,9 @@ walk_down:
>>>>>>              btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
>>>>>>              path->locks[level] = BTRFS_READ_LOCK_BLOCKING;
>>>>>>
>>>>>> -            ret = record_one_subtree_extent(trans, root,
>>>>>> child_bytenr,
>>>>>> -                            root->nodesize);
>>>>>> +            ret = btrfs_qgroup_record_dirty_extent(trans,
>>>>>> +                    root->fs_info, child_bytenr,
>>>>>> +                    root->nodesize, GFP_NOFS);
>>>>>>              if (ret)
>>>>>>                  goto out;
>>>>>>          }
>>>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>>>> index 9d4c05b..76d4f67 100644
>>>>>> --- a/fs/btrfs/qgroup.c
>>>>>> +++ b/fs/btrfs/qgroup.c
>>>>>> @@ -1453,9 +1453,9 @@ int btrfs_qgroup_prepare_account_extents(struct
>>>>>> btrfs_trans_handle *trans,
>>>>>>      return ret;
>>>>>>  }
>>>>>>
>>>>>> -struct btrfs_qgroup_extent_record
>>>>>> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root
>>>>>> *delayed_refs,
>>>>>> -                  struct btrfs_qgroup_extent_record *record)
>>>>>> +int _btrfs_qgroup_insert_dirty_extent(
>>>>>> +        struct btrfs_delayed_ref_root *delayed_refs,
>>>>>> +        struct btrfs_qgroup_extent_record *record)
>>>>>
>>>>> Why do you rename the function preceding with an underscore? just leave
>>>>> it as it is.
>>>>
>>>> Because these two functions have different usage.
>>>
>>> Where is the "other" function used? AFAICS, you are replacing the
>>> function name.
>>
>> See 2nd and 3rd patch.
>> Or be more specific, dirty hack fixes, which is the case where we need
>> to do manual tracking for qgroup.
>
> I still don't see it. Can you be more specific with function names to
> help me understand?

_btrfs_qgroup_insert_dirty_extent() is the real function, which inserts
btrfs_qgroup_extent_record structure into dirty_extent_root.

btrfs_qgroup_record_dirty_extent() is just the calling above 
_btrfs_qgroup_insert_dirty_extent() with proper lock content.

_btrfs_qgroup_insert_dirty_extent() should only be called in delayed_ref 
codes, more specifically, add_delayed_ref_head() function.

In add_delayed_ref_head(), the function already has already hold the 
need lock, so _btrfs_qgroup_insert_dirty_extent() don't need to acquire 
the lock.
And normally, that's all needed hook for qgroup.

For special dirty hack routine, like merge_reloc_roots() and log replay 
code, which doesn't go through normal delayed_ref to handle reference 
modification, we need btrfs_qgroup_record_dirty_extent() function to 
manually info qgroup to track specific extents.

In that case, we need more encapsulated function, so that's 
btrfs_qgroup_record_dirty_extent().


>
> All you are doing is adding is btrfs_qgroup_record_dirty_extent() which
> calls _btrfs_qgroup_insert_dirty_extent() with the locks. Is that what
> you are referring to?

Yes.

>
>
>>
>>>
>>>>
>>>> the underscore one are used when caller has already acquired needed
>>>> lock, which is used by delayed-refs code.
>>>>
>>>> And the one without underscore is for other usage.
>>>
>>> What/Where is the "other usage"? Please don't say "previous".
>>>
>>
>> See above.
>>
>>>>
>>>> Despite the lock, these two functions are all the same.
>>>>
>>>> So I prefer to use underscore.
>>>
>>> BTW, you missed include/trace/events/btrfs.h, the reference to
>>> btrfs_qgroup_insert_dirty_extent in DEFINE_EVENT.
>>>
>>
>> Since btrfs_qgroup_insert_dirty_extent() is just calling the underscore
>> one with proper lock content, so the trace is not affected.
>
> There is NO btrfs_qgroup_insert_dirty_extent(). Your patches REMOVED it
> and replaced it with _btrfs_qgroup_insert_dirty_extent(). Only
> btrfs_qgroup_record_dirty_extent() which is not in trace.

My fault, I should rename the new function to 
btrfs_qgroup_insert_dirty_extent(), or rename the old function to 
_btrfs_qgroup_record_dirty_extent().

I think that would reduce the confusion.

Or, do you prefer adding a new 'nolock' parameter to the old 
btrfs_qgroup_insert_dirty_extent()?

Thanks,
Qu

>
>
> <snipped>
>

Re: [PATCH v2 1/3] btrfs: qgroup: Refactor btrfs_qgroup_insert_dirty_extent()

[Goldwyn Rodrigues]

On 08/08/2016 08:12 PM, Qu Wenruo wrote:
> 
> 
> At 08/09/2016 09:01 AM, Goldwyn Rodrigues wrote:
>>
>>
>> On 08/08/2016 07:26 PM, Qu Wenruo wrote:
>>>
>>>
>>> At 08/08/2016 10:54 AM, Goldwyn Rodrigues wrote:
>>>>
>>>>
>>>> On 08/07/2016 07:39 PM, Qu Wenruo wrote:
>>>>>
>>>>>
>>>>> At 08/07/2016 01:19 AM, Goldwyn Rodrigues wrote:
>>>>>> Thanks Qu,
>>>>>>
>>>>>> This patch set fixes all the reported problems. However, I have some
>>>>>> minor issues with coding style.
>>>>>>
>>>>>
>>>>> Thanks for the test.
>>>>>
>>>>>>
>>>>>> On 08/04/2016 09:29 PM, Qu Wenruo wrote:
>>>>>>> Refactor btrfs_qgroup_insert_dirty_extent() function, to two
>>>>>>> functions:
>>>>>>> 1. _btrfs_qgroup_insert_dirty_extent()
>>>>>>>    Almost the same with original code.
>>>>>>>    For delayed_ref usage, which has delayed refs locked.
>>>>>>>
>>>>>>>    Change the return value type to int, since caller never needs the
>>>>>>>    pointer, but only needs to know if they need to free the
>>>>>>> allocated
>>>>>>>    memory.
>>>>>>>
>>>>>>> 2. btrfs_qgroup_record_dirty_extent()
>>>>>>>    The more encapsulated version.
>>>>>>>
>>>>>>>    Will do the delayed_refs lock, memory allocation, quota enabled
>>>>>>> check
>>>>>>>    and other misc things.
>>>>>>>
>>>>>>> The original design is to keep exported functions to minimal, but
>>>>>>> since
>>>>>>> more btrfs hacks exposed, like replacing path in balance, needs
>>>>>>> us to
>>>>>>> record dirty extents manually, so we have to add such functions.
>>>>>>>
>>>>>>> Also, add comment for both functions, to info developers how to keep
>>>>>>> qgroup correct when doing hacks.
>>>>>>>
>>>>>>> Cc: Mark Fasheh <mfasheh@suse.de>
>>>>>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>>>>>> ---
>>>>>>>  fs/btrfs/delayed-ref.c |  5 +----
>>>>>>>  fs/btrfs/extent-tree.c | 36 +++++-------------------------------
>>>>>>>  fs/btrfs/qgroup.c      | 39 ++++++++++++++++++++++++++++++++++-----
>>>>>>>  fs/btrfs/qgroup.h      | 44
>>>>>>> +++++++++++++++++++++++++++++++++++++++++---
>>>>>>>  4 files changed, 81 insertions(+), 43 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>>>>>>> index 430b368..5eed597 100644
>>>>>>> --- a/fs/btrfs/delayed-ref.c
>>>>>>> +++ b/fs/btrfs/delayed-ref.c
>>>>>>> @@ -541,7 +541,6 @@ add_delayed_ref_head(struct btrfs_fs_info
>>>>>>> *fs_info,
>>>>>>>      struct btrfs_delayed_ref_head *existing;
>>>>>>>      struct btrfs_delayed_ref_head *head_ref = NULL;
>>>>>>>      struct btrfs_delayed_ref_root *delayed_refs;
>>>>>>> -    struct btrfs_qgroup_extent_record *qexisting;
>>>>>>>      int count_mod = 1;
>>>>>>>      int must_insert_reserved = 0;
>>>>>>>
>>>>>>> @@ -606,9 +605,7 @@ add_delayed_ref_head(struct btrfs_fs_info
>>>>>>> *fs_info,
>>>>>>>          qrecord->num_bytes = num_bytes;
>>>>>>>          qrecord->old_roots = NULL;
>>>>>>>
>>>>>>> -        qexisting = btrfs_qgroup_insert_dirty_extent(delayed_refs,
>>>>>>> -                                 qrecord);
>>>>>>> -        if (qexisting)
>>>>>>> +        if(_btrfs_qgroup_insert_dirty_extent(delayed_refs,
>>>>>>> qrecord))
>>>>>>>              kfree(qrecord);
>>>>>>>      }
>>>>>>>
>>>>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>>>>> index 9fcb8c9..47c85ff 100644
>>>>>>> --- a/fs/btrfs/extent-tree.c
>>>>>>> +++ b/fs/btrfs/extent-tree.c
>>>>>>> @@ -8519,34 +8519,6 @@ reada:
>>>>>>>      wc->reada_slot = slot;
>>>>>>>  }
>>>>>>>
>>>>>>> -/*
>>>>>>> - * These may not be seen by the usual inc/dec ref code so we
>>>>>>> have to
>>>>>>> - * add them here.
>>>>>>> - */
>>>>>>> -static int record_one_subtree_extent(struct btrfs_trans_handle
>>>>>>> *trans,
>>>>>>> -                     struct btrfs_root *root, u64 bytenr,
>>>>>>> -                     u64 num_bytes)
>>>>>>> -{
>>>>>>> -    struct btrfs_qgroup_extent_record *qrecord;
>>>>>>> -    struct btrfs_delayed_ref_root *delayed_refs;
>>>>>>> -
>>>>>>> -    qrecord = kmalloc(sizeof(*qrecord), GFP_NOFS);
>>>>>>> -    if (!qrecord)
>>>>>>> -        return -ENOMEM;
>>>>>>> -
>>>>>>> -    qrecord->bytenr = bytenr;
>>>>>>> -    qrecord->num_bytes = num_bytes;
>>>>>>> -    qrecord->old_roots = NULL;
>>>>>>> -
>>>>>>> -    delayed_refs = &trans->transaction->delayed_refs;
>>>>>>> -    spin_lock(&delayed_refs->lock);
>>>>>>> -    if (btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>>>>>>> -        kfree(qrecord);
>>>>>>> -    spin_unlock(&delayed_refs->lock);
>>>>>>> -
>>>>>>> -    return 0;
>>>>>>> -}
>>>>>>> -
>>>>>>>  static int account_leaf_items(struct btrfs_trans_handle *trans,
>>>>>>>                    struct btrfs_root *root,
>>>>>>>                    struct extent_buffer *eb)
>>>>>>> @@ -8580,7 +8552,8 @@ static int account_leaf_items(struct
>>>>>>> btrfs_trans_handle *trans,
>>>>>>>
>>>>>>>          num_bytes = btrfs_file_extent_disk_num_bytes(eb, fi);
>>>>>>>
>>>>>>> -        ret = record_one_subtree_extent(trans, root, bytenr,
>>>>>>> num_bytes);
>>>>>>> +        ret = btrfs_qgroup_record_dirty_extent(trans,
>>>>>>> root->fs_info,
>>>>>>> +                bytenr, num_bytes, GFP_NOFS);
>>>>>>>          if (ret)
>>>>>>>              return ret;
>>>>>>>      }
>>>>>>> @@ -8729,8 +8702,9 @@ walk_down:
>>>>>>>              btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
>>>>>>>              path->locks[level] = BTRFS_READ_LOCK_BLOCKING;
>>>>>>>
>>>>>>> -            ret = record_one_subtree_extent(trans, root,
>>>>>>> child_bytenr,
>>>>>>> -                            root->nodesize);
>>>>>>> +            ret = btrfs_qgroup_record_dirty_extent(trans,
>>>>>>> +                    root->fs_info, child_bytenr,
>>>>>>> +                    root->nodesize, GFP_NOFS);
>>>>>>>              if (ret)
>>>>>>>                  goto out;
>>>>>>>          }
>>>>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>>>>> index 9d4c05b..76d4f67 100644
>>>>>>> --- a/fs/btrfs/qgroup.c
>>>>>>> +++ b/fs/btrfs/qgroup.c
>>>>>>> @@ -1453,9 +1453,9 @@ int
>>>>>>> btrfs_qgroup_prepare_account_extents(struct
>>>>>>> btrfs_trans_handle *trans,
>>>>>>>      return ret;
>>>>>>>  }
>>>>>>>
>>>>>>> -struct btrfs_qgroup_extent_record
>>>>>>> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root
>>>>>>> *delayed_refs,
>>>>>>> -                  struct btrfs_qgroup_extent_record *record)
>>>>>>> +int _btrfs_qgroup_insert_dirty_extent(
>>>>>>> +        struct btrfs_delayed_ref_root *delayed_refs,
>>>>>>> +        struct btrfs_qgroup_extent_record *record)
>>>>>>
>>>>>> Why do you rename the function preceding with an underscore? just
>>>>>> leave
>>>>>> it as it is.
>>>>>
>>>>> Because these two functions have different usage.
>>>>
>>>> Where is the "other" function used? AFAICS, you are replacing the
>>>> function name.
>>>
>>> See 2nd and 3rd patch.
>>> Or be more specific, dirty hack fixes, which is the case where we need
>>> to do manual tracking for qgroup.
>>
>> I still don't see it. Can you be more specific with function names to
>> help me understand?
> 
> _btrfs_qgroup_insert_dirty_extent() is the real function, which inserts
> btrfs_qgroup_extent_record structure into dirty_extent_root.
> 
> btrfs_qgroup_record_dirty_extent() is just the calling above
> _btrfs_qgroup_insert_dirty_extent() with proper lock content.
> 
> _btrfs_qgroup_insert_dirty_extent() should only be called in delayed_ref
> codes, more specifically, add_delayed_ref_head() function.
> 
> In add_delayed_ref_head(), the function already has already hold the
> need lock, so _btrfs_qgroup_insert_dirty_extent() don't need to acquire
> the lock.
> And normally, that's all needed hook for qgroup.
> 
> For special dirty hack routine, like merge_reloc_roots() and log replay
> code, which doesn't go through normal delayed_ref to handle reference
> modification, we need btrfs_qgroup_record_dirty_extent() function to
> manually info qgroup to track specific extents.
> 
> In that case, we need more encapsulated function, so that's
> btrfs_qgroup_record_dirty_extent().
> 
> 
>>
>> All you are doing is adding is btrfs_qgroup_record_dirty_extent() which
>> calls _btrfs_qgroup_insert_dirty_extent() with the locks. Is that what
>> you are referring to?
> 
> Yes.
> 
>>
>>
>>>
>>>>
>>>>>
>>>>> the underscore one are used when caller has already acquired needed
>>>>> lock, which is used by delayed-refs code.
>>>>>
>>>>> And the one without underscore is for other usage.
>>>>
>>>> What/Where is the "other usage"? Please don't say "previous".
>>>>
>>>
>>> See above.
>>>
>>>>>
>>>>> Despite the lock, these two functions are all the same.
>>>>>
>>>>> So I prefer to use underscore.
>>>>
>>>> BTW, you missed include/trace/events/btrfs.h, the reference to
>>>> btrfs_qgroup_insert_dirty_extent in DEFINE_EVENT.
>>>>
>>>
>>> Since btrfs_qgroup_insert_dirty_extent() is just calling the underscore
>>> one with proper lock content, so the trace is not affected.
>>
>> There is NO btrfs_qgroup_insert_dirty_extent(). Your patches REMOVED it
>> and replaced it with _btrfs_qgroup_insert_dirty_extent(). Only
>> btrfs_qgroup_record_dirty_extent() which is not in trace.
> 
> My fault, I should rename the new function to
> btrfs_qgroup_insert_dirty_extent(), or rename the old function to
> _btrfs_qgroup_record_dirty_extent().
> 
> I think that would reduce the confusion.
> 
> Or, do you prefer adding a new 'nolock' parameter to the old
> btrfs_qgroup_insert_dirty_extent()?
> 

Honestly, I don't like the idea of a function name which looks like a
"wrappee" function (as opposed to a wrapper function.. IOW a function
preceded with underscores) to be in the header files.

So, if I would to write it, the functions would be called as
btrfs_qgroup_insert_extent() and btrfs_qgroup_insert_extent_nolock(),
with former calling the latter. How does that sound?


-- 
Goldwyn

Re: [PATCH v2 1/3] btrfs: qgroup: Refactor btrfs_qgroup_insert_dirty_extent()

[Qu Wenruo]

At 08/09/2016 10:24 AM, Goldwyn Rodrigues wrote:
>
>
> On 08/08/2016 08:12 PM, Qu Wenruo wrote:
>>
>>
>> At 08/09/2016 09:01 AM, Goldwyn Rodrigues wrote:
>>>
>>>
>>> On 08/08/2016 07:26 PM, Qu Wenruo wrote:
>>>>
>>>>
>>>> At 08/08/2016 10:54 AM, Goldwyn Rodrigues wrote:
>>>>>
>>>>>
>>>>> On 08/07/2016 07:39 PM, Qu Wenruo wrote:
>>>>>>
>>>>>>
>>>>>> At 08/07/2016 01:19 AM, Goldwyn Rodrigues wrote:
>>>>>>> Thanks Qu,
>>>>>>>
>>>>>>> This patch set fixes all the reported problems. However, I have some
>>>>>>> minor issues with coding style.
>>>>>>>
>>>>>>
>>>>>> Thanks for the test.
>>>>>>
>>>>>>>
>>>>>>> On 08/04/2016 09:29 PM, Qu Wenruo wrote:
>>>>>>>> Refactor btrfs_qgroup_insert_dirty_extent() function, to two
>>>>>>>> functions:
>>>>>>>> 1. _btrfs_qgroup_insert_dirty_extent()
>>>>>>>>    Almost the same with original code.
>>>>>>>>    For delayed_ref usage, which has delayed refs locked.
>>>>>>>>
>>>>>>>>    Change the return value type to int, since caller never needs the
>>>>>>>>    pointer, but only needs to know if they need to free the
>>>>>>>> allocated
>>>>>>>>    memory.
>>>>>>>>
>>>>>>>> 2. btrfs_qgroup_record_dirty_extent()
>>>>>>>>    The more encapsulated version.
>>>>>>>>
>>>>>>>>    Will do the delayed_refs lock, memory allocation, quota enabled
>>>>>>>> check
>>>>>>>>    and other misc things.
>>>>>>>>
>>>>>>>> The original design is to keep exported functions to minimal, but
>>>>>>>> since
>>>>>>>> more btrfs hacks exposed, like replacing path in balance, needs
>>>>>>>> us to
>>>>>>>> record dirty extents manually, so we have to add such functions.
>>>>>>>>
>>>>>>>> Also, add comment for both functions, to info developers how to keep
>>>>>>>> qgroup correct when doing hacks.
>>>>>>>>
>>>>>>>> Cc: Mark Fasheh <mfasheh@suse.de>
>>>>>>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>>>>>>> ---
>>>>>>>>  fs/btrfs/delayed-ref.c |  5 +----
>>>>>>>>  fs/btrfs/extent-tree.c | 36 +++++-------------------------------
>>>>>>>>  fs/btrfs/qgroup.c      | 39 ++++++++++++++++++++++++++++++++++-----
>>>>>>>>  fs/btrfs/qgroup.h      | 44
>>>>>>>> +++++++++++++++++++++++++++++++++++++++++---
>>>>>>>>  4 files changed, 81 insertions(+), 43 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>>>>>>>> index 430b368..5eed597 100644
>>>>>>>> --- a/fs/btrfs/delayed-ref.c
>>>>>>>> +++ b/fs/btrfs/delayed-ref.c
>>>>>>>> @@ -541,7 +541,6 @@ add_delayed_ref_head(struct btrfs_fs_info
>>>>>>>> *fs_info,
>>>>>>>>      struct btrfs_delayed_ref_head *existing;
>>>>>>>>      struct btrfs_delayed_ref_head *head_ref = NULL;
>>>>>>>>      struct btrfs_delayed_ref_root *delayed_refs;
>>>>>>>> -    struct btrfs_qgroup_extent_record *qexisting;
>>>>>>>>      int count_mod = 1;
>>>>>>>>      int must_insert_reserved = 0;
>>>>>>>>
>>>>>>>> @@ -606,9 +605,7 @@ add_delayed_ref_head(struct btrfs_fs_info
>>>>>>>> *fs_info,
>>>>>>>>          qrecord->num_bytes = num_bytes;
>>>>>>>>          qrecord->old_roots = NULL;
>>>>>>>>
>>>>>>>> -        qexisting = btrfs_qgroup_insert_dirty_extent(delayed_refs,
>>>>>>>> -                                 qrecord);
>>>>>>>> -        if (qexisting)
>>>>>>>> +        if(_btrfs_qgroup_insert_dirty_extent(delayed_refs,
>>>>>>>> qrecord))
>>>>>>>>              kfree(qrecord);
>>>>>>>>      }
>>>>>>>>
>>>>>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>>>>>> index 9fcb8c9..47c85ff 100644
>>>>>>>> --- a/fs/btrfs/extent-tree.c
>>>>>>>> +++ b/fs/btrfs/extent-tree.c
>>>>>>>> @@ -8519,34 +8519,6 @@ reada:
>>>>>>>>      wc->reada_slot = slot;
>>>>>>>>  }
>>>>>>>>
>>>>>>>> -/*
>>>>>>>> - * These may not be seen by the usual inc/dec ref code so we
>>>>>>>> have to
>>>>>>>> - * add them here.
>>>>>>>> - */
>>>>>>>> -static int record_one_subtree_extent(struct btrfs_trans_handle
>>>>>>>> *trans,
>>>>>>>> -                     struct btrfs_root *root, u64 bytenr,
>>>>>>>> -                     u64 num_bytes)
>>>>>>>> -{
>>>>>>>> -    struct btrfs_qgroup_extent_record *qrecord;
>>>>>>>> -    struct btrfs_delayed_ref_root *delayed_refs;
>>>>>>>> -
>>>>>>>> -    qrecord = kmalloc(sizeof(*qrecord), GFP_NOFS);
>>>>>>>> -    if (!qrecord)
>>>>>>>> -        return -ENOMEM;
>>>>>>>> -
>>>>>>>> -    qrecord->bytenr = bytenr;
>>>>>>>> -    qrecord->num_bytes = num_bytes;
>>>>>>>> -    qrecord->old_roots = NULL;
>>>>>>>> -
>>>>>>>> -    delayed_refs = &trans->transaction->delayed_refs;
>>>>>>>> -    spin_lock(&delayed_refs->lock);
>>>>>>>> -    if (btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>>>>>>>> -        kfree(qrecord);
>>>>>>>> -    spin_unlock(&delayed_refs->lock);
>>>>>>>> -
>>>>>>>> -    return 0;
>>>>>>>> -}
>>>>>>>> -
>>>>>>>>  static int account_leaf_items(struct btrfs_trans_handle *trans,
>>>>>>>>                    struct btrfs_root *root,
>>>>>>>>                    struct extent_buffer *eb)
>>>>>>>> @@ -8580,7 +8552,8 @@ static int account_leaf_items(struct
>>>>>>>> btrfs_trans_handle *trans,
>>>>>>>>
>>>>>>>>          num_bytes = btrfs_file_extent_disk_num_bytes(eb, fi);
>>>>>>>>
>>>>>>>> -        ret = record_one_subtree_extent(trans, root, bytenr,
>>>>>>>> num_bytes);
>>>>>>>> +        ret = btrfs_qgroup_record_dirty_extent(trans,
>>>>>>>> root->fs_info,
>>>>>>>> +                bytenr, num_bytes, GFP_NOFS);
>>>>>>>>          if (ret)
>>>>>>>>              return ret;
>>>>>>>>      }
>>>>>>>> @@ -8729,8 +8702,9 @@ walk_down:
>>>>>>>>              btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
>>>>>>>>              path->locks[level] = BTRFS_READ_LOCK_BLOCKING;
>>>>>>>>
>>>>>>>> -            ret = record_one_subtree_extent(trans, root,
>>>>>>>> child_bytenr,
>>>>>>>> -                            root->nodesize);
>>>>>>>> +            ret = btrfs_qgroup_record_dirty_extent(trans,
>>>>>>>> +                    root->fs_info, child_bytenr,
>>>>>>>> +                    root->nodesize, GFP_NOFS);
>>>>>>>>              if (ret)
>>>>>>>>                  goto out;
>>>>>>>>          }
>>>>>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>>>>>> index 9d4c05b..76d4f67 100644
>>>>>>>> --- a/fs/btrfs/qgroup.c
>>>>>>>> +++ b/fs/btrfs/qgroup.c
>>>>>>>> @@ -1453,9 +1453,9 @@ int
>>>>>>>> btrfs_qgroup_prepare_account_extents(struct
>>>>>>>> btrfs_trans_handle *trans,
>>>>>>>>      return ret;
>>>>>>>>  }
>>>>>>>>
>>>>>>>> -struct btrfs_qgroup_extent_record
>>>>>>>> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root
>>>>>>>> *delayed_refs,
>>>>>>>> -                  struct btrfs_qgroup_extent_record *record)
>>>>>>>> +int _btrfs_qgroup_insert_dirty_extent(
>>>>>>>> +        struct btrfs_delayed_ref_root *delayed_refs,
>>>>>>>> +        struct btrfs_qgroup_extent_record *record)
>>>>>>>
>>>>>>> Why do you rename the function preceding with an underscore? just
>>>>>>> leave
>>>>>>> it as it is.
>>>>>>
>>>>>> Because these two functions have different usage.
>>>>>
>>>>> Where is the "other" function used? AFAICS, you are replacing the
>>>>> function name.
>>>>
>>>> See 2nd and 3rd patch.
>>>> Or be more specific, dirty hack fixes, which is the case where we need
>>>> to do manual tracking for qgroup.
>>>
>>> I still don't see it. Can you be more specific with function names to
>>> help me understand?
>>
>> _btrfs_qgroup_insert_dirty_extent() is the real function, which inserts
>> btrfs_qgroup_extent_record structure into dirty_extent_root.
>>
>> btrfs_qgroup_record_dirty_extent() is just the calling above
>> _btrfs_qgroup_insert_dirty_extent() with proper lock content.
>>
>> _btrfs_qgroup_insert_dirty_extent() should only be called in delayed_ref
>> codes, more specifically, add_delayed_ref_head() function.
>>
>> In add_delayed_ref_head(), the function already has already hold the
>> need lock, so _btrfs_qgroup_insert_dirty_extent() don't need to acquire
>> the lock.
>> And normally, that's all needed hook for qgroup.
>>
>> For special dirty hack routine, like merge_reloc_roots() and log replay
>> code, which doesn't go through normal delayed_ref to handle reference
>> modification, we need btrfs_qgroup_record_dirty_extent() function to
>> manually info qgroup to track specific extents.
>>
>> In that case, we need more encapsulated function, so that's
>> btrfs_qgroup_record_dirty_extent().
>>
>>
>>>
>>> All you are doing is adding is btrfs_qgroup_record_dirty_extent() which
>>> calls _btrfs_qgroup_insert_dirty_extent() with the locks. Is that what
>>> you are referring to?
>>
>> Yes.
>>
>>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> the underscore one are used when caller has already acquired needed
>>>>>> lock, which is used by delayed-refs code.
>>>>>>
>>>>>> And the one without underscore is for other usage.
>>>>>
>>>>> What/Where is the "other usage"? Please don't say "previous".
>>>>>
>>>>
>>>> See above.
>>>>
>>>>>>
>>>>>> Despite the lock, these two functions are all the same.
>>>>>>
>>>>>> So I prefer to use underscore.
>>>>>
>>>>> BTW, you missed include/trace/events/btrfs.h, the reference to
>>>>> btrfs_qgroup_insert_dirty_extent in DEFINE_EVENT.
>>>>>
>>>>
>>>> Since btrfs_qgroup_insert_dirty_extent() is just calling the underscore
>>>> one with proper lock content, so the trace is not affected.
>>>
>>> There is NO btrfs_qgroup_insert_dirty_extent(). Your patches REMOVED it
>>> and replaced it with _btrfs_qgroup_insert_dirty_extent(). Only
>>> btrfs_qgroup_record_dirty_extent() which is not in trace.
>>
>> My fault, I should rename the new function to
>> btrfs_qgroup_insert_dirty_extent(), or rename the old function to
>> _btrfs_qgroup_record_dirty_extent().
>>
>> I think that would reduce the confusion.
>>
>> Or, do you prefer adding a new 'nolock' parameter to the old
>> btrfs_qgroup_insert_dirty_extent()?
>>
>
> Honestly, I don't like the idea of a function name which looks like a
> "wrappee" function (as opposed to a wrapper function.. IOW a function
> preceded with underscores) to be in the header files.
>
> So, if I would to write it, the functions would be called as
> btrfs_qgroup_insert_extent() and btrfs_qgroup_insert_extent_nolock(),
> with former calling the latter. How does that sound?
>
>
Sounds good for me.

I'll update them in next version.

Thanks,
Qu