| Message ID | f9d69d94feeab53df416837d5e8bcc85da4df394.1598792561.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: seed fix null ptr, use only main device_list_mutex, and cleanups | expand |
On 30.08.20 г. 17:41 ч., Anand Jain wrote: > In a mounted sprout FS, all threads now are using the > sprout::device_list_mutex, and this is the only piece of code using > the seed::device_list_mutex. This patch converts to use the sprouts > fs_info->fs_devices->device_list_mutex. > > The same reasoning holds true here, that device delete is holding > the sprout::device_list_mutex. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/volumes.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 9921b43ef839..7639a048c6cf 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -7184,16 +7184,14 @@ void btrfs_init_devices_late(struct btrfs_fs_info *fs_info) > mutex_lock(&fs_devices->device_list_mutex); > list_for_each_entry(device, &fs_devices->devices, dev_list) > device->fs_info = fs_info; > - mutex_unlock(&fs_devices->device_list_mutex); > > list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) { > - mutex_lock(&seed_devs->device_list_mutex); > list_for_each_entry(device, &seed_devs->devices, dev_list) > device->fs_info = fs_info; > - mutex_unlock(&seed_devs->device_list_mutex); > > seed_devs->fs_info = fs_info; > } > + mutex_unlock(&fs_devices->device_list_mutex); Instead of doing the looping here to set fs_info I think it makes more sense to move the initialization of fs_info for the seed fs_info/seed devices to open_seed_devices. As a matter of fact at the point where btrfs_init_devices_late is called btrfs_read_chunk_tree would have already been called which would have resulted in all present seed devices be added to fs_info::seed_list. So acquiring the lock here serves no purpose really. > } > > static u64 btrfs_dev_stats_value(const struct extent_buffer *eb, >
On 31/8/20 4:37 pm, Nikolay Borisov wrote: > > > On 30.08.20 г. 17:41 ч., Anand Jain wrote: >> In a mounted sprout FS, all threads now are using the >> sprout::device_list_mutex, and this is the only piece of code using >> the seed::device_list_mutex. This patch converts to use the sprouts >> fs_info->fs_devices->device_list_mutex. >> >> The same reasoning holds true here, that device delete is holding >> the sprout::device_list_mutex. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> fs/btrfs/volumes.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index 9921b43ef839..7639a048c6cf 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -7184,16 +7184,14 @@ void btrfs_init_devices_late(struct btrfs_fs_info *fs_info) >> mutex_lock(&fs_devices->device_list_mutex); >> list_for_each_entry(device, &fs_devices->devices, dev_list) >> device->fs_info = fs_info; >> - mutex_unlock(&fs_devices->device_list_mutex); >> >> list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) { >> - mutex_lock(&seed_devs->device_list_mutex); >> list_for_each_entry(device, &seed_devs->devices, dev_list) >> device->fs_info = fs_info; >> - mutex_unlock(&seed_devs->device_list_mutex); >> >> seed_devs->fs_info = fs_info; >> } >> + mutex_unlock(&fs_devices->device_list_mutex); > > Instead of doing the looping here to set fs_info I think it makes more > sense to move the initialization of fs_info for the seed fs_info/seed > devices to open_seed_devices. > But that will be out of the purpose of this patch. May be in a different patch. And seed_devs->fs_info = fs_info co-exists with the device->fs_info = fs_info as well. > As a matter of fact at the point where btrfs_init_devices_late is called > btrfs_read_chunk_tree would have already been called which would have > resulted in all present seed devices be added to fs_info::seed_list. So > acquiring the lock here serves no purpose really. I tried to tests few times before. If any of the user initiated device operation could race with the mount thread. The ans was no. So the lock is not really required. But then there was mount and unmount racing bug, which was very strange to me. With that bug, if unmount wins the race, user initiated forget thread may free the devices. Now its fixed. So should be safe. But I am not too sure if that covers all the threads that could potentially race. So for now, its ok to consolidate the lock to sprout::device_list_mutex at least. Thanks, Anand > >> } >> >> static u64 btrfs_dev_stats_value(const struct extent_buffer *eb, >>
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 9921b43ef839..7639a048c6cf 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -7184,16 +7184,14 @@ void btrfs_init_devices_late(struct btrfs_fs_info *fs_info) mutex_lock(&fs_devices->device_list_mutex); list_for_each_entry(device, &fs_devices->devices, dev_list) device->fs_info = fs_info; - mutex_unlock(&fs_devices->device_list_mutex); list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) { - mutex_lock(&seed_devs->device_list_mutex); list_for_each_entry(device, &seed_devs->devices, dev_list) device->fs_info = fs_info; - mutex_unlock(&seed_devs->device_list_mutex); seed_devs->fs_info = fs_info; } + mutex_unlock(&fs_devices->device_list_mutex); } static u64 btrfs_dev_stats_value(const struct extent_buffer *eb,
In a mounted sprout FS, all threads now are using the sprout::device_list_mutex, and this is the only piece of code using the seed::device_list_mutex. This patch converts to use the sprouts fs_info->fs_devices->device_list_mutex. The same reasoning holds true here, that device delete is holding the sprout::device_list_mutex. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/volumes.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)