[PATCH 4/9] libnvme: add support for retrieving per-path gendisk I/O statistics

[Nilay Shroff]

On 3/24/26 2:35 PM, Daniel Wagner wrote:
> On Sat, Mar 21, 2026 at 08:58:03PM +0530, Nilay Shroff wrote:
>> +static int nvme_update_stat(const char *sysfs_stat_path, nvme_stat_t stat)
>> +{
>> +	int n;
>> +	struct timespec ts;
>> +	unsigned long rd_ios, rd_merges, wr_ios, wr_merges;
>> +	unsigned long dc_ios, dc_merges, fl_ios;
>> +	unsigned long long rd_sectors, wr_sectors, dc_sectors;
>> +	unsigned int rd_ticks, wr_ticks, dc_ticks, fl_ticks;
>> +	unsigned int io_ticks, tot_ticks, inflights;
>> +
>> +	memset(stat, 0, sizeof(struct nvme_stat));
>> +
>> +	n = sscanf(sysfs_stat_path,
>> +		"%lu %lu %llu %u %lu %lu %llu %u %u %u %u %lu %lu %llu %u %lu %u",
>> +		&rd_ios, &rd_merges, &rd_sectors, &rd_ticks,
>> +		&wr_ios, &wr_merges, &wr_sectors, &wr_ticks,
>> +		&inflights, &io_ticks, &tot_ticks,
>> +		&dc_ios, &dc_merges, &dc_sectors, &dc_ticks,
>> +		&fl_ios, &fl_ticks);
>> +
>> +	if (n < 17)
>> +		return -1;
> 
> return a proper error code, e.g. -EINVAL or whatever matches better.

Yes will return the proper error code.

> 
>> +__public int nvme_path_update_stat(nvme_path_t p, int curr)
> 
> Do we need to expose 'curr' to the user? I'd prefer not to expose this
> implementation detail to the user if possible.
> 

Here, @curr represents the bucket index. As you may have noticed,
nvme_path_t maintains a stats[2] array that stores both the previous
and current statistics.

Typically, at a regular interval (for example, when nvme-top runs with
a 1-second interval), the user/nvme-cli invokes nvme_path_update_stat().
This function reads the gendisk I/O statistics and updates one of the
entries in stats[]. The @curr parameter indicates which bucket should
be updated with the latest values.

After updating the stat, the caller/nvme-cli invokes nvme_path_get_xxx()
APIs to retrieve specific metrics. These APIs also take the current index
and compute results by comparing the current and previous statistics.

In the next interval, the caller/nvme-cli toggles the index. This effectively
swaps the roles of the two buckets—what was previously the current stats becomes
the previous stats, and vice versa. The caller then invokes nvme_path_update_stat()
again with the updated index, followed by the nvme_path_get_xxx() APIs to
retrieve the latest computed statistics.

 From this, you can see that if this logic were handled entirely within
libnvme, the library would need to maintain internal state to track which
bucket to update. Instead, I prefer leaving this responsibility to the
caller, since it already operates within the interval loop. This keeps the
library stateless and gives the caller explicit control over the bucket
selection for each interval. Does this make sense?

>> +{
>> +	_cleanup_free_ char *sysfs_stat_path = NULL;
>> +	nvme_stat_t stat;
>> +
>> +	stat = nvme_path_get_stat(p, curr);
>> +	if (!stat)
>> +		return -1;
> 
> return an error code instead -1. same comment for the rest of the patch.
> 
>> +__public unsigned int nvme_path_get_io_ticks(nvme_path_t p, int this)
>> +{
>> +	nvme_stat_t curr, prev;
>> +
>> +	curr = nvme_path_get_stat(p, this);
>> +	prev = nvme_path_get_stat(p, !this);
> 
> Do we really need to expose 'this'?
> 
As explained above @this is also a bucket index which is used to
calculate/derive the io_ticks comparing the ticks values in the
current and previous stat buckets.

Thanks,
--Nilay