diff mbox series

[2/2] generic/500: skip this test if formatting fails

Message ID 167521270079.2382722.2799074346773170090.stgit@magnolia (mailing list archive)
State Deferred, archived
Headers show
Series fstests: random fixes for v2023.01.22 | expand

Commit Message

Darrick J. Wong Feb. 1, 2023, 12:51 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

This testcase exercises what happens when we race a filesystem
perforing discard operations against a thin provisioning device that has
run out of space.  To constrain runtime, it creates a 128M thinp volume
and formats it.

However, if that initial format fails because (say) the 128M volume is
too small, then the test fails.  This is really a case of test
preconditions not being satisfied, so let's make the test _notrun when
this happens.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/dmthin     |    7 ++++-
 common/rc         |   80 +++++++++++++++++++++++++++--------------------------
 tests/generic/500 |    3 +-
 3 files changed, 48 insertions(+), 42 deletions(-)

Comments

Zorro Lang Feb. 2, 2023, 10 a.m. UTC | #1 
On Tue, Jan 31, 2023 at 04:51:40PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> This testcase exercises what happens when we race a filesystem
> perforing discard operations against a thin provisioning device that has
> run out of space.  To constrain runtime, it creates a 128M thinp volume
> and formats it.
> 
> However, if that initial format fails because (say) the 128M volume is
> too small, then the test fails.  This is really a case of test
> preconditions not being satisfied, so let's make the test _notrun when
> this happens.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  common/dmthin     |    7 ++++-
>  common/rc         |   80 +++++++++++++++++++++++++++--------------------------
>  tests/generic/500 |    3 +-
>  3 files changed, 48 insertions(+), 42 deletions(-)
> 
> 
> diff --git a/common/dmthin b/common/dmthin
> index 91147e47ac..7107d50804 100644
> --- a/common/dmthin
> +++ b/common/dmthin
> @@ -234,5 +234,10 @@ _dmthin_mount()
>  _dmthin_mkfs()
>  {
>  	_scratch_options mkfs
> -	_mkfs_dev $SCRATCH_OPTIONS $@ $DMTHIN_VOL_DEV
> +	_mkfs_dev $SCRATCH_OPTIONS "$@" $DMTHIN_VOL_DEV

This patch adds quote marks to $@, that an extra change. So I hope to know if
this change fix something wrong or you just felt it's better to have?

If this part has a good explanation, others good to me, then I'd like to give
it:

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

Thanks,
Zorro

> +}
> +_dmthin_try_mkfs()
> +{
> +	_scratch_options mkfs
> +	_try_mkfs_dev $SCRATCH_OPTIONS "$@" $DMTHIN_VOL_DEV
>  }
> diff --git a/common/rc b/common/rc
> index 36eb90e1f1..376a0138b4 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -604,49 +604,49 @@ _test_mkfs()
>      esac
>  }
>  
> +_try_mkfs_dev()
> +{
> +    case $FSTYP in
> +    nfs*)
> +	# do nothing for nfs
> +	;;
> +    9p)
> +	# do nothing for 9p
> +	;;
> +    fuse)
> +	# do nothing for fuse
> +	;;
> +    virtiofs)
> +	# do nothing for virtiofs
> +	;;
> +    overlay)
> +	# do nothing for overlay
> +	;;
> +    pvfs2)
> +	# do nothing for pvfs2
> +	;;
> +    udf)
> +        $MKFS_UDF_PROG $MKFS_OPTIONS $*
> +	;;
> +    btrfs)
> +        $MKFS_BTRFS_PROG $MKFS_OPTIONS $*
> +	;;
> +    ext2|ext3|ext4)
> +	$MKFS_PROG -t $FSTYP -- -F $MKFS_OPTIONS $*
> +	;;
> +    xfs)
> +	$MKFS_PROG -t $FSTYP -- -f $MKFS_OPTIONS $*
> +	;;
> +    *)
> +	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $*
> +	;;
> +    esac
> +}
> +
>  _mkfs_dev()
>  {
>      local tmp=`mktemp -u`
> -    case $FSTYP in
> -    nfs*)
> -	# do nothing for nfs
> -	;;
> -    9p)
> -	# do nothing for 9p
> -	;;
> -    fuse)
> -	# do nothing for fuse
> -	;;
> -    virtiofs)
> -	# do nothing for virtiofs
> -	;;
> -    overlay)
> -	# do nothing for overlay
> -	;;
> -    pvfs2)
> -	# do nothing for pvfs2
> -	;;
> -    udf)
> -        $MKFS_UDF_PROG $MKFS_OPTIONS $* 2>$tmp.mkfserr 1>$tmp.mkfsstd
> -	;;
> -    btrfs)
> -        $MKFS_BTRFS_PROG $MKFS_OPTIONS $* 2>$tmp.mkfserr 1>$tmp.mkfsstd
> -	;;
> -    ext2|ext3|ext4)
> -	$MKFS_PROG -t $FSTYP -- -F $MKFS_OPTIONS $* \
> -		2>$tmp.mkfserr 1>$tmp.mkfsstd
> -	;;
> -    xfs)
> -	$MKFS_PROG -t $FSTYP -- -f $MKFS_OPTIONS $* \
> -		2>$tmp.mkfserr 1>$tmp.mkfsstd
> -	;;
> -    *)
> -	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* \
> -		2>$tmp.mkfserr 1>$tmp.mkfsstd
> -	;;
> -    esac
> -
> -    if [ $? -ne 0 ]; then
> +    if ! _try_mkfs_dev "$@" 2>$tmp.mkfserr 1>$tmp.mkfsstd; then
>  	# output stored mkfs output
>  	cat $tmp.mkfserr >&2
>  	cat $tmp.mkfsstd
> diff --git a/tests/generic/500 b/tests/generic/500
> index bc84d219fa..1151c8f234 100755
> --- a/tests/generic/500
> +++ b/tests/generic/500
> @@ -58,7 +58,8 @@ CLUSTER_SIZE=$((64 * 1024 / 512))		# 64K
>  
>  _dmthin_init $BACKING_SIZE $VIRTUAL_SIZE $CLUSTER_SIZE 0
>  _dmthin_set_fail
> -_dmthin_mkfs
> +_dmthin_try_mkfs >> $seqres.full 2>&1 || \
> +	_notrun "Could not format small thinp filesystem for test"
>  _dmthin_mount
>  
>  # There're two bugs at here, one is dm-thin bug, the other is filesystem
>
Darrick J. Wong Feb. 2, 2023, 5:04 p.m. UTC | #2 
On Thu, Feb 02, 2023 at 06:00:57PM +0800, Zorro Lang wrote:
> On Tue, Jan 31, 2023 at 04:51:40PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > This testcase exercises what happens when we race a filesystem
> > perforing discard operations against a thin provisioning device that has
> > run out of space.  To constrain runtime, it creates a 128M thinp volume
> > and formats it.
> > 
> > However, if that initial format fails because (say) the 128M volume is
> > too small, then the test fails.  This is really a case of test
> > preconditions not being satisfied, so let's make the test _notrun when
> > this happens.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  common/dmthin     |    7 ++++-
> >  common/rc         |   80 +++++++++++++++++++++++++++--------------------------
> >  tests/generic/500 |    3 +-
> >  3 files changed, 48 insertions(+), 42 deletions(-)
> > 
> > 
> > diff --git a/common/dmthin b/common/dmthin
> > index 91147e47ac..7107d50804 100644
> > --- a/common/dmthin
> > +++ b/common/dmthin
> > @@ -234,5 +234,10 @@ _dmthin_mount()
> >  _dmthin_mkfs()
> >  {
> >  	_scratch_options mkfs
> > -	_mkfs_dev $SCRATCH_OPTIONS $@ $DMTHIN_VOL_DEV
> > +	_mkfs_dev $SCRATCH_OPTIONS "$@" $DMTHIN_VOL_DEV
> 
> This patch adds quote marks to $@, that an extra change. So I hope to know if
> this change fix something wrong or you just felt it's better to have?

Putting double quotes around "$@" (and any other array access) tells
bash that it shouldn't word split any array elements that contain
spaces[1].  Observe:

$ cat /tmp/a.sh
#!/bin/bash

strace -s99 -e execve echo $@
$ /tmp/a.sh 'how now' moo cow
execve("/usr/bin/echo", ["echo", "how", "now", "moo", "cow"], 0x7ffcfb44f218 /* 68 vars */) = 0
how now moo cow
+++ exited with 0 +++

Oops, 'how now' was supposed to be a single argument, but now it is two.

$ cat /tmp/a.sh
#!/bin/bash

strace -s99 -e execve echo "$@"
$ /tmp/a.sh 'how now' moo cow
execve("/usr/bin/echo", ["echo", "how now", "moo", "cow"], 0x7ffccb72d810 /* 68 vars */) = 0
how now moo cow
+++ exited with 0 +++

With the quoted usage, strings with spaces are copied through correctly.
At the moment I don't think this has any practical effect on fstests
because most kernel developers "know" they shouldn't be putting spaces
in anything they feed to fstests, but I've decided that I'll at least
try to fix some of the more egregious problems as I go.

On that topic, does anyone use shellcheck to static-analyze their
fstests code?

--D

[1] https://www.shellcheck.net/wiki/SC2068

> If this part has a good explanation, others good to me, then I'd like to give
> it:
> 
> Reviewed-by: Zorro Lang <zlang@redhat.com>
> 
> Thanks,
> Zorro
> 
> > +}
> > +_dmthin_try_mkfs()
> > +{
> > +	_scratch_options mkfs
> > +	_try_mkfs_dev $SCRATCH_OPTIONS "$@" $DMTHIN_VOL_DEV
> >  }
> > diff --git a/common/rc b/common/rc
> > index 36eb90e1f1..376a0138b4 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -604,49 +604,49 @@ _test_mkfs()
> >      esac
> >  }
> >  
> > +_try_mkfs_dev()
> > +{
> > +    case $FSTYP in
> > +    nfs*)
> > +	# do nothing for nfs
> > +	;;
> > +    9p)
> > +	# do nothing for 9p
> > +	;;
> > +    fuse)
> > +	# do nothing for fuse
> > +	;;
> > +    virtiofs)
> > +	# do nothing for virtiofs
> > +	;;
> > +    overlay)
> > +	# do nothing for overlay
> > +	;;
> > +    pvfs2)
> > +	# do nothing for pvfs2
> > +	;;
> > +    udf)
> > +        $MKFS_UDF_PROG $MKFS_OPTIONS $*
> > +	;;
> > +    btrfs)
> > +        $MKFS_BTRFS_PROG $MKFS_OPTIONS $*
> > +	;;
> > +    ext2|ext3|ext4)
> > +	$MKFS_PROG -t $FSTYP -- -F $MKFS_OPTIONS $*
> > +	;;
> > +    xfs)
> > +	$MKFS_PROG -t $FSTYP -- -f $MKFS_OPTIONS $*
> > +	;;
> > +    *)
> > +	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $*
> > +	;;
> > +    esac
> > +}
> > +
> >  _mkfs_dev()
> >  {
> >      local tmp=`mktemp -u`
> > -    case $FSTYP in
> > -    nfs*)
> > -	# do nothing for nfs
> > -	;;
> > -    9p)
> > -	# do nothing for 9p
> > -	;;
> > -    fuse)
> > -	# do nothing for fuse
> > -	;;
> > -    virtiofs)
> > -	# do nothing for virtiofs
> > -	;;
> > -    overlay)
> > -	# do nothing for overlay
> > -	;;
> > -    pvfs2)
> > -	# do nothing for pvfs2
> > -	;;
> > -    udf)
> > -        $MKFS_UDF_PROG $MKFS_OPTIONS $* 2>$tmp.mkfserr 1>$tmp.mkfsstd
> > -	;;
> > -    btrfs)
> > -        $MKFS_BTRFS_PROG $MKFS_OPTIONS $* 2>$tmp.mkfserr 1>$tmp.mkfsstd
> > -	;;
> > -    ext2|ext3|ext4)
> > -	$MKFS_PROG -t $FSTYP -- -F $MKFS_OPTIONS $* \
> > -		2>$tmp.mkfserr 1>$tmp.mkfsstd
> > -	;;
> > -    xfs)
> > -	$MKFS_PROG -t $FSTYP -- -f $MKFS_OPTIONS $* \
> > -		2>$tmp.mkfserr 1>$tmp.mkfsstd
> > -	;;
> > -    *)
> > -	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* \
> > -		2>$tmp.mkfserr 1>$tmp.mkfsstd
> > -	;;
> > -    esac
> > -
> > -    if [ $? -ne 0 ]; then
> > +    if ! _try_mkfs_dev "$@" 2>$tmp.mkfserr 1>$tmp.mkfsstd; then
> >  	# output stored mkfs output
> >  	cat $tmp.mkfserr >&2
> >  	cat $tmp.mkfsstd
> > diff --git a/tests/generic/500 b/tests/generic/500
> > index bc84d219fa..1151c8f234 100755
> > --- a/tests/generic/500
> > +++ b/tests/generic/500
> > @@ -58,7 +58,8 @@ CLUSTER_SIZE=$((64 * 1024 / 512))		# 64K
> >  
> >  _dmthin_init $BACKING_SIZE $VIRTUAL_SIZE $CLUSTER_SIZE 0
> >  _dmthin_set_fail
> > -_dmthin_mkfs
> > +_dmthin_try_mkfs >> $seqres.full 2>&1 || \
> > +	_notrun "Could not format small thinp filesystem for test"
> >  _dmthin_mount
> >  
> >  # There're two bugs at here, one is dm-thin bug, the other is filesystem
> > 
>
diff mbox series

Patch

diff --git a/common/dmthin b/common/dmthin
index 91147e47ac..7107d50804 100644
--- a/common/dmthin
+++ b/common/dmthin
@@ -234,5 +234,10 @@  _dmthin_mount()
 _dmthin_mkfs()
 {
 	_scratch_options mkfs
-	_mkfs_dev $SCRATCH_OPTIONS $@ $DMTHIN_VOL_DEV
+	_mkfs_dev $SCRATCH_OPTIONS "$@" $DMTHIN_VOL_DEV
+}
+_dmthin_try_mkfs()
+{
+	_scratch_options mkfs
+	_try_mkfs_dev $SCRATCH_OPTIONS "$@" $DMTHIN_VOL_DEV
 }
diff --git a/common/rc b/common/rc
index 36eb90e1f1..376a0138b4 100644
--- a/common/rc
+++ b/common/rc
@@ -604,49 +604,49 @@  _test_mkfs()
     esac
 }
 
+_try_mkfs_dev()
+{
+    case $FSTYP in
+    nfs*)
+	# do nothing for nfs
+	;;
+    9p)
+	# do nothing for 9p
+	;;
+    fuse)
+	# do nothing for fuse
+	;;
+    virtiofs)
+	# do nothing for virtiofs
+	;;
+    overlay)
+	# do nothing for overlay
+	;;
+    pvfs2)
+	# do nothing for pvfs2
+	;;
+    udf)
+        $MKFS_UDF_PROG $MKFS_OPTIONS $*
+	;;
+    btrfs)
+        $MKFS_BTRFS_PROG $MKFS_OPTIONS $*
+	;;
+    ext2|ext3|ext4)
+	$MKFS_PROG -t $FSTYP -- -F $MKFS_OPTIONS $*
+	;;
+    xfs)
+	$MKFS_PROG -t $FSTYP -- -f $MKFS_OPTIONS $*
+	;;
+    *)
+	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $*
+	;;
+    esac
+}
+
 _mkfs_dev()
 {
     local tmp=`mktemp -u`
-    case $FSTYP in
-    nfs*)
-	# do nothing for nfs
-	;;
-    9p)
-	# do nothing for 9p
-	;;
-    fuse)
-	# do nothing for fuse
-	;;
-    virtiofs)
-	# do nothing for virtiofs
-	;;
-    overlay)
-	# do nothing for overlay
-	;;
-    pvfs2)
-	# do nothing for pvfs2
-	;;
-    udf)
-        $MKFS_UDF_PROG $MKFS_OPTIONS $* 2>$tmp.mkfserr 1>$tmp.mkfsstd
-	;;
-    btrfs)
-        $MKFS_BTRFS_PROG $MKFS_OPTIONS $* 2>$tmp.mkfserr 1>$tmp.mkfsstd
-	;;
-    ext2|ext3|ext4)
-	$MKFS_PROG -t $FSTYP -- -F $MKFS_OPTIONS $* \
-		2>$tmp.mkfserr 1>$tmp.mkfsstd
-	;;
-    xfs)
-	$MKFS_PROG -t $FSTYP -- -f $MKFS_OPTIONS $* \
-		2>$tmp.mkfserr 1>$tmp.mkfsstd
-	;;
-    *)
-	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* \
-		2>$tmp.mkfserr 1>$tmp.mkfsstd
-	;;
-    esac
-
-    if [ $? -ne 0 ]; then
+    if ! _try_mkfs_dev "$@" 2>$tmp.mkfserr 1>$tmp.mkfsstd; then
 	# output stored mkfs output
 	cat $tmp.mkfserr >&2
 	cat $tmp.mkfsstd
diff --git a/tests/generic/500 b/tests/generic/500
index bc84d219fa..1151c8f234 100755
--- a/tests/generic/500
+++ b/tests/generic/500
@@ -58,7 +58,8 @@  CLUSTER_SIZE=$((64 * 1024 / 512))		# 64K
 
 _dmthin_init $BACKING_SIZE $VIRTUAL_SIZE $CLUSTER_SIZE 0
 _dmthin_set_fail
-_dmthin_mkfs
+_dmthin_try_mkfs >> $seqres.full 2>&1 || \
+	_notrun "Could not format small thinp filesystem for test"
 _dmthin_mount
 
 # There're two bugs at here, one is dm-thin bug, the other is filesystem