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

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

On 01/03/2019 12:00 AM, David Sterba wrote:
> On Wed, Jan 02, 2019 at 01:49:31PM +0800, 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>
>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>> v11->v12: fix coding style add spacing before after ":".
>> v1->v11: Pls ref to the cover-letter. (sorry about that).
>>
>>   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 d3c6bbc0aa3a..eba2966913ae 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -2247,6 +2247,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 8e36cbb355df..48415a9edd46 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1397,6 +1397,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);
>> +	mutex_unlock(&uuid_mutex);
>> +
>> +	return 0;
> 
> I think this should reflect the status of the operation. The ioctl
> caller has no information about success/error. Eg. an invalid device
> path passed could return -ENOENT, 0 if it was released and 1 if not.
> 
> In case of all devices, it's probably ok to return 0.


OK. v13 will return error if the given device is not found,
and I find ENXIO is more suitable (instead of ENOENT).
If you disagree pls can you make the changes as in [1]
when integrating. I have tested with both.

--------
[1]
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2ab42bfe5d3c..e04784c64a98 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -763,7 +763,7 @@ static int btrfs_free_stale_devices(const char *path,
         int ret;

         if (path)
-               ret = -ENXIO;
+               ret = -ENOENT;
         else
                 ret = 0;
----------------

Thanks, Anand