[PATCH 1/4] nbd: use our own workqueue for recv threads

[PATCH 1/4] nbd: use our own workqueue for recv threads

[Josef Bacik]

Since we are in the memory reclaim path we need our recv work to be on a
workqueue that has WQ_MEM_RECLAIM set so we can avoid deadlocks.  Also
set WQ_HIGHPRI since we are in the completion path for IO.

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 drivers/block/nbd.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 9fd06ee..9fe9763 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -91,6 +91,7 @@ static struct dentry *nbd_dbg_dir;
 static unsigned int nbds_max = 16;
 static struct nbd_device *nbd_dev;
 static int max_part;
+static struct workqueue_struct *recv_workqueue;
 
 static inline struct device *nbd_to_dev(struct nbd_device *nbd)
 {
@@ -785,7 +786,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 			INIT_WORK(&args[i].work, recv_work);
 			args[i].nbd = nbd;
 			args[i].index = i;
-			queue_work(system_long_wq, &args[i].work);
+			queue_work(recv_workqueue, &args[i].work);
 		}
 		wait_event_interruptible(nbd->recv_wq,
 					 atomic_read(&nbd->recv_threads) == 0);
@@ -1034,10 +1035,16 @@ static int __init nbd_init(void)
 
 	if (nbds_max > 1UL << (MINORBITS - part_shift))
 		return -EINVAL;
+	recv_workqueue = alloc_workqueue("knbd-recv",
+					 WQ_MEM_RECLAIM | WQ_HIGHPRI, 0);
+	if (!recv_workqueue)
+		return -ENOMEM;
 
 	nbd_dev = kcalloc(nbds_max, sizeof(*nbd_dev), GFP_KERNEL);
-	if (!nbd_dev)
+	if (!nbd_dev) {
+		destroy_workqueue(recv_workqueue);
 		return -ENOMEM;
+	}
 
 	for (i = 0; i < nbds_max; i++) {
 		struct request_queue *q;
@@ -1117,6 +1124,7 @@ static int __init nbd_init(void)
 		put_disk(nbd_dev[i].disk);
 	}
 	kfree(nbd_dev);
+	destroy_workqueue(recv_workqueue);
 	return err;
 }
 
@@ -1136,6 +1144,7 @@ static void __exit nbd_cleanup(void)
 			put_disk(disk);
 		}
 	}
+	destroy_workqueue(recv_workqueue);
 	unregister_blkdev(NBD_MAJOR, "nbd");
 	kfree(nbd_dev);
 	printk(KERN_INFO "nbd: unregistered device at major %d\n", NBD_MAJOR);
-- 
2.7.4

[PATCH 2/4] miscdevice: add a minor number for nbd-control

[Josef Bacik]

NBD is moving to a loop-control esque way of managing it's device
creation and deletion.  Reserver a minor number for the new device.

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 include/linux/miscdevice.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
index ed30d5d..7e5c140 100644
--- a/include/linux/miscdevice.h
+++ b/include/linux/miscdevice.h
@@ -44,6 +44,7 @@
 #define MISC_MCELOG_MINOR	227
 #define HPET_MINOR		228
 #define FUSE_MINOR		229
+#define NBD_CTRL_MINOR		230
 #define KVM_MINOR		232
 #define BTRFS_MINOR		234
 #define AUTOFS_MINOR		235
-- 
2.7.4

Re: [PATCH 2/4] miscdevice: add a minor number for nbd-control

[Greg KH]

On Fri, Jan 20, 2017 at 04:56:50PM -0500, Josef Bacik wrote:
> NBD is moving to a loop-control esque way of managing it's device
> creation and deletion.  Reserver a minor number for the new device.
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  include/linux/miscdevice.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
> index ed30d5d..7e5c140 100644
> --- a/include/linux/miscdevice.h
> +++ b/include/linux/miscdevice.h
> @@ -44,6 +44,7 @@
>  #define MISC_MCELOG_MINOR	227
>  #define HPET_MINOR		228
>  #define FUSE_MINOR		229
> +#define NBD_CTRL_MINOR		230

Ick, really?  Why not just use a dynamic one?  What 1990's-based systems
are you running that you would need a static misc device number for?

thanks,

greg k-h

Re: [PATCH 2/4] miscdevice: add a minor number for nbd-control

[Josef Bacik]

> On Jan 21, 2017, at 4:03 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>=20
>> On Fri, Jan 20, 2017 at 04:56:50PM -0500, Josef Bacik wrote:
>> NBD is moving to a loop-control esque way of managing it's device
>> creation and deletion.  Reserver a minor number for the new device.
>>=20
>> Signed-off-by: Josef Bacik <jbacik@fb.com>
>> ---
>> include/linux/miscdevice.h | 1 +
>> 1 file changed, 1 insertion(+)
>>=20
>> diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
>> index ed30d5d..7e5c140 100644
>> --- a/include/linux/miscdevice.h
>> +++ b/include/linux/miscdevice.h
>> @@ -44,6 +44,7 @@
>> #define MISC_MCELOG_MINOR    227
>> #define HPET_MINOR        228
>> #define FUSE_MINOR        229
>> +#define NBD_CTRL_MINOR        230
>=20
> Ick, really?  Why not just use a dynamic one?  What 1990's-based systems
> are you running that you would need a static misc device number for?
>=20

Honestly I was just doing what loop did, I wasn't sure if there was some ad=
ministration reason for needing a static minor number.  If nobody cares the=
n I can just use a dynamic one.  Thanks,

Josef=

Re: [PATCH 2/4] miscdevice: add a minor number for nbd-control

[Greg KH]

On Sat, Jan 21, 2017 at 01:25:32PM +0000, Josef Bacik wrote:
> 
> > On Jan 21, 2017, at 4:03 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> >> On Fri, Jan 20, 2017 at 04:56:50PM -0500, Josef Bacik wrote:
> >> NBD is moving to a loop-control esque way of managing it's device
> >> creation and deletion.  Reserver a minor number for the new device.
> >> 
> >> Signed-off-by: Josef Bacik <jbacik@fb.com>
> >> ---
> >> include/linux/miscdevice.h | 1 +
> >> 1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
> >> index ed30d5d..7e5c140 100644
> >> --- a/include/linux/miscdevice.h
> >> +++ b/include/linux/miscdevice.h
> >> @@ -44,6 +44,7 @@
> >> #define MISC_MCELOG_MINOR    227
> >> #define HPET_MINOR        228
> >> #define FUSE_MINOR        229
> >> +#define NBD_CTRL_MINOR        230
> > 
> > Ick, really?  Why not just use a dynamic one?  What 1990's-based systems
> > are you running that you would need a static misc device number for?
> > 
> 
> Honestly I was just doing what loop did, I wasn't sure if there was
> some administration reason for needing a static minor number.  If
> nobody cares then I can just use a dynamic one.  Thanks,

Loop is very legacy, that's why it is that way, please don't duplicate
it...

thanks,

greg k-h

[PATCH 3/4] nbd: use an idr to keep track of nbd devices

[Josef Bacik]

To prepare for dynamically adding new nbd devices to the system switch
from using an array for the nbd devices and instead use an idr.  This
copies what loop does for keeping track of its devices.

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 drivers/block/nbd.c | 213 ++++++++++++++++++++++++++++------------------------
 1 file changed, 115 insertions(+), 98 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 9fe9763..96f7681 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -41,6 +41,9 @@
 
 #include <linux/nbd.h>
 
