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

diff --git a/backend.c b/backend.c
index c035baed607a09a320e2583abd815ab1fe2ef1e0..a21dfef637bb74c2d6bf645d7f0e6a025961c072 100644 (file)
--- a/backend.c
+++ b/backend.c
@@ -136,13 +136,10 @@ static void set_sig_handlers(void)
 static bool __check_min_rate(struct thread_data *td, struct timespec *now,
                             enum fio_ddir ddir)
 {
-       unsigned long long bytes = 0;
-       unsigned long iops = 0;
-       unsigned long spent;
-       unsigned long long rate;
-       unsigned long long ratemin = 0;
-       unsigned int rate_iops = 0;
-       unsigned int rate_iops_min = 0;
+       unsigned long long current_rate_check_bytes = td->this_io_bytes[ddir];
+       unsigned long current_rate_check_blocks = td->this_io_blocks[ddir];
+       unsigned long long option_rate_bytes_min = td->o.ratemin[ddir];
+       unsigned int option_rate_iops_min = td->o.rate_iops_min[ddir];
 
        assert(ddir_rw(ddir));
 
@@ -155,68 +152,44 @@ static bool __check_min_rate(struct thread_data *td, struct timespec *now,
        if (mtime_since(&td->start, now) < 2000)
                return false;
 
-       iops += td->this_io_blocks[ddir];
-       bytes += td->this_io_bytes[ddir];
-       ratemin += td->o.ratemin[ddir];
-       rate_iops += td->o.rate_iops[ddir];
-       rate_iops_min += td->o.rate_iops_min[ddir];
-
        /*
-        * if rate blocks is set, sample is running
+        * if last_rate_check_blocks or last_rate_check_bytes is set,
+        * we can compute a rate per ratecycle
         */
-       if (td->rate_bytes[ddir] || td->rate_blocks[ddir]) {
-               spent = mtime_since(&td->lastrate[ddir], now);
-               if (spent < td->o.ratecycle)
+       if (td->last_rate_check_bytes[ddir] || td->last_rate_check_blocks[ddir]) {
+               unsigned long spent = mtime_since(&td->last_rate_check_time[ddir], now);
+               if (spent < td->o.ratecycle || spent==0)
                        return false;
 
-               if (td->o.rate[ddir] || td->o.ratemin[ddir]) {
+               if (td->o.ratemin[ddir]) {
                        /*
                         * check bandwidth specified rate
                         */
-                       if (bytes < td->rate_bytes[ddir]) {
-                               log_err("%s: rate_min=%lluB/s not met, only transferred %lluB\n",
-                                       td->o.name, ratemin, bytes);
+                       unsigned long long current_rate_bytes =
+                               ((current_rate_check_bytes - td->last_rate_check_bytes[ddir]) * 1000) / spent;
+                       if (current_rate_bytes < option_rate_bytes_min) {
+                               log_err("%s: rate_min=%lluB/s not met, got %lluB/s\n",
+                                       td->o.name, option_rate_bytes_min, current_rate_bytes);
                                return true;
-                       } else {
-                               if (spent)
-                                       rate = ((bytes - td->rate_bytes[ddir]) * 1000) / spent;
-                               else
-                                       rate = 0;
-
-                               if (rate < ratemin ||
-                                   bytes < td->rate_bytes[ddir]) {
-                                       log_err("%s: rate_min=%lluB/s not met, got %lluB/s\n",
-                                               td->o.name, ratemin, rate);
-                                       return true;
-                               }
                        }
                } else {
                        /*
                         * checks iops specified rate
                         */
-                       if (iops < rate_iops) {
-                               log_err("%s: rate_iops_min=%u not met, only performed %lu IOs\n",
-                                               td->o.name, rate_iops, iops);
+                       unsigned long long current_rate_iops =
+                               ((current_rate_check_blocks - td->last_rate_check_blocks[ddir]) * 1000) / spent;
+
+                       if (current_rate_iops < option_rate_iops_min) {
+                               log_err("%s: rate_iops_min=%u not met, got %llu IOPS\n",
+                                       td->o.name, option_rate_iops_min, current_rate_iops);
                                return true;
-                       } else {
-                               if (spent)
-                                       rate = ((iops - td->rate_blocks[ddir]) * 1000) / spent;
-                               else
-                                       rate = 0;
-
-                               if (rate < rate_iops_min ||
-                                   iops < td->rate_blocks[ddir]) {
-                                       log_err("%s: rate_iops_min=%u not met, got %llu IOPS\n",
-                                               td->o.name, rate_iops_min, rate);
-                                       return true;
-                               }
                        }
                }
        }
 
-       td->rate_bytes[ddir] = bytes;
-       td->rate_blocks[ddir] = iops;
-       memcpy(&td->lastrate[ddir], now, sizeof(*now));
+       td->last_rate_check_bytes[ddir] = current_rate_check_bytes;
+       td->last_rate_check_blocks[ddir] = current_rate_check_blocks;
+       memcpy(&td->last_rate_check_time[ddir], now, sizeof(*now));
        return false;
 }
 
@@ -1845,11 +1818,11 @@ static void *thread_main(void *data)
 
        if (o->ratemin[DDIR_READ] || o->ratemin[DDIR_WRITE] ||
                        o->ratemin[DDIR_TRIM]) {
-               memcpy(&td->lastrate[DDIR_READ], &td->bw_sample_time,
+               memcpy(&td->last_rate_check_time[DDIR_READ], &td->bw_sample_time,
                                        sizeof(td->bw_sample_time));
-               memcpy(&td->lastrate[DDIR_WRITE], &td->bw_sample_time,
+               memcpy(&td->last_rate_check_time[DDIR_WRITE], &td->bw_sample_time,
                                        sizeof(td->bw_sample_time));
-               memcpy(&td->lastrate[DDIR_TRIM], &td->bw_sample_time,
+               memcpy(&td->last_rate_check_time[DDIR_TRIM], &td->bw_sample_time,
                                        sizeof(td->bw_sample_time));
        }
 

diff --git a/fio.h b/fio.h
index 7b0ca8435978f7eb1216d287584c780941fe4049..88df117de405456d66c4687e7c9d35592531455f 100644 (file)
--- a/fio.h
+++ b/fio.h
@@ -335,10 +335,10 @@ struct thread_data {
         */
        uint64_t rate_bps[DDIR_RWDIR_CNT];
        uint64_t rate_next_io_time[DDIR_RWDIR_CNT];
-       unsigned long long rate_bytes[DDIR_RWDIR_CNT];
-       unsigned long rate_blocks[DDIR_RWDIR_CNT];
+       unsigned long long last_rate_check_bytes[DDIR_RWDIR_CNT];
+       unsigned long last_rate_check_blocks[DDIR_RWDIR_CNT];
        unsigned long long rate_io_issue_bytes[DDIR_RWDIR_CNT];
-       struct timespec lastrate[DDIR_RWDIR_CNT];
+       struct timespec last_rate_check_time[DDIR_RWDIR_CNT];
        int64_t last_usec[DDIR_RWDIR_CNT];
        struct frand_state poisson_state[DDIR_RWDIR_CNT];
 

diff --git a/libfio.c b/libfio.c
index 01fa74529fa9fedd1833e33082b73b1198bf19e1..1a8917768b86eeabe8ffefa8e0bc9675c51162c3 100644 (file)
--- a/libfio.c
+++ b/libfio.c
@@ -87,8 +87,8 @@ static void reset_io_counters(struct thread_data *td, int all)
                        td->this_io_bytes[ddir] = 0;
                        td->stat_io_blocks[ddir] = 0;
                        td->this_io_blocks[ddir] = 0;
-                       td->rate_bytes[ddir] = 0;
-                       td->rate_blocks[ddir] = 0;
+                       td->last_rate_check_bytes[ddir] = 0;
+                       td->last_rate_check_blocks[ddir] = 0;
                        td->bytes_done[ddir] = 0;
                        td->rate_io_issue_bytes[ddir] = 0;
                        td->rate_next_io_time[ddir] = 0;