diff mbox series

block: fix inflight statistics of part0

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

Commit Message

JeffleXu Nov. 26, 2020, 9:48 a.m. UTC
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(-)

Comments

JeffleXu Nov. 30, 2020, 6:46 a.m. UTC | #1
Hello, any comment? Is this indeed a BUG or just a deliberate design?
Christoph Hellwig Nov. 30, 2020, 5:05 p.m. UTC | #2
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?
JeffleXu Dec. 1, 2020, 1:53 a.m. UTC | #3
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?
>
Christoph Hellwig Dec. 2, 2020, 8:29 a.m. UTC | #4
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?
JeffleXu Dec. 2, 2020, 8:49 a.m. UTC | #5
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 mbox series

Patch

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;