[RFC PATCH] btrfs: do not warn if fs_devices has no device when call btrfs_show_devname

[RFC PATCH] btrfs: do not warn if fs_devices has no device when call btrfs_show_devname

[Su Yue]

while running btrfs/238 in my test box, the following warning occurs
in high chance:

------------[ 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  ]---

It's also reproducible by creating a sprout filesystem and reading
/proc/self/mounts in parallel.

The warning is produced if btrfs_show_devname() can't find any available
device in fs_info->fs_devices->devices which is protected by RCU.
The warning is desirable to exercise there is at least one device in the
mounted filesystem. However, it's not always true for a sprouting fs.

While a new device is being added into fs to be sprouted, call stack is:
 btrfs_ioctl_add_dev
  btrfs_init_new_device
    btrfs_prepare_sprout
      list_splice_init_rcu(&fs_devices->devices, &seed_devices->devices,
      synchronize_rcu);
    list_add_rcu(&device->dev_list, &fs_devices->devices);

Looking at btrfs_prepare_sprout(), every new RCU reader will read a
empty fs_devices->devices once synchronize_rcu() is called.
After commit 4faf55b03823 ("btrfs: don't traverse into the seed devices
in show_devname"), btrfs_show_devname() won't looking into
fs_devices->seed_list even there is no device in fs_devices->devices.

And Since commit 88c14590cdd6 ("btrfs: use RCU in btrfs_show_devname for
device list traversal"), btrfs_show_devname() only uses RCU no heavy
mutex lock for device list traversal. It read an empty
fs_devices->devices and found no device in the list then triggers the
warning. The commit just enlarged the window that the fs device list
could be empty. Even btrfs_show_devname() uses mutex_lock(), there is a
tiny chance of reading an empty devices list between mutex_unlock() in
btrfs_prepare_sprout() and next mutex_lock() in btrfs_init_new_device().

Just remove the WARN_ON(1) if there is no valid device since the least
one device check is not suitable for the one short period of sprouting
filesystem.

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

---
Make it RFC since I wonder if there is any better solution not dropping
the sanity check for normal fs.
---
 fs/btrfs/super.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index d07b18b2b250..34f9b1c930f8 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2503,8 +2503,6 @@ static int btrfs_show_devname(struct seq_file *m, struct dentry *root)
 
 	if (first_dev)
 		seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\");
-	else
-		WARN_ON(1);
 	rcu_read_unlock();
 	return 0;
 }
-- 
2.30.1

Re: [RFC PATCH] btrfs: do not warn if fs_devices has no device when call btrfs_show_devname

[David Sterba]

On Wed, Jul 14, 2021 at 05:30:49PM +0800, Su Yue wrote:
> while running btrfs/238 in my test box, the following warning occurs
> in high chance:
> 
> ------------[ 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  ]---
> 
> It's also reproducible by creating a sprout filesystem and reading
> /proc/self/mounts in parallel.
> 
> The warning is produced if btrfs_show_devname() can't find any available
> device in fs_info->fs_devices->devices which is protected by RCU.
> The warning is desirable to exercise there is at least one device in the
> mounted filesystem. However, it's not always true for a sprouting fs.
> 
> While a new device is being added into fs to be sprouted, call stack is:
>  btrfs_ioctl_add_dev
>   btrfs_init_new_device
>     btrfs_prepare_sprout
>       list_splice_init_rcu(&fs_devices->devices, &seed_devices->devices,
>       synchronize_rcu);
>     list_add_rcu(&device->dev_list, &fs_devices->devices);
> 
> Looking at btrfs_prepare_sprout(), every new RCU reader will read a
> empty fs_devices->devices once synchronize_rcu() is called.
> After commit 4faf55b03823 ("btrfs: don't traverse into the seed devices
> in show_devname"), btrfs_show_devname() won't looking into
> fs_devices->seed_list even there is no device in fs_devices->devices.
> 
> And Since commit 88c14590cdd6 ("btrfs: use RCU in btrfs_show_devname for
> device list traversal"), btrfs_show_devname() only uses RCU no heavy
> mutex lock for device list traversal. It read an empty
> fs_devices->devices and found no device in the list then triggers the
> warning. The commit just enlarged the window that the fs device list
> could be empty. Even btrfs_show_devname() uses mutex_lock(), there is a
> tiny chance of reading an empty devices list between mutex_unlock() in
> btrfs_prepare_sprout() and next mutex_lock() in btrfs_init_new_device().
> 
> Just remove the WARN_ON(1) if there is no valid device since the least
> one device check is not suitable for the one short period of sprouting
> filesystem.
> 
> Signed-off-by: Su Yue <l@damenly.su>
> 
> ---
> Make it RFC since I wonder if there is any better solution not dropping
> the sanity check for normal fs.

We should also print a device name otherwise the format of the line
won't be complete. As the missing device does not happen in a typical
case, we could possibly add some heavy weight checks in case the pointer
is NULL after the RCU lookup.

This could be using the mutex or traversing another seeding device list
but some sort of sensible device name should be printed. Running mkfs
and reading the mountinfo does not have exactly clear semantics from
user POV, reading status when some operation is in progress would either
return the old state or the new state.

Removing the warning makes sense in case we do all we can to find the
device, or in case if this turns out to be unreliable, do one attempt to
read the device and keep the warning.