Re: [PATCH] common/rc: teach _scratch_mkfs to handle mkfs option conflicts

[Eryu Guan <eguan@redhat.com>]

On Fri, Nov 25, 2016 at 07:00:13AM +1100, Dave Chinner wrote:
> On Fri, Nov 25, 2016 at 12:06:43AM +0800, Eryu Guan wrote:
> > 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.
> 
> Nice!
> 
> .....
> > +	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=$?
> 
> 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.

> 
> 	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.

> 	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"
> 		;;
> 	.....
> > +
> > +	# 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.
> 
> > +
> > +	# 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
> 
> 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