On Thu, Oct 22, 2015 at 1:42 AM, Qu Wenruo <[email protected]> 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 <[email protected]> > Signed-off-by: Qu Wenruo <[email protected]> > --- > 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 [email protected] > 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
