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