diff mbox series

syscalls/fsync02: restore runtime to 5m

Message ID 19d19a5d6bbf5b19940a936b62db6dfdd29a085f.1658313770.git.jstancek@redhat.com
State Accepted, archived
Headers show
Series syscalls/fsync02: restore runtime to 5m | expand

Commit Message

Jan Stancek July 20, 2022, 10:43 a.m. UTC
Test allows up to 240 seconds for PASS result (depending if its VM or not),
but on slower systems library now kills it after a minute. Restore
runtime to 5 minutes.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/kernel/syscalls/fsync/fsync02.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Cyril Hrubis July 20, 2022, 11:56 a.m. UTC | #1
Hi!
> Test allows up to 240 seconds for PASS result (depending if its VM or not),
> but on slower systems library now kills it after a minute. Restore
> runtime to 5 minutes.

Looking at the test itself it's a bit messed up too.

The test uses rand(); to initialize the buffer size but without
initializing the seed which is not random at all. It also uses number of
available disk blocks as a upper limit, which makes the test runtime
completely unpredictable.

I guess that it would make sense to randomize the buffer sizes but in
certain bounds to make the test more predictable and print the numbers
we are going to use too. Maybe run the test with a few different sizes
and time limits. Maybe the size of the buffers can be function of the
test runtime.

All in all I think that we should really rething what we are doing here
since the current code does not make that much sense to me.

> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  testcases/kernel/syscalls/fsync/fsync02.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/syscalls/fsync/fsync02.c b/testcases/kernel/syscalls/fsync/fsync02.c
> index e13ba89f1b63..55c7a71c1d65 100644
> --- a/testcases/kernel/syscalls/fsync/fsync02.c
> +++ b/testcases/kernel/syscalls/fsync/fsync02.c
> @@ -114,5 +114,6 @@ static struct tst_test test = {
>  	.test_all = run,
>  	.setup = setup,
>  	.cleanup = cleanup,
> -	.needs_tmpdir = 1
> +	.needs_tmpdir = 1,
> +	.max_runtime = 300,
>  };
Petr Vorel Sept. 14, 2022, 8:50 p.m. UTC | #2
Hi Jan, Cyril,

> Hi!
> > Test allows up to 240 seconds for PASS result (depending if its VM or not),
> > but on slower systems library now kills it after a minute. Restore
> > runtime to 5 minutes.

> Looking at the test itself it's a bit messed up too.

> The test uses rand(); to initialize the buffer size but without
> initializing the seed which is not random at all. It also uses number of
> available disk blocks as a upper limit, which makes the test runtime
> completely unpredictable.

> I guess that it would make sense to randomize the buffer sizes but in
> certain bounds to make the test more predictable and print the numbers
> we are going to use too. Maybe run the test with a few different sizes
> and time limits. Maybe the size of the buffers can be function of the
> test runtime.

> All in all I think that we should really rething what we are doing here
> since the current code does not make that much sense to me.

Jan, do you plan to do anything with the test before the release?

Kind regards,
Petr

> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> > ---
> >  testcases/kernel/syscalls/fsync/fsync02.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)

> > diff --git a/testcases/kernel/syscalls/fsync/fsync02.c b/testcases/kernel/syscalls/fsync/fsync02.c
> > index e13ba89f1b63..55c7a71c1d65 100644
> > --- a/testcases/kernel/syscalls/fsync/fsync02.c
> > +++ b/testcases/kernel/syscalls/fsync/fsync02.c
> > @@ -114,5 +114,6 @@ static struct tst_test test = {
> >  	.test_all = run,
> >  	.setup = setup,
> >  	.cleanup = cleanup,
> > -	.needs_tmpdir = 1
> > +	.needs_tmpdir = 1,
> > +	.max_runtime = 300,
> >  };
Jan Stancek Sept. 15, 2022, 6:39 a.m. UTC | #3
On Wed, Sep 14, 2022 at 10:50 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Jan, Cyril,
>
> > Hi!
> > > Test allows up to 240 seconds for PASS result (depending if its VM or not),
> > > but on slower systems library now kills it after a minute. Restore
> > > runtime to 5 minutes.
>
> > Looking at the test itself it's a bit messed up too.
>
> > The test uses rand(); to initialize the buffer size but without
> > initializing the seed which is not random at all. It also uses number of
> > available disk blocks as a upper limit, which makes the test runtime
> > completely unpredictable.
>
> > I guess that it would make sense to randomize the buffer sizes but in
> > certain bounds to make the test more predictable and print the numbers
> > we are going to use too. Maybe run the test with a few different sizes
> > and time limits. Maybe the size of the buffers can be function of the
> > test runtime.
>
> > All in all I think that we should really rething what we are doing here
> > since the current code does not make that much sense to me.
>
> Jan, do you plan to do anything with the test before the release?

