diff mbox series

[v2,3/3] Btrfs: remove no longer necessary chunk mutex locking cases

Message ID 20200601181227.28585-1-fdmanana@kernel.org (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Filipe Manana June 1, 2020, 6:12 p.m. UTC 
From: Filipe Manana <fdmanana@suse.com>

Initially when the 'removed' flag was added to a block group to avoid
races between block group removal and fitrim, by commit 04216820fe83d5
("Btrfs: fix race between fs trimming and block group remove/allocation"),
we had to lock the chunks mutex because we could be moving the block
group from its current list, the pending chunks list, into the pinned
chunks list, or we could just be adding it to the pinned chunks if it was
not in the pending chunks list. Both lists were protected by the chunk
mutex.

However we no longer have those lists since commit 1c11b63eff2a67
("btrfs: replace pending/pinned chunks lists with io tree"), and locking
the chunk mutex is no longer necessary because of that. The same happens
at btrfs_unfreeze_block_group(), we lock the chunk mutex because the block
group's extent map could be part of the pinned chunks list and the call
to remove_extent_mapping() could be deleting it from that list, which
used to be protected by that mutex.

So just remove those lock and unlock calls as they are not needed anymore.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---

V2: Rebased on 5.8 changes, since the previous patch had to be rebased too.

 fs/btrfs/block-group.c | 5 -----
 1 file changed, 5 deletions(-)

Comments

Nikolay Borisov June 3, 2020, 7:36 a.m. UTC | #1 
On 1.06.20 г. 21:12 ч., fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Initially when the 'removed' flag was added to a block group to avoid
> races between block group removal and fitrim, by commit 04216820fe83d5
> ("Btrfs: fix race between fs trimming and block group remove/allocation"),
> we had to lock the chunks mutex because we could be moving the block
> group from its current list, the pending chunks list, into the pinned
> chunks list, or we could just be adding it to the pinned chunks if it was
> not in the pending chunks list. Both lists were protected by the chunk
> mutex.
> 
> However we no longer have those lists since commit 1c11b63eff2a67
> ("btrfs: replace pending/pinned chunks lists with io tree"), and locking
> the chunk mutex is no longer necessary because of that. The same happens
> at btrfs_unfreeze_block_group(), we lock the chunk mutex because the block
> group's extent map could be part of the pinned chunks list and the call
> to remove_extent_mapping() could be deleting it from that list, which
> used to be protected by that mutex.
> 
> So just remove those lock and unlock calls as they are not needed anymore.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

The critical section really involves checking btrfs_block_group::frozen
and btrfs_block_group::removed  atomically which is ensured by holding
btrfs_block_group::lock. And in the unfreeze function we only modify the
mapping tree under the tree's write lock, so:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
David Sterba June 4, 2020, 5:14 p.m. UTC | #2 
On Mon, Jun 01, 2020 at 07:12:27PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Initially when the 'removed' flag was added to a block group to avoid
> races between block group removal and fitrim, by commit 04216820fe83d5
> ("Btrfs: fix race between fs trimming and block group remove/allocation"),
> we had to lock the chunks mutex because we could be moving the block
> group from its current list, the pending chunks list, into the pinned
> chunks list, or we could just be adding it to the pinned chunks if it was
> not in the pending chunks list. Both lists were protected by the chunk
> mutex.
> 
> However we no longer have those lists since commit 1c11b63eff2a67
> ("btrfs: replace pending/pinned chunks lists with io tree"), and locking
> the chunk mutex is no longer necessary because of that. The same happens
> at btrfs_unfreeze_block_group(), we lock the chunk mutex because the block
> group's extent map could be part of the pinned chunks list and the call
> to remove_extent_mapping() could be deleting it from that list, which
> used to be protected by that mutex.
> 
> So just remove those lock and unlock calls as they are not needed anymore.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
> 
> V2: Rebased on 5.8 changes, since the previous patch had to be rebased too.

2 and 3 in misc next, thanks.
diff mbox series

Patch

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index be73055ee3a6..a10834b837a5 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1111,7 +1111,6 @@  int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
 	if (ret < 0)
 		goto out;
 
-	mutex_lock(&fs_info->chunk_mutex);
 	spin_lock(&block_group->lock);
 	block_group->removed = 1;
 	/*
@@ -1143,8 +1142,6 @@  int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
 	remove_em = (atomic_read(&block_group->frozen) == 0);
 	spin_unlock(&block_group->lock);
 
-	mutex_unlock(&fs_info->chunk_mutex);
-
 	if (remove_em) {
 		struct extent_map_tree *em_tree;
 
@@ -3447,7 +3444,6 @@  void btrfs_unfreeze_block_group(struct btrfs_block_group *block_group)
 	spin_unlock(&block_group->lock);
 
 	if (cleanup) {
-		mutex_lock(&fs_info->chunk_mutex);
 		em_tree = &fs_info->mapping_tree;
 		write_lock(&em_tree->lock);
 		em = lookup_extent_mapping(em_tree, block_group->start,
@@ -3455,7 +3451,6 @@  void btrfs_unfreeze_block_group(struct btrfs_block_group *block_group)
 		BUG_ON(!em); /* logic error, can't happen */
 		remove_extent_mapping(em_tree, em);
 		write_unlock(&em_tree->lock);
-		mutex_unlock(&fs_info->chunk_mutex);
 
 		/* once for us and once for the tree */
 		free_extent_map(em);