[PATCH v4 0/2] Add cli and ioctl to ignore a scanned device

[PATCH v4 0/2] Add cli and ioctl to ignore a scanned device

[Anand Jain]

Patch 1/2 is preparatory patch to get a function to delete a device
  from the list.
Patch 2/2 adds the ioctl and feature to delete a device from the
device list.

Also includes Patch 1/1 for the btrfs-progs support.

v2:
  Accepts review from Nikolay, details are in the specific patch.
  Patch 1/2 is renamed from
    [PATCH 1/2] btrfs: refactor btrfs_free_stale_device() to get device list delete
  to
    [PATCH 1/2] btrfs: add function to device list delete
v3:
  No change. Send to correct ML.
v4:
  No change. But as the ML thread may be confusing, so resend.

Anand Jain (2):
  btrfs: add function to device list delete
  btrfs: introduce feature to ignore a btrfs device

 fs/btrfs/super.c           |  4 +++
 fs/btrfs/volumes.c         | 86 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/volumes.h         |  2 ++
 include/uapi/linux/btrfs.h |  2 ++
 4 files changed, 94 insertions(+)

Anand Jain (1):
  btrfs-progs: add 'btrfs device ignore' cli

 cmds-device.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ioctl.h       |  2 ++
 2 files changed, 56 insertions(+)

-- 
2.7.0

[PATCH v4 1/2] btrfs: add function to device list delete

[Anand Jain]

We need device delete from the dev_list so create a new function.
New instead of refactor of btrfs_free_stale_device() because,
btrfs_free_stale_device() doesn't hold device_list_mutex which
is actually needed here.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1: title of this patch
  btrfs: refactor btrfs_free_stale_device() to get device list delete
v2:
 delete_device_from_list() is not pealed from btrfs_free_stale_device()
 as this needs device_list_mutex. And its static now.
v3: Send to correct ML
v4: no change

 fs/btrfs/volumes.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 04164337ac69..5deda80316f0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -531,6 +531,28 @@ static void pending_bios_fn(struct btrfs_work *work)
 	run_scheduled_bios(device);
 }
 
+static void delete_device_from_list(struct btrfs_device *device)
+{
+	struct btrfs_fs_devices *fs_devices;
+
+	fs_devices = device->fs_devices;
+
+	lockdep_assert_held(&uuid_mutex);
+
+	mutex_lock(&fs_devices->device_list_mutex);
+	fs_devices->num_devices--;
+	list_del(&device->dev_list);
+	mutex_unlock(&fs_devices->device_list_mutex);
+
+	rcu_string_free(device->name);
+	kfree(device);
+
+	if (fs_devices->num_devices == 0) {
+		btrfs_sysfs_remove_fsid(fs_devices);
+		list_del(&fs_devices->list);
+		free_fs_devices(fs_devices);
+	}
+}
 
 static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
 {
-- 
2.7.0

[PATCH v4] btrfs-progs: add 'btrfs device ignore' cli

[Anand Jain]

This patch adds
  btrfs device ignode <dev>
so that a device can be ignored/missed during mount if the device is
already been scanned. Basically, this command will undo the effect
of the command
  btrfs device scan <dev>

This change is compatible with older kernel without the ioctl
BTRFS_IOC_IGNORE_DEV which is added here. At which it shall
report 'Inappropriate ioctl for device'. And will report
'Operation not permitted' on mounted device.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1-4: No change.

 cmds-device.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ioctl.h       |  2 ++
 2 files changed, 56 insertions(+)

diff --git a/cmds-device.c b/cmds-device.c
index f4cdb39f64ac..31b1945d3339 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -329,6 +329,59 @@ out:
 	return !!ret;
 }
 