+static DEFINE_IDR(nbd_index_idr);
+static DEFINE_MUTEX(nbd_index_mutex);
+
 struct nbd_sock {
 	struct socket *sock;
 	struct mutex tx_lock;
@@ -89,9 +92,9 @@ static struct dentry *nbd_dbg_dir;
 #define NBD_MAGIC 0x68797548
 
 static unsigned int nbds_max = 16;
-static struct nbd_device *nbd_dev;
 static int max_part;
 static struct workqueue_struct *recv_workqueue;
+static int part_shift;
 
 static inline struct device *nbd_to_dev(struct nbd_device *nbd)
 {
@@ -997,6 +1000,103 @@ static struct blk_mq_ops nbd_mq_ops = {
 	.timeout	= nbd_xmit_timeout,
 };
 
+static void nbd_dev_remove(struct nbd_device *nbd)
+{
+	struct gendisk *disk = nbd->disk;
+	nbd->magic = 0;
+	if (disk) {
+		del_gendisk(disk);
+		blk_cleanup_queue(disk->queue);
+		blk_mq_free_tag_set(&nbd->tag_set);
+		put_disk(disk);
+	}
+	kfree(nbd);
+}
+
+static int nbd_dev_add(int index)
+{
+	struct nbd_device *nbd;
+	struct gendisk *disk;
+	struct request_queue *q;
+	int err = -ENOMEM;
+
+	nbd = kzalloc(sizeof(struct nbd_device), GFP_KERNEL);
+	if (!nbd)
+		goto out;
+
+	disk = alloc_disk(1 << part_shift);
+	if (!disk)
+		goto out_free_nbd;
+
+	if (index >= 0) {
+		err = idr_alloc(&nbd_index_idr, nbd, index, index + 1,
+				GFP_KERNEL);
+		if (err == -ENOSPC)
+			err = -EEXIST;
+	} else {
+		err = idr_alloc(&nbd_index_idr, nbd, 0, 0, GFP_KERNEL);
+		if (err >= 0)
+			index = err;
+	}
+	if (err < 0)
+		goto out_free_disk;
+
+	nbd->disk = disk;
+	nbd->tag_set.ops = &nbd_mq_ops;
+	nbd->tag_set.nr_hw_queues = 1;
+	nbd->tag_set.queue_depth = 128;
+	nbd->tag_set.numa_node = NUMA_NO_NODE;
+	nbd->tag_set.cmd_size = sizeof(struct nbd_cmd);
+	nbd->tag_set.flags = BLK_MQ_F_SHOULD_MERGE |
+		BLK_MQ_F_SG_MERGE | BLK_MQ_F_BLOCKING;
+	nbd->tag_set.driver_data = nbd;
+
+	err = blk_mq_alloc_tag_set(&nbd->tag_set);
+	if (err)
+		goto out_free_idr;
+
+	q = blk_mq_init_queue(&nbd->tag_set);
+	if (IS_ERR(q)) {
+		err = PTR_ERR(q);
+		goto out_free_tags;
+	}
+	disk->queue = q;
+
+	/*
+	 * Tell the block layer that we are not a rotational device
+	 */
+	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, disk->queue);
+	queue_flag_clear_unlocked(QUEUE_FLAG_ADD_RANDOM, disk->queue);
+	disk->queue->limits.discard_granularity = 512;
+	blk_queue_max_discard_sectors(disk->queue, UINT_MAX);
+	disk->queue->limits.discard_zeroes_data = 0;
+	blk_queue_max_hw_sectors(disk->queue, 65536);
+	disk->queue->limits.max_sectors = 256;
+
+	nbd->magic = NBD_MAGIC;
+	mutex_init(&nbd->config_lock);
+	disk->major = NBD_MAJOR;
+	disk->first_minor = index << part_shift;
+	disk->fops = &nbd_fops;
+	disk->private_data = nbd;
+	sprintf(disk->disk_name, "nbd%d", index);
+	init_waitqueue_head(&nbd->recv_wq);
+	nbd_reset(nbd);
+	add_disk(disk);
+	return index;
+
+out_free_tags:
+	blk_mq_free_tag_set(&nbd->tag_set);
+out_free_idr:
+	idr_remove(&nbd_index_idr, index);
+out_free_disk:
+	put_disk(disk);
+out_free_nbd:
+	kfree(nbd);
+out:
+	return err;
+}
+
 /*
  * And here should be modules and kernel interface 
  *  (Just smiley confuses emacs :-)
@@ -1004,9 +1104,7 @@ static struct blk_mq_ops nbd_mq_ops = {
 
 static int __init nbd_init(void)
 {
-	int err = -ENOMEM;
 	int i;
-	int part_shift;
 
 	BUILD_BUG_ON(sizeof(struct nbd_request) != 28);
 
@@ -1040,114 +1138,33 @@ static int __init nbd_init(void)
 	if (!recv_workqueue)
 		return -ENOMEM;
 
-	nbd_dev = kcalloc(nbds_max, sizeof(*nbd_dev), GFP_KERNEL);
-	if (!nbd_dev) {
-		destroy_workqueue(recv_workqueue);
-		return -ENOMEM;
-	}
-
-	for (i = 0; i < nbds_max; i++) {
-		struct request_queue *q;
-		struct gendisk *disk = alloc_disk(1 << part_shift);
-		if (!disk)
-			goto out;
-		nbd_dev[i].disk = disk;
-
-		nbd_dev[i].tag_set.ops = &nbd_mq_ops;
-		nbd_dev[i].tag_set.nr_hw_queues = 1;
-		nbd_dev[i].tag_set.queue_depth = 128;
-		nbd_dev[i].tag_set.numa_node = NUMA_NO_NODE;
-		nbd_dev[i].tag_set.cmd_size = sizeof(struct nbd_cmd);
-		nbd_dev[i].tag_set.flags = BLK_MQ_F_SHOULD_MERGE |
-			BLK_MQ_F_SG_MERGE | BLK_MQ_F_BLOCKING;
-		nbd_dev[i].tag_set.driver_data = &nbd_dev[i];
-
-		err = blk_mq_alloc_tag_set(&nbd_dev[i].tag_set);
-		if (err) {
-			put_disk(disk);
-			goto out;
-		}
-
-		/*
-		 * The new linux 2.5 block layer implementation requires
-		 * every gendisk to have its very own request_queue struct.
-		 * These structs are big so we dynamically allocate them.
-		 */
-		q = blk_mq_init_queue(&nbd_dev[i].tag_set);
-		if (IS_ERR(q)) {
-			blk_mq_free_tag_set(&nbd_dev[i].tag_set);
-			put_disk(disk);
-			goto out;
-		}
-		disk->queue = q;
-
-		/*
-		 * Tell the block layer that we are not a rotational device
-		 */
-		queue_flag_set_unlocked(QUEUE_FLAG_NONROT, disk->queue);
-		queue_flag_clear_unlocked(QUEUE_FLAG_ADD_RANDOM, disk->queue);
-		disk->queue->limits.discard_granularity = 512;
-		blk_queue_max_discard_sectors(disk->queue, UINT_MAX);
-		disk->queue->limits.discard_zeroes_data = 0;
-		blk_queue_max_hw_sectors(disk->queue, 65536);
-		disk->queue->limits.max_sectors = 256;
-	}
-
-	if (register_blkdev(NBD_MAJOR, "nbd")) {
-		err = -EIO;
-		goto out;
-	}
-
-	printk(KERN_INFO "nbd: registered device at major %d\n", NBD_MAJOR);
+	if (register_blkdev(NBD_MAJOR, "nbd"))
+		return -EIO;
 
 	nbd_dbg_init();
 
-	for (i = 0; i < nbds_max; i++) {
-		struct gendisk *disk = nbd_dev[i].disk;
-		nbd_dev[i].magic = NBD_MAGIC;
-		mutex_init(&nbd_dev[i].config_lock);
-		disk->major = NBD_MAJOR;
-		disk->first_minor = i << part_shift;
-		disk->fops = &nbd_fops;
-		disk->private_data = &nbd_dev[i];
-		sprintf(disk->disk_name, "nbd%d", i);
-		init_waitqueue_head(&nbd_dev[i].recv_wq);
-		nbd_reset(&nbd_dev[i]);
-		add_disk(disk);
-	}
+	mutex_lock(&nbd_index_mutex);
+	for (i = 0; i < nbds_max; i++)
+		nbd_dev_add(i);
+	mutex_unlock(&nbd_index_mutex);
+	return 0;
+}
 
+static int nbd_exit_cb(int id, void *ptr, void *data)
+{
+	struct nbd_device *nbd = ptr;
+	nbd_dev_remove(nbd);
 	return 0;
-out:
-	while (i--) {
-		blk_mq_free_tag_set(&nbd_dev[i].tag_set);
-		blk_cleanup_queue(nbd_dev[i].disk->queue);
-		put_disk(nbd_dev[i].disk);
-	}
-	kfree(nbd_dev);
-	destroy_workqueue(recv_workqueue);
-	return err;
 }
 
 static void __exit nbd_cleanup(void)
 {
-	int i;
-
 	nbd_dbg_close();
 
-	for (i = 0; i < nbds_max; i++) {
-		struct gendisk *disk = nbd_dev[i].disk;
-		nbd_dev[i].magic = 0;
-		if (disk) {
-			del_gendisk(disk);
-			blk_cleanup_queue(disk->queue);
-			blk_mq_free_tag_set(&nbd_dev[i].tag_set);
-			put_disk(disk);
-		}
-	}
+	idr_for_each(&nbd_index_idr, &nbd_exit_cb, NULL);
+	idr_destroy(&nbd_index_idr);
 	destroy_workqueue(recv_workqueue);
 	unregister_blkdev(NBD_MAJOR, "nbd");
-	kfree(nbd_dev);
-	printk(KERN_INFO "nbd: unregistered device at major %d\n", NBD_MAJOR);
 }
 
 module_init(nbd_init);
-- 
2.7.4

[PATCH 4/4] nbd: add a nbd-control interface

[Josef Bacik]

