suggestion for fio --client to remove garbled output

suggestion for fio --client to remove garbled output

[Ben England]

This patch makes fio --client JSON output work better when many server threads are used by removing interleaved output from fio servers with --output-format=json .  

Background: Red Hat is testing large numbers of OpenStack guests with Cinder block devices backed by Ceph storage using fio.  Originally we were using ssh to start independent fio processes on the guests, but we ran into problems with threads not starting and stopping at the same time, despite using options to use fixed elapsed time for all threads.   But when we started using --client, we noticed that thread time skew is greatly reduced, because fio is *already running* on the server and we only have to connect to it and start the test.    This is great, but for large numbers of fio servers, it becomes impossible to parse the output.  

For example, here's a sample of per-VM JSON output:

Disk stats (read/write):
<vm4> {
<vm4>   "fio version" : "fio-2.2.4",
<vm4>   "timestamp" : 1421677173,
<vm4>   "time" : "Mon Jan 19 09:19:33 2015",
<vm4>   "jobs" : [
<vm4> 
<vm4>   ],
<vm4>   "disk_util" : [
...
<vm4>   ]
<vm4> }

https://s3.amazonaws.com/ben.england/client.c.diff
https://s3.amazonaws.com/ben.england/fio-results.2015-01-20-11-50.json.gz

The above output works fine for a few threads and is not hard to filter out, but the various guests' output sections get mixed together for large thread counts such as 256 client threads.  if you look at the previous URL fio-results*json.gz containing garbled output, the cause of this output is the handle_text() routine, which is called by fio whenever a FIO_NET_CMD_TEXT PDU is received by the fio client (in fio_handle_client()).  If you use --debug=all you can see that these PDUs are processed in whatever order they are received from the fio server processes, essentially in random order.  But almost all of this info is in the final json object output at the end of the file (except for the timestamp field which I wish was there!) so there is no reason to output twice, except as a debugging tool perhaps.  The patch is to V2.2.4.

[ben@localhost fio-master]$ diff -u client.{c.sav2,c}
--- client.c.sav2	2015-01-28 11:30:02.657073505 -0500
+++ client.c	2015-01-28 11:33:02.248373685 -0500
@@ -968,12 +968,6 @@
 	const char *name;
 	int fio_unused ret;
 
-	name = client->name ? client->name : client->hostname;
-
-	if (!client->skip_newline)
-		fprintf(f_out, "<%s> ", name);
-	ret = fwrite(buf, pdu->buf_len, 1, f_out);
-	fflush(f_out);
 	client->skip_newline = strchr(buf, '\n') == NULL;
 }

suggestion: patch to make per-thread IOPS more accurate

[Ben England]

The following small patch to stat.c (using fio 2.2.4 from github) outputs IOPS field in JSON format as a floating point number instead of an integer.  This improves accuracy in case where fio --client runs use lots of threads with single-digit IOPS per thread.  It seems to work, here's a snippet of output from a short fio run with rate_iops=10 in the job file.

     "write" : {
        "io_bytes" : 6464,
        "bw" : 646,
        "iops" : 10.10,
        "runtime" : 10000,

Why the patch: IOPS number is rounded to integer in stats.c calls to num2str().  This doesn't sound like much of a problem because in many use cases, with large IOPS number the error is negligible.  But in this use case where we have many threads (we would like to get into the thousands eventually), the IOPS/thread may be quite low and integer rounding can introduce significant error.  For example, if we are doing 5,000 IOPS over 1,000 threads, average throughput is 5 IOPS and potential error is ~20%, but some threads could have much higher error in IOPS because of integer format.  

background: fio's --client option could prove *extremely* useful for work where we inject I/O workload from tens, hundreds or even thousands of VMs to an OpenStack, container or other virtualization environment.  I really like this feature combined with "fio --server --daemonize=/var/run/fio-svr.pid" command run on workload generators, because it means that we don't have to use ssh/pdsh to start up a connection and launch the workload generator on each host.  ssh is problematic for this, I have to throttle it because it locks up if you have too many concurrent ssh sessions starting up.  pdsh is better but still has limits because you actually have to start each thread up from scratch, sometimes in competition with the workload you want to measure.

--- stat.c.sav	2015-01-23 16:22:14.566717417 -0500
+++ stat.c	2015-01-23 16:49:53.287962156 -0500
@@ -674,7 +674,8 @@
 		struct group_run_stats *rs, int ddir, struct json_object *parent)
 {
 	unsigned long min, max;
-	unsigned long long bw, iops;
+	unsigned long long bw;
+	double iops;
 	unsigned int *ovals = NULL;
 	double mean, dev;
 	unsigned int len, minv, maxv;
@@ -698,12 +699,12 @@
 		uint64_t runt = ts->runtime[ddir];
 
 		bw = ((1000 * ts->io_bytes[ddir]) / runt) / 1024;
-		iops = (1000 * (uint64_t) ts->total_io_u[ddir]) / runt;
+		iops = (1000.0 * (uint64_t) ts->total_io_u[ddir]) / runt;
 	}
 
 	json_object_add_value_int(dir_object, "io_bytes", ts->io_bytes[ddir] >> 10);
 	json_object_add_value_int(dir_object, "bw", bw);
-	json_object_add_value_int(dir_object, "iops", iops);
+	json_object_add_value_float(dir_object, "iops", iops);
 	json_object_add_value_int(dir_object, "runtime", ts->runtime[ddir]);
 	json_object_add_value_int(dir_object, "total_ios", ts->total_io_u[ddir]);
 	json_object_add_value_int(dir_object, "short_ios", ts->short_io_u[ddir]);

Re: suggestion: patch to make per-thread IOPS more accurate

[Mark Nelson]

Hi Ben,

I confess that I'm not using JSON output myself, but I think this would 
be really beneficial when there are a lot of clients (like VMs).

Jens, would this be an acceptable change?  I think I'd appreciate this 
if I switch over to using the JSON output.

Mark

On 02/15/2015 04:33 PM, Ben England wrote:
> The following small patch to stat.c (using fio 2.2.4 from github) outputs IOPS field in JSON format as a floating point number instead of an integer.  This improves accuracy in case where fio --client runs use lots of threads with single-digit IOPS per thread.  It seems to work, here's a snippet of output from a short fio run with rate_iops=10 in the job file.
>
>       "write" : {
>          "io_bytes" : 6464,
>          "bw" : 646,
>          "iops" : 10.10,
>          "runtime" : 10000,
>
> Why the patch: IOPS number is rounded to integer in stats.c calls to num2str().  This doesn't sound like much of a problem because in many use cases, with large IOPS number the error is negligible.  But in this use case where we have many threads (we would like to get into the thousands eventually), the IOPS/thread may be quite low and integer rounding can introduce significant error.  For example, if we are doing 5,000 IOPS over 1,000 threads, average throughput is 5 IOPS and potential error is ~20%, but some threads could have much higher error in IOPS because of integer format.
>
> background: fio's --client option could prove *extremely* useful for work where we inject I/O workload from tens, hundreds or even thousands of VMs to an OpenStack, container or other virtualization environment.  I really like this feature combined with "fio --server --daemonize=/var/run/fio-svr.pid" command run on workload generators, because it means that we don't have to use ssh/pdsh to start up a connection and launch the workload generator on each host.  ssh is problematic for this, I have to throttle it because it locks up if you have too many concurrent ssh sessions starting up.  pdsh is better but still has limits because you actually have to start each thread up from scratch, sometimes in competition with the workload you want to measure.
>
> --- stat.c.sav	2015-01-23 16:22:14.566717417 -0500
> +++ stat.c	2015-01-23 16:49:53.287962156 -0500
> @@ -674,7 +674,8 @@
>   		struct group_run_stats *rs, int ddir, struct json_object *parent)
>   {
>   	unsigned long min, max;
> -	unsigned long long bw, iops;
> +	unsigned long long bw;
> +	double iops;
>   	unsigned int *ovals = NULL;
>   	double mean, dev;
>   	unsigned int len, minv, maxv;
> @@ -698,12 +699,12 @@
>   		uint64_t runt = ts->runtime[ddir];
>
>   		bw = ((1000 * ts->io_bytes[ddir]) / runt) / 1024;
> -		iops = (1000 * (uint64_t) ts->total_io_u[ddir]) / runt;
> +		iops = (1000.0 * (uint64_t) ts->total_io_u[ddir]) / runt;
>   	}
>
>   	json_object_add_value_int(dir_object, "io_bytes", ts->io_bytes[ddir] >> 10);
>   	json_object_add_value_int(dir_object, "bw", bw);
> -	json_object_add_value_int(dir_object, "iops", iops);
> +	json_object_add_value_float(dir_object, "iops", iops);
>   	json_object_add_value_int(dir_object, "runtime", ts->runtime[ddir]);
>   	json_object_add_value_int(dir_object, "total_ios", ts->total_io_u[ddir]);
>   	json_object_add_value_int(dir_object, "short_ios", ts->short_io_u[ddir]);
> --
> To unsubscribe from this list: send the line "unsubscribe fio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: suggestion: patch to make per-thread IOPS more accurate

