Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3] block: fix the problem of io_ticks becoming smaller
To:     Christoph Hellwig <hch@infradead.org>
Cc:     axboe@kernel.dk, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <1625521646-1069-1-git-send-email-brookxu.cn@gmail.com>
 <YOPpM6et9rFNrBOn@infradead.org>
From:   brookxu <brookxu.cn@gmail.com>
Message-ID: <38566226-a5d3-35c3-3e06-cdf7f3f71609@gmail.com>
Date:   Tue, 6 Jul 2021 20:00:22 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <YOPpM6et9rFNrBOn@infradead.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


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@tencent.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@tencent.com>
> 
> The change looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.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.