I put it at todo list, but haven't dived into it yet. For release, I'd
go with timelimit
extension as that can't cause any regressions.

>
> Kind regards,
> Petr
>
> > > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> > > ---
> > >  testcases/kernel/syscalls/fsync/fsync02.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
>
> > > diff --git a/testcases/kernel/syscalls/fsync/fsync02.c b/testcases/kernel/syscalls/fsync/fsync02.c
> > > index e13ba89f1b63..55c7a71c1d65 100644
> > > --- a/testcases/kernel/syscalls/fsync/fsync02.c
> > > +++ b/testcases/kernel/syscalls/fsync/fsync02.c
> > > @@ -114,5 +114,6 @@ static struct tst_test test = {
> > >     .test_all = run,
> > >     .setup = setup,
> > >     .cleanup = cleanup,
> > > -   .needs_tmpdir = 1
> > > +   .needs_tmpdir = 1,
> > > +   .max_runtime = 300,
> > >  };
>
Petr Vorel Sept. 15, 2022, 8:04 a.m. UTC | #4
> On Wed, Sep 14, 2022 at 10:50 PM Petr Vorel <pvorel@suse.cz> wrote:

> > Hi Jan, Cyril,

> > > Hi!
> > > > Test allows up to 240 seconds for PASS result (depending if its VM or not),
> > > > but on slower systems library now kills it after a minute. Restore
> > > > runtime to 5 minutes.

> > > Looking at the test itself it's a bit messed up too.

> > > The test uses rand(); to initialize the buffer size but without
> > > initializing the seed which is not random at all. It also uses number of
> > > available disk blocks as a upper limit, which makes the test runtime
> > > completely unpredictable.

> > > I guess that it would make sense to randomize the buffer sizes but in
> > > certain bounds to make the test more predictable and print the numbers
> > > we are going to use too. Maybe run the test with a few different sizes
> > > and time limits. Maybe the size of the buffers can be function of the
> > > test runtime.

> > > All in all I think that we should really rething what we are doing here
> > > since the current code does not make that much sense to me.

> > Jan, do you plan to do anything with the test before the release?

> I put it at todo list, but haven't dived into it yet. For release, I'd
> go with timelimit
> extension as that can't cause any regressions.

Make sense to me.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

> > Kind regards,
> > Petr

> > > > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> > > > ---
> > > >  testcases/kernel/syscalls/fsync/fsync02.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)

> > > > diff --git a/testcases/kernel/syscalls/fsync/fsync02.c b/testcases/kernel/syscalls/fsync/fsync02.c
> > > > index e13ba89f1b63..55c7a71c1d65 100644
> > > > --- a/testcases/kernel/syscalls/fsync/fsync02.c
> > > > +++ b/testcases/kernel/syscalls/fsync/fsync02.c
> > > > @@ -114,5 +114,6 @@ static struct tst_test test = {
> > > >     .test_all = run,
> > > >     .setup = setup,
> > > >     .cleanup = cleanup,
> > > > -   .needs_tmpdir = 1
> > > > +   .needs_tmpdir = 1,
> > > > +   .max_runtime = 300,
> > > >  };
Cyril Hrubis Sept. 15, 2022, 3:05 p.m. UTC | #5
Hi!
> I put it at todo list, but haven't dived into it yet. For release, I'd
> go with timelimit
> extension as that can't cause any regressions.

Let's go with that then.

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Jan Stancek Sept. 16, 2022, 9:03 a.m. UTC | #6
On Thu, Sep 15, 2022 at 5:03 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > I put it at todo list, but haven't dived into it yet. For release, I'd
> > go with timelimit
> > extension as that can't cause any regressions.
>
> Let's go with that then.
>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

Pushed.
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/fsync/fsync02.c b/testcases/kernel/syscalls/fsync/fsync02.c
index e13ba89f1b63..55c7a71c1d65 100644
--- a/testcases/kernel/syscalls/fsync/fsync02.c
+++ b/testcases/kernel/syscalls/fsync/fsync02.c
@@ -114,5 +114,6 @@  static struct tst_test test = {
 	.test_all = run,
 	.setup = setup,
 	.cleanup = cleanup,
-	.needs_tmpdir = 1
+	.needs_tmpdir = 1,
+	.max_runtime = 300,
 };