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@xxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
--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
> > >
> > >
> > >
> >
> >
> > .
> >
>
>
>
