[PATCH] block: fix inflight statistics of part0

[PATCH] block: fix inflight statistics of part0

[Jeffle Xu]

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(-)

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;
-- 
2.27.0

Re: [PATCH] block: fix inflight statistics of part0

[JeffleXu]

Hello, any comment? Is this indeed a BUG or just a deliberate design?

-- 
Thanks,
Jeffle


On 11/26/20 5:48 PM, 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>
> ---
>  block/blk-mq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 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;
> 

Re: [PATCH] block: fix inflight statistics of part0

[Christoph Hellwig]

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?

Re: [PATCH] block: fix inflight statistics of part0

[JeffleXu]

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?
> 

-- 
Thanks,
Jeffle

Re: [PATCH] block: fix inflight statistics of part0

[Christoph Hellwig]

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?

Re: [PATCH] block: fix inflight statistics of part0

[JeffleXu]

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.

-- 
Thanks,
Jeffle