Re: [PATCH v8 1/7] common/rc: Introduce helpers for new dax mount options and FS_XFLAG_DAX

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

On 2020/8/4 6:26, Darrick J. Wong wrote:
> On Mon, Aug 03, 2020 at 12:49:19PM -0700, Ira Weiny wrote:
>> On Mon, Aug 03, 2020 at 04:38:32PM +0800, Xiao Yang wrote:
>>> 1) _check_scratch_dax_mountopt() checks old/new dax mount option and
>>>     returns a value.
>>> 2) _require_scratch_dax_mountopt() throws notrun if _check_scratch_dax_mountopt()
>>>     returns a non-zero value.
>>> 3) _require_dax_iflag() checks FS_XFLAG_DAX.
>>>
>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>>> ---
>>>   common/rc | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 42 insertions(+)
>>>
>>> diff --git a/common/rc b/common/rc
>>> index 1b7b2575..d7926bc5 100644
>>> --- a/common/rc
>>> +++ b/common/rc
>>> @@ -3188,6 +3188,48 @@ _require_scratch_dax()
>>>   	_scratch_unmount
>>>   }
>>>
>>> +# Only accept dax/dax=always mount option becasue dax=always, dax=inode
>>> +# and dax=never are always introduced together.
>>> +# Return 0 if filesystem/device supports the specified dax option.
>>> +# Return 1 if mount fails with the specified dax option.
>>> +# Return 2 if /proc/mounts shows wrong dax option.
>>> +# Check new dax=inode, dax=always or dax=never option by passing "dax=always".
>>> +# Check old dax or new dax=always by passing "dax".
>>> +_check_scratch_dax_mountopt()
>> So I think I see what you are trying to say here but I'm not sure the
>> comment/doc is clear.  How about:
>>
>> # Check if dax mount options are supported
>> #
>> # $1 can be either 'dax=always' or 'dax'
>> #
>> # dax=always
>> #      Check for the new dax options (dax=inode, dax=always or dax=never) by
>> #      passing "dax=always".
>> # dax
>> #      Check for the old dax or new dax=always by passing "dax".
>> #
>> # This only accepts 'dax=always' because dax=always, dax=inode
>> # and dax=never are always supported together.  So if the other options are
>> # required checking for 'dax=always' indicates support for the other 2.
>> #
>> # Return 0 if filesystem/device supports the specified dax option.
>> # Return 1 if mount fails with the specified dax option.
>> # Return 2 if /proc/mounts shows wrong dax option.
> /me likes this better.
Hi Ira, Darrick

It is clearer and better, I will update the comment in v9 patch set. :-)
Besides,  could you help me review another new test which has been sent 
to mail list:
"generic: Verify how to change the S_DAX flag on an existing file"
I want to squash it into v9 patch set after you review it.

Thanks,
Xiao Yang
> --D
>
>> Ira
>>
>>> +{
>>> +	local option=$1
>>> +
>>> +	_require_scratch
>>> +	_scratch_mkfs>  /dev/null 2>&1
>>> +
>>> +	_try_scratch_mount "-o $option">  /dev/null 2>&1 || return 1
>>> +
>>> +	if _fs_options $SCRATCH_DEV | egrep -q "dax(=always|,|$)"; then
>>> +		_scratch_unmount
>>> +		return 0
>>> +	else
>>> +		_scratch_unmount
>>> +		return 2
>>> +	fi
>>> +}
>>> +
>>> +# Throw notrun if _check_scratch_dax_mountopt() returns a non-zero value.
>>> +_require_scratch_dax_mountopt()
>>> +{
>>> +	local mountopt=$1
>>> +
>>> +	_check_scratch_dax_mountopt "$mountopt"
>>> +	local res=$?
>>> +
>>> +	[ $res -eq 1 ]&&  _notrun "mount $SCRATCH_DEV with $mountopt failed"
>>> +	[ $res -eq 2 ]&&  _notrun "$SCRATCH_DEV $FSTYP does not support -o $mountopt"
>>> +}
>>> +
>>> +_require_dax_iflag()
>>> +{
>>> +	_require_xfs_io_command "chattr" "x"
>>> +}
>>> +
>>>   # Does norecovery support by this fs?
>>>   _require_norecovery()
>>>   {
>>> -- 
>>> 2.21.0
>>>
>>>
>>>
>
> .
>