* [PATCH] generic/019: kill background processes on interrupt @ 2022-04-11 5:48 Dave Chinner 2022-04-12 12:59 ` David Disseldorp 0 siblings, 1 reply; 14+ messages in thread From: Dave Chinner @ 2022-04-11 5:48 UTC (permalink / raw) To: fstests 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/019: kill background processes on interrupt 2022-04-11 5:48 [PATCH] generic/019: kill background processes on interrupt Dave Chinner @ 2022-04-12 12:59 ` David Disseldorp 2022-04-12 14:25 ` Zorro Lang 0 siblings, 1 reply; 14+ messages in thread From: David Disseldorp @ 2022-04-12 12:59 UTC (permalink / raw) To: Dave Chinner; +Cc: fstests 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/019: kill background processes on interrupt 2022-04-12 12:59 ` David Disseldorp @ 2022-04-12 14:25 ` Zorro Lang 2022-04-13 0:26 ` Dave Chinner 0 siblings, 1 reply; 14+ messages in thread From: Zorro Lang @ 2022-04-12 14:25 UTC (permalink / raw) To: Dave Chinner; +Cc: David Disseldorp, fstests 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/019: kill background processes on interrupt 2022-04-12 14:25 ` Zorro Lang @ 2022-04-13 0:26 ` Dave Chinner 2022-04-13 7:13 ` Amir Goldstein 2022-04-14 16:25 ` Zorro Lang 0 siblings, 2 replies; 14+ messages in thread From: Dave Chinner @ 2022-04-13 0:26 UTC (permalink / raw) To: David Disseldorp, fstests 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/019: kill background processes on interrupt 2022-04-13 0:26 ` Dave Chinner @ 2022-04-13 7:13 ` Amir Goldstein 2022-04-13 23:27 ` Darrick J. Wong 2022-04-18 0:02 ` Dave Chinner 2022-04-14 16:25 ` Zorro Lang 1 sibling, 2 replies; 14+ messages in thread From: Amir Goldstein @ 2022-04-13 7:13 UTC (permalink / raw) To: Dave Chinner; +Cc: David Disseldorp, fstests 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/019: kill background processes on interrupt 2022-04-13 7:13 ` Amir Goldstein @ 2022-04-13 23:27 ` Darrick J. Wong 2022-04-18 0:02 ` Dave Chinner 1 sibling, 0 replies; 14+ messages in thread From: Darrick J. Wong @ 2022-04-13 23:27 UTC (permalink / raw) To: Amir Goldstein; +Cc: Dave Chinner, David Disseldorp, fstests 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/019: kill background processes on interrupt 2022-04-13 7:13 ` Amir Goldstein 2022-04-13 23:27 ` Darrick J. Wong @ 2022-04-18 0:02 ` Dave Chinner 1 sibling, 0 replies; 14+ messages in thread From: Dave Chinner @ 2022-04-18 0:02 UTC (permalink / raw) To: Amir Goldstein; +Cc: David Disseldorp, fstests 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/019: kill background processes on interrupt 2022-04-13 0:26 ` Dave Chinner 2022-04-13 7:13 ` Amir Goldstein @ 2022-04-14 16:25 ` Zorro Lang 2022-04-14 19:16 ` Zorro Lang 1 sibling, 1 reply; 14+ messages in thread From: Zorro Lang @ 2022-04-14 16:25 UTC (permalink / raw) To: Dave Chinner; +Cc: David Disseldorp, fstests 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/019: kill background processes on interrupt 2022-04-14 16:25 ` Zorro Lang @ 2022-04-14 19:16 ` Zorro Lang 2022-04-16 9:20 ` Amir Goldstein 0 siblings, 1 reply; 14+ messages in thread From: Zorro Lang @ 2022-04-14 19:16 UTC (permalink / raw) To: Dave Chinner, David Disseldorp, fstests 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 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/019: kill background processes on interrupt 2022-04-14 19:16 ` Zorro Lang @ 2022-04-16 9:20 ` Amir Goldstein 2022-04-16 14:11 ` Zorro Lang 0 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2022-04-16 9:20 UTC (permalink / raw) To: Dave Chinner, David Disseldorp, fstests 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 > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/019: kill background processes on interrupt 2022-04-16 9:20 ` Amir Goldstein @ 2022-04-16 14:11 ` Zorro Lang 2022-04-16 17:13 ` Amir Goldstein 0 siblings, 1 reply; 14+ messages in thread From: Zorro Lang @ 2022-04-16 14:11 UTC (permalink / raw) To: Amir Goldstein; +Cc: fstests 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 > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/019: kill background processes on interrupt 2022-04-16 14:11 ` Zorro Lang @ 2022-04-16 17:13 ` Amir Goldstein 2022-04-18 17:07 ` Darrick J. Wong 0 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2022-04-16 17:13 UTC (permalink / raw) To: Amir Goldstein, fstests 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/019: kill background processes on interrupt 2022-04-16 17:13 ` Amir Goldstein @ 2022-04-18 17:07 ` Darrick J. Wong 2022-04-18 18:08 ` Zorro Lang 0 siblings, 1 reply; 14+ messages in thread From: Darrick J. Wong @ 2022-04-18 17:07 UTC (permalink / raw) To: Amir Goldstein; +Cc: fstests 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/019: kill background processes on interrupt 2022-04-18 17:07 ` Darrick J. Wong @ 2022-04-18 18:08 ` Zorro Lang 0 siblings, 0 replies; 14+ messages in thread From: Zorro Lang @ 2022-04-18 18:08 UTC (permalink / raw) To: Darrick J. Wong; +Cc: fstests 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. > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-04-18 18:08 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-11 5:48 [PATCH] generic/019: kill background processes on interrupt Dave Chinner 2022-04-12 12:59 ` David Disseldorp 2022-04-12 14:25 ` Zorro Lang 2022-04-13 0:26 ` Dave Chinner 2022-04-13 7:13 ` Amir Goldstein 2022-04-13 23:27 ` Darrick J. Wong 2022-04-18 0:02 ` Dave Chinner 2022-04-14 16:25 ` Zorro Lang 2022-04-14 19:16 ` Zorro Lang 2022-04-16 9:20 ` Amir Goldstein 2022-04-16 14:11 ` Zorro Lang 2022-04-16 17:13 ` Amir Goldstein 2022-04-18 17:07 ` Darrick J. Wong 2022-04-18 18:08 ` Zorro Lang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.