Skip to contentSkip to conversation

Virtio draft IV: the block driver

PATCH Martin Peschke 2 participants 4 messages Jul 9, 2007 – Jul 10, 2007 virtio core

lore
Share & tools
lore

Martin Peschke

author
#0 lore raw
Quoted 12 lines
(quoted diff) +12 -0
+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;

Review

Quoted 10 lines
(quoted diff) +10 -0
+
+		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

Quoted 9 lines
(quoted diff) +9 -0
+		vblk->vq->ops->sync(vblk->vq);
+	return;
+
+stop:
+	/* Queue full?  Wait. */
+	blk_stop_queue(q);
+	mempool_free(vbr, vblk->pool);
+	goto sync;
+}
author
#1 lore raw
Quoted 17 lines · Martin Peschke
Quoted 17 lines
(quoted diff) +17 -0
+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;
Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+		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);
}
#2 lore raw
Quoted 21 lines · Martin Peschke

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;
Quoted 8 lines
		}
		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.

author
#3 lore raw
Quoted 18 lines · Rusty Russell

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;
Quoted 8 lines
		}
		blkdev_dequeue_request(req);
		issued++;
	}

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

ACK ;)

Thanks,
Martin