[PATCH] test: Retry umount if necessary

[PATCH] test: Retry umount if necessary

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

The io tests tst.local.sh and tst.wait.sh fail with some frequency,
with error messages like "umount: $iodir: target is busy."

Modify the tests' dtrace trigger, doio.sh, so that it will retry
umount a few times, if necessary.  Specifically, remove the "set -e"
and individually check for and report errors, in the case of umount
retrying a few times before giving up.

While we're at it, notice that the two tests use the same $iodir.
There is no need for the tests to be coupled in this way.  Change
those two tests so that each test has a fresh value of iodir.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 test/triggers/doio.sh         | 29 ++++++++++++++++++++++++++---
 test/unittest/io/tst.local.sh |  2 +-
 test/unittest/io/tst.wait.sh  |  2 +-
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/test/triggers/doio.sh b/test/triggers/doio.sh
index a2a39c245..0171a4b4c 100755
--- a/test/triggers/doio.sh
+++ b/test/triggers/doio.sh
@@ -22,15 +22,38 @@ mountdir=$4
 mountarg1=${5-""}
 mountarg2=${6-""}
 
-set -e
-
 # do writes
 dd if=/dev/urandom of=$tempfile count=$filesize bs=1 status=none
+if [ $? -ne 0 ]; then
+	echo ERROR dd
+	exit 1
+fi
 
 # flush cache and remount the file system to force the IO
-umount $mountdir
+ntries=3
+while [ $ntries -gt 0 ]; do
+	umount $mountdir >& /dev/null
+	if [ $? -eq 0 ]; then
+		break
+	fi
+	sleep 1
+	ntries=$(($ntries - 1))
+done
+if [ $ntries -eq 0 ]; then
+	echo ERROR umount
+	exit 1
+fi
+
 echo 3 > /proc/sys/vm/drop_caches
 $mountcmd $mountdir $mountarg1 $mountarg2
+if [ $? -ne 0 ]; then
+	echo ERROR $mountcmd
+	exit 1
+fi
 
 # do reads
 sum $tempfile > /dev/null
+if [ $? -ne 0 ]; then
+	echo ERROR sum
+	exit 1
+fi
diff --git a/test/unittest/io/tst.local.sh b/test/unittest/io/tst.local.sh
index d3dbf1713..702c6f453 100755
--- a/test/unittest/io/tst.local.sh
+++ b/test/unittest/io/tst.local.sh
@@ -18,7 +18,7 @@ minsize=$((filesize / 10 * 9))
 fstype=xfs
 # file system-specific options
 fsoptions="defaults,atime,diratime,nosuid,nodev"
-iodir=$tmpdir/test-$fstype-io
+iodir=$tmpdir/test-$fstype-io-local.$$
 tempfile=`mktemp -u -p $iodir`
 
 trap "umount $iodir; rmdir $iodir; rm -f $iodir.img" QUIT EXIT
diff --git a/test/unittest/io/tst.wait.sh b/test/unittest/io/tst.wait.sh
index 016b922eb..24ac2e436 100755
--- a/test/unittest/io/tst.wait.sh
+++ b/test/unittest/io/tst.wait.sh
@@ -15,7 +15,7 @@ filesize=$((1024*$nblocks))
 fstype=xfs
 # file system-specific options
 fsoptions="defaults,atime,diratime,nosuid,nodev"
-iodir=$tmpdir/test-$fstype-io
+iodir=$tmpdir/test-$fstype-io-wait.$$
 tempfile=`mktemp -u -p $iodir`
 
 trap "umount $iodir; rmdir $iodir; rm -f $iodir.img" QUIT EXIT
-- 
2.43.5

Re: [DTrace-devel] [PATCH] test: Retry umount if necessary

[Nick Alcock]

On 3 Dec 2024, eugene loh said:

> From: Eugene Loh <eugene.loh@oracle.com>
>
> The io tests tst.local.sh and tst.wait.sh fail with some frequency,
> with error messages like "umount: $iodir: target is busy."

Can you just do a umount -l, then? The only caveat is that while this
will disconnect the mount from the mount tree immediately, the *actual
umount* might happen arbitrarily later -- is that a problem?

Re: [DTrace-devel] [PATCH] test: Retry umount if necessary

[Eugene Loh]

On 12/3/24 15:31, Nick Alcock wrote:

> On 3 Dec 2024, eugene loh said:
>
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> The io tests tst.local.sh and tst.wait.sh fail with some frequency,
>> with error messages like "umount: $iodir: target is busy."
> Can you just do a umount -l, then? The only caveat is that while this
> will disconnect the mount from the mount tree immediately, the *actual
> umount* might happen arbitrarily later -- is that a problem?

Maybe?  In test/triggers/doio.sh, you can see what's going on.  In 
essence, we want to flush operations out.  A lazy umount might not do 
that?  I'm not sure.

Re: [DTrace-devel] [PATCH] test: Retry umount if necessary

[Nick Alcock]

On 3 Dec 2024, Eugene Loh via DTrace-devel uttered the following:

