| Message ID | 20201126094833.61309-1-jefflexu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: fix inflight statistics of part0 | expand |
Hello, any comment? Is this indeed a BUG or just a deliberate design?
On Thu, Nov 26, 2020 at 05:48:33PM +0800, Jeffle Xu wrote: > The inflight of partition 0 doesn't include inflight IOs to all > sub-partitions, since currently mq calculates inflight of specific > partition by simply camparing the value of the partition pointer. > > Thus the following case is possible: > > $ cat /sys/block/vda/inflight > ?? ?? ?? ??0 ?? ?? ?? ??0 > $ cat /sys/block/vda/vda1/inflight > ?? ?? ?? ??0 ?? ?? ??128 > > Partition 0 should be specially handled since it represents the whole > disk. I'm not sure and can see arguments for either side. In doubt we should stick to historic behavior, can you check what old kernels (especially before blk-mq) did?
The traditional single queue block device has no problem. Following is
the output when I writes to sda3 on version v3.10.
$cat /sys/block/sda/sda3/inflight
0 33
$cat /sys/block/sda/inflight
0 33
On the other hand, we can analyze this from the code. Following code
path for single-queue block device is from v4.19.
1. When reading '/sys/block/sda/inflight', the statistics is actually
fetched from part 0.
part_inflight_show
part_in_flight_rw
inflight[0] = atomic_read(&part->in_flight[0]);
inflight[1] = atomic_read(&part->in_flight[1]);
2. part 0 will always be updated whenever sub partition is updated.
blk_queue_bio
add_acct_request
blk_account_io_start
part_inc_in_flight
atomic_inc(&part->in_flight[rw])
if (part->partno)
atomic_inc(part0.in_flight[rw]);
On 12/1/20 1:05 AM, Christoph Hellwig wrote:
> On Thu, Nov 26, 2020 at 05:48:33PM +0800, Jeffle Xu wrote:
>> The inflight of partition 0 doesn't include inflight IOs to all
>> sub-partitions, since currently mq calculates inflight of specific
>> partition by simply camparing the value of the partition pointer.
>>
>> Thus the following case is possible:
>>
>> $ cat /sys/block/vda/inflight
>> ?? ?? ?? ??0 ?? ?? ?? ??0
>> $ cat /sys/block/vda/vda1/inflight
>> ?? ?? ?? ??0 ?? ?? ??128
>>
>> Partition 0 should be specially handled since it represents the whole
>> disk.
>
> I'm not sure and can see arguments for either side. In doubt we should
> stick to historic behavior, can you check what old kernels (especially
> before blk-mq) did?
>
On Thu, Nov 26, 2020 at 05:48:33PM +0800, Jeffle Xu wrote: > The inflight of partition 0 doesn't include inflight IOs to all > sub-partitions, since currently mq calculates inflight of specific > partition by simply camparing the value of the partition pointer. > > Thus the following case is possible: > > $ cat /sys/block/vda/inflight > ?? ?? ?? ??0 ?? ?? ?? ??0 > $ cat /sys/block/vda/vda1/inflight > ?? ?? ?? ??0 ?? ?? ??128 > > Partition 0 should be specially handled since it represents the whole > disk. > > Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com> Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de> But can you resend with your information on the old kernels, and maybe even with a Fixes tag?
On 12/2/20 4:29 PM, Christoph Hellwig wrote: > On Thu, Nov 26, 2020 at 05:48:33PM +0800, Jeffle Xu wrote: >> The inflight of partition 0 doesn't include inflight IOs to all >> sub-partitions, since currently mq calculates inflight of specific >> partition by simply camparing the value of the partition pointer. >> >> Thus the following case is possible: >> >> $ cat /sys/block/vda/inflight >> ?? ?? ?? ??0 ?? ?? ?? ??0 >> $ cat /sys/block/vda/vda1/inflight >> ?? ?? ?? ??0 ?? ?? ??128 >> >> Partition 0 should be specially handled since it represents the whole >> disk. >> >> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com> > > Looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > But can you resend with your information on the old kernels, and maybe > even with a Fixes tag? > Thanks, I will send a v2 version later.
diff --git a/block/blk-mq.c b/block/blk-mq.c index 55bcee5dc032..04b6b4d21ce6 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -105,7 +105,8 @@ static bool blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx, { struct mq_inflight *mi = priv; - if (rq->part == mi->part && blk_mq_rq_state(rq) == MQ_RQ_IN_FLIGHT) + if ((!mi->part->partno || rq->part == mi->part) && + blk_mq_rq_state(rq) == MQ_RQ_IN_FLIGHT) mi->inflight[rq_data_dir(rq)]++; return true;
The inflight of partition 0 doesn't include inflight IOs to all sub-partitions, since currently mq calculates inflight of specific partition by simply camparing the value of the partition pointer. Thus the following case is possible: $ cat /sys/block/vda/inflight 0 0 $ cat /sys/block/vda/vda1/inflight 0 128 Partition 0 should be specially handled since it represents the whole disk. Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com> --- block/blk-mq.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)