[PATCH 0/3] btrf_show_devname related fixes

[PATCH 0/3] btrf_show_devname related fixes

[Anand Jain]

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

[PATCH 1/3] btrfs: fix comment about the btrfs_show_devname

[Anand Jain]

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

[PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

[Anand Jain]

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

Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

[Su Yue]

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);

Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

[Anand Jain]

>> @@ -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.

Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

[Su Yue]

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.

Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

[Su Yue]

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

Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

[Anand Jain]

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

Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

[Anand Jain]

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

Re: [PATCH RFC 2/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

[David Sterba]

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.

[PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname

[Anand Jain]

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

Re: [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname

[Su Yue]

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;
>  }

Re: [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname

[Anand Jain]

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.

Re: [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname

[David Sterba]

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

Re: [PATCH RFC 3/3] btrfs: use latest_bdev in btrfs_show_devname

[Anand Jain]

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