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;
Date:   Thu, 23 Mar 2023 12:07:46 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     David Stevens <stevensd@chromium.org>
cc:     linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
        Peter Xu <peterx@redhat.com>,
        Matthew Wilcox <willy@infradead.org>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Yang Shi <shy828301@gmail.com>,
        David Hildenbrand <david@redhat.com>,
        Hugh Dickins <hughd@google.com>,
        Jiaqi Yan <jiaqiyan@google.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 3/3] mm/khugepaged: maintain page cache uptodate
 flag
In-Reply-To: <20230307052036.1520708-4-stevensd@google.com>
Message-ID: <866d1a75-d462-563-dfd7-1aa2971a285b@google.com>
References: <20230307052036.1520708-1-stevensd@google.com> <20230307052036.1520708-4-stevensd@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Tue, 7 Mar 2023, David Stevens wrote:

> From: David Stevens <stevensd@chromium.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@chromium.org>
> Acked-by: Peter Xu <peterx@redhat.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().

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.

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().

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

> ---
>  mm/khugepaged.c | 50 ++++++++++++-------------------------------------
>  1 file changed, 12 insertions(+), 38 deletions(-)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 51ae399f2035..bdde0a02811b 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1930,12 +1930,6 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,
>  		}
>  	} while (1);
>  
> -	/*
> -	 * At this point the hpage is locked and not up-to-date.
> -	 * It's safe to insert it into the page cache, because nobody would
> -	 * be able to map it or use it in another way until we unlock it.
> -	 */
> -
>  	xas_set(&xas, start);
>  	for (index = start; index < end; index++) {
>  		page = xas_next(&xas);
> @@ -2104,13 +2098,9 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,
>  		}
>  
>  		/*
> -		 * Add the page to the list to be able to undo the collapse if
> -		 * something go wrong.
> +		 * Accumulate the pages that are being collapsed.
>  		 */
>  		list_add_tail(&page->lru, &pagelist);
> -
> -		/* Finally, replace with the new page. */
> -		xas_store(&xas, hpage);
>  		continue;
>  out_unlock:
>  		unlock_page(page);
> @@ -2149,8 +2139,7 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,
>  		goto rollback;
>  
>  	/*
> -	 * Replacing old pages with new one has succeeded, now we
> -	 * attempt to copy the contents.
> +	 * The old pages are locked, so they won't change anymore.
>  	 */
>  	index = start;
>  	list_for_each_entry(page, &pagelist, lru) {
> @@ -2230,11 +2219,11 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,
>  		/* nr_none is always 0 for non-shmem. */
>  		__mod_lruvec_page_state(hpage, NR_SHMEM, nr_none);
>  	}
> -	/* Join all the small entries into a single multi-index entry. */
> -	xas_set_order(&xas, start, HPAGE_PMD_ORDER);
> -	xas_store(&xas, hpage);
> -	xas_unlock_irq(&xas);
>  
> +	/*
> +	 * Mark hpage as uptodate before inserting it into the page cache so
> +	 * that it isn't mistaken for an fallocated but unwritten page.
> +	 */
>  	folio = page_folio(hpage);
>  	folio_mark_uptodate(folio);
>  	folio_ref_add(folio, HPAGE_PMD_NR - 1);
> @@ -2243,6 +2232,11 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,
>  		folio_mark_dirty(folio);
>  	folio_add_lru(folio);
>  
> +	/* Join all the small entries into a single multi-index entry. */
> +	xas_set_order(&xas, start, HPAGE_PMD_ORDER);
> +	xas_store(&xas, hpage);
> +	xas_unlock_irq(&xas);
> +
>  	/*
>  	 * Remove pte page tables, so we can re-fault the page as huge.
>  	 */
> @@ -2267,36 +2261,18 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,
>  
>  rollback:
>  	/* Something went wrong: roll back page cache changes */
> -	xas_lock_irq(&xas);
>  	if (nr_none) {
>  		mapping->nrpages -= nr_none;
>  		shmem_uncharge(mapping->host, nr_none);
>  	}
>  
> -	xas_set(&xas, start);
> -	end = index;
> -	for (index = start; index < end; index++) {
> -		xas_next(&xas);
> -		page = list_first_entry_or_null(&pagelist,
> -				struct page, lru);
> -		if (!page || xas.xa_index < page->index) {
> -			nr_none--;
> -			continue;
> -		}
> -
> -		VM_BUG_ON_PAGE(page->index != xas.xa_index, page);
> -
> +	list_for_each_entry_safe(page, tmp, &pagelist, lru) {
>  		/* Unfreeze the page. */
>  		list_del(&page->lru);
>  		page_ref_unfreeze(page, 2);
> -		xas_store(&xas, page);
> -		xas_pause(&xas);
> -		xas_unlock_irq(&xas);
>  		unlock_page(page);
>  		putback_lru_page(page);
> -		xas_lock_irq(&xas);
>  	}
> -	VM_BUG_ON(nr_none);
>  	/*
>  	 * Undo the updates of filemap_nr_thps_inc for non-SHMEM file only.
>  	 * This undo is not needed unless failure is due to SCAN_COPY_MC.
> @@ -2304,8 +2280,6 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,
>  	if (!is_shmem && result == SCAN_COPY_MC)
>  		filemap_nr_thps_dec(mapping);
>  
> -	xas_unlock_irq(&xas);
> -
>  	hpage->mapping = NULL;
>  
>  	unlock_page(hpage);
> -- 
> 2.40.0.rc0.216.gc4246ad0f0-goog