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
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
