Message-ID: <CAD=HUj6rVKt=gYBM_y-biDXc_QkFc2-JBRzYNsPJokGEdokH2w@mail.gmail.com>
Date:   Tue, 28 Mar 2023 18:48:56 +0900
From:   David Stevens <stevensd@...omium.org>
To:     Hugh Dickins <hughd@...gle.com>
Cc:     linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        Peter Xu <peterx@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Yang Shi <shy828301@...il.com>,
        David Hildenbrand <david@...hat.com>,
        Jiaqi Yan <jiaqiyan@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/3] mm/khugepaged: maintain page cache uptodate flag

On Fri, Mar 24, 2023 at 4:08 AM Hugh Dickins <hughd@...gle.com> wrote:
>
> On Tue, 7 Mar 2023, David Stevens wrote:
>
> > From: David Stevens <stevensd@...omium.org>
> >
> > Make sure that collapse_file doesn't interfere with checking the
> > uptodate flag in the page cache by only inserting hpage into the page
> > cache after it has been updated and marked uptodate. This is achieved by
> > simply not replacing present pages with hpage when iterating over the
> > target range. The present pages are already locked, so replacing the
> > with the locked hpage before the collapse is finalized is unnecessary.
> >
> > This fixes a race where folio_seek_hole_data would mistake hpage for
> > an fallocated but unwritten page. This race is visible to userspace via
> > data temporarily disappearing from SEEK_DATA/SEEK_HOLE.
> >
> > Fixes: f3f0e1d2150b ("khugepaged: add support of collapse for tmpfs/shmem pages")
> > Signed-off-by: David Stevens <stevensd@...omium.org>
> > Acked-by: Peter Xu <peterx@...hat.com>
>
> NAK to this patch, I'm afraid: it deadlocks.
>
> What I know it to deadlock against, does not make the most persuasive
> argument: cgroup v1 deprecated memcg moving, where mc_handle_file_pte()
> uses filemap_get_incore_folio() while holding page table lock, and spins
> around doing "goto repeat" in filemap_get_entry() while folio_try_get_rcu()
> keeps failing because collapse_file()'s old page has been left in the
> xarray with its refcount frozen to 0.  Meanwhile, collapse_file() is
> spinning to get that page table lock, to unmap pte of a later page.
>
> mincore()'s use of filemap_get_incore_folio() would be liable to hit
> the same deadlock.  If we think for longer, we may find more examples.
> But even when not actually deadlocking, it's wasting lots of CPU on
> concurrent lookups (e.g. faults) spinning in filemap_get_entry().

Ignoring my changes for now, these callers of filemap_get_incore_folio
seem broken to some degree with respect to khugepaged. Mincore can
show mlocked pages spuriously disappearing - this is pretty easy to
reproduce with concurrent calls to MADV_COLLAPSE and mincore. As for
the memcg code, I'm not sure how precise it is expected to be, but it
seems likely that khugepaged can make task migration accounting less
reliable (although I don't really understand the code).

> I don't suppose it's entirely accurate, but think of keeping a folio
> refcount frozen to 0 as like holding a spinlock (and this lock sadly out
> of sight from lockdep).  The pre-existing code works because the old page
> with refcount frozen to 0 is immediately replaced in the xarray by an
> entry for the new hpage, so the old page is no longer discoverable:
> and the new hpage is locked, not with a spinlock but the usual
> folio/page lock, on which concurrent lookups will sleep.

Is it actually necessary to freeze the original pages? At least at a
surface level, it seems that the arguments in 87c460a0bded
("mm/khugepaged: collapse_shmem() without freezing new_page") would
apply to the original pages as well. And if it is actually necessary
to freeze the original pages, why is it not necessary to freeze the
hugepage for the rollback case? Rolling back hugepage->original pages
seems more-or-less symmetric to collapsing original pages->hugepage.

> Your discovery of the SEEK_DATA/SEEK_HOLE issue is important - thank
> you - but I believe collapse_file() should be left as is, and the fix
> made instead in mapping_seek_hole_data() or folio_seek_hole_data():
> I believe that should not jump to assume that a !uptodate folio is a
> hole (as was reasonable to assume for shmem, before collapsing to huge
> got added), but should lock the folio if !uptodate, and check again
> after getting the lock - if still !uptodate, it's a shmem hole, not
> a transient race with collapse_file().

This sounds like it would work for lseek. I guess it could maybe be
made to sort of work for mincore if we abort the walk, lock the page,
restart the walk, and then re-validate the locked page. Although
that's not exactly efficient.

-David

> I was (pleased but) a little surprised when Matthew found in 5.12 that
> shmem_seek_hole_data() could be generalized to filemap_seek_hole_data():
> he will have a surer grasp of what's safe or unsafe to assume of
> !uptodate in non-shmem folios.
>
> On an earlier audit, for different reasons, I did also run across
> lib/buildid.c build_id_parse() using find_get_page() without checking
> PageUptodate() - looks as if it might do the wrong thing if it races
> with khugepaged collapsing text to huge, and should probably have a
> similar fix.
>
> Hugh

Powered by blists - more mailing lists