On 09/21/2017 07:03 AM, weiping zhang wrote:
> On Tue, Sep 12, 2017 at 09:57:32PM +0800, weiping zhang wrote:
>> On Wed, Sep 06, 2017 at 01:00:44PM +0000, Bart Van Assche wrote:
>>> On Wed, 2017-09-06 at 15:34 +0800, weiping zhang wrote:
>>>> On Tue, Sep 05, 2017 at 03:42:45PM +0000, Bart Van Assche wrote:
>>>>> On Sun, 2017-09-03 at 21:46 +0800, weiping zhang wrote:
>>>>>> 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 <zhangweiping@xxxxxxxxxxxxxxx>
>>>>>> ---
>>>>>> 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.
--
Jens Axboe
