linux-kernel - Re: [PATCH 02/11] blk-throttle: Fix that bps of child could exceed bps limited in parent

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: <Y35h9SmFeEJtBNgM@slm.duckdns.org>
Date:   Wed, 23 Nov 2022 08:09:57 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Kemeng Shi <shikemeng@...wei.com>
Cc:     josef@...icpanda.com, axboe@...nel.dk, cgroups@...r.kernel.org,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/11] blk-throttle: Fix that bps of child could exceed
 bps limited in parent

On Wed, Nov 23, 2022 at 02:03:52PM +0800, Kemeng Shi wrote:
> @@ -964,10 +963,8 @@ static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio)
>  	unsigned int bio_size = throtl_bio_data_size(bio);
>  
>  	/* Charge the bio to the group */
> -	if (!bio_flagged(bio, BIO_BPS_THROTTLED)) {
> -		tg->bytes_disp[rw] += bio_size;
> -		tg->last_bytes_disp[rw] += bio_size;
> -	}
> +	tg->bytes_disp[rw] += bio_size;
> +	tg->last_bytes_disp[rw] += bio_size;

Are you sure this isn't gonna lead to double accounting? IIRC, the primary
purpose of this flag is avoiding duplicate accounting of bios which end up
going through the throttling path multiple times for whatever reason and
we've had numerous breakages around it.

To address the problem you're describing in this patch, wouldn't it make
more sense to set the flag only when the bio traversed the entire tree
rather than after each tg?

Thanks.

-- 
tejun