+static const char * const cmd_device_ignore_usage[] = {
+	"btrfs device ignore [<device>]",
+	"Ignore a device in the btrfs kernel module.",
+	NULL
+};
+
+static int btrfs_ignore_one_device(char *path)
+{
+	struct btrfs_ioctl_vol_args args;
+	int fd;
+	int ret;
+
+	fd = open("/dev/btrfs-control", O_RDWR);
+	if (fd < 0)
+		return -errno;
+
+	memset(&args, 0, sizeof(args));
+	strncpy_null(args.name, path);
+	ret = ioctl(fd, BTRFS_IOC_IGNORE_DEV, &args);
+	if (ret)
+		ret = -errno;
+	close(fd);
+	return ret;
+}
+
+static int cmd_device_ignore(int argc, char **argv)
+{
+	int ret = 0;
+	char *path;
+
+	if (check_argc_max(argc - optind, 1))
+		usage(cmd_device_ignore_usage);
+
+	if (is_block_device(argv[1]) != 1) {
+		error("Not a block device: %s", argv[1]);
+		return -ENOENT;
+	}
+
+	path = canonicalize_path(argv[1]);
+	if (!path) {
+		error("Could not canonicalize path '%s': %s",
+					argv[1], strerror(errno));
+		return -ENOENT;
+	}
+
+	ret  = btrfs_ignore_one_device(path);
+	if (ret)
+		error("Can't ignore %s: %s", path, strerror(-ret));
+
+	free(path);
+	return ret;
+}
+
 static const char * const cmd_device_ready_usage[] = {
 	"btrfs device ready <device>",
 	"Check device to see if it has all of its devices in cache for mounting",
@@ -604,6 +657,7 @@ const struct cmd_group device_cmd_group = {
 			CMD_ALIAS },
 		{ "remove", cmd_device_remove, cmd_device_remove_usage, NULL, 0 },
 		{ "scan", cmd_device_scan, cmd_device_scan_usage, NULL, 0 },
+		{ "ignore", cmd_device_ignore, cmd_device_ignore_usage, NULL, 0 },
 		{ "ready", cmd_device_ready, cmd_device_ready_usage, NULL, 0 },
 		{ "stats", cmd_device_stats, cmd_device_stats_usage, NULL, 0 },
 		{ "usage", cmd_device_usage,
diff --git a/ioctl.h b/ioctl.h
index 709e996f401c..bef8508119aa 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_IGNORE_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
-- 
2.7.0

[PATCH v4 2/2] btrfs: introduce feature to ignore a btrfs device

[Anand Jain]

Support for a new command is being added here:
 btrfs dev ignore <dev>
Which shall undo the effects of the command
 btrfs dev scan <dev>

This cli/ioctl is needed as there is no way to continue to mount in
degraded mode if the device is already scanned, which is required to
recover from the split brain raid conditions.

This patch proposes to use ioctl #5 as it was empty.
	IOW(BTRFS_IOCTL_MAGIC, 5, ..)
If #5 is reserved for some other purpose, I think I should change this.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: Use -EBUSY instead of -ENOENT
    Since now delete_device_from_list() holds device_list_mutex
    so dont hold device_list_mutex in its parent. Reword and indent
    pr_err/info.
v3: Send to correct ML
v4: no change.

 fs/btrfs/super.c           |  4 +++
 fs/btrfs/volumes.c         | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/volumes.h         |  2 ++
 include/uapi/linux/btrfs.h |  2 ++
 4 files changed, 72 insertions(+)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index f443517fa2f8..fcc4a6ef4795 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2212,6 +2212,10 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
 		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
 					    &btrfs_fs_type, &fs_devices);
 		break;
+	case BTRFS_IOC_IGNORE_DEV:
+		ret = btrfs_ignore_one_device(vol->name, FMODE_READ,
+					    &btrfs_fs_type, &fs_devices);
+		break;
 	case BTRFS_IOC_DEVICES_READY:
 		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
 					    &btrfs_fs_type, &fs_devices);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5deda80316f0..2bae2ccd262d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1205,6 +1205,70 @@ static int btrfs_read_disk_super(struct block_device *bdev, u64 bytenr,
 	return 0;
 }
 
