Skip to content

Commit

Permalink
Handle 32-bit overflows in disk utilization stats
Browse files Browse the repository at this point in the history
Linux prints [1] some of the values reported in block device's `stat`
sysfs file as 32-bit unsigned integers. fio interprets them as 64-bit
integers when reading that sysfs file and performs further arithmetics
on them in 64-bits. If the reported value overflows during fio run,
a huge bogus value is reported in the "disk utilization" block instead.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/block/genhd.c#n962

Signed-off-by: Alexey Neyman <aneyman@google.com>
  • Loading branch information
stilor committed Oct 4, 2023
1 parent 6f9cdcf commit 5e3b600
Showing 1 changed file with 36 additions and 5 deletions.
41 changes: 36 additions & 5 deletions diskutil.c
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,24 @@ static int get_io_ticks(struct disk_util *du, struct disk_util_stat *dus)
return ret != 10;
}

static void update_io_tick_disk(struct disk_util *du)
static uint64_t safe_32bit_diff(uint64_t nval, uint64_t oval)
{
/* Linux kernel prints some of the stat fields as 32-bit integers. It is
* possible that the value overflows, but since fio uses unsigned 64-bit
* arithmetic in update_io_tick_disk(), it instead results in a huge
* bogus value being added to the respective accumulating field. Just
* in case Linux starts reporting these metrics as 64-bit values in the
* future, check that overflow actually happens around the 32-bit
* unsigned boundary; assume overflow only happens once between
* successive polls.
*/
if (oval <= nval || oval >= (1ull << 32))
return nval - oval;
else
return (1ull << 32) + nval - oval;
}

static void __attribute__((noinline)) update_io_tick_disk(struct disk_util *du)
{
struct disk_util_stat __dus, *dus, *ldus;
struct timespec t;
Expand All @@ -96,14 +113,21 @@ static void update_io_tick_disk(struct disk_util *du)
dus->s.ios[1] += (__dus.s.ios[1] - ldus->s.ios[1]);
dus->s.merges[0] += (__dus.s.merges[0] - ldus->s.merges[0]);
dus->s.merges[1] += (__dus.s.merges[1] - ldus->s.merges[1]);
dus->s.ticks[0] += (__dus.s.ticks[0] - ldus->s.ticks[0]);
dus->s.ticks[1] += (__dus.s.ticks[1] - ldus->s.ticks[1]);
dus->s.io_ticks += (__dus.s.io_ticks - ldus->s.io_ticks);
dus->s.time_in_queue += (__dus.s.time_in_queue - ldus->s.time_in_queue);
dus->s.ticks[0] += safe_32bit_diff(__dus.s.ticks[0], ldus->s.ticks[0]);
dus->s.ticks[1] += safe_32bit_diff(__dus.s.ticks[1], ldus->s.ticks[1]);
dus->s.io_ticks += safe_32bit_diff(__dus.s.io_ticks, ldus->s.io_ticks);
dus->s.time_in_queue +=
safe_32bit_diff(__dus.s.time_in_queue, ldus->s.time_in_queue);

fio_gettime(&t, NULL);
dus->s.msec += mtime_since(&du->time, &t);
memcpy(&du->time, &t, sizeof(t));

// memcpy() below triggers a spurious warning at -O2 or above, starting with
// GCC11.
#if defined(__GNUC__) && __GNUC__ >= 11
#pragma GCC diagnostic ignored "-Wstringop-overflow"
#endif
memcpy(&ldus->s, &__dus.s, sizeof(__dus.s));
}

Expand All @@ -121,6 +145,13 @@ int update_io_ticks(void)
flist_for_each(entry, &disk_list) {
du = flist_entry(entry, struct disk_util, list);
update_io_tick_disk(du);

// Re-enable the warning turned off in update_io_tick_disk().
// Warning suppression has to be in effect up to the location where the
// function is inlined.
#if defined(__GNUC__) && __GNUC__ >= 11
#pragma GCC diagnostic warning "-Wstringop-overflow"
#endif
}
} else
ret = 1;
Expand Down

0 comments on commit 5e3b600

Please sign in to comment.