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

[Xiao Yang <yangx.jy@cn.fujitsu.com>]

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?

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