This patch mirrors the loop back device behavior with a few changes.  First
there is no DEL operation as NBD doesn't get as much churn as loop devices do.
Secondly the GET_NEXT operation can optionally create a new NBD device or not.
Our infrastructure people want to not allow NBD to create new devices as it
causes problems for them in containers.  However allow this to be optional as
things like the OSS NBD client probably doesn't care and would like to just be
given a device to use.

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 drivers/block/nbd.c      | 129 ++++++++++++++++++++++++++++++++++++++++++-----
 include/uapi/linux/nbd.h |  10 ++++
 2 files changed, 125 insertions(+), 14 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 96f7681..c06ed36 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -35,6 +35,7 @@
 #include <linux/types.h>
 #include <linux/debugfs.h>
 #include <linux/blk-mq.h>
+#include <linux/miscdevice.h>
 
 #include <linux/uaccess.h>
 #include <asm/types.h>
@@ -59,6 +60,7 @@ struct nbd_device {
 	unsigned long runtime_flags;
 	struct nbd_sock **socks;
 	int magic;
+	int index;
 
 	struct blk_mq_tag_set tag_set;
 
@@ -1041,6 +1043,7 @@ static int nbd_dev_add(int index)
 	if (err < 0)
 		goto out_free_disk;
 
+	nbd->index = index;
 	nbd->disk = disk;
 	nbd->tag_set.ops = &nbd_mq_ops;
 	nbd->tag_set.nr_hw_queues = 1;
@@ -1097,20 +1100,109 @@ static int nbd_dev_add(int index)
 	return err;
 }
 
-/*
- * And here should be modules and kernel interface 
- *  (Just smiley confuses emacs :-)
- */
+static int find_free_cb(int id, void *ptr, void *data)
+{
+	struct nbd_device *nbd = ptr;
+	struct nbd_device **ret = data;
+
+	if (nbd->num_connections == 0) {
+		*ret = nbd;
+		return 1;
+	}
+	if (*ret == NULL || (*ret)->index < nbd->index)
+		*ret = nbd;
+	return 0;
+}
+
+static int nbd_dev_lookup(struct nbd_device **ret, int index)
+{
+	struct nbd_device *nbd = NULL;
+
+	if (index < 0) {
+		int err = idr_for_each(&nbd_index_idr, &find_free_cb, &nbd);
+		if (err != 1) {
+			if (nbd != NULL) {
+				*ret = NULL;
+				return nbd->index + 1;
+			} else {
+				return 0;
+			}
+		}
+	} else
+		nbd = idr_find(&nbd_index_idr, index);
+	if (nbd) {
+		*ret = nbd;
+		return nbd->index;
+	}
+	return -ENODEV;
+}
+
+static long nbd_control_ioctl(struct file *file, unsigned int cmd,
+			      unsigned long parm)
+{
+	struct nbd_device *nbd = NULL;
+	int ret = -ENOSYS;
+
+	mutex_lock(&nbd_index_mutex);
+	switch (cmd) {
+	case NBD_CTL_ADD:
+		ret = nbd_dev_lookup(&nbd, parm);
+		if (ret >= 0) {
+			ret = -EEXIST;
+			break;
+		}
+		ret = nbd_dev_add(parm);
+		break;
+	case NBD_CTL_GET_FREE:
+		ret = nbd_dev_lookup(&nbd, -1);
+		if (ret < 0)
+			break;
+		if (parm && !nbd) {
+			int index = ret;
+
+			ret = nbd_dev_add(ret);
+			if (ret < 0)
+				break;
+			ret = index;
+		}
+		break;
+	default:
+		break;
+	}
+	mutex_unlock(&nbd_index_mutex);
+	return ret;
+}
+
+static const struct file_operations nbd_ctl_fops = {
+	.open		= nonseekable_open,
+	.unlocked_ioctl	= nbd_control_ioctl,
+	.compat_ioctl	= nbd_control_ioctl,
+	.owner		= THIS_MODULE,
+	.llseek		= noop_llseek,
+};
+
+static struct miscdevice nbd_misc = {
+	.minor	= NBD_CTRL_MINOR,
+	.name	= "nbd-control",
+	.fops	= &nbd_ctl_fops,
+};
+
+MODULE_ALIAS_MISCDEV(NBD_CTRL_MINOR);
+MODULE_ALIAS("devname:nbd-control");
 
 static int __init nbd_init(void)
 {
-	int i;
+	int i, ret;
 
 	BUILD_BUG_ON(sizeof(struct nbd_request) != 28);
+	ret = misc_register(&nbd_misc);
+	if (ret < 0)
+		return ret;
 
+	ret = -EINVAL;
 	if (max_part < 0) {
 		printk(KERN_ERR "nbd: max_part must be >= 0\n");
-		return -EINVAL;
+		goto out_misc;
 	}
 
 	part_shift = 0;
@@ -1129,17 +1221,20 @@ static int __init nbd_init(void)
 	}
 
 	if ((1UL << part_shift) > DISK_MAX_PARTS)
-		return -EINVAL;
-
+		goto out_misc;
 	if (nbds_max > 1UL << (MINORBITS - part_shift))
-		return -EINVAL;
+		goto out_misc;
+
 	recv_workqueue = alloc_workqueue("knbd-recv",
 					 WQ_MEM_RECLAIM | WQ_HIGHPRI, 0);
-	if (!recv_workqueue)
-		return -ENOMEM;
-
-	if (register_blkdev(NBD_MAJOR, "nbd"))
-		return -EIO;
+	if (!recv_workqueue) {
+		ret = -ENOMEM;
+		goto out_misc;
+	}
+	if (register_blkdev(NBD_MAJOR, "nbd")) {
+		ret = -EIO;
+		goto out_workqueue;
+	}
 
 	nbd_dbg_init();
 
@@ -1148,6 +1243,11 @@ static int __init nbd_init(void)
 		nbd_dev_add(i);
 	mutex_unlock(&nbd_index_mutex);
 	return 0;
+out_workqueue:
+	destroy_workqueue(recv_workqueue);
+out_misc:
+	misc_deregister(&nbd_misc);
+	return ret;
 }
 
 static int nbd_exit_cb(int id, void *ptr, void *data)
@@ -1164,6 +1264,7 @@ static void __exit nbd_cleanup(void)
 	idr_for_each(&nbd_index_idr, &nbd_exit_cb, NULL);
 	idr_destroy(&nbd_index_idr);
 	destroy_workqueue(recv_workqueue);
+	misc_deregister(&nbd_misc);
 	unregister_blkdev(NBD_MAJOR, "nbd");
 }
 
diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
index c91c642..a76924b 100644
--- a/include/uapi/linux/nbd.h
+++ b/include/uapi/linux/nbd.h
@@ -29,6 +29,16 @@
 #define NBD_SET_TIMEOUT _IO( 0xab, 9 )
 #define NBD_SET_FLAGS   _IO( 0xab, 10)
 