[Jens Axboe]

On 02/26/2015 02:21 PM, Mark Nelson wrote:
> Hi Ben,
> 
> I confess that I'm not using JSON output myself, but I think this would
> be really beneficial when there are a lot of clients (like VMs).
> 
> Jens, would this be an acceptable change?  I think I'd appreciate this
> if I switch over to using the JSON output.

I like the change, my only worry is that it'll break somebody's
parsing... So from that end, perhaps it'd be better to add a new field
instead of reusing the 'iops' field to dump it.

Maybe I'm just being overly cautious here, feel free to tell me I'm wrong.

-- 
Jens Axboe

Re: suggestion: patch to make per-thread IOPS more accurate

[Andrew Theurer]

----- Original Message -----
> From: "Jens Axboe" <axboe@kernel.dk>
> To: "Mark Nelson" <mark.a.nelson@gmail.com>, "Ben England" <bengland@redhat.com>, fio@vger.kernel.org
> Sent: Thursday, February 26, 2015 5:20:04 PM
> Subject: Re: suggestion:  patch to make per-thread IOPS more accurate
> 
> On 02/26/2015 02:21 PM, Mark Nelson wrote:
> > Hi Ben,
> > 
> > I confess that I'm not using JSON output myself, but I think this would
> > be really beneficial when there are a lot of clients (like VMs).
> > 
> > Jens, would this be an acceptable change?  I think I'd appreciate this
> > if I switch over to using the JSON output.
> 
> I like the change, my only worry is that it'll break somebody's
> parsing... So from that end, perhaps it'd be better to add a new field
> instead of reusing the 'iops' field to dump it.
> 
> Maybe I'm just being overly cautious here, feel free to tell me I'm wrong.

Since this is for the json data, I don't think the parsing would be an issue.  I have at least tried reading this data with parl::JSON with this patch, and there have been no issues.

-Andrew
> 
> --
> Jens Axboe
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: suggestion: patch to make per-thread IOPS more accurate

[Jens Axboe]

On 02/26/2015 09:14 PM, Andrew Theurer wrote:
>
>
> ----- Original Message -----
>> From: "Jens Axboe" <axboe@kernel.dk>
>> To: "Mark Nelson" <mark.a.nelson@gmail.com>, "Ben England" <bengland@redhat.com>, fio@vger.kernel.org
>> Sent: Thursday, February 26, 2015 5:20:04 PM
>> Subject: Re: suggestion:  patch to make per-thread IOPS more accurate
>>
>> On 02/26/2015 02:21 PM, Mark Nelson wrote:
>>> Hi Ben,
>>>
>>> I confess that I'm not using JSON output myself, but I think this would
>>> be really beneficial when there are a lot of clients (like VMs).
>>>
>>> Jens, would this be an acceptable change?  I think I'd appreciate this
>>> if I switch over to using the JSON output.
>>
>> I like the change, my only worry is that it'll break somebody's
>> parsing... So from that end, perhaps it'd be better to add a new field
>> instead of reusing the 'iops' field to dump it.
>>
>> Maybe I'm just being overly cautious here, feel free to tell me I'm wrong.
>
> Since this is for the json data, I don't think the parsing would be an issue.  I have at least tried reading this data with parl::JSON with this patch, and there have been no issues.

