linux-kernel - Re: [PATCH] blk-throttle: fix repeat limit on bio with BIO_BPS

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: <ea781ccc-c29e-894e-c54a-f44ea349edca@huaweicloud.com>
Date: Sat, 20 Apr 2024 09:48:20 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: zhoutaiyu <zhoutaiyu@...ishou.com>, tj@...nel.org
Cc: josef@...icpanda.com, axboe@...nel.dk, cgroups@...r.kernel.org,
 linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
 "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH] blk-throttle: fix repeat limit on bio with
 BIO_BPS_THROTTLED

Hi,

在 2024/04/19 20:07, zhoutaiyu 写道:
> Give a concrete example, a bio is throtted because of reaching bps
> limit. It is then dispatched to request layer after a delay. In the
> request layer, it is split and the split bio flagged with
> BIO_BPS_THROTTLED will re-enter blkthrottle.
> The bio with BIO_BPS_THROTTLED should not be throttled for its bytes
> again. However, when the bps_limit and iops_limit are both set and
> sq->queue is not empty, the bio will be throttled again even the tg is
> still within iops limit.

I don't understand here, split bio should be throttled by iops limit
again, this is expected. If you mean that that throtl time calculated
by iops_limit is wrong, you need to provide more informatiom.
> 
> Test scrips:
> cgpath=/sys/fs/cgroup/blkio/test0
> mkdir -p $cgpath
> echo "8:0 10485760" > $cgpath/blkio.throttle.write_bps_device
> echo "8:16 100000" > $cgpath/blkio.throttle.write_iops_device

What? 8:0 and 8:16?

> for ((i=0;i<50;i++));do
>    fio -rw=write -direct=1 -bs=4M -iodepth=8 -size=200M -numjobs=1 \
> -time_based=1 -runtime=30  -name=testt_$i -filename=testf_$i > /dev/null &
>    echo $! > $cgpath/tasks
> done
> 
> The output of iostat:
> Device:  ...  wMB/s  ...
> sdb      ...  3.75  ...
> sdb      ...  2.50  ...
> sdb      ...  3.75  ...
> sdb      ...  2.50  ...
> sdb      ...  3.75  ...
> 
> In order to fix this problem, early throttled the bio only when
> sq->queue is no empty and the bio is not flagged with BIO_BPS_THROTTLED.
> 
> Signed-off-by: zhoutaiyu <zhoutaiyu@...ishou.com>
> ---
>   block/blk-throttle.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index f4850a6..499c006 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -913,7 +913,8 @@ static bool tg_may_dispatch(struct throtl_grp *tg, struct bio *bio,
>   	 * queued.
>   	 */
>   	BUG_ON(tg->service_queue.nr_queued[rw] &&
> -	       bio != throtl_peek_queued(&tg->service_queue.queued[rw]));
> +	       bio != throtl_peek_queued(&tg->service_queue.queued[rw]) &&
> +	       !bio_flagged(bio, BIO_BPS_THROTTLED));
>   
>   	/* If tg->bps = -1, then BW is unlimited */
>   	if ((bps_limit == U64_MAX && iops_limit == UINT_MAX) ||
> @@ -2201,7 +2202,7 @@ bool __blk_throtl_bio(struct bio *bio)
>   		throtl_downgrade_check(tg);
>   		throtl_upgrade_check(tg);
>   		/* throtl is FIFO - if bios are already queued, should queue */
> -		if (sq->nr_queued[rw])
> +		if (sq->nr_queued[rw] && !bio_flagged(bio, BIO_BPS_THROTTLED))

No, this change is wrong. Split IO will not be throttled by iops limit
anymore.

Thanks,
Kuai

>   			break;
>   
>   		/* if above limits, break to queue */
>