On Thu, Dec 01, 2022 at 11:39:12PM +0800, Shiyang Ruan wrote: > > > 在 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. Ok, thank you. Since akpm/Dan aren't moving forward with the page refcounting changes for 6.2, I think I'll try to merge these fixes for 6.2-rc1 without so much rebasing. :) --D > > -- > 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 > > > > > >
