| Message ID | 20200714172153.12956-1-marcos@mpdesouza.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] btrfs: volumes: Drop chunk_mutex lock/unlock on btrfs_read_chunk_tree | expand |
On 14.07.20 г. 20:43 ч., Nikolay Borisov wrote: > > > On 14.07.20 г. 20:21 ч., Marcos Paulo de Souza wrote: >> From: Marcos Paulo de Souza <mpdesouza@suse.com> >> >> There is a lockdep report when running xfstests btrfs/161 (seed + sprout >> devices) related to chunk_mutex: >> > > For whatever reason this submission seems garbled. In any case my > proposal was to replace the chunk mutex with FS_EXCL_OP in > btrfs_read_chunk_tree which you haven't done. You simply remove the > chunk mutex. > Actually the mutex could possibly be removed because in order to add/remove a device we need a well-formed mount i.e the mount process must have been completed. Given that chunk tree read happens as part of this I'd assume it's safe to drop it altogether.
On Tue, Jul 14, 2020 at 02:21:53PM -0300, Marcos Paulo de Souza wrote: > From: Marcos Paulo de Souza <mpdesouza@suse.com> > In volumes.c there are comments stating that chunk_mutex is used to > protect add/remove chunks, trim or add/remove devices. Since > btrfs_read_chunk_tree is only called on mount, and trim and chunk alloc > cannot happen at mount time, it's safe to remove this lock/unlock. > > Also, btrfs_ioctl_{add|rm}_dev calls set BTRFS_FS_EXCL_OP, which should > be safe. I've analyzed it in https://lore.kernel.org/linux-btrfs/20200512232827.GI18421@twin.jikos.cz/ and an ioctl gets called during mount with the seeding device. I don't see it mentioned in your changelog. The BTRFS_FS_EXCL_OP do not affect that because there's only one the device add at this time. > Dropping the mutex lock/unlock solves the lockdep warning. Yes, but the question is if this is also correct :) Right now I don't see how the excl_op flag is relevant here. The patch proposed in https://lore.kernel.org/linux-btrfs/20200513194659.34493-1-anand.jain@oracle.com/ has some issues, I tried to fix them back then but for some reason I don't remember the patch did not work as expected.
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index c7a3d4d730a3..94cbdadd9d26 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -7033,7 +7033,6 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info) * otherwise we don't need it. */ mutex_lock(&uuid_mutex); - mutex_lock(&fs_info->chunk_mutex); /* * Read all device items, and then all the chunk items. All @@ -7100,7 +7099,6 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info) } ret = 0; error: - mutex_unlock(&fs_info->chunk_mutex); mutex_unlock(&uuid_mutex); btrfs_free_path(path);