Fix ETA calculations

PATCH In review Jens Rosenboom 2 participants 2 messages Nov 19, 2015

lore

Share & tools

lore

Jens Rosenboom

author

2015-11-19 14:53 UTC #0 lore raw

PATCH 1/1 Fix ETA calculations

There are a couple of issues in eta.c:

- The comparison for time- vs. rate-based ETA calculation is done the wrong way round.
- The elapsed ramp time should only be subtracted when the thread has indeed entered the ramp phase.
- Currently jobs that are stonewalled are not taken into account for ETA calculation. This patch at least fixes this for the case that there is only a single job per stonewall group.
---

This patch tries to fix some issues I have found, this may also be related to the ones mentioned earlier in
http://www.spinics.net/lists/fio/msg04187.html

The part regarding stonewalled jobs may require some further work, but at least it gives proper values now for my standard fio runs containing just a sequence of singular jobs.

Also, I'm still wondering about the intention behind the way done_secs is meant to work. It cumulates time spent in finished jobs globally, but is being used in per-job ETA calculations. This seems to break at least for the case of stonewalled jobs, too.

 eta.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

eta.c +18 ~~-11~~

index 7eb1c27..e73cc2c 100644
@@ -214,7 +214,7 @@ static unsigned long thread_eta(struct thread_data *td)
 		if (td->o.time_based) {
 			if (timeout) {
 				perc_t = (double) elapsed / (double) timeout;
-				if (perc_t < perc)
+				if (perc_t > perc)
 					perc = perc_t;
 			} else {
 				/*

@@ -226,6 +226,7 @@ static unsigned long thread_eta(struct thread_data *td)
 		}
 
 		eta_sec = (unsigned long) (elapsed * (1.0 / perc)) - elapsed;
+		// printf("eta_sec: %lu, elapsed: %lu, perc: %02.6f, done: %llu, total: %llu\n", eta_sec, elapsed, perc, bytes_done, bytes_total);
 
 		if (td->o.timeout &&
 		    eta_sec > (timeout + done_secs - elapsed))

@@ -250,7 +251,7 @@ static unsigned long thread_eta(struct thread_data *td)
 			t_eta = __timeout + start_delay + ramp_time;
 			t_eta /= 1000000ULL;
 
-			if (in_ramp_time(td)) {
+			if ((td->runstate == TD_RAMP) && in_ramp_time(td)) {
 				unsigned long ramp_left;
 
 				ramp_left = mtime_since_now(&td->epoch);

@@ -438,19 +439,25 @@ int calc_thread_status(struct jobs_eta *je, int force)
 		}
 	}
 
-	if (exitall_on_terminate)
+	if (exitall_on_terminate) {
 		je->eta_sec = INT_MAX;
-	else
-		je->eta_sec = 0;
-
-	for_each_td(td, i) {
-		if (exitall_on_terminate) {
+		for_each_td(td, i) {
 			if (eta_secs[i] < je->eta_sec)
 				je->eta_sec = eta_secs[i];
-		} else {
-			if (eta_secs[i] > je->eta_sec)
-				je->eta_sec = eta_secs[i];
 		}
+	} else {
+		unsigned long eta_stone = 0;
+		je->eta_sec = 0;
+		for_each_td(td, i) {
+			if ((td->runstate == TD_NOT_CREATED) && (td->o.stonewall)) {
+				eta_stone += eta_secs[i];
+			} else {
+				if (eta_secs[i] > je->eta_sec)
+					je->eta_sec = eta_secs[i];
+			}
+		}
+		je->eta_sec += eta_stone;
+		// printf("stoned: %lu\n", eta_stone);
 	}
 
 	free(eta_secs);

Signature · 1 line

2.4.10

Jens Axboe

2015-11-19 20:32 UTC #1 lore raw

Quoted 11 lines · Jens Rosenboom

There are a couple of issues in eta.c:
- The comparison for time- vs. rate-based ETA calculation is done the wrong way round.
- The elapsed ramp time should only be subtracted when the thread has indeed entered the ramp phase.
- Currently jobs that are stonewalled are not taken into account for ETA calculation. This patch at least fixes this for the case that there is only a single job per stonewall group.
---

Quoted 7 lines

This patch tries to fix some issues I have found, this may also be related to the ones mentioned earlier in
http://www.spinics.net/lists/fio/msg04187.html
The part regarding stonewalled jobs may require some further work, but at least it gives proper values now for my standard fio runs containing just a sequence of singular jobs.

Thanks this is great, I'll get this applied and tested.

Quoted 4 lines

Also, I'm still wondering about the intention behind the way done_secs is meant to work. It cumulates time spent in finished jobs globally, but is being used in per-job ETA calculations. This seems to break at least for the case of stonewalled jobs, too.

The eta output could use some love, no doubt about that. It could easily be a lot more clever wrt multiple jobs, fluctuations, etc.

Signature · 1 line

Jens Axboe