* [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs @ 2017-09-03 13:46 weiping zhang 2017-09-04 10:02 ` Ming Lei 2017-09-05 15:42 ` Bart Van Assche 0 siblings, 2 replies; 9+ messages in thread From: weiping zhang @ 2017-09-03 13:46 UTC (permalink / raw) To: axboe; +Cc: linux-block 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@didichuxing.com> --- 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, -- 2.9.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs 2017-09-03 13:46 [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs weiping zhang @ 2017-09-04 10:02 ` Ming Lei 2017-09-05 15:42 ` Bart Van Assche 1 sibling, 0 replies; 9+ messages in thread From: Ming Lei @ 2017-09-04 10:02 UTC (permalink / raw) To: Jens Axboe, linux-block On Sun, Sep 3, 2017 at 9:46 PM, weiping zhang <zhangweiping@didichuxing.com> 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@didichuxing.com> > --- > 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, Not sure if the pr_warn() is useful, but the idea is correct: Reviewed-by: Ming Lei <ming.lei@redhat.com> -- Ming Lei ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs 2017-09-03 13:46 [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs weiping zhang 2017-09-04 10:02 ` Ming Lei @ 2017-09-05 15:42 ` Bart Van Assche 2017-09-06 7:34 ` weiping zhang 1 sibling, 1 reply; 9+ messages in thread From: Bart Van Assche @ 2017-09-05 15:42 UTC (permalink / raw) To: zhangweiping@didichuxing.com, axboe@kernel.dk; +Cc: linux-block@vger.kernel.org T24gU3VuLCAyMDE3LTA5LTAzIGF0IDIxOjQ2ICswODAwLCB3ZWlwaW5nIHpoYW5nIHdyb3RlOg0K PiBpZiBibGstbXEgdXNlICJub25lIiBpbyBzY2hlZHVsZXIsIG5yX3JlcXVlc3QgZ2V0IGEgd3Jv bmcgdmFsdWUgd2hlbg0KPiBpbnB1dCBhIG51bWJlciA+IHRhZ19zZXQtPnF1ZXVlX2RlcHRoLiBi bGtfbXFfdGFnX3VwZGF0ZV9kZXB0aCB3aWxsIGdldA0KPiB0aGUgc21hbGxlciBvbmUgbWluKG5y LCBzZXQtPnF1ZXVlX2RlcHRoKSwgYW5kIHRoZW4gcS0+bnJfcmVxdWVzdCBnZXQgYQ0KPiB3cm9u ZyB2YWx1ZS4NCj4gDQo+IFJlcHJvZHVjZToNCj4gDQo+IGVjaG8gbm9uZSA+IC9zeXMvYmxvY2sv bnZtZTBuMS9xdWV1ZS9pb3NjaGVkdWxlcg0KPiBlY2hvIDEwMDAwMDAgPiAvc3lzL2Jsb2NrL252 bWUwbjEvcXVldWUvbnJfcmVxdWVzdHMNCj4gY2F0IC9zeXMvYmxvY2svbnZtZTBuMS9xdWV1ZS9u cl9yZXF1ZXN0cw0KPiAxMDAwMDAwDQo+IA0KPiBTaWduZWQtb2ZmLWJ5OiB3ZWlwaW5nIHpoYW5n IDx6aGFuZ3dlaXBpbmdAZGlkaWNodXhpbmcuY29tPg0KPiAtLS0NCj4gIGJsb2NrL2Jsay1tcS5j IHwgNyArKysrKy0tDQo+ICAxIGZpbGUgY2hhbmdlZCwgNSBpbnNlcnRpb25zKCspLCAyIGRlbGV0 aW9ucygtKQ0KPiANCj4gZGlmZiAtLWdpdCBhL2Jsb2NrL2Jsay1tcS5jIGIvYmxvY2svYmxrLW1x LmMNCj4gaW5kZXggZjg0ZDE0NS4uODMwM2U1ZSAxMDA2NDQNCj4gLS0tIGEvYmxvY2svYmxrLW1x LmMNCj4gKysrIGIvYmxvY2svYmxrLW1xLmMNCj4gQEAgLTI2MjIsOCArMjYyMiwxMSBAQCBpbnQg YmxrX21xX3VwZGF0ZV9ucl9yZXF1ZXN0cyhzdHJ1Y3QgcmVxdWVzdF9xdWV1ZSAqcSwgdW5zaWdu ZWQgaW50IG5yKQ0KPiAgCQkgKiBxdWV1ZSBkZXB0aC4gVGhpcyBpcyBzaW1pbGFyIHRvIHdoYXQg dGhlIG9sZCBjb2RlIHdvdWxkIGRvLg0KPiAgCQkgKi8NCj4gIAkJaWYgKCFoY3R4LT5zY2hlZF90 YWdzKSB7DQo+IC0JCQlyZXQgPSBibGtfbXFfdGFnX3VwZGF0ZV9kZXB0aChoY3R4LCAmaGN0eC0+ dGFncywNCj4gLQkJCQkJCQltaW4obnIsIHNldC0+cXVldWVfZGVwdGgpLA0KPiArCQkJaWYgKG5y ID4gc2V0LT5xdWV1ZV9kZXB0aCkgew0KPiArCQkJCW5yID0gc2V0LT5xdWV1ZV9kZXB0aDsNCj4g KwkJCQlwcl93YXJuKCJyZWR1Y2UgbnJfcmVxdWVzdCB0byAldVxuIiwgbnIpOw0KPiArCQkJfQ0K PiArCQkJcmV0ID0gYmxrX21xX3RhZ191cGRhdGVfZGVwdGgoaGN0eCwgJmhjdHgtPnRhZ3MsIG5y LA0KPiAgCQkJCQkJCWZhbHNlKTsNCj4gIAkJfSBlbHNlIHsNCj4gIAkJCXJldCA9IGJsa19tcV90 YWdfdXBkYXRlX2RlcHRoKGhjdHgsICZoY3R4LT5zY2hlZF90YWdzLA0KDQpTaG91bGRuJ3QgdGhp cyBjb2RlIHJldHVybiAtRUlOVkFMIG9yIC1FUkFOR0UgaWYgJ25yJyBpcyB0b28gbGFyZ2U/IFRo YXQgd2lsbCBoZWxwIHRvDQprZWVwIHVzZXIgc3BhY2UgY29kZSBzaW1wbGUgdGhhdCB1cGRhdGVz IHRoZSBxdWV1ZSBkZXB0aC4NCg0KVGhhbmtzLA0KDQpCYXJ0Lg== ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs 2017-09-05 15:42 ` Bart Van Assche @ 2017-09-06 7:34 ` weiping zhang 2017-09-06 13:00 ` Bart Van Assche 0 siblings, 1 reply; 9+ messages in thread From: weiping zhang @ 2017-09-06 7:34 UTC (permalink / raw) To: Bart Van Assche; +Cc: axboe@kernel.dk, linux-block@vger.kernel.org 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@didichuxing.com> > > --- > > 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; Thanks ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs 2017-09-06 7:34 ` weiping zhang @ 2017-09-06 13:00 ` Bart Van Assche 2017-09-12 13:57 ` weiping zhang 0 siblings, 1 reply; 9+ messages in thread From: Bart Van Assche @ 2017-09-06 13:00 UTC (permalink / raw) To: zhangweiping@didichuxing.com; +Cc: linux-block@vger.kernel.org, axboe@kernel.dk T24gV2VkLCAyMDE3LTA5LTA2IGF0IDE1OjM0ICswODAwLCB3ZWlwaW5nIHpoYW5nIHdyb3RlOg0K PiBPbiBUdWUsIFNlcCAwNSwgMjAxNyBhdCAwMzo0Mjo0NVBNICswMDAwLCBCYXJ0IFZhbiBBc3Nj aGUgd3JvdGU6DQo+ID4gT24gU3VuLCAyMDE3LTA5LTAzIGF0IDIxOjQ2ICswODAwLCB3ZWlwaW5n IHpoYW5nIHdyb3RlOg0KPiA+ID4gaWYgYmxrLW1xIHVzZSAibm9uZSIgaW8gc2NoZWR1bGVyLCBu cl9yZXF1ZXN0IGdldCBhIHdyb25nIHZhbHVlIHdoZW4NCj4gPiA+IGlucHV0IGEgbnVtYmVyID4g dGFnX3NldC0+cXVldWVfZGVwdGguIGJsa19tcV90YWdfdXBkYXRlX2RlcHRoIHdpbGwgZ2V0DQo+ ID4gPiB0aGUgc21hbGxlciBvbmUgbWluKG5yLCBzZXQtPnF1ZXVlX2RlcHRoKSwgYW5kIHRoZW4g cS0+bnJfcmVxdWVzdCBnZXQgYQ0KPiA+ID4gd3JvbmcgdmFsdWUuDQo+ID4gPiANCj4gPiA+IFJl cHJvZHVjZToNCj4gPiA+IA0KPiA+ID4gZWNobyBub25lID4gL3N5cy9ibG9jay9udm1lMG4xL3F1 ZXVlL2lvc2NoZWR1bGVyDQo+ID4gPiBlY2hvIDEwMDAwMDAgPiAvc3lzL2Jsb2NrL252bWUwbjEv cXVldWUvbnJfcmVxdWVzdHMNCj4gPiA+IGNhdCAvc3lzL2Jsb2NrL252bWUwbjEvcXVldWUvbnJf cmVxdWVzdHMNCj4gPiA+IDEwMDAwMDANCj4gPiA+IA0KPiA+ID4gU2lnbmVkLW9mZi1ieTogd2Vp cGluZyB6aGFuZyA8emhhbmd3ZWlwaW5nQGRpZGljaHV4aW5nLmNvbT4NCj4gPiA+IC0tLQ0KPiA+ ID4gIGJsb2NrL2Jsay1tcS5jIHwgNyArKysrKy0tDQo+ID4gPiAgMSBmaWxlIGNoYW5nZWQsIDUg aW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkNCj4gPiA+IA0KPiA+ID4gZGlmZiAtLWdpdCBh L2Jsb2NrL2Jsay1tcS5jIGIvYmxvY2svYmxrLW1xLmMNCj4gPiA+IGluZGV4IGY4NGQxNDUuLjgz MDNlNWUgMTAwNjQ0DQo+ID4gPiAtLS0gYS9ibG9jay9ibGstbXEuYw0KPiA+ID4gKysrIGIvYmxv Y2svYmxrLW1xLmMNCj4gPiA+IEBAIC0yNjIyLDggKzI2MjIsMTEgQEAgaW50IGJsa19tcV91cGRh dGVfbnJfcmVxdWVzdHMoc3RydWN0IHJlcXVlc3RfcXVldWUgKnEsIHVuc2lnbmVkIGludCBucikN Cj4gPiA+ICAJCSAqIHF1ZXVlIGRlcHRoLiBUaGlzIGlzIHNpbWlsYXIgdG8gd2hhdCB0aGUgb2xk IGNvZGUgd291bGQgZG8uDQo+ID4gPiAgCQkgKi8NCj4gPiA+ICAJCWlmICghaGN0eC0+c2NoZWRf dGFncykgew0KPiA+ID4gLQkJCXJldCA9IGJsa19tcV90YWdfdXBkYXRlX2RlcHRoKGhjdHgsICZo Y3R4LT50YWdzLA0KPiA+ID4gLQkJCQkJCQltaW4obnIsIHNldC0+cXVldWVfZGVwdGgpLA0KPiA+ ID4gKwkJCWlmIChuciA+IHNldC0+cXVldWVfZGVwdGgpIHsNCj4gPiA+ICsJCQkJbnIgPSBzZXQt PnF1ZXVlX2RlcHRoOw0KPiA+ID4gKwkJCQlwcl93YXJuKCJyZWR1Y2UgbnJfcmVxdWVzdCB0byAl dVxuIiwgbnIpOw0KPiA+ID4gKwkJCX0NCj4gPiA+ICsJCQlyZXQgPSBibGtfbXFfdGFnX3VwZGF0 ZV9kZXB0aChoY3R4LCAmaGN0eC0+dGFncywgbnIsDQo+ID4gPiAgCQkJCQkJCWZhbHNlKTsNCj4g PiA+ICAJCX0gZWxzZSB7DQo+ID4gPiAgCQkJcmV0ID0gYmxrX21xX3RhZ191cGRhdGVfZGVwdGgo aGN0eCwgJmhjdHgtPnNjaGVkX3RhZ3MsDQo+ID4gDQo+ID4gU2hvdWxkbid0IHRoaXMgY29kZSBy ZXR1cm4gLUVJTlZBTCBvciAtRVJBTkdFIGlmICducicgaXMgdG9vIGxhcmdlPyBUaGF0IHdpbGwg aGVscCB0bw0KPiA+IGtlZXAgdXNlciBzcGFjZSBjb2RlIHNpbXBsZSB0aGF0IHVwZGF0ZXMgdGhl IHF1ZXVlIGRlcHRoLg0KPiANCj4gSGkgQmFydCwNCj4gDQo+IFRoZSByZWFzb24gd2h5IG5vdCBy ZXR1cm4gLUVJTlZBTCBpcyBrZWVwaW5nIGFsaW4gd2l0aCBtaW5pbXVtIGNoZWNraW5nIGluIHF1 ZXVlX3JlcXVlc3RzX3N0b3JlLA0KPiBpZiB5b3UgaW5zaXN0IHJldHVybiAtRUlOVkFMLy1FUkFO R0UsIG1pbmltdW0gY2hlY2tpbmcgc2hvdWxkIGFsc28ga2VlcA0KPiBzYW1lIGJlaGF2aW9yLiBC b3RoIHJldHVybiBlcnJvciBtZWVzYWdlIGFuZCBxdWlldGx5IGNoYW5naW5nIGFyZSBva2V5DQo+ IGZvciBtZS4gV2hpY2ggd2F5IGRvIHlvdSBwcmVmZXIgPw0KPiANCj4gc3RhdGljIHNzaXplX3QN Cj4gcXVldWVfcmVxdWVzdHNfc3RvcmUoc3RydWN0IHJlcXVlc3RfcXVldWUgKnEsIGNvbnN0IGNo YXIgKnBhZ2UsIHNpemVfdCBjb3VudCkNCj4gew0KPiAJdW5zaWduZWQgbG9uZyBucjsNCj4gCWlu dCByZXQsIGVycjsNCj4gDQo+IAlpZiAoIXEtPnJlcXVlc3RfZm4gJiYgIXEtPm1xX29wcykNCj4g CQlyZXR1cm4gLUVJTlZBTDsNCj4gDQo+IAlyZXQgPSBxdWV1ZV92YXJfc3RvcmUoJm5yLCBwYWdl LCBjb3VudCk7DQo+IAlpZiAocmV0IDwgMCkNCj4gCQlyZXR1cm4gcmV0Ow0KPiANCj4gCWlmIChu ciA8IEJMS0RFVl9NSU5fUlEpDQo+IAkJbnIgPSBCTEtERVZfTUlOX1JROw0KDQpIZWxsbyBKZW5z LA0KDQpEbyB5b3UgcGVyaGFwcyBoYXZlIGEgcHJlZmVyZW5jZSBmb3Igb25lIG9mIHRoZSBhcHBy b2FjaGVzIHRoYXQgaGF2ZSBiZWVuIGRpc2N1c3NlZA0KaW4gdGhpcyBlLW1haWwgdGhyZWFkPw0K DQpUaGFua3MsDQoNCkJhcnQu ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs 2017-09-06 13:00 ` Bart Van Assche @ 2017-09-12 13:57 ` weiping zhang 2017-09-21 13:03 ` weiping zhang 0 siblings, 1 reply; 9+ messages in thread From: weiping zhang @ 2017-09-12 13:57 UTC (permalink / raw) To: axboe, Bart Van Assche; +Cc: linux-block@vger.kernel.org, axboe@kernel.dk 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@didichuxing.com> > > > > --- > > > > 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, Thanks Weiping. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs 2017-09-12 13:57 ` weiping zhang @ 2017-09-21 13:03 ` weiping zhang 2017-09-21 14:09 ` Jens Axboe 0 siblings, 1 reply; 9+ messages in thread From: weiping zhang @ 2017-09-21 13:03 UTC (permalink / raw) To: axboe, Bart Van Assche; +Cc: linux-block@vger.kernel.org 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@didichuxing.com> > > > > > --- > > > > > 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, Thanks Weiping. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs 2017-09-21 13:03 ` weiping zhang @ 2017-09-21 14:09 ` Jens Axboe 2017-09-21 15:14 ` weiping zhang 0 siblings, 1 reply; 9+ messages in thread From: Jens Axboe @ 2017-09-21 14:09 UTC (permalink / raw) To: Bart Van Assche, linux-block@vger.kernel.org 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@didichuxing.com> >>>>>> --- >>>>>> 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs 2017-09-21 14:09 ` Jens Axboe @ 2017-09-21 15:14 ` weiping zhang 0 siblings, 0 replies; 9+ messages in thread From: weiping zhang @ 2017-09-21 15:14 UTC (permalink / raw) To: Jens Axboe; +Cc: Bart Van Assche, linux-block@vger.kernel.org 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 +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@didichuxing.com> > >>>>>> --- > >>>>>> 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-09-21 15:14 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-09-03 13:46 [PATCH] blk-mq: fix nr_requests wrong value when modify it from sysfs weiping zhang 2017-09-04 10:02 ` Ming Lei 2017-09-05 15:42 ` Bart Van Assche 2017-09-06 7:34 ` weiping zhang 2017-09-06 13:00 ` Bart Van Assche 2017-09-12 13:57 ` weiping zhang 2017-09-21 13:03 ` weiping zhang 2017-09-21 14:09 ` Jens Axboe 2017-09-21 15:14 ` weiping zhang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.