[PATCH V5 0/4] btrf_show_devname related fixes

[PATCH V5 0/4] btrf_show_devname related fixes

[Anand Jain]

v5: Patches reorged.
 Patch (btrfs: consolidate device_list_mutex in prepare_sprout to its parent)
 moved into a new set as it has a dependency on an older patch in the ML.
 Change log updated.
v4: Fix unrelated changes
v3: Add missing rcu_lock in show_devname
v2: Use latest_dev so that device path is also shown

Su Yue reported [1] warn() as a result of a race between the following two
threads,
  Thread-A: function stack leading to btrfs_prepare_sprout()
and
  Thread-B: function stack leading to btrfs_show_devname()

[1]  https://patchwork.kernel.org/project/linux-btrfs/patch/20210818041944.5793-1-l@damenly.su/

While btrfs_prepare_sprout() moves the fs_devices::devices into
fs_devices::seed_list, the btrfs_show_devname searched for the devices
and found none, leading to the warning as in [1] (above).

The btrfs_prepare_sprout() uses device_list_mutex however
btrfs_show_devname() don't and, the device_list_mutex in
btrfs_show_devname() was removed by the patch 88c14590cdd6
(btrfs: use RCU in btrfs_show_devname for device list traversal)
for the perforamcne reasons.

This series does not intend to reintroduce the device_list_mutex in
btrfs_prepare_sprout() but instead saves the pointer to btrfs_devices
in the fs_devices::latest_dev so that btrfs_show_devname() can use it.

patch 1 converts fs_devices::latest_bdev type from struct block_device to
        struct btrfs_device and renames it to latest_dev
patch 2 btrfs_show_devname() uses the fs_devices::latest_dev::name to show
        the device path in the /proc/self/mounts
patch 3 fixes a stale latest_dev pointer after the sprout operation
patch 4 fixes an old comment about the function btrfs_show_devname()

Anand Jain (4):
  btrfs: convert latest_bdev type to struct btrfs_device and rename
  btrfs: use latest_dev in btrfs_show_devname
  btrfs: update latest_dev when we sprout
  btrfs: fix comment about the btrfs_show_devname

 fs/btrfs/disk-io.c   |  6 +++---
 fs/btrfs/extent_io.c |  2 +-
 fs/btrfs/inode.c     |  2 +-
 fs/btrfs/super.c     | 26 ++++----------------------
 fs/btrfs/volumes.c   | 17 ++++++++---------
 fs/btrfs/volumes.h   |  2 +-
 6 files changed, 18 insertions(+), 37 deletions(-)

-- 
2.31.1

[PATCH V5 1/4] btrfs: convert latest_bdev type to struct btrfs_device and rename

[Anand Jain]

In preparation to fix a bug in btrfs_show_devname().

Convert fs_devices::latest_bdev type from struct block_device to struct
btrfs_device and, rename the member to fs_devices::latest_dev.
So that btrfs_show_devname() can use fs_devices::latest_dev::name.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/disk-io.c   |  6 +++---
 fs/btrfs/extent_io.c |  2 +-
 fs/btrfs/inode.c     |  2 +-
 fs/btrfs/super.c     |  2 +-
 fs/btrfs/volumes.c   | 10 +++++-----
 fs/btrfs/volumes.h   |  2 +-
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 1052437cec64..c0d2c093b874 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3228,12 +3228,12 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	mapping_set_gfp_mask(fs_info->btree_inode->i_mapping, GFP_NOFS);
 	btrfs_init_btree_inode(fs_info);
 
-	invalidate_bdev(fs_devices->latest_bdev);
+	invalidate_bdev(fs_devices->latest_dev->bdev);
 
 	/*
 	 * Read super block and check the signature bytes only
 	 */
-	disk_super = btrfs_read_dev_super(fs_devices->latest_bdev);
+	disk_super = btrfs_read_dev_super(fs_devices->latest_dev->bdev);
 	if (IS_ERR(disk_super)) {
 		err = PTR_ERR(disk_super);
 		goto fail_alloc;
@@ -3466,7 +3466,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	 * below in btrfs_init_dev_replace().
 	 */
 	btrfs_free_extra_devids(fs_devices);
-	if (!fs_devices->latest_bdev) {
+	if (!fs_devices->latest_dev->bdev) {
 		btrfs_err(fs_info, "failed to read devices");
 		goto fail_tree_roots;
 	}
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index aaddd7225348..edf0162c9020 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3327,7 +3327,7 @@ static int alloc_new_bio(struct btrfs_inode *inode,
 	if (wbc) {
 		struct block_device *bdev;
 
-		bdev = fs_info->fs_devices->latest_bdev;
+		bdev = fs_info->fs_devices->latest_dev->bdev;
 		bio_set_dev(bio, bdev);
 		wbc_init_bio(wbc, bio);
 	}
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 2aa9646bce56..ceedcd54e6d2 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7961,7 +7961,7 @@ static int btrfs_dio_iomap_begin(struct inode *inode, loff_t start,
 		iomap->type = IOMAP_MAPPED;
 	}
 	iomap->offset = start;
-	iomap->bdev = fs_info->fs_devices->latest_bdev;
+	iomap->bdev = fs_info->fs_devices->latest_dev->bdev;
 	iomap->length = len;
 
 	if (write && btrfs_use_zone_append(BTRFS_I(inode), em->block_start))
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 1f9dd1a4faa3..64ecbdb50c1a 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1706,7 +1706,7 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 		goto error_close_devices;
 	}
 
-	bdev = fs_devices->latest_bdev;
+	bdev = fs_devices->latest_dev->bdev;
 	s = sget(fs_type, btrfs_test_super, btrfs_set_super, flags | SB_NOSEC,
 		 fs_info);
 	if (IS_ERR(s)) {
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 55f0a82ff5d7..ae317b0c39a3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1091,7 +1091,7 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices)
 	list_for_each_entry(seed_dev, &fs_devices->seed_list, seed_list)
 		__btrfs_free_extra_devids(seed_dev, &latest_dev);
 
-	fs_devices->latest_bdev = latest_dev->bdev;
+	fs_devices->latest_dev = latest_dev;
 
 	mutex_unlock(&uuid_mutex);
 }
@@ -1206,7 +1206,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
 		return -EINVAL;
 
 	fs_devices->opened = 1;
-	fs_devices->latest_bdev = latest_dev->bdev;
+	fs_devices->latest_dev = latest_dev;
 	fs_devices->total_rw_bytes = 0;
 	fs_devices->chunk_alloc_policy = BTRFS_CHUNK_ALLOC_REGULAR;
 	fs_devices->read_policy = BTRFS_READ_POLICY_PID;
@@ -1968,7 +1968,7 @@ static struct btrfs_device * btrfs_find_next_active_device(
 }
 
 /*
- * Helper function to check if the given device is part of s_bdev / latest_bdev
+ * Helper function to check if the given device is part of s_bdev / latest_dev
  * and replace it with the provided or the next active device, in the context
  * where this function called, there should be always be another device (or
  * this_dev) which is active.
@@ -1987,8 +1987,8 @@ void __cold btrfs_assign_next_active_device(struct btrfs_device *device,
 			(fs_info->sb->s_bdev == device->bdev))
 		fs_info->sb->s_bdev = next_device->bdev;
 
-	if (fs_info->fs_devices->latest_bdev == device->bdev)
-		fs_info->fs_devices->latest_bdev = next_device->bdev;
+	if (fs_info->fs_devices->latest_dev->bdev == device->bdev)
+		fs_info->fs_devices->latest_dev = next_device;
 }
 
 /*
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 4c941b4dd269..150b4cd8f81f 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -246,7 +246,7 @@ struct btrfs_fs_devices {
 	/* Highest generation number of seen devices */
 	u64 latest_generation;
 
-	struct block_device *latest_bdev;
+	struct btrfs_device *latest_dev;
 
 	/* all of the devices in the FS, protected by a mutex
 	 * so we can safely walk it to write out the supers without
-- 
2.31.1

[PATCH RFC V5 2/4] btrfs: use latest_dev in btrfs_show_devname

[Anand Jain]

btrfs/238 reports warning as below.
[1]
------------[ cut here  ]------------
WARNING: CPU: 3 PID: 481 at fs/btrfs/super.c:2509 btrfs_show_devname+0x104/0x1e8 [btrfs]
CPU: 2 PID: 1 Comm: systemd Tainted: G        W  O 5.14.0-rc1-custom #72
Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
Call trace:
  btrfs_show_devname+0x108/0x1b4 [btrfs]
  show_mountinfo+0x234/0x2c4
  m_show+0x28/0x34
  seq_read_iter+0x12c/0x3c4
  vfs_read+0x29c/0x2c8
  ksys_read+0x80/0xec
  __arm64_sys_read+0x28/0x34
  invoke_syscall+0x50/0xf8
  do_el0_svc+0x88/0x138
  el0_svc+0x2c/0x8c
  el0t_64_sync_handler+0x84/0xe4
  el0t_64_sync+0x198/0x19c
---[ end trace 3efd7e5950b8af05  ]---

Reason:
While btrfs_prepare_sprout() moves the fs_devices::devices into
fs_devices::seed_list, the btrfs_show_devname() searched for the devices
and found none, leading to the warning as in [1] (above).

Fix:
latest_dev is updated according to the changes to the device list.
That means we could use the latest_dev->name to show the device name in
/proc/self/mounts. So this patch makes that change.

Reported-by: Su Yue <l@damenly.su>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
RFC because,
With this patch, /proc/self/mounts might not show the lowest devid
device as we did before. Instead we show the device that has the greatest
generation and, we used it to build the tree. IMO it is ok because
/proc/self/mounts should show a device the belongs to the fsid not,
necessarily the lowest devid device as devid is internal to btrfs.
IMO this won't affect the ABI?

 fs/btrfs/super.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 64ecbdb50c1a..61682a143bf3 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2464,30 +2464,12 @@ 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;
 
-	/*
-	 * 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;
-	}
-
-	if (first_dev)
-		seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\");
-	else
-		WARN_ON(1);
+	seq_escape(m, rcu_str_deref(fs_info->fs_devices->latest_dev->name),
+		   " \t\n\\");
 	rcu_read_unlock();
+
 	return 0;
 }
 
-- 
2.31.1

[PATCH V5 3/4] btrfs: update latest_dev when we sprout

[Anand Jain]

When we add a device to the seed filesystem (sprouting) it is a new
filesystem (and fsid) on the device added. Update the latest_device so
that /proc/self/mounts shows the correct device.

For example:
 $ btrfstune -S1 /dev/vg/scratch1
 $ mount /dev/vg/scratch1 /btrfs
 mount: /btrfs: WARNING: device write-protected, mounted read-only.

 $ cat /proc/self/mounts | grep btrfs
 /dev/mapper/vg-scratch1 /btrfs btrfs ro,relatime,space_cache,subvolid=5,subvol=/ 0 0

 $ btrfs dev add -f /dev/vg/scratch0 /btrfs

Before:
 $ cat /proc/self/mounts | grep btrfs
 /dev/mapper/vg-scratch1 /btrfs btrfs ro,relatime,space_cache,subvolid=5,subvol=/ 0 0

After:
 $ cat /proc/self/mounts | grep btrfs
 /dev/mapper/vg-scratch0 /btrfs btrfs ro,relatime,space_cache,subvolid=5,subvol=/ 0 0

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ae317b0c39a3..8470c5b5f35e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2600,6 +2600,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 			btrfs_abort_transaction(trans, ret);
 			goto error_trans;
 		}
+		btrfs_assign_next_active_device(fs_info->fs_devices->latest_dev,
+						device);
 	}
 
 	device->fs_devices = fs_devices;
-- 
2.31.1

[PATCH V5 4/4] 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 8470c5b5f35e..1d1204547e72 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

Re: [PATCH V5 0/4] btrf_show_devname related fixes

[Anand Jain]

Ping.

This patchset is fixing a bug reported by btrfs/238.

Here I am proposing to remove traversing the device list in 
btrfs_show_devname(), instead use device->name as stored in 
fs_devices::latest_dev.

Kindly review/comment.

Thx, Anand



On 24/08/2021 13:05, Anand Jain wrote:
> v5: Patches reorged.
>   Patch (btrfs: consolidate device_list_mutex in prepare_sprout to its parent)
>   moved into a new set as it has a dependency on an older patch in the ML.
>   Change log updated.
> v4: Fix unrelated changes
> v3: Add missing rcu_lock in show_devname
> v2: Use latest_dev so that device path is also shown
> 
> Su Yue reported [1] warn() as a result of a race between the following two
> threads,
>    Thread-A: function stack leading to btrfs_prepare_sprout()
> and
>    Thread-B: function stack leading to btrfs_show_devname()
> 
> [1]  https://patchwork.kernel.org/project/linux-btrfs/patch/20210818041944.5793-1-l@damenly.su/
> 
> While btrfs_prepare_sprout() moves the fs_devices::devices into
> fs_devices::seed_list, the btrfs_show_devname searched for the devices
> and found none, leading to the warning as in [1] (above).
> 
> The btrfs_prepare_sprout() uses device_list_mutex however
> btrfs_show_devname() don't and, the device_list_mutex in
> btrfs_show_devname() was removed by the patch 88c14590cdd6
> (btrfs: use RCU in btrfs_show_devname for device list traversal)
> for the perforamcne reasons.
> 
> This series does not intend to reintroduce the device_list_mutex in
> btrfs_prepare_sprout() but instead saves the pointer to btrfs_devices
> in the fs_devices::latest_dev so that btrfs_show_devname() can use it.
> 
> patch 1 converts fs_devices::latest_bdev type from struct block_device to
>          struct btrfs_device and renames it to latest_dev
> patch 2 btrfs_show_devname() uses the fs_devices::latest_dev::name to show
>          the device path in the /proc/self/mounts
> patch 3 fixes a stale latest_dev pointer after the sprout operation
> patch 4 fixes an old comment about the function btrfs_show_devname()
> 
> Anand Jain (4):
>    btrfs: convert latest_bdev type to struct btrfs_device and rename
>    btrfs: use latest_dev in btrfs_show_devname
>    btrfs: update latest_dev when we sprout
>    btrfs: fix comment about the btrfs_show_devname
> 
>   fs/btrfs/disk-io.c   |  6 +++---
>   fs/btrfs/extent_io.c |  2 +-
>   fs/btrfs/inode.c     |  2 +-
>   fs/btrfs/super.c     | 26 ++++----------------------
>   fs/btrfs/volumes.c   | 17 ++++++++---------
>   fs/btrfs/volumes.h   |  2 +-
>   6 files changed, 18 insertions(+), 37 deletions(-)
> 

Re: [PATCH V5 0/4] btrf_show_devname related fixes

[Su Yue]

On Tue 31 Aug 2021 at 06:41, Anand Jain <anand.jain@oracle.com> 
wrote:

> Ping.
>
In the past week, I spent some time on testing this patchset and
the patch 'btrfs: fix lockdep warning while mounting sprout fs'.

Same as you, I wonder if there is any race condition. Just wrote
some scripts including device removal, replace and sprout while
busy looping showing mount info. It runned well and passed 
xfstests
in my two VMs. I'm not a pundit in btrfs device field so I can't 
give my
RBs even these patches looks almost okay to me.

--
Su

> This patchset is fixing a bug reported by btrfs/238.
>
> Here I am proposing to remove traversing the device list in
> btrfs_show_devname(), instead use device->name as stored in
> fs_devices::latest_dev.
>
> Kindly review/comment.
>
> Thx, Anand
>
>
>
> On 24/08/2021 13:05, Anand Jain wrote:
>> v5: Patches reorged.
>>   Patch (btrfs: consolidate device_list_mutex in prepare_sprout 
>>   to its parent)
>>   moved into a new set as it has a dependency on an older patch 
>>   in the ML.
>>   Change log updated.
>> v4: Fix unrelated changes
>> v3: Add missing rcu_lock in show_devname
>> v2: Use latest_dev so that device path is also shown
>> Su Yue reported [1] warn() as a result of a race between the 
>> following two
>> threads,
>>    Thread-A: function stack leading to btrfs_prepare_sprout()
>> and
>>    Thread-B: function stack leading to btrfs_show_devname()
>> [1]
>> https://patchwork.kernel.org/project/linux-btrfs/patch/20210818041944.5793-1-l@damenly.su/
>> While btrfs_prepare_sprout() moves the fs_devices::devices into
>> fs_devices::seed_list, the btrfs_show_devname searched for the 
>> devices
>> and found none, leading to the warning as in [1] (above).
>> The btrfs_prepare_sprout() uses device_list_mutex however
>> btrfs_show_devname() don't and, the device_list_mutex in
>> btrfs_show_devname() was removed by the patch 88c14590cdd6
>> (btrfs: use RCU in btrfs_show_devname for device list 
>> traversal)
>> for the perforamcne reasons.
>> This series does not intend to reintroduce the 
>> device_list_mutex in
>> btrfs_prepare_sprout() but instead saves the pointer to 
>> btrfs_devices
>> in the fs_devices::latest_dev so that btrfs_show_devname() can 
>> use it.
>> patch 1 converts fs_devices::latest_bdev type from struct 
>> block_device to
>>          struct btrfs_device and renames it to latest_dev
>> patch 2 btrfs_show_devname() uses the 
>> fs_devices::latest_dev::name to show
>>          the device path in the /proc/self/mounts
>> patch 3 fixes a stale latest_dev pointer after the sprout 
>> operation
>> patch 4 fixes an old comment about the function 
>> btrfs_show_devname()
>> Anand Jain (4):
>>    btrfs: convert latest_bdev type to struct btrfs_device and 
>>    rename
>>    btrfs: use latest_dev in btrfs_show_devname
>>    btrfs: update latest_dev when we sprout
>>    btrfs: fix comment about the btrfs_show_devname
>>   fs/btrfs/disk-io.c   |  6 +++---
>>   fs/btrfs/extent_io.c |  2 +-
>>   fs/btrfs/inode.c     |  2 +-
>>   fs/btrfs/super.c     | 26 ++++----------------------
>>   fs/btrfs/volumes.c   | 17 ++++++++---------
>>   fs/btrfs/volumes.h   |  2 +-
>>   6 files changed, 18 insertions(+), 37 deletions(-)
>>

Re: [PATCH V5 0/4] btrf_show_devname related fixes

[David Sterba]

On Tue, Aug 31, 2021 at 10:28:24PM +0800, Su Yue wrote:
> 
> On Tue 31 Aug 2021 at 06:41, Anand Jain <anand.jain@oracle.com> 
> wrote:
> 
> > Ping.
> >
> In the past week, I spent some time on testing this patchset and
> the patch 'btrfs: fix lockdep warning while mounting sprout fs'.
> 
> Same as you, I wonder if there is any race condition. Just wrote
> some scripts including device removal, replace and sprout while
> busy looping showing mount info. It runned well and passed 
> xfstests
> in my two VMs. I'm not a pundit in btrfs device field so I can't 
> give my
> RBs even these patches looks almost okay to me.

So this could be a Tested-by if you want.

Re: [PATCH V5 0/4] btrf_show_devname related fixes

[Su Yue]

On Tue 31 Aug 2021 at 17:41, David Sterba <dsterba@suse.cz> wrote:

> On Tue, Aug 31, 2021 at 10:28:24PM +0800, Su Yue wrote:
>>
>> On Tue 31 Aug 2021 at 06:41, Anand Jain <anand.jain@oracle.com>
>> wrote:
>>
>> > Ping.
>> >
>> In the past week, I spent some time on testing this patchset 
>> and
>> the patch 'btrfs: fix lockdep warning while mounting sprout 
>> fs'.
>>
>> Same as you, I wonder if there is any race condition. Just 
>> wrote
>> some scripts including device removal, replace and sprout while
>> busy looping showing mount info. It runned well and passed
>> xfstests
>> in my two VMs. I'm not a pundit in btrfs device field so I 
>> can't
>> give my
>> RBs even these patches looks almost okay to me.
>
> So this could be a Tested-by if you want.

Sure. Thanks.

--
Su

Re: [PATCH V5 0/4] btrf_show_devname related fixes

[Su Yue]

On Tue 24 Aug 2021 at 13:05, Anand Jain <anand.jain@oracle.com> 
wrote:

> v5: Patches reorged.
>  Patch (btrfs: consolidate device_list_mutex in prepare_sprout 
>  to its parent)
>  moved into a new set as it has a dependency on an older patch 
>  in the ML.
>  Change log updated.
> v4: Fix unrelated changes
> v3: Add missing rcu_lock in show_devname
> v2: Use latest_dev so that device path is also shown
>
> Su Yue reported [1] warn() as a result of a race between the 
> following two
> threads,
>   Thread-A: function stack leading to btrfs_prepare_sprout()
> and
>   Thread-B: function stack leading to btrfs_show_devname()
>
> [1] 
> https://patchwork.kernel.org/project/linux-btrfs/patch/20210818041944.5793-1-l@damenly.su/
>
> While btrfs_prepare_sprout() moves the fs_devices::devices into
> fs_devices::seed_list, the btrfs_show_devname searched for the 
> devices
> and found none, leading to the warning as in [1] (above).
>
> The btrfs_prepare_sprout() uses device_list_mutex however
> btrfs_show_devname() don't and, the device_list_mutex in
> btrfs_show_devname() was removed by the patch 88c14590cdd6
> (btrfs: use RCU in btrfs_show_devname for device list traversal)
> for the perforamcne reasons.
>
> This series does not intend to reintroduce the device_list_mutex 
> in
> btrfs_prepare_sprout() but instead saves the pointer to 
> btrfs_devices
> in the fs_devices::latest_dev so that btrfs_show_devname() can 
> use it.
>
> patch 1 converts fs_devices::latest_bdev type from struct 
> block_device to
>         struct btrfs_device and renames it to latest_dev
> patch 2 btrfs_show_devname() uses the 
> fs_devices::latest_dev::name to show
>         the device path in the /proc/self/mounts
> patch 3 fixes a stale latest_dev pointer after the sprout 
> operation
>

For patch [123], you can add

Tested-by: Su Yue <l@damenly.su>

Thanks.
--
Su
> patch 4 fixes an old comment about the function 
> btrfs_show_devname()
>
> Anand Jain (4):
>   btrfs: convert latest_bdev type to struct btrfs_device and 
>   rename
>   btrfs: use latest_dev in btrfs_show_devname
>   btrfs: update latest_dev when we sprout
>   btrfs: fix comment about the btrfs_show_devname
>
>  fs/btrfs/disk-io.c   |  6 +++---
>  fs/btrfs/extent_io.c |  2 +-
>  fs/btrfs/inode.c     |  2 +-
>  fs/btrfs/super.c     | 26 ++++----------------------
>  fs/btrfs/volumes.c   | 17 ++++++++---------
>  fs/btrfs/volumes.h   |  2 +-
>  6 files changed, 18 insertions(+), 37 deletions(-)

Re: [PATCH RFC V5 2/4] btrfs: use latest_dev in btrfs_show_devname

[David Sterba]

On Tue, Aug 24, 2021 at 01:05:20PM +0800, Anand Jain wrote:
> RFC because,
> With this patch, /proc/self/mounts might not show the lowest devid
> device as we did before. Instead we show the device that has the greatest
> generation and, we used it to build the tree. IMO it is ok because
> /proc/self/mounts should show a device the belongs to the fsid not,
> necessarily the lowest devid device as devid is internal to btrfs.
> IMO this won't affect the ABI?

I tend to agree, the only thing that should be consistent that any
number of mounts of the same filesystem (eg. by subvolume) should print
the same device path. But given that fs_devices is shared then the same
output is guaranteed. The time when the latest_bdev changes is after
remove or replace, that's an intermediate state so the results may vary.

And maybe when printing the device by which the fs was mounted is more
correct, as it may be different from the lowest id and that could
potentially be confusing.  The commit 9c5085c14798 ("Btrfs: implement
->show_devname") adding the show_devname callback even mentions not
showing the mount device as a drawback.  The multi-device fs devices
should be treated equally for the purpose of mount.

Re: [PATCH V5 4/4] btrfs: fix comment about the btrfs_show_devname

[David Sterba]

On Tue, Aug 24, 2021 at 01:05:22PM +0800, Anand Jain wrote:
> 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 8470c5b5f35e..1d1204547e72 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().

I think the whole comment can be deleted, it was there namely for the
device_list_mutex and potential problems with show_devname, but this is
now sorted. What's remaining is that btrfs_scratch_superblocks happens
out of all locks but it's IMO clear from the code that the device is
being now disconnected from all structures and freed.

Re: [PATCH V5 4/4] btrfs: fix comment about the btrfs_show_devname

[David Sterba]

On Tue, Aug 24, 2021 at 01:05:22PM +0800, Anand Jain wrote:
> 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)

Commit ccd05285e7f does not exist in my tree, but the subject matches
0ccd05285e7f. Please format the references like it's done for the Fixes:
tag with HASH ("Subject"). Nobody types that by hand, a simple git alias
does that for free:

.gitconfig:
[alias]
	one = show -s --pretty='format:%h (\"%s\")'

Re: [PATCH V5 0/4] btrf_show_devname related fixes

[David Sterba]

On Tue, Aug 24, 2021 at 01:05:18PM +0800, Anand Jain wrote:
> Su Yue reported [1] warn() as a result of a race between the following two
> threads,
>   Thread-A: function stack leading to btrfs_prepare_sprout()
> and
>   Thread-B: function stack leading to btrfs_show_devname()
> 
> [1]  https://patchwork.kernel.org/project/linux-btrfs/patch/20210818041944.5793-1-l@damenly.su/
> 
> Anand Jain (4):
>   btrfs: convert latest_bdev type to struct btrfs_device and rename
>   btrfs: use latest_dev in btrfs_show_devname
>   btrfs: update latest_dev when we sprout
>   btrfs: fix comment about the btrfs_show_devname

Added to misc-next, with some minor fixups, thanks.