Re: [PATCH 4/5] common/populate: enable xfs quota accounting

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Aug 04, 2017 at 10:22:00AM +0800, Eryu Guan wrote:
> On Fri, Jul 21, 2017 at 03:04:52PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > When we're creating a populated xfs image, turn on quotas so that we can
> > fuzz those fields too.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  common/populate |   50 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 49 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/common/populate b/common/populate
> > index 498151f..b77c508 100644
> > --- a/common/populate
> > +++ b/common/populate
> > @@ -21,6 +21,7 @@
> >  #  Contact information: Silicon Graphics, Inc., 1500 Crittenden Lane,
> >  #  Mountain View, CA 94043, USA, or: http://www.sgi.com
> >  #-----------------------------------------------------------------------
> > +. ./common/quota
> >  
> >  _require_populate_commands() {
> >  	_require_xfs_io_command "falloc"
> > @@ -94,6 +95,47 @@ __populate_fill_fs() {
> >  	done
> >  }
> >  
> > +# For XFS, force on all the quota options if quota is enabled
> > +# and the user didn't feed us noquota.
> > +_populate_xfs_qmount_option()
> > +{
> > +	# User explicitly told us not to quota
> > +	if echo "${MOUNT_OPTIONS}" | grep -q 'noquota'; then
> > +		return
> > +	fi
> > +
> > +	# Don't bother if we can't turn on quotas
> > +	if [ ! -f /proc/fs/xfs/xqmstat ]; then
> > +		# No quota support
> > +		return
> > +	elif [ "${USE_EXTERNAL}" = "yes" ] && [ ! -z "${SCRATCH_RTDEV}" ]; then
> > +		# Quotas not supported on rt filesystems
> > +		return
> > +	elif [ -z "${XFS_QUOTA_PROG}" ]; then
> > +		# xfs quota tools not installed
> > +		return
> > +	fi
> > +
> > +	# Turn on all the quotas
> > +	if xfs_info "${TEST_DIR}" | grep -q 'crc=1'; then
> > +		# v5 filesystems can have group & project quotas
> > +		quota="usrquota,grpquota,prjquota"
> > +	else
> > +		# v4 filesystems cannot mix group & project quotas
> > +		quota="usrquota,grpquota"
> > +	fi
> > +
> > +	# Inject our quota mount options
> > +	if echo "${MOUNT_OPTIONS}" | grep -q "${quota}"; then
> > +		return
> > +	elif echo "${MOUNT_OPTIONS}" | egrep -q '(quota|noenforce)'; then
> > +		_qmount_option "${quota}"
> > +	else
> > +		export MOUNT_OPTIONS="$MOUNT_OPTIONS -o ${quota}"
> > +		echo "MOUNT_OPTIONS = $MOUNT_OPTIONS" >>$seqres.full
> > +	fi
> > +}
> > +
> >  # Populate an XFS on the scratch device with (we hope) all known
> >  # types of metadata block
> >  _scratch_xfs_populate() {
> > @@ -106,6 +148,7 @@ _scratch_xfs_populate() {
> >  		esac
> >  	done
> >  
> > +	_populate_xfs_qmount_option
> 
> Sorry, another question just poped up..
> 
> This enables quota and modifies MOUNT_OPTIONS unconditionally and
> implicitly, I suspect users of _scratch_populate_cached() or
> _scratch_xfs_populate() may be bitten by this implicit change.

I suppose we could be more explicit about the fact that we
unconditionally enable all quota types and change MOUNT_OPTIONS, though
I did go run all the _scratch_xfs_populate tests and they all seemed
fine.

(Well, as fine as we can get considering that most of them are fuzz
tests that cause all manners of other crazy behavior.)

> Would it be better if there's a argument to control this quota
> enablement and default to disable quota? Just like the "nofill"
> argument, introduce a new "quota" argument? So existing callers of
> _scratch_populate_cached() are not affected by this change, and tests
> want quota can enable it explicitly.

Hmm.  The way I look at it, though, _scratch_xfs_populate is supposed to
create all types of filesystem metadata, which includes quota files, so
we have to enable the quota options or we're not actually doing what the
documentation says it's supposed to be doing:

# Populate an XFS on the scratch device with (we hope) all known
# types of metadata block

The reason there's a 'nofill' option is that, having created an fs with
all known types of metadata, we can optionally use up any remaining free
space, but doing so isn't necessary to create all known types of
metadata block.

--D

> 
> Thanks,
> Eryu
> 
> >  	_scratch_mount
> >  	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> >  	dblksz="$(xfs_info "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> > @@ -720,7 +763,12 @@ _scratch_populate_cached() {
> >  		test -n "${SCRATCH_LOGDEV}" && rm -f "${POPULATE_METADUMP}"
> >  		;;
> >  	"xfs")
> > -		extra_descr="LOGDEV ${SCRATCH_LOGDEV} USE_EXTERNAL ${USE_EXTERNAL} RTDEV ${SCRATCH_RTDEV}";;
> > +		extra_descr="LOGDEV ${SCRATCH_LOGDEV} USE_EXTERNAL ${USE_EXTERNAL} RTDEV ${SCRATCH_RTDEV}"
> > +		_populate_xfs_qmount_option
> > +		if echo "${MOUNT_OPTIONS}" | grep -q 'usrquota'; then
> > +			extra_descr="${extra_descr} QUOTAS"
> > +		fi
> > +		;;
> >  	*)
> >  		extra_descr="";;
> >  	esac
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html