Re: [PATCH] btrfs: do not skip re-registration for the mounted device

[David Sterba <dsterba@suse.cz>]

On Mon, Feb 05, 2024 at 07:45:05PM +0800, Anand Jain wrote:
> We skip device registration for a single device. However, we do not do
> that if the device is already mounted, as it might be coming in again
> for scanning a different path.
> 
> This patch is lightly tested; for verification if it fixes.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> I still have some unknowns about the problem. Pls test if this fixes
> the problem.
> 
>  fs/btrfs/volumes.c | 44 ++++++++++++++++++++++++++++++++++----------
>  fs/btrfs/volumes.h |  1 -
>  2 files changed, 34 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 474ab7ed65ea..192c540a650c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1299,6 +1299,31 @@ int btrfs_forget_devices(dev_t devt)
>  	return ret;
>  }
>  
> +static bool btrfs_skip_registration(struct btrfs_super_block *disk_super,
> +				    dev_t devt, bool mount_arg_dev)
> +{
> +	struct btrfs_fs_devices *fs_devices;
> +
> +	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
> +		struct btrfs_device *device;
> +
> +		mutex_lock(&fs_devices->device_list_mutex);
> +		list_for_each_entry(device, &fs_devices->devices, dev_list) {
> +			if (device->devt == devt) {
> +				mutex_unlock(&fs_devices->device_list_mutex);
> +				return false;
> +			}
> +		}
> +		mutex_unlock(&fs_devices->device_list_mutex);

This is locking and unlocking again before going to device_list_add, so
if something changes regarding the registered device then it's not up to
date.


> +	}
> +
> +	if (!mount_arg_dev && btrfs_super_num_devices(disk_super) == 1 &&
> +	    !(btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_SEEDING))
> +		return true;

The way I implemented it is to check the above conditions as a
prerequisite but leave the heavy work for device_list_add that does all
the uuid and device list locking and we are quite sure it survives all
the races between scanning and mounts.

> +
> +	return false;
> +}
> +
>  /*
>   * Look for a btrfs signature on a device. This may be called out of the mount path
>   * and we are not allowed to call set_blocksize during the scan. The superblock
> @@ -1316,6 +1341,7 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>  	struct btrfs_device *device = NULL;
>  	struct bdev_handle *bdev_handle;
>  	u64 bytenr, bytenr_orig;
> +	dev_t devt = 0;
>  	int ret;
>  
>  	lockdep_assert_held(&uuid_mutex);
> @@ -1355,18 +1381,16 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>  		goto error_bdev_put;
>  	}
>  
> -	if (!mount_arg_dev && btrfs_super_num_devices(disk_super) == 1 &&
> -	    !(btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_SEEDING)) {
> -		dev_t devt;
> +	ret = lookup_bdev(path, &devt);
> +	if (ret)
> +		btrfs_warn(NULL, "lookup bdev failed for path %s: %d",
> +			   path, ret);
>  
> -		ret = lookup_bdev(path, &devt);

Do we actually need this check? It was added with the patch skipping the
registration, so it's validating the block device but how can we pass
something that is not a valid block device?

Besides there's a call to bdev_open_by_path() that in turn does the
lookup_bdev so checking it here is redundant. It's not related to the
fix itself but I deleted it in my fix.

> -		if (ret)
> -			btrfs_warn(NULL, "lookup bdev failed for path %s: %d",
> -				   path, ret);
> -		else
> +	if (btrfs_skip_registration(disk_super, devt, mount_arg_dev)) {
> +		pr_debug("BTRFS: skip registering single non-seed device %s\n",
> +			  path);
> +		if (devt)
>  			btrfs_free_stale_devices(devt, NULL);
> -
> -		pr_debug("BTRFS: skip registering single non-seed device %s\n", path);
>  		device = NULL;
>  		goto free_disk_super;
>  	}