| Message ID | 20220613151721.18664-1-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blktests: Ignore errors from wait(1) | expand |
On 6/13/22 08:17, Jan Kara wrote: > Multiple blktests use wait(1) to wait for background tasks. However in > some cases tasks can exit before wait(1) is called and in that case > wait(1) complains which breaks expected output. Make sure we ignore > output from wait(1) to avoid this breakage. > > Signed-off-by: Jan Kara <jack@suse.cz> Please note that Shinichiro (CC'd here) is a new blktests maintainer and not Omar. Looks good. Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> -ck
On Jun 13, 2022 / 17:17, Jan Kara wrote: > Multiple blktests use wait(1) to wait for background tasks. However in > some cases tasks can exit before wait(1) is called and in that case > wait(1) complains which breaks expected output. Make sure we ignore > output from wait(1) to avoid this breakage. Hi Jan, thanks for the patch. May I know how to create the wait(1) complaint message? I added sleep command before the waits to ensure the background tasks completed, but was not able to see message by wait on my test system. I suspect it may depend on bash version.
On Mon 13-06-22 22:48:20, Chaitanya Kulkarni wrote: > On 6/13/22 08:17, Jan Kara wrote: > > Multiple blktests use wait(1) to wait for background tasks. However in > > some cases tasks can exit before wait(1) is called and in that case > > wait(1) complains which breaks expected output. Make sure we ignore > > output from wait(1) to avoid this breakage. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > Please note that Shinichiro (CC'd here) is a new blktests > maintainer and not Omar. Thanks for review and notifying me. Maybe CONTRIBUTING.md should be updated to speak about Shinichiro and not Omar? Honza
On Tue 14-06-22 07:04:54, Shinichiro Kawasaki wrote: > On Jun 13, 2022 / 17:17, Jan Kara wrote: > > Multiple blktests use wait(1) to wait for background tasks. However in > > some cases tasks can exit before wait(1) is called and in that case > > wait(1) complains which breaks expected output. Make sure we ignore > > output from wait(1) to avoid this breakage. > > Hi Jan, thanks for the patch. > > May I know how to create the wait(1) complaint message? I added sleep > command before the waits to ensure the background tasks completed, but > was not able to see message by wait on my test system. I suspect it may > depend on bash version. Yes, I suspect it depends on the shell as well (because otherwise I expect people would hit this much earlier than me :). The bash I have is "4.4.23(1)-release" - the one in openSUSE 15.3 and it shows the error pretty reliably... Honza
On Jun 14, 2022 / 15:15, Jan Kara wrote: > On Mon 13-06-22 22:48:20, Chaitanya Kulkarni wrote: > > On 6/13/22 08:17, Jan Kara wrote: > > > Multiple blktests use wait(1) to wait for background tasks. However in > > > some cases tasks can exit before wait(1) is called and in that case > > > wait(1) complains which breaks expected output. Make sure we ignore > > > output from wait(1) to avoid this breakage. > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > Please note that Shinichiro (CC'd here) is a new blktests > > maintainer and not Omar. > > Thanks for review and notifying me. Maybe CONTRIBUTING.md should be updated > to speak about Shinichiro and not Omar? Yes, it was done, just a few weeks ago :) https://lore.kernel.org/linux-block/20220525020424.14131-1-shinichiro.kawasaki@wdc.com/
On Jun 14, 2022 / 15:18, Jan Kara wrote: > On Tue 14-06-22 07:04:54, Shinichiro Kawasaki wrote: [...] > Yes, I suspect it depends on the shell as well (because otherwise I expect > people would hit this much earlier than me :). The bash I have is > "4.4.23(1)-release" - the one in openSUSE 15.3 and it shows the error > pretty reliably... I guess the problematic wait command output is job exit status report. To confirm my guess, could your share example of the wait command output? Based on my guess, I checked difference between bash 4.4 and my bash 5.1, but I did not find notable bash code change about job exit status report. Hmm. Your patch covers several waits in the block group. I suspect other waits in other groups may have same risk. Instead of your patch, could you try the patch below on your system? If it works, all waits in all groups can be addressed. diff --git a/check b/check index 7037d88..ac24afa 100755 --- a/check +++ b/check @@ -440,6 +440,10 @@ _run_test() { RUN_FOR_ZONED=0 FALLBACK_DEVICE=0 + # Ensure job control monitor mode is off in this sub-shell to suppress + # job status output. + set +m + # shellcheck disable=SC1090 . "tests/${TEST_NAME}"
On Wed 15-06-22 11:50:14, Shinichiro Kawasaki wrote: > On Jun 14, 2022 / 15:18, Jan Kara wrote: > > On Tue 14-06-22 07:04:54, Shinichiro Kawasaki wrote: > > [...] > > > Yes, I suspect it depends on the shell as well (because otherwise I expect > > people would hit this much earlier than me :). The bash I have is > > "4.4.23(1)-release" - the one in openSUSE 15.3 and it shows the error > > pretty reliably... > > I guess the problematic wait command output is job exit status report. To > confirm my guess, could your share example of the wait command output? > > Based on my guess, I checked difference between bash 4.4 and my bash 5.1, but I > did not find notable bash code change about job exit status report. Hmm. > > Your patch covers several waits in the block group. I suspect other waits in > other groups may have same risk. Instead of your patch, could you try the patch > below on your system? If it works, all waits in all groups can be addressed. Ah, indeed. This patch fixes the problem for me as well. Thanks for looking into this! Honza > > diff --git a/check b/check > index 7037d88..ac24afa 100755 > --- a/check > +++ b/check > @@ -440,6 +440,10 @@ _run_test() { > RUN_FOR_ZONED=0 > FALLBACK_DEVICE=0 > > + # Ensure job control monitor mode is off in this sub-shell to suppress > + # job status output. > + set +m > + > # shellcheck disable=SC1090 > . "tests/${TEST_NAME}" > > > > -- > Shin'ichiro Kawasaki
On Jun 15, 2022 / 18:56, Jan Kara wrote: > On Wed 15-06-22 11:50:14, Shinichiro Kawasaki wrote: [...] > > Your patch covers several waits in the block group. I suspect other waits in > > other groups may have same risk. Instead of your patch, could you try the patch > > below on your system? If it works, all waits in all groups can be addressed. > > Ah, indeed. This patch fixes the problem for me as well. Thanks for looking > into this! Okay, thanks! I've posted the official patch to go through the review process.
diff --git a/tests/block/016 b/tests/block/016 index 775069c..d399ec6 100755 --- a/tests/block/016 +++ b/tests/block/016 @@ -41,7 +41,7 @@ test() { # While dd is blocked, send a signal which we know dd has a handler # for. kill -USR1 $! - wait + wait &>/dev/null _exit_null_blk diff --git a/tests/block/017 b/tests/block/017 index 8596888..ff68e24 100755 --- a/tests/block/017 +++ b/tests/block/017 @@ -40,7 +40,7 @@ test() { sleep 0.1 show_inflight - wait + wait &>/dev/null show_inflight _exit_null_blk diff --git a/tests/block/018 b/tests/block/018 index e7ac445..d2c97ea 100755 --- a/tests/block/018 +++ b/tests/block/018 @@ -50,7 +50,7 @@ test() { dd if=/dev/nullb1 of=/dev/null bs=4096 iflag=direct count=1 status=none & dd if=/dev/zero of=/dev/nullb1 bs=4096 oflag=direct count=1 status=none & dd if=/dev/zero of=/dev/nullb1 bs=4096 oflag=direct count=1 status=none & - wait + wait &>/dev/null show_times _exit_null_blk diff --git a/tests/block/024 b/tests/block/024 index 2a7c934..dd99f0c 100755 --- a/tests/block/024 +++ b/tests/block/024 @@ -57,7 +57,7 @@ test() { dd if=/dev/nullb1 of=/dev/null bs=4096 iflag=direct count=1500 status=none & dd if=/dev/zero of=/dev/nullb1 bs=4096 oflag=direct count=1800 status=none & dd if=/dev/zero of=/dev/nullb1 bs=4096 oflag=direct count=1800 status=none & - wait + wait &>/dev/null show_times _exit_null_blk diff --git a/tests/block/029 b/tests/block/029 index b9a897d..cb8fd03 100755 --- a/tests/block/029 +++ b/tests/block/029 @@ -41,7 +41,7 @@ test() { --runtime="${TIMEOUT}" --name=nullb1 \ --output="${RESULTS_DIR}/block/fio-output-029.txt" \ >>"$FULL" - wait + wait &>/dev/null else echo "Skipping test because $sq cannot be modified" >>"$FULL" fi
Multiple blktests use wait(1) to wait for background tasks. However in some cases tasks can exit before wait(1) is called and in that case wait(1) complains which breaks expected output. Make sure we ignore output from wait(1) to avoid this breakage. Signed-off-by: Jan Kara <jack@suse.cz> --- tests/block/016 | 2 +- tests/block/017 | 2 +- tests/block/018 | 2 +- tests/block/024 | 2 +- tests/block/029 | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-)