common/rc: teach _scratch_mkfs to handle mkfs option conflicts

PATCH Superseded Eryu Guan 2 participants 3 messages Nov 24, 2016 – Nov 25, 2016

Share & tools

Eryu Guan

author

2016-11-24 16:06 UTC #0 lore raw

PATCH 1/1 common/rc: teach _scratch_mkfs to handle mkfs option conflicts

Currently in _scratch_mkfs only xfs and ext4 could handle the mkfs failure caused by conflicts between $MKFS_OPTIONS and mkfs options specified by tests, because of _scratch_mkfs_xfs and _scratch_mkfs_ext4. This is a very useful functionality that allows tests to specify mkfs options safely and to test specific fs configurations, without worrying about mkfs failures caused by these options.

Now teach _scratch_mkfs to handle such mkfs option conflicts for other filesystems too, i.e. mkfs again only with mkfs options specified by tests. Also add the ability to filter unnecessary messages from mkfs stderr.

Signed-off-by Eryu Guan <[email protected]>

---
 common/rc | 141 ++++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 96 insertions(+), 45 deletions(-)

common/rc +96 ~~-45~~

index 8c99306..5336524 100644
@@ -782,51 +782,102 @@ _scratch_cleanup_files()
 
 _scratch_mkfs()
 {
-    case $FSTYP in
-    xfs)
-        _scratch_mkfs_xfs $*
-	;;
-    nfs*)
-	# unable to re-create NFS, just remove all files in $SCRATCH_MNT to
-	# avoid EEXIST caused by the leftover files created in previous runs
-        _scratch_cleanup_files
-	;;
-    cifs)
-	# unable to re-create CIFS, just remove all files in $SCRATCH_MNT to
-	# avoid EEXIST caused by the leftover files created in previous runs
-        _scratch_cleanup_files
-	;;
-    ceph)
-	# Don't re-create CephFS, just remove all files
-	_scratch_cleanup_files
-	;;
-    overlay)
-	# unable to re-create overlay, remove all files in $SCRATCH_MNT to
-	# avoid EEXIST caused by the leftover files created in previous runs
-        _scratch_cleanup_files
-	;;
-    udf)
-        $MKFS_UDF_PROG $MKFS_OPTIONS $* $SCRATCH_DEV > /dev/null
-	;;
-    btrfs)
-        $MKFS_BTRFS_PROG $MKFS_OPTIONS $* $SCRATCH_DEV > /dev/null
-	;;
-    ext2|ext3)
-	$MKFS_PROG -t $FSTYP -- -F $MKFS_OPTIONS $* $SCRATCH_DEV
-	;;
-    ext4)
-	_scratch_mkfs_ext4 $*
-	;;
-    tmpfs)
-	# do nothing for tmpfs
-	;;
-    f2fs)
-        $MKFS_F2FS_PROG $MKFS_OPTIONS $* $SCRATCH_DEV > /dev/null
-	;;
-    *)
-	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* $SCRATCH_DEV
-	;;
-    esac
+	local extra_mkfs_options=$*
+	local mkfs_cmd=""
+	local mkfs_filter=""
+	local mkfs_status
+
+	case $FSTYP in
+	xfs)
+		_scratch_mkfs_xfs $extra_mkfs_options
+		;;
+	nfs*)
+		# unable to re-create NFS, just remove all files in
+		# $SCRATCH_MNT to avoid EEXIST caused by the leftover files
+		# created in previous runs
+		_scratch_cleanup_files
+		;;
+	cifs)
+		# unable to re-create CIFS, just remove all files in
+		# $SCRATCH_MNT to avoid EEXIST caused by the leftover files
+		# created in previous runs
+		_scratch_cleanup_files
+		;;
+	ceph)
+		# Don't re-create CephFS, just remove all files
+		_scratch_cleanup_files
+		;;
+	overlay)
+		# unable to re-create overlay, remove all files in $SCRATCH_MNT
+		# to avoid EEXIST caused by the leftover files created in
+		# previous runs
+		_scratch_cleanup_files
+		;;
+	tmpfs)
+		# do nothing for tmpfs
+		;;
+	ext4)
+		_scratch_mkfs_ext4 $extra_mkfs_options
+		;;
+	udf)
+		mkfs_cmd="$MKFS_UDF_PROG"
+		mkfs_filter="cat"
+		;;
+	btrfs)
+		mkfs_cmd="$MKFS_BTRFS_PROG"
+		mkfs_filter="cat"
+		;;
+	ext2|ext3)
+		mkfs_cmd="$MKFS_PROG -t $FSTYP -- -F"
+		mkfs_filter="grep -v -e ^Warning: -e \"^mke2fs \""
+		;;
+	f2fs)
+		mkfs_cmd="$MKFS_F2FS_PROG"
+		mkfs_filter="cat"
+		;;
+	ocfs2)
+		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
+		mkfs_filter="grep -v -e ^mkfs\.ocfs2"
+		;;
+	*)
+		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
+		mkfs_filter="cat"
+		;;
+	esac
+	mkfs_status=$?
+
+	# return immediately if FSTYP is handled by dedicated helpers
+	if [ -z "$mkfs_cmd" ]; then
+		return $mkfs_status
+	fi
+
+	# save mkfs output in case conflict means we need to run again.
+	# only the output for the mkfs that applies should be shown
+	eval "$mkfs_cmd $MKFS_OPTIONS $extra_mkfs_options $SCRATCH_DEV" \
+		2>$tmp.mkfserr 1>$tmp.mkfsstd
+	mkfs_status=$?
+
+	# a mkfs failure may be caused by conflicts between $MKFS_OPTIONS and
+	# $extra_mkfs_options
+	if [ $mkfs_status -ne 0 -a -n "$extra_mkfs_options" ]; then
+		(
+		echo -n "** mkfs failed with extra mkfs options "
+		echo "added to \"$MKFS_OPTIONS\" by test $seq **"
+		echo -n "** attempting to mkfs using only test $seq "
+		echo "options: $extra_mkfs_options **"
+		) >> $seqres.full
+
+		# running mkfs again. overwrite previous mkfs output files
+		eval "$mkfs_cmd $extra_mkfs_options $SCRATCH_DEV" \
+			2>$tmp.mkfserr 1>$tmp.mkfsstd
+		mkfs_status=$?
+	fi
+
+	# output stored mkfs output, filtering unnecessary output from stderr
+	cat $tmp.mkfsstd
+	cat $tmp.mkfserr | $mkfs_filter >&2
+
+	return $mkfs_status
 }
 
 # Helper function to get a spare or replace-target device from

Signature · 1 line

2.9.3

Dave Chinner

2016-11-24 20:00 UTC #1 lore raw

Quoted 8 lines · Eryu Guan

Currently in _scratch_mkfs only xfs and ext4 could handle the mkfs failure caused by conflicts between $MKFS_OPTIONS and mkfs options specified by tests, because of _scratch_mkfs_xfs and _scratch_mkfs_ext4. This is a very useful functionality that allows tests to specify mkfs options safely and to test specific fs configurations, without worrying about mkfs failures caused by these options.

Quoted 4 lines

Now teach _scratch_mkfs to handle such mkfs option conflicts for other filesystems too, i.e. mkfs again only with mkfs options specified by tests. Also add the ability to filter unnecessary messages from mkfs stderr.

Nice!

.....

Quoted 53 lines

common/rc +53 -0

+	local extra_mkfs_options=$*
+	local mkfs_cmd=""
+	local mkfs_filter=""
+	local mkfs_status
+
+	case $FSTYP in
+	xfs)
+		_scratch_mkfs_xfs $extra_mkfs_options
+		;;
+	nfs*)
+		# unable to re-create NFS, just remove all files in
+		# $SCRATCH_MNT to avoid EEXIST caused by the leftover files
+		# created in previous runs
+		_scratch_cleanup_files
+		;;
+	cifs)
+		# unable to re-create CIFS, just remove all files in
+		# $SCRATCH_MNT to avoid EEXIST caused by the leftover files
+		# created in previous runs
+		_scratch_cleanup_files
+		;;
+	ceph)
+		# Don't re-create CephFS, just remove all files
+		_scratch_cleanup_files
+		;;
+	overlay)
+		# unable to re-create overlay, remove all files in $SCRATCH_MNT
+		# to avoid EEXIST caused by the leftover files created in
+		# previous runs
+		_scratch_cleanup_files
+		;;
+	tmpfs)
+		# do nothing for tmpfs
+		;;
+	ext4)
+		_scratch_mkfs_ext4 $extra_mkfs_options
+		;;
+	udf)
+		mkfs_cmd="$MKFS_UDF_PROG"
+		mkfs_filter="cat"
+		;;
+	btrfs)
+		mkfs_cmd="$MKFS_BTRFS_PROG"
+		mkfs_filter="cat"
+		;;
+	ext2|ext3)
+		mkfs_cmd="$MKFS_PROG -t $FSTYP -- -F"
+		mkfs_filter="grep -v -e ^Warning: -e \"^mke2fs \""
+		;;
+	f2fs)
+		mkfs_cmd="$MKFS_F2FS_PROG"
+		mkfs_filter="cat"
+		;;

common/rc _scratch_cleanup_files() · line 847

Quoted 10 lines

common/rc +10 -0

+	ocfs2)
+		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
+		mkfs_filter="grep -v -e ^mkfs\.ocfs2"
+		;;
+	*)
+		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
+		mkfs_filter="cat"
+		;;
+	esac
+	mkfs_status=$?

I suspect that $? can be undefined at this point - it's value is set by whatever the last command was run, and not all the cases above run a command. This might be better handled by something like:

	case $FSTYP in
	nfs*|cifs|ceph|overlay)
		# unable to re-create this fstyp, just remove all files in
		# $SCRATCH_MNT to avoid EEXIST caused by the leftover files
		# created in previous runs
		_scratch_cleanup_files
		return 0
		;;
	tmpfs)
		# do nothing
		return 0
		;;
	ext4)
		_scratch_mkfs_ext4 $extra_mkfs_options
		return $?
		;;
	xfs)
		_scratch_mkfs_xfs $extra_mkfs_options
		return $?
		;;
	udf)
		mkfs_cmd="$MKFS_UDF_PROG"
		mkfs_filter="cat"
		;;
	.....

common/rc _scratch_cleanup_files() · line 852

Quoted 5 lines

common/rc +5 -0

+
+	# return immediately if FSTYP is handled by dedicated helpers
+	if [ -z "$mkfs_cmd" ]; then
+		return $mkfs_status
+	fi

And then this can go as well.

Quoted 16 lines

common/rc +16 -0

+
+	# save mkfs output in case conflict means we need to run again.
+	# only the output for the mkfs that applies should be shown
+	eval "$mkfs_cmd $MKFS_OPTIONS $extra_mkfs_options $SCRATCH_DEV" \
+		2>$tmp.mkfserr 1>$tmp.mkfsstd
+	mkfs_status=$?
+
+	# a mkfs failure may be caused by conflicts between $MKFS_OPTIONS and
+	# $extra_mkfs_options
+	if [ $mkfs_status -ne 0 -a -n "$extra_mkfs_options" ]; then
+		(
+		echo -n "** mkfs failed with extra mkfs options "
+		echo "added to \"$MKFS_OPTIONS\" by test $seq **"
+		echo -n "** attempting to mkfs using only test $seq "
+		echo "options: $extra_mkfs_options **"
+		) >> $seqres.full

common/rc _scratch_cleanup_files() · line 878

Quoted 10 lines

common/rc +10 -0

+
+		# running mkfs again. overwrite previous mkfs output files
+		eval "$mkfs_cmd $extra_mkfs_options $SCRATCH_DEV" \
+			2>$tmp.mkfserr 1>$tmp.mkfsstd
+		mkfs_status=$?
+	fi
+
+	# output stored mkfs output, filtering unnecessary output from stderr
+	cat $tmp.mkfsstd
+	cat $tmp.mkfserr | $mkfs_filter >&2

Perhaps you could make this a function? Because then it can probably be used in _scratch_mkfs_ext4 and _scratch_mkfs_xfs as well?

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Eryu Guan

author

2016-11-25 11:30 UTC #2 lore raw

Quoted 76 lines · Dave Chinner

Quoted 8 lines · Eryu Guan
Currently in _scratch_mkfs only xfs and ext4 could handle the mkfs failure caused by conflicts between $MKFS_OPTIONS and mkfs options specified by tests, because of _scratch_mkfs_xfs and _scratch_mkfs_ext4. This is a very useful functionality that allows tests to specify mkfs options safely and to test specific fs configurations, without worrying about mkfs failures caused by these options.
Quoted 4 lines
Now teach _scratch_mkfs to handle such mkfs option conflicts for other filesystems too, i.e. mkfs again only with mkfs options specified by tests. Also add the ability to filter unnecessary messages from mkfs stderr.
Nice!
.....
Quoted 59 lines
common/rc +59 -0
+	local extra_mkfs_options=$*
+	local mkfs_cmd=""
+	local mkfs_filter=""
+	local mkfs_status
+
+	case $FSTYP in
+	xfs)
+		_scratch_mkfs_xfs $extra_mkfs_options
+		;;
+	nfs*)
+		# unable to re-create NFS, just remove all files in
+		# $SCRATCH_MNT to avoid EEXIST caused by the leftover files
+		# created in previous runs
+		_scratch_cleanup_files
+		;;
+	cifs)
+		# unable to re-create CIFS, just remove all files in
+		# $SCRATCH_MNT to avoid EEXIST caused by the leftover files
+		# created in previous runs
+		_scratch_cleanup_files
+		;;
+	ceph)
+		# Don't re-create CephFS, just remove all files
+		_scratch_cleanup_files
+		;;
+	overlay)
+		# unable to re-create overlay, remove all files in $SCRATCH_MNT
+		# to avoid EEXIST caused by the leftover files created in
+		# previous runs
+		_scratch_cleanup_files
+		;;
+	tmpfs)
+		# do nothing for tmpfs
+		;;
+	ext4)
+		_scratch_mkfs_ext4 $extra_mkfs_options
+		;;
+	udf)
+		mkfs_cmd="$MKFS_UDF_PROG"
+		mkfs_filter="cat"
+		;;
+	btrfs)
+		mkfs_cmd="$MKFS_BTRFS_PROG"
+		mkfs_filter="cat"
+		;;
+	ext2|ext3)
+		mkfs_cmd="$MKFS_PROG -t $FSTYP -- -F"
+		mkfs_filter="grep -v -e ^Warning: -e \"^mke2fs \""
+		;;
+	f2fs)
+		mkfs_cmd="$MKFS_F2FS_PROG"
+		mkfs_filter="cat"
+		;;
+	ocfs2)
+		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
+		mkfs_filter="grep -v -e ^mkfs\.ocfs2"
+		;;
+	*)
+		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"

Quoted 8 lines

common/rc _scratch_cleanup_files() · line 847
Quoted 4 lines
common/rc +4 -0
+		mkfs_filter="cat"
+		;;
+	esac
+	mkfs_status=$?
I suspect that $? can be undefined at this point - it's value is set by whatever the last command was run, and not all the cases above run a command. This might be better handled by something like:

I did spend some time on the return value handling, because I didn't want to do so many "return"s, and I confirmed that assigning a value does reset $? to 0 too, so mkfs_status is always defined.

Quoted 9 lines

case $FSTYP in
nfs*|cifs|ceph|overlay)
# unable to re-create this fstyp, just remove all files in # $SCRATCH_MNT to avoid EEXIST caused by the leftover files # created in previous runs _scratch_cleanup_files
return 0
;;

But apparently I forgot that I can group these cases together, this saves us three "return 0"s, and the logic is easier to understand, I'll take this approach.

Quoted 46 lines

	tmpfs)
		# do nothing
		return 0
		;;
	ext4)
		_scratch_mkfs_ext4 $extra_mkfs_options
		return $?
		;;
	xfs)
		_scratch_mkfs_xfs $extra_mkfs_options
		return $?
		;;
	udf)
		mkfs_cmd="$MKFS_UDF_PROG"
		mkfs_filter="cat"
		;;
	.....

common/rc _scratch_cleanup_files() · line 852

Quoted 5 lines

common/rc +5 -0

+
+	# return immediately if FSTYP is handled by dedicated helpers
+	if [ -z "$mkfs_cmd" ]; then
+		return $mkfs_status
+	fi

And then this can go as well.

Quoted 21 lines

common/rc +21 -0

+
+	# save mkfs output in case conflict means we need to run again.
+	# only the output for the mkfs that applies should be shown
+	eval "$mkfs_cmd $MKFS_OPTIONS $extra_mkfs_options $SCRATCH_DEV" \
+		2>$tmp.mkfserr 1>$tmp.mkfsstd
+	mkfs_status=$?
+
+	# a mkfs failure may be caused by conflicts between $MKFS_OPTIONS and
+	# $extra_mkfs_options
+	if [ $mkfs_status -ne 0 -a -n "$extra_mkfs_options" ]; then
+		(
+		echo -n "** mkfs failed with extra mkfs options "
+		echo "added to \"$MKFS_OPTIONS\" by test $seq **"
+		echo -n "** attempting to mkfs using only test $seq "
+		echo "options: $extra_mkfs_options **"
+		) >> $seqres.full
+
+		# running mkfs again. overwrite previous mkfs output files
+		eval "$mkfs_cmd $extra_mkfs_options $SCRATCH_DEV" \
+			2>$tmp.mkfserr 1>$tmp.mkfsstd
+		mkfs_status=$?

Quoted 8 lines

common/rc _scratch_cleanup_files() · line 878
Quoted 5 lines
common/rc +5 -0
+	fi
+
+	# output stored mkfs output, filtering unnecessary output from stderr
+	cat $tmp.mkfsstd
+	cat $tmp.mkfserr | $mkfs_filter >&2
Perhaps you could make this a function? Because then it can probably be used in _scratch_mkfs_ext4 and _scratch_mkfs_xfs as well?

I thought about it too, but I noticed that there're some codes to setup large fs in _scratch_mkfs_{xfs,ext4}, so I didn't dig into them.

But now I think it actually can be done, and I'm testing an updated patch, will send v2 for review later, after the new patch passes my testing.

Thanks for the review!

Eryu