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

Shiyang Ruan
2022-08-04T01:37:01Z

在 2022/8/4 8:51, Darrick J. Wong 写道:
> On Wed, Aug 03, 2022 at 06:47:24AM +0000, ruansy.fnst@fujitsu.com wrote:
>>
>>
>> 在 2022/7/29 12:54, Darrick J. Wong 写道:
>>> On Fri, Jul 29, 2022 at 03:55:24AM +0000, ruansy.fnst@fujitsu.com wrote:
>>>>
>>>>
>>>> 在 2022/7/22 0:16, Darrick J. Wong 写道:
>>>>> On Thu, Jul 21, 2022 at 02:06:10PM +0000, ruansy.fnst@fujitsu.com 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@fujitsu.com>
>>>>>>>
>>>>>>> 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.
>
> dm devices used to have fsdax support; I think Christoph is actively
> removing (or already has removed) all that support.
>
>> In addition, is there any other testcase has the same problem? so that
>> we can deal with them together.
>
> The last I checked, there aren't any that require MAP_SYNC or pmem aside
> from g/470 and the three poison notification tests that you sent a few
> days ago.

Ok. Got it. Thank you!

--
Ruan.

>
> --D
>
>>
>> --
>> Thanks,
>> Ruan
>>
>>
>>>
>>> --D
>>>
>>>>
>>>> --
>>>> Thanks,
>>>> Ruan.
>>>>
>>>>
>>>>
>>>>>
>>>>> I'm not sure how to fix that...
>>>>>
>>>>> --D
>>>>>
>>>>>>
>>>>>> --
>>>>>> Thanks,
>>>>>> Ruan.
>>>>>>
>>>>>>>
>>>>>>> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>>>>>>>
>>>>>>> --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
>>>>>>>>
>>>>>>>>
>>>>>>>>