On Fri, Jun 21, 2019 at 04:57:48PM +0800, Eryu Guan wrote:
> On Tue, Jun 18, 2019 at 02:07:15PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > The recent _scratch_find_xfs_min_logblocks helper has a major thinko in
> > it -- it relies on feeding a too-small size to _scratch_do_mkfs so that
> > mkfs will tell us the minimum log size. Unfortunately, _scratch_do_mkfs
> > will see that first failure and retry the mkfs without MKFS_OPTIONS,
> > which means that we return the minimum log size for the default mkfs
> > settings without MKFS_OPTIONS.
> >
> > This is a problem if someone's running fstests with a set of
> > MKFS_OPTIONS that affects the minimum log size. To fix this, open-code
> > the _scratch_do_mkfs retry behavior so that we only do the "retry
> > without MKFS_OPTIONS" behavior if the mkfs failed for a reason other
> > than the minimum log size check.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > common/rc | 13 ++++++++++---
> > common/xfs | 23 +++++++++++++++++++++--
> > 2 files changed, 31 insertions(+), 5 deletions(-)
> >
> >
> > diff --git a/common/rc b/common/rc
> > index 25203bb4..a38b7f02 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -438,6 +438,14 @@ _scratch_mkfs_options()
> > echo $SCRATCH_OPTIONS $MKFS_OPTIONS $* $SCRATCH_DEV
> > }
> >
> > +# Format the scratch device directly. First argument is the mkfs command.
> > +# Second argument are all the parameters. stdout goes to $tmp.mkfsstd and
> > +# stderr goes to $tmp.mkfserr.
> > +__scratch_do_mkfs()
> > +{
> > + eval "$1 $2 $SCRATCH_DEV" 2>$tmp.mkfserr 1>$tmp.mkfsstd
>
> I'd prefer leaving stdout and stderr to caller to handle, because ..
>
>
> > +}
> > +
> > # Do the actual mkfs work on SCRATCH_DEV. Firstly mkfs with both MKFS_OPTIONS
> > # and user specified mkfs options, if that fails (due to conflicts between mkfs
> > # options), do a second mkfs with only user provided mkfs options.
> > @@ -456,8 +464,7 @@ _scratch_do_mkfs()
> >
> > # 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
>
> it's easier to know the $tmp.mkfserr and $tmp.mkfsstd files should be
> cleaned up, otherwise it's not that clear where these files come from.
>
> > + __scratch_do_mkfs "$mkfs_cmd" "$MKFS_OPTIONS $extra_mkfs_options"
> > mkfs_status=$?
> >
> > # a mkfs failure may be caused by conflicts between $MKFS_OPTIONS and
> > @@ -471,7 +478,7 @@ _scratch_do_mkfs()
> > ) >> $seqres.full
> >
> > # running mkfs again. overwrite previous mkfs output files
> > - eval "$mkfs_cmd $extra_mkfs_options $SCRATCH_DEV" \
> > + __scratch_do_mkfs "$mkfs_cmd" "$extra_mkfs_options" \
> > 2>$tmp.mkfserr 1>$tmp.mkfsstd
>
> With the implemention in the patch, the "2>$tmp.mkfserr 1>$tmp.mkfsstd"
> part should be removed too, but with the suggested implemention we can
> keep it :)
Ok, will change this.
> > mkfs_status=$?
> > fi
> > diff --git a/common/xfs b/common/xfs
> > index f8dafc6c..8733e2ae 100644
> > --- a/common/xfs
> > +++ b/common/xfs
> > @@ -87,16 +87,33 @@ _scratch_find_xfs_min_logblocks()
> > # minimum log size.
> > local XFS_MIN_LOG_BYTES=2097152
> >
> > - _scratch_do_mkfs "$mkfs_cmd" "cat" $* -N -l size=$XFS_MIN_LOG_BYTES \
> > - 2>$tmp.mkfserr 1>$tmp.mkfsstd
> > + # Try formatting the filesystem with all the options given and the
> > + # minimum log size. We hope either that this succeeds or that mkfs
> > + # tells us the required minimum log size for the feature set.
> > + #
> > + # We cannot use _scratch_do_mkfs because it will retry /any/ failed
> > + # mkfs with MKFS_OPTIONS removed even if the only "failure" was that
> > + # the log was too small.
> > + local extra_mkfs_options="$* -N -l size=$XFS_MIN_LOG_BYTES"
> > + __scratch_do_mkfs "$mkfs_cmd" "$MKFS_OPTIONS $extra_mkfs_options"
> > local mkfs_status=$?
> >
> > + # If the format fails for a reason other than the log being too small,
> > + # try again without MKFS_OPTIONS because that's what _scratch_do_mkfs
> > + # will do if we pass in the log size option.
> > + if [ $mkfs_status -ne 0 ] &&
> > + ! grep -q 'log size.*too small, minimum' $tmp.mkfserr; then
> > + __scratch_do_mkfs "$mkfs_cmd" "$extra_mkfs_options"
> > + local mkfs_status=$?
>
> We've already declared mkfs_status as local, no need to do it again
> here.
Will fix.
--D
> Thanks,
> Eryu
>
> > + fi
> > +
> > # mkfs suceeded, so we must pick out the log block size to do the
> > # unit conversion
> > if [ $mkfs_status -eq 0 ]; then
> > local blksz="$(grep '^log.*bsize' $tmp.mkfsstd | \
> > sed -e 's/log.*bsize=\([0-9]*\).*$/\1/g')"
> > echo $((XFS_MIN_LOG_BYTES / blksz))
> > + rm -f $tmp.mkfsstd $tmp.mkfserr
> > return
> > fi
> >
> > @@ -104,6 +121,7 @@ _scratch_find_xfs_min_logblocks()
> > if grep -q 'minimum size is' $tmp.mkfserr; then
> > grep 'minimum size is' $tmp.mkfserr | \
> > sed -e 's/^.*minimum size is \([0-9]*\) blocks/\1/g'
> > + rm -f $tmp.mkfsstd $tmp.mkfserr
> > return
> > fi
> >
> > @@ -111,6 +129,7 @@ _scratch_find_xfs_min_logblocks()
> > echo "Cannot determine minimum log size" >&2
> > cat $tmp.mkfsstd >> $seqres.full
> > cat $tmp.mkfserr >> $seqres.full
> > + rm -f $tmp.mkfsstd $tmp.mkfserr
> > }
> >
> > _scratch_mkfs_xfs()
> >
