[PATCH] generic/019: kill background processes on interrupt

[PATCH] generic/019: kill background processes on interrupt

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

If you ctrl-c generic/019, it leaves fsstress processes running.
Kill them in the cleanup function so that they don't have to be
manually killed after interrupting the test.

While touching the _cleanup() function, make it do everything that
the generic _cleanup function it overrides does and fix the
indenting.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 tests/generic/019 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/generic/019 b/tests/generic/019
index db56dac1..cda107f4 100755
--- a/tests/generic/019
+++ b/tests/generic/019
@@ -53,8 +53,10 @@ stop_fail_scratch_dev()
 # Override the default cleanup function.
 _cleanup()
 {
-    disallow_fail_make_request
-    rm -f $tmp.*
+	kill $fs_pid $fio_pid &> /dev/null
+	disallow_fail_make_request
+	cd /
+	rm -r -f $tmp.*
 }
 
 RUN_TIME=$((20+10*$TIME_FACTOR))
-- 
2.35.1

Re: [PATCH] generic/019: kill background processes on interrupt

[David Disseldorp]

On Mon, 11 Apr 2022 15:48:33 +1000, Dave Chinner wrote:

> From: Dave Chinner <dchinner@redhat.com>
> 
> If you ctrl-c generic/019, it leaves fsstress processes running.
> Kill them in the cleanup function so that they don't have to be
> manually killed after interrupting the test.
> 
> While touching the _cleanup() function, make it do everything that
> the generic _cleanup function it overrides does and fix the
> indenting.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  tests/generic/019 | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/generic/019 b/tests/generic/019
> index db56dac1..cda107f4 100755
> --- a/tests/generic/019
> +++ b/tests/generic/019
> @@ -53,8 +53,10 @@ stop_fail_scratch_dev()
>  # Override the default cleanup function.
>  _cleanup()
>  {
> -    disallow_fail_make_request
> -    rm -f $tmp.*
> +	kill $fs_pid $fio_pid &> /dev/null
> +	disallow_fail_make_request
> +	cd /
> +	rm -r -f $tmp.*
>  }
>  
>  RUN_TIME=$((20+10*$TIME_FACTOR))

Might be worth unset'ing the "fs_pid" and "fio_pid" variables after the
wait, but should be fine as-is:
Reviewed-by: David Disseldorp <ddiss@suse.de>

Cheers, David

Re: [PATCH] generic/019: kill background processes on interrupt

[Zorro Lang]

On Tue, Apr 12, 2022 at 02:59:42PM +0200, David Disseldorp wrote:
> On Mon, 11 Apr 2022 15:48:33 +1000, Dave Chinner wrote:
> 
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > If you ctrl-c generic/019, it leaves fsstress processes running.
> > Kill them in the cleanup function so that they don't have to be
> > manually killed after interrupting the test.
> > 
> > While touching the _cleanup() function, make it do everything that
> > the generic _cleanup function it overrides does and fix the
> > indenting.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  tests/generic/019 | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/generic/019 b/tests/generic/019
> > index db56dac1..cda107f4 100755
> > --- a/tests/generic/019
> > +++ b/tests/generic/019
> > @@ -53,8 +53,10 @@ stop_fail_scratch_dev()
> >  # Override the default cleanup function.
> >  _cleanup()
> >  {
> > -    disallow_fail_make_request
> > -    rm -f $tmp.*
> > +	kill $fs_pid $fio_pid &> /dev/null
> > +	disallow_fail_make_request
> > +	cd /
> > +	rm -r -f $tmp.*
> >  }
> >  
> >  RUN_TIME=$((20+10*$TIME_FACTOR))
> 
> Might be worth unset'ing the "fs_pid" and "fio_pid" variables after the
> wait, but should be fine as-is:

I agree. Better to avoid killing other system processes. Or how about this place
does (avoid killing system useful processes):
$KILLALL_PROG -q $FSSTRESS_PROG
$KILLALL_PROG -q $FIO_PROG

Another picky question is, do we need to use a while loop checking, until the
processes really get killed? :)

Thanks,
Zorro

> Reviewed-by: David Disseldorp <ddiss@suse.de>
> 
> Cheers, David
> 

Re: [PATCH] generic/019: kill background processes on interrupt

[Dave Chinner]

On Tue, Apr 12, 2022 at 10:25:00PM +0800, Zorro Lang wrote:
> On Tue, Apr 12, 2022 at 02:59:42PM +0200, David Disseldorp wrote:
> > On Mon, 11 Apr 2022 15:48:33 +1000, Dave Chinner wrote:
> > 
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > If you ctrl-c generic/019, it leaves fsstress processes running.
> > > Kill them in the cleanup function so that they don't have to be
> > > manually killed after interrupting the test.
> > > 
> > > While touching the _cleanup() function, make it do everything that
> > > the generic _cleanup function it overrides does and fix the
> > > indenting.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > >  tests/generic/019 | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/generic/019 b/tests/generic/019
> > > index db56dac1..cda107f4 100755
> > > --- a/tests/generic/019
> > > +++ b/tests/generic/019
> > > @@ -53,8 +53,10 @@ stop_fail_scratch_dev()
> > >  # Override the default cleanup function.
> > >  _cleanup()
> > >  {
> > > -    disallow_fail_make_request
> > > -    rm -f $tmp.*
> > > +	kill $fs_pid $fio_pid &> /dev/null
> > > +	disallow_fail_make_request
> > > +	cd /
> > > +	rm -r -f $tmp.*
> > >  }
> > >  
> > >  RUN_TIME=$((20+10*$TIME_FACTOR))
> > 
> > Might be worth unset'ing the "fs_pid" and "fio_pid" variables after the
> > wait, but should be fine as-is:
> 
> I agree. Better to avoid killing other system processes. Or how about this place
> does (avoid killing system useful processes):
> $KILLALL_PROG -q $FSSTRESS_PROG
> $KILLALL_PROG -q $FIO_PROG
> 
> Another picky question is, do we need to use a while loop checking, until the
> processes really get killed? :)

Do we really need to paint the bikeshed over how best to kill a
process? I don't have time to do that, this is just a drive-by fix
that works for me....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] generic/019: kill background processes on interrupt

[Amir Goldstein]

On Wed, Apr 13, 2022 at 4:53 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Tue, Apr 12, 2022 at 10:25:00PM +0800, Zorro Lang wrote:
> > On Tue, Apr 12, 2022 at 02:59:42PM +0200, David Disseldorp wrote:
> > > On Mon, 11 Apr 2022 15:48:33 +1000, Dave Chinner wrote:
> > >
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > >
> > > > If you ctrl-c generic/019, it leaves fsstress processes running.
> > > > Kill them in the cleanup function so that they don't have to be
> > > > manually killed after interrupting the test.
> > > >
> > > > While touching the _cleanup() function, make it do everything that
> > > > the generic _cleanup function it overrides does and fix the
> > > > indenting.
> > > >
> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > ---
> > > >  tests/generic/019 | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/tests/generic/019 b/tests/generic/019
> > > > index db56dac1..cda107f4 100755
> > > > --- a/tests/generic/019
> > > > +++ b/tests/generic/019
> > > > @@ -53,8 +53,10 @@ stop_fail_scratch_dev()
> > > >  # Override the default cleanup function.
> > > >  _cleanup()
> > > >  {
> > > > -    disallow_fail_make_request
> > > > -    rm -f $tmp.*
> > > > + kill $fs_pid $fio_pid &> /dev/null
> > > > + disallow_fail_make_request
> > > > + cd /
> > > > + rm -r -f $tmp.*
> > > >  }
> > > >
> > > >  RUN_TIME=$((20+10*$TIME_FACTOR))
> > >
> > > Might be worth unset'ing the "fs_pid" and "fio_pid" variables after the
> > > wait, but should be fine as-is:
> >
> > I agree. Better to avoid killing other system processes. Or how about this place
> > does (avoid killing system useful processes):
> > $KILLALL_PROG -q $FSSTRESS_PROG
> > $KILLALL_PROG -q $FIO_PROG
> >
> > Another picky question is, do we need to use a while loop checking, until the
> > processes really get killed? :)
>
> Do we really need to paint the bikeshed over how best to kill a
> process? I don't have time to do that, this is just a drive-by fix
> that works for me....
>

This is not a kind response to reviewers.
Does a "drive-by fix" get exempt from the review process?
The review comments are legit even if they could be dismissed
on technical grounds, because the risk of pid wraparound is quite low.

I don't think this is about "bikeshed over how best to kill a process"
I think this is about how to have better test cleanup practices.
It would have been nice to have better isolation by having fstests
run a test without a control group and cleanup the control group
processes after the test if someone wants to take on this task.

I personally prefer the pattern of dedicated cleanup trap for aborting the test
like generic/251 that leaves the generic _cleanup on EXIT instead of
duplicating _cleanup (which generic/251 also duplicate incorrectly),
but no strong feeling about that, so as a "drive-by fix" you may add:

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks,
Amir.

Re: [PATCH] generic/019: kill background processes on interrupt

[Darrick J. Wong]

On Wed, Apr 13, 2022 at 10:13:35AM +0300, Amir Goldstein wrote:
> On Wed, Apr 13, 2022 at 4:53 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Tue, Apr 12, 2022 at 10:25:00PM +0800, Zorro Lang wrote:
> > > On Tue, Apr 12, 2022 at 02:59:42PM +0200, David Disseldorp wrote:
> > > > On Mon, 11 Apr 2022 15:48:33 +1000, Dave Chinner wrote:
> > > >
> > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > >
> > > > > If you ctrl-c generic/019, it leaves fsstress processes running.
> > > > > Kill them in the cleanup function so that they don't have to be
> > > > > manually killed after interrupting the test.
> > > > >
> > > > > While touching the _cleanup() function, make it do everything that
> > > > > the generic _cleanup function it overrides does and fix the
> > > > > indenting.
> > > > >
> > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > ---
> > > > >  tests/generic/019 | 6 ++++--
> > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/tests/generic/019 b/tests/generic/019
> > > > > index db56dac1..cda107f4 100755
> > > > > --- a/tests/generic/019
> > > > > +++ b/tests/generic/019
> > > > > @@ -53,8 +53,10 @@ stop_fail_scratch_dev()
> > > > >  # Override the default cleanup function.
> > > > >  _cleanup()
> > > > >  {
> > > > > -    disallow_fail_make_request
> > > > > -    rm -f $tmp.*
> > > > > + kill $fs_pid $fio_pid &> /dev/null
> > > > > + disallow_fail_make_request
> > > > > + cd /
> > > > > + rm -r -f $tmp.*
> > > > >  }
> > > > >
> > > > >  RUN_TIME=$((20+10*$TIME_FACTOR))
> > > >
> > > > Might be worth unset'ing the "fs_pid" and "fio_pid" variables after the
> > > > wait, but should be fine as-is:
> > >
> > > I agree. Better to avoid killing other system processes. Or how about this place
> > > does (avoid killing system useful processes):
> > > $KILLALL_PROG -q $FSSTRESS_PROG
> > > $KILLALL_PROG -q $FIO_PROG
> > >
> > > Another picky question is, do we need to use a while loop checking, until the
> > > processes really get killed? :)
> >
> > Do we really need to paint the bikeshed over how best to kill a
> > process? I don't have time to do that, this is just a drive-by fix
> > that works for me....
> >
> 
> This is not a kind response to reviewers.
> Does a "drive-by fix" get exempt from the review process?
> The review comments are legit even if they could be dismissed
> on technical grounds, because the risk of pid wraparound is quite low.
> 
> I don't think this is about "bikeshed over how best to kill a process"
> I think this is about how to have better test cleanup practices.

I agree, but this is a broad treewide cleanup, which itself is a
separate project that shouldn't hold up this quick cleanup...

> It would have been nice to have better isolation by having fstests
> run a test without a control group and cleanup the control group
> processes after the test if someone wants to take on this task.

...because there are quite a few places (particularly anything that runs
fsx/fsstress/iogen for fun) where we kick off a group of background
processes and later require a reliable way to shoot them all down.
Fixing all that in a consistent way is a *much* bigger task than what
Dave is trying to accomplish here.

The current "scheme" is that ./check will run each test in its own
systemd scope (if available) to try to improve the reliability of test
program cleanup if the _cleanup method itself fails to kill all the
child tasks.  This isn't foolproof because some people refuse to use
systemd, and the systemd tools themselves can't do a whole lot about
processes stuck in D state.

In the ideal world, whoever takes on cleaning up process cleanup
probably ought to figure out a more general solution, or at least
investigate it more thoroughly than I did to decide if it's worth
reimplementing process control group control via bash script for people
who do not use systemd.

Does anyone want to take on this task?

> I personally prefer the pattern of dedicated cleanup trap for aborting the test
> like generic/251 that leaves the generic _cleanup on EXIT instead of
> duplicating _cleanup (which generic/251 also duplicate incorrectly),
> but no strong feeling about that, so as a "drive-by fix" you may add:
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>

For this patch,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> Thanks,
> Amir.

Re: [PATCH] generic/019: kill background processes on interrupt

[Zorro Lang]

On Wed, Apr 13, 2022 at 10:26:56AM +1000, Dave Chinner wrote:
> On Tue, Apr 12, 2022 at 10:25:00PM +0800, Zorro Lang wrote:
> > On Tue, Apr 12, 2022 at 02:59:42PM +0200, David Disseldorp wrote:
> > > On Mon, 11 Apr 2022 15:48:33 +1000, Dave Chinner wrote:
> > > 
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > > 
> > > > If you ctrl-c generic/019, it leaves fsstress processes running.
> > > > Kill them in the cleanup function so that they don't have to be
> > > > manually killed after interrupting the test.
> > > > 
> > > > While touching the _cleanup() function, make it do everything that
> > > > the generic _cleanup function it overrides does and fix the
> > > > indenting.
> > > > 
> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > ---
> > > >  tests/generic/019 | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/tests/generic/019 b/tests/generic/019
> > > > index db56dac1..cda107f4 100755
> > > > --- a/tests/generic/019
> > > > +++ b/tests/generic/019
> > > > @@ -53,8 +53,10 @@ stop_fail_scratch_dev()
> > > >  # Override the default cleanup function.
> > > >  _cleanup()
> > > >  {
> > > > -    disallow_fail_make_request
> > > > -    rm -f $tmp.*
> > > > +	kill $fs_pid $fio_pid &> /dev/null
> > > > +	disallow_fail_make_request
> > > > +	cd /
> > > > +	rm -r -f $tmp.*
> > > >  }
> > > >  
> > > >  RUN_TIME=$((20+10*$TIME_FACTOR))
> > > 
> > > Might be worth unset'ing the "fs_pid" and "fio_pid" variables after the
> > > wait, but should be fine as-is:
> > 
> > I agree. Better to avoid killing other system processes. Or how about this place
> > does (avoid killing system useful processes):
> > $KILLALL_PROG -q $FSSTRESS_PROG
> > $KILLALL_PROG -q $FIO_PROG
> > 
> > Another picky question is, do we need to use a while loop checking, until the
> > processes really get killed? :)
> 
> Do we really need to paint the bikeshed over how best to kill a
> process? I don't have time to do that, this is just a drive-by fix
> that works for me....

Sure Dave:) The while loop checking is a little picky, I just ask what do you think, no
objection if you don't like that. Due to quick cleanup and exit is a good reason too.

I think it might be worth doing "$KILLALL_PROG -q $FSSTRESS_PROG $FIO_PROG" at least, to
avoid killing other processes might be useful. Or as David Disseldorp suggested "unset'ing
the "fs_pid" and "fio_pid" variables after the wait. If you're busy, I can help to make
this simple change, no push :-D

Thanks,
Zorro

> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH] generic/019: kill background processes on interrupt

[Zorro Lang]

