[PATCH 0/2] fio: few enhancements

[PATCH 0/2] fio: few enhancements

[Luis Chamberlain]

I've been toying around with fio's output for a bit, mostly because
Intel's fiovisualizer [0] was the first graphing tool I found for fio, even
though gfio exists. The way fiovisualizer works is it reads terse
ouptut, parses that, and plots data. gfio on the other hand is a proper
built-in client, where communication is exchanged between client/server
and the client plots the data.

If you end up using scraper for terse data such as fiovisualizer with
the prospect to scrape data you will end up with different results if
you use a remote server as a backend.

While studying fio code I realized that the way fiovisualizer implements
graphing is perhaps the wrong / least optimal approach, but also that
fio terse output needed a bit of love for client/server relationship to
keep fidelity in terms of the expected output.

These are just two small enhancements which I hope wil make things
clearer.

[0] https://github.com/intel/fiovisualizer

Luis Chamberlain (2):
  client: respect terse output on client <--> backend relationship
  init: add semantics for all types of backends running

 backend.c  |  2 ++
 client.c   | 16 +++++++++++++---
 diskutil.c |  2 +-
 fio.h      |  8 ++++++++
 init.c     |  1 +
 stat.c     |  4 +++-
 6 files changed, 28 insertions(+), 5 deletions(-)

-- 
2.18.0

[PATCH 1/2] client: respect terse output on client <--> backend relationship

[Luis Chamberlain]

You end up with different results if you run this terse output
on a local system which also runs its own backend Vs running a
client to connect to a remote server which is running fio as a
backend only. The reason is the client ops handle printing of
threads / disk utils separately. The terse output created *by*
the backend is the right and expected output, so just use that,
and we can piggy back off of the fact that the server will send
its own output via FIO_NET_CMD_TEXT.

Another solution is to address getting the disk util data sent
to be cached locally, and then upon handle_ts() print that, but that
would require significant re-architecturing.

Terse output is supposed to be just that, terse. This implies
that it will not be clear from what backend data came from, but
for this the best strategy is to *extend* the terse version with
yet another field, the remote hostname/ip address.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 client.c | 16 +++++++++++++---
 stat.c   |  2 ++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/client.c b/client.c
index e2525c81..bc0275b6 100644
--- a/client.c
+++ b/client.c
@@ -1058,6 +1058,9 @@ static void handle_ts(struct fio_client *client, struct fio_net_cmd *cmd)
 	struct flist_head *opt_list = NULL;
 	struct json_object *tsobj;
 
+	if (output_format & FIO_OUTPUT_TERSE)
+		return;
+
 	if (client->opt_lists && p->ts.thread_number <= client->jobs)
 		opt_list = &client->opt_lists[p->ts.thread_number - 1];
 
@@ -1094,6 +1097,9 @@ static void handle_gs(struct fio_client *client, struct fio_net_cmd *cmd)
 {
 	struct group_run_stats *gs = (struct group_run_stats *) cmd->payload;
 
+	if (output_format & FIO_OUTPUT_TERSE)
+		return;
+
 	if (output_format & FIO_OUTPUT_NORMAL)
 		show_group_stats(gs, NULL);
 }
@@ -1140,7 +1146,7 @@ static void handle_text(struct fio_client *client, struct fio_net_cmd *cmd)
 
 	name = client->name ? client->name : client->hostname;
 
-	if (!client->skip_newline)
+	if (!client->skip_newline && !(output_format & FIO_OUTPUT_TERSE))
 		fprintf(f_out, "<%s> ", name);
 	ret = fwrite(buf, pdu->buf_len, 1, f_out);
 	fflush(f_out);
@@ -1184,6 +1190,9 @@ static void handle_du(struct fio_client *client, struct fio_net_cmd *cmd)
 {
 	struct cmd_du_pdu *du = (struct cmd_du_pdu *) cmd->payload;
 
+	if (output_format & FIO_OUTPUT_TERSE)
+		return;
+
 	if (!client->disk_stats_shown) {
 		client->disk_stats_shown = true;
 		log_info("\nDisk stats (read/write):\n");
@@ -1195,8 +1204,6 @@ static void handle_du(struct fio_client *client, struct fio_net_cmd *cmd)
 		duobj = json_array_last_value_object(du_array);
 		json_object_add_client_info(duobj, client);
 	}
-	if (output_format & FIO_OUTPUT_TERSE)
-		print_disk_util(&du->dus, &du->agg, 1, NULL);
 	if (output_format & FIO_OUTPUT_NORMAL)
 		print_disk_util(&du->dus, &du->agg, 0, NULL);
 }
@@ -1456,6 +1463,9 @@ static void handle_probe(struct fio_client *client, struct fio_net_cmd *cmd)
 	const char *os, *arch;
 	char bit[16];
 
+	if (output_format & FIO_OUTPUT_TERSE)
+		return;
+
 	os = fio_get_os_string(probe->os);
 	if (!os)
 		os = "unknown";
diff --git a/stat.c b/stat.c
index 82e79dfc..edf9ecf6 100644
--- a/stat.c
+++ b/stat.c
@@ -1922,6 +1922,8 @@ void __show_run_stats(void)
 		if (is_backend) {
 			fio_server_send_job_options(opt_lists[i], i);
 			fio_server_send_ts(ts, rs);
+			if (output_format & FIO_OUTPUT_TERSE)
+				show_thread_status_terse(ts, rs, &output[__FIO_OUTPUT_TERSE]);
 		} else {
 			if (output_format & FIO_OUTPUT_TERSE)
 				show_thread_status_terse(ts, rs, &output[__FIO_OUTPUT_TERSE]);
-- 
2.18.0

Re: [PATCH 1/2] client: respect terse output on client <--> backend relationship

[Sitsofe Wheeler]

On Tue, 21 Aug 2018 at 20:31, Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> You end up with different results if you run this terse output
> on a local system which also runs its own backend Vs running a
> client to connect to a remote server which is running fio as a
> backend only. The reason is the client ops handle printing of
> threads / disk utils separately. The terse output created *by*
> the backend is the right and expected output, so just use that,
> and we can piggy back off of the fact that the server will send
> its own output via FIO_NET_CMD_TEXT.
>
> Another solution is to address getting the disk util data sent
> to be cached locally, and then upon handle_ts() print that, but that
> would require significant re-architecturing.
>
> Terse output is supposed to be just that, terse. This implies
> that it will not be clear from what backend data came from, but
> for this the best strategy is to *extend* the terse version with
> yet another field, the remote hostname/ip address.
>

[...]

> diff --git a/stat.c b/stat.c
> index 82e79dfc..edf9ecf6 100644
> --- a/stat.c
> +++ b/stat.c
> @@ -1922,6 +1922,8 @@ void __show_run_stats(void)
>                 if (is_backend) {
>                         fio_server_send_job_options(opt_lists[i], i);
>                         fio_server_send_ts(ts, rs);
> +                       if (output_format & FIO_OUTPUT_TERSE)
> +                               show_thread_status_terse(ts, rs, &output[__FIO_OUTPUT_TERSE]);

What stops the extra disk usage output being suppressed in the
sub-calls from show_thread_status_terse? Were all the early returns
just stop the client requesting things like disk stats periodically
and to just let the servers send it at the end?

-- 
Sitsofe | http://sucs.org/~sits/

[PATCH 2/2] init: add semantics for all types of backends running

[Luis Chamberlain]

Commit 67a176fc7 ("Fix segfault with client/server and minimal output")
addressed a segfault but it wasn't very clear *why* the fix was needed.
The reason it was needed is that the disk util and its respective
semaphore are only initialized when setup_disk_util() is called, this
happens upon fio_backend() calls. That is, either we have a dedicated
backend or have initiated a backend for the localhost due to some local
work. And show_thread_status() is currently called from a complete
client setup -- handle_ts() calls show_thread_status(), and a client
does not collect any local disk data, it receives this from the backend.
As such, the semaphore won't be setup in a client setup and this is why
we segfault here.

We can enable show_thread_status() then only if any type of backend is
running, however there is are no semantics currently which enable us to
query for this. Add such semantics and replace the previous check with
a check for if fio_backend() was ever called. This will make it clearer.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 backend.c  | 2 ++
 diskutil.c | 2 +-
 fio.h      | 8 ++++++++
 init.c     | 1 +
 stat.c     | 2 +-
 5 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/backend.c b/backend.c
index 36bde6a5..8fec1ce3 100644
--- a/backend.c
+++ b/backend.c
@@ -2481,6 +2481,8 @@ int fio_backend(struct sk_out *sk_out)
 	}
 
 	startup_sem = fio_sem_init(FIO_SEM_LOCKED);
+	if (!sk_out)
+		is_local_backend = true;
 	if (startup_sem == NULL)
 		return 1;
 
diff --git a/diskutil.c b/diskutil.c
index 5b4eb46d..7be4c022 100644
--- a/diskutil.c
+++ b/diskutil.c
@@ -701,7 +701,7 @@ void show_disk_util(int terse, struct json_object *parent,
 	struct disk_util *du;
 	bool do_json;
 
-	if (!disk_util_sem)
+	if (!is_running_backend())
 		return;
 
 	fio_sem_down(disk_util_sem);
diff --git a/fio.h b/fio.h
index b58057f7..ce3583cb 100644
--- a/fio.h
+++ b/fio.h
@@ -522,6 +522,7 @@ extern int fio_clock_source_set;
 extern int warnings_fatal;
 extern int terse_version;
 extern int is_backend;
+extern int is_local_backend;
 extern int nr_clients;
 extern int log_syslog;
 extern int status_interval;
@@ -534,6 +535,13 @@ extern char *aux_path;
 
 extern struct thread_data *threads;
 
+static inline bool is_running_backend(void)
+{
+	if (is_backend || is_local_backend)
+		return true;
+	return false;
+}
+
 extern bool eta_time_within_slack(unsigned int time);
 
 static inline void fio_ro_check(const struct thread_data *td, struct io_u *io_u)
diff --git a/init.c b/init.c
index 06f69719..3ed57570 100644
--- a/init.c
+++ b/init.c
@@ -63,6 +63,7 @@ char *exec_profile = NULL;
 int warnings_fatal = 0;
 int terse_version = 3;
 int is_backend = 0;
+int is_local_backend = 0;
 int nr_clients = 0;
 int log_syslog = 0;
 
diff --git a/stat.c b/stat.c
index edf9ecf6..6cb704eb 100644
--- a/stat.c
+++ b/stat.c
@@ -1205,7 +1205,7 @@ static void show_thread_status_terse_all(struct thread_stat *ts,
 		log_buf(out, ";%3.2f%%", io_u_lat_m[i]);
 
 	/* disk util stats, if any */
-	if (ver >= 3)
+	if (ver >= 3 && is_running_backend())
 		show_disk_util(1, NULL, out);
 
 	/* Additional output if continue_on_error set - default off*/
-- 
2.18.0