[PATCH 2/3] Virtio draft IV: the block driver

[PATCH 2/3] Virtio draft IV: the block driver

[Rusty Russell]

The block driver uses scatter-gather lists with sg[0] being the
request information (struct virtio_blk_outhdr) with the type, sector
and inbuf id.  The next N sg entries are the bio itself, then the last
sg is the status byte.  Whether the N entries are in or out depends on
whether it's a read or a write.

We accept the normal (SCSI) ioctls: they get handed through to the other
side which can then handle it or reply that it's unsupported.  It's
not clear that this actually works in general, since I don't know
if blk_pc_request() requests have an accurate rq_data_dir().

Although we try to reply -ENOTTY on unsupported commands, the block
layer in its infinite wisdom suppressed the error so ioctl(fd,
CDROMEJECT) returns success to userspace.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/block/Makefile     |    1 
 drivers/block/virtio_blk.c |  253 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/Kbuild       |    1 
 include/linux/virtio_blk.h |   45 +++++++
 4 files changed, 300 insertions(+)

===================================================================
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_BLK_CPQ_CISS_DA)  += cciss.
 obj-$(CONFIG_BLK_CPQ_CISS_DA)  += cciss.o
 obj-$(CONFIG_BLK_DEV_DAC960)	+= DAC960.o
 obj-$(CONFIG_CDROM_PKTCDVD)	+= pktcdvd.o
+obj-y				+= virtio_blk.o
 
 obj-$(CONFIG_BLK_DEV_UMEM)	+= umem.o
 obj-$(CONFIG_BLK_DEV_NBD)	+= nbd.o
===================================================================
--- /dev/null
+++ b/drivers/block/virtio_blk.c
@@ -0,0 +1,253 @@
+#define DEBUG
+#include <linux/spinlock.h>
+#include <linux/blkdev.h>
+#include <linux/hdreg.h>
+#include <linux/virtio.h>
+#include <linux/virtio_blk.h>
+
+static unsigned char virtblk_index = 'a';
+struct virtio_blk
+{
+	spinlock_t lock;
+
+	struct virtqueue *vq;
+
+	/* The disk structure for the kernel. */
+	struct gendisk *disk;
+
+	/* Request tracking. */
+	struct list_head reqs;
+
+	mempool_t *pool;
+
+	/* Scatterlist: can be too big for stack. */
+	struct scatterlist sg[3+MAX_PHYS_SEGMENTS];
+};
+
+struct virtblk_req
+{
+	struct list_head list;
+	struct request *req;
+	struct virtio_blk_outhdr out_hdr;
+	struct virtio_blk_inhdr in_hdr;
+};
+
+static void end_dequeued_request(struct request *req,
+				 request_queue_t *q, int uptodate)
+{
+	/* And so the insanity of the block layer infects us here. */
+	int nsectors = req->hard_nr_sectors;
+
+	if (blk_pc_request(req)) {
+		nsectors = (req->data_len + 511) >> 9;
+		if (!nsectors)
+			nsectors = 1;
+	}
+	if (end_that_request_first(req, uptodate, nsectors))
+		BUG();
+	add_disk_randomness(req->rq_disk);
+	end_that_request_last(req, uptodate);
+}
+
+static bool blk_done(struct virtqueue *vq)
+{
+	struct virtio_blk *vblk = vq->priv;
+	struct virtblk_req *vbr;
+	unsigned int len;
+	unsigned long flags;
+
+	spin_lock_irqsave(&vblk->lock, flags);
+	while ((vbr = vq->ops->get_buf(vq, &len)) != NULL) {
+		int uptodate;
+		switch (vbr->in_hdr.status) {
+		case VIRTIO_BLK_S_OK:
+			uptodate = 1;
+			break;
+		case VIRTIO_BLK_S_UNSUPP:
+			uptodate = -ENOTTY;
+			break;
+		default:
+			uptodate = 0;
+			break;
+		}
+
+		end_dequeued_request(vbr->req, vblk->disk->queue, uptodate);
+		list_del(&vbr->list);
+		mempool_free(vbr, vblk->pool);
+	}
+	/* In case queue is stopped waiting for more buffers. */
+	blk_start_queue(vblk->disk->queue);
+	spin_unlock_irqrestore(&vblk->lock, flags);
+	return true;
+}
+
+static bool do_req(request_queue_t *q, struct virtio_blk *vblk,
+		   struct virtblk_req *vbr)
+{
+	unsigned long num, out_num, in_num;
+
+	if (blk_fs_request(vbr->req)) {
+		vbr->out_hdr.type = 0;
+		vbr->out_hdr.sector = vbr->req->sector;
+		vbr->out_hdr.ioprio = vbr->req->ioprio;
+	} else if (blk_pc_request(vbr->req)) {
+		vbr->out_hdr.type = VIRTIO_BLK_T_SCSI_CMD;
+		vbr->out_hdr.sector = 0;
+		vbr->out_hdr.ioprio = vbr->req->ioprio;
+	} else {
+		/* We don't put anything else in the queue. */
+		BUG();
+	}
+
+	if (blk_barrier_rq(vbr->req))
+		vbr->out_hdr.type |= VIRTIO_BLK_T_BARRIER;
+
+	vblk->sg[0].page = virt_to_page(&vbr->out_hdr);
+	vblk->sg[0].offset = offset_in_page(&vbr->out_hdr);
+	vblk->sg[0].length = sizeof(vbr->out_hdr);
+	num = blk_rq_map_sg(q, vbr->req, vblk->sg+1);
+	vblk->sg[num+1].page = virt_to_page(&vbr->in_hdr);
+	vblk->sg[num+1].offset = offset_in_page(&vbr->in_hdr);
+	vblk->sg[num+1].length = sizeof(vbr->in_hdr);
+
+	if (rq_data_dir(vbr->req) == WRITE) {
+		vbr->out_hdr.type |= VIRTIO_BLK_T_OUT;
+		out_num = 1 + num;
+		in_num = 1;
+	} else {
+		vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
+		out_num = 1;
+		in_num = 1 + num;
+	}
+
+	if (vblk->vq->ops->add_buf(vblk->vq, vblk->sg, out_num, in_num, vbr))
+		return false;
+
+	list_add_tail(&vbr->list, &vblk->reqs);
+	return true;
+}
+
+static void do_virtblk_request(request_queue_t *q)
+{
+	struct virtio_blk *vblk = NULL;
+	struct request *req;
+	struct virtblk_req *vbr;
+
+	while ((req = elv_next_request(q)) != NULL) {
+		vblk = req->rq_disk->private_data;
+
+		vbr = mempool_alloc(vblk->pool, GFP_ATOMIC);
+		if (!vbr)
+			goto stop;
+
+		BUG_ON(req->nr_phys_segments > ARRAY_SIZE(vblk->sg));
+		vbr->req = req;
+		if (!do_req(q, vblk, vbr))
+			goto stop;
+		blkdev_dequeue_request(req);
+	}
+
+sync:
+	if (vblk)
+		vblk->vq->ops->sync(vblk->vq);
+	return;
+
+stop:
+	/* Queue full?  Wait. */
+	blk_stop_queue(q);
+	mempool_free(vbr, vblk->pool);
+	goto sync;
+}
+
+static int virtblk_ioctl(struct inode *inode, struct file *filp,
+			 unsigned cmd, unsigned long data)
+{
+	return scsi_cmd_ioctl(filp, inode->i_bdev->bd_disk, cmd,
+			      (void __user *)data);
+}
+
+static struct block_device_operations virtblk_fops = {
+	.ioctl = virtblk_ioctl,
+	.owner = THIS_MODULE,
+};
+
+struct gendisk *virtblk_probe(struct virtqueue *vq)
+{
+	struct virtio_blk *vblk;
+	int err, major;
+
+	vblk = kmalloc(sizeof(*vblk), GFP_KERNEL);
+	if (!vblk) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	INIT_LIST_HEAD(&vblk->reqs);
+	spin_lock_init(&vblk->lock);
+	vblk->vq = vq;
+	vq->cb = blk_done;
+	vq->priv = vblk;
+
+	vblk->pool = mempool_create_kmalloc_pool(1,sizeof(struct virtblk_req));
+	if (!vblk->pool) {
+		err = -ENOMEM;
+		goto out_free_vblk;
+	}
+
+	major = register_blkdev(0, "virtblk");
+	if (major < 0) {
+		err = major;
+		goto out_mempool;
+	}
+
+	/* FIXME: How many partitions?  How long is a piece of string? */
+	vblk->disk = alloc_disk(1 << 3);
+	if (!vblk->disk) {
+		err = -ENOMEM;
+		goto out_unregister_blkdev;
+	}
+
+	vblk->disk->queue = blk_init_queue(do_virtblk_request, &vblk->lock);
+	if (!vblk->disk->queue) {
+		err = -ENOMEM;
+		goto out_put_disk;
+	}
+
+	sprintf(vblk->disk->disk_name, "vd%c", virtblk_index++);
+	vblk->disk->major = major;
+	vblk->disk->first_minor = 0;
+	vblk->disk->private_data = vblk;
+	vblk->disk->fops = &virtblk_fops;
+
+	blk_queue_ordered(vblk->disk->queue, QUEUE_ORDERED_TAG, NULL);
+
+	/* Caller can do blk_queue_max_hw_segments(), set_capacity()
+	 * etc then add_disk(). */
+	return vblk->disk;
+
+out_put_disk:
+	put_disk(vblk->disk);
+out_unregister_blkdev:
+	unregister_blkdev(major, "virtblk");
+out_mempool:
+	mempool_destroy(vblk->pool);
+out_free_vblk:
+	kfree(vblk);
+out:
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(virtblk_probe);
+
+void virtblk_remove(struct gendisk *disk)
+{
+	struct virtio_blk *vblk = disk->private_data;
+	int major = vblk->disk->major;
+
+	BUG_ON(!list_empty(&vblk->reqs));
+	blk_cleanup_queue(vblk->disk->queue);
+	put_disk(vblk->disk);
+	unregister_blkdev(major, "virtblk");
+	mempool_destroy(vblk->pool);
+	kfree(vblk);
+}
+EXPORT_SYMBOL_GPL(virtblk_remove);
===================================================================
--- a/include/linux/Kbuild
+++ b/include/linux/Kbuild
@@ -341,6 +341,7 @@ unifdef-y += utsname.h
 unifdef-y += utsname.h
 unifdef-y += videodev2.h
 unifdef-y += videodev.h
+unifdef-y += virtio_blk.h
 unifdef-y += wait.h
 unifdef-y += wanrouter.h
 unifdef-y += watchdog.h
===================================================================
--- /dev/null
+++ b/include/linux/virtio_blk.h
@@ -0,0 +1,45 @@
+#ifndef _LINUX_VIRTIO_BLK_H
+#define _LINUX_VIRTIO_BLK_H
+#include <linux/types.h>
+
+/* These two define direction. */
+#define VIRTIO_BLK_T_IN		0
+#define VIRTIO_BLK_T_OUT	1
+
+/* This bit says it's a scsi command, not an actual read or write. */
+#define VIRTIO_BLK_T_SCSI_CMD	2
+
+/* Barrier before this op. */
+#define VIRTIO_BLK_T_BARRIER	0x80000000
+
+/* This is the first element of the read scatter-gather list. */
+struct virtio_blk_outhdr
+{
+	/* VIRTIO_BLK_T* */
+	__u32 type;
+	/* io priority. */
+	__u32 ioprio;
+	/* Sector (ie. 512 byte offset) */
+	__u64 sector;
+	/* Where to put reply. */
+	__u64 id;
+};
+
+#define VIRTIO_BLK_S_OK		0
+#define VIRTIO_BLK_S_IOERR	1
+#define VIRTIO_BLK_S_UNSUPP	2
+
+/* This is the first element of the write scatter-gather list */
+struct virtio_blk_inhdr
+{
+	unsigned char status;
+};
+
+#ifdef __KERNEL__
+struct gendisk;
+struct virtio_device;
+
+struct gendisk *virtblk_probe(struct virtqueue *vq);
+void virtblk_remove(struct gendisk *disk);
+#endif /* __KERNEL__ */
+#endif /* _LINUX_VIRTIO_BLK_H */

Re: [PATCH 2/3] Virtio draft IV: the block driver

[Christian Borntraeger]

Am Mittwoch, 4. Juli 2007 schrieb Rusty Russell:
> +		vbr = mempool_alloc(vblk->pool, GFP_ATOMIC);
> +		if (!vbr)
> +			goto stop;
[...]
> +		BUG_ON(req->nr_phys_segments > ARRAY_SIZE(vblk->sg));
> +		vbr->req = req;
> +		if (!do_req(q, vblk, vbr))
> +			goto stop;
[...]
> +stop:
> +	/* Queue full?  Wait. */
> +	blk_stop_queue(q);
> +	mempool_free(vbr, vblk->pool);


Hmm, can mempool_free really handle NULL as its first argument? (first goto). 
As far as I can see we have a problem here. Our pool has preallocated 1 
element. So what about:
- first alloc fails in mempool_alloc fails -> we get the pre-allocated 
element, no more elements left
- second alloc fails in mempool_alloc fails -> we get NULL -> we return NULL 
to mempool_free which adds NULL back to the pool as the pool is empty. Now 
the pool has an NULL entry, no? 


Christian

Re: [PATCH 2/3] Virtio draft IV: the block driver

[Rusty Russell]

On Thu, 2007-07-05 at 09:32 +0200, Christian Borntraeger wrote:
> Am Mittwoch, 4. Juli 2007 schrieb Rusty Russell:
> > +		vbr = mempool_alloc(vblk->pool, GFP_ATOMIC);
> > +		if (!vbr)
> > +			goto stop;
> [...]
> > +		BUG_ON(req->nr_phys_segments > ARRAY_SIZE(vblk->sg));
> > +		vbr->req = req;
> > +		if (!do_req(q, vblk, vbr))
> > +			goto stop;
> [...]
> > +stop:
> > +	/* Queue full?  Wait. */
> > +	blk_stop_queue(q);
> > +	mempool_free(vbr, vblk->pool);
> 
> 
> Hmm, can mempool_free really handle NULL as its first argument? (first goto). 

Good point.  Any objections to fixing that?

Cheers,
Rusty.
===
Christian Borntraeger points out that mempool_free() doesn't noop when
handed NULL.  This is inconsistent with the other free-like functions
in the kernel.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r a306f0a8de5e mm/mempool.c
--- a/mm/mempool.c	Fri Jul 06 10:28:39 2007 +1000
+++ b/mm/mempool.c	Fri Jul 06 10:29:40 2007 +1000
@@ -263,6 +263,9 @@ void mempool_free(void *element, mempool
 {
 	unsigned long flags;
 
+	if (unlikely(element == NULL))
+		return;
+
 	smp_mb();
 	if (pool->curr_nr < pool->min_nr) {
 		spin_lock_irqsave(&pool->lock, flags);

Re: [PATCH 2/3] Virtio draft IV: the block driver

[Martin Peschke]

 > +static void do_virtblk_request(request_queue_t *q)
 > +{
 > +	struct virtio_blk *vblk = NULL;
 > +	struct request *req;
 > +	struct virtblk_req *vbr;
 > +
 > +	while ((req = elv_next_request(q)) != NULL) {
 > +		vblk = req->rq_disk->private_data;
 > +
 > +		vbr = mempool_alloc(vblk->pool, GFP_ATOMIC);
 > +		if (!vbr)
 > +			goto stop;
 > +
 > +		BUG_ON(req->nr_phys_segments > ARRAY_SIZE(vblk->sg));
 > +		vbr->req = req;
 > +		if (!do_req(q, vblk, vbr))
 > +			goto stop;
 > +		blkdev_dequeue_request(req);
 > +	}
 > +
 > +sync:
 > +	if (vblk)


this check looks bogus, as vblk->pool has been accessed unconditionally above


 > +		vblk->vq->ops->sync(vblk->vq);
 > +	return;
 > +
 > +stop:
 > +	/* Queue full?  Wait. */
 > +	blk_stop_queue(q);
 > +	mempool_free(vbr, vblk->pool);
 > +	goto sync;
 > +}

Re: [PATCH 2/3] Virtio draft IV: the block driver

[Martin Peschke]

On Mon, 2007-07-09 at 16:17 +0200, Martin Peschke wrote:
>  > +static void do_virtblk_request(request_queue_t *q)
>  > +{
>  > +	struct virtio_blk *vblk = NULL;
>  > +	struct request *req;
>  > +	struct virtblk_req *vbr;
>  > +
>  > +	while ((req = elv_next_request(q)) != NULL) {
>  > +		vblk = req->rq_disk->private_data;
>  > +
>  > +		vbr = mempool_alloc(vblk->pool, GFP_ATOMIC);
>  > +		if (!vbr)
>  > +			goto stop;
>  > +
>  > +		BUG_ON(req->nr_phys_segments > ARRAY_SIZE(vblk->sg));
>  > +		vbr->req = req;
>  > +		if (!do_req(q, vblk, vbr))
>  > +			goto stop;
>  > +		blkdev_dequeue_request(req);
>  > +	}
>  > +
>  > +sync:
>  > +	if (vblk)
> 
> 
> this check looks bogus, as vblk->pool has been accessed unconditionally above


Sorry, I have been seduced by the code to misread it.
The check is correct and needed.

Looks like we can make it more readable, though.

- get rid of convoluted goto's
- no need for calling mempool_free() with NULL-pointer
- read private data from q->queuedata once instead of poking inside
  every request
- use a local variable to track issued requests and the need for
  a sync-operation
- pass req to do_req() as it is used there throughout the function
  (nice symmetry in parameter list, isn't it?)

It's just an untested and incomplete sketch done while I am acquainting
myself with the virtio code without having set up lguest yet.


static void do_virtblk_request(request_queue_t *q)
{
	struct virtio_blk *vblk = q->queuedata;
	struct request *req;
	struct virtblk_req *vbr;
	int issued = 0;

	while ((req = elv_next_request(q)) != NULL) {
		vbr = mempool_alloc(vblk->pool, GFP_ATOMIC);
		if (!vbr) {
			blk_stop_queue(q);
			break;
		}

		BUG_ON(req->nr_phys_segments > ARRAY_SIZE(vblk->sg));
		vbr->req = req;
		if (!do_req(q, vblk, req, vbr)) {
			/* Queue full?  Wait. */
			blk_stop_queue(q);
			mempool_free(vbr, vblk->pool);
			break;
		}
		blkdev_dequeue_request(req);
		issued++;
	}

	if (issued)
		vblk->vq->ops->sync(vblk->vq);
}

Re: [PATCH 2/3] Virtio draft IV: the block driver

[Rusty Russell]

On Mon, 2007-07-09 at 17:54 +0200, Martin Peschke wrote:
> static void do_virtblk_request(request_queue_t *q)
> {
> 	struct virtio_blk *vblk = q->queuedata;
> 	struct request *req;
> 	struct virtblk_req *vbr;
> 	int issued = 0;
> 
> 	while ((req = elv_next_request(q)) != NULL) {
> 		vbr = mempool_alloc(vblk->pool, GFP_ATOMIC);
> 		if (!vbr) {
> 			blk_stop_queue(q);
> 			break;
> 		}
> 
> 		BUG_ON(req->nr_phys_segments > ARRAY_SIZE(vblk->sg));
> 		vbr->req = req;
> 		if (!do_req(q, vblk, req, vbr)) {
> 			/* Queue full?  Wait. */
> 			blk_stop_queue(q);
> 			mempool_free(vbr, vblk->pool);
> 			break;
> 		}
> 		blkdev_dequeue_request(req);
> 		issued++;
> 	}
> 
> 	if (issued)
> 		vblk->vq->ops->sync(vblk->vq);
> }

Hi Martin!

	I like this: moving the allocation into do_req() makes it even simpler:

static void do_virtblk_request(request_queue_t *q)
{
	struct virtio_blk *vblk = NULL;
	struct request *req;
	unsigned int issued = 0;

	while ((req = elv_next_request(q)) != NULL) {
		vblk = req->rq_disk->private_data;
		BUG_ON(req->nr_phys_segments > ARRAY_SIZE(vblk->sg));

		if (!do_req(q, vblk, req)) {
			/* Queue full?  Wait. */
			blk_stop_queue(q);
			break;
		}
		blkdev_dequeue_request(req);
		issued++;
	}

	if (issued)
		vblk->vq->ops->sync(vblk->vq);
}

Thanks!
Rusty.

Re: [PATCH 2/3] Virtio draft IV: the block driver

[Martin Peschke]

Rusty Russell wrote:
> Hi Martin!
> 
> 	I like this: moving the allocation into do_req() makes it even simpler:
> 
> static void do_virtblk_request(request_queue_t *q)
> {
> 	struct virtio_blk *vblk = NULL;
> 	struct request *req;
> 	unsigned int issued = 0;
> 
> 	while ((req = elv_next_request(q)) != NULL) {
> 		vblk = req->rq_disk->private_data;
> 		BUG_ON(req->nr_phys_segments > ARRAY_SIZE(vblk->sg));
> 
> 		if (!do_req(q, vblk, req)) {
> 			/* Queue full?  Wait. */
> 			blk_stop_queue(q);
> 			break;
> 		}
> 		blkdev_dequeue_request(req);
> 		issued++;
> 	}
> 
> 	if (issued)
> 		vblk->vq->ops->sync(vblk->vq);
> }

ACK ;)

Thanks,
Martin

Re: [PATCH 2/3] Virtio draft IV: the block driver

[Christian Borntraeger]

> +#ifdef __KERNEL__
> +struct gendisk;
> +struct virtio_device;
> +
> +struct gendisk *virtblk_probe(struct virtqueue *vq);

Hello Rusty,

I am currently do some prototyping with virtio IV and found tis small issue in 
the block device. virtblk_probe requires a declaration of virtqueue, not 
virtio_device. 

---
 include/linux/virtio_blk.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.22/include/linux/virtio_blk.h
===================================================================
--- linux-2.6.22.orig/include/linux/virtio_blk.h
+++ linux-2.6.22/include/linux/virtio_blk.h
@@ -37,7 +37,7 @@ struct virtio_blk_inhdr
 
 #ifdef __KERNEL__
 struct gendisk;
-struct virtio_device;
+struct virtqueue;
 
 struct gendisk *virtblk_probe(struct virtqueue *vq);
 void virtblk_remove(struct gendisk *disk);

Re: [PATCH 2/3] Virtio draft IV: the block driver

[Rusty Russell]

On Mon, 2007-07-23 at 13:13 +0200, Christian Borntraeger wrote:
> > +#ifdef __KERNEL__
> > +struct gendisk;
> > +struct virtio_device;
> > +
> > +struct gendisk *virtblk_probe(struct virtqueue *vq);
> 
> Hello Rusty,
> 
> I am currently do some prototyping with virtio IV and found tis small issue in 
> the block device. virtblk_probe requires a declaration of virtqueue, not 
> virtio_device. 

Oops, thanks Christian!

	I'll roll this in, thanks!

Rusty.
PS.  2.6.23-rc1 broke the block driver anyway, so will publish new
versions soon.