Re: [PATCH] blk-throttle: fix repeat limit on bio with BIO_BPS_THROTTLED

[Yu Kuai]

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@xxxxxxxxxxxx>
> ---
>   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 */