On Fri, Apr 15, 2022 at 12:25:44AM +0800, Zorro Lang wrote:
> On Wed, Apr 13, 2022 at 10:26:56AM +1000, Dave Chinner wrote:
> > On Tue, Apr 12, 2022 at 10:25:00PM +0800, Zorro Lang wrote:
> > > On Tue, Apr 12, 2022 at 02:59:42PM +0200, David Disseldorp wrote:
> > > > On Mon, 11 Apr 2022 15:48:33 +1000, Dave Chinner wrote:
> > > > 
> > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > 
> > > > > If you ctrl-c generic/019, it leaves fsstress processes running.
> > > > > Kill them in the cleanup function so that they don't have to be
> > > > > manually killed after interrupting the test.
> > > > > 
> > > > > While touching the _cleanup() function, make it do everything that
> > > > > the generic _cleanup function it overrides does and fix the
> > > > > indenting.
> > > > > 
> > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > ---
> > > > >  tests/generic/019 | 6 ++++--
> > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/tests/generic/019 b/tests/generic/019
> > > > > index db56dac1..cda107f4 100755
> > > > > --- a/tests/generic/019
> > > > > +++ b/tests/generic/019
> > > > > @@ -53,8 +53,10 @@ stop_fail_scratch_dev()
> > > > >  # Override the default cleanup function.
> > > > >  _cleanup()
> > > > >  {
> > > > > -    disallow_fail_make_request
> > > > > -    rm -f $tmp.*
> > > > > +	kill $fs_pid $fio_pid &> /dev/null
> > > > > +	disallow_fail_make_request
> > > > > +	cd /
> > > > > +	rm -r -f $tmp.*
> > > > >  }
> > > > >  
> > > > >  RUN_TIME=$((20+10*$TIME_FACTOR))
> > > > 
> > > > Might be worth unset'ing the "fs_pid" and "fio_pid" variables after the
> > > > wait, but should be fine as-is:
> > > 
> > > I agree. Better to avoid killing other system processes. Or how about this place
> > > does (avoid killing system useful processes):
> > > $KILLALL_PROG -q $FSSTRESS_PROG
> > > $KILLALL_PROG -q $FIO_PROG
> > > 
> > > Another picky question is, do we need to use a while loop checking, until the
> > > processes really get killed? :)
> > 
> > Do we really need to paint the bikeshed over how best to kill a
> > process? I don't have time to do that, this is just a drive-by fix
> > that works for me....
> 
> Sure Dave:) The while loop checking is a little picky, I just ask what do you think, no
> objection if you don't like that. Due to quick cleanup and exit is a good reason too.
> 
> I think it might be worth doing "$KILLALL_PROG -q $FSSTRESS_PROG $FIO_PROG" at least, to
> avoid killing other processes might be useful. Or as David Disseldorp suggested "unset'ing
> the "fs_pid" and "fio_pid" variables after the wait. If you're busy, I can help to make
> this simple change, no push :-D

Anyway, we'll change this a little bit when merge it.

Reviewed-by: Zorro Lang <zlang@redhat.com>

> 
> Thanks,
> Zorro
> 
> > 
> > -Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > 

Re: [PATCH] generic/019: kill background processes on interrupt

[Amir Goldstein]

On Fri, Apr 15, 2022 at 12:56 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Fri, Apr 15, 2022 at 12:25:44AM +0800, Zorro Lang wrote:
> > On Wed, Apr 13, 2022 at 10:26:56AM +1000, Dave Chinner wrote:
> > > On Tue, Apr 12, 2022 at 10:25:00PM +0800, Zorro Lang wrote:
> > > > On Tue, Apr 12, 2022 at 02:59:42PM +0200, David Disseldorp wrote:
> > > > > On Mon, 11 Apr 2022 15:48:33 +1000, Dave Chinner wrote:
> > > > >
> > > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > >
> > > > > > If you ctrl-c generic/019, it leaves fsstress processes running.
> > > > > > Kill them in the cleanup function so that they don't have to be
> > > > > > manually killed after interrupting the test.
> > > > > >
> > > > > > While touching the _cleanup() function, make it do everything that
> > > > > > the generic _cleanup function it overrides does and fix the
> > > > > > indenting.
> > > > > >
> > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > > ---
> > > > > >  tests/generic/019 | 6 ++++--
> > > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/tests/generic/019 b/tests/generic/019
> > > > > > index db56dac1..cda107f4 100755
> > > > > > --- a/tests/generic/019
> > > > > > +++ b/tests/generic/019
> > > > > > @@ -53,8 +53,10 @@ stop_fail_scratch_dev()
> > > > > >  # Override the default cleanup function.
> > > > > >  _cleanup()
> > > > > >  {
> > > > > > -    disallow_fail_make_request
> > > > > > -    rm -f $tmp.*
> > > > > > +     kill $fs_pid $fio_pid &> /dev/null
> > > > > > +     disallow_fail_make_request
> > > > > > +     cd /
> > > > > > +     rm -r -f $tmp.*
> > > > > >  }
> > > > > >
> > > > > >  RUN_TIME=$((20+10*$TIME_FACTOR))
> > > > >
> > > > > Might be worth unset'ing the "fs_pid" and "fio_pid" variables after the
> > > > > wait, but should be fine as-is:
> > > >
> > > > I agree. Better to avoid killing other system processes. Or how about this place
> > > > does (avoid killing system useful processes):
> > > > $KILLALL_PROG -q $FSSTRESS_PROG
> > > > $KILLALL_PROG -q $FIO_PROG
> > > >
> > > > Another picky question is, do we need to use a while loop checking, until the
> > > > processes really get killed? :)
> > >
> > > Do we really need to paint the bikeshed over how best to kill a
> > > process? I don't have time to do that, this is just a drive-by fix
> > > that works for me....
> >
> > Sure Dave:) The while loop checking is a little picky, I just ask what do you think, no
> > objection if you don't like that. Due to quick cleanup and exit is a good reason too.
> >
> > I think it might be worth doing "$KILLALL_PROG -q $FSSTRESS_PROG $FIO_PROG" at least, to
> > avoid killing other processes might be useful.

That has much more likelihood to kill processes not spawn by the test
then pid wraparound.

Thanks,
Amir.

> > Or as David Disseldorp suggested "unset'ing
> > the "fs_pid" and "fio_pid" variables after the wait. If you're busy, I can help to make
> > this simple change, no push :-D
>
> Anyway, we'll change this a little bit when merge it.
>
> Reviewed-by: Zorro Lang <zlang@redhat.com>
>
> >
> > Thanks,
> > Zorro
> >
> > >
> > > -Dave.
> > > --
> > > Dave Chinner
> > > david@fromorbit.com
> > >
>

