| Message ID | 1563553816-148827-7-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow Valgrind checking all QEMU processes | expand |
On 7/19/19 12:30 PM, Andrey Shinkevich wrote: > To synchronize the time when QEMU is running longer under the Valgrind, > increase the sleeping time in the test 247. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > tests/qemu-iotests/247 | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247 > index 546a794..c853b73 100755 > --- a/tests/qemu-iotests/247 > +++ b/tests/qemu-iotests/247 > @@ -57,7 +57,11 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size > {"execute":"block-commit", > "arguments":{"device":"format-4", "top-node": "format-2", "base-node":"format-0", "job-id":"job0"}} > EOF > -sleep 1 > +if [ "${VALGRIND_QEMU}" == "y" ]; then > + sleep 10 > +else > + sleep 1 > +fi > echo '{"execute":"quit"}' > ) | $QEMU -qmp stdio -nographic -nodefaults \ > -blockdev file,node-name=file-0,filename=$TEST_IMG.0,auto-read-only=on \ > This makes me nervous, though. Won't this race terribly? (Wait, why doesn't it race already?)
16.08.2019 4:01, John Snow wrote: > > > On 7/19/19 12:30 PM, Andrey Shinkevich wrote: >> To synchronize the time when QEMU is running longer under the Valgrind, >> increase the sleeping time in the test 247. >> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> tests/qemu-iotests/247 | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247 >> index 546a794..c853b73 100755 >> --- a/tests/qemu-iotests/247 >> +++ b/tests/qemu-iotests/247 >> @@ -57,7 +57,11 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size >> {"execute":"block-commit", >> "arguments":{"device":"format-4", "top-node": "format-2", "base-node":"format-0", "job-id":"job0"}} >> EOF >> -sleep 1 >> +if [ "${VALGRIND_QEMU}" == "y" ]; then >> + sleep 10 >> +else >> + sleep 1 >> +fi >> echo '{"execute":"quit"}' >> ) | $QEMU -qmp stdio -nographic -nodefaults \ >> -blockdev file,node-name=file-0,filename=$TEST_IMG.0,auto-read-only=on \ >> > > This makes me nervous, though. Won't this race terribly? (Wait, why > doesn't it race already?) > Hmm, however it works somehow. I'm afraid that everything with "sleep" is definitely racy.. Or what do you mean?
On 16/08/2019 04:01, John Snow wrote: > > > On 7/19/19 12:30 PM, Andrey Shinkevich wrote: >> To synchronize the time when QEMU is running longer under the Valgrind, >> increase the sleeping time in the test 247. >> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> tests/qemu-iotests/247 | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247 >> index 546a794..c853b73 100755 >> --- a/tests/qemu-iotests/247 >> +++ b/tests/qemu-iotests/247 >> @@ -57,7 +57,11 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size >> {"execute":"block-commit", >> "arguments":{"device":"format-4", "top-node": "format-2", "base-node":"format-0", "job-id":"job0"}} >> EOF >> -sleep 1 >> +if [ "${VALGRIND_QEMU}" == "y" ]; then >> + sleep 10 >> +else >> + sleep 1 >> +fi >> echo '{"execute":"quit"}' >> ) | $QEMU -qmp stdio -nographic -nodefaults \ >> -blockdev file,node-name=file-0,filename=$TEST_IMG.0,auto-read-only=on \ >> > > This makes me nervous, though. Won't this race terribly? (Wait, why > doesn't it race already?) > It maybe better to rewrite this test in Python. To let it work under Valgrind in the current implementation, I reserved more seconds. We could have this tolerance just for the test. Andrey
On 8/23/19 11:27 AM, Vladimir Sementsov-Ogievskiy wrote: > 16.08.2019 4:01, John Snow wrote: >> >> >> On 7/19/19 12:30 PM, Andrey Shinkevich wrote: >>> To synchronize the time when QEMU is running longer under the Valgrind, >>> increase the sleeping time in the test 247. >>> >>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> tests/qemu-iotests/247 | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247 >>> index 546a794..c853b73 100755 >>> --- a/tests/qemu-iotests/247 >>> +++ b/tests/qemu-iotests/247 >>> @@ -57,7 +57,11 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size >>> {"execute":"block-commit", >>> "arguments":{"device":"format-4", "top-node": "format-2", "base-node":"format-0", "job-id":"job0"}} >>> EOF >>> -sleep 1 >>> +if [ "${VALGRIND_QEMU}" == "y" ]; then >>> + sleep 10 >>> +else >>> + sleep 1 >>> +fi >>> echo '{"execute":"quit"}' >>> ) | $QEMU -qmp stdio -nographic -nodefaults \ >>> -blockdev file,node-name=file-0,filename=$TEST_IMG.0,auto-read-only=on \ >>> >> >> This makes me nervous, though. Won't this race terribly? (Wait, why >> doesn't it race already?) >> > > Hmm, however it works somehow. I'm afraid that everything with "sleep" is definitely racy.. > Or what do you mean? > Right -- anything with a sleep is already at risk for racing. What I am picking up on here is that with valgrind, there is an even greater computational overhead that's much harder to predict, so I was wondering how these values were determined. (I wouldn't withhold an RB for that alone -- the sleeps are existing problems.) What I moved on to wondering in particular is why test 247 doesn't already have race problems, because it looks quite fragile. Neither of these are really Andrey's problems; I was just surprised momentarily that I don't see 247 fail more often already, as-is. --js
On 27/08/2019 22:42, John Snow wrote: > > > On 8/23/19 11:27 AM, Vladimir Sementsov-Ogievskiy wrote: >> 16.08.2019 4:01, John Snow wrote: >>> >>> >>> On 7/19/19 12:30 PM, Andrey Shinkevich wrote: >>>> To synchronize the time when QEMU is running longer under the Valgrind, >>>> increase the sleeping time in the test 247. >>>> >>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> tests/qemu-iotests/247 | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247 >>>> index 546a794..c853b73 100755 >>>> --- a/tests/qemu-iotests/247 >>>> +++ b/tests/qemu-iotests/247 >>>> @@ -57,7 +57,11 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size >>>> {"execute":"block-commit", >>>> "arguments":{"device":"format-4", "top-node": "format-2", "base-node":"format-0", "job-id":"job0"}} >>>> EOF >>>> -sleep 1 >>>> +if [ "${VALGRIND_QEMU}" == "y" ]; then >>>> + sleep 10 >>>> +else >>>> + sleep 1 >>>> +fi >>>> echo '{"execute":"quit"}' >>>> ) | $QEMU -qmp stdio -nographic -nodefaults \ >>>> -blockdev file,node-name=file-0,filename=$TEST_IMG.0,auto-read-only=on \ >>>> >>> >>> This makes me nervous, though. Won't this race terribly? (Wait, why >>> doesn't it race already?) >>> >> >> Hmm, however it works somehow. I'm afraid that everything with "sleep" is definitely racy.. >> Or what do you mean? >> > > Right -- anything with a sleep is already at risk for racing. > > What I am picking up on here is that with valgrind, there is an even > greater computational overhead that's much harder to predict, so I was > wondering how these values were determined. > I just followed the trend and extended the sleeping time with a grater tolerance so that the test could pass on systems where the 'sleep 1' command helps to pass without Valgrind. We could rewrite the test 247 in Python in a separate series, shall we? Andrey > (I wouldn't withhold an RB for that alone -- the sleeps are existing > problems.) > > What I moved on to wondering in particular is why test 247 doesn't > already have race problems, because it looks quite fragile. > > Neither of these are really Andrey's problems; I was just surprised > momentarily that I don't see 247 fail more often already, as-is. > > --js >
On 8/28/19 11:24 AM, Andrey Shinkevich wrote: > > > On 27/08/2019 22:42, John Snow wrote: >> >> >> On 8/23/19 11:27 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 16.08.2019 4:01, John Snow wrote: >>>> >>>> >>>> On 7/19/19 12:30 PM, Andrey Shinkevich wrote: >>>>> To synchronize the time when QEMU is running longer under the Valgrind, >>>>> increase the sleeping time in the test 247. >>>>> >>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>> --- >>>>> tests/qemu-iotests/247 | 6 +++++- >>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247 >>>>> index 546a794..c853b73 100755 >>>>> --- a/tests/qemu-iotests/247 >>>>> +++ b/tests/qemu-iotests/247 >>>>> @@ -57,7 +57,11 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size >>>>> {"execute":"block-commit", >>>>> "arguments":{"device":"format-4", "top-node": "format-2", "base-node":"format-0", "job-id":"job0"}} >>>>> EOF >>>>> -sleep 1 >>>>> +if [ "${VALGRIND_QEMU}" == "y" ]; then >>>>> + sleep 10 >>>>> +else >>>>> + sleep 1 >>>>> +fi >>>>> echo '{"execute":"quit"}' >>>>> ) | $QEMU -qmp stdio -nographic -nodefaults \ >>>>> -blockdev file,node-name=file-0,filename=$TEST_IMG.0,auto-read-only=on \ >>>>> >>>> >>>> This makes me nervous, though. Won't this race terribly? (Wait, why >>>> doesn't it race already?) >>>> >>> >>> Hmm, however it works somehow. I'm afraid that everything with "sleep" is definitely racy.. >>> Or what do you mean? >>> >> >> Right -- anything with a sleep is already at risk for racing. >> >> What I am picking up on here is that with valgrind, there is an even >> greater computational overhead that's much harder to predict, so I was >> wondering how these values were determined. >> > > I just followed the trend and extended the sleeping time with a grater > tolerance so that the test could pass on systems where the 'sleep 1' > command helps to pass without Valgrind. We could rewrite the test 247 in > Python in a separate series, shall we? > If you have the time, but I don't think anyone will require it for this series. Just reviewing "out loud." I'll look at V6 soon. --js
diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247 index 546a794..c853b73 100755 --- a/tests/qemu-iotests/247 +++ b/tests/qemu-iotests/247 @@ -57,7 +57,11 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size {"execute":"block-commit", "arguments":{"device":"format-4", "top-node": "format-2", "base-node":"format-0", "job-id":"job0"}} EOF -sleep 1 +if [ "${VALGRIND_QEMU}" == "y" ]; then + sleep 10 +else + sleep 1 +fi echo '{"execute":"quit"}' ) | $QEMU -qmp stdio -nographic -nodefaults \ -blockdev file,node-name=file-0,filename=$TEST_IMG.0,auto-read-only=on \