| Message ID | 92caa5382783c709d6ddab7d06747327bd7c120c.1529516228.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/21/2018 01:51 AM, David Sterba wrote: > The stale device list removal needs to be protected by device_list_mutex > too as this could delete from the list and could race with another list > modification and cause crash. Its is very less likely or almost practically impossible (unless instrumented) to have same device with same fsid but with different devid, which then it would delete the device which is under the same fs_devices which device_list_add just operated. Otherwise in most of the common cases it shall delete the device which does not belong to the fs_devices which the device_list_add() just added a device. So fs_devices::device_list_mutex lock won't help. I have few patches to fix this. I shall send it. But then it was all based on the atomic volume exclusive flag which in stages I had plans to replace fs_info::BTRFS_FS_EXCL_OP with volume exclusive flag as well. Also we need locks to manage with in btrfs_free_stale_devices() so that we can reuse this code to support user land forget device feature, (38cf665d338fca33af4b16f9ec7cad6637fc0fec btrfs: make btrfs_free_stale_device() to iterate all stales). Thanks, Anand > The device needs to be fully initialized before it's added to the list > so the fs_devices also need to be set under the mutex. > > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/volumes.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 1da162928d1a..02246f9af0a3 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -791,12 +791,11 @@ static noinline struct btrfs_device *device_list_add(const char *path, > rcu_assign_pointer(device->name, name); > > mutex_lock(&fs_devices->device_list_mutex); > + device->fs_devices = fs_devices; > list_add_rcu(&device->dev_list, &fs_devices->devices); > fs_devices->num_devices++; > - mutex_unlock(&fs_devices->device_list_mutex); > - > - device->fs_devices = fs_devices; > btrfs_free_stale_devices(path, device); > + mutex_unlock(&fs_devices->device_list_mutex); > > if (disk_super->label[0]) > pr_info("BTRFS: device label %s devid %llu transid %llu %s\n", > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/21/2018 01:51 AM, David Sterba wrote: > The stale device list removal needs to be protected by device_list_mutex > too as this could delete from the list and could race with another list > modification and cause crash. > > The device needs to be fully initialized before it's added to the list > so the fs_devices also need to be set under the mutex. > > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/volumes.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 1da162928d1a..02246f9af0a3 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -791,12 +791,11 @@ static noinline struct btrfs_device *device_list_add(const char *path, > rcu_assign_pointer(device->name, name); > > mutex_lock(&fs_devices->device_list_mutex); > + device->fs_devices = fs_devices; > list_add_rcu(&device->dev_list, &fs_devices->devices); > fs_devices->num_devices++; > - mutex_unlock(&fs_devices->device_list_mutex); > - > - device->fs_devices = fs_devices; > btrfs_free_stale_devices(path, device); This is not correct. btrfs_free_stale_devices need the per fs_devices local device_list_mutex lock as it traverses through the fs_uuids list. Holding just the lock of the %fs_devices which is being scanned or mounted is not correct. In my workspace I have replaced this patch with .. 4130724ec926 btrfs: fix btrfs_free_stale_devices() with needed locks 90fedec0c200 btrfs: btrfs_free_stale_devices() rename local variables 439eec4e943c btrfs: fix device_list_add() missing device_list_mutex() 417ecc91e223 btrfs: do btrfs_free_stale_devices() outside of device_list_add() which are in the ML as well. Thanks, Anand > + mutex_unlock(&fs_devices->device_list_mutex); > > if (disk_super->label[0]) > pr_info("BTRFS: device label %s devid %llu transid %llu %s\n", > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 04, 2018 at 04:18:09PM +0800, Anand Jain wrote: > > > On 06/21/2018 01:51 AM, David Sterba wrote: > > The stale device list removal needs to be protected by device_list_mutex > > too as this could delete from the list and could race with another list > > modification and cause crash. > > > > The device needs to be fully initialized before it's added to the list > > so the fs_devices also need to be set under the mutex. > > > > Signed-off-by: David Sterba <dsterba@suse.com> > > --- > > fs/btrfs/volumes.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > > index 1da162928d1a..02246f9af0a3 100644 > > --- a/fs/btrfs/volumes.c > > +++ b/fs/btrfs/volumes.c > > @@ -791,12 +791,11 @@ static noinline struct btrfs_device *device_list_add(const char *path, > > rcu_assign_pointer(device->name, name); > > > > mutex_lock(&fs_devices->device_list_mutex); > > + device->fs_devices = fs_devices; > > list_add_rcu(&device->dev_list, &fs_devices->devices); > > fs_devices->num_devices++; > > - mutex_unlock(&fs_devices->device_list_mutex); > > - > > - device->fs_devices = fs_devices; > > btrfs_free_stale_devices(path, device); > > > This is not correct. > > btrfs_free_stale_devices need the per fs_devices local device_list_mutex > lock as it traverses through the fs_uuids list. Holding just the lock of > the %fs_devices which is being scanned or mounted is not correct. I see, ok. The use of fs_devs is not very visible in btrfs_free_stale_devices but at least the function comment hints that all devices are being traversed. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 1da162928d1a..02246f9af0a3 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -791,12 +791,11 @@ static noinline struct btrfs_device *device_list_add(const char *path, rcu_assign_pointer(device->name, name); mutex_lock(&fs_devices->device_list_mutex); + device->fs_devices = fs_devices; list_add_rcu(&device->dev_list, &fs_devices->devices); fs_devices->num_devices++; - mutex_unlock(&fs_devices->device_list_mutex); - - device->fs_devices = fs_devices; btrfs_free_stale_devices(path, device); + mutex_unlock(&fs_devices->device_list_mutex); if (disk_super->label[0]) pr_info("BTRFS: device label %s devid %llu transid %llu %s\n",
The stale device list removal needs to be protected by device_list_mutex too as this could delete from the list and could race with another list modification and cause crash. The device needs to be fully initialized before it's added to the list so the fs_devices also need to be set under the mutex. Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/volumes.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)