Linux Virtualization - Apr 2018 - [RFC PATCH 2/2] virtio_blk: add new module parameter to set max request size

Hi,

For virtio block device, actually there is no a hard limit for max request
size, and virtio_blk driver set -1 to blk_queue_max_hw_sectors(q, -1U);.
But it doesn't work, because there is a default upper limitation
BLK_DEF_MAX_SECTORS (1280 sectors). So this series want to add a new helper
blk_queue_max_hw_sectors_no_limit to set a proper max reqeust size.

Weiping Zhang (2):
  blk-setting: add new helper blk_queue_max_hw_sectors_no_limit
  virtio_blk: add new module parameter to set max request size

 block/blk-settings.c       | 20 ++++++++++++++++++++
 drivers/block/virtio_blk.c | 32 ++++++++++++++++++++++++++++++--
 include/linux/blkdev.h     |  2 ++
 3 files changed, 52 insertions(+), 2 deletions(-)

-- 
2.9.4

There is a default upper limitation BLK_DEF_MAX_SECTORS, but for
some virtual block device driver there is no such limitation. So
add a new help to set max request size.

Signed-off-by: Weiping Zhang <zhangweiping at didichuxing.com>
---
 block/blk-settings.c   | 20 ++++++++++++++++++++
 include/linux/blkdev.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index 48ebe6b..685c30c 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -253,6 +253,26 @@ void blk_queue_max_hw_sectors(struct request_queue *q,
unsigned int max_hw_secto
 }
 EXPORT_SYMBOL(blk_queue_max_hw_sectors);
 
+/* same as blk_queue_max_hw_sectors but without default upper limitation */
+void blk_queue_max_hw_sectors_no_limit(struct request_queue *q,
+		unsigned int max_hw_sectors)
+{
+	struct queue_limits *limits = &q->limits;
+	unsigned int max_sectors;
+
+	if ((max_hw_sectors << 9) < PAGE_SIZE) {
+		max_hw_sectors = 1 << (PAGE_SHIFT - 9);
+		printk(KERN_INFO "%s: set to minimum %d\n",
+		       __func__, max_hw_sectors);
+	}
+
+	limits->max_hw_sectors = max_hw_sectors;
+	max_sectors = min_not_zero(max_hw_sectors, limits->max_dev_sectors);
+	limits->max_sectors = max_sectors;
+	q->backing_dev_info->io_pages = max_sectors >> (PAGE_SHIFT - 9);
+}
+EXPORT_SYMBOL(blk_queue_max_hw_sectors_no_limit);
+
 /**
  * blk_queue_chunk_sectors - set size of the chunk for this queue
  * @q:  the request queue for the device
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index ed63f3b..2250709 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1243,6 +1243,8 @@ extern void blk_cleanup_queue(struct request_queue *);
 extern void blk_queue_make_request(struct request_queue *, make_request_fn *);
 extern void blk_queue_bounce_limit(struct request_queue *, u64);
 extern void blk_queue_max_hw_sectors(struct request_queue *, unsigned int);
+extern void blk_queue_max_hw_sectors_no_limit(struct request_queue *,
+						unsigned int);
 extern void blk_queue_chunk_sectors(struct request_queue *, unsigned int);
 extern void blk_queue_max_segments(struct request_queue *, unsigned short);
 extern void blk_queue_max_discard_segments(struct request_queue *,
-- 
2.9.4

Actually there is no upper limitation, so add new module parameter
to provide a way to set a proper max request size for virtio block.
Using a larger request size can improve sequence performance in theory,
and reduce the interaction between guest and hypervisor.

Signed-off-by: Weiping Zhang <zhangweiping at didichuxing.com>
---
 drivers/block/virtio_blk.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4a07593c..5ac6d59 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -64,6 +64,34 @@ struct virtblk_req {
 	struct scatterlist sg[];
 };
 
+
+static int max_request_size_set(const char *val, const struct kernel_param
*kp);
+
+static const struct kernel_param_ops max_request_size_ops = {
+	.set = max_request_size_set,
+	.get = param_get_uint,
+};
+
+static unsigned long max_request_size = 4096; /* in unit of KiB */
+module_param_cb(max_request_size, &max_request_size_ops,
&max_request_size,
+		0444);
+MODULE_PARM_DESC(max_request_size, "set max request size, in unit of
KiB");
+
+static int max_request_size_set(const char *val, const struct kernel_param *kp)
+{
+	int ret;
+	unsigned int size_kb, page_kb = 1 << (PAGE_SHIFT - 10);
+
+	ret = kstrtouint(val, 10, &size_kb);
+	if (ret != 0)
+		return -EINVAL;
+
+	if (size_kb < page_kb)
+		return -EINVAL;
+
+	return param_set_uint(val, kp);
+}
+
 static inline blk_status_t virtblk_result(struct virtblk_req *vbr)
 {
 	switch (vbr->status) {
@@ -730,8 +758,8 @@ static int virtblk_probe(struct virtio_device *vdev)
 	/* We can handle whatever the host told us to handle. */
 	blk_queue_max_segments(q, vblk->sg_elems-2);
 
-	/* No real sector limit. */
-	blk_queue_max_hw_sectors(q, -1U);
+	/* No real sector limit, use 512b (max_request_size << 10) >> 9 */
+	blk_queue_max_hw_sectors_no_limit(q, max_request_size << 1);
 
 	/* Host can optionally specify maximum segment size and number of
 	 * segments. */
-- 
2.9.4

Weiping,
> For virtio block device, actually there is no a hard limit for max
> request size, and virtio_blk driver set -1 to
> blk_queue_max_hw_sectors(q, -1U);.  But it doesn't work, because there
> is a default upper limitation BLK_DEF_MAX_SECTORS (1280 sectors).
That's intentional (although it's an ongoing debate what the actual
value should be).
> So this series want to add a new helper
> blk_queue_max_hw_sectors_no_limit to set a proper max reqeust size.
BLK_DEF_MAX_SECTORS is a kernel default empirically chosen to strike a
decent balance between I/O latency and bandwidth. It sets an upper bound
for filesystem requests only. Regardless of the capabilities of the
block device driver and underlying hardware.

You can override the limit on a per-device basis via max_sectors_kb in
sysfs. People generally do it via a udev rule.

-- 
Martin K. Petersen	Oracle Linux Engineering

On 4/5/18 4:09 AM, Weiping Zhang wrote:
> Hi,
> 
> For virtio block device, actually there is no a hard limit for max request
> size, and virtio_blk driver set -1 to blk_queue_max_hw_sectors(q, -1U);.
> But it doesn't work, because there is a default upper limitation
> BLK_DEF_MAX_SECTORS (1280 sectors). So this series want to add a new helper
> blk_queue_max_hw_sectors_no_limit to set a proper max reqeust size.
> 
> Weiping Zhang (2):
>   blk-setting: add new helper blk_queue_max_hw_sectors_no_limit
>   virtio_blk: add new module parameter to set max request size
> 
>  block/blk-settings.c       | 20 ++++++++++++++++++++
>  drivers/block/virtio_blk.c | 32 ++++++++++++++++++++++++++++++--
>  include/linux/blkdev.h     |  2 ++
>  3 files changed, 52 insertions(+), 2 deletions(-)
The driver should just use blk_queue_max_hw_sectors() to set the limit,
and then the soft limit can be modified by a udev rule. Technically the
driver doesn't own the software limit, it's imposed to ensure that we
don't introduce too much latency per request.

Your situation is no different from many other setups, where the
hw limit is much higher than the default 1280k.

-- 
Jens Axboe

2018-04-05 22:29 GMT+08:00 Jens Axboe <axboe at
kernel.dk>:
> On 4/5/18 4:09 AM, Weiping Zhang wrote:
>> Hi,
>>
>> For virtio block device, actually there is no a hard limit for max
request
>> size, and virtio_blk driver set -1 to blk_queue_max_hw_sectors(q,
-1U);.
>> But it doesn't work, because there is a default upper limitation
>> BLK_DEF_MAX_SECTORS (1280 sectors). So this series want to add a new
helper
>> blk_queue_max_hw_sectors_no_limit to set a proper max reqeust size.
>>
>> Weiping Zhang (2):
>>   blk-setting: add new helper blk_queue_max_hw_sectors_no_limit
>>   virtio_blk: add new module parameter to set max request size
>>
>>  block/blk-settings.c       | 20 ++++++++++++++++++++
>>  drivers/block/virtio_blk.c | 32 ++++++++++++++++++++++++++++++--
>>  include/linux/blkdev.h     |  2 ++
>>  3 files changed, 52 insertions(+), 2 deletions(-)
>
> The driver should just use blk_queue_max_hw_sectors() to set the limit,
> and then the soft limit can be modified by a udev rule. Technically the
> driver doesn't own the software limit, it's imposed to ensure that
we
> don't introduce too much latency per request.
>
> Your situation is no different from many other setups, where the
> hw limit is much higher than the default 1280k.
>
Hi Martin, Jens,

It seems more reasonable to change software limitation by udev rule,
thanks you.
>
> _______________________________________________
> Virtualization mailing list
> Virtualization at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization