linux-kernel - RE: [PATCH] block: Consider inflight IO in io accounting for high latency devices

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] [day] [month] [year] [list]

Date:   Tue, 12 Sep 2023 21:01:48 +0000
From:   Gulam Mohamed <gulam.mohamed@...cle.com>
To:     Jens Axboe <axboe@...nel.dk>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] block: Consider inflight IO in io accounting for high
 latency devices

Thanks Jens for reviewing this patch. Can you please look my comments inline?

Regards,
Gulam Mohamed.
-----Original Message-----
From: Jens Axboe <axboe@...nel.dk> 
Sent: Friday, September 8, 2023 8:04 PM
To: Gulam Mohamed <gulam.mohamed@...cle.com>; linux-block@...r.kernel.org; linux-kernel@...r.kernel.org
Subject: Re: [PATCH] block: Consider inflight IO in io accounting for high latency devices

On 9/7/23 3:45 PM, Gulam Mohamed wrote:
> diff --git a/block/blk-mq.c b/block/blk-mq.c index 
> ec922c6bccbe..70e5763fb799 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1000,6 +1000,8 @@ static inline void blk_account_io_done(struct 
> request *req, u64 now)
>  
>  static inline void blk_account_io_start(struct request *req)  {
> +	bool delta = false;
> +

This is an odd name for this variable...
[GULAM]: Thanks. I will change this.

> @@ -1015,7 +1017,10 @@ static inline void blk_account_io_start(struct request *req)
>  			req->part = req->q->disk->part0;
>  
>  		part_stat_lock();
> -		update_io_ticks(req->part, jiffies, false);
> +		if (req->q->nr_hw_queues == 1) {
> +			delta = !!part_in_flight(req->part);
> +		}

No parens needed here. But that aside, I think this could be a lot better. You don't really care about the number of requests inflight, only if there are some. A better helper than part_in_flight() could do that ala:

static bool part_any_in_flight(struct block_device *part) {
	int cpu;

	for_each_possible_cpu(cpu) {
		if (part_stat_local_read_cpu(part, in_flight[0], cpu) ||
		    part_stat_local_read_cpu(part, in_flight[1], cpu))
			return true;
	}

	return false;
}
[GULAM]:  Is there a possibility that the IO request submit and completion can happen on different CPU? I am thinking that there could be positive numbers and negative numbers from different CPUs resulting in total inflight to 0. The negative number could be due to that the IO completion could happen on another CPU.

But I do wonder if it's just missed state checking for the request itself that's missing this, and this is fixing it entirely the wrong way around.
[GULAM]:  Trying to understand this comment. Can you please explore more on this?

--
Jens Axboe