Re: [LTP] [PATCH v8 1/2] Rewrite aio-stress test using LTP API

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

2 nits below.

very nit: I'd put copyright 2022.

> +++ b/runtest/ltp-aio-stress.part1
...
> - * will open or create each file on the command line, and start a series
> - * of aio to it.
> - *
> - * aio is done in a rotating loop.  first file1 gets 8 requests, then
> - * file2, then file3 etc.  As each file finishes writing, it is switched
> - * to reads
> - *
> - * io buffers are aligned in case you want to do raw io
> - *
> - * compile with gcc -Wall -laio -lpthread -o aio-stress aio-stress.c
> + * Will open or create each file on the command line, and start a series
nit: I'm not a native English speaker but "Will" sound strange to me.
Maybe just:
"Test opens or creates ..."

I'd personally put blank line before starting multiline comment, before opening
if, below } - ending if and return before the end of function (readability). But
you can ignore these.

/* this
=> should be

/*
 * This

+ using comments in style like in include/tst_fuzzy_sync.h (@return etc) would
be nice, but again, you can ignore these comments :).

> + * of AIO to it.
>   *
> - * run aio-stress -h to see the options
> + * AIO is done in a rotating loop. First file1.bin gets 8 requests, then
> + * file2.bin, then file3.bin etc. As each file finishes writing, it is switched
> + * to reads.
>   *
> - * Please mail Chris Mason (mason@suse.com) with bug reports or patches
> + * IO buffers are aligned in case you want to do raw IO.
>   */
> +
>  #define _FILE_OFFSET_BITS 64
> -#define PROG_VERSION "0.21"
>  #define NEW_GETEVENTS
Why this? io_getevents() takes now 4 args, man does not mention when it was
changed. I'd delete this definition and #else.

...
>  /*
>   * latencies during io_submit are measured, these are the
>   * granularities for deviations
>   */
>  #define DEVIATIONS 6
> -int deviations[DEVIATIONS] = { 100, 250, 500, 1000, 5000, 10000 };
> +static int deviations[DEVIATIONS] = { 100, 250, 500, 1000, 5000, 10000 };

How about use ARRAY_SIZE()?

diff --git testcases/kernel/io/ltp-aiodio/aio-stress.c testcases/kernel/io/ltp-aiodio/aio-stress.c
index ca51b3a52..b24ca17eb 100644
--- testcases/kernel/io/ltp-aiodio/aio-stress.c
+++ testcases/kernel/io/ltp-aiodio/aio-stress.c
@@ -103,15 +103,14 @@ static char *unlink_files;
  * latencies during io_submit are measured, these are the
  * granularities for deviations
  */
-#define DEVIATIONS 6
-static int deviations[DEVIATIONS] = { 100, 250, 500, 1000, 5000, 10000 };
+static int deviations[] = { 100, 250, 500, 1000, 5000, 10000 };
 
 struct io_latency {
 	double max;
 	double min;
 	double total_io;
 	double total_lat;
-	double deviations[DEVIATIONS];
+	double deviations[ARRAY_SIZE(deviations)];
 };
 
 /* container for a series of operations to a file */
@@ -278,7 +277,7 @@ static void calc_latency(struct timeval *start_tv, struct timeval *stop_tv,
 			 struct io_latency *lat)
 {
 	double delta;
-	int i;
+	size_t i;
 
 	delta = time_since(start_tv, stop_tv);
 	delta = delta * 1000;
@@ -289,7 +288,7 @@ static void calc_latency(struct timeval *start_tv, struct timeval *stop_tv,
 		lat->min = delta;
 	lat->total_io++;
 	lat->total_lat += delta;
-	for (i = 0; i < DEVIATIONS; i++) {
+	for (i = 0; i < ARRAY_SIZE(deviations); i++) {
 		if (delta < deviations[i]) {
 			lat->deviations[i]++;
 			break;
@@ -416,12 +415,12 @@ static void print_lat(char *str, struct io_latency *lat)
 	char out[4 * 1024];
 	char *ptr = out;
 	double avg = lat->total_lat / lat->total_io;
-	int i;
+	size_t i;
 	double total_counted = 0;
 
 	tst_res(TINFO, "%s min %.2f avg %.2f max %.2f", str, lat->min, avg, lat->max);
 
-	for (i = 0; i < DEVIATIONS; i++) {
+	for (i = 0; i < ARRAY_SIZE(deviations); i++) {
 		ptr += sprintf(ptr, "%.0f < %d", lat->deviations[i], deviations[i]);
 		total_counted += lat->deviations[i];
 	}

Again, you can ignore this.

> +		{ "f:", &str_num_files, "Number of files to generate" },
nit: this is not sorted alphabetically.
> +		{ "b:", &str_max_io_submit, "Max number of iocbs to give io_submit at once" },
> +		{ "c:", &str_num_contexts, "Number of io contexts per file" },
> +		{ "g:", &str_context_offset, "Offset between contexts (default 2M)" },
> +		{ "s:", &str_file_size, "Size in MB of the test file(s) (default 1024M)" },
> +		{ "r:", &str_rec_len, "Record size in KB used for each io (default 64K)" },
> +		{ "d:", &str_depth, "Number of pending aio requests for each file (default 64)" },
> +		{ "e:", &str_io_iter, "Number of I/O per file sent before switching to the next file (default 8)" },
> +		{ "a:", &str_iterations, "Total number of ayncs I/O the program will run, default is run until Cntl-C (default 500)" },
This should be without ", default is run until Cntl-C".
OT: Cntl looks trange to me, I'd use ^C or <ctrl> + C (irrelevant now)

I also see RUN_FOREVER, do we really need it? No other LTP test runs forever.

> +		{ "O", &str_o_flag, "Use O_DIRECT" },
> +		{ "o:", &str_stages, "Add an operation to the list: write=0, read=1, random write=2, random read=3" },
> +		{ "m", &str_use_shm, "SHM use ipc shared memory for io buffers instead of malloc" },
> +		{ "n", &no_fsync_stages, "No fsyncs between write stage and read stage" },
> +		{ "l", &latency_stats, "Print io_submit latencies after each stage" },
> +		{ "L", &completion_latency_stats, "Print io completion latencies after each stage" },
Also this is not sorted.
> +		{ "t:", &str_num_threads, "Number of threads to run" },
> +		{ "u", &unlink_files, "Unlink files after completion" },
> +		{ "v", &verify, "Verification of bytes written" },
> +		{ "x", &no_stonewall, "Turn off thread stonewalling" },
> +		{},
> +	},
> +};
>  #else
> -int main(void)
> -{
> -	fprintf(stderr, "test requires libaio and it's development packages\n");
> -	return TCONF;
> -}
> +TST_TEST_TCONF("test requires libaio and its development packages");
>  #endif

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp