linux-kernel - Re: [PATCH v3] block: fix the problem of io

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <38566226-a5d3-35c3-3e06-cdf7f3f71609@gmail.com>
Date:   Tue, 6 Jul 2021 20:00:22 +0800
From:   brookxu <brookxu.cn@...il.com>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     axboe@...nel.dk, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] block: fix the problem of io_ticks becoming smaller

Christoph Hellwig wrote on 2021/7/6 13:25:
> On Tue, Jul 06, 2021 at 05:47:26AM +0800, brookxu wrote:
>> From: Chunguang Xu <brookxu@...cent.com>
>>
>> On the IO submission path, blk_account_io_start() may interrupt
>> the system interruption. When the interruption returns, the value
>> of part->stamp may have been updated by other cores, so the time
>> value collected before the interruption may be less than part->
>> stamp. So when this happens, we should do nothing to make io_ticks
>> more accurate? For kernels less than 5.0, this may cause io_ticks
>> to become smaller, which in turn may cause abnormal ioutil values.
>>
>> v3: update the commit log
>> v2: sorry, fix compile error due to the missed ')'
>>
>> Signed-off-by: Chunguang Xu <brookxu@...cent.com>
> 
> The change looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@....de>
> 
> Although I still have trouble understanding the commit log, especially
> the last sentence.

Thanks for your time，5b18b5a73760 ("block: delete part_round_stats and
switch to less precise counting") Merged in 5.0-rc1. Before that, we relied
on in_flight to count the disk's io_ticks，as follows:

static void part_round_stats_single(struct request_queue *q,
				    struct hd_struct *part, unsigned long now,
				    unsigned int inflight)
{
	if (inflight) {
		__part_stat_add(part, time_in_queue,
				inflight * (now - part->stamp));
		__part_stat_add(part, io_ticks, (now - part->stamp));
	}
	part->stamp = now;
}

The value of io_ticks should increase monotonically before the count wraps
around. However, due to the reasons mentioned above, the interrupt on the
IO submission path may cause the time obtained before the interrupt to be
less than part->stamp after the interrupt returns, resulting in the value
of io_ticks becoming smaller than the previous value. If the user task
periodically samples /proc/diskstats and calculates ioutil, since io_ticks
increases non-monotonously, the difference between adjacent times may be
negative, which in turn makes ioutil abnormal.Fortunately, this problem
can be easily circumvented.