diff mbox series

[v2,2/2] Increase default appends operations in dio_append

Message ID 20240131100018.15767-3-andrea.cervesato@suse.de
State Accepted
Headers show
Series Fix dio_append test | expand

Commit Message

Andrea Cervesato Jan. 31, 2024, 10 a.m. UTC 
From: Andrea Cervesato <andrea.cervesato@suse.com>

This test is currently ending quite fast on regular systems, so we
increase the number of operations in order to have bigger chances to
find bugs.

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 testcases/kernel/io/ltp-aiodio/dio_append.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Petr Vorel Jan. 31, 2024, 10:25 a.m. UTC | #1 
Hi Andrea,

> From: Andrea Cervesato <andrea.cervesato@suse.com>

> This test is currently ending quite fast on regular systems, so we
> increase the number of operations in order to have bigger chances to
> find bugs.

> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  testcases/kernel/io/ltp-aiodio/dio_append.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

> diff --git a/testcases/kernel/io/ltp-aiodio/dio_append.c b/testcases/kernel/io/ltp-aiodio/dio_append.c
> index 057ae73d9..0ecb76e2f 100644
> --- a/testcases/kernel/io/ltp-aiodio/dio_append.c
> +++ b/testcases/kernel/io/ltp-aiodio/dio_append.c
> @@ -33,7 +33,7 @@ static void setup(void)
>  {
>  	numchildren = 16;
>  	writesize = 64 * 1024;
> -	appends = 1000;
> +	appends = 10000;

>  	if (tst_parse_int(str_numchildren, &numchildren, 1, INT_MAX))
>  		tst_brk(TBROK, "Invalid number of children '%s'", str_numchildren);
> @@ -44,6 +44,9 @@ static void setup(void)
>  	if (tst_parse_int(str_appends, &appends, 1, INT_MAX))
>  		tst_brk(TBROK, "Invalid number of appends '%s'", str_appends);

> +	if (!tst_fs_has_free(".", appends, writesize))
> +		tst_brk(TCONF, "Not enough space to run the test");

I'm ok to keep this in single commit with increasing the number of operations,
but it should have been at least mentioned in the commit message.

And, if I understand correctly, this was meant by Cyril in v1
https://lore.kernel.org/ltp/ZbjpATp6cK9AkvBm@yuki/

Kind regards,
Petr

> +
>  	run_child = SAFE_MMAP(NULL, sizeof(int), PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0);
>  }

> @@ -97,7 +100,7 @@ static struct tst_test test = {
>  	.options = (struct tst_option[]) {
>  		{"n:", &str_numchildren, "Number of processes (default 16)"},
>  		{"w:", &str_writesize, "Write size for each append (default 64K)"},
> -		{"c:", &str_appends, "Number of appends (default 1000)"},
> +		{"c:", &str_appends, "Number of appends (default 10000)"},
>  		{}
>  	},
>  	.skip_filesystems = (const char *[]) {
Petr Vorel Jan. 31, 2024, 10:30 a.m. UTC | #2 
Hi Andrea, Cyril,

merged. Thanks!

Kind regards,
Petr
Cyril Hrubis Jan. 31, 2024, 10:30 a.m. UTC | #3 
Hi!
> I'm ok to keep this in single commit with increasing the number of operations,
> but it should have been at least mentioned in the commit message.

The commit changelog should mention the addition of the function to
check the available space, but otherwise it's ok.

> And, if I understand correctly, this was meant by Cyril in v1
> https://lore.kernel.org/ltp/ZbjpATp6cK9AkvBm@yuki/

I mostly object to having two unrelated chnages in a single commit,
functional fix shouldn't be cobbled togegher with a change that adjusts
default parameters.

Also the check for a free space should be ideally added into all I/O
tests eventually.
Petr Vorel Jan. 31, 2024, 10:33 a.m. UTC | #4 
> Hi!
> > I'm ok to keep this in single commit with increasing the number of operations,
> > but it should have been at least mentioned in the commit message.

> The commit changelog should mention the addition of the function to
> check the available space, but otherwise it's ok.

> > And, if I understand correctly, this was meant by Cyril in v1
> > https://lore.kernel.org/ltp/ZbjpATp6cK9AkvBm@yuki/

> I mostly object to having two unrelated chnages in a single commit,
> functional fix shouldn't be cobbled togegher with a change that adjusts
> default parameters.

I figured that out, that's why I merged before you wrote. I'm sorry for the
noise.

> Also the check for a free space should be ideally added into all I/O
> tests eventually.

+1

Kind regards,
Petr
diff mbox series

Patch

diff --git a/testcases/kernel/io/ltp-aiodio/dio_append.c b/testcases/kernel/io/ltp-aiodio/dio_append.c
index 057ae73d9..0ecb76e2f 100644
--- a/testcases/kernel/io/ltp-aiodio/dio_append.c
+++ b/testcases/kernel/io/ltp-aiodio/dio_append.c
@@ -33,7 +33,7 @@  static void setup(void)
 {
 	numchildren = 16;
 	writesize = 64 * 1024;
-	appends = 1000;
+	appends = 10000;
 
 	if (tst_parse_int(str_numchildren, &numchildren, 1, INT_MAX))
 		tst_brk(TBROK, "Invalid number of children '%s'", str_numchildren);
@@ -44,6 +44,9 @@  static void setup(void)
 	if (tst_parse_int(str_appends, &appends, 1, INT_MAX))
 		tst_brk(TBROK, "Invalid number of appends '%s'", str_appends);
 
+	if (!tst_fs_has_free(".", appends, writesize))
+		tst_brk(TCONF, "Not enough space to run the test");
+
 	run_child = SAFE_MMAP(NULL, sizeof(int), PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0);
 }
 
@@ -97,7 +100,7 @@  static struct tst_test test = {
 	.options = (struct tst_option[]) {
 		{"n:", &str_numchildren, "Number of processes (default 16)"},
 		{"w:", &str_writesize, "Write size for each append (default 64K)"},
-		{"c:", &str_appends, "Number of appends (default 1000)"},
+		{"c:", &str_appends, "Number of appends (default 10000)"},
 		{}
 	},
 	.skip_filesystems = (const char *[]) {