Re: [PATCH] xfs: fail dax mount if reflink is enabled on a partition

[ruansy.fnst@xxxxxxxxxxx]

在 2022/7/29 12:54, Darrick J. Wong 写道:
> On Fri, Jul 29, 2022 at 03:55:24AM +0000, ruansy.fnst@xxxxxxxxxxx wrote:
>>
>>
>> 在 2022/7/22 0:16, Darrick J. Wong 写道:
>>> On Thu, Jul 21, 2022 at 02:06:10PM +0000, ruansy.fnst@xxxxxxxxxxx wrote:
>>>> 在 2022/7/1 8:31, Darrick J. Wong 写道:
>>>>> On Thu, Jun 09, 2022 at 10:34:35PM +0800, Shiyang Ruan wrote:
>>>>>> Failure notification is not supported on partitions.  So, when we mount
>>>>>> a reflink enabled xfs on a partition with dax option, let it fail with
>>>>>> -EINVAL code.
>>>>>>
>>>>>> Signed-off-by: Shiyang Ruan <ruansy.fnst@xxxxxxxxxxx>
>>>>>
>>>>> Looks good to me, though I think this patch applies to ... wherever all
>>>>> those rmap+reflink+dax patches went.  I think that's akpm's tree, right?
>>>>>
>>>>> Ideally this would go in through there to keep the pieces together, but
>>>>> I don't mind tossing this in at the end of the 5.20 merge window if akpm
>>>>> is unwilling.
>>>>
>>>> BTW, since these patches (dax&reflink&rmap + THIS + pmem-unbind) are
>>>> waiting to be merged, is it time to think about "removing the
>>>> experimental tag" again?  :)
>>>
>>> It's probably time to take up that question again.
>>>
>>> Yesterday I tried running generic/470 (aka the MAP_SYNC test) and it
>>> didn't succeed because it sets up dmlogwrites atop dmthinp atop pmem,
>>> and at least one of those dm layers no longer allows fsdax pass-through,
>>> so XFS silently turned mount -o dax into -o dax=never. :(
>>
>> Hi Darrick,
>>
>> I tried generic/470 but it didn't run:
>>     [not run] Cannot use thin-pool devices on DAX capable block devices.
>>
>> Did you modify the _require_dm_target() in common/rc?  I added thin-pool
>> to not to check dax capability:
>>
>>           case $target in
>>           stripe|linear|log-writes|thin-pool)  # add thin-pool here
>>                   ;;
>>
>> then the case finally ran and it silently turned off dax as you said.
>>
>> Are the steps for reproduction correct? If so, I will continue to
>> investigate this problem.
> 
> Ah, yes, I did add thin-pool to that case statement.  Sorry I forgot to
> mention that.  I suspect that the removal of dm support for pmem is
> going to force us to completely redesign this test.  I can't really
> think of how, though, since there's no good way that I know of to gain a
> point-in-time snapshot of a pmem device.

Hi Darrick,

 > removal of dm support for pmem
I think here we are saying about xfstest who removed the support, not 
kernel?

I found some xfstests commits:
fc7b3903894a6213c765d64df91847f4460336a2  # common/rc: add the restriction.
fc5870da485aec0f9196a0f2bed32f73f6b2c664  # generic/470: use thin-pool

So, this case was never able to run since the second commit?  (I didn't 
notice the not run case.  I thought it was expected to be not run.)

And according to the first commit, the restriction was added because 
some of dm devices don't support dax.  So my understanding is: we should 
redesign the case to make the it work, and firstly, we should add dax 
support for dm devices in kernel.


In addition, is there any other testcase has the same problem?  so that 
we can deal with them together.


--
Thanks,
Ruan


> 
> --D
> 
>>
>> --
>> Thanks,
>> Ruan.
>>
>>
>>
>>>
>>> I'm not sure how to fix that...
>>>
>>> --D
>>>
>>>>
>>>> --
>>>> Thanks,
>>>> Ruan.
>>>>
>>>>>
>>>>> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
>>>>>
>>>>> --D
>>>>>
>>>>>> ---
>>>>>>     fs/xfs/xfs_super.c | 6 ++++--
>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>>>>>> index 8495ef076ffc..a3c221841fa6 100644
>>>>>> --- a/fs/xfs/xfs_super.c
>>>>>> +++ b/fs/xfs/xfs_super.c
>>>>>> @@ -348,8 +348,10 @@ xfs_setup_dax_always(
>>>>>>     		goto disable_dax;
>>>>>>     	}
>>>>>>     
>>>>>> -	if (xfs_has_reflink(mp)) {
>>>>>> -		xfs_alert(mp, "DAX and reflink cannot be used together!");
>>>>>> +	if (xfs_has_reflink(mp) &&
>>>>>> +	    bdev_is_partition(mp->m_ddev_targp->bt_bdev)) {
>>>>>> +		xfs_alert(mp,
>>>>>> +			"DAX and reflink cannot work with multi-partitions!");
>>>>>>     		return -EINVAL;
>>>>>>     	}
>>>>>>     
>>>>>> -- 
>>>>>> 2.36.1
>>>>>>
>>>>>>
>>>>>>