* [PATCH V2] block: make nr_requests tunable for loop
@ 2014-06-10 2:31 Junxiao Bi
2014-06-10 2:41 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Junxiao Bi @ 2014-06-10 2:31 UTC (permalink / raw)
To: axboe; +Cc: andi, linux-kernel
commit 7b5a3522 (loop: Limit the number of requests in the bio list) limit
the request number in loop queue to not over 128. Since the "request_fn" of
loop device is null, the requests number is not allowed tuned. Make it tunable
from sysfs can improve performance.
The following test is done on a machine with 512M memory. The backend of
/dev/loop1 is a nfs file.
[root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
128
[root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
5000+0 records in
5000+0 records out
5242880000 bytes (5.2 GB) copied, 501.572 s, 10.5 MB/s
[root@bijx mnt]#
[root@bijx mnt]# echo 1024 > /sys/block/loop0/queue/nr_requests
[root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
1024
[root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
5000+0 records in
5000+0 records out
5242880000 bytes (5.2 GB) copied, 464.481 s, 11.3 MB/s
Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
---
block/blk-core.c | 6 ++++++
block/blk-sysfs.c | 9 +++------
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 40d6548..58c4bd4 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -851,6 +851,12 @@ int blk_update_nr_requests(struct request_queue *q, unsigned int nr)
q->nr_requests = nr;
blk_queue_congestion_threshold(q);
+ /* for loop device, return after set its nr_requests */
+ if (!q->request_fn) {
+ spin_unlock_irq(q->queue_lock);
+ return 0;
+ }
+
/* congestion isn't cgroup aware and follows root blkcg for now */
rl = &q->root_rl;
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 23321fb..c5456a5 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -51,9 +51,6 @@ 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;
@@ -61,10 +58,10 @@ queue_requests_store(struct request_queue *q, const char *page, size_t count)
if (nr < BLKDEV_MIN_RQ)
nr = BLKDEV_MIN_RQ;
- if (q->request_fn)
- err = blk_update_nr_requests(q, nr);
- else
+ if (q->mq_ops)
err = blk_mq_update_nr_requests(q, nr);
+ else
+ err = blk_update_nr_requests(q, nr);
if (err)
return err;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH V2] block: make nr_requests tunable for loop
2014-06-10 2:31 [PATCH V2] block: make nr_requests tunable for loop Junxiao Bi
@ 2014-06-10 2:41 ` Jens Axboe
2014-06-10 2:50 ` Junxiao Bi
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2014-06-10 2:41 UTC (permalink / raw)
To: Junxiao Bi; +Cc: andi, linux-kernel
On 2014-06-09 20:31, Junxiao Bi wrote:
> commit 7b5a3522 (loop: Limit the number of requests in the bio list) limit
> the request number in loop queue to not over 128. Since the "request_fn" of
> loop device is null, the requests number is not allowed tuned. Make it tunable
> from sysfs can improve performance.
>
> The following test is done on a machine with 512M memory. The backend of
> /dev/loop1 is a nfs file.
>
> [root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
> 128
> [root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
> 5000+0 records in
> 5000+0 records out
> 5242880000 bytes (5.2 GB) copied, 501.572 s, 10.5 MB/s
> [root@bijx mnt]#
> [root@bijx mnt]# echo 1024 > /sys/block/loop0/queue/nr_requests
> [root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
> 1024
> [root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
> 5000+0 records in
> 5000+0 records out
> 5242880000 bytes (5.2 GB) copied, 464.481 s, 11.3 MB/s
>
> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
> ---
> block/blk-core.c | 6 ++++++
> block/blk-sysfs.c | 9 +++------
> 2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 40d6548..58c4bd4 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -851,6 +851,12 @@ int blk_update_nr_requests(struct request_queue *q, unsigned int nr)
> q->nr_requests = nr;
> blk_queue_congestion_threshold(q);
>
> + /* for loop device, return after set its nr_requests */
> + if (!q->request_fn) {
> + spin_unlock_irq(q->queue_lock);
> + return 0;
> + }
It'd be prettier to split this differently - something ala:
if (request_fn)
blk_update_congestion_thresholds(q);
But I think you have a larger issue here... For the request lists, we
update the congestion thresholds and wakeup anyone waiting, if we need
to. There's no way to do that for loop, since the waitqueue is internal
to loop.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH V2] block: make nr_requests tunable for loop
2014-06-10 2:41 ` Jens Axboe
@ 2014-06-10 2:50 ` Junxiao Bi
2014-06-10 3:12 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Junxiao Bi @ 2014-06-10 2:50 UTC (permalink / raw)
To: Jens Axboe; +Cc: andi, linux-kernel
On 06/10/2014 10:41 AM, Jens Axboe wrote:
> On 2014-06-09 20:31, Junxiao Bi wrote:
>> commit 7b5a3522 (loop: Limit the number of requests in the bio list)
>> limit
>> the request number in loop queue to not over 128. Since the
>> "request_fn" of
>> loop device is null, the requests number is not allowed tuned. Make
>> it tunable
>> from sysfs can improve performance.
>>
>> The following test is done on a machine with 512M memory. The backend of
>> /dev/loop1 is a nfs file.
>>
>> [root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
>> 128
>> [root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
>> 5000+0 records in
>> 5000+0 records out
>> 5242880000 bytes (5.2 GB) copied, 501.572 s, 10.5 MB/s
>> [root@bijx mnt]#
>> [root@bijx mnt]# echo 1024 > /sys/block/loop0/queue/nr_requests
>> [root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
>> 1024
>> [root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
>> 5000+0 records in
>> 5000+0 records out
>> 5242880000 bytes (5.2 GB) copied, 464.481 s, 11.3 MB/s
>>
>> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
>> ---
>> block/blk-core.c | 6 ++++++
>> block/blk-sysfs.c | 9 +++------
>> 2 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 40d6548..58c4bd4 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -851,6 +851,12 @@ int blk_update_nr_requests(struct request_queue
>> *q, unsigned int nr)
>> q->nr_requests = nr;
>> blk_queue_congestion_threshold(q);
>>
>> + /* for loop device, return after set its nr_requests */
>> + if (!q->request_fn) {
>> + spin_unlock_irq(q->queue_lock);
>> + return 0;
>> + }
>
> It'd be prettier to split this differently - something ala:
>
> if (request_fn)
> blk_update_congestion_thresholds(q);
The congestion threshholds is needed in commit 7b5a3522 (loop: Limit the
number of requests in the bio list). So I think it needs be set even
request_fn is null.
>
> But I think you have a larger issue here... For the request lists, we
> update the congestion thresholds and wakeup anyone waiting, if we need
> to. There's no way to do that for loop, since the waitqueue is
> internal to loop.
Loop do the congestion control by itself, in loop_make_request() /
loop_thread().
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH V2] block: make nr_requests tunable for loop
2014-06-10 2:50 ` Junxiao Bi
@ 2014-06-10 3:12 ` Jens Axboe
2014-06-10 3:29 ` Junxiao Bi
2014-06-11 2:40 ` Junxiao Bi
0 siblings, 2 replies; 6+ messages in thread
From: Jens Axboe @ 2014-06-10 3:12 UTC (permalink / raw)
To: Junxiao Bi; +Cc: andi, linux-kernel
On 2014-06-09 20:50, Junxiao Bi wrote:
> On 06/10/2014 10:41 AM, Jens Axboe wrote:
>> On 2014-06-09 20:31, Junxiao Bi wrote:
>>> commit 7b5a3522 (loop: Limit the number of requests in the bio list)
>>> limit
>>> the request number in loop queue to not over 128. Since the
>>> "request_fn" of
>>> loop device is null, the requests number is not allowed tuned. Make
>>> it tunable
>>> from sysfs can improve performance.
>>>
>>> The following test is done on a machine with 512M memory. The backend of
>>> /dev/loop1 is a nfs file.
>>>
>>> [root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
>>> 128
>>> [root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
>>> 5000+0 records in
>>> 5000+0 records out
>>> 5242880000 bytes (5.2 GB) copied, 501.572 s, 10.5 MB/s
>>> [root@bijx mnt]#
>>> [root@bijx mnt]# echo 1024 > /sys/block/loop0/queue/nr_requests
>>> [root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
>>> 1024
>>> [root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
>>> 5000+0 records in
>>> 5000+0 records out
>>> 5242880000 bytes (5.2 GB) copied, 464.481 s, 11.3 MB/s
>>>
>>> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
>>> ---
>>> block/blk-core.c | 6 ++++++
>>> block/blk-sysfs.c | 9 +++------
>>> 2 files changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>> index 40d6548..58c4bd4 100644
>>> --- a/block/blk-core.c
>>> +++ b/block/blk-core.c
>>> @@ -851,6 +851,12 @@ int blk_update_nr_requests(struct request_queue
>>> *q, unsigned int nr)
>>> q->nr_requests = nr;
>>> blk_queue_congestion_threshold(q);
>>>
>>> + /* for loop device, return after set its nr_requests */
>>> + if (!q->request_fn) {
>>> + spin_unlock_irq(q->queue_lock);
>>> + return 0;
>>> + }
>>
>> It'd be prettier to split this differently - something ala:
>>
>> if (request_fn)
>> blk_update_congestion_thresholds(q);
> The congestion threshholds is needed in commit 7b5a3522 (loop: Limit the
> number of requests in the bio list). So I think it needs be set even
> request_fn is null.
I mean the request list thresholds, the part below where you currently
just exit.
>> But I think you have a larger issue here... For the request lists, we
>> update the congestion thresholds and wakeup anyone waiting, if we need
>> to. There's no way to do that for loop, since the waitqueue is
>> internal to loop.
> Loop do the congestion control by itself, in loop_make_request() /
> loop_thread().
Yes, that is my point! You update nr_congestion_off, but you don't wake
anyone currently sitting in wait_event_lock_irq() on that value. See
what the code below where you just exit does for request list based devices.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH V2] block: make nr_requests tunable for loop
2014-06-10 3:12 ` Jens Axboe
@ 2014-06-10 3:29 ` Junxiao Bi
2014-06-11 2:40 ` Junxiao Bi
1 sibling, 0 replies; 6+ messages in thread
From: Junxiao Bi @ 2014-06-10 3:29 UTC (permalink / raw)
To: Jens Axboe; +Cc: andi, linux-kernel
On 06/10/2014 11:12 AM, Jens Axboe wrote:
> On 2014-06-09 20:50, Junxiao Bi wrote:
>> On 06/10/2014 10:41 AM, Jens Axboe wrote:
>>> On 2014-06-09 20:31, Junxiao Bi wrote:
>>>> commit 7b5a3522 (loop: Limit the number of requests in the bio list)
>>>> limit
>>>> the request number in loop queue to not over 128. Since the
>>>> "request_fn" of
>>>> loop device is null, the requests number is not allowed tuned. Make
>>>> it tunable
>>>> from sysfs can improve performance.
>>>>
>>>> The following test is done on a machine with 512M memory. The
>>>> backend of
>>>> /dev/loop1 is a nfs file.
>>>>
>>>> [root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
>>>> 128
>>>> [root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
>>>> 5000+0 records in
>>>> 5000+0 records out
>>>> 5242880000 bytes (5.2 GB) copied, 501.572 s, 10.5 MB/s
>>>> [root@bijx mnt]#
>>>> [root@bijx mnt]# echo 1024 > /sys/block/loop0/queue/nr_requests
>>>> [root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
>>>> 1024
>>>> [root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
>>>> 5000+0 records in
>>>> 5000+0 records out
>>>> 5242880000 bytes (5.2 GB) copied, 464.481 s, 11.3 MB/s
>>>>
>>>> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
>>>> ---
>>>> block/blk-core.c | 6 ++++++
>>>> block/blk-sysfs.c | 9 +++------
>>>> 2 files changed, 9 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>> index 40d6548..58c4bd4 100644
>>>> --- a/block/blk-core.c
>>>> +++ b/block/blk-core.c
>>>> @@ -851,6 +851,12 @@ int blk_update_nr_requests(struct request_queue
>>>> *q, unsigned int nr)
>>>> q->nr_requests = nr;
>>>> blk_queue_congestion_threshold(q);
>>>>
>>>> + /* for loop device, return after set its nr_requests */
>>>> + if (!q->request_fn) {
>>>> + spin_unlock_irq(q->queue_lock);
>>>> + return 0;
>>>> + }
>>>
>>> It'd be prettier to split this differently - something ala:
>>>
>>> if (request_fn)
>>> blk_update_congestion_thresholds(q);
>> The congestion threshholds is needed in commit 7b5a3522 (loop: Limit the
>> number of requests in the bio list). So I think it needs be set even
>> request_fn is null.
>
> I mean the request list thresholds, the part below where you currently
> just exit.
>
>>> But I think you have a larger issue here... For the request lists, we
>>> update the congestion thresholds and wakeup anyone waiting, if we need
>>> to. There's no way to do that for loop, since the waitqueue is
>>> internal to loop.
>> Loop do the congestion control by itself, in loop_make_request() /
>> loop_thread().
>
> Yes, that is my point! You update nr_congestion_off, but you don't
> wake anyone currently sitting in wait_event_lock_irq() on that value.
> See what the code below where you just exit does for request list
> based devices.
Ah, i see. It can't be wake up once nr_congestion_off is updated. But
after a little delay, loop_thread will consume the requests in list and
wake up it. Is this OK?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH V2] block: make nr_requests tunable for loop
2014-06-10 3:12 ` Jens Axboe
2014-06-10 3:29 ` Junxiao Bi
@ 2014-06-11 2:40 ` Junxiao Bi
1 sibling, 0 replies; 6+ messages in thread
From: Junxiao Bi @ 2014-06-11 2:40 UTC (permalink / raw)
To: Jens Axboe; +Cc: andi, linux-kernel
On 06/10/2014 11:12 AM, Jens Axboe wrote:
> On 2014-06-09 20:50, Junxiao Bi wrote:
>> On 06/10/2014 10:41 AM, Jens Axboe wrote:
>>> On 2014-06-09 20:31, Junxiao Bi wrote:
>>>> commit 7b5a3522 (loop: Limit the number of requests in the bio list)
>>>> limit
>>>> the request number in loop queue to not over 128. Since the
>>>> "request_fn" of
>>>> loop device is null, the requests number is not allowed tuned. Make
>>>> it tunable
>>>> from sysfs can improve performance.
>>>>
>>>> The following test is done on a machine with 512M memory. The
>>>> backend of
>>>> /dev/loop1 is a nfs file.
>>>>
>>>> [root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
>>>> 128
>>>> [root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
>>>> 5000+0 records in
>>>> 5000+0 records out
>>>> 5242880000 bytes (5.2 GB) copied, 501.572 s, 10.5 MB/s
>>>> [root@bijx mnt]#
>>>> [root@bijx mnt]# echo 1024 > /sys/block/loop0/queue/nr_requests
>>>> [root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
>>>> 1024
>>>> [root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
>>>> 5000+0 records in
>>>> 5000+0 records out
>>>> 5242880000 bytes (5.2 GB) copied, 464.481 s, 11.3 MB/s
>>>>
>>>> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
>>>> ---
>>>> block/blk-core.c | 6 ++++++
>>>> block/blk-sysfs.c | 9 +++------
>>>> 2 files changed, 9 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>> index 40d6548..58c4bd4 100644
>>>> --- a/block/blk-core.c
>>>> +++ b/block/blk-core.c
>>>> @@ -851,6 +851,12 @@ int blk_update_nr_requests(struct request_queue
>>>> *q, unsigned int nr)
>>>> q->nr_requests = nr;
>>>> blk_queue_congestion_threshold(q);
>>>>
>>>> + /* for loop device, return after set its nr_requests */
>>>> + if (!q->request_fn) {
>>>> + spin_unlock_irq(q->queue_lock);
>>>> + return 0;
>>>> + }
>>>
>>> It'd be prettier to split this differently - something ala:
>>>
>>> if (request_fn)
>>> blk_update_congestion_thresholds(q);
>> The congestion threshholds is needed in commit 7b5a3522 (loop: Limit the
>> number of requests in the bio list). So I think it needs be set even
>> request_fn is null.
>
> I mean the request list thresholds, the part below where you currently
> just exit.
>
>>> But I think you have a larger issue here... For the request lists, we
>>> update the congestion thresholds and wakeup anyone waiting, if we need
>>> to. There's no way to do that for loop, since the waitqueue is
>>> internal to loop.
>> Loop do the congestion control by itself, in loop_make_request() /
>> loop_thread().
>
> Yes, that is my point! You update nr_congestion_off, but you don't
> wake anyone currently sitting in wait_event_lock_irq() on that value.
> See what the code below where you just exit does for request list
> based devices.
Jens, do you have an idea to resolve it?
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-11 2:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-10 2:31 [PATCH V2] block: make nr_requests tunable for loop Junxiao Bi
2014-06-10 2:41 ` Jens Axboe
2014-06-10 2:50 ` Junxiao Bi
2014-06-10 3:12 ` Jens Axboe
2014-06-10 3:29 ` Junxiao Bi
2014-06-11 2:40 ` Junxiao Bi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox