Cleanup __check_min_rate
authoraggieNick02 <nick@pcpartpicker.com>
Mon, 14 Feb 2022 03:42:27 +0000 (21:42 -0600)
committeraggieNick02 <nick@pcpartpicker.com>
Mon, 14 Feb 2022 19:50:45 +0000 (13:50 -0600)
commitf5204b8af34dfda32fd6fec768c97d6c17ed3da4
tree544e42805309ade5d7a9cfac58d7d1852184fd14
parenta1db4528a59a99c5e2aa66091c505fb60e3a70ca
Cleanup __check_min_rate

This is a cleanup of __check_min_rate. In looking at stuff for previous fixes,
it seems like there are a lot of boolean checks of things that are always true
or always false. I'll explain my reasoning for each change; it is possible I'm
missing something somehow but I've run through it a few times.

Here's my logic:

1) td->rate_bytes and td->rate_blocks are 0 on first call to __check_min_rate,
and then are the previous iteration's value of td->this_io_bytes and
td->this_io_blocks on subsequent calls

2) bytes and iops are the current iteration's values of td->this_io_bytes and
td->this_io_blocks

3) The values of td->this_io_bytes and td->this_io_blocks are monotonic with
respect to each call of __check_min_rate

Therefore, bytes and iops are always greater than or equal to td->rate_bytes
and td->rate_blocks. This means the "if (bytes < td->rate_bytes[ddir]) {" on
line 176 can never happen.

Now, I want to say the same thing about line 197, but that line is weird/wrong
in another way. rate_iops is td->o.rate_iops, the specified desired iops rate
from the job. So I believe that is a bug - the specified desired iops rate
should not even be examined in this function, just like the same is true for
the desired bytes rate. I'm pretty sure what is meant is to compare iops to
td->rate_blocks just like bytes is compared to td->rate_bytes in line 176,
which would similarly always be false.

Now we can focus on the else caluses (lines 180-192 and lines 202-213). If
spent is 0, we should just be returning false early like in 169-170, so let's
move that case up with it. The "if (rate < ratemin || bytes <
td->rate_bytes[ddir]) {" and "if (rate < rate_iops_min || iops <
td->rate_blocks[ddir]) {" both have impossibilities as the second part of the
or clause. All we really want is to compare computed bytes rate to ratemin, and
computed iops rate to rate_iops_min.

With all of that, this function becomes a lot simpler. The rest of the cleanup
is renaming of variables to make what they are clearer, and some other simple
things (like initializing the variables directly instead of initializing to
zero and then doing +=). The renames are as follows:

- td->lastrate to td->last_rate_check_time, the last time a min rate check was
performed

- bytes to current_rate_check_bytes, the number of bytes transferred so far at
the time this call to __check_min_rate was made

- iops to current_rate_check_blocks, the number of blocks transferred so far at
the time this call to __check_min_rate was made

- rate to current_rate_bytes or current_rate_iops, depending on if it is used
as the current cycle's byte rate or block rate

- ratemin to option_rate_bytes_min, the user supplied desired minimum bytes
rate

- rate_iops eliminated - should not be used in this function

- rate_iops_min to option_rate_iops_min, the user supplied desired minimum
block rate

- td->rate_bytes to td->last_rate_check_bytes - the number of bytes transferred
the *last* time a minimum rate check was called *and* passed (not
shortcircuited because not enough time had elapsed for the cycle or settling)

- td->rate_blocks to td->last_rate_check_blocks - the number of blocks
transferred the *last* time a minimum rate check was called *and* passed (not
shortcircuited because not enough time had elapsed for the cycle or settling)

Signed-off-by: Nick Neumann nick@pcpartpicker.com
backend.c
fio.h
libfio.c