[PATCH 0/3] Qgroup fixes for dirty hack routines

[PATCH 0/3] Qgroup fixes 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.

The 2nd patch is sent to mail list weeks ago, only commit message and
comment is 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  | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/tree-log.c    | 16 ++++++++++
 6 files changed, 178 insertions(+), 43 deletions(-)

-- 
2.9.0

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

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 82b912a..cf63150 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -8307,34 +8307,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)
@@ -8368,7 +8340,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;
 	}
@@ -8517,8 +8490,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.0

[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
balanced data extents.

The root cause is that balancing is doing non-standard and almost insane
tree block swap hack.

The problem happens in the following steps:
(Use 4M as original data extent size, and 257 as src root objectid)

1) Balance create new data extents and increase its refs

Balance will alloc new data extent and create new EXTENT_DATA in data
reloc tree, while its refernece is increased with 2 different
referencer: 257 and data reloc tree.

While at that time, file tree is still referring to old extents.

So, the backref walker will only find data reloc tree as referencer for
the new data extent, which doesn't count as file tree.

Extent bytenr   |  Real referencer  |     backrefs     | nr_old_roots |
-----------------------------------------------------------------------
New             | Data reloc        | Data reloc + 257 | 0            |
Old             | 257               | 257              | 1            |

Qgroup number: 4M + metadata

2) Commit trans before merging reloc tree
Then we goes to prepare_to_merge(), which will commit transacation.

In the qgroup update codes inside commit_transaction, although backref
walk codes find the new data extents has 2 backref, but file tree
backref can't find referencer(file tree is still referring to old
extents), and data reloc doesn't count as file tree.

So for nr_old_roots and nr_new_roots of that 2 extents:
Extent bytenr   |   nr_old_roots   | nr_new_roots       | qgroup change |
------------------------------------------------------------------------|
New             | 0(doesn't exist) | 0(data reloc tree) | 0             |
Old             | 1(root 257)      | 1(257)             | 0             |

Qgroup number: 4M + metadata +-0 = 4M + metadata

3) Swap tree blocks and free old tree blocks
Then we goes to merge_reloc_roots(), which swaps the tree blocks
directly, and free the old tree blocks.
Freeing tree blocks will also free its data extents, this goes through
normal routine, and qgroup handles it well, decreasing the numbers.

And since new data extent is not updated here (updated in step 1), so
qgroup won't scan new data extent.

Extent bytenr   | nr_old_roots |   nr_new_roots   | qgroup change |
------------------------------------------------------------------|
New             |---No modification, doesn't go through qgroup----|Error
Old             | 1(root 257)  | 0(doesn't exist) | -4M           |

In fact, New extent's owner changed from data reloc to root 257, which
must go through qgroup accounting.
But the hideous hack of replacing path doesn't do it.

Qgroup number: 4M + metadata -4M = metadata

This patch will fix it by re-dirtying new extent at step 3), so backref
walk and qgroup can get correct result.

After this patch, step 3) will be:

