[2/4] xfs: rework min log size helper
diff mbox series

Message ID 156089203509.345809.3448903728041546348.stgit@magnolia
State New
Headers show
Series
  • fstests: various fixes
Related show

Commit Message

Darrick J. Wong June 18, 2019, 9:07 p.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

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@oracle.com>
---
 common/rc  |   13 ++++++++++---
 common/xfs |   23 +++++++++++++++++++++--
 2 files changed, 31 insertions(+), 5 deletions(-)

Comments

Eryu Guan June 21, 2019, 8:57 a.m. UTC | #1
On Tue, Jun 18, 2019 at 02:07:15PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 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@oracle.com>
> ---
>  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 :)

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

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()
>
Darrick J. Wong June 21, 2019, 7:02 p.m. UTC | #2
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@oracle.com>
> > 
> > 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@oracle.com>
> > ---
> >  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()
> >

Patch
diff mbox series

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
+}
+
 # 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
+	__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
 		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=$?
+	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()