+/* Use the last 8 values of our allocation for CTRL ioctls. */
+#define NBD_CTL_ADD		_IO( 0xab, 23)
+#define NBD_CTL_GET_FREE	_IO( 0xab, 24)
+
+enum {
+	NBD_CTL_GET_FREE_SCAN = 0, /* Get the next free available device. */
+	NBD_CTL_GET_FREE_ADD = 1, /* Get the next free available device, but
+				     create one if one does not exist. */
+};
+
 enum {
 	NBD_CMD_READ = 0,
 	NBD_CMD_WRITE = 1,
-- 
2.7.4

Re: [PATCH 4/4] nbd: add a nbd-control interface

[Greg KH]

On Fri, Jan 20, 2017 at 04:56:52PM -0500, Josef Bacik wrote:
> This patch mirrors the loop back device behavior with a few changes.  First
> there is no DEL operation as NBD doesn't get as much churn as loop devices do.
> Secondly the GET_NEXT operation can optionally create a new NBD device or not.
> Our infrastructure people want to not allow NBD to create new devices as it
> causes problems for them in containers.  However allow this to be optional as
> things like the OSS NBD client probably doesn't care and would like to just be
> given a device to use.
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>

A random char device with odd ioctls?  Why?  There's no other
configuration choice you could possibly use?  Where is the userspace
tool that uses this new kernel api?

You aren't passing in structures to the ioctl, so why does this HAVE to
be an ioctl?

thanks,

greg k-h

Re: [PATCH 4/4] nbd: add a nbd-control interface

[Josef Bacik]

On Sat, Jan 21, 2017 at 4:05 AM, Greg KH <gregkh@linuxfoundation.org> 
wrote:
> On Fri, Jan 20, 2017 at 04:56:52PM -0500, Josef Bacik wrote:
>>  This patch mirrors the loop back device behavior with a few 
>> changes.  First
>>  there is no DEL operation as NBD doesn't get as much churn as loop 
>> devices do.
>>  Secondly the GET_NEXT operation can optionally create a new NBD 
>> device or not.
>>  Our infrastructure people want to not allow NBD to create new 
>> devices as it
>>  causes problems for them in containers.  However allow this to be 
>> optional as
>>  things like the OSS NBD client probably doesn't care and would like 
>> to just be
>>  given a device to use.
>> 
>>  Signed-off-by: Josef Bacik <jbacik@fb.com>
> 
> A random char device with odd ioctls?  Why?  There's no other
> configuration choice you could possibly use?  Where is the userspace
> tool that uses this new kernel api?
> 
> You aren't passing in structures to the ioctl, so why does this HAVE 
> to
> be an ioctl?

Again, this is how loop does it so I assumed a known, regularly used 
API was the best bet.  I can do literally anything, but these 
interfaces have to be used by other people, including internal people.  
The /dev/whatever-control is a well established way for interacting 
with dynamic device drivers (loop, DM, btrfs), so that's what I went 
with.  Thanks,

Josef

Re: [PATCH 4/4] nbd: add a nbd-control interface

[Greg KH]

On Mon, Jan 23, 2017 at 09:42:08AM -0500, Josef Bacik wrote:
> 
> 
> On Sat, Jan 21, 2017 at 4:05 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Jan 20, 2017 at 04:56:52PM -0500, Josef Bacik wrote:
> > >  This patch mirrors the loop back device behavior with a few
> > > changes.  First
> > >  there is no DEL operation as NBD doesn't get as much churn as loop
> > > devices do.
> > >  Secondly the GET_NEXT operation can optionally create a new NBD
> > > device or not.
> > >  Our infrastructure people want to not allow NBD to create new
> > > devices as it
> > >  causes problems for them in containers.  However allow this to be
> > > optional as
> > >  things like the OSS NBD client probably doesn't care and would like
> > > to just be
> > >  given a device to use.
> > > 
> > >  Signed-off-by: Josef Bacik <jbacik@fb.com>
> > 
> > A random char device with odd ioctls?  Why?  There's no other
> > configuration choice you could possibly use?  Where is the userspace
> > tool that uses this new kernel api?
> > 
> > You aren't passing in structures to the ioctl, so why does this HAVE to
> > be an ioctl?
> 
> Again, this is how loop does it so I assumed a known, regularly used API was
> the best bet.  I can do literally anything, but these interfaces have to be
> used by other people, including internal people.  The /dev/whatever-control
> is a well established way for interacting with dynamic device drivers (loop,
> DM, btrfs), so that's what I went with.  Thanks,

Again, please don't duplicate what loop did, we must _learn_ from
history, not repeat it :(

Re: [PATCH 4/4] nbd: add a nbd-control interface

[Josef Bacik]

On Mon, Jan 23, 2017 at 9:52 AM, Greg KH <gregkh@linuxfoundation.org> 
wrote:
> On Mon, Jan 23, 2017 at 09:42:08AM -0500, Josef Bacik wrote:
>> 
>> 
>>  On Sat, Jan 21, 2017 at 4:05 AM, Greg KH 
>> <gregkh@linuxfoundation.org> wrote:
>>  > On Fri, Jan 20, 2017 at 04:56:52PM -0500, Josef Bacik wrote:
>>  > >  This patch mirrors the loop back device behavior with a few
>>  > > changes.  First
>>  > >  there is no DEL operation as NBD doesn't get as much churn as 
>> loop
>>  > > devices do.
>>  > >  Secondly the GET_NEXT operation can optionally create a new NBD
>>  > > device or not.
>>  > >  Our infrastructure people want to not allow NBD to create new
>>  > > devices as it
>>  > >  causes problems for them in containers.  However allow this to 
>> be
>>  > > optional as
>>  > >  things like the OSS NBD client probably doesn't care and would 
>> like
>>  > > to just be
>>  > >  given a device to use.
>>  > >
>>  > >  Signed-off-by: Josef Bacik <jbacik@fb.com>
>>  >
>>  > A random char device with odd ioctls?  Why?  There's no other
>>  > configuration choice you could possibly use?  Where is the 
>> userspace
>>  > tool that uses this new kernel api?
>>  >
>>  > You aren't passing in structures to the ioctl, so why does this 
>> HAVE to
>>  > be an ioctl?
>> 
>>  Again, this is how loop does it so I assumed a known, regularly 
>> used API was
>>  the best bet.  I can do literally anything, but these interfaces 
>> have to be
>>  used by other people, including internal people.  The 
>> /dev/whatever-control
>>  is a well established way for interacting with dynamic device 
>> drivers (loop,
>>  DM, btrfs), so that's what I went with.  Thanks,
> 
> Again, please don't duplicate what loop did, we must _learn_ from
> history, not repeat it :(

Sure but what am I supposed to do?  Have some random sysfs knobs?  
Thanks,

Josef

Re: [PATCH 4/4] nbd: add a nbd-control interface

[Greg KH]

On Mon, Jan 23, 2017 at 09:57:52AM -0500, Josef Bacik wrote:
> On Mon, Jan 23, 2017 at 9:52 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Jan 23, 2017 at 09:42:08AM -0500, Josef Bacik wrote:
> > > 
> > > 
> > >  On Sat, Jan 21, 2017 at 4:05 AM, Greg KH
> > > <gregkh@linuxfoundation.org> wrote:
> > >  > On Fri, Jan 20, 2017 at 04:56:52PM -0500, Josef Bacik wrote:
> > >  > >  This patch mirrors the loop back device behavior with a few
> > >  > > changes.  First
> > >  > >  there is no DEL operation as NBD doesn't get as much churn as
> > > loop
> > >  > > devices do.
> > >  > >  Secondly the GET_NEXT operation can optionally create a new NBD
> > >  > > device or not.
> > >  > >  Our infrastructure people want to not allow NBD to create new
> > >  > > devices as it
> > >  > >  causes problems for them in containers.  However allow this to
> > > be
> > >  > > optional as
> > >  > >  things like the OSS NBD client probably doesn't care and would
> > > like
> > >  > > to just be
> > >  > >  given a device to use.
> > >  > >
> > >  > >  Signed-off-by: Josef Bacik <jbacik@fb.com>
> > >  >
> > >  > A random char device with odd ioctls?  Why?  There's no other
> > >  > configuration choice you could possibly use?  Where is the
> > > userspace
> > >  > tool that uses this new kernel api?
> > >  >
> > >  > You aren't passing in structures to the ioctl, so why does this
> > > HAVE to
> > >  > be an ioctl?
> > > 
> > >  Again, this is how loop does it so I assumed a known, regularly
> > > used API was
> > >  the best bet.  I can do literally anything, but these interfaces
> > > have to be
> > >  used by other people, including internal people.  The
> > > /dev/whatever-control
> > >  is a well established way for interacting with dynamic device
> > > drivers (loop,
> > >  DM, btrfs), so that's what I went with.  Thanks,
> > 
> > Again, please don't duplicate what loop did, we must _learn_ from
> > history, not repeat it :(
> 
> Sure but what am I supposed to do?  Have some random sysfs knobs?  Thanks,

It all depends on what you are trying to do.  I have yet to figure that
out at all here :(

Re: [PATCH 4/4] nbd: add a nbd-control interface

[Josef Bacik]

On Mon, Jan 23, 2017 at 10:03 AM, Greg KH <gregkh@linuxfoundation.org> 
wrote:
> On Mon, Jan 23, 2017 at 09:57:52AM -0500, Josef Bacik wrote:
>>  On Mon, Jan 23, 2017 at 9:52 AM, Greg KH 
>> <gregkh@linuxfoundation.org> wrote:
>>  > On Mon, Jan 23, 2017 at 09:42:08AM -0500, Josef Bacik wrote:
>>  > >
>>  > >
>>  > >  On Sat, Jan 21, 2017 at 4:05 AM, Greg KH
>>  > > <gregkh@linuxfoundation.org> wrote:
>>  > >  > On Fri, Jan 20, 2017 at 04:56:52PM -0500, Josef Bacik wrote:
>>  > >  > >  This patch mirrors the loop back device behavior with a 
>> few
>>  > >  > > changes.  First
>>  > >  > >  there is no DEL operation as NBD doesn't get as much 
>> churn as
>>  > > loop
>>  > >  > > devices do.
>>  > >  > >  Secondly the GET_NEXT operation can optionally create a 
>> new NBD
>>  > >  > > device or not.
>>  > >  > >  Our infrastructure people want to not allow NBD to create 
>> new
>>  > >  > > devices as it
>>  > >  > >  causes problems for them in containers.  However allow 
>> this to
>>  > > be
>>  > >  > > optional as
>>  > >  > >  things like the OSS NBD client probably doesn't care and 
>> would
>>  > > like
>>  > >  > > to just be
>>  > >  > >  given a device to use.
>>  > >  > >
>>  > >  > >  Signed-off-by: Josef Bacik <jbacik@fb.com>
>>  > >  >
>>  > >  > A random char device with odd ioctls?  Why?  There's no other
>>  > >  > configuration choice you could possibly use?  Where is the
>>  > > userspace
>>  > >  > tool that uses this new kernel api?
>>  > >  >
>>  > >  > You aren't passing in structures to the ioctl, so why does 
>> this
>>  > > HAVE to
>>  > >  > be an ioctl?
>>  > >
>>  > >  Again, this is how loop does it so I assumed a known, regularly
>>  > > used API was
>>  > >  the best bet.  I can do literally anything, but these 
>> interfaces
>>  > > have to be
>>  > >  used by other people, including internal people.  The
>>  > > /dev/whatever-control
>>  > >  is a well established way for interacting with dynamic device
>>  > > drivers (loop,
>>  > >  DM, btrfs), so that's what I went with.  Thanks,
>>  >
>>  > Again, please don't duplicate what loop did, we must _learn_ from
>>  > history, not repeat it :(
>> 
>>  Sure but what am I supposed to do?  Have some random sysfs knobs?  
>> Thanks,
> 
> It all depends on what you are trying to do.  I have yet to figure 
> that
> out at all here :(

I explained it in the changelog and my response to Wouter.  NBD 
preallocates all of its /dev/nbd# devices at modprobe time, so there's 
no way to add new devices as we need them.  Loop accomplishes this with 
the /dev/loop-control and an ioctl.  Then we also need a way to figure 
out what is the first /dev/nbd# device that isn't currently in use in 
order to pick the next one to configure.  Keep in mind that all of the 
configuration for /dev/nbd# devices is done through ioctls to those 
devices, so having a ioctl interface for the control device is 
consistent with the rest of how NBD works.  Thanks,

Josef

Re: [PATCH 4/4] nbd: add a nbd-control interface

[Greg KH]

On Mon, Jan 23, 2017 at 10:52:42AM -0500, Josef Bacik wrote:
> On Mon, Jan 23, 2017 at 10:03 AM, Greg KH <gregkh@linuxfoundation.org>
> wrote:
> > On Mon, Jan 23, 2017 at 09:57:52AM -0500, Josef Bacik wrote:
> > >  On Mon, Jan 23, 2017 at 9:52 AM, Greg KH
> > > <gregkh@linuxfoundation.org> wrote:
> > >  > On Mon, Jan 23, 2017 at 09:42:08AM -0500, Josef Bacik wrote:
> > >  > >
> > >  > >
> > >  > >  On Sat, Jan 21, 2017 at 4:05 AM, Greg KH
> > >  > > <gregkh@linuxfoundation.org> wrote:
> > >  > >  > On Fri, Jan 20, 2017 at 04:56:52PM -0500, Josef Bacik wrote:
> > >  > >  > >  This patch mirrors the loop back device behavior with a
> > > few
> > >  > >  > > changes.  First
> > >  > >  > >  there is no DEL operation as NBD doesn't get as much
> > > churn as
> > >  > > loop
> > >  > >  > > devices do.
> > >  > >  > >  Secondly the GET_NEXT operation can optionally create a
> > > new NBD
> > >  > >  > > device or not.
> > >  > >  > >  Our infrastructure people want to not allow NBD to create
> > > new
> > >  > >  > > devices as it
> > >  > >  > >  causes problems for them in containers.  However allow
> > > this to
> > >  > > be
> > >  > >  > > optional as
> > >  > >  > >  things like the OSS NBD client probably doesn't care and
> > > would
> > >  > > like
> > >  > >  > > to just be
> > >  > >  > >  given a device to use.
> > >  > >  > >
> > >  > >  > >  Signed-off-by: Josef Bacik <jbacik@fb.com>
> > >  > >  >
> > >  > >  > A random char device with odd ioctls?  Why?  There's no other
> > >  > >  > configuration choice you could possibly use?  Where is the
> > >  > > userspace
> > >  > >  > tool that uses this new kernel api?
> > >  > >  >
> > >  > >  > You aren't passing in structures to the ioctl, so why does
> > > this
> > >  > > HAVE to
> > >  > >  > be an ioctl?
> > >  > >
> > >  > >  Again, this is how loop does it so I assumed a known, regularly
> > >  > > used API was
> > >  > >  the best bet.  I can do literally anything, but these
> > > interfaces
> > >  > > have to be
> > >  > >  used by other people, including internal people.  The
> > >  > > /dev/whatever-control
> > >  > >  is a well established way for interacting with dynamic device
> > >  > > drivers (loop,
> > >  > >  DM, btrfs), so that's what I went with.  Thanks,
> > >  >
> > >  > Again, please don't duplicate what loop did, we must _learn_ from
> > >  > history, not repeat it :(
> > > 
> > >  Sure but what am I supposed to do?  Have some random sysfs knobs?
> > > Thanks,
> > 
> > It all depends on what you are trying to do.  I have yet to figure that
> > out at all here :(
> 
> I explained it in the changelog and my response to Wouter.  NBD preallocates
> all of its /dev/nbd# devices at modprobe time, so there's no way to add new
> devices as we need them.

Why not fix that odd restriction?

> Loop accomplishes this with the /dev/loop-control
> and an ioctl.  Then we also need a way to figure out what is the first
> /dev/nbd# device that isn't currently in use in order to pick the next one
> to configure.  Keep in mind that all of the configuration for /dev/nbd#
> devices is done through ioctls to those devices, so having a ioctl interface
> for the control device is consistent with the rest of how NBD works.

But adding a random char device node and using ioctls on it is different
than using an ioctl on an existing block device node that is already
there.

So what _exactly_ do you need to do with this interface?  What data do
you need sent/received to/from the kernel?  Specifics please.

thanks,

greg k-h

Re: [Nbd] [PATCH 4/4] nbd: add a nbd-control interface

[Wouter Verhelst]

On Tue, Jan 24, 2017 at 08:11:52AM +0100, Greg KH wrote:
> On Mon, Jan 23, 2017 at 10:52:42AM -0500, Josef Bacik wrote:
> > I explained it in the changelog and my response to Wouter.  NBD preallocates
> > all of its /dev/nbd# devices at modprobe time, so there's no way to add new
> > devices as we need them.
> 
> Why not fix that odd restriction?

Isn't that what this patch is trying to do?

> > Loop accomplishes this with the /dev/loop-control
> > and an ioctl.  Then we also need a way to figure out what is the first
> > /dev/nbd# device that isn't currently in use in order to pick the next one
> > to configure.  Keep in mind that all of the configuration for /dev/nbd#
> > devices is done through ioctls to those devices, so having a ioctl interface
> > for the control device is consistent with the rest of how NBD works.
> 
> But adding a random char device node and using ioctls on it is different
> than using an ioctl on an existing block device node that is already
> there.
> 
> So what _exactly_ do you need to do with this interface?  What data do
> you need sent/received to/from the kernel?  Specifics please.

AIUI: Create new devices, and figure out which device is the next one
free so we can find one without having to check the pid attribute for
every device (as is needed today, and which is racy).

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

Re: [PATCH 4/4] nbd: add a nbd-control interface

[Josef Bacik]

On Tue, Jan 24, 2017 at 2:11 AM, Greg KH <gregkh@linuxfoundation.org> 
wrote:
> On Mon, Jan 23, 2017 at 10:52:42AM -0500, Josef Bacik wrote:
>>  On Mon, Jan 23, 2017 at 10:03 AM, Greg KH 
>> <gregkh@linuxfoundation.org>
>>  wrote:
>>  > On Mon, Jan 23, 2017 at 09:57:52AM -0500, Josef Bacik wrote:
>>  > >  On Mon, Jan 23, 2017 at 9:52 AM, Greg KH
>>  > > <gregkh@linuxfoundation.org> wrote:
>>  > >  > On Mon, Jan 23, 2017 at 09:42:08AM -0500, Josef Bacik wrote:
>>  > >  > >
>>  > >  > >
>>  > >  > >  On Sat, Jan 21, 2017 at 4:05 AM, Greg KH
>>  > >  > > <gregkh@linuxfoundation.org> wrote:
>>  > >  > >  > On Fri, Jan 20, 2017 at 04:56:52PM -0500, Josef Bacik 
>> wrote:
>>  > >  > >  > >  This patch mirrors the loop back device behavior 
>> with a
>>  > > few
>>  > >  > >  > > changes.  First
>>  > >  > >  > >  there is no DEL operation as NBD doesn't get as much
>>  > > churn as
>>  > >  > > loop
>>  > >  > >  > > devices do.
>>  > >  > >  > >  Secondly the GET_NEXT operation can optionally 
>> create a
>>  > > new NBD
>>  > >  > >  > > device or not.
>>  > >  > >  > >  Our infrastructure people want to not allow NBD to 
>> create
>>  > > new
>>  > >  > >  > > devices as it
>>  > >  > >  > >  causes problems for them in containers.  However 
>> allow
>>  > > this to
>>  > >  > > be
>>  > >  > >  > > optional as
>>  > >  > >  > >  things like the OSS NBD client probably doesn't care 
>> and
>>  > > would
>>  > >  > > like
>>  > >  > >  > > to just be
>>  > >  > >  > >  given a device to use.
>>  > >  > >  > >
>>  > >  > >  > >  Signed-off-by: Josef Bacik <jbacik@fb.com>
>>  > >  > >  >
>>  > >  > >  > A random char device with odd ioctls?  Why?  There's no 
>> other
>>  > >  > >  > configuration choice you could possibly use?  Where is 
>> the
>>  > >  > > userspace
>>  > >  > >  > tool that uses this new kernel api?
>>  > >  > >  >
>>  > >  > >  > You aren't passing in structures to the ioctl, so why 
>> does
>>  > > this
>>  > >  > > HAVE to
>>  > >  > >  > be an ioctl?
>>  > >  > >
>>  > >  > >  Again, this is how loop does it so I assumed a known, 
>> regularly
>>  > >  > > used API was
>>  > >  > >  the best bet.  I can do literally anything, but these
>>  > > interfaces
>>  > >  > > have to be
>>  > >  > >  used by other people, including internal people.  The
>>  > >  > > /dev/whatever-control
>>  > >  > >  is a well established way for interacting with dynamic 
>> device
>>  > >  > > drivers (loop,
>>  > >  > >  DM, btrfs), so that's what I went with.  Thanks,
>>  > >  >
>>  > >  > Again, please don't duplicate what loop did, we must _learn_ 
>> from
>>  > >  > history, not repeat it :(
>>  > >
>>  > >  Sure but what am I supposed to do?  Have some random sysfs 
>> knobs?
>>  > > Thanks,
>>  >
>>  > It all depends on what you are trying to do.  I have yet to 
>> figure that
>>  > out at all here :(
>> 
>>  I explained it in the changelog and my response to Wouter.  NBD 
>> preallocates
>>  all of its /dev/nbd# devices at modprobe time, so there's no way to 
>> add new
>>  devices as we need them.
> 
> Why not fix that odd restriction?

That's the entire point of this patchset, adding the ability to create 
new devices on the fly as needed.

> 
> 
>>  Loop accomplishes this with the /dev/loop-control
>>  and an ioctl.  Then we also need a way to figure out what is the 
>> first
>>  /dev/nbd# device that isn't currently in use in order to pick the 
>> next one
>>  to configure.  Keep in mind that all of the configuration for 
>> /dev/nbd#
>>  devices is done through ioctls to those devices, so having a ioctl 
>> interface
>>  for the control device is consistent with the rest of how NBD works.
> 
> But adding a random char device node and using ioctls on it is 
> different
> than using an ioctl on an existing block device node that is already
> there.
> 
> So what _exactly_ do you need to do with this interface?  What data do
> you need sent/received to/from the kernel?  Specifics please.

I need to say "Hey kernel, can you setup some new nbd devices for me?" 
and "Hey kernel, what is the first free nbd device I can use for this 
new connection and (optionally) create a new device for me if one does 
not exist?"  Loop accomplished this (in 2011 btw, not some far off 
date) with /dev/loop-control.  Btrfs has it's scanning stuff controlled 
by /dev/btrfs-control.  Device mapper has /dev/mapper/control.  This is 
a well established and widely used way for control block based device 
drivers.  Thanks,

Josef

Re: [PATCH 4/4] nbd: add a nbd-control interface

[Arnd Bergmann]

On Wed, Jan 25, 2017 at 2:47 PM, Josef Bacik <jbacik@fb.com> wrote:
> On Tue, Jan 24, 2017 at 2:11 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Mon, Jan 23, 2017 at 10:52:42AM -0500, Josef Bacik wrote:
>>>  On Mon, Jan 23, 2017 at 10:03 AM, Greg KH <gregkh@linuxfoundation.org>
>>>  Loop accomplishes this with the /dev/loop-control
>>>  and an ioctl.  Then we also need a way to figure out what is the first
>>>  /dev/nbd# device that isn't currently in use in order to pick the next
>>> one
>>>  to configure.  Keep in mind that all of the configuration for /dev/nbd#
>>>  devices is done through ioctls to those devices, so having a ioctl
>>> interface
>>>  for the control device is consistent with the rest of how NBD works.
>>
>>
>> But adding a random char device node and using ioctls on it is different
>> than using an ioctl on an existing block device node that is already
>> there.
>>
>> So what _exactly_ do you need to do with this interface?  What data do
>> you need sent/received to/from the kernel?  Specifics please.
>
>
> I need to say "Hey kernel, can you setup some new nbd devices for me?" and
> "Hey kernel, what is the first free nbd device I can use for this new
> connection and (optionally) create a new device for me if one does not
> exist?"  Loop accomplished this (in 2011 btw, not some far off date) with
> /dev/loop-control.  Btrfs has it's scanning stuff controlled by
> /dev/btrfs-control.  Device mapper has /dev/mapper/control.  This is a well
> established and widely used way for control block based device drivers.

We have multiple established ways to deal with this kind of problem, the most
common ones being a separate chardev as you propose, a sysfs interface and
netlink.

They all have their own set of problems, but the needs of nbd as a network
storage interface seem most closely resemble what we have for other network
related interfaces, where we typically use netlink to do the setup, see e.g.
macvtap as an example for a network chardev interface.

Can you have a look if that would solve your problem more nicely?

     Arnd

Re: [PATCH 4/4] nbd: add a nbd-control interface

[Alex Gartrell]

T24gMS8yNS8xNywgNjoyMyBBTSwgImFybmRiZXJnbWFubkBnbWFpbC5jb20gb24gYmVoYWxmIG9m
IEFybmQgQmVyZ21hbm4iIDxhcm5kYmVyZ21hbm5AZ21haWwuY29tIG9uIGJlaGFsZiBvZiBhcm5k
QGFybmRiLmRlPiB3cm90ZToNCj4gT24gV2VkLCBKYW4gMjUsIDIwMTcgYXQgMjo0NyBQTSwgSm9z
ZWYgQmFjaWsgPGpiYWNpa0BmYi5jb20+IHdyb3RlOg0KPiA+IE9uIFR1ZSwgSmFuIDI0LCAyMDE3
IGF0IDI6MTEgQU0sIEdyZWcgS0ggPGdyZWdraEBsaW51eGZvdW5kYXRpb24ub3JnPiB3cm90ZToN
Cj4gPj4gT24gTW9uLCBKYW4gMjMsIDIwMTcgYXQgMTA6NTI6NDJBTSAtMDUwMCwgSm9zZWYgQmFj
aWsgd3JvdGU6DQo+ID4+PiAgT24gTW9uLCBKYW4gMjMsIDIwMTcgYXQgMTA6MDMgQU0sIEdyZWcg
S0ggPGdyZWdraEBsaW51eGZvdW5kYXRpb24ub3JnPg0KPiA+Pj4gIExvb3AgYWNjb21wbGlzaGVz
IHRoaXMgd2l0aCB0aGUgL2Rldi9sb29wLWNvbnRyb2wNCj4gPj4+ICBhbmQgYW4gaW9jdGwuICBU
aGVuIHdlIGFsc28gbmVlZCBhIHdheSB0byBmaWd1cmUgb3V0IHdoYXQgaXMgdGhlIGZpcnN0DQo+
ID4+PiAgL2Rldi9uYmQjIGRldmljZSB0aGF0IGlzbid0IGN1cnJlbnRseSBpbiB1c2UgaW4gb3Jk
ZXIgdG8gcGljayB0aGUgbmV4dA0KPiA+Pj4gb25lDQo+ID4+PiAgdG8gY29uZmlndXJlLiAgS2Vl
cCBpbiBtaW5kIHRoYXQgYWxsIG9mIHRoZSBjb25maWd1cmF0aW9uIGZvciAvZGV2L25iZCMNCj4g
Pj4+ICBkZXZpY2VzIGlzIGRvbmUgdGhyb3VnaCBpb2N0bHMgdG8gdGhvc2UgZGV2aWNlcywgc28g
aGF2aW5nIGEgaW9jdGwNCj4gPj4+IGludGVyZmFjZQ0KPiA+Pj4gIGZvciB0aGUgY29udHJvbCBk
ZXZpY2UgaXMgY29uc2lzdGVudCB3aXRoIHRoZSByZXN0IG9mIGhvdyBOQkQgd29ya3MuDQo+ID4+
DQo+ID4+DQo+ID4+IEJ1dCBhZGRpbmcgYSByYW5kb20gY2hhciBkZXZpY2Ugbm9kZSBhbmQgdXNp
bmcgaW9jdGxzIG9uIGl0IGlzIGRpZmZlcmVudA0KPiA+PiB0aGFuIHVzaW5nIGFuIGlvY3RsIG9u
IGFuIGV4aXN0aW5nIGJsb2NrIGRldmljZSBub2RlIHRoYXQgaXMgYWxyZWFkeQ0KPiA+PiB0aGVy
ZS4NCj4gPj4NCj4gPj4gU28gd2hhdCBfZXhhY3RseV8gZG8geW91IG5lZWQgdG8gZG8gd2l0aCB0
aGlzIGludGVyZmFjZT8gIFdoYXQgZGF0YSBkbw0KPiA+PiB5b3UgbmVlZCBzZW50L3JlY2VpdmVk
IHRvL2Zyb20gdGhlIGtlcm5lbD8gIFNwZWNpZmljcyBwbGVhc2UuDQo+ID4NCj4gPg0KPiA+IEkg
bmVlZCB0byBzYXkgIkhleSBrZXJuZWwsIGNhbiB5b3Ugc2V0dXAgc29tZSBuZXcgbmJkIGRldmlj
ZXMgZm9yIG1lPyIgYW5kDQo+ID4gIkhleSBrZXJuZWwsIHdoYXQgaXMgdGhlIGZpcnN0IGZyZWUg
bmJkIGRldmljZSBJIGNhbiB1c2UgZm9yIHRoaXMgbmV3DQo+ID4gY29ubmVjdGlvbiBhbmQgKG9w
dGlvbmFsbHkpIGNyZWF0ZSBhIG5ldyBkZXZpY2UgZm9yIG1lIGlmIG9uZSBkb2VzIG5vdA0KPiA+
IGV4aXN0PyIgIExvb3AgYWNjb21wbGlzaGVkIHRoaXMgKGluIDIwMTEgYnR3LCBub3Qgc29tZSBm
YXIgb2ZmIGRhdGUpIHdpdGgNCj4gPiAvZGV2L2xvb3AtY29udHJvbC4gIEJ0cmZzIGhhcyBpdCdz
IHNjYW5uaW5nIHN0dWZmIGNvbnRyb2xsZWQgYnkNCj4gPiAvZGV2L2J0cmZzLWNvbnRyb2wuICBE
ZXZpY2UgbWFwcGVyIGhhcyAvZGV2L21hcHBlci9jb250cm9sLiAgVGhpcyBpcyBhIHdlbGwNCj4g
PiBlc3RhYmxpc2hlZCBhbmQgd2lkZWx5IHVzZWQgd2F5IGZvciBjb250cm9sIGJsb2NrIGJhc2Vk
IGRldmljZSBkcml2ZXJzLg0KPiANCj4gV2UgaGF2ZSBtdWx0aXBsZSBlc3RhYmxpc2hlZCB3YXlz
IHRvIGRlYWwgd2l0aCB0aGlzIGtpbmQgb2YgcHJvYmxlbSwgdGhlIG1vc3QNCj4gY29tbW9uIG9u
ZXMgYmVpbmcgYSBzZXBhcmF0ZSBjaGFyZGV2IGFzIHlvdSBwcm9wb3NlLCBhIHN5c2ZzIGludGVy
ZmFjZSBhbmQNCj4gbmV0bGluay4NCj4gDQo+IFRoZXkgYWxsIGhhdmUgdGhlaXIgb3duIHNldCBv
ZiBwcm9ibGVtcywgYnV0IHRoZSBuZWVkcyBvZiBuYmQgYXMgYSBuZXR3b3JrDQo+IHN0b3JhZ2Ug
aW50ZXJmYWNlIHNlZW0gbW9zdCBjbG9zZWx5IHJlc2VtYmxlIHdoYXQgd2UgaGF2ZSBmb3Igb3Ro
ZXIgbmV0d29yaw0KPiByZWxhdGVkIGludGVyZmFjZXMsIHdoZXJlIHdlIHR5cGljYWxseSB1c2Ug
bmV0bGluayB0byBkbyB0aGUgc2V0dXAsIHNlZSBlLmcuDQo+IG1hY3Z0YXAgYXMgYW4gZXhhbXBs
ZSBmb3IgYSBuZXR3b3JrIGNoYXJkZXYgaW50ZXJmYWNlLg0KPiANCj4gQ2FuIHlvdSBoYXZlIGEg
bG9vayBpZiB0aGF0IHdvdWxkIHNvbHZlIHlvdXIgcHJvYmxlbSBtb3JlIG5pY2VseT8NCg0KRldJ
VywgSSdtIHRoZSBvbmUgY29uc3VtaW5nIHRoaXMgbmJkIHN0dWZmIGF0IEZhY2Vib29rIGFuZCBi
cmluZ2luZw0KbmV0bGluayBpbnRvIHRoaXMgd291bGQgYmUgYSBodWdlIHBhaW4gZm9yIG1lLiAg
SXQncyB2ZXJ5IHdlaXJkIHRvDQpuZWVkIHRvIHB1bGwgc29ja2V0cyBpbiBhbmQgdGhlbiBkcm9w
IGJhY2sgdG8gaW9jdGxzIGZyb20gYSBkZXNpZ24NCnBlcnNwZWN0aXZlLCBhbmQgZnV0dXJlIGNv
bnN1bWVycyBvZiB0aGlzIEFQSSB3b3VsZCBiZSBzYWQgYXMgd2VsbC4NClRoaXMgaXMgY29tcG91
bmRlZCwgSSB0aGluaywgYnkgdGhlIGZhY3QgdGhhdCB0aGUgYnJlYWR0aCBvZg0KZnVuY3Rpb25h
bGl0eSBoZXJlIGRvZXNuJ3QgcmVhbGx5IG1lcml0IGEgc2hhcmVkIGxpYnJhcnkgZm9yIGV2ZXJ5
b25lDQp0byB1c2UgLS0gY291bGQgeW91IGltYWdpbmUgbGlibmJkY29udHJvbCwgd2hpY2ggZXhw
b3NlcyBhIHNpbmdsZQ0KImdldF9uZXh0X2RldmljZSIgZnVuY3Rpb24/DQoNCklmIG5iZCB3ZXJl
ICphbGwqIG5ldGxpbmsgSSB0aGluayB0aGF0IHRoYXQnZCBiZSBmaW5lLCBidXQgeW91J2QgaGF2
ZQ0KcHJvYmxlbXMgaW1wbGVtZW50aW5nIHRoZSBOQkRfRE9JVCBmdW5jdGlvbiBpbiB0aGF0IGZh
c2hpb24uICBTbyBJJ2QNCnJhdGhlciBzdGljayB0byB0aGUgY2hhciBkZXZpY2UgaW9jdGwgdGhp
bmcgYmVjYXVzZSBpdCdzIG1vcmUNCmNvbnNpc3RlbnQgd2l0aCB0aGUgb2xkIE5CRCBzdHVmZiBh
cyB3ZWxsIGFzIHRoZSBsb29wIGRldmljZSBzdHVmZi4NCg0KLS0gDQpBbGV4IEdhcnRyZWxsIDxh
Z2FydHJlbGxAZmIuY29tPg0KDQo=

Re: [Nbd] [PATCH 4/4] nbd: add a nbd-control interface

[Wouter Verhelst]

On Wed, Jan 25, 2017 at 04:48:27PM +0000, Alex Gartrell wrote:
> On 1/25/17, 6:23 AM, "arndbergmann@gmail.com on behalf of Arnd Bergmann" <arndbergmann@gmail.com on behalf of arnd@arndb.de> wrote:
> > We have multiple established ways to deal with this kind of problem, the most
> > common ones being a separate chardev as you propose, a sysfs interface and
> > netlink.
> > 
> > They all have their own set of problems, but the needs of nbd as a network
> > storage interface seem most closely resemble what we have for other network
> > related interfaces, where we typically use netlink to do the setup, see e.g.
> > macvtap as an example for a network chardev interface.
> > 
> > Can you have a look if that would solve your problem more nicely?
> 
> FWIW, I'm the one consuming this nbd stuff at Facebook and bringing
> netlink into this would be a huge pain for me.  It's very weird to
> need to pull sockets in and then drop back to ioctls from a design
> perspective, and future consumers of this API would be sad as well.

As who's been maintaining the official userspace nbd client for 15 years
now, I have to concur. NBD has had an API that deals with /dev/nbdX
since forever, adding other APIs to that just feels wrong.

> This is compounded, I think, by the fact that the breadth of
> functionality here doesn't really merit a shared library for everyone
> to use -- could you imagine libnbdcontrol, which exposes a single
> "get_next_device" function?

Please no :)

> If nbd were *all* netlink I think that that'd be fine, but you'd have
> problems implementing the NBD_DOIT function in that fashion.  So I'd
> rather stick to the char device ioctl thing because it's more
> consistent with the old NBD stuff as well as the loop device stuff.

Indeed.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

Re: [Nbd] [PATCH 4/4] nbd: add a nbd-control interface

[Alex Bligh]

> On 25 Jan 2017, at 16:48, Alex Gartrell <agartrell@fb.com> wrote:
> 
> 
> If nbd were *all* netlink I think that that'd be fine, but you'd have
> problems implementing the NBD_DOIT function in that fashion.  So I'd
> rather stick to the char device ioctl thing because it's more
> consistent with the old NBD stuff as well as the loop device stuff.

I spend most of my time looking at the userspace side of NBD so
apologies if this is off base.

Given (because of NBD_DO_IT) we need an ioctl anyway, and we have
an ioctl that isn't going to go away, it would seem better if possible
to stick with ioctls, and not introduce either a dependency
on netlink (which would presumably bloat static binaries that
are used early in the boot process). Personally I'd have thought
adding a new NBD ioctl (or extending an existing one) would be
less entropy than adding a new char device.

-- 
Alex Bligh

Re: [Nbd] [PATCH 4/4] nbd: add a nbd-control interface

[Greg KH]

On Wed, Jan 25, 2017 at 06:25:11PM +0000, Alex Bligh wrote:
> 
> > On 25 Jan 2017, at 16:48, Alex Gartrell <agartrell@fb.com> wrote:
> > 
> > 
> > If nbd were *all* netlink I think that that'd be fine, but you'd have
> > problems implementing the NBD_DOIT function in that fashion.  So I'd
> > rather stick to the char device ioctl thing because it's more
> > consistent with the old NBD stuff as well as the loop device stuff.
> 
> I spend most of my time looking at the userspace side of NBD so
> apologies if this is off base.
> 
> Given (because of NBD_DO_IT) we need an ioctl anyway, and we have
> an ioctl that isn't going to go away, it would seem better if possible
> to stick with ioctls, and not introduce either a dependency
> on netlink (which would presumably bloat static binaries that
> are used early in the boot process). Personally I'd have thought
> adding a new NBD ioctl (or extending an existing one) would be
> less entropy than adding a new char device.

Why can't you just do this on any existing nbd block device with an
ioctl to it?  No need to have to do it on an out-of-band char device
node, right?

thanks,

greg k-h

Re: [Nbd] [PATCH 4/4] nbd: add a nbd-control interface

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1276 bytes --]

On 01/25/2017 03:30 PM, Greg KH wrote:
>> Given (because of NBD_DO_IT) we need an ioctl anyway, and we have
>> an ioctl that isn't going to go away, it would seem better if possible
>> to stick with ioctls, and not introduce either a dependency
>> on netlink (which would presumably bloat static binaries that
>> are used early in the boot process). Personally I'd have thought
>> adding a new NBD ioctl (or extending an existing one) would be
>> less entropy than adding a new char device.
> 
> Why can't you just do this on any existing nbd block device with an
> ioctl to it?  No need to have to do it on an out-of-band char device
> node, right?

How do you get an fd to existing nbd block device?  Your intent is to
use an ioctl to request creating/opening a new nbd device that no one
else is using; opening an existing device in order to send that ioctl
may have negative ramifications to the actual user of that existing
device, if not permissions issues that prevent the open from even
happening.  Having a separate control fd makes it obvious that you are
asking for a new nbd device, and don't want to stomp on any existing
devices.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Nbd] [PATCH 4/4] nbd: add a nbd-control interface

[Christoph Hellwig]

On Wed, Jan 25, 2017 at 03:36:20PM -0600, Eric Blake wrote:
> How do you get an fd to existing nbd block device?  Your intent is to
> use an ioctl to request creating/opening a new nbd device that no one
> else is using; opening an existing device in order to send that ioctl
> may have negative ramifications to the actual user of that existing
> device, if not permissions issues that prevent the open from even
> happening.  Having a separate control fd makes it obvious that you are
> asking for a new nbd device, and don't want to stomp on any existing
> devices.

Yes, - this whole concept of needing a device first to then associate
it with a backing store is something we had a in a lot of drivers,
and it's always been a major problem.  Thus we've been slowly moving
all the drivers off it.

Re: [Nbd] [PATCH 4/4] nbd: add a nbd-control interface

[Greg KH]

On Thu, Jan 26, 2017 at 12:40:47AM -0800, Christoph Hellwig wrote:
> On Wed, Jan 25, 2017 at 03:36:20PM -0600, Eric Blake wrote:
> > How do you get an fd to existing nbd block device?  Your intent is to
> > use an ioctl to request creating/opening a new nbd device that no one
> > else is using; opening an existing device in order to send that ioctl
> > may have negative ramifications to the actual user of that existing
> > device, if not permissions issues that prevent the open from even
> > happening.  Having a separate control fd makes it obvious that you are
> > asking for a new nbd device, and don't want to stomp on any existing
> > devices.
> 
> Yes, - this whole concept of needing a device first to then associate
> it with a backing store is something we had a in a lot of drivers,
> and it's always been a major problem.  Thus we've been slowly moving
> all the drivers off it.

Ok, but do you feel the "loop method" of using a char device node to
create/control these devices is a good model to follow for new devices
like ndb?

thanks,

greg k-h

Re: [Nbd] [PATCH 4/4] nbd: add a nbd-control interface

[Christoph Hellwig]

On Thu, Jan 26, 2017 at 10:17:58AM +0100, Greg KH wrote:
> Ok, but do you feel the "loop method" of using a char device node to
> create/control these devices is a good model to follow for new devices
> like ndb?

Yes. We've done the same for NVMe over fabrics.

Re: [Nbd] [PATCH 4/4] nbd: add a nbd-control interface

[Paul Clements]

On Wed, Jan 25, 2017 at 9:23 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> They all have their own set of problems, but the needs of nbd as a network
> storage interface seem most closely resemble what we have for other network
> related interfaces, where we typically use netlink to do the setup, see e.g.
> macvtap as an example for a network chardev interface.

But nbd is a virtual block device, first and foremost. That means it
is very similar to both loop (Pavel began nbd as a copy of loop, I
believe) and device mapper.

Also, it uses ioctls for all configuration and administration currently.

--
Paul

Re: [Nbd] [PATCH 4/4] nbd: add a nbd-control interface

[Wouter Verhelst]

On Fri, Jan 20, 2017 at 04:56:52PM -0500, Josef Bacik wrote:
> This patch mirrors the loop back device behavior with a few changes.  First
> there is no DEL operation as NBD doesn't get as much churn as loop devices do.
> Secondly the GET_NEXT operation can optionally create a new NBD device or not.
> Our infrastructure people want to not allow NBD to create new devices as it
> causes problems for them in containers.  However allow this to be optional as
> things like the OSS NBD client probably doesn't care and would like to just be
> given a device to use.

Don't be so sure :-)

I agree that having a control device for NBD is useful and would make
certain things much easier. If that's added, then I'll move to using
that as a way to control the device rather than opening a device and
dealing with it that way. In fact, at some point in the past I did
suggest something along those ways myself; it's just not happened yet.

Obviously though this would require an intermediate situation in which
the control master would be available as well as (optionally perhaps)
the old way where you open a specific device node, so that we don't
break existing implementations before they've had a chance to follow
suit.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

Re: [Nbd] [PATCH 4/4] nbd: add a nbd-control interface

[Josef Bacik]

> On Jan 21, 2017, at 7:12 AM, Wouter Verhelst <w@uter.be> wrote:
>=20
>> On Fri, Jan 20, 2017 at 04:56:52PM -0500, Josef Bacik wrote:
>> This patch mirrors the loop back device behavior with a few changes.  Fi=
rst
>> there is no DEL operation as NBD doesn't get as much churn as loop devic=
es do.
>> Secondly the GET_NEXT operation can optionally create a new NBD device o=
r not.
>> Our infrastructure people want to not allow NBD to create new devices as=
 it
>> causes problems for them in containers.  However allow this to be option=
al as
>> things like the OSS NBD client probably doesn't care and would like to j=
ust be
>> given a device to use.
>=20
> Don't be so sure :-)
>=20
> I agree that having a control device for NBD is useful and would make
> certain things much easier. If that's added, then I'll move to using
> that as a way to control the device rather than opening a device and
> dealing with it that way. In fact, at some point in the past I did
> suggest something along those ways myself; it's just not happened yet.
>=20
> Obviously though this would require an intermediate situation in which
> the control master would be available as well as (optionally perhaps)
> the old way where you open a specific device node, so that we don't
> break existing implementations before they've had a chance to follow
> suit.

Sorry I wasn't super clear.  This doesn't change anything about how the dev=
ices are setup, it just means if you do max_nbds=3D0 you can dynamically ad=
d more as you need them, and you can find unused nbd devices easily instead=
 of making the user specify them.  When I get home tonight I'll push my nbd=
-client patch so you can see how I use it.  Thanks,

Josef=