Re: [PATCH] generic/019: kill background processes on interrupt

[Zorro Lang]

On Sat, Apr 16, 2022 at 12:20:22PM +0300, Amir Goldstein wrote:
> On Fri, Apr 15, 2022 at 12:56 PM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Fri, Apr 15, 2022 at 12:25:44AM +0800, Zorro Lang wrote:
> > > On Wed, Apr 13, 2022 at 10:26:56AM +1000, Dave Chinner wrote:
> > > > On Tue, Apr 12, 2022 at 10:25:00PM +0800, Zorro Lang wrote:
> > > > > On Tue, Apr 12, 2022 at 02:59:42PM +0200, David Disseldorp wrote:
> > > > > > On Mon, 11 Apr 2022 15:48:33 +1000, Dave Chinner wrote:
> > > > > >
> > > > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > > >
> > > > > > > If you ctrl-c generic/019, it leaves fsstress processes running.
> > > > > > > Kill them in the cleanup function so that they don't have to be
> > > > > > > manually killed after interrupting the test.
> > > > > > >
> > > > > > > While touching the _cleanup() function, make it do everything that
> > > > > > > the generic _cleanup function it overrides does and fix the
> > > > > > > indenting.
> > > > > > >
> > > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > > > ---
> > > > > > >  tests/generic/019 | 6 ++++--
> > > > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/tests/generic/019 b/tests/generic/019
> > > > > > > index db56dac1..cda107f4 100755
> > > > > > > --- a/tests/generic/019
> > > > > > > +++ b/tests/generic/019
> > > > > > > @@ -53,8 +53,10 @@ stop_fail_scratch_dev()
> > > > > > >  # Override the default cleanup function.
> > > > > > >  _cleanup()
> > > > > > >  {
> > > > > > > -    disallow_fail_make_request
> > > > > > > -    rm -f $tmp.*
> > > > > > > +     kill $fs_pid $fio_pid &> /dev/null
> > > > > > > +     disallow_fail_make_request
> > > > > > > +     cd /
> > > > > > > +     rm -r -f $tmp.*
> > > > > > >  }
> > > > > > >
> > > > > > >  RUN_TIME=$((20+10*$TIME_FACTOR))
> > > > > >
> > > > > > Might be worth unset'ing the "fs_pid" and "fio_pid" variables after the
> > > > > > wait, but should be fine as-is:
> > > > >
> > > > > I agree. Better to avoid killing other system processes. Or how about this place
> > > > > does (avoid killing system useful processes):
> > > > > $KILLALL_PROG -q $FSSTRESS_PROG
> > > > > $KILLALL_PROG -q $FIO_PROG
> > > > >
> > > > > Another picky question is, do we need to use a while loop checking, until the
> > > > > processes really get killed? :)
> > > >
> > > > Do we really need to paint the bikeshed over how best to kill a
> > > > process? I don't have time to do that, this is just a drive-by fix
> > > > that works for me....
> > >
> > > Sure Dave:) The while loop checking is a little picky, I just ask what do you think, no
> > > objection if you don't like that. Due to quick cleanup and exit is a good reason too.
> > >
> > > I think it might be worth doing "$KILLALL_PROG -q $FSSTRESS_PROG $FIO_PROG" at least, to
> > > avoid killing other processes might be useful.
> 
> That has much more likelihood to kill processes not spawn by the test
> then pid wraparound.

Yes, you're right. But generally we don't run xfstests with other testing, to avoid other
testing break dmesg checking of xfstests. (Is there a way to help different process get
independent dmesg out? If there's, I'm very glad to know how to do that, then I can run
xfstests concurrently in one system:)

Anyway, we can choose another way -- "unset'ing the "fs_pid" and "fio_pid" variables
after the wait", both are fine to me.

Thanks,
Zorro

> 
> Thanks,
> Amir.
> 
> > > Or as David Disseldorp suggested "unset'ing
> > > the "fs_pid" and "fio_pid" variables after the wait. If you're busy, I can help to make
> > > this simple change, no push :-D
> >
> > Anyway, we'll change this a little bit when merge it.
> >
> > Reviewed-by: Zorro Lang <zlang@redhat.com>
> >
> > >
> > > Thanks,
> > > Zorro
> > >
> > > >
> > > > -Dave.
> > > > --
> > > > Dave Chinner
> > > > david@fromorbit.com
> > > >
> >
> 

Re: [PATCH] generic/019: kill background processes on interrupt

[Amir Goldstein]

