Linux Virtualization - Nov 2008 - [PATCH RFC] virtio: use QUEUE_FLAG_CLUSTER in virtio_blk

This allows more requests to fit in the descriptor ring.

Copying 1.7M kernel image 100 times (with sync between)
  Before: totsegs = 55661 totlen = 148859962 avg. 2674
  After: totsegs = 36097 totlen = 139439355 avg: 3862

Unfortunately, this coalescing is done at blk_rq_map_sg() which is too
late to be optimal: requests have already been limited to the value set
by blk_queue_max_hw_segments().  For us, that value reflects the
number of sg slots we can handle (ie. after clustering).

I suspect other drivers have the same issue.  Jens?

(This patch is a one liner + temporary stats-gathering cruft for show)

Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
---
 drivers/block/virtio_blk.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff -r 35ec755dd1d6 drivers/block/virtio_blk.c
--- a/drivers/block/virtio_blk.c	Wed Nov 12 22:41:12 2008 +1030
+++ b/drivers/block/virtio_blk.c	Fri Nov 14 11:06:03 2008 +1030
@@ -37,6 +37,8 @@
 	struct virtio_blk_outhdr out_hdr;
 	u8 status;
 };
+
+static unsigned long totlen, totsegs;
 
 static void blk_done(struct virtqueue *vq)
 {
@@ -102,7 +104,6 @@
 	sg_set_buf(&vblk->sg[0], &vbr->out_hdr,
sizeof(vbr->out_hdr));
 	num = blk_rq_map_sg(q, vbr->req, vblk->sg+1);
 	sg_set_buf(&vblk->sg[num+1], &vbr->status,
sizeof(vbr->status));
-
 	if (rq_data_dir(vbr->req) == WRITE) {
 		vbr->out_hdr.type |= VIRTIO_BLK_T_OUT;
 		out = 1 + num;
@@ -111,6 +112,13 @@
 		vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
 		out = 1;
 		in = 1 + num;
+	}
+
+	totsegs += out + in + 1;
+	{
+		unsigned int i;
+		for (i = 0; i < num+2; i++)
+			totlen += vblk->sg[i].length;
 	}
 
 	if (vblk->vq->vq_ops->add_buf(vblk->vq, vblk->sg, out, in,
vbr)) {
@@ -176,6 +184,9 @@
 		geo->sectors = 1 << 5;
 		geo->cylinders = get_capacity(bd->bd_disk) >> 11;
 	}
+
+	printk("totsegs = %lu totlen = %lu\n", totsegs, totlen);
+	totsegs = totlen = 0;
 	return 0;
 }
 
@@ -260,6 +271,9 @@
 	/* If barriers are supported, tell block layer that queue is ordered */
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_BARRIER))
 		blk_queue_ordered(vblk->disk->queue, QUEUE_ORDERED_TAG, NULL);
+
+	/* Gather adjacent buffers to minimize sg length. */
+	queue_flag_set(QUEUE_FLAG_CLUSTER, vblk->disk->queue);
 
 	/* If disk is read-only in the host, the guest should obey */
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_RO))

On Fri, Nov 14 2008, Rusty Russell wrote:
> This allows more requests to fit in the descriptor ring.
> 
> Copying 1.7M kernel image 100 times (with sync between)
>   Before: totsegs = 55661 totlen = 148859962 avg. 2674
>   After: totsegs = 36097 totlen = 139439355 avg: 3862
> 
> Unfortunately, this coalescing is done at blk_rq_map_sg() which is too
> late to be optimal: requests have already been limited to the value set
> by blk_queue_max_hw_segments().  For us, that value reflects the
> number of sg slots we can handle (ie. after clustering).
> 
> I suspect other drivers have the same issue.  Jens?
blk_queue_max_hw_segments() is the size of your sg list, so yes the
block layer will stop merging more into a request if we go beyond that.
But it tracks merging along the way, so I don't see why there's a
discrepancy between the two ends? Unless there's a bug there, of
course...

Queue clustering is on by default though when you allocate your queue,
so I'm surprised you see a difference by doing:

+	/* Gather adjacent buffers to minimize sg length. */
+	queue_flag_set(QUEUE_FLAG_CLUSTER, vblk->disk->queue);

did test_bit(QUEUE_FLAG_CLUSTER, &vblk->disk->queue->queue_flags)
really
return 0 before?

-- 
Jens Axboe

* Rusty Russell (rusty at rustcorp.com.au) wrote:
> Setting max_segment_size allows more than 64k per sg element, unless
> the host specified a limit.  Setting max_sectors indicates that our
> max_hw_segments is the only limit.
We had been using a simple hardocded constant to increase max sectors to
improve throughput.  This (along with 2/2) are tested and showing nice numbers.

Acked-by: Chris Wright <chrisw at sous-sol.org>