Re: [PATCH] btrfs: Create sysfs entries only for non-stale devices

[Goffredo Baroncelli <kreijack@libero.it>]

Hi Nikolay,

On 12/15/21 15:46, Nikolay Borisov wrote:
> Currently /sys/fs/btrfs/<uuid>/devinfo/<devid> entry is always created
> for a device present in btrfs_fs_devices on the other hand
> /sys/fs/btrfs/<uuid>/devices/<devname> sysfs link is created only when
> the given btrfs_device::bdisk member is populated. This can lead to
> cases where a filesystem consisting of 2 device, one of which is stale
> ends up having 2 entries under /sys/fs/btrfs/<uuid>/devinfo/<devid>
> but only one under /sys/fs/btrfs/<uuid>/devices/<devname>.

What happened in case of a degraded mode ? Is correct to not show a missing devices ?


> Another case that occurs is if a filesystem initially occupied 2
> devices, then got unmounted, and a new filesystem is created, which
> occupies a single device but with the same UUID as the one occupying 2
> devices. In this case /sys/fs/btrfs/<uuid>/devices/<devname> will
> correctly have 1 entry but /sys/fs/btrfs/<uuid>/devices/<devname> will
> incorrectly has 2. This behavior is demonstrated by the following
> script:
> 
>      UUID=292afefb-6e8c-4fb3-9d12-8c4ecb1f2374
>      rm /tmp/d1
>      rm /tmp/d2
>      truncate -s 1G /tmp/d1
>      truncate -s 1G /tmp/d2
>      sudo losetup /dev/loop1 /tmp/d1
>      sudo losetup /dev/loop2 /tmp/d2
>      sudo mkfs.btrfs -U $UUID /dev/loop1 /dev/loop2
>      sudo mount /dev/loop1 /mnt/btrfs1
>      sudo umount /dev/loop1
>      sudo losetup -d /dev/loop2
>      sudo losetup -d /dev/loop1
> 
>      # create a new filesystem with only ONE loop-device; mount it
>      rm /tmp/d1
>      truncate -s 1G /tmp/d1
>      sudo losetup /dev/loop1 /tmp/d1
>      sudo mkfs.btrfs -U $UUID /dev/loop1
>      sudo mount /dev/loop1 /mnt/btrfs1
> 
> Fix this by ensuring that device sysfs attributes are only added for
> devices which are actually present at the time of mount.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/sysfs.c | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index beb7f72d50b8..e2e110d7798a 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -12,6 +12,7 @@
>   #include <crypto/hash.h>
>   
>   #include "ctree.h"
> +#include "rcu-string.h"
>   #include "discard.h"
>   #include "disk-io.h"
>   #include "send.h"
> @@ -1611,9 +1612,13 @@ int btrfs_sysfs_add_device(struct btrfs_device *device)
>   {
>   	int ret;
>   	unsigned int nofs_flag;
> +	struct kobject *disk_kobj;
>   	struct kobject *devices_kobj;
>   	struct kobject *devinfo_kobj;
>   
> +	if (!device->bdev)
> +		return 0;
> +
>   	/*
>   	 * Make sure we use the fs_info::fs_devices to fetch the kobjects even
>   	 * for the seed fs_devices
> @@ -1625,16 +1630,14 @@ int btrfs_sysfs_add_device(struct btrfs_device *device)
>   
>   	nofs_flag = memalloc_nofs_save();
>   
> -	if (device->bdev) {
> -		struct kobject *disk_kobj = bdev_kobj(device->bdev);
> +	disk_kobj = bdev_kobj(device->bdev);
>   
> -		ret = sysfs_create_link(devices_kobj, disk_kobj, disk_kobj->name);
> -		if (ret) {
> -			btrfs_warn(device->fs_info,
> -				"creating sysfs device link for devid %llu failed: %d",
> -				device->devid, ret);
> -			goto out;
> -		}
> +	ret = sysfs_create_link(devices_kobj, disk_kobj, disk_kobj->name);
> +	if (ret) {
> +		btrfs_warn(device->fs_info,
> +			   "creating sysfs device link for devid %llu failed: %d",
> +			   device->devid, ret);
> +		goto out;
>   	}
>   
>   	init_completion(&device->kobj_unregister);


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5