Handle 32-bit overflows in disk utilization stats #1641
Conversation
stilor
force-pushed
the
fix-stat-overflow
branch
from
73879d0
to
f15a6ea
Compare
diskutil.c
Outdated
| { | ||
| /* 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 | ||
| * arithmetics in update_io_tick_disk(), it instead results in a huge |
There was a problem hiding this comment.
Suggest changing from arithmetics to arithmetic, sorry not grammar police. :)
diskutil.c
Outdated
| * unsigned boundary; assume overflow only happens once between | ||
| * successive polls. | ||
| */ | ||
| if (oval <= nval || oval >= (1ull << 32)) { |
There was a problem hiding this comment.
No need for curly braces for this if else condition
|
Please change the link in the commit message to one that doesn't require login credentials. |
This seems to be a GCC bug. I minimized the testcase to https://godbolt.org/z/nMEGzfW7Y and it still produces this error. Reported https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111696 to GCC maintainers with a minimal testcase derived from diskutil.c. For now, disabled the warning using GCC diagnostic pragmas around the code triggering the spurious warning. Alternatively, if you prefer I could add a test for affected compilers in configure and if the compiler fails the test, add -Wno-stringop-overflow to prevent such spurious warnings in all files. |
stilor
force-pushed
the
fix-stat-overflow
branch
3 times, most recently
from
d43fbc8
to
36c1447
Compare
diskutil.c
Outdated
|
|
||
| fio_gettime(&t, NULL); | ||
| dus->s.msec += mtime_since(&du->time, &t); | ||
| memcpy(&du->time, &t, sizeof(t)); | ||
|
|
There was a problem hiding this comment.
Please remove this #pragma and change the memcpy() calls in this function into assignments. There is no reason to use memcpy() here.
stilor
force-pushed
the
fix-stat-overflow
branch
from
36c1447
to
627ad15
Compare
|
|
||
| fio_gettime(&t, NULL); | ||
| dus->s.msec += mtime_since(&du->time, &t); | ||
| memcpy(&du->time, &t, sizeof(t)); |
There was a problem hiding this comment.
Since this change is unrelated to the 32-bit overflow fix, please consider moving this change into a separate patch.
This is to avoid triggering a spurious warning caused by [1], which is triggered by the next commit in chain (unrelated change in update_io_tick_disk()). [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111696 Signed-off-by: Alexey Neyman <aneyman@google.com>
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>
stilor
force-pushed
the
fix-stat-overflow
branch
from
627ad15
to
5f7bd35
Compare
Linux prints [1] some of the values reported in block device's
statsysfs 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