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: <2b492386e60c4819ac694a65de9b5846@kuaishou.com>
Date: Mon, 22 Apr 2024 13:05:08 +0000
From: 周泰宇 <zhoutaiyu@...ishou.com>
To: Yu Kuai <yukuai1@...weicloud.com>, "tj@...nel.org" <tj@...nel.org>
CC: "josef@...icpanda.com" <josef@...icpanda.com>, "axboe@...nel.dk"
	<axboe@...nel.dk>, "cgroups@...r.kernel.org" <cgroups@...r.kernel.org>,
	"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
	"linux-kernel@...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, kuai

Thank you so much for your feedback.

> The ponit here is that you break the rules about FIFO, blk-throttle
> only judge the bio from head if it's within limit. Current code to judge
> if tg iops reaches limit on the condition that no bio is throttled. And
> throtl time is always caculated by first throttled bio. But this patch
> will ignore throttled bio case, and that's why I said IO will not be
> throttled by iops limist anymore. You can test this with bps limit
> disabled.

Sorry, I don't get it.  Yes, I know this patch will break the FIFO rule.
But I didn't find any problems after letting split bio break this rule. 
I think the split bio should be ignored if it does not
make the tg out of iops limit and be judged in tg_within_iops_limit().

In the patch, split bio within iops limit  will be dispatched directly.
However, if the split bio makes a tg out of IOPS limit, the split bio will be throttled 
and be added to the tg's sq->queue[rw] by calling throtl_add_bio_tg().

There exists two cases after the split bio is judged to be throttled.

1) The tg->sq->queue is empty. In this case, the split bio will be the first queued bio.  The tg's 
latest dispatch time is calculated according to the split bio by calling tg_update_disptime(), and 
the dispatch timer is waked up by calling throtl_schedule_next_dispatch().

2) The tg->sq->queue is not empty. In this case, the tg's dispatch time has been updated
according to the first queued bio and the dispatch timer is also waked up.
So, blk-throttle just adds the split bio to the tail of the sq->queue[rw] which acts like
the current code (split bio is throttled because sq->nr_queue[rw] is not zero).

I have tested this patch by only setting iops limit as you advised, with following scripts

#!/bin/bash

CNUM=30
mkdir -p /sys/fs/cgroup/blkio/test0
#echo "8:16 10485760" > /sys/fs/cgroup/blkio/test0/blkio.throttle.write_bps_device
echo "8:16 50" > /sys/fs/cgroup/blkio/test0/blkio.throttle.write_iops_device

for ((i=0;i<$CNUM;i++));do
        fio -rw=write -direct=1 -bs=4M -iodepth=8 -size=200M -numjobs=1 -time_based=1 -runtime=110  -name=testt_$i -filename=testf_$i  > /dev/null &
        echo $! > /sys/fs/cgroup/blkio/test0/tasks
done

I monitor the w/s on iostat and the change of /sys/fs/cgroup/blkio/test0/blkio.throttle.io_serviced every second.
It seems work perfectly (around 50).  

I also see the split bio be throttled through the trace_pipe

    kworker/19:2-1603    [019] d..1.   583.556012:   8,16   X  WS 977153216 / 977155776 [kworker/19:2]
    kworker/19:2-1603    [019] d..1.   583.556012:   8,16  1,0  m   N throtl [W] bio. bdisp=0 sz=1572864 bps=18446744073709551615 iodisp=5 iops=50 queued=0/28
    ......
    kworker/19:2-1603    [019] d..1.   583.556017:   8,16   X  WS 1870472 / 1873032 [kworker/19:2]
    kworker/19:2-1603    [019] d..1.   583.556018:   8,16  1,0  m   N throtl [W] bio. bdisp=0 sz=1572864 bps=18446744073709551615 iodisp=5 iops=50 queued=0/29

Would you mind give me more hints to find out the problems of my patch or try my patch? 

Sincerely,
Taiyu Zhou