Re: [PATCH] btrfs: introduce feature to forget a btrfs device

[Anand Jain <anand.jain@oracle.com>]

On 10/12/2018 04:48 PM, Nikolay Borisov wrote:
> 
> 
> On 12.10.2018 07:06, Anand Jain wrote:
>> Support for a new command 'btrfs dev forget [dev]' is proposed here
>> to undo the effects of 'btrfs dev scan [dev]'. For this purpose
>> this patch proposes to use ioctl #5 as it was empty.
>> 	IOW(BTRFS_IOCTL_MAGIC, 5, ..)
>> This patch adds new ioctl BTRFS_IOC_FORGET_DEV which can be sent from
>> the /dev/btrfs-control to forget one or all devices, (devices which are
>> not mounted) from the btrfs kernel.
>>
>> The argument it takes is struct btrfs_ioctl_vol_args, and ::name can be
>> set to specify the device path. And all unmounted devices can be removed
>> from the kernel if no device path is provided.
>>
>> Again, the devices are removed only if the relevant fsid aren't mounted.
>>
>> This new cli can provide..
>>   . Release of unwanted btrfs_fs_devices and btrfs_devices memory if the
>>     device is not going to be mounted.
>>   . Ability to mount the device in degraded mode when one of the other
>>     device is corrupted like in split brain raid1.
>>   . Running test cases which requires btrfs.ko-reload if the rootfs
>>     is btrfs.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/super.c           | 3 +++
>>   fs/btrfs/volumes.c         | 9 +++++++++
>>   fs/btrfs/volumes.h         | 1 +
>>   include/uapi/linux/btrfs.h | 2 ++
>>   4 files changed, 15 insertions(+)
>>
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 6601c9aa5e35..f14610ef1c66 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -2244,6 +2244,9 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
>>   		ret = PTR_ERR_OR_ZERO(device);
>>   		mutex_unlock(&uuid_mutex);
>>   		break;
>> +	case BTRFS_IOC_FORGET_DEV:
>> +		ret = btrfs_forget_devices(vol->name);
>> +		break;
>>   	case BTRFS_IOC_DEVICES_READY:
>>   		mutex_lock(&uuid_mutex);
>>   		device = btrfs_scan_one_device(vol->name, FMODE_READ,
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index f4405e430da6..e2ab203d9c1d 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1208,6 +1208,15 @@ static int btrfs_read_disk_super(struct block_device *bdev, u64 bytenr,
>>   	return 0;
>>   }
>>   
>> +int btrfs_forget_devices(const char *path)
>> +{
>> +	mutex_lock(&uuid_mutex);
>> +	btrfs_free_stale_devices(strlen(path) ? path:NULL, NULL);
> 
> Checking the len of path is redundant since userspace will pass a zeroed
> struct which will be NULL in any case. Additionally in
> btrfs_free_stalce_devices there is existing check whether path is set or
> not.

That won't work. btrfs_free_stalce_devices() expects the pointer to be 
NULL. User space has zeroed the name which is char array and its pointer 
is not NULL.

Thanks, Anand

>> +	mutex_unlock(&uuid_mutex);
>> +
>> +	return 0;
>> +}
>> +
>>   /*
>>    * 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
>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
>> index 23e9285d88de..394079d5d08d 100644
>> --- a/fs/btrfs/volumes.h
>> +++ b/fs/btrfs/volumes.h
>> @@ -406,6 +406,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>>   		       fmode_t flags, void *holder);
>>   struct btrfs_device *btrfs_scan_one_device(const char *path,
>>   					   fmode_t flags, void *holder);
>> +int btrfs_forget_devices(const char *path);
>>   int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
>>   void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step);
>>   void btrfs_assign_next_active_device(struct btrfs_device *device,
>> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
>> index 5ca1d21fc4a7..b1be7f828cb4 100644
>> --- a/include/uapi/linux/btrfs.h
>> +++ b/include/uapi/linux/btrfs.h
>> @@ -836,6 +836,8 @@ enum btrfs_err_code {
>>   				   struct btrfs_ioctl_vol_args)
>>   #define BTRFS_IOC_SCAN_DEV _IOW(BTRFS_IOCTL_MAGIC, 4, \
>>   				   struct btrfs_ioctl_vol_args)
>> +#define BTRFS_IOC_FORGET_DEV _IOW(BTRFS_IOCTL_MAGIC, 5, \
>> +				   struct btrfs_ioctl_vol_args)
>>   /* trans start and trans end are dangerous, and only for
>>    * use by applications that know how to avoid the
>>    * resulting deadlocks
>>