On Thu, Oct 22, 2015 at 1:42 AM, Qu Wenruo <quwenruo@xxxxxxxxxxxxxx> 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: 0000 [#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] [<ffffffffc035a5d8>] ? read_extent_buffer+0xb8/0x110
> [btrfs]
> [92098.842304] [<ffffffffc0396d00>] ? btrfs_find_all_roots+0x60/0x70
> [btrfs]
> [92098.842329] [<ffffffffc039af3d>]
> 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 <stephane_btrfs@xxxxxxxxxxx>
> Signed-off-by: Qu Wenruo <quwenruo@xxxxxxxxxxxxxx>
> ---
> 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, &tree_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(&fs_info->qgroup_rescan_lock);
>
> - for (; slot < btrfs_header_nritems(scratch_leaf); ++slot) {
> - btrfs_item_key_to_cpu(scratch_leaf, &found, slot);
> + for (; slot < btrfs_stack_header_nritems(header); ++slot) {
> + btrfs_stack_item_key_to_cpu(header, &found, 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(&fs_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 majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Filipe David Manana,
"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves.
That's why all progress depends on unreasonable men."
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
