* [PATCH 0/3] btrf_show_devname related fixes
@ 2021-08-19 18:18 Anand Jain
2021-08-19 18:18 ` [PATCH 1/3] btrfs: fix comment about the btrfs_show_devname Anand Jain
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Anand Jain @ 2021-08-19 18:18 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba, l
Related to the bug/fix posted to the mailing list as below
btrfs: traverse seed devices if fs_devices::devices is empty in show_devname
Anand Jain (3):
btrfs: fix comment about the btrfs_show_devname
btrfs: consolidate device_list_mutex in prepare_sprout to its parent
btrfs: use latest_bdev in btrfs_show_devname
fs/btrfs/super.c | 25 +++----------------------
fs/btrfs/volumes.c | 11 ++++-------
2 files changed, 7 insertions(+), 29 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] btrfs: fix comment about the btrfs_show_devname
2021-08-19 18:18 [PATCH 0/3] btrf_show_devname related fixes Anand Jain
@ 2021-08-19 18:18 ` Anand Jain
2021-08-19 18:18 ` [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent Anand Jain
2021-08-19 18:18 ` [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname Anand Jain
2 siblings, 0 replies; 15+ messages in thread
From: Anand Jain @ 2021-08-19 18:18 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba, l
There were few lock dep warnings because btrfs_show_devname() was using
device_list_mutex as recorded in the commits
ccd05285e7f (btrfs: fix a possible umount deadlock)
779bf3fefa8 (btrfs: fix lock dep warning, move scratch dev out of
device_list_mutex and uuid_mutex)
And finally, commit 88c14590cdd6 (btrfs: use RCU in btrfs_show_devname
for device list traversal) removed the device_list_mutex from
btrfs_show_devname for performance reasons.
This patch fixes a stale comment about the function btrfs_show_devname.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/volumes.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9cb4ed90888d..91b8422b3f67 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2278,10 +2278,7 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev)
/*
* The update_dev_time() with in btrfs_scratch_superblocks()
- * may lead to a call to btrfs_show_devname() which will try
- * to hold device_list_mutex. And here this device
- * is already out of device list, so we don't have to hold
- * the device_list_mutex lock.
+ * may lead to a call to btrfs_show_devname().
*/
btrfs_scratch_superblocks(tgtdev->fs_info, tgtdev->bdev,
tgtdev->name->str);
--
2.31.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent
2021-08-19 18:18 [PATCH 0/3] btrf_show_devname related fixes Anand Jain
2021-08-19 18:18 ` [PATCH 1/3] btrfs: fix comment about the btrfs_show_devname Anand Jain
@ 2021-08-19 18:18 ` Anand Jain
2021-08-20 7:51 ` Su Yue
2021-08-19 18:18 ` [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname Anand Jain
2 siblings, 1 reply; 15+ messages in thread
From: Anand Jain @ 2021-08-19 18:18 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba, l
btrfs_prepare_sprout() moves seed devices into its own struct fs_devices,
so that its parent function btrfs_init_new_device() can add the new sprout
device to fs_info->fs_devices.
Both btrfs_prepare_sprout() and btrfs_init_new_device() needs
device_list_mutex. But they are holding it sequentially, thus creates a
small window to an opportunity to race. Close this opportunity and hold
device_list_mutex common to both btrfs_init_new_device() and
btrfs_prepare_sprout().
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
RFC because I haven't identified the other thread which could race with
this, but still does this cleanup makes sense?
fs/btrfs/volumes.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 91b8422b3f67..f490d1897c56 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
u64 super_flags;
lockdep_assert_held(&uuid_mutex);
+ lockdep_assert_held(&fs_devices->device_list_mutex);
+
if (!fs_devices->seeding)
return -EINVAL;
@@ -2397,7 +2399,6 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
INIT_LIST_HEAD(&seed_devices->alloc_list);
mutex_init(&seed_devices->device_list_mutex);
- mutex_lock(&fs_devices->device_list_mutex);
list_splice_init_rcu(&fs_devices->devices, &seed_devices->devices,
synchronize_rcu);
list_for_each_entry(device, &seed_devices->devices, dev_list)
@@ -2413,7 +2414,6 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
generate_random_uuid(fs_devices->fsid);
memcpy(fs_devices->metadata_uuid, fs_devices->fsid, BTRFS_FSID_SIZE);
memcpy(disk_super->fsid, fs_devices->fsid, BTRFS_FSID_SIZE);
- mutex_unlock(&fs_devices->device_list_mutex);
super_flags = btrfs_super_flags(disk_super) &
~BTRFS_SUPER_FLAG_SEEDING;
@@ -2588,6 +2588,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
device->dev_stats_valid = 1;
set_blocksize(device->bdev, BTRFS_BDEV_BLOCKSIZE);
+ mutex_lock(&fs_devices->device_list_mutex);
if (seeding_dev) {
btrfs_clear_sb_rdonly(sb);
ret = btrfs_prepare_sprout(fs_info);
@@ -2599,7 +2600,6 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
device->fs_devices = fs_devices;
- mutex_lock(&fs_devices->device_list_mutex);
mutex_lock(&fs_info->chunk_mutex);
list_add_rcu(&device->dev_list, &fs_devices->devices);
list_add(&device->dev_alloc_list, &fs_devices->alloc_list);
--
2.31.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname
2021-08-19 18:18 [PATCH 0/3] btrf_show_devname related fixes Anand Jain
2021-08-19 18:18 ` [PATCH 1/3] btrfs: fix comment about the btrfs_show_devname Anand Jain
2021-08-19 18:18 ` [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent Anand Jain
@ 2021-08-19 18:18 ` Anand Jain
2021-08-20 7:31 ` Su Yue
2021-08-20 10:57 ` David Sterba
2 siblings, 2 replies; 15+ messages in thread
From: Anand Jain @ 2021-08-19 18:18 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba, l
latest_bdev is updated according to the changes to the device list.
That means we could use the latest_bdev to show the device name in
/proc/self/mounts. So this patch makes that change.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
RFC because
1. latest_bdev might not be the lowest devid but, we showed
the lowest devid in /proc/self/mount.
2. The device's path is not shown now but, previously we did.
So does these break ABI? Maybe yes for 2 howabout for 1 above?
fs/btrfs/super.c | 25 +++----------------------
1 file changed, 3 insertions(+), 22 deletions(-)
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 1f9dd1a4faa3..4ad3fe174c41 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2464,30 +2464,11 @@ static int btrfs_unfreeze(struct super_block *sb)
static int btrfs_show_devname(struct seq_file *m, struct dentry *root)
{
struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb);
- struct btrfs_device *dev, *first_dev = NULL;
+ char name[BDEVNAME_SIZE];
- /*
- * Lightweight locking of the devices. We should not need
- * device_list_mutex here as we only read the device data and the list
- * is protected by RCU. Even if a device is deleted during the list
- * traversals, we'll get valid data, the freeing callback will wait at
- * least until the rcu_read_unlock.
- */
- rcu_read_lock();
- list_for_each_entry_rcu(dev, &fs_info->fs_devices->devices, dev_list) {
- if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state))
- continue;
- if (!dev->name)
- continue;
- if (!first_dev || dev->devid < first_dev->devid)
- first_dev = dev;
- }
+ seq_escape(m, bdevname(fs_info->fs_devices->latest_bdev, name),
+ " \t\n\\");
- if (first_dev)
- seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\");
- else
- WARN_ON(1);
- rcu_read_unlock();
return 0;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname
2021-08-19 18:18 ` [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname Anand Jain
@ 2021-08-20 7:31 ` Su Yue
2021-08-20 9:13 ` Anand Jain
2021-08-20 10:57 ` David Sterba
1 sibling, 1 reply; 15+ messages in thread
From: Su Yue @ 2021-08-20 7:31 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, dsterba
On Fri 20 Aug 2021 at 02:18, Anand Jain <anand.jain@oracle.com>
wrote:
> latest_bdev is updated according to the changes to the device
> list.
> That means we could use the latest_bdev to show the device name
> in
> /proc/self/mounts. So this patch makes that change.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>
> RFC because
> 1. latest_bdev might not be the lowest devid but, we showed
> the lowest devid in /proc/self/mount.
> 2. The device's path is not shown now but, previously we did.
> So does these break ABI? Maybe yes for 2 howabout for 1 above?
>
Mabybe a naive question but I have no time to dive into btrfs code
recently. If a device which has highest devid was replaced, will
the
new device be the fs_devices->latest_bdev instead of the existing
older
one?
Thanks.
--
Su
> fs/btrfs/super.c | 25 +++----------------------
> 1 file changed, 3 insertions(+), 22 deletions(-)
>
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 1f9dd1a4faa3..4ad3fe174c41 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -2464,30 +2464,11 @@ static int btrfs_unfreeze(struct
> super_block *sb)
> static int btrfs_show_devname(struct seq_file *m, struct dentry
> *root)
> {
> struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb);
> - struct btrfs_device *dev, *first_dev = NULL;
> + char name[BDEVNAME_SIZE];
>
> - /*
> - * Lightweight locking of the devices. We should not need
> - * device_list_mutex here as we only read the device data and
> the list
> - * is protected by RCU. Even if a device is deleted during
> the list
> - * traversals, we'll get valid data, the freeing callback will
> wait at
> - * least until the rcu_read_unlock.
> - */
> - rcu_read_lock();
> - list_for_each_entry_rcu(dev, &fs_info->fs_devices->devices,
> dev_list) {
> - if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state))
> - continue;
> - if (!dev->name)
> - continue;
> - if (!first_dev || dev->devid < first_dev->devid)
> - first_dev = dev;
> - }
> + seq_escape(m, bdevname(fs_info->fs_devices->latest_bdev,
> name),
> + " \t\n\\");
>
> - if (first_dev)
> - seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\");
> - else
> - WARN_ON(1);
> - rcu_read_unlock();
> return 0;
> }
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent
2021-08-19 18:18 ` [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent Anand Jain
@ 2021-08-20 7:51 ` Su Yue
2021-08-20 8:53 ` Anand Jain
0 siblings, 1 reply; 15+ messages in thread
From: Su Yue @ 2021-08-20 7:51 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, dsterba
On Fri 20 Aug 2021 at 02:18, Anand Jain <anand.jain@oracle.com>
wrote:
> btrfs_prepare_sprout() moves seed devices into its own struct
> fs_devices,
> so that its parent function btrfs_init_new_device() can add the
> new sprout
> device to fs_info->fs_devices.
>
> Both btrfs_prepare_sprout() and btrfs_init_new_device() needs
> device_list_mutex. But they are holding it sequentially, thus
> creates a
> small window to an opportunity to race. Close this opportunity
> and hold
> device_list_mutex common to both btrfs_init_new_device() and
> btrfs_prepare_sprout().
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> RFC because I haven't identified the other thread which could
> race with
> this, but still does this cleanup makes sense?
>
> fs/btrfs/volumes.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 91b8422b3f67..f490d1897c56 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct
> btrfs_fs_info *fs_info)
> u64 super_flags;
>
> lockdep_assert_held(&uuid_mutex);
> + lockdep_assert_held(&fs_devices->device_list_mutex);
> +
>
Just a reminder: clone_fs_devices() still takes the mutex in
misc-next.
> if (!fs_devices->seeding)
> return -EINVAL;
>
> @@ -2397,7 +2399,6 @@ static int btrfs_prepare_sprout(struct
> btrfs_fs_info *fs_info)
> INIT_LIST_HEAD(&seed_devices->alloc_list);
> mutex_init(&seed_devices->device_list_mutex);
>
> - mutex_lock(&fs_devices->device_list_mutex);
> list_splice_init_rcu(&fs_devices->devices,
> &seed_devices->devices,
> synchronize_rcu);
> list_for_each_entry(device, &seed_devices->devices, dev_list)
> @@ -2413,7 +2414,6 @@ static int btrfs_prepare_sprout(struct
> btrfs_fs_info *fs_info)
> generate_random_uuid(fs_devices->fsid);
> memcpy(fs_devices->metadata_uuid, fs_devices->fsid,
> BTRFS_FSID_SIZE);
> memcpy(disk_super->fsid, fs_devices->fsid, BTRFS_FSID_SIZE);
> - mutex_unlock(&fs_devices->device_list_mutex);
>
> super_flags = btrfs_super_flags(disk_super) &
> ~BTRFS_SUPER_FLAG_SEEDING;
> @@ -2588,6 +2588,7 @@ int btrfs_init_new_device(struct
> btrfs_fs_info *fs_info, const char *device_path
> device->dev_stats_valid = 1;
> set_blocksize(device->bdev, BTRFS_BDEV_BLOCKSIZE);
>
> + mutex_lock(&fs_devices->device_list_mutex);
> if (seeding_dev) {
> btrfs_clear_sb_rdonly(sb);
> ret = btrfs_prepare_sprout(fs_info);
>
the erorr case:
if (ret) {
mutex_unlock(&fs_devices->device_list_mutex);
...
}
Thanks.
--
Su
> @@ -2599,7 +2600,6 @@ int btrfs_init_new_device(struct
> btrfs_fs_info *fs_info, const char *device_path
>
> device->fs_devices = fs_devices;
>
> - mutex_lock(&fs_devices->device_list_mutex);
> mutex_lock(&fs_info->chunk_mutex);
> list_add_rcu(&device->dev_list, &fs_devices->devices);
> list_add(&device->dev_alloc_list, &fs_devices->alloc_list);
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent
2021-08-20 7:51 ` Su Yue
@ 2021-08-20 8:53 ` Anand Jain
2021-08-21 14:57 ` Su Yue
0 siblings, 1 reply; 15+ messages in thread
From: Anand Jain @ 2021-08-20 8:53 UTC (permalink / raw)
To: Su Yue; +Cc: linux-btrfs, dsterba
>> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct
>> btrfs_fs_info *fs_info)
>> u64 super_flags;
>>
>> lockdep_assert_held(&uuid_mutex);
>> + lockdep_assert_held(&fs_devices->device_list_mutex);
>> +
>>
> Just a reminder: clone_fs_devices() still takes the mutex in misc-next.
As I am checking clone_fs_devices() does not take any lock.
Could you pls recheck?
>> @@ -2588,6 +2588,7 @@ int btrfs_init_new_device(struct btrfs_fs_info
>> *fs_info, const char *device_path
>> device->dev_stats_valid = 1;
>> set_blocksize(device->bdev, BTRFS_BDEV_BLOCKSIZE);
>>
>> + mutex_lock(&fs_devices->device_list_mutex);
>> if (seeding_dev) {
>> btrfs_clear_sb_rdonly(sb);
>> ret = btrfs_prepare_sprout(fs_info);
>>
> the erorr case:
> if (ret) {
> mutex_unlock(&fs_devices->device_list_mutex);
> ...
> }
Right. I missed it will fix.
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname
2021-08-20 7:31 ` Su Yue
@ 2021-08-20 9:13 ` Anand Jain
0 siblings, 0 replies; 15+ messages in thread
From: Anand Jain @ 2021-08-20 9:13 UTC (permalink / raw)
To: Su Yue; +Cc: linux-btrfs, dsterba
On 20/08/2021 15:31, Su Yue wrote:
>
> On Fri 20 Aug 2021 at 02:18, Anand Jain <anand.jain@oracle.com> wrote:
>
>> latest_bdev is updated according to the changes to the device list.
>> That means we could use the latest_bdev to show the device name in
>> /proc/self/mounts. So this patch makes that change.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>
>> RFC because
>> 1. latest_bdev might not be the lowest devid but, we showed
>> the lowest devid in /proc/self/mount.
>> 2. The device's path is not shown now but, previously we did.
>> So does these break ABI? Maybe yes for 2 howabout for 1 above?
In v2 I am fix the path part. By replacing latest_bdev to latest_dev
which means it shall hold the pointer to btrfs_device instead of
just to its bdev.
> Mabybe a naive question but I have no time to dive into btrfs code
> recently. If a device which has highest devid was replaced, will the
> new device be the fs_devices->latest_bdev instead of the existing older
> one?
It is handled by btrfs_assign_next_active_device().
If the replace source device is also the latest_dev then after replace
it shall point to the replace target device. So the new device path is
seen in /proc/self/mounts. Which is fine and normal to expect.
Similarly btrfs_assign_next_active_device() is called during remove
device as well.
There is a bug that we didn't update the latest_bdev when we sprout
I am fixing this as well in v2.
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname
2021-08-19 18:18 ` [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname Anand Jain
2021-08-20 7:31 ` Su Yue
@ 2021-08-20 10:57 ` David Sterba
2021-08-20 11:03 ` Anand Jain
1 sibling, 1 reply; 15+ messages in thread
From: David Sterba @ 2021-08-20 10:57 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, dsterba, l
On Fri, Aug 20, 2021 at 02:18:14AM +0800, Anand Jain wrote:
> latest_bdev is updated according to the changes to the device list.
> That means we could use the latest_bdev to show the device name in
> /proc/self/mounts. So this patch makes that change.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>
> RFC because
> 1. latest_bdev might not be the lowest devid but, we showed
> the lowest devid in /proc/self/mount.
> 2. The device's path is not shown now but, previously we did.
> So does these break ABI? Maybe yes for 2 howabout for 1 above?
The path needs to be preserved, that would break a lot of things..
> fs/btrfs/super.c | 25 +++----------------------
> 1 file changed, 3 insertions(+), 22 deletions(-)
>
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 1f9dd1a4faa3..4ad3fe174c41 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -2464,30 +2464,11 @@ static int btrfs_unfreeze(struct super_block *sb)
> static int btrfs_show_devname(struct seq_file *m, struct dentry *root)
> {
> struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb);
> - struct btrfs_device *dev, *first_dev = NULL;
> + char name[BDEVNAME_SIZE];
>
> - /*
> - * Lightweight locking of the devices. We should not need
> - * device_list_mutex here as we only read the device data and the list
> - * is protected by RCU. Even if a device is deleted during the list
> - * traversals, we'll get valid data, the freeing callback will wait at
> - * least until the rcu_read_unlock.
> - */
> - rcu_read_lock();
> - list_for_each_entry_rcu(dev, &fs_info->fs_devices->devices, dev_list) {
> - if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state))
> - continue;
> - if (!dev->name)
> - continue;
> - if (!first_dev || dev->devid < first_dev->devid)
> - first_dev = dev;
> - }
> + seq_escape(m, bdevname(fs_info->fs_devices->latest_bdev, name),
> + " \t\n\\");
No protection at all? So what if latest_bdev or latest_dev gets updated
in parallel with devicre remove and there's a window where the pointer
is invalid but still is accessed. The whole point of RCU section here is
to prevent that.
>
> - if (first_dev)
> - seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\");
> - else
> - WARN_ON(1);
> - rcu_read_unlock();
> return 0;
> }
>
> --
> 2.31.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname
2021-08-20 10:57 ` David Sterba
@ 2021-08-20 11:03 ` Anand Jain
0 siblings, 0 replies; 15+ messages in thread
From: Anand Jain @ 2021-08-20 11:03 UTC (permalink / raw)
To: dsterba, linux-btrfs, dsterba, l
On 20/08/2021 18:57, David Sterba wrote:
> On Fri, Aug 20, 2021 at 02:18:14AM +0800, Anand Jain wrote:
>> latest_bdev is updated according to the changes to the device list.
>> That means we could use the latest_bdev to show the device name in
>> /proc/self/mounts. So this patch makes that change.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>
>> RFC because
>> 1. latest_bdev might not be the lowest devid but, we showed
>> the lowest devid in /proc/self/mount.
>> 2. The device's path is not shown now but, previously we did.
>> So does these break ABI? Maybe yes for 2 howabout for 1 above?
>
> The path needs to be preserved, that would break a lot of things..
v2 fixed it.
>
>> fs/btrfs/super.c | 25 +++----------------------
>> 1 file changed, 3 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 1f9dd1a4faa3..4ad3fe174c41 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -2464,30 +2464,11 @@ static int btrfs_unfreeze(struct super_block *sb)
>> static int btrfs_show_devname(struct seq_file *m, struct dentry *root)
>> {
>> struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb);
>> - struct btrfs_device *dev, *first_dev = NULL;
>> + char name[BDEVNAME_SIZE];
>>
>> - /*
>> - * Lightweight locking of the devices. We should not need
>> - * device_list_mutex here as we only read the device data and the list
>> - * is protected by RCU. Even if a device is deleted during the list
>> - * traversals, we'll get valid data, the freeing callback will wait at
>> - * least until the rcu_read_unlock.
>> - */
>> - rcu_read_lock();
>> - list_for_each_entry_rcu(dev, &fs_info->fs_devices->devices, dev_list) {
>> - if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state))
>> - continue;
>> - if (!dev->name)
>> - continue;
>> - if (!first_dev || dev->devid < first_dev->devid)
>> - first_dev = dev;
>> - }
>> + seq_escape(m, bdevname(fs_info->fs_devices->latest_bdev, name),
>> + " \t\n\\");
>
> No protection at all? So what if latest_bdev or latest_dev gets updated
> in parallel with devicre remove and there's a window where the pointer
> is invalid but still is accessed. The whole point of RCU section here is
> to prevent that.
>
YEsh. I noticed it. I am just on it. Will fix in v3.
>>
>> - if (first_dev)
>> - seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\");
>> - else
>> - WARN_ON(1);
>> - rcu_read_unlock();
>> return 0;
>> }
>>
>> --
>> 2.31.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent
2021-08-20 8:53 ` Anand Jain
@ 2021-08-21 14:57 ` Su Yue
2021-08-21 15:00 ` Su Yue
0 siblings, 1 reply; 15+ messages in thread
From: Su Yue @ 2021-08-21 14:57 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, dsterba
On Fri 20 Aug 2021 at 16:53, Anand Jain <anand.jain@oracle.com>
wrote:
>>> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct
>>> btrfs_fs_info *fs_info)
>>> u64 super_flags;
>>>
>>> lockdep_assert_held(&uuid_mutex);
>>> + lockdep_assert_held(&fs_devices->device_list_mutex);
>>> +
>>>
>> Just a reminder: clone_fs_devices() still takes the mutex in
>> misc-next.
> As I am checking clone_fs_devices() does not take any lock.
> Could you pls recheck?
>
Hmmmm... misc-next:
https://github.com/kdave/btrfs-devel/blob/e05983195f31374ad51a0f3712efec381397f3cb/fs/btrfs/volumes.c#L381
--
Su
>
>>> @@ -2588,6 +2588,7 @@ int btrfs_init_new_device(struct
>>> btrfs_fs_info
>>> *fs_info, const char *device_path
>>> device->dev_stats_valid = 1;
>>> set_blocksize(device->bdev, BTRFS_BDEV_BLOCKSIZE);
>>>
>>> + mutex_lock(&fs_devices->device_list_mutex);
>>> if (seeding_dev) {
>>> btrfs_clear_sb_rdonly(sb);
>>> ret = btrfs_prepare_sprout(fs_info);
>>>
>> the erorr case:
>> if (ret) {
>> mutex_unlock(&fs_devices->device_list_mutex);
>> ...
>> }
>
> Right. I missed it will fix.
>
> Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent
2021-08-21 14:57 ` Su Yue
@ 2021-08-21 15:00 ` Su Yue
2021-08-23 10:34 ` Anand Jain
0 siblings, 1 reply; 15+ messages in thread
From: Su Yue @ 2021-08-21 15:00 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs, dsterba
On Sat 21 Aug 2021 at 22:57, Su Yue <l@damenly.su> wrote:
> On Fri 20 Aug 2021 at 16:53, Anand Jain <anand.jain@oracle.com>
> wrote:
>
>>>> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct
>>>> btrfs_fs_info *fs_info)
>>>> u64 super_flags;
>>>>
>>>> lockdep_assert_held(&uuid_mutex);
>>>> + lockdep_assert_held(&fs_devices->device_list_mutex);
>>>> +
>>>>
>>> Just a reminder: clone_fs_devices() still takes the mutex in
>>> misc-next.
>> As I am checking clone_fs_devices() does not take any lock.
>> Could you pls recheck?
>>
>
> Hmmmm... misc-next:
>
> https://github.com/kdave/btrfs-devel/blob/e05983195f31374ad51a0f3712efec381397f3cb/fs/btrfs/volumes.c#L381
Sorry, it should be
https://github.com/kdave/btrfs-devel/blob/misc-next/fs/btrfs/volumes.c#L995
--
Su
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent
2021-08-21 15:00 ` Su Yue
@ 2021-08-23 10:34 ` Anand Jain
2021-08-23 10:54 ` Anand Jain
0 siblings, 1 reply; 15+ messages in thread
From: Anand Jain @ 2021-08-23 10:34 UTC (permalink / raw)
To: Su Yue; +Cc: linux-btrfs, dsterba
On 21/08/2021 23:00, Su Yue wrote:
>
> On Sat 21 Aug 2021 at 22:57, Su Yue <l@damenly.su> wrote:
>
>> On Fri 20 Aug 2021 at 16:53, Anand Jain <anand.jain@oracle.com>
>> wrote:
>>
>>>>> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct
>>>>> btrfs_fs_info *fs_info)
>>>>> u64 super_flags;
>>>>>
>>>>> lockdep_assert_held(&uuid_mutex);
>>>>> + lockdep_assert_held(&fs_devices->device_list_mutex);
>>>>> +
>>>>>
>>>> Just a reminder: clone_fs_devices() still takes the mutex in
>>>> misc-next.
>>> As I am checking clone_fs_devices() does not take any lock.
>>> Could you pls recheck?
>>>
>>
>> Hmmmm... misc-next:
>>
>> https://github.com/kdave/btrfs-devel/blob/e05983195f31374ad51a0f3712efec381397f3cb/fs/btrfs/volumes.c#L381
>>
>
> Sorry, it should be
>
> https://github.com/kdave/btrfs-devel/blob/misc-next/fs/btrfs/volumes.c#L995
Some of the Git commands stopped working. I had to run git fsck.
Now I see mutex in close_fs_devices(), not sure if I was blind to it
before.
Anyway, this is a bad patch. I am working to fix it.
Thanks, Anand
>
> --
> Su
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent
2021-08-23 10:34 ` Anand Jain
@ 2021-08-23 10:54 ` Anand Jain
2021-08-23 12:20 ` David Sterba
0 siblings, 1 reply; 15+ messages in thread
From: Anand Jain @ 2021-08-23 10:54 UTC (permalink / raw)
To: Su Yue; +Cc: linux-btrfs, dsterba
On 23/08/2021 18:34, Anand Jain wrote:
> On 21/08/2021 23:00, Su Yue wrote:
>>
>> On Sat 21 Aug 2021 at 22:57, Su Yue <l@damenly.su> wrote:
>>
>>> On Fri 20 Aug 2021 at 16:53, Anand Jain <anand.jain@oracle.com>
>>> wrote:
>>>
>>>>>> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct
>>>>>> btrfs_fs_info *fs_info)
>>>>>> u64 super_flags;
>>>>>>
>>>>>> lockdep_assert_held(&uuid_mutex);
>>>>>> + lockdep_assert_held(&fs_devices->device_list_mutex);
>>>>>> +
>>>>>>
>>>>> Just a reminder: clone_fs_devices() still takes the mutex in
>>>>> misc-next.
>>>> As I am checking clone_fs_devices() does not take any lock.
>>>> Could you pls recheck?
>>>>
>>>
>>> Hmmmm... misc-next:
>>>
>>> https://github.com/kdave/btrfs-devel/blob/e05983195f31374ad51a0f3712efec381397f3cb/fs/btrfs/volumes.c#L381
>>>
>>
>> Sorry, it should be
>>
>> https://github.com/kdave/btrfs-devel/blob/misc-next/fs/btrfs/volumes.c#L995
>>
>
>
> Some of the Git commands stopped working. I had to run git fsck.
> Now I see mutex in close_fs_devices(), not sure if I was blind to it
> before.
> Anyway, this is a bad patch. I am working to fix it.
>
Git errors were distracting. But why I didn't see the
device_list_mutex is because I had the following patch [1] in my
workspace, which is not yet integrated to the misc-next / commented.
[1] btrfs: fix lockdep warning while mounting sprout fs
https://lore.kernel.org/linux-btrfs/8325002d-f8a6-a9b7-a27d-4cf62cbbc672@oracle.com/
So as of now, I can say this patch needs the above patch.
Could you please apply the above patch before this patch for tests?
Thanks, Anand
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent
2021-08-23 10:54 ` Anand Jain
@ 2021-08-23 12:20 ` David Sterba
0 siblings, 0 replies; 15+ messages in thread
From: David Sterba @ 2021-08-23 12:20 UTC (permalink / raw)
To: Anand Jain; +Cc: Su Yue, linux-btrfs, dsterba
On Mon, Aug 23, 2021 at 06:54:04PM +0800, Anand Jain wrote:
>
>
> On 23/08/2021 18:34, Anand Jain wrote:
> > On 21/08/2021 23:00, Su Yue wrote:
> >>
> >> On Sat 21 Aug 2021 at 22:57, Su Yue <l@damenly.su> wrote:
> >>
> >>> On Fri 20 Aug 2021 at 16:53, Anand Jain <anand.jain@oracle.com>
> >>> wrote:
> >>>
> >>>>>> @@ -2366,6 +2366,8 @@ static int btrfs_prepare_sprout(struct
> >>>>>> btrfs_fs_info *fs_info)
> >>>>>> u64 super_flags;
> >>>>>>
> >>>>>> lockdep_assert_held(&uuid_mutex);
> >>>>>> + lockdep_assert_held(&fs_devices->device_list_mutex);
> >>>>>> +
> >>>>>>
> >>>>> Just a reminder: clone_fs_devices() still takes the mutex in
> >>>>> misc-next.
> >>>> As I am checking clone_fs_devices() does not take any lock.
> >>>> Could you pls recheck?
> >>>>
> >>>
> >>> Hmmmm... misc-next:
> >>>
> >>> https://github.com/kdave/btrfs-devel/blob/e05983195f31374ad51a0f3712efec381397f3cb/fs/btrfs/volumes.c#L381
> >>>
> >>
> >> Sorry, it should be
> >>
> >> https://github.com/kdave/btrfs-devel/blob/misc-next/fs/btrfs/volumes.c#L995
> >>
> >
> >
> > Some of the Git commands stopped working. I had to run git fsck.
> > Now I see mutex in close_fs_devices(), not sure if I was blind to it
> > before.
> > Anyway, this is a bad patch. I am working to fix it.
> >
>
> Git errors were distracting. But why I didn't see the
> device_list_mutex is because I had the following patch [1] in my
> workspace, which is not yet integrated to the misc-next / commented.
>
> [1] btrfs: fix lockdep warning while mounting sprout fs
>
> https://lore.kernel.org/linux-btrfs/8325002d-f8a6-a9b7-a27d-4cf62cbbc672@oracle.com/
>
> So as of now, I can say this patch needs the above patch.
>
> Could you please apply the above patch before this patch for tests?
So if there's a dependency I'd apply the whole series.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-08-23 12:23 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-19 18:18 [PATCH 0/3] btrf_show_devname related fixes Anand Jain
2021-08-19 18:18 ` [PATCH 1/3] btrfs: fix comment about the btrfs_show_devname Anand Jain
2021-08-19 18:18 ` [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent Anand Jain
2021-08-20 7:51 ` Su Yue
2021-08-20 8:53 ` Anand Jain
2021-08-21 14:57 ` Su Yue
2021-08-21 15:00 ` Su Yue
2021-08-23 10:34 ` Anand Jain
2021-08-23 10:54 ` Anand Jain
2021-08-23 12:20 ` David Sterba
2021-08-19 18:18 ` [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname Anand Jain
2021-08-20 7:31 ` Su Yue
2021-08-20 9:13 ` Anand Jain
2021-08-20 10:57 ` David Sterba
2021-08-20 11:03 ` Anand Jain
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.