Hi Filipe,
On Tue, Dec 2, 2014 at 8:08 PM Filipe Manana <fdmanana@xxxxxxxx> wrote:
>
> On block group remove if the corresponding extent map was on the
> transaction->pending_chunks list, we were deleting the extent map
> from that list, through remove_extent_mapping(), without any
> synchronization with chunk allocation (which iterates that list
> and adds new elements to it). Fix this by ensure that this is done
> while the chunk mutex is held, since that's the mutex that protects
> the list in the chunk allocation code path.
>
> This applies on top (depends on) of my previous patch titled:
> "Btrfs: fix race between fs trimming and block group remove/allocation"
>
> But the issue in fact was already present before that change, it only
> became easier to hit after Josef's 3.18 patch that added automatic
> removal of empty block groups.
>
> Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
> ---
> fs/btrfs/extent-tree.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 17d429d..a7b81b4 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -9524,19 +9524,25 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
> list_move_tail(&em->list, &root->fs_info->pinned_chunks);
> }
> spin_unlock(&block_group->lock);
> - unlock_chunks(root);
>
> if (remove_em) {
> struct extent_map_tree *em_tree;
>
> em_tree = &root->fs_info->mapping_tree.map_tree;
> write_lock(&em_tree->lock);
> + /*
> + * The em might be in the pending_chunks list, so make sure the
> + * chunk mutex is locked, since remove_extent_mapping() will
> + * delete us from that list.
> + */
> remove_extent_mapping(em_tree, em);
> write_unlock(&em_tree->lock);
If the "em" was in pending_chunks, it will be deleted from that list
by "remove_extent_mapping". But it looks like in this case we also
need to drop the extra ref on "em", which was held by pending_chunks
list. I don't see it being done anywhere else. So we should check
before the remove_extent_mapping() call whether "em" was in
pending_chunks, and, if yes, drop the extra ref?
Thanks,
Alex.
> /* once for the tree */
> free_extent_map(em);
> }
>
> + unlock_chunks(root);
> +
> btrfs_put_block_group(block_group);
> btrfs_put_block_group(block_group);
>
> --
> 2.1.3
>
> --
> 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