Extent bytenr | nr_old_roots       |   nr_new_roots   | qgroup change |
----------------------------------------------------------------------|
New           | 0(data reloc tree) | 1(root 257)      | +4M           |
Old           | 1(root 257)        | 0(doesn't exist) | -4M           |

And Qgroup number will remain correct:
4M + metadata +4M -4M = 4M + metadata

So we only need to ensure we don't miss some extents.

Reported-by: Mark Fasheh <mfasheh@suse.de>
Reported-by: Filipe Manana <fdmanana@gmail.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
v2:
  Use refactored interface.
  Update comment and commit message
---
 fs/btrfs/relocation.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 0477dca..0a7ddf1 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
@@ -1750,6 +1751,68 @@ int memcmp_node_keys(struct extent_buffer *eb, int slot,
 }
 
 /*
+ * Helper function to fixup screwed qgroups caused by increased extent ref,
+ * which doesn't follow normal extent ref update behavior.
+ * (Correct behavior is, increase extent ref and modify source root in
+ *  one trans)
+ * No better solution as long as we're doing swapping trick to do balance.
+ */
+static int qgroup_redirty_data_extents(struct btrfs_trans_handle *trans,
+				       struct btrfs_root *root, u64 bytenr,
+				       u64 gen)
+{
+	struct btrfs_fs_info *fs_info = root->fs_info;
+	struct extent_buffer *leaf;
+	struct btrfs_delayed_ref_root *delayed_refs;
+	int slot;
+	int ret = 0;
+
+	if (!fs_info->quota_enabled || !is_fstree(root->objectid))
+		return 0;
+	if (WARN_ON(!trans))
+		return -EINVAL;
+
+	delayed_refs = &trans->transaction->delayed_refs;
+
+	leaf = read_tree_block(root, bytenr, gen);
+	if (IS_ERR(leaf)) {
+		return PTR_ERR(leaf);
+	} else if (!extent_buffer_uptodate(leaf)) {
+		ret = -EIO;
+		goto out;
+	}
+
+	/* We only care leaf, which may contains EXTENT_DATA */
+	if (btrfs_header_level(leaf) != 0)
+		goto out;
+
+	for (slot = 0; slot < btrfs_header_nritems(leaf); slot++) {
+		struct btrfs_key key;
+		struct btrfs_file_extent_item *fi;
+
+		btrfs_item_key_to_cpu(leaf, &key, slot);
+		if (key.type != BTRFS_EXTENT_DATA_KEY)
+			continue;
+		fi = btrfs_item_ptr(leaf, slot, struct btrfs_file_extent_item);
+		if (btrfs_file_extent_type(leaf, fi) ==
+		    BTRFS_FILE_EXTENT_INLINE ||
+		    btrfs_file_extent_disk_bytenr(leaf, fi) == 0)
+			continue;
+
+		ret = btrfs_qgroup_record_dirty_extent(trans, fs_info,
+				btrfs_file_extent_disk_bytenr(leaf, fi),
+				btrfs_file_extent_disk_num_bytes(leaf, fi),
+				GFP_NOFS);
+		if (ret < 0)
+			break;
+	}
+out:
+	free_extent_buffer(leaf);
+	return ret;
+
+}
+
+/*
  * try to replace tree blocks in fs tree with the new blocks
  * in reloc tree. tree blocks haven't been modified since the
  * reloc tree was create can be replaced.
@@ -1919,7 +1982,25 @@ again:
 					0);
 		BUG_ON(ret);
 
+		/*
+		 * The relocated EXTENT_DATA inside new tree block is allocated
+		 * from data reloc tree, which doesn't affect qgroup.
+		 *
+		 * While after path replacement, EXTENT_DATA in the tree block
+		 * is accounted as fs/file tree, which does affect qgroup.
+		 *
+		 * So here we need to info qgroup to re-account all these
+		 * possible owner-changed EXTENT_DATA, to make qgroup
+		 * correct again.
+		 *
+		 * XXX: Why balance is always using such hacks? Isn't there any
+		 * more sane method?
+		 */
+		ret = qgroup_redirty_data_extents(trans, dest, new_bytenr,
+						  new_ptr_gen);
 		btrfs_unlock_up_safe(path, 0);
+		if (ret < 0)
+			break;
 
 		ret = level;
 		break;
-- 
2.9.0

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

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 8ab1dc6..a322aa5 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.0

Re: [PATCH 0/3] Qgroup fixes for dirty hack routines

[Qu Wenruo]

Hi David,

For your for-next-20160726 branch, I find that the old version of the 
2nd patch is merged.
Not the latest one in the patchset.

Just in case, if you would like to merge the second patch(fix leaking 
qgroups numbers on data extents), please use the the first 2 patches of 
the patchset.
As the code style and commit message is much better than old version.

Thanks,
Qu

At 07/05/2016 05:32 PM, Qu Wenruo wrote:
> 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.
>
> The 2nd patch is sent to mail list weeks ago, only commit message and
> comment is 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  | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/tree-log.c    | 16 ++++++++++
>  6 files changed, 178 insertions(+), 43 deletions(-)
>