On Sat, Apr 16, 2022 at 5:11 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Sat, Apr 16, 2022 at 12:20:22PM +0300, Amir Goldstein wrote:
> > On Fri, Apr 15, 2022 at 12:56 PM Zorro Lang <zlang@redhat.com> wrote:
> > >
> > > On Fri, Apr 15, 2022 at 12:25:44AM +0800, Zorro Lang wrote:
> > > > On Wed, Apr 13, 2022 at 10:26:56AM +1000, Dave Chinner wrote:
> > > > > On Tue, Apr 12, 2022 at 10:25:00PM +0800, Zorro Lang wrote:
> > > > > > On Tue, Apr 12, 2022 at 02:59:42PM +0200, David Disseldorp wrote:
> > > > > > > On Mon, 11 Apr 2022 15:48:33 +1000, Dave Chinner wrote:
> > > > > > >
> > > > > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > > > >
> > > > > > > > If you ctrl-c generic/019, it leaves fsstress processes running.
> > > > > > > > Kill them in the cleanup function so that they don't have to be
> > > > > > > > manually killed after interrupting the test.
> > > > > > > >
> > > > > > > > While touching the _cleanup() function, make it do everything that
> > > > > > > > the generic _cleanup function it overrides does and fix the
> > > > > > > > indenting.
> > > > > > > >
> > > > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > > > > ---
> > > > > > > >  tests/generic/019 | 6 ++++--
> > > > > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/tests/generic/019 b/tests/generic/019
> > > > > > > > index db56dac1..cda107f4 100755
> > > > > > > > --- a/tests/generic/019
> > > > > > > > +++ b/tests/generic/019
> > > > > > > > @@ -53,8 +53,10 @@ stop_fail_scratch_dev()
> > > > > > > >  # Override the default cleanup function.
> > > > > > > >  _cleanup()
> > > > > > > >  {
> > > > > > > > -    disallow_fail_make_request
> > > > > > > > -    rm -f $tmp.*
> > > > > > > > +     kill $fs_pid $fio_pid &> /dev/null
> > > > > > > > +     disallow_fail_make_request
> > > > > > > > +     cd /
> > > > > > > > +     rm -r -f $tmp.*
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  RUN_TIME=$((20+10*$TIME_FACTOR))
> > > > > > >
> > > > > > > Might be worth unset'ing the "fs_pid" and "fio_pid" variables after the
> > > > > > > wait, but should be fine as-is:
> > > > > >
> > > > > > I agree. Better to avoid killing other system processes. Or how about this place
> > > > > > does (avoid killing system useful processes):
> > > > > > $KILLALL_PROG -q $FSSTRESS_PROG
> > > > > > $KILLALL_PROG -q $FIO_PROG
> > > > > >
> > > > > > Another picky question is, do we need to use a while loop checking, until the
> > > > > > processes really get killed? :)
> > > > >
> > > > > Do we really need to paint the bikeshed over how best to kill a
> > > > > process? I don't have time to do that, this is just a drive-by fix
> > > > > that works for me....
> > > >
> > > > Sure Dave:) The while loop checking is a little picky, I just ask what do you think, no
> > > > objection if you don't like that. Due to quick cleanup and exit is a good reason too.
> > > >
> > > > I think it might be worth doing "$KILLALL_PROG -q $FSSTRESS_PROG $FIO_PROG" at least, to
> > > > avoid killing other processes might be useful.
> >
> > That has much more likelihood to kill processes not spawn by the test
> > then pid wraparound.
>
> Yes, you're right. But generally we don't run xfstests with other testing, to avoid other
> testing break dmesg checking of xfstests. (Is there a way to help different process get
> independent dmesg out? If there's, I'm very glad to know how to do that, then I can run
> xfstests concurrently in one system:)
>
> Anyway, we can choose another way -- "unset'ing the "fs_pid" and "fio_pid" variables
> after the wait", both are fine to me.
>

Fine by me

Thanks,
Amir.

Re: [PATCH] generic/019: kill background processes on interrupt

[Dave Chinner]

On Wed, Apr 13, 2022 at 10:13:35AM +0300, Amir Goldstein wrote:
> On Wed, Apr 13, 2022 at 4:53 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Tue, Apr 12, 2022 at 10:25:00PM +0800, Zorro Lang wrote:
> > > On Tue, Apr 12, 2022 at 02:59:42PM +0200, David Disseldorp wrote:
> > > > On Mon, 11 Apr 2022 15:48:33 +1000, Dave Chinner wrote:
> > > >
> > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > >
> > > > > If you ctrl-c generic/019, it leaves fsstress processes running.
> > > > > Kill them in the cleanup function so that they don't have to be
> > > > > manually killed after interrupting the test.
> > > > >
> > > > > While touching the _cleanup() function, make it do everything that
> > > > > the generic _cleanup function it overrides does and fix the
> > > > > indenting.
> > > > >
> > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > ---
> > > > >  tests/generic/019 | 6 ++++--
> > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/tests/generic/019 b/tests/generic/019
> > > > > index db56dac1..cda107f4 100755
> > > > > --- a/tests/generic/019
> > > > > +++ b/tests/generic/019
> > > > > @@ -53,8 +53,10 @@ stop_fail_scratch_dev()
> > > > >  # Override the default cleanup function.
> > > > >  _cleanup()
> > > > >  {
> > > > > -    disallow_fail_make_request
> > > > > -    rm -f $tmp.*
> > > > > + kill $fs_pid $fio_pid &> /dev/null
> > > > > + disallow_fail_make_request
> > > > > + cd /
> > > > > + rm -r -f $tmp.*
> > > > >  }
> > > > >
> > > > >  RUN_TIME=$((20+10*$TIME_FACTOR))
> > > >
> > > > Might be worth unset'ing the "fs_pid" and "fio_pid" variables after the
> > > > wait, but should be fine as-is:
> > >
> > > I agree. Better to avoid killing other system processes. Or how about this place
> > > does (avoid killing system useful processes):
> > > $KILLALL_PROG -q $FSSTRESS_PROG
> > > $KILLALL_PROG -q $FIO_PROG

In general I dislike the use of killall because because it is a
barrier to being able to run multiple tests in parallel on the same
machine. One of the things I'd really like to be able to do is run
tests in parallel and widespread use of killall is one of the
primary reasons we can't do that.

So, yeah, what one person sees as a "better method" another person
will see as having significant drawbacks and problems.

> > > Another picky question is, do we need to use a while loop checking, until the
> > > processes really get killed? :)
> >
> > Do we really need to paint the bikeshed over how best to kill a
> > process? I don't have time to do that, this is just a drive-by fix
> > that works for me....
> 
> This is not a kind response to reviewers.
> Does a "drive-by fix" get exempt from the review process?

No. But suggesting that the fix has to take into account unnecessary
and increasingly esoteric edge cases is not "review" - that's called
"moving the goal posts".

IOWs, my comment is based on the observation that a simple fix to a
problem that has been encountered during some other work gets turned
into a "fix the world" discussion that implies the original patch
author needs to fix the world rather than the single problem they
encountered and sent a patch for.

We talk about "drive by contributions" repeatedly in terms of how to
review/accept them without placing undue burdens on the people
sending such patches.  That fix might not be a 100% perfect fix but
it is still an improvement and it addresses that person's immediate
problem. That's largely all that matters to the person sending the
patch.

> The review comments are legit even if they could be dismissed
> on technical grounds, because the risk of pid wraparound is quite low.
>
> I don't think this is about "bikeshed over how best to kill a process"
> I think this is about how to have better test cleanup practices.
> It would have been nice to have better isolation by having fstests
> run a test without a control group and cleanup the control group
> processes after the test if someone wants to take on this task.

And now your are demonstrating my point - you've gone off into
bikeshedding over how to change the cleanup processes. This is
exactly what I pushed back against in the first place.

Review is not about turning a simple fix into a huge chunk of work
that the original patch author neither cares about nor has time or
resources to perform. It doesn't encourage them to send other simple
5 minute fixup patches as they encounter them, either.

Requiring patches to be perfect rather than just an *improvement* is
the problem here.  Turning a simple fix into a bikeshed session that
ends up going nowhere is in no-ones best interests.  Taking the fix
as it stands and then noting that there's a wider problem that needs
to be addressed (as Darrick has already described) is a much better
way forward. Address that bigger problem in it's own thread that
starts with patches that demonstrate solutions to that problem,
don't morph the "review" process into a discussion that implies that
the person who sent a simple fix is now on the hook to do a major
piece of cleanup work....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] generic/019: kill background processes on interrupt

[Darrick J. Wong]

On Sat, Apr 16, 2022 at 08:13:59PM +0300, Amir Goldstein wrote:
> On Sat, Apr 16, 2022 at 5:11 PM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Sat, Apr 16, 2022 at 12:20:22PM +0300, Amir Goldstein wrote:
> > > On Fri, Apr 15, 2022 at 12:56 PM Zorro Lang <zlang@redhat.com> wrote:
> > > >
> > > > On Fri, Apr 15, 2022 at 12:25:44AM +0800, Zorro Lang wrote:
> > > > > On Wed, Apr 13, 2022 at 10:26:56AM +1000, Dave Chinner wrote:
> > > > > > On Tue, Apr 12, 2022 at 10:25:00PM +0800, Zorro Lang wrote:
> > > > > > > On Tue, Apr 12, 2022 at 02:59:42PM +0200, David Disseldorp wrote:
> > > > > > > > On Mon, 11 Apr 2022 15:48:33 +1000, Dave Chinner wrote:
> > > > > > > >
> > > > > > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > > > > >
> > > > > > > > > If you ctrl-c generic/019, it leaves fsstress processes running.
> > > > > > > > > Kill them in the cleanup function so that they don't have to be
> > > > > > > > > manually killed after interrupting the test.
> > > > > > > > >
> > > > > > > > > While touching the _cleanup() function, make it do everything that
> > > > > > > > > the generic _cleanup function it overrides does and fix the
> > > > > > > > > indenting.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > > > > > ---
> > > > > > > > >  tests/generic/019 | 6 ++++--
> > > > > > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/tests/generic/019 b/tests/generic/019
> > > > > > > > > index db56dac1..cda107f4 100755
> > > > > > > > > --- a/tests/generic/019
> > > > > > > > > +++ b/tests/generic/019
> > > > > > > > > @@ -53,8 +53,10 @@ stop_fail_scratch_dev()
> > > > > > > > >  # Override the default cleanup function.
> > > > > > > > >  _cleanup()
> > > > > > > > >  {
> > > > > > > > > -    disallow_fail_make_request
> > > > > > > > > -    rm -f $tmp.*
> > > > > > > > > +     kill $fs_pid $fio_pid &> /dev/null
> > > > > > > > > +     disallow_fail_make_request
> > > > > > > > > +     cd /
> > > > > > > > > +     rm -r -f $tmp.*
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > >  RUN_TIME=$((20+10*$TIME_FACTOR))
> > > > > > > >
> > > > > > > > Might be worth unset'ing the "fs_pid" and "fio_pid" variables after the
> > > > > > > > wait, but should be fine as-is:
> > > > > > >
> > > > > > > I agree. Better to avoid killing other system processes. Or how about this place
> > > > > > > does (avoid killing system useful processes):
> > > > > > > $KILLALL_PROG -q $FSSTRESS_PROG
> > > > > > > $KILLALL_PROG -q $FIO_PROG
> > > > > > >
> > > > > > > Another picky question is, do we need to use a while loop checking, until the
> > > > > > > processes really get killed? :)
> > > > > >
> > > > > > Do we really need to paint the bikeshed over how best to kill a
> > > > > > process? I don't have time to do that, this is just a drive-by fix
> > > > > > that works for me....
> > > > >
> > > > > Sure Dave:) The while loop checking is a little picky, I just ask what do you think, no
> > > > > objection if you don't like that. Due to quick cleanup and exit is a good reason too.
> > > > >
> > > > > I think it might be worth doing "$KILLALL_PROG -q $FSSTRESS_PROG $FIO_PROG" at least, to
> > > > > avoid killing other processes might be useful.
> > >
> > > That has much more likelihood to kill processes not spawn by the test
> > > then pid wraparound.
> >
> > Yes, you're right. But generally we don't run xfstests with other testing, to avoid other
> > testing break dmesg checking of xfstests. (Is there a way to help different process get
> > independent dmesg out? If there's, I'm very glad to know how to do that, then I can run
> > xfstests concurrently in one system:)
> >
> > Anyway, we can choose another way -- "unset'ing the "fs_pid" and "fio_pid" variables
> > after the wait", both are fine to me.
> >
> 
> Fine by me

The change to clear the fs_pid/fio_pid variables was not correct and now
causes generic/019 to fail.  I'll send a patch shortly.

--D

> Thanks,
> Amir.

Re: [PATCH] generic/019: kill background processes on interrupt

[Zorro Lang]

On Mon, Apr 18, 2022 at 10:07:37AM -0700, Darrick J. Wong wrote:
> On Sat, Apr 16, 2022 at 08:13:59PM +0300, Amir Goldstein wrote:
> > On Sat, Apr 16, 2022 at 5:11 PM Zorro Lang <zlang@redhat.com> wrote:
> > >
> > > On Sat, Apr 16, 2022 at 12:20:22PM +0300, Amir Goldstein wrote:
> > > > On Fri, Apr 15, 2022 at 12:56 PM Zorro Lang <zlang@redhat.com> wrote:
> > > > >
> > > > > On Fri, Apr 15, 2022 at 12:25:44AM +0800, Zorro Lang wrote:
> > > > > > On Wed, Apr 13, 2022 at 10:26:56AM +1000, Dave Chinner wrote:
> > > > > > > On Tue, Apr 12, 2022 at 10:25:00PM +0800, Zorro Lang wrote:
> > > > > > > > On Tue, Apr 12, 2022 at 02:59:42PM +0200, David Disseldorp wrote:
> > > > > > > > > On Mon, 11 Apr 2022 15:48:33 +1000, Dave Chinner wrote:
> > > > > > > > >
> > > > > > > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > > > > > >
> > > > > > > > > > If you ctrl-c generic/019, it leaves fsstress processes running.
> > > > > > > > > > Kill them in the cleanup function so that they don't have to be
> > > > > > > > > > manually killed after interrupting the test.
> > > > > > > > > >
> > > > > > > > > > While touching the _cleanup() function, make it do everything that
> > > > > > > > > > the generic _cleanup function it overrides does and fix the
> > > > > > > > > > indenting.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > > > > > > ---
> > > > > > > > > >  tests/generic/019 | 6 ++++--
> > > > > > > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/tests/generic/019 b/tests/generic/019
> > > > > > > > > > index db56dac1..cda107f4 100755
> > > > > > > > > > --- a/tests/generic/019
> > > > > > > > > > +++ b/tests/generic/019
> > > > > > > > > > @@ -53,8 +53,10 @@ stop_fail_scratch_dev()
> > > > > > > > > >  # Override the default cleanup function.
> > > > > > > > > >  _cleanup()
> > > > > > > > > >  {
> > > > > > > > > > -    disallow_fail_make_request
> > > > > > > > > > -    rm -f $tmp.*
> > > > > > > > > > +     kill $fs_pid $fio_pid &> /dev/null
> > > > > > > > > > +     disallow_fail_make_request
> > > > > > > > > > +     cd /
> > > > > > > > > > +     rm -r -f $tmp.*
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > >  RUN_TIME=$((20+10*$TIME_FACTOR))
> > > > > > > > >
> > > > > > > > > Might be worth unset'ing the "fs_pid" and "fio_pid" variables after the
> > > > > > > > > wait, but should be fine as-is:
> > > > > > > >
> > > > > > > > I agree. Better to avoid killing other system processes. Or how about this place
> > > > > > > > does (avoid killing system useful processes):
> > > > > > > > $KILLALL_PROG -q $FSSTRESS_PROG
> > > > > > > > $KILLALL_PROG -q $FIO_PROG
> > > > > > > >
> > > > > > > > Another picky question is, do we need to use a while loop checking, until the
> > > > > > > > processes really get killed? :)
> > > > > > >
> > > > > > > Do we really need to paint the bikeshed over how best to kill a
> > > > > > > process? I don't have time to do that, this is just a drive-by fix
> > > > > > > that works for me....
> > > > > >
> > > > > > Sure Dave:) The while loop checking is a little picky, I just ask what do you think, no
> > > > > > objection if you don't like that. Due to quick cleanup and exit is a good reason too.
> > > > > >
> > > > > > I think it might be worth doing "$KILLALL_PROG -q $FSSTRESS_PROG $FIO_PROG" at least, to
> > > > > > avoid killing other processes might be useful.
> > > >
> > > > That has much more likelihood to kill processes not spawn by the test
> > > > then pid wraparound.
> > >
> > > Yes, you're right. But generally we don't run xfstests with other testing, to avoid other
> > > testing break dmesg checking of xfstests. (Is there a way to help different process get
> > > independent dmesg out? If there's, I'm very glad to know how to do that, then I can run
> > > xfstests concurrently in one system:)
> > >
> > > Anyway, we can choose another way -- "unset'ing the "fs_pid" and "fio_pid" variables
> > > after the wait", both are fine to me.
> > >
> > 
> > Fine by me
> 
> The change to clear the fs_pid/fio_pid variables was not correct and now
> causes generic/019 to fail.  I'll send a patch shortly.

Yeah, it shouldn't be:
  unset $fs_pid
  unset $fio_pid

The "$" should be removed.

> 
> --D
> 
> > Thanks,
> > Amir.
>