Re: [PATCH RFC] virtio: use QUEUE_FLAG_CLUSTER in virtio_blk

Re: [PATCH RFC] virtio: use QUEUE_FLAG_CLUSTER in virtio_blk

[Rusty Russell]

On Friday 14 November 2008 20:00:23 Jens Axboe wrote:
> 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?

Apparently not.  My results must be noise :(

However, it made me look harder at the behaviour of the driver.  I have two 
other enhancement patches while I'll send now, but I still only see sgs of 116 
elements.

Thanks for the clue donation,
Rusty.

[PATCH 1/2] virtio: block: set max_segment_size and max_sectors to infinite.

[Rusty Russell]

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.

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

diff -r 446171198334 drivers/block/virtio_blk.c
--- a/drivers/block/virtio_blk.c	Mon Nov 17 08:34:57 2008 +1030
+++ b/drivers/block/virtio_blk.c	Mon Nov 17 17:16:35 2008 +1030
@@ -277,6 +277,9 @@
 	}
 	set_capacity(vblk->disk, cap);
 
+	/* No real sector limit. */
+	blk_queue_max_sectors(vblk->disk->queue, -1U);
+
 	/* Host can optionally specify maximum segment size and number of
 	 * segments. */
 	err = virtio_config_val(vdev, VIRTIO_BLK_F_SIZE_MAX,
@@ -284,6 +287,8 @@
 				&v);
 	if (!err)
 		blk_queue_max_segment_size(vblk->disk->queue, v);
+	else
+		blk_queue_max_segment_size(vblk->disk->queue, -1UL);
 
 	err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
 				offsetof(struct virtio_blk_config, seg_max),

[PATCH 2/2] virtio: block: dynamic maximum segments

[Rusty Russell]

Enhance the driver to handle whatever maximum segment number the host
tells us to handle.  Do to this, we need to allocate the scatterlist
dynamically.

We set max_phys_segments and max_hw_segments to the same value (1 if
the host doesn't tell us, since that's safest and all known hosts do
tell us).

Note that kmalloc'ing the structure for large sg_elems might be
problematic: the fix for this is sg_table, but that requires more
work.

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

diff -r 177219c4da07 drivers/block/virtio_blk.c
--- a/drivers/block/virtio_blk.c	Mon Nov 17 17:16:35 2008 +1030
+++ b/drivers/block/virtio_blk.c	Mon Nov 17 17:16:41 2008 +1030
@@ -6,7 +6,6 @@
 #include <linux/virtio_blk.h>
 #include <linux/scatterlist.h>
 
-#define VIRTIO_MAX_SG	(3+MAX_PHYS_SEGMENTS)
 #define PART_BITS 4
 
 static int major, index;
@@ -26,8 +25,11 @@
 
 	mempool_t *pool;
 
+	/* What host tells us, plus 2 for header & tailer. */
+	unsigned int sg_elems;
+
 	/* Scatterlist: can be too big for stack. */
-	struct scatterlist sg[VIRTIO_MAX_SG];
+	struct scatterlist sg[/*sg_elems*/];
 };
 
 struct virtblk_req
@@ -97,8 +99,6 @@
 	if (blk_barrier_rq(vbr->req))
 		vbr->out_hdr.type |= VIRTIO_BLK_T_BARRIER;
 
-	/* This init could be done at vblk creation time */
-	sg_init_table(vblk->sg, VIRTIO_MAX_SG);
 	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));
@@ -130,7 +130,7 @@
 
 	while ((req = elv_next_request(q)) != NULL) {
 		vblk = req->rq_disk->private_data;
-		BUG_ON(req->nr_phys_segments > ARRAY_SIZE(vblk->sg));
+		BUG_ON(req->nr_phys_segments + 2 > vblk->sg_elems);
 
 		/* If this request fails, stop queue and wait for something to
 		   finish to restart it. */
@@ -196,12 +196,22 @@
 	int err;
 	u64 cap;
 	u32 v;
-	u32 blk_size;
+	u32 blk_size, sg_elems;
 
 	if (index_to_minor(index) >= 1 << MINORBITS)
 		return -ENOSPC;
 
-	vdev->priv = vblk = kmalloc(sizeof(*vblk), GFP_KERNEL);
+	/* We need to know how many segments before we allocate. */
+	err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
+				offsetof(struct virtio_blk_config, seg_max),
+				&sg_elems);
+	if (err)
+		sg_elems = 1;
+
+	/* We need an extra sg elements at head and tail. */
+	sg_elems += 2;
+	vdev->priv = vblk = kmalloc(sizeof(*vblk) +
+				    sizeof(vblk->sg[0]) * sg_elems, GFP_KERNEL);
 	if (!vblk) {
 		err = -ENOMEM;
 		goto out;
@@ -210,6 +220,8 @@
 	INIT_LIST_HEAD(&vblk->reqs);
 	spin_lock_init(&vblk->lock);
 	vblk->vdev = vdev;
+	vblk->sg_elems = sg_elems;
+	sg_init_table(vblk->sg, vblk->sg_elems);
 
 	/* We expect one virtqueue, for output. */
 	vblk->vq = vdev->config->find_vq(vdev, 0, blk_done);
@@ -277,6 +289,10 @@
 	}
 	set_capacity(vblk->disk, cap);
 
+	/* We can handle whatever the host told us to handle. */
+	blk_queue_max_phys_segments(vblk->disk->queue, vblk->sg_elems-2);
+	blk_queue_max_hw_segments(vblk->disk->queue, vblk->sg_elems-2);
+
 	/* No real sector limit. */
 	blk_queue_max_sectors(vblk->disk->queue, -1U);
 
@@ -289,12 +305,6 @@
 		blk_queue_max_segment_size(vblk->disk->queue, v);
 	else
 		blk_queue_max_segment_size(vblk->disk->queue, -1UL);
-
-	err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
-				offsetof(struct virtio_blk_config, seg_max),
-				&v);
-	if (!err)
-		blk_queue_max_hw_segments(vblk->disk->queue, v);
 
 	/* Host can optionally specify the block size of the device */
 	err = virtio_config_val(vdev, VIRTIO_BLK_F_BLK_SIZE,

Re: [PATCH 1/2] virtio: block: set max_segment_size and max_sectors to infinite.

[Chris Wright]

* Rusty Russell (rusty@rustcorp.com.au) wrote:
> +	/* No real sector limit. */
> +	blk_queue_max_sectors(vblk->disk->queue, -1U);
> +

Is that actually legitimate?  I think it'd still work out, but seems
odd, e.g. all the spots that do:

	q->max_hw_sectors << 9

will just toss the upper bits...

thanks,
-chris

Re: [PATCH 1/2] virtio: block: set max_segment_size and max_sectors to infinite.

[Rusty Russell]

On Thursday 27 November 2008 04:12:25 Chris Wright wrote:
> * Rusty Russell (rusty@rustcorp.com.au) wrote:
> > +	/* No real sector limit. */
> > +	blk_queue_max_sectors(vblk->disk->queue, -1U);
> > +
>
> Is that actually legitimate?  I think it'd still work out, but seems
> odd, e.g. all the spots that do:
>
> 	q->max_hw_sectors << 9
>
> will just toss the upper bits...

I think this just indicates that *my driver* doesn't have an issue with large 
numbers of sectors.  The block layer may well...

Rusty.


>
> thanks,
> -chris

Re: [PATCH 1/2] virtio: block: set max_segment_size and max_sectors to infinite.

[Jens Axboe]

On Thu, Nov 27 2008, Rusty Russell wrote:
> On Thursday 27 November 2008 04:12:25 Chris Wright wrote:
> > * Rusty Russell (rusty@rustcorp.com.au) wrote:
> > > +	/* No real sector limit. */
> > > +	blk_queue_max_sectors(vblk->disk->queue, -1U);
> > > +
> >
> > Is that actually legitimate?  I think it'd still work out, but seems
> > odd, e.g. all the spots that do:
> >
> > 	q->max_hw_sectors << 9
> >
> > will just toss the upper bits...
> 
> I think this just indicates that *my driver* doesn't have an issue
> with large numbers of sectors.  The block layer may well...

We should probably just truncate insanely large values like -1 to
something sane. For the above case we already do trunc it to 1MB, as
it's the soft IO size. The segment size is already in bytes, so in fact
everything already looks fine.

-- 
Jens Axboe

Re: [PATCH 1/2] virtio: block: set max_segment_size and max_sectors to infinite.

[Chris Wright]

* Rusty Russell (rusty@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@sous-sol.org>