Re: [PATCH] btrfs-progs: add cli to forget one or all scanned devices

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

On 10/26/2018 08:21 PM, Nikolay Borisov wrote:
> 
> 
> On 24.10.2018 07:31, Anand Jain wrote:
>> This patch adds cli
>>    btrfs device forget [dev]
>> to remove the given device structure in the kernel if the device
>> is unmounted. If no argument is given it shall remove all stale
>> (device which are not mounted) from the kernel.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   cmds-device.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
>>   ioctl.h       |  2 ++
>>   2 files changed, 53 insertions(+), 8 deletions(-)
>>
>> diff --git a/cmds-device.c b/cmds-device.c
>> index 2a05f70a76a9..c2a2b7f304b8 100644
>> --- a/cmds-device.c
>> +++ b/cmds-device.c
>> @@ -254,10 +254,32 @@ static int cmd_device_delete(int argc, char **argv)
>>   	return _cmd_device_remove(argc, argv, cmd_device_delete_usage);
>>   }
>>   
>> +static int btrfs_forget_devices(char *path)
>> +{
>> +	struct btrfs_ioctl_vol_args args;
>> +	int ret;
>> +	int fd;
>> +
>> +	fd = open("/dev/btrfs-control", O_RDWR);
>> +	if (fd < 0)
>> +		return -errno;
>> +
>> +	memset(&args, 0, sizeof(args));
>> +	if (path)
>> +		strncpy_null(args.name, path);
>> +	ret = ioctl(fd, BTRFS_IOC_FORGET_DEV, &args);
>> +	if (ret)
>> +		ret = -errno;
>> +	close(fd);
>> +	return ret;
>> +}
>> +
>>   static const char * const cmd_device_scan_usage[] = {
>> -	"btrfs device scan [(-d|--all-devices)|<device> [<device>...]]",
>> -	"Scan devices for a btrfs filesystem",
>> +	"btrfs device scan [(-d|--all-devices)|(-u|--forget)|<device> "\
>> +							"[<device>...]]",
>> +	"Scan or forget (deregister) devices for a btrfs filesystem",
>>   	" -d|--all-devices (deprecated)",
>> +	" -u|--forget [<device> ..]",
>>   	NULL
>>   };
>>   
>> @@ -267,32 +289,45 @@ static int cmd_device_scan(int argc, char **argv)
>>   	int devstart;
>>   	int all = 0;
>>   	int ret = 0;
>> +	int forget = 0;
>>   
>>   	optind = 0;
>>   	while (1) {
>>   		int c;
>>   		static const struct option long_options[] = {
>>   			{ "all-devices", no_argument, NULL, 'd'},
>> +			{ "forget", no_argument, NULL, 'u'},
>>   			{ NULL, 0, NULL, 0}
>>   		};
>>   
>> -		c = getopt_long(argc, argv, "d", long_options, NULL);
>> +		c = getopt_long(argc, argv, "du", long_options, NULL);
>>   		if (c < 0)
>>   			break;
>>   		switch (c) {
>>   		case 'd':
>>   			all = 1;
>>   			break;
>> +		case 'u':
>> +			forget = 1;
>> +			break;
>>   		default:
>>   			usage(cmd_device_scan_usage);
>>   		}
>>   	}
>>   	devstart = optind;
>>   
>> +	if (all && forget)
>> +		usage(cmd_device_scan_usage);
>> +
>>   	if (all && check_argc_max(argc - optind, 1))
>>   		usage(cmd_device_scan_usage);
>>   
>>   	if (all || argc - optind == 0) {
>> +		if (forget) {
>> +			ret = btrfs_forget_devices(NULL);
>> +			error_on(ret, "error %d while running forget", ret);
>> +			goto out;
>> +		}
> 
> nit: I would prefer to also have an if () {} else {} construct here,
> similar to what you do below.

  Will fix.

  Also, I think I can use sterror() instead of ret, in the error_on().

  Fixed both of these in v11.

Thanks, Anand

>>   		printf("Scanning for Btrfs filesystems\n");
>>   		ret = btrfs_scan_devices();
>>   		error_on(ret, "error %d while scanning", ret);
>> @@ -315,11 +350,19 @@ static int cmd_device_scan(int argc, char **argv)
>>   			ret = 1;
>>   			goto out;
>>   		}
>> -		printf("Scanning for Btrfs filesystems in '%s'\n", path);
>> -		if (btrfs_register_one_device(path) != 0) {
>> -			ret = 1;
>> -			free(path);
>> -			goto out;
>> +		if (forget) {
>> +			ret = btrfs_forget_devices(path);
>> +			if (ret)
>> +				error("Can't forget '%s': %s",
>> +							path, strerror(-ret));
>> +		} else {
>> +			printf("Scanning for Btrfs filesystems in '%s'\n",
>> +									path);
>> +			if (btrfs_register_one_device(path) != 0) {
>> +				ret = 1;
>> +				free(path);
>> +				goto out;
>> +			}
>>   		}
>>   		free(path);
>>   	}
>> diff --git a/ioctl.h b/ioctl.h
>> index 709e996f401c..e27d80e09392 100644
>> --- a/ioctl.h
>> +++ b/ioctl.h
>> @@ -721,6 +721,8 @@ static inline char *btrfs_err_str(enum btrfs_err_code 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
>>