linux-kernel - Re: [PATCH 7/7] blk-throtl: don't throttle the same IO multiple times

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171113111348.GF983427@devbig577.frc2.facebook.com>
Date:   Mon, 13 Nov 2017 03:13:48 -0800
From:   Tejun Heo <tj@...nel.org>
To:     Shaohua Li <shli@...nel.org>
Cc:     axboe@...nel.dk, linux-kernel@...r.kernel.org, kernel-team@...com,
        lizefan@...wei.com, hannes@...xchg.org, cgroups@...r.kernel.org,
        guro@...com
Subject: Re: [PATCH 7/7] blk-throtl: don't throttle the same IO multiple times

Hello, Shaohua.

On Sun, Nov 12, 2017 at 08:07:16PM -0800, Shaohua Li wrote:
> Thanks for looking into this while I was absence. I don't understand how this
> works. Assume a bio will be splitted into 2 small bios. In
> generic_make_request, we charge the whole bio. 'q->make_request_fn' will
> dispatch the first small bio, and call generic_make_request for the second
> small bio. Then generic_make_request charge the second small bio and we add the
> second small bio to current->bio_list[0] (please check the order). In above

You're right.  If we wanna take this approach, we need to keep the
throttled flag while cloning.  The clearing part is still correct tho.
Without that, I get 1/4 bw limit enforced.  Hmm... I'm not quite sure
where that 1/4 is coming from tho.  Will investigate more.

> code the patch changed, we pop the second small bio and set BIO_THROTTLED for
> it. But this is already too late, because generic_make_request already charged
> the second small bio.
> 
> Did you look at my original patch
> (https://marc.info/?l=linux-block&m=150791825327628&w=2), anything wrong?

That should work too but if we have to modify clone path anyway and
clear the flag when the bio leaves the queue, we don't need to add a
dedicated field to bio, right?

Thanks.

-- 
tejun