Received-SPF: pass (google.com: domain of linux-kernel+bounces-152077-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Subject: Re: [PATCH] blk-throttle: fix repeat limit on bio with
 BIO_BPS_THROTTLED
To: zhoutaiyu <zhoutaiyu@kuaishou.com>, tj@kernel.org
Cc: josef@toxicpanda.com, axboe@kernel.dk, cgroups@vger.kernel.org,
 linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
 "yukuai (C)" <yukuai3@huawei.com>
References: <20240419120747.38031-1-zhoutaiyu@kuaishou.com>
From: Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <ea781ccc-c29e-894e-c54a-f44ea349edca@huaweicloud.com>
Date: Sat, 20 Apr 2024 09:48:20 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240419120747.38031-1-zhoutaiyu@kuaishou.com>
Content-Type: text/plain; charset=gbk; format=flowed
Content-Transfer-Encoding: 8bit

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@kuaishou.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 */
>