On Thu, Sep 21, 2017 at 08:09:47AM -0600, Jens Axboe wrote:

On 09/21/2017 07:03 AM, weiping zhang wrote:

quoted

On Tue, Sep 12, 2017 at 09:57:32PM +0800, weiping zhang wrote:

quoted

On Wed, Sep 06, 2017 at 01:00:44PM +0000, Bart Van Assche wrote:

quoted

On Wed, 2017-09-06 at 15:34 +0800, weiping zhang wrote:

quoted

On Tue, Sep 05, 2017 at 03:42:45PM +0000, Bart Van Assche wrote:

quoted

On Sun, 2017-09-03 at 21:46 +0800, weiping zhang wrote:

quoted

if blk-mq use "none" io scheduler, nr_request get a wrong value when
input a number > tag_set->queue_depth. blk_mq_tag_update_depth will get
the smaller one min(nr, set->queue_depth), and then q->nr_request get a
wrong value.

Reproduce:

echo none > /sys/block/nvme0n1/queue/ioscheduler
echo 1000000 > /sys/block/nvme0n1/queue/nr_requests
cat /sys/block/nvme0n1/queue/nr_requests
1000000

Signed-off-by: weiping zhang <redacted>
---
 block/blk-mq.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index f84d145..8303e5e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c

@@ -2622,8 +2622,11 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr)
 		 * queue depth. This is similar to what the old code would do.
 		 */
 		if (!hctx->sched_tags) {
-			ret = blk_mq_tag_update_depth(hctx, &hctx->tags,
-							min(nr, set->queue_depth),
+			if (nr > set->queue_depth) {
+				nr = set->queue_depth;
+				pr_warn("reduce nr_request to %u\n", nr);
+			}
+			ret = blk_mq_tag_update_depth(hctx, &hctx->tags, nr,
 							false);
 		} else {
 			ret = blk_mq_tag_update_depth(hctx, &hctx->sched_tags,

Shouldn't this code return -EINVAL or -ERANGE if 'nr' is too large? That will help to
keep user space code simple that updates the queue depth.

Hi Bart,

The reason why not return -EINVAL is keeping alin with minimum checking in queue_requests_store,
if you insist return -EINVAL/-ERANGE, minimum checking should also keep
same behavior. Both return error meesage and quietly changing are okey
for me. Which way do you prefer ?

static ssize_t
queue_requests_store(struct request_queue *q, const char *page, size_t count)
{
	unsigned long nr;
	int ret, err;

	if (!q->request_fn && !q->mq_ops)
		return -EINVAL;

	ret = queue_var_store(&nr, page, count);
	if (ret < 0)
		return ret;

	if (nr < BLKDEV_MIN_RQ)
		nr = BLKDEV_MIN_RQ;

Hello Jens,

Do you perhaps have a preference for one of the approaches that have been discussed
in this e-mail thread?

Thanks,

Bart.

Hello Jens,

Would you please give some comments about this patch,

If someone writes a value that's too large, return -EINVAL and
don't set it. Don't add weird debug printks.

OK, I send patch V2 soon.