Re: [PATCH v3 RESENT 2/2] btrfs: qgroup: Don't copy extent buffer to do qgroup rescan

[Qu Wenruo]

Filipe Manana wrote on 2015/10/22 09:16 +0100:

On Thu, Oct 22, 2015 at 1:42 AM, Qu Wenruo  wrote:

Ancient qgroup code call memcpy() on a extent buffer and use it for leaf
iteration.

As extent buffer contains lock, pointers to pages, it's never sane to do
such copy.

The following bug may be caused by this insane operation:
[92098.841309] general protection fault:  [#1] SMP
[92098.841338] Modules linked in: ...
[92098.841814] CPU: 1 PID: 24655 Comm: kworker/u4:12 Not tainted
4.3.0-rc1 #1
[92098.841868] Workqueue: btrfs-qgroup-rescan btrfs_qgroup_rescan_helper
[btrfs]
[92098.842261] Call Trace:
[92098.842277]  [] ? read_extent_buffer+0xb8/0x110
[btrfs]
[92098.842304]  [] ? btrfs_find_all_roots+0x60/0x70
[btrfs]
[92098.842329]  []
btrfs_qgroup_rescan_worker+0x28d/0x5a0 [btrfs]

Where btrfs_qgroup_rescan_worker+0x28d is btrfs_disk_key_to_cpu(),
called in reading key from the memcpied extent_buffer.

This patch will read the whole leaf into memory, and use newly
introduced stack function to do qgroup rescan.


Hi Qu,

Instead of introducing more new functions, why not clone the extent
buffer (btrfs_clone_extent_buffer) and then use it the
regular/existing functions? Iow, the same as we do in backref walking,
should make the change much smaller than it is.

thanks


Thanks Filipe,

I didn't know there is such a nice function.
And it's setting EXTENT_BUFFER_DUMMY, so it should be quite safe for the 
use case.


Thanks for your advice a lot!
Qu



Reported-by: Stephane Lesimple 
Signed-off-by: Qu Wenruo 
---
v2:
   Follow the parameter change in previous patch.
v3:
   None
---
  fs/btrfs/qgroup.c | 22 --
  1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index e9ace09..6a83a40 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2183,11 +2183,11 @@ void assert_qgroups_uptodate(struct btrfs_trans_handle 
*trans)
   */
  static int
  qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
-  struct btrfs_trans_handle *trans,
-  struct extent_buffer *scratch_leaf)
+  struct btrfs_trans_handle *trans, char *stack_leaf)
  {
 struct btrfs_key found;
 struct ulist *roots = NULL;
+   struct btrfs_header *header;
 struct seq_list tree_mod_seq_elem = SEQ_LIST_INIT(tree_mod_seq_elem);
 u64 num_bytes;
 int slot;
@@ -2224,13 +2224,15 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, 
struct btrfs_path *path,
 fs_info->qgroup_rescan_progress.objectid = found.objectid + 1;

 btrfs_get_tree_mod_seq(fs_info, _mod_seq_elem);
-   memcpy(scratch_leaf, path->nodes[0], sizeof(*scratch_leaf));
+   read_extent_buffer(path->nodes[0], stack_leaf, 0,
+  fs_info->extent_root->nodesize);
+   header = (struct btrfs_header *)stack_leaf;
 slot = path->slots[0];
 btrfs_release_path(path);
 mutex_unlock(_info->qgroup_rescan_lock);

-   for (; slot < btrfs_header_nritems(scratch_leaf); ++slot) {
-   btrfs_item_key_to_cpu(scratch_leaf, , slot);
+   for (; slot < btrfs_stack_header_nritems(header); ++slot) {
+   btrfs_stack_item_key_to_cpu(header, , slot);
 if (found.type != BTRFS_EXTENT_ITEM_KEY &&
 found.type != BTRFS_METADATA_ITEM_KEY)
 continue;
@@ -2261,15 +2263,15 @@ static void btrfs_qgroup_rescan_worker(struct 
btrfs_work *work)
  qgroup_rescan_work);
 struct btrfs_path *path;
 struct btrfs_trans_handle *trans = NULL;
-   struct extent_buffer *scratch_leaf = NULL;
+   char *stack_leaf = NULL;
 int err = -ENOMEM;
 int ret = 0;

 path = btrfs_alloc_path();
 if (!path)
 goto out;
-   scratch_leaf = kmalloc(sizeof(*scratch_leaf), GFP_NOFS);
-   if (!scratch_leaf)
+   stack_leaf = kmalloc(fs_info->extent_root->nodesize, GFP_NOFS);
+   if (!stack_leaf)
 goto out;

 err = 0;
@@ -2283,7 +2285,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work 
*work)
 err = -EINTR;
 } else {
 err = qgroup_rescan_leaf(fs_info, path, trans,
-scratch_leaf);
+stack_leaf);
 }
 if (err > 0)
 btrfs_commit_transaction(trans, fs_info->fs_root);
@@ -2292,7 +2294,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work 
*work)
 }

  out:
-   kfree(scratch_leaf);
+   kfree(stack_leaf);
 btrfs_free_path(path);

 mutex_lock(_info->qgroup_rescan_lock);
--
2.6.1

--
To unsubscribe from this list: send the line 

Re: [PATCH v3 RESENT 2/2] btrfs: qgroup: Don't copy extent buffer to do qgroup rescan

[Filipe Manana]

On Thu, Oct 22, 2015 at 1:42 AM, Qu Wenruo  wrote:
> Ancient qgroup code call memcpy() on a extent buffer and use it for leaf
> iteration.
>
> As extent buffer contains lock, pointers to pages, it's never sane to do
> such copy.
>
> The following bug may be caused by this insane operation:
> [92098.841309] general protection fault:  [#1] SMP
> [92098.841338] Modules linked in: ...
> [92098.841814] CPU: 1 PID: 24655 Comm: kworker/u4:12 Not tainted
> 4.3.0-rc1 #1
> [92098.841868] Workqueue: btrfs-qgroup-rescan btrfs_qgroup_rescan_helper
> [btrfs]
> [92098.842261] Call Trace:
> [92098.842277]  [] ? read_extent_buffer+0xb8/0x110
> [btrfs]
> [92098.842304]  [] ? btrfs_find_all_roots+0x60/0x70
> [btrfs]
> [92098.842329]  []
> btrfs_qgroup_rescan_worker+0x28d/0x5a0 [btrfs]
>
> Where btrfs_qgroup_rescan_worker+0x28d is btrfs_disk_key_to_cpu(),
> called in reading key from the memcpied extent_buffer.
>
> This patch will read the whole leaf into memory, and use newly
> introduced stack function to do qgroup rescan.

Hi Qu,

Instead of introducing more new functions, why not clone the extent
buffer (btrfs_clone_extent_buffer) and then use it the
regular/existing functions? Iow, the same as we do in backref walking,
should make the change much smaller than it is.

thanks

>
> Reported-by: Stephane Lesimple 
> Signed-off-by: Qu Wenruo 
> ---
> v2:
>   Follow the parameter change in previous patch.
> v3:
>   None
> ---
>  fs/btrfs/qgroup.c | 22 --
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index e9ace09..6a83a40 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2183,11 +2183,11 @@ void assert_qgroups_uptodate(struct 
> btrfs_trans_handle *trans)
>   */
>  static int
>  qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
> -  struct btrfs_trans_handle *trans,
> -  struct extent_buffer *scratch_leaf)
> +  struct btrfs_trans_handle *trans, char *stack_leaf)
>  {
> struct btrfs_key found;
> struct ulist *roots = NULL;
> +   struct btrfs_header *header;
> struct seq_list tree_mod_seq_elem = SEQ_LIST_INIT(tree_mod_seq_elem);
> u64 num_bytes;
> int slot;
> @@ -2224,13 +2224,15 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, 
> struct btrfs_path *path,
> fs_info->qgroup_rescan_progress.objectid = found.objectid + 1;
>
> btrfs_get_tree_mod_seq(fs_info, _mod_seq_elem);
> -   memcpy(scratch_leaf, path->nodes[0], sizeof(*scratch_leaf));
> +   read_extent_buffer(path->nodes[0], stack_leaf, 0,
> +  fs_info->extent_root->nodesize);
> +   header = (struct btrfs_header *)stack_leaf;
> slot = path->slots[0];
> btrfs_release_path(path);
> mutex_unlock(_info->qgroup_rescan_lock);
>
> -   for (; slot < btrfs_header_nritems(scratch_leaf); ++slot) {
> -   btrfs_item_key_to_cpu(scratch_leaf, , slot);
> +   for (; slot < btrfs_stack_header_nritems(header); ++slot) {
> +   btrfs_stack_item_key_to_cpu(header, , slot);
> if (found.type != BTRFS_EXTENT_ITEM_KEY &&
> found.type != BTRFS_METADATA_ITEM_KEY)
> continue;
> @@ -2261,15 +2263,15 @@ static void btrfs_qgroup_rescan_worker(struct 
> btrfs_work *work)
>  qgroup_rescan_work);
> struct btrfs_path *path;
> struct btrfs_trans_handle *trans = NULL;
> -   struct extent_buffer *scratch_leaf = NULL;
> +   char *stack_leaf = NULL;
> int err = -ENOMEM;
> int ret = 0;
>
> path = btrfs_alloc_path();
> if (!path)
> goto out;
> -   scratch_leaf = kmalloc(sizeof(*scratch_leaf), GFP_NOFS);
> -   if (!scratch_leaf)
> +   stack_leaf = kmalloc(fs_info->extent_root->nodesize, GFP_NOFS);
> +   if (!stack_leaf)
> goto out;
>
> err = 0;
> @@ -2283,7 +2285,7 @@ static void btrfs_qgroup_rescan_worker(struct 
> btrfs_work *work)
> err = -EINTR;
> } else {
> err = qgroup_rescan_leaf(fs_info, path, trans,
> -scratch_leaf);
> +stack_leaf);
> }
> if (err > 0)
> btrfs_commit_transaction(trans, fs_info->fs_root);
> @@ -2292,7 +2294,7 @@ static void btrfs_qgroup_rescan_worker(struct 
> btrfs_work *work)
> }
>
>  out:
> -   kfree(scratch_leaf);
> +   kfree(stack_leaf);
> btrfs_free_path(path);
>
> mutex_lock(_info->qgroup_rescan_lock);
> --
> 2.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to 

[PATCH v3 RESENT 2/2] btrfs: qgroup: Don't copy extent buffer to do qgroup rescan

[Qu Wenruo]

Ancient qgroup code call memcpy() on a extent buffer and use it for leaf
iteration.

As extent buffer contains lock, pointers to pages, it's never sane to do
such copy.

The following bug may be caused by this insane operation:
[92098.841309] general protection fault:  [#1] SMP
[92098.841338] Modules linked in: ...
[92098.841814] CPU: 1 PID: 24655 Comm: kworker/u4:12 Not tainted
4.3.0-rc1 #1
[92098.841868] Workqueue: btrfs-qgroup-rescan btrfs_qgroup_rescan_helper
[btrfs]
[92098.842261] Call Trace:
[92098.842277]  [] ? read_extent_buffer+0xb8/0x110
[btrfs]
[92098.842304]  [] ? btrfs_find_all_roots+0x60/0x70
[btrfs]
[92098.842329]  []
btrfs_qgroup_rescan_worker+0x28d/0x5a0 [btrfs]

Where btrfs_qgroup_rescan_worker+0x28d is btrfs_disk_key_to_cpu(),
called in reading key from the memcpied extent_buffer.

This patch will read the whole leaf into memory, and use newly
introduced stack function to do qgroup rescan.

Reported-by: Stephane Lesimple 
Signed-off-by: Qu Wenruo 
---
v2:
  Follow the parameter change in previous patch.
v3:
  None
---
 fs/btrfs/qgroup.c | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index e9ace09..6a83a40 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2183,11 +2183,11 @@ void assert_qgroups_uptodate(struct btrfs_trans_handle 
*trans)
  */
 static int
 qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
-  struct btrfs_trans_handle *trans,
-  struct extent_buffer *scratch_leaf)
+  struct btrfs_trans_handle *trans, char *stack_leaf)
 {
struct btrfs_key found;
struct ulist *roots = NULL;
+   struct btrfs_header *header;
struct seq_list tree_mod_seq_elem = SEQ_LIST_INIT(tree_mod_seq_elem);
u64 num_bytes;
int slot;
@@ -2224,13 +2224,15 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, 
struct btrfs_path *path,
fs_info->qgroup_rescan_progress.objectid = found.objectid + 1;
 
btrfs_get_tree_mod_seq(fs_info, _mod_seq_elem);
-   memcpy(scratch_leaf, path->nodes[0], sizeof(*scratch_leaf));
+   read_extent_buffer(path->nodes[0], stack_leaf, 0,
+  fs_info->extent_root->nodesize);
+   header = (struct btrfs_header *)stack_leaf;
slot = path->slots[0];
btrfs_release_path(path);
mutex_unlock(_info->qgroup_rescan_lock);
 
-   for (; slot < btrfs_header_nritems(scratch_leaf); ++slot) {
-   btrfs_item_key_to_cpu(scratch_leaf, , slot);
+   for (; slot < btrfs_stack_header_nritems(header); ++slot) {
+   btrfs_stack_item_key_to_cpu(header, , slot);
if (found.type != BTRFS_EXTENT_ITEM_KEY &&
found.type != BTRFS_METADATA_ITEM_KEY)
continue;
@@ -2261,15 +2263,15 @@ static void btrfs_qgroup_rescan_worker(struct 
btrfs_work *work)
 qgroup_rescan_work);
struct btrfs_path *path;
struct btrfs_trans_handle *trans = NULL;
-   struct extent_buffer *scratch_leaf = NULL;
+   char *stack_leaf = NULL;
int err = -ENOMEM;
int ret = 0;
 
path = btrfs_alloc_path();
if (!path)
goto out;
-   scratch_leaf = kmalloc(sizeof(*scratch_leaf), GFP_NOFS);
-   if (!scratch_leaf)
+   stack_leaf = kmalloc(fs_info->extent_root->nodesize, GFP_NOFS);
+   if (!stack_leaf)
goto out;
 
err = 0;
@@ -2283,7 +2285,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work 
*work)
err = -EINTR;
} else {
err = qgroup_rescan_leaf(fs_info, path, trans,
-scratch_leaf);
+stack_leaf);
}
if (err > 0)
btrfs_commit_transaction(trans, fs_info->fs_root);
@@ -2292,7 +2294,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work 
*work)
}
 
 out:
-   kfree(scratch_leaf);
+   kfree(stack_leaf);
btrfs_free_path(path);
 
mutex_lock(_info->qgroup_rescan_lock);
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html