linux-kernel - Re: [PATCH V2] block: make nr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 11 Jun 2014 10:40:34 +0800
From:	Junxiao Bi <junxiao.bi@...cle.com>
To:	Jens Axboe <axboe@...nel.dk>
CC:	andi@...as.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] block: make nr_requests tunable for loop

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@...x mnt]# cat /sys/block/loop0/queue/nr_requests
>>>> 128
>>>> [root@...x 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@...x mnt]#
>>>> [root@...x mnt]# echo 1024 > /sys/block/loop0/queue/nr_requests
>>>> [root@...x mnt]# cat /sys/block/loop0/queue/nr_requests
>>>> 1024
>>>> [root@...x 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@...cle.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?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/