Re: [PATCH 1/4] common/rc: Introduce new helpers for DAX mount options and FS_XFLAG_DAX

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

On Wed, Jun 10, 2020 at 10:01:58AM +0800, Xiao Yang wrote:
> Hi Darrick,
> 
> Thanks a lot for your comments.
> 
> On 2020/6/10 0:39, Darrick J. Wong wrote:
> > Oh, this wasn't the cover letter. ;)
> 
> I will add the cover letter.
> 
> > 
> > On Wed, Jun 03, 2020 at 10:01:12PM +0800, Xiao Yang wrote:
> > > 1) _require_scratch_dax_mountopt() checks both old and new DAX mount option
> > > 2) _require_scratch_daX_iflag() checks FS_XFLAG_DAX
> > > 
> > > Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> > > ---
> > >   common/rc | 35 +++++++++++++++++++++++++++++++++++
> > >   1 file changed, 35 insertions(+)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index a6967831..ec7c19e4 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -3188,6 +3188,41 @@ _require_scratch_dax()
> > >   	_scratch_unmount
> > >   }
> > > 
> > > +_require_scratch_dax_mountopt()
> > > +{
> > > +	local mountopt=$1
> > > +	local output
> > > +
> > > +	_require_scratch
> > > +	_scratch_mkfs>  /dev/null 2>&1
> > > +	_try_scratch_mount -o "$mountopt" || \
> > > +		_notrun "mount $SCRATCH_DEV with $mountopt failed"
> > 
> > What happens if MOUNT_OPTS already contains a dax option?  Should we
> > clear it out ala _qmount_option, on the assumption that a test that
> > cares about specific options probably wants to override whatever the
> > test runner passed in?
> 
> Good point, but it seems that the last dax option is actually used if we
> mount with multiple dax options, as below:
> ----------------------------------------------
> ext4:
> # blkid /dev/pmem1
> /dev/pmem1: UUID="cd2eb9f0-af2a-4c89-a381-4d2d9d2e8054" TYPE="ext4"
> # mount -o dax -odax=inode /dev/pmem1 /mnt/xfstests/scratch/
> # mount | grep pmem1
> /dev/pmem1 on /mnt/xfstests/scratch type ext4
> (rw,relatime,seclabel,dax=inode)
> 
> # mount -o dax=never -odax=inode -odax=always /dev/pmem1
> /mnt/xfstests/scratch/
> # mount | grep pmem1
> /dev/pmem1 on /mnt/xfstests/scratch type ext4
> (rw,relatime,seclabel,dax=always)
> 
> # mount -o dax=never -odax /dev/pmem1 /mnt/xfstests/scratch/
> # mount | grep pmem1
> /dev/pmem1 on /mnt/xfstests/scratch type ext4
> (rw,relatime,seclabel,dax=always)
> 
> 
> xfs:
> # blkid /dev/pmem0
> /dev/pmem0: UUID="bc830790-1ea8-48fb-9cda-7d5bb96b8961" TYPE="xfs"
> # mount -o dax=never -o dax=always /dev/pmem0 /mnt/xfstests/test/
> # mount | grep pmem0
> /dev/pmem0 on /mnt/xfstests/test type xfs (rw,relatime,seclabel,attr2,dax=always,inode64,logbufs=8,logbsize=32k,noquota)
> 
> # mount -o dax=never -o dax=inode /dev/pmem0 /mnt/xfstests/test/
> # mount | grep pmem0
> /dev/pmem0 on /mnt/xfstests/test type xfs
> (rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota)
> 
> # mount -o dax=always -o dax=inode -o dax=never /dev/pmem0
> /mnt/xfstests/test/
> # mount | grep pmem0
> /dev/pmem0 on /mnt/xfstests/test type xfs
> (rw,relatime,seclabel,attr2,dax=never,inode64,logbufs=8,logbsize=32k,noquota)
> ----------------------------------------------
> all dax options are exclusive, so do you think is it necessary to implement
> a function as _qmount_option?

Heh, ok, I guess we don't complain about respecified mount options, so
(at least for fstests) the current behavior is ok.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> Best Regards,
> Xiao Yang
> > 
> > --D
> > 
> > > +
> > > +	output=$(_fs_options $SCRATCH_DEV)
> > > +
> > > +	# For new dax mount option, /proc/mounts shows different outputs if we
> > > +	# mount with -o dax=inode on ext4 and xfs so skip checking it.
> > > +	# /proc/mounts shows 'dax=inode' on ext4 but shows nothing on xfs.
> > > +	if [ "$mountopt" != "dax=inode" ]; then
> > > +		echo $output | grep -qw "$mountopt" || \
> > > +			_notrun "$SCRATCH_DEV $FSTYP does not support -o $mountopt"
> > > +	fi
> > > +
> > > +	# For new dax mount option, /proc/mounts shows "dax=never" if we
> > > +	# mount with -o dax on xfs and underlying device doesn't support dax.
> > > +	if [ "$mountopt" = "dax" ]; then
> > > +		echo $output | grep -qw "dax=never"&&  \
> > > +			_notrun "$SCRATCH_DEV $FSTYP does not support -o $mountopt"
> > > +	fi
> > > +
> > > +	_scratch_unmount
> > > +}
> > > +
> > > +_require_scratch_dax_iflag()
> > > +{
> > > +	_require_xfs_io_command "chattr" "x"
> > > +}
> > > +
> > >   # Does norecovery support by this fs?
> > >   _require_norecovery()
> > >   {
> > > --
> > > 2.21.0
> > > 
> > > 
> > > 
> > 
> > 
> > .
> > 
> 
> 
>