Alright, I was hoping that might be the case. I have applied the patch, 
thanks guys.

-- 
Jens Axboe

Re: suggestion: patch to make per-thread IOPS more accurate

[Andrew Theurer]

> The following small patch to stat.c (using fio 2.2.4 from github) outputs
> IOPS field in JSON format as a floating point number instead of an integer.
> This improves accuracy in case where fio --client runs use lots of threads
> with single-digit IOPS per thread.  It seems to work, here's a snippet of
> output from a short fio run with rate_iops=10 in the job file.
> 
>      "write" : {
>         "io_bytes" : 6464,
>         "bw" : 646,
>         "iops" : 10.10,
>         "runtime" : 10000,

Thanks for this.  I have applied the patch, and it works fine for me.  Very useful when testing with many fio jobs on slower storage.

> Why the patch: IOPS number is rounded to integer in stats.c calls to
> num2str().  This doesn't sound like much of a problem because in many use
> cases, with large IOPS number the error is negligible.  But in this use case
> where we have many threads (we would like to get into the thousands
> eventually), the IOPS/thread may be quite low and integer rounding can
> introduce significant error.  For example, if we are doing 5,000 IOPS over
> 1,000 threads, average throughput is 5 IOPS and potential error is ~20%, but
> some threads could have much higher error in IOPS because of integer format.
> 
> background: fio's --client option could prove *extremely* useful for work
> where we inject I/O workload from tens, hundreds or even thousands of VMs to
> an OpenStack, container or other virtualization environment.  I really like
> this feature combined with "fio --server --daemonize=/var/run/fio-svr.pid"
> command run on workload generators, because it means that we don't have to
> use ssh/pdsh to start up a connection and launch the workload generator on
> each host.  ssh is problematic for this, I have to throttle it because it
> locks up if you have too many concurrent ssh sessions starting up.  pdsh is
> better but still has limits because you actually have to start each thread
> up from scratch, sometimes in competition with the workload you want to
> measure.
> 
> --- stat.c.sav	2015-01-23 16:22:14.566717417 -0500
> +++ stat.c	2015-01-23 16:49:53.287962156 -0500
> @@ -674,7 +674,8 @@
>  		struct group_run_stats *rs, int ddir, struct json_object *parent)
>  {
>  	unsigned long min, max;
> -	unsigned long long bw, iops;
> +	unsigned long long bw;
> +	double iops;
>  	unsigned int *ovals = NULL;
>  	double mean, dev;
>  	unsigned int len, minv, maxv;
> @@ -698,12 +699,12 @@
>  		uint64_t runt = ts->runtime[ddir];
>  
>  		bw = ((1000 * ts->io_bytes[ddir]) / runt) / 1024;
> -		iops = (1000 * (uint64_t) ts->total_io_u[ddir]) / runt;
> +		iops = (1000.0 * (uint64_t) ts->total_io_u[ddir]) / runt;
>  	}
>  
>  	json_object_add_value_int(dir_object, "io_bytes", ts->io_bytes[ddir] >>
>  	10);
>  	json_object_add_value_int(dir_object, "bw", bw);
> -	json_object_add_value_int(dir_object, "iops", iops);
> +	json_object_add_value_float(dir_object, "iops", iops);
>  	json_object_add_value_int(dir_object, "runtime", ts->runtime[ddir]);
>  	json_object_add_value_int(dir_object, "total_ios", ts->total_io_u[ddir]);
>  	json_object_add_value_int(dir_object, "short_ios", ts->short_io_u[ddir]);

-Andrew