Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bf1ef4da-de16-c6bb-7ef5-374c6ed197e2@fujitsu.com>
Date:   Thu, 1 Dec 2022 23:39:12 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH 0/2] fsdax,xfs: fix warning messages
To:     "Darrick J. Wong" <djwong@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>
CC:     <linux-kernel@vger.kernel.org>, <linux-xfs@vger.kernel.org>,
        <nvdimm@lists.linux.dev>, <linux-fsdevel@vger.kernel.org>,
        <david@fromorbit.com>, <akpm@linux-foundation.org>
References: <1669301694-16-1-git-send-email-ruansy.fnst@fujitsu.com>
 <6386d512ce3fc_c9572944e@dwillia2-mobl3.amr.corp.intel.com.notmuch>
 <Y4bZGvP8Ozp+4De/@magnolia>
 <638700ba5db1_c95729435@dwillia2-mobl3.amr.corp.intel.com.notmuch>
 <Y4fGRurfXoFSBqMB@magnolia>
From:   Shiyang Ruan <ruansy.fnst@fujitsu.com>
In-Reply-To: <Y4fGRurfXoFSBqMB@magnolia>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 2022/12/1 5:08, Darrick J. Wong 写道:
> On Tue, Nov 29, 2022 at 11:05:30PM -0800, Dan Williams wrote:
>> Darrick J. Wong wrote:
>>> On Tue, Nov 29, 2022 at 07:59:14PM -0800, Dan Williams wrote:
>>>> [ add Andrew ]
>>>>
>>>> Shiyang Ruan wrote:
>>>>> Many testcases failed in dax+reflink mode with warning message in dmesg.
>>>>> This also effects dax+noreflink mode if we run the test after a
>>>>> dax+reflink test.  So, the most urgent thing is solving the warning
>>>>> messages.
>>>>>
>>>>> Patch 1 fixes some mistakes and adds handling of CoW cases not
>>>>> previously considered (srcmap is HOLE or UNWRITTEN).
>>>>> Patch 2 adds the implementation of unshare for fsdax.
>>>>>
>>>>> With these fixes, most warning messages in dax_associate_entry() are
>>>>> gone.  But honestly, generic/388 will randomly failed with the warning.
>>>>> The case shutdown the xfs when fsstress is running, and do it for many
>>>>> times.  I think the reason is that dax pages in use are not able to be
>>>>> invalidated in time when fs is shutdown.  The next time dax page to be
>>>>> associated, it still remains the mapping value set last time.  I'll keep
>>>>> on solving it.
>>>>>
>>>>> The warning message in dax_writeback_one() can also be fixed because of
>>>>> the dax unshare.
>>>>
>>>> Thank you for digging in on this, I had been pinned down on CXL tasks
>>>> and worried that we would need to mark FS_DAX broken for a cycle, so
>>>> this is timely.
>>>>
>>>> My only concern is that these patches look to have significant collisions with
>>>> the fsdax page reference counting reworks pending in linux-next. Although,
>>>> those are still sitting in mm-unstable:
>>>>
>>>> http://lore.kernel.org/r/20221108162059.2ee440d5244657c4f16bdca0@linux-foundation.org
>>>>
>>>> My preference would be to move ahead with both in which case I can help
>>>> rebase these fixes on top. In that scenario everything would go through
>>>> Andrew.
>>>>
>>>> However, if we are getting too late in the cycle for that path I think
>>>> these dax-fixes take precedence, and one more cycle to let the page
>>>> reference count reworks sit is ok.
>>>
>>> Well now that raises some interesting questions -- dax and reflink are
>>> totally broken on 6.1.  I was thinking about cramming them into 6.2 as a
>>> data corruption fix on the grounds that is not an acceptable state of
>>> affairs.
>>
>> I agree it's not an acceptable state of affairs, but for 6.1 the answer
>> may be to just revert to dax+reflink being forbidden again. The fact
>> that no end user has noticed is probably a good sign that we can disable
>> that without any one screaming. That may be the easy answer for 6.2 as
>> well given how late this all is.
>>
>>> OTOH we're past -rc7, which is **really late** to be changing core code.
>>> Then again, there aren't so many fsdax users and nobody's complained
>>> about 6.0/6.1 being busted, so perhaps the risk of regression isn't so
>>> bad?  Then again, that could be a sign that this could wait, if you and
>>> Andrew are really eager to merge the reworks.
>>
>> The page reference counting has also been languishing for a long time. A
>> 6.2 merge would be nice, it relieves maintenance burden, but they do not
>> start to have real end user implications until CXL memory hotplug
>> platforms arrive and the warts in the reference counting start to show
>> real problems in production.
> 
> Hm.  How bad *would* it be to rebase that patchset atop this one?
> 
> After overnight testing on -rc7 it looks like Ruan's patchset fixes all
> the problems AFAICT.  Most of the remaining regressions are to mask off
> fragmentation testing because fsdax cow (like the directio write paths)
> doesn't make much use of extent size hints.
> 
>>> Just looking at the stuff that's still broken with dax+reflink -- I
>>> noticed that xfs/550-552 (aka the dax poison tests) are still regressing
>>> on reflink filesystems.
>>
>> That's worrying because the whole point of reworking dax, xfs, and
>> mm/memory-failure all at once was to handle the collision of poison and
>> reflink'd dax files.
> 
> I just tried out -rc7 and all three pass, so disregard this please.
> 
>>> So, uh, what would this patchset need to change if the "fsdax page
>>> reference counting reworks" were applied?  Would it be changing the page
>>> refcount instead of stashing that in page->index?
>>
>> Nah, it's things like switching from pages to folios and shifting how
>> dax goes from pfns to pages.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/commit/?h=mm-unstable&id=cca48ba3196
>>
>> Ideally fsdax would never deal in pfns at all and do everything in terms
>> of offsets relative to a 'struct dax_device'.
>>
>> My gut is saying these patches, the refcount reworks, and the
>> dax+reflink fixes, are important but not end user critical. One more
>> status quo release does not hurt, and we can circle back to get this all
>> straightened early in v6.3.
> 
> Being a data corruption fix, I don't see why we shouldn't revisit this
> during the 6.2 cycle, even if it comes after merging the refcounting
> stuff.
> 
> Question for Ruan: Would it be terribly difficult to push out a v2 with
> the review comments applied so that we have something we can backport to
> 6.1; and then rebase the series atop 6.2-rc1 so we can apply it to
> upstream (and then apply the 6.1 version to the LTS)?  Or is this too
> convoluted...?

It's fine to me.  V2 has been posted just now.  The big patch has been 
separated.


--
Thanks,
Ruan.

> 
>> I.e. just revert:
>>
>> 35fcd75af3ed xfs: fail dax mount if reflink is enabled on a partition
>>
>> ...for v6.1-rc8 and get back to this early in the New Year.
> 
> Hm.  Tempting.
> 
> --D
> 
>>>
>>> --D
>>>
>>>>> Shiyang Ruan (2):
>>>>>    fsdax,xfs: fix warning messages at dax_[dis]associate_entry()
>>>>>    fsdax,xfs: port unshare to fsdax
>>>>>
>>>>>   fs/dax.c             | 166 ++++++++++++++++++++++++++++++-------------
>>>>>   fs/xfs/xfs_iomap.c   |   6 +-
>>>>>   fs/xfs/xfs_reflink.c |   8 ++-
>>>>>   include/linux/dax.h  |   2 +
>>>>>   4 files changed, 129 insertions(+), 53 deletions(-)
>>>>>
>>>>> -- 
>>>>> 2.38.1
>>
>>