+static int device_list_remove(struct btrfs_super_block *disk_super, u64 devid)
+{
+	int ret = 0;
+	struct btrfs_fs_devices *fs_devices;
+	struct btrfs_device *device;
+
+	fs_devices = find_fsid(disk_super->fsid);
+	if (!fs_devices)
+		return -ENOENT;
+
+	if (fs_devices->opened)
+		return -EBUSY;
+
+	mutex_lock(&uuid_mutex);
+
+	device = find_device(fs_devices, devid, disk_super->dev_item.uuid);
+	if (device)
+		delete_device_from_list(device);
+
+	mutex_unlock(&uuid_mutex);
+
+	return ret;
+}
+
+int btrfs_ignore_one_device(const char *path, fmode_t flags, void *holder,
+			  struct btrfs_fs_devices **fs_devices_ret)
+{
+	struct btrfs_super_block *disk_super;
+	struct block_device *bdev;
+	struct page *page;
+	int ret = -EINVAL;
+	u64 devid;
+	u64 bytenr;
+
+	bytenr = btrfs_sb_offset(0);
+	flags |= FMODE_EXCL;
+
+	bdev = blkdev_get_by_path(path, flags, holder);
+	if (IS_ERR(bdev)) {
+		ret = PTR_ERR(bdev);
+		goto error;
+	}
+
+	if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super))
+		goto error_bdev_put;
+
+	devid = btrfs_stack_device_id(&disk_super->dev_item);
+
+	ret = device_list_remove(disk_super, devid);
+	if (ret)
+		pr_err("BTRFS: %pU device %s devid %llu failed to ignore: %d\n",
+			disk_super->fsid, path, devid, ret);
+	else
+		pr_info("BTRFS: %pU device %s devid %llu ignored\n",
+			disk_super->fsid, path, devid);
+
+	btrfs_release_disk_super(page);
+
+error_bdev_put:
+	blkdev_put(bdev, flags);
+error:
+	return ret;
+}
+
 /*
  * 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 7acfd61611aa..08d3425bc880 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -423,6 +423,8 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		       fmode_t flags, void *holder);
 int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 			  struct btrfs_fs_devices **fs_devices_ret);
+int btrfs_ignore_one_device(const char *path, fmode_t flags, void *holder,
+			  struct btrfs_fs_devices **fs_devices_ret);
 int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
 void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices, int step);
 void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index ce615b75e855..bb13bad41b30 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -744,6 +744,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_IGNORE_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
-- 
2.7.0

Re: [PATCH v4 1/2] btrfs: add function to device list delete

[David Sterba]

On Tue, Dec 05, 2017 at 04:52:56PM +0800, Anand Jain wrote:
> We need device delete from the dev_list so create a new function.
> New instead of refactor of btrfs_free_stale_device() because,
> btrfs_free_stale_device() doesn't hold device_list_mutex which
> is actually needed here.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v1: title of this patch
>   btrfs: refactor btrfs_free_stale_device() to get device list delete
> v2:
>  delete_device_from_list() is not pealed from btrfs_free_stale_device()
>  as this needs device_list_mutex. And its static now.
> v3: Send to correct ML
> v4: no change
> 
>  fs/btrfs/volumes.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 04164337ac69..5deda80316f0 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -531,6 +531,28 @@ static void pending_bios_fn(struct btrfs_work *work)
>  	run_scheduled_bios(device);
>  }
>  
> +static void delete_device_from_list(struct btrfs_device *device)
> +{
> +	struct btrfs_fs_devices *fs_devices;
> +
> +	fs_devices = device->fs_devices;
> +
> +	lockdep_assert_held(&uuid_mutex);
> +
> +	mutex_lock(&fs_devices->device_list_mutex);
> +	fs_devices->num_devices--;
> +	list_del(&device->dev_list);
> +	mutex_unlock(&fs_devices->device_list_mutex);
> +
> +	rcu_string_free(device->name);
> +	kfree(device);

Please use the new helper introduced in patch "btrfs: introduce
free_device helper" (currently in misc-next), you'd leak the flush bio
here.

> +
> +	if (fs_devices->num_devices == 0) {
> +		btrfs_sysfs_remove_fsid(fs_devices);
> +		list_del(&fs_devices->list);
> +		free_fs_devices(fs_devices);
> +	}
> +}

Re: [PATCH v4] btrfs-progs: add 'btrfs device ignore' cli

[David Sterba]

On Tue, Dec 05, 2017 at 04:52:57PM +0800, Anand Jain wrote:
> This patch adds
>   btrfs device ignode <dev>
> so that a device can be ignored/missed during mount if the device is
> already been scanned. Basically, this command will undo the effect
> of the command
>   btrfs device scan <dev>

As it logicall belongs to scan, I'd rather make it an option, like

  $ btrfs scan --unregister

or

  $ btrfs scan --forget

The meaning of 'ignore' in this context is not clear to me, we can
ignore errors on the device.

> This change is compatible with older kernel without the ioctl
> BTRFS_IOC_IGNORE_DEV which is added here. At which it shall
> report 'Inappropriate ioctl for device'. And will report
> 'Operation not permitted' on mounted device.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v1-4: No change.
> 
>  cmds-device.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  ioctl.h       |  2 ++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/cmds-device.c b/cmds-device.c
> index f4cdb39f64ac..31b1945d3339 100644
> --- a/cmds-device.c
> +++ b/cmds-device.c
> @@ -329,6 +329,59 @@ out:
>  	return !!ret;
>  }
>  
> +static const char * const cmd_device_ignore_usage[] = {
> +	"btrfs device ignore [<device>]",
> +	"Ignore a device in the btrfs kernel module.",
> +	NULL
> +};
> +
> +static int btrfs_ignore_one_device(char *path)
> +{
> +	struct btrfs_ioctl_vol_args args;
> +	int fd;
> +	int ret;
> +
> +	fd = open("/dev/btrfs-control", O_RDWR);
> +	if (fd < 0)
> +		return -errno;
> +
> +	memset(&args, 0, sizeof(args));
> +	strncpy_null(args.name, path);
> +	ret = ioctl(fd, BTRFS_IOC_IGNORE_DEV, &args);
> +	if (ret)
> +		ret = -errno;
> +	close(fd);
> +	return ret;
> +}
> +
> +static int cmd_device_ignore(int argc, char **argv)
> +{
> +	int ret = 0;
> +	char *path;
> +
> +	if (check_argc_max(argc - optind, 1))
> +		usage(cmd_device_ignore_usage);
> +
> +	if (is_block_device(argv[1]) != 1) {
> +		error("Not a block device: %s", argv[1]);
> +		return -ENOENT;
> +	}
> +
> +	path = canonicalize_path(argv[1]);
> +	if (!path) {
> +		error("Could not canonicalize path '%s': %s",
> +					argv[1], strerror(errno));
> +		return -ENOENT;
> +	}
> +
> +	ret  = btrfs_ignore_one_device(path);
> +	if (ret)
> +		error("Can't ignore %s: %s", path, strerror(-ret));
> +
> +	free(path);
> +	return ret;
> +}
> +
>  static const char * const cmd_device_ready_usage[] = {
>  	"btrfs device ready <device>",
>  	"Check device to see if it has all of its devices in cache for mounting",
> @@ -604,6 +657,7 @@ const struct cmd_group device_cmd_group = {
>  			CMD_ALIAS },
>  		{ "remove", cmd_device_remove, cmd_device_remove_usage, NULL, 0 },
>  		{ "scan", cmd_device_scan, cmd_device_scan_usage, NULL, 0 },
> +		{ "ignore", cmd_device_ignore, cmd_device_ignore_usage, NULL, 0 },
>  		{ "ready", cmd_device_ready, cmd_device_ready_usage, NULL, 0 },
>  		{ "stats", cmd_device_stats, cmd_device_stats_usage, NULL, 0 },
>  		{ "usage", cmd_device_usage,
> diff --git a/ioctl.h b/ioctl.h
> index 709e996f401c..bef8508119aa 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_IGNORE_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
> -- 
> 2.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] btrfs: introduce feature to ignore a btrfs device

[David Sterba]

On Tue, Dec 05, 2017 at 04:52:58PM +0800, Anand Jain wrote:
> Support for a new command is being added here:
>  btrfs dev ignore <dev>
> Which shall undo the effects of the command
>  btrfs dev scan <dev>
> 
> This cli/ioctl is needed as there is no way to continue to mount in
> degraded mode if the device is already scanned, which is required to
> recover from the split brain raid conditions.
> 
> This patch proposes to use ioctl #5 as it was empty.
> 	IOW(BTRFS_IOCTL_MAGIC, 5, ..)
> If #5 is reserved for some other purpose, I think I should change this.

I think 5 is free for use.

> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2: Use -EBUSY instead of -ENOENT
>     Since now delete_device_from_list() holds device_list_mutex
>     so dont hold device_list_mutex in its parent. Reword and indent
>     pr_err/info.
> v3: Send to correct ML
> v4: no change.
> 
>  fs/btrfs/super.c           |  4 +++
>  fs/btrfs/volumes.c         | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/volumes.h         |  2 ++
>  include/uapi/linux/btrfs.h |  2 ++
>  4 files changed, 72 insertions(+)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index f443517fa2f8..fcc4a6ef4795 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -2212,6 +2212,10 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
>  		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
>  					    &btrfs_fs_type, &fs_devices);
>  		break;
> +	case BTRFS_IOC_IGNORE_DEV:
> +		ret = btrfs_ignore_one_device(vol->name, FMODE_READ,
> +					    &btrfs_fs_type, &fs_devices);
> +		break;
>  	case BTRFS_IOC_DEVICES_READY:
>  		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
>  					    &btrfs_fs_type, &fs_devices);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 5deda80316f0..2bae2ccd262d 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1205,6 +1205,70 @@ static int btrfs_read_disk_super(struct block_device *bdev, u64 bytenr,
>  	return 0;
>  }
>  
> +static int device_list_remove(struct btrfs_super_block *disk_super, u64 devid)
> +{
> +	int ret = 0;
> +	struct btrfs_fs_devices *fs_devices;
> +	struct btrfs_device *device;
> +
> +	fs_devices = find_fsid(disk_super->fsid);

Don't we need uuid mutext to call find_fsid? All other users do that.

> +	if (!fs_devices)
> +		return -ENOENT;
> +
> +	if (fs_devices->opened)
> +		return -EBUSY;
> +
> +	mutex_lock(&uuid_mutex);
> +
> +	device = find_device(fs_devices, devid, disk_super->dev_item.uuid);
> +	if (device)
> +		delete_device_from_list(device);
> +
> +	mutex_unlock(&uuid_mutex);
> +
> +	return ret;
> +}
> +
> +int btrfs_ignore_one_device(const char *path, fmode_t flags, void *holder,
> +			  struct btrfs_fs_devices **fs_devices_ret)
> +{
> +	struct btrfs_super_block *disk_super;
> +	struct block_device *bdev;
> +	struct page *page;
> +	int ret = -EINVAL;

Please move EINVAL to the point where this happens (ie. after the
btrfs_read_disk_super call). This is the common pattern and makes
reading the code smooth.

> +	u64 devid;
> +	u64 bytenr;
> +
> +	bytenr = btrfs_sb_offset(0);
> +	flags |= FMODE_EXCL;
> +
> +	bdev = blkdev_get_by_path(path, flags, holder);
> +	if (IS_ERR(bdev)) {
> +		ret = PTR_ERR(bdev);
> +		goto error;
> +	}
> +
> +	if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super))
> +		goto error_bdev_put;
> +
> +	devid = btrfs_stack_device_id(&disk_super->dev_item);
> +
> +	ret = device_list_remove(disk_super, devid);
> +	if (ret)
> +		pr_err("BTRFS: %pU device %s devid %llu failed to ignore: %d\n",
> +			disk_super->fsid, path, devid, ret);

So we can't easily use btrfs_printk here due to lack of fs_info that
would appear as "<unknown>" in place of the device. Ok.

> +	else
> +		pr_info("BTRFS: %pU device %s devid %llu ignored\n",
> +			disk_super->fsid, path, devid);
> +
> +	btrfs_release_disk_super(page);
> +
> +error_bdev_put:
> +	blkdev_put(bdev, flags);
> +error:
> +	return ret;
> +}
> +
>  /*
>   * 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 7acfd61611aa..08d3425bc880 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -423,6 +423,8 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>  		       fmode_t flags, void *holder);
>  int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>  			  struct btrfs_fs_devices **fs_devices_ret);
> +int btrfs_ignore_one_device(const char *path, fmode_t flags, void *holder,
> +			  struct btrfs_fs_devices **fs_devices_ret);
>  int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
>  void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices, int step);
>  void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> index ce615b75e855..bb13bad41b30 100644
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -744,6 +744,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_IGNORE_DEV _IOW(BTRFS_IOCTL_MAGIC, 5, \
> +				   struct btrfs_ioctl_vol_args)

While reading the patches, I realized we may want to extend the ioctl to
unregister/forge all devices that are not currently mounted. For that
purpose using the btrfs_ioctl_vol_args_v2 would be suitable as it has
more struct members.

Another extension is to unregister only stale devices (when there's no
device node under /dev), eg. after the device is unuplugged and readded
by another name.

Re: [PATCH v4 1/2] btrfs: add function to device list delete

[Anand Jain]

On 12/06/2017 03:06 AM, David Sterba wrote:
> On Tue, Dec 05, 2017 at 04:52:56PM +0800, Anand Jain wrote:
>> We need device delete from the dev_list so create a new function.
>> New instead of refactor of btrfs_free_stale_device() because,
>> btrfs_free_stale_device() doesn't hold device_list_mutex which
>> is actually needed here.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v1: title of this patch
>>    btrfs: refactor btrfs_free_stale_device() to get device list delete
>> v2:
>>   delete_device_from_list() is not pealed from btrfs_free_stale_device()
>>   as this needs device_list_mutex. And its static now.
>> v3: Send to correct ML
>> v4: no change
>>
>>   fs/btrfs/volumes.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 04164337ac69..5deda80316f0 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -531,6 +531,28 @@ static void pending_bios_fn(struct btrfs_work *work)
>>   	run_scheduled_bios(device);
>>   }
>>   
>> +static void delete_device_from_list(struct btrfs_device *device)
>> +{
>> +	struct btrfs_fs_devices *fs_devices;
>> +
>> +	fs_devices = device->fs_devices;
>> +
>> +	lockdep_assert_held(&uuid_mutex);
>> +
>> +	mutex_lock(&fs_devices->device_list_mutex);
>> +	fs_devices->num_devices--;
>> +	list_del(&device->dev_list);
>> +	mutex_unlock(&fs_devices->device_list_mutex);
>> +
>> +	rcu_string_free(device->name);
>> +	kfree(device);
> 
> Please use the new helper introduced in patch "btrfs: introduce
> free_device helper" (currently in misc-next), you'd leak the flush bio
> here.

   Ok. Will do.

Thanks, Anand

>> +
>> +	if (fs_devices->num_devices == 0) {
>> +		btrfs_sysfs_remove_fsid(fs_devices);
>> +		list_del(&fs_devices->list);
>> +		free_fs_devices(fs_devices);
>> +	}
>> +}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v4] btrfs-progs: add 'btrfs device ignore' cli

[Anand Jain]

On 12/06/2017 03:11 AM, David Sterba wrote:
> On Tue, Dec 05, 2017 at 04:52:57PM +0800, Anand Jain wrote:
>> This patch adds
>>    btrfs device ignode <dev>
>> so that a device can be ignored/missed during mount if the device is
>> already been scanned. Basically, this command will undo the effect
>> of the command
>>    btrfs device scan <dev>
> 
> As it logicall belongs to scan, I'd rather make it an option, like
> 
>    $ btrfs scan --unregister
> 
> or
> 
>    $ btrfs scan --forget
>
> The meaning of 'ignore' in this context is not clear to me, we can
> ignore errors on the device.

  Do you mean to say..

      $btrfs dev scan --unregister
   or
      $btrfs dev scan --forget

  further, as there is a suggestion to have an option to purge all
  unmounted devices, in view of adding further options, how
  about..

     $btrfs dev <sub-sub-cmd> [ <--all> | <dev-path> ]

  all:       purge all unmounted devices
  dev-path:  purge one given device

  and name choices for <sub-sub-cmd>  are:
     unscan
     unregister
     forget
     purge

  I am fine with any, will go by suggested.

Thanks, Anand

>> This change is compatible with older kernel without the ioctl
>> BTRFS_IOC_IGNORE_DEV which is added here. At which it shall
>> report 'Inappropriate ioctl for device'. And will report
>> 'Operation not permitted' on mounted device.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v1-4: No change.
>>
>>   cmds-device.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   ioctl.h       |  2 ++
>>   2 files changed, 56 insertions(+)
>>
>> diff --git a/cmds-device.c b/cmds-device.c
>> index f4cdb39f64ac..31b1945d3339 100644
>> --- a/cmds-device.c
>> +++ b/cmds-device.c
>> @@ -329,6 +329,59 @@ out:
>>   	return !!ret;
>>   }
>>   
>> +static const char * const cmd_device_ignore_usage[] = {
>> +	"btrfs device ignore [<device>]",
>> +	"Ignore a device in the btrfs kernel module.",
>> +	NULL
>> +};
>> +
>> +static int btrfs_ignore_one_device(char *path)
>> +{
>> +	struct btrfs_ioctl_vol_args args;
>> +	int fd;
>> +	int ret;
>> +
>> +	fd = open("/dev/btrfs-control", O_RDWR);
>> +	if (fd < 0)
>> +		return -errno;
>> +
>> +	memset(&args, 0, sizeof(args));
>> +	strncpy_null(args.name, path);
>> +	ret = ioctl(fd, BTRFS_IOC_IGNORE_DEV, &args);
>> +	if (ret)
>> +		ret = -errno;
>> +	close(fd);
>> +	return ret;
>> +}
>> +
>> +static int cmd_device_ignore(int argc, char **argv)
>> +{
>> +	int ret = 0;
>> +	char *path;
>> +
>> +	if (check_argc_max(argc - optind, 1))
>> +		usage(cmd_device_ignore_usage);
>> +
>> +	if (is_block_device(argv[1]) != 1) {
>> +		error("Not a block device: %s", argv[1]);
>> +		return -ENOENT;
>> +	}
>> +
>> +	path = canonicalize_path(argv[1]);
>> +	if (!path) {
>> +		error("Could not canonicalize path '%s': %s",
>> +					argv[1], strerror(errno));
>> +		return -ENOENT;
>> +	}
>> +
>> +	ret  = btrfs_ignore_one_device(path);
>> +	if (ret)
>> +		error("Can't ignore %s: %s", path, strerror(-ret));
>> +
>> +	free(path);
>> +	return ret;
>> +}
>> +
>>   static const char * const cmd_device_ready_usage[] = {
>>   	"btrfs device ready <device>",
>>   	"Check device to see if it has all of its devices in cache for mounting",
>> @@ -604,6 +657,7 @@ const struct cmd_group device_cmd_group = {
>>   			CMD_ALIAS },
>>   		{ "remove", cmd_device_remove, cmd_device_remove_usage, NULL, 0 },
>>   		{ "scan", cmd_device_scan, cmd_device_scan_usage, NULL, 0 },
>> +		{ "ignore", cmd_device_ignore, cmd_device_ignore_usage, NULL, 0 },
>>   		{ "ready", cmd_device_ready, cmd_device_ready_usage, NULL, 0 },
>>   		{ "stats", cmd_device_stats, cmd_device_stats_usage, NULL, 0 },
>>   		{ "usage", cmd_device_usage,
>> diff --git a/ioctl.h b/ioctl.h
>> index 709e996f401c..bef8508119aa 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_IGNORE_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
>> -- 
>> 2.7.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v4 2/2] btrfs: introduce feature to ignore a btrfs device

[Anand Jain]

>> This patch proposes to use ioctl #5 as it was empty.
>> 	IOW(BTRFS_IOCTL_MAGIC, 5, ..)
>> If #5 is reserved for some other purpose, I think I should change this.
> 
> I think 5 is free for use.

ok.

>> +static int device_list_remove(struct btrfs_super_block *disk_super, u64 devid)
>> +{
>> +	int ret = 0;
>> +	struct btrfs_fs_devices *fs_devices;
>> +	struct btrfs_device *device;
>> +
>> +	fs_devices = find_fsid(disk_super->fsid);
> 
> Don't we need uuid mutext to call find_fsid? All other users do that.

Actually we need it. Will fix. Thanks.

>> +int btrfs_ignore_one_device(const char *path, fmode_t flags, void *holder,
>> +			  struct btrfs_fs_devices **fs_devices_ret)
>> +{
>> +	struct btrfs_super_block *disk_super;
>> +	struct block_device *bdev;
>> +	struct page *page;
>> +	int ret = -EINVAL;
> 
> Please move EINVAL to the point where this happens (ie. after the
> btrfs_read_disk_super call). This is the common pattern and makes
> reading the code smooth.

Right.

>> +	ret = device_list_remove(disk_super, devid);
>> +	if (ret)
>> +		pr_err("BTRFS: %pU device %s devid %llu failed to ignore: %d\n",
>> +			disk_super->fsid, path, devid, ret);
> 
> So we can't easily use btrfs_printk here due to lack of fs_info that
> would appear as "<unknown>" in place of the device. Ok.

  Side topic. We should rather make btrfs_printk independent of fs_info
  by passing fs_devices, also that means use fsid instead of s_id,
  something like [1]. For the concern of long-prefixes, we need new
  ideas. I can resend RFC.

    [1]
    https://www.spinics.net/lists/linux-btrfs/msg47759.html
    [2]
    https://patchwork.kernel.org/patch/7014191/

>> +#define BTRFS_IOC_IGNORE_DEV _IOW(BTRFS_IOCTL_MAGIC, 5, \
>> +				   struct btrfs_ioctl_vol_args)
> 
> While reading the patches, I realized we may want to extend the ioctl to
> unregister/forge all devices that are not currently mounted. For that > purpose using the btrfs_ioctl_vol_args_v2 would be suitable as it has
> more struct members.
  Good point. How about using a flag to indicate to forget all unmounted
  devices BTRFS_IOCTL_PURGE_ALL_DEVS and with in btrfs_ioctl_vol_args
  as below [3], since btrfs_control_ioctl() uses only
  btrfs_ioctl_vol_args, so we decode arg before the cmd ioctl check,
  which is not a roadblock though.

[3]
------------------------------
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index de0f1144d945..eaf6ef04b300 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -28,8 +28,12 @@

  /* this should be 4k */
  #define BTRFS_PATH_NAME_MAX 4087
+#define BTRFS_IOCTL_PURGE_ALL_DEVS     (1ULL << 0)
  struct btrfs_ioctl_vol_args {
-       __s64 fd;
+       union {
+               __s64 fd;
+               __u64 ioctl_flag;
+       };
         char name[BTRFS_PATH_NAME_MAX + 1];
  };
------------------------------

  Or I am ok with btrfs_ioctl_vol_args_v2 as well.


> Another extension is to unregister only stale devices (when there's no
> device node under /dev), eg. after the device is unuplugged and readded
> by another name.

  Ok. So that means match the device-path instead of its fsid && devid.


  This part of the code is messy -- all because we don't free per fsid
  btrfs_fs_devices and btrfs_devices structures upon unmount. Any idea
  why not we free them instead ? Which means we have rerun 'btrfs dev
  scan' to mount it again. May be that was the reason. But not too sure.

Thanks, Anand

> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>