> On 12/3/24 15:31, Nick Alcock wrote:
>
>> On 3 Dec 2024, eugene loh said:
>>
>>> From: Eugene Loh <eugene.loh@oracle.com>
>>>
>>> The io tests tst.local.sh and tst.wait.sh fail with some frequency,
>>> with error messages like "umount: $iodir: target is busy."
>> Can you just do a umount -l, then? The only caveat is that while this
>> will disconnect the mount from the mount tree immediately, the *actual
>> umount* might happen arbitrarily later -- is that a problem?
>
> Maybe?  In test/triggers/doio.sh, you can see what's going on.  In essence, we want to flush operations out.  A lazy umount might
> not do that?  I'm not sure.

No, it wouldn't. A simple sync should, though: once it returns, the bios
should have got all the way through the stack, IIRC.

-- 
NULL && (void)

Re: [PATCH] test: Retry umount if necessary

[Kris Van Hees]

On Tue, Dec 03, 2024 at 03:16:01PM -0500, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> The io tests tst.local.sh and tst.wait.sh fail with some frequency,
> with error messages like "umount: $iodir: target is busy."
> 
> Modify the tests' dtrace trigger, doio.sh, so that it will retry
> umount a few times, if necessary.  Specifically, remove the "set -e"
> and individually check for and report errors, in the case of umount
> retrying a few times before giving up.
> 
> While we're at it, notice that the two tests use the same $iodir.
> There is no need for the tests to be coupled in this way.  Change
> those two tests so that each test has a fresh value of iodir.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

... I did read the discussion about lazy umount, but I think that in terms
of what is best for the testsuite, being able to ascertain that the umount
is actually getting done (or reporting an error on it) is important in case
later tests might get affected by it.  Lazy umount seems like it would lead
to possible silent umount conditions which is not ideal for a test.

> ---
>  test/triggers/doio.sh         | 29 ++++++++++++++++++++++++++---
>  test/unittest/io/tst.local.sh |  2 +-
>  test/unittest/io/tst.wait.sh  |  2 +-
>  3 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/test/triggers/doio.sh b/test/triggers/doio.sh
> index a2a39c245..0171a4b4c 100755
> --- a/test/triggers/doio.sh
> +++ b/test/triggers/doio.sh
> @@ -22,15 +22,38 @@ mountdir=$4
>  mountarg1=${5-""}
>  mountarg2=${6-""}
>  
> -set -e
> -
>  # do writes
>  dd if=/dev/urandom of=$tempfile count=$filesize bs=1 status=none
> +if [ $? -ne 0 ]; then
> +	echo ERROR dd
> +	exit 1
> +fi
>  
>  # flush cache and remount the file system to force the IO
> -umount $mountdir
> +ntries=3
> +while [ $ntries -gt 0 ]; do
> +	umount $mountdir >& /dev/null
> +	if [ $? -eq 0 ]; then
> +		break
> +	fi
> +	sleep 1
> +	ntries=$(($ntries - 1))
> +done
> +if [ $ntries -eq 0 ]; then
> +	echo ERROR umount
> +	exit 1
> +fi
> +
>  echo 3 > /proc/sys/vm/drop_caches
>  $mountcmd $mountdir $mountarg1 $mountarg2
> +if [ $? -ne 0 ]; then
> +	echo ERROR $mountcmd
> +	exit 1
> +fi
>  
>  # do reads
>  sum $tempfile > /dev/null
> +if [ $? -ne 0 ]; then
> +	echo ERROR sum
> +	exit 1
> +fi
> diff --git a/test/unittest/io/tst.local.sh b/test/unittest/io/tst.local.sh
> index d3dbf1713..702c6f453 100755
> --- a/test/unittest/io/tst.local.sh
> +++ b/test/unittest/io/tst.local.sh
> @@ -18,7 +18,7 @@ minsize=$((filesize / 10 * 9))
>  fstype=xfs
>  # file system-specific options
>  fsoptions="defaults,atime,diratime,nosuid,nodev"
> -iodir=$tmpdir/test-$fstype-io
> +iodir=$tmpdir/test-$fstype-io-local.$$
>  tempfile=`mktemp -u -p $iodir`
>  
>  trap "umount $iodir; rmdir $iodir; rm -f $iodir.img" QUIT EXIT
> diff --git a/test/unittest/io/tst.wait.sh b/test/unittest/io/tst.wait.sh
> index 016b922eb..24ac2e436 100755
> --- a/test/unittest/io/tst.wait.sh
> +++ b/test/unittest/io/tst.wait.sh
> @@ -15,7 +15,7 @@ filesize=$((1024*$nblocks))
>  fstype=xfs
>  # file system-specific options
>  fsoptions="defaults,atime,diratime,nosuid,nodev"
> -iodir=$tmpdir/test-$fstype-io
> +iodir=$tmpdir/test-$fstype-io-wait.$$
>  tempfile=`mktemp -u -p $iodir`
>  
>  trap "umount $iodir; rmdir $iodir; rm -f $iodir.img" QUIT EXIT
> -- 
> 2.43.5
>