Re: [PATCH 0/2] fsdax,xfs: fix warning messages
From: Shiyang Ruan
Date: Thu Dec 01 2022 - 10:40:40 EST
在 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@xxxxxxxxxxxxxxxxxxxx
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