fix wrong value when user modify nr_request by sysfs

PATCH Superseded weiping zhang 2 participants 7 messages Sep 21, 2017 Block layer

lore

Share & tools

lore

weiping zhang

author

2017-09-21 15:16 UTC #0 lore raw

v2 cover fix wrong value when user modify nr_request by sysfs

Hi Jens,

This is v2 of fixing nr_request.

v1 -> v2:

blk-mq: fix nr_requests wrong value when modify it from sysfs this patch return -EINVAL when user write a value that's too large.

blk-sysfs: return EINVAL when user modify nr_request less than
BLKDEV_MIN_RQ
In order to keep same behavior with former patch, also return EINVAL when user write a value less than BLKDEV_MIN_RQ

weiping zhang (2):
blk-mq: fix nr_requests wrong value when modify it from sysfs
blk-sysfs: return EINVAL when user modify nr_request less than
BLKDEV_MIN_RQ

 block/blk-mq.c    | 8 ++++++--
 block/blk-sysfs.c | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

Signature · 1 line

2.9.4

weiping zhang

author

2017-09-21 15:17 UTC #1 lore raw

PATCH v2 1/2 blk-mq: fix nr_requests wrong value when modify it from sysfs

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 <[email protected]>

---
 block/blk-mq.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

block/blk-mq.c +6 -2

index 98a1860..479c35a 100644
@@ -2642,8 +2642,12 @@ 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) {
+				ret = -EINVAL;
+				break;
+			}
+
+			ret = blk_mq_tag_update_depth(hctx, &hctx->tags, nr,
 							false);
 		} else {
 			ret = blk_mq_tag_update_depth(hctx, &hctx->sched_tags,

Signature · 1 line

2.9.4

Jens Axboe

maintainer

2017-09-21 16:37 UTC #2 lore raw

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

Quoted 24 lines · 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 1000000 > /sys/block/nvme0n1/queue/nr_requests cat /sys/block/nvme0n1/queue/nr_requests 1000000
Signed-off-by weiping zhang <[email protected]>
---
 block/blk-mq.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
block/blk-mq.c +0 -0
index 98a1860..479c35a 100644
@@ -2642,8 +2642,12 @@ 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.
 		 */

block/blk-mq.c int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr) · line 2651

Quoted 10 lines

block/blk-mq.c +6 -2

 		if (!hctx->sched_tags) {
-			ret = blk_mq_tag_update_depth(hctx, &hctx->tags,
-							min(nr, set->queue_depth),
+			if (nr > set->queue_depth) {
+				ret = -EINVAL;
+				break;
+			}
+
+			ret = blk_mq_tag_update_depth(hctx, &hctx->tags, nr,
 							false);

What am I missing here? blk_mq_tag_update_depth() should already return -EINVAL for the case where we can't grow the tags. Looks like this patch should simply remove the min(nr, set->queue_depth) and just pass in 'nr'. Should not need the duplicated check for depth.

Signature · 1 line

Jens Axboe

weiping zhang

author

2017-09-21 17:09 UTC #3 lore raw

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

Quoted 32 lines · Jens Axboe

Quoted 31 lines · 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 1000000 > /sys/block/nvme0n1/queue/nr_requests cat /sys/block/nvme0n1/queue/nr_requests 1000000
Signed-off-by weiping zhang <[email protected]>
---
 block/blk-mq.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
block/blk-mq.c +4 -2
index 98a1860..479c35a 100644
@@ -2642,8 +2642,12 @@ 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) {
+				ret = -EINVAL;
+				break;
+			}

Quoted 9 lines

block/blk-mq.c int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr) · line 2651
Quoted 3 lines
block/blk-mq.c +2 -0
+
+			ret = blk_mq_tag_update_depth(hctx, &hctx->tags, nr,
 							false);
What am I missing here? blk_mq_tag_update_depth() should already return -EINVAL for the case where we can't grow the tags. Looks like this patch should simply remove the min(nr, set->queue_depth) and just pass in 'nr'. Should not need the duplicated check for depth.

Ya, you are right, I will send V3 later.

Thanks

weiping zhang

author

2017-09-21 15:17 UTC #4 lore raw

PATCH v2 2/2 blk-sysfs: return EINVAL when user modify nr_request less than BLKDEV_MIN_RQ

In reply to weiping zhang

Signed-off-by weiping zhang <[email protected]>

---
 block/blk-sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

block/blk-sysfs.c +1 -1

index b8362c0..03a6e19 100644
@@ -75,7 +75,7 @@ queue_requests_store(struct request_queue *q, const char *page, size_t count)
 		return ret;
 
 	if (nr < BLKDEV_MIN_RQ)
-		nr = BLKDEV_MIN_RQ;
+		return -EINVAL;
 
 	if (q->request_fn)
 		err = blk_update_nr_requests(q, nr);

Signature · 1 line

2.9.4

Jens Axboe

maintainer

2017-09-21 16:38 UTC #5 lore raw

Re: [PATCH v2 2/2] blk-sysfs: return EINVAL when user modify nr_request less than BLKDEV_MIN_RQ

Quoted 10 lines · weiping zhang

Signed-off-by weiping zhang <[email protected]>
---
 block/blk-sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
block/blk-sysfs.c +0 -0
index b8362c0..03a6e19 100644

block/blk-sysfs.c queue_requests_store(struct request_queue *q, const char *page, size_t count) · line 78

Quoted 5 lines

block/blk-sysfs.c +1 -1

@@ -75,7 +75,7 @@ queue_requests_store(struct request_queue *q, const char *page, size_t count)
 		return ret;
 
 	if (nr < BLKDEV_MIN_RQ)
-		nr = BLKDEV_MIN_RQ;
+		return -EINVAL;

This is potentially breaking existing scripts.

Signature · 1 line

Jens Axboe

weiping zhang

author

2017-09-21 17:12 UTC #6 lore raw

Re: [PATCH v2 2/2] blk-sysfs: return EINVAL when user modify nr_request less than BLKDEV_MIN_RQ

Quoted 13 lines · Jens Axboe

Quoted 12 lines · weiping zhang

Signed-off-by weiping zhang <[email protected]>

---
 block/blk-sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

block/blk-sysfs.c +0 -0

index b8362c0..03a6e19 100644
@@ -75,7 +75,7 @@ queue_requests_store(struct request_queue *q, const char *page, size_t count)
 		return ret;

Quoted 6 lines

block/blk-sysfs.c queue_requests_store(struct request_queue *q, const char *page, size_t count) · line 78
Quoted 3 lines
block/blk-sysfs.c +1 -1
 	if (nr < BLKDEV_MIN_RQ)
-		nr = BLKDEV_MIN_RQ;
+		return -EINVAL;
This is potentially breaking existing scripts.

I just want this keep same behavior with the too larger case, If this may make other side effect, I drop this patch.

Thanks
weiping