Record job start time to fix time pain points #1621
Conversation
|
Unrelated to the content of the PR - in testing client/server, I spun my wheels for a bit before I realized that command-line options from the client don't really seem to work - instead, everything needs to be in a job file. Any idea if this is known/expected? The docs make it look like command-line options are supported, but it doesn't seem like it. Wondering if it's a bug or if the docs need fixing. |
client.c
Outdated
| @@ -956,6 +956,7 @@ static void convert_ts(struct thread_stat *dst, struct thread_stat *src) | |||
| dst->error = le32_to_cpu(src->error); | |||
| dst->thread_number = le32_to_cpu(src->thread_number); | |||
| dst->groupid = le32_to_cpu(src->groupid); | |||
| dst->job_start = le64_to_cpu(src->job_start); | |||
There was a problem hiding this comment.
Can you line this up like the other lines?
There was a problem hiding this comment.
Sure, sorry about that.
server.c
Outdated
| @@ -1706,6 +1706,7 @@ void fio_server_send_ts(struct thread_stat *ts, struct group_run_stats *rs) | |||
| p.ts.error = cpu_to_le32(ts->error); | |||
| p.ts.thread_number = cpu_to_le32(ts->thread_number); | |||
| p.ts.groupid = cpu_to_le32(ts->groupid); | |||
| p.ts.job_start = cpu_to_le64(ts->job_start); | |||
There was a problem hiding this comment.
Please line this up like the other lines
There was a problem hiding this comment.
Sure, sorry about that.
|
The code changes all look reasonable. The only issue I see is that there is a single patch doing two things (adding the new key and making two options synonyms of each other). Could you separate these into two distinct patches? |
This sounds like something we should fix, although it's not clear whether we should change the documentation to match fio's behavior or vice versa. |
aggieNick02
force-pushed
the
pcpp_epoch_fixing_2
branch
from
3de59e2 to
66134c2
Compare
Sure, I can definitely do this if you want. The one wrinkle in doing so is that there will be conflicts as the changes touch some of the same lines. Do you want me to first do the new key and then wait until that PR is merged and then do the synonyms? |
I'll take a quick look and see if I can see what might be going wrong. |
I would just have this one PR include both patches. Either order is fine. |
Add a new key in the json per-job output, job_start, that records the job start time obtained via a call to clock_gettime using the clock_id specified by the new job_start_clock_id option. This allows times of fio jobs and log entries to be compared/ordered against each other and against other system events recorded against the same clock_id. Add a note to the documentation for group_reporting about how there are several per-job values for which only the first job's value is recorded in the json output format when group_reporting is enabled. Fixes axboe#1544 Signed-off-by: Nick Neumann [email protected]
log_alternate_epoch was introduced along with log_alternate_epoch_clock_id, and generalized the idea of log_unix_epoch. Both options had the same effect. So we make log_unix_epoch an official alias of log_alternate_epoch, instead of maintaining both redundant options. Signed-off-by: Nick Neumann [email protected]
aggieNick02
force-pushed
the
pcpp_epoch_fixing_2
branch
from
66134c2 to
d252275
Compare
Ah, ok. I just updated the PR with two separate commits. Thanks! |
Add a new key in the json per-job output, job_start, that records the job start time obtained via a call to clock_gettime using the clock_id specified by the new job_start_clock_id option. This allows times of fio jobs and log entries to be compared/ordered against each other and against other system events recorded against the same clock_id.
Also make log_unix_epoch an official alias of log_alternate_epoch, instead of maintaining both redundant options.
Add a note to the documentation for group_reporting about how there are several per-job values for which only the first job's value is recorded in the json output format when group_reporting is enabled.
Fixes #1544
Signed-off-by: Nick Neumann [email protected]