Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs

[weiping zhang]

On Thu, Sep 21, 2017 at 08:09:47AM -0600, Jens Axboe wrote:
> 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 +, 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 +, 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 100 > /sys/block/nvme0n1/queue/nr_requests
> >> cat /sys/block/nvme0n1/queue/nr_requests
> >> 100
> >>
> >> Signed-off-by: weiping zhang 
> >> ---
> >>  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, >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, 
> >> >tags, nr,
> >>false);
> >>} else {
> >>ret = blk_mq_tag_update_depth(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(, 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.

Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs

[Jens Axboe]

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 +, 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 +, 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 100 > /sys/block/nvme0n1/queue/nr_requests
>> cat /sys/block/nvme0n1/queue/nr_requests
>> 100
>>
>> Signed-off-by: weiping zhang 
>> ---
>>  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, >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, 
>> >tags, nr,
>>  false);
>>  } else {
>>  ret = blk_mq_tag_update_depth(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(, 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

Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs

[weiping zhang]

On Tue, Sep 12, 2017 at 09:57:32PM +0800, weiping zhang wrote:
> On Wed, Sep 06, 2017 at 01:00:44PM +, 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 +, 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 100 > /sys/block/nvme0n1/queue/nr_requests
> > > > > cat /sys/block/nvme0n1/queue/nr_requests
> > > > > 100
> > > > > 
> > > > > Signed-off-by: weiping zhang 
> > > > > ---
> > > > >  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, >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, 
> > > > > >tags, nr,
> > > > >   false);
> > > > >   } else {
> > > > >   ret = blk_mq_tag_update_depth(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(, 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,

Thanks
Weiping.

Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs

[weiping zhang]

On Wed, Sep 06, 2017 at 01:00:44PM +, 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 +, 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 100 > /sys/block/nvme0n1/queue/nr_requests
> > > > cat /sys/block/nvme0n1/queue/nr_requests
> > > > 100
> > > > 
> > > > Signed-off-by: weiping zhang 
> > > > ---
> > > >  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, >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, 
> > > > >tags, nr,
> > > > false);
> > > > } else {
> > > > ret = blk_mq_tag_update_depth(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(, 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,

Thanks
Weiping.

Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs

[Bart Van Assche]

On Wed, 2017-09-06 at 15:34 +0800, weiping zhang wrote:
> On Tue, Sep 05, 2017 at 03:42:45PM +, 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 100 > /sys/block/nvme0n1/queue/nr_requests
> > > cat /sys/block/nvme0n1/queue/nr_requests
> > > 100
> > > 
> > > Signed-off-by: weiping zhang 
> > > ---
> > >  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, >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, >tags, nr,
> > >   false);
> > >   } else {
> > >   ret = blk_mq_tag_update_depth(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(, 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.

Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs

[weiping zhang]

On Tue, Sep 05, 2017 at 03:42:45PM +, 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 100 > /sys/block/nvme0n1/queue/nr_requests
> > cat /sys/block/nvme0n1/queue/nr_requests
> > 100
> > 
> > Signed-off-by: weiping zhang 
> > ---
> >  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, >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, >tags, nr,
> > false);
> > } else {
> > ret = blk_mq_tag_update_depth(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(, page, count);
if (ret < 0)
return ret;

if (nr < BLKDEV_MIN_RQ)
nr = BLKDEV_MIN_RQ;

Thanks

Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs

[Ming Lei]

On Sun, Sep 3, 2017 at 9:46 PM, 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 100 > /sys/block/nvme0n1/queue/nr_requests
> cat /sys/block/nvme0n1/queue/nr_requests
> 100
>
> Signed-off-by: weiping zhang 
> ---
>  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, >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, >tags, nr,
> false);
> } else {
> ret = blk_mq_tag_update_depth(hctx, >sched_tags,

Not sure if the pr_warn() is useful, but the idea is correct:

  Reviewed-by: Ming Lei 

-- 
Ming Lei

[PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs

[weiping zhang]

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 100 > /sys/block/nvme0n1/queue/nr_requests
cat /sys/block/nvme0n1/queue/nr_requests
100

Signed-off-by: weiping zhang 
---
 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, >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, >tags, nr,
false);
} else {
ret = blk_mq_tag_update_depth(hctx, >sched_tags,
-- 
2.9.4