[PATCH v2 0/2] btrfs: qgroup rescan fixes part 1

[PATCH v2 0/2] btrfs: qgroup rescan fixes part 1

[Qu Wenruo]

This patchset is mainly focused on fixing qgroup rescan corruption.

Since the whole btrfs qgroup is based on the modification between 2
transactions, it only has correct qgroup delta.
While if the rescan can't provide a correct result from the very
beginning, qgroup numbers are corrupted.

The patchset is fixing two types of qgroup corruption which could happen
by looping btrfs/017 with some possibility.

1) Not accounting tree blocks
   Caused by the fact that qgroup rescan only searches commit root for
   backref, while we're passing current extent root to search.
   Fix it by also passing commit extent root.

2) Double accounting tree blocks
   Caused by wrong rescan exit condition.
   Currently qgroup only exit when it can't find any leaves beyond
   rescan progress.
   However it could cause problem when new transaction happens after
   last rescan, and old leaves CoWed to new location, and double
   accounting could happen.
   Fix it by checking and leave qgroup rescan if we have hit last leaf,
   instead of leaving it to next leaf rescan.


Changelog:
v2:
  Remove unused tree_mod_seq_elem for the 1st patch.
  Fix double unlock in 2nd patch.
  Thanks Jeff for the update.

Qu Wenruo (2):
  btrfs: qgroup: Search commit root for rescan to avoid missing extent
  btrfs: qgroup: Finish rescan when hit the last leaf of extent tree

 fs/btrfs/qgroup.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

-- 
2.17.0

[PATCH v2 1/2] btrfs: qgroup: Search commit root for rescan to avoid missing extent

[Qu Wenruo]

When doing qgroup rescan using the following script (modified from
btrfs/017 test case), we can sometimes hit qgroup corruption.

------
umount $dev &> /dev/null
umount $mnt &> /dev/null

mkfs.btrfs -f -n 64k $dev
mount $dev $mnt

extent_size=8192

xfs_io -f -d -c "pwrite 0 $extent_size" $mnt/foo > /dev/null
btrfs subvolume snapshot $mnt $mnt/snap

xfs_io -f -c "reflink $mnt/foo" $mnt/foo-reflink > /dev/null
xfs_io -f -c "reflink $mnt/foo" $mnt/snap/foo-reflink > /dev/null
xfs_io -f -c "reflink $mnt/foo" $mnt/snap/foo-reflink2 > /dev/unll
btrfs quota enable $mnt

 # -W is the new option to only wait rescan while not starting new one
btrfs quota rescan -W $mnt
btrfs qgroup show -prce $mnt
umount $mnt

 # Need to patch btrfs-progs to report qgroup mismatch as error
btrfs check $dev || _fail
------

For fast machine, we can hit some corruption which missed accounting
tree blocks:
------
qgroupid         rfer         excl     max_rfer     max_excl parent  child
--------         ----         ----     --------     -------- ------  -----
0/5           8.00KiB        0.00B         none         none ---     ---
0/257         8.00KiB        0.00B         none         none ---     ---
------

This is due to the fact that we're always searching commit root for
btrfs_find_all_roots() at qgroup_rescan_leaf(), but the leaf we get is
from current transaction, not commit root.

And if our tree blocks get modified in current transaction, we won't
find any owner in commit root, thus causing the corruption.

Fix it by searching commit root for extent tree for
qgroup_rescan_leaf().

Reported-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 9fb758d5077a..197f47a764f1 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2590,7 +2590,6 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 	struct btrfs_key found;
 	struct extent_buffer *scratch_leaf = NULL;
 	struct ulist *roots = NULL;
-	struct seq_list tree_mod_seq_elem = SEQ_LIST_INIT(tree_mod_seq_elem);
 	u64 num_bytes;
 	int slot;
 	int ret;
@@ -2625,7 +2624,6 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 			      btrfs_header_nritems(path->nodes[0]) - 1);
 	fs_info->qgroup_rescan_progress.objectid = found.objectid + 1;
 
-	btrfs_get_tree_mod_seq(fs_info, &tree_mod_seq_elem);
 	scratch_leaf = btrfs_clone_extent_buffer(path->nodes[0]);
 	if (!scratch_leaf) {
 		ret = -ENOMEM;
@@ -2664,7 +2662,6 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 		btrfs_tree_read_unlock_blocking(scratch_leaf);
 		free_extent_buffer(scratch_leaf);
 	}
-	btrfs_put_tree_mod_seq(fs_info, &tree_mod_seq_elem);
 
 	return ret;
 }
@@ -2681,6 +2678,11 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 	path = btrfs_alloc_path();
 	if (!path)
 		goto out;
+	/*
+	 * Rescan should only search for commit root, and any later difference
+	 * should be recorded by qgroup
+	 */
+	path->search_commit_root = 1;
 
 	err = 0;
 	while (!err && !btrfs_fs_closing(fs_info)) {
-- 
2.17.0

[PATCH v2 2/2] btrfs: qgroup: Finish rescan when hit the last leaf of extent tree

[Qu Wenruo]

Under the following case, qgroup rescan can double account cowed tree
blocks:

In this case, extent tree only has one tree block.

-
| transid=5 last committed=4
| btrfs_qgroup_rescan_worker()
| |- btrfs_start_transaction()
| |  transid = 5
| |- qgroup_rescan_leaf()
|    |- btrfs_search_slot_for_read() on extent tree
|       Get the only extent tree block from commit root (transid = 4).
|       Scan it, set qgroup_rescan_progress to the last
|       EXTENT/META_ITEM + 1
|       now qgroup_rescan_progress = A + 1.
|
| fs tree get CoWed, new tree block is at A + 16K
| transid 5 get committed
-
| transid=6 last committed=5
| btrfs_qgroup_rescan_worker()
| btrfs_qgroup_rescan_worker()
| |- btrfs_start_transaction()
| |  transid = 5
| |- qgroup_rescan_leaf()
|    |- btrfs_search_slot_for_read() on extent tree
|       Get the only extent tree block from commit root (transid = 5).
|       scan it using qgroup_rescan_progress (A + 1).
|       found new tree block beyong A, and it's fs tree block,
|       account it to increase qgroup numbers.
-

In above case, tree block A, and tree block A + 16K get accounted twice,
while qgroup rescan should stop when it already reach the last leaf,
other than continue using its qgroup_rescan_progress.

Such case could happen by just looping btrfs/017 and with some
possibility it can hit such double qgroup accounting problem.

Fix it by checking the path to determine if we should finish qgroup
rescan, other than relying on next loop to exit.

Reported-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 197f47a764f1..500e26d5c2c0 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2579,6 +2579,21 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
 	spin_unlock(&fs_info->qgroup_lock);
 }
 
+/*
+ * Check if the leaf is the last leaf. Which means all node pointers
+ * are at their last position.
+ */
+static bool is_last_leaf(struct btrfs_path *path)
+{
+	int i;
+
+	for (i = 1; i < BTRFS_MAX_LEVEL && path->nodes[i]; i++) {
+		if (path->slots[i] != btrfs_header_nritems(path->nodes[i]) - 1)
+			return false;
+	}
+	return true;
+}
+
 /*
  * returns < 0 on error, 0 when more leafs are to be scanned.
  * returns 1 when done.
@@ -2591,6 +2606,7 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 	struct extent_buffer *scratch_leaf = NULL;
 	struct ulist *roots = NULL;
 	u64 num_bytes;
+	bool done;
 	int slot;
 	int ret;
 
@@ -2619,6 +2635,7 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 		mutex_unlock(&fs_info->qgroup_rescan_lock);
 		return ret;
 	}
+	done = is_last_leaf(path);
 
 	btrfs_item_key_to_cpu(path->nodes[0], &found,
 			      btrfs_header_nritems(path->nodes[0]) - 1);
@@ -2663,6 +2680,8 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 		free_extent_buffer(scratch_leaf);
 	}
 
+	if (done && !ret)
+		ret = 1;
 	return ret;
 }
 
-- 
2.17.0

Re: [PATCH v2 0/2] btrfs: qgroup rescan fixes part 1

[David Sterba]

On Mon, May 14, 2018 at 09:38:11AM +0800, Qu Wenruo wrote:
> This patchset is mainly focused on fixing qgroup rescan corruption.
> 
> Since the whole btrfs qgroup is based on the modification between 2
> transactions, it only has correct qgroup delta.
> While if the rescan can't provide a correct result from the very
> beginning, qgroup numbers are corrupted.
> 
> The patchset is fixing two types of qgroup corruption which could happen
> by looping btrfs/017 with some possibility.
> 
> 1) Not accounting tree blocks
>    Caused by the fact that qgroup rescan only searches commit root for
>    backref, while we're passing current extent root to search.
>    Fix it by also passing commit extent root.
> 
> 2) Double accounting tree blocks
>    Caused by wrong rescan exit condition.
>    Currently qgroup only exit when it can't find any leaves beyond
>    rescan progress.
>    However it could cause problem when new transaction happens after
>    last rescan, and old leaves CoWed to new location, and double
>    accounting could happen.
>    Fix it by checking and leave qgroup rescan if we have hit last leaf,
>    instead of leaving it to next leaf rescan.
> 
> 
> Changelog:
> v2:
>   Remove unused tree_mod_seq_elem for the 1st patch.
>   Fix double unlock in 2nd patch.
>   Thanks Jeff for the update.

V1 patches replaced, thanks.

[PATCH v3 1/2] btrfs: qgroup: Search commit root for rescan to avoid missing extent

[Qu Wenruo]

When doing qgroup rescan using the following script (modified from
btrfs/017 test case), we can sometimes hit qgroup corruption.

------
umount $dev &> /dev/null
umount $mnt &> /dev/null

mkfs.btrfs -f -n 64k $dev
mount $dev $mnt

extent_size=8192

xfs_io -f -d -c "pwrite 0 $extent_size" $mnt/foo > /dev/null
btrfs subvolume snapshot $mnt $mnt/snap

xfs_io -f -c "reflink $mnt/foo" $mnt/foo-reflink > /dev/null
xfs_io -f -c "reflink $mnt/foo" $mnt/snap/foo-reflink > /dev/null
xfs_io -f -c "reflink $mnt/foo" $mnt/snap/foo-reflink2 > /dev/unll
btrfs quota enable $mnt

 # -W is the new option to only wait rescan while not starting new one
btrfs quota rescan -W $mnt
btrfs qgroup show -prce $mnt
umount $mnt

 # Need to patch btrfs-progs to report qgroup mismatch as error
btrfs check $dev || _fail
------

For fast machine, we can hit some corruption which missed accounting
tree blocks:
------
qgroupid         rfer         excl     max_rfer     max_excl parent  child
--------         ----         ----     --------     -------- ------  -----
0/5           8.00KiB        0.00B         none         none ---     ---
0/257         8.00KiB        0.00B         none         none ---     ---
------

This is due to the fact that we're always searching commit root for
btrfs_find_all_roots() at qgroup_rescan_leaf(), but the leaf we get is
from current transaction, not commit root.

And if our tree blocks get modified in current transaction, we won't
find any owner in commit root, thus causing the corruption.

Fix it by searching commit root for extent tree for
qgroup_rescan_leaf().

Reported-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
changelog:
v2:
  Remove unused tree_mod_seq_elem for the 1st patch.
v3:
  Also skip locking to avoid lock assert() with Liu Bo's
  btrfs_search_slot() patchset.
---
 fs/btrfs/qgroup.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 9fb758d5077a..a88330088d03 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2590,7 +2590,6 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 	struct btrfs_key found;
 	struct extent_buffer *scratch_leaf = NULL;
 	struct ulist *roots = NULL;
-	struct seq_list tree_mod_seq_elem = SEQ_LIST_INIT(tree_mod_seq_elem);
 	u64 num_bytes;
 	int slot;
 	int ret;
@@ -2625,7 +2624,6 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 			      btrfs_header_nritems(path->nodes[0]) - 1);
 	fs_info->qgroup_rescan_progress.objectid = found.objectid + 1;
 
-	btrfs_get_tree_mod_seq(fs_info, &tree_mod_seq_elem);
 	scratch_leaf = btrfs_clone_extent_buffer(path->nodes[0]);
 	if (!scratch_leaf) {
 		ret = -ENOMEM;
@@ -2664,7 +2662,6 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 		btrfs_tree_read_unlock_blocking(scratch_leaf);
 		free_extent_buffer(scratch_leaf);
 	}
-	btrfs_put_tree_mod_seq(fs_info, &tree_mod_seq_elem);
 
 	return ret;
 }
@@ -2681,6 +2678,12 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
 	path = btrfs_alloc_path();
 	if (!path)
 		goto out;
+	/*
+	 * Rescan should only search for commit root, and any later difference
+	 * should be recorded by qgroup
+	 */
+	path->search_commit_root = 1;
+	path->skip_locking = 1;
 
 	err = 0;
 	while (!err && !btrfs_fs_closing(fs_info)) {
-- 
2.17.0

Re: [PATCH v3 1/2] btrfs: qgroup: Search commit root for rescan to avoid missing extent

[Nikolay Borisov]

On 23.05.2018 10:29, Qu Wenruo wrote:
> When doing qgroup rescan using the following script (modified from
> btrfs/017 test case), we can sometimes hit qgroup corruption.
> 
> ------
> umount $dev &> /dev/null
> umount $mnt &> /dev/null
> 
> mkfs.btrfs -f -n 64k $dev
> mount $dev $mnt
> 
> extent_size=8192
> 
> xfs_io -f -d -c "pwrite 0 $extent_size" $mnt/foo > /dev/null
> btrfs subvolume snapshot $mnt $mnt/snap
> 
> xfs_io -f -c "reflink $mnt/foo" $mnt/foo-reflink > /dev/null
> xfs_io -f -c "reflink $mnt/foo" $mnt/snap/foo-reflink > /dev/null
> xfs_io -f -c "reflink $mnt/foo" $mnt/snap/foo-reflink2 > /dev/unll
> btrfs quota enable $mnt
> 
>  # -W is the new option to only wait rescan while not starting new one
> btrfs quota rescan -W $mnt
> btrfs qgroup show -prce $mnt
> umount $mnt
> 
>  # Need to patch btrfs-progs to report qgroup mismatch as error
> btrfs check $dev || _fail
> ------
> 
> For fast machine, we can hit some corruption which missed accounting
> tree blocks:
> ------
> qgroupid         rfer         excl     max_rfer     max_excl parent  child
> --------         ----         ----     --------     -------- ------  -----
> 0/5           8.00KiB        0.00B         none         none ---     ---
> 0/257         8.00KiB        0.00B         none         none ---     ---
> ------
> 
> This is due to the fact that we're always searching commit root for
> btrfs_find_all_roots() at qgroup_rescan_leaf(), but the leaf we get is
> from current transaction, not commit root.
> 
> And if our tree blocks get modified in current transaction, we won't
> find any owner in commit root, thus causing the corruption.
> 
> Fix it by searching commit root for extent tree for
> qgroup_rescan_leaf().
> 
> Reported-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

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

> ---
> changelog:
> v2:
>   Remove unused tree_mod_seq_elem for the 1st patch.
> v3:
>   Also skip locking to avoid lock assert() with Liu Bo's
>   btrfs_search_slot() patchset.
> ---
>  fs/btrfs/qgroup.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 9fb758d5077a..a88330088d03 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2590,7 +2590,6 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
>  	struct btrfs_key found;
>  	struct extent_buffer *scratch_leaf = NULL;
>  	struct ulist *roots = NULL;
> -	struct seq_list tree_mod_seq_elem = SEQ_LIST_INIT(tree_mod_seq_elem);
>  	u64 num_bytes;
>  	int slot;
>  	int ret;
> @@ -2625,7 +2624,6 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
>  			      btrfs_header_nritems(path->nodes[0]) - 1);
>  	fs_info->qgroup_rescan_progress.objectid = found.objectid + 1;
>  
> -	btrfs_get_tree_mod_seq(fs_info, &tree_mod_seq_elem);
>  	scratch_leaf = btrfs_clone_extent_buffer(path->nodes[0]);
>  	if (!scratch_leaf) {
>  		ret = -ENOMEM;
> @@ -2664,7 +2662,6 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
>  		btrfs_tree_read_unlock_blocking(scratch_leaf);
>  		free_extent_buffer(scratch_leaf);
>  	}
> -	btrfs_put_tree_mod_seq(fs_info, &tree_mod_seq_elem);
>  
>  	return ret;
>  }
> @@ -2681,6 +2678,12 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)
>  	path = btrfs_alloc_path();
>  	if (!path)
>  		goto out;
> +	/*
> +	 * Rescan should only search for commit root, and any later difference
> +	 * should be recorded by qgroup
> +	 */
> +	path->search_commit_root = 1;
> +	path->skip_locking = 1;
>  
>  	err = 0;
>  	while (!err && !btrfs_fs_closing(fs_info)) {
> 

Re: [PATCH v3 1/2] btrfs: qgroup: Search commit root for rescan to avoid missing extent

[David Sterba]

On Wed, May 23, 2018 at 10:32:13AM +0300, Nikolay Borisov wrote:
> 
> 
> On 23.05.2018 10:29, Qu Wenruo wrote:
> > When doing qgroup rescan using the following script (modified from
> > btrfs/017 test case), we can sometimes hit qgroup corruption.
> > 
> > ------
> > umount $dev &> /dev/null
> > umount $mnt &> /dev/null
> > 
> > mkfs.btrfs -f -n 64k $dev
> > mount $dev $mnt
> > 
> > extent_size=8192
> > 
> > xfs_io -f -d -c "pwrite 0 $extent_size" $mnt/foo > /dev/null
> > btrfs subvolume snapshot $mnt $mnt/snap
> > 
> > xfs_io -f -c "reflink $mnt/foo" $mnt/foo-reflink > /dev/null
> > xfs_io -f -c "reflink $mnt/foo" $mnt/snap/foo-reflink > /dev/null
> > xfs_io -f -c "reflink $mnt/foo" $mnt/snap/foo-reflink2 > /dev/unll
> > btrfs quota enable $mnt
> > 
> >  # -W is the new option to only wait rescan while not starting new one
> > btrfs quota rescan -W $mnt
> > btrfs qgroup show -prce $mnt
> > umount $mnt
> > 
> >  # Need to patch btrfs-progs to report qgroup mismatch as error
> > btrfs check $dev || _fail
> > ------
> > 
> > For fast machine, we can hit some corruption which missed accounting
> > tree blocks:
> > ------
> > qgroupid         rfer         excl     max_rfer     max_excl parent  child
> > --------         ----         ----     --------     -------- ------  -----
> > 0/5           8.00KiB        0.00B         none         none ---     ---
> > 0/257         8.00KiB        0.00B         none         none ---     ---
> > ------
> > 
> > This is due to the fact that we're always searching commit root for
> > btrfs_find_all_roots() at qgroup_rescan_leaf(), but the leaf we get is
> > from current transaction, not commit root.
> > 
> > And if our tree blocks get modified in current transaction, we won't
> > find any owner in commit root, thus causing the corruption.
> > 
> > Fix it by searching commit root for extent tree for
> > qgroup_rescan_leaf().
> > 
> > Reported-by: Nikolay Borisov <nborisov@suse.com>
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>

V3 replaced in misc-next, thanks.