Re: [PATCH v1 0/2] mm/madvise: make MADV_POPULATE_(READ|WRITE) handle VM_FAULT_RETRY properly

From: "Darrick J. Wong" <djwong@kernel.org>
To: David Hildenbrand <david@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Andrew Morton <akpm@linux-foundation.org>,
	John Hubbard <jhubbard@nvidia.com>,
	Jason Gunthorpe <jgg@nvidia.com>, Hugh Dickins <hughd@google.com>
Subject: Re: [PATCH v1 0/2] mm/madvise: make MADV_POPULATE_(READ|WRITE) handle VM_FAULT_RETRY properly
Date: Thu, 14 Mar 2024 19:25:40 -0700	[thread overview]
Message-ID: <20240315022540.GD6226@frogsfrogsfrogs> (raw)
In-Reply-To: <20240314161300.382526-1-david@redhat.com>

On Thu, Mar 14, 2024 at 05:12:58PM +0100, David Hildenbrand wrote:
> Derrick reports that in some cases where pread() would fail with -EIO and
> mmap()+access would generate a SIGBUS signal, MADV_POPULATE_READ /
> MADV_POPULATE_WRITE will keep retrying forever and not fail with -EFAULT.
> 
> It all boils down to missing VM_FAULT_RETRY handling. Let's try to handle
> that in a better way, similar to how ordinary GUP handles it.
> 
> Details in patch #1. In short, move special MADV_POPULATE_(READ|WRITE)
> VMA handling into __get_user_pages(), and make faultin_page_range()
> call __get_user_pages_locked(), which handles VM_FAULT_RETRY. Further,
> avoid the now-useless madvise VMA walk, because __get_user_pages() will
> perform the VMA lookup either way.
> 
> I briefly played with handling the FOLL_MADV_POPULATE checks in
> __get_user_pages() a bit differently, integrating them with existing
> handling, but it ended up looking worse. So I decided to keep it simple.
> 
> Likely, we need better selftests, but the reproducer from Darrick might
> be a bit hard to convert into a simple selftest.

No worries, I can convert my reproducer into an fstest.  I actually had
no idea that there were so many madvise flags, it's tempting to wire up
fsx and fsstress so that the long soak group tests will exercise them.

> Note that using mlock() in Darricks reproducer results in a similar
> endless retry. Likely, that is not what we want, and we should handle
> VM_FAULT_RETRY in populate_vma_page_range() / __mm_populate() as well.
> However, similarly using __get_user_pages_locked() might be more
> complicated, because of the advanced VMA handling in
> populate_vma_page_range().
> 
> Further, most populate_vma_page_range() callers simply ignore the return
> values, so it's unclear in which cases we expect to just silently fail, or
> where we'd want to retry+fail or endlessly retry instead.

With this patchset applied, my reproducer no longer gets stuck in an
infinite loop.  I'll throw this at fstests overnight and see if anything
else falls out.  Thank you!

--D

> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Darrick J. Wong <djwong@kernel.org>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Jason Gunthorpe <jgg@nvidia.com>
> Cc: Hugh Dickins <hughd@google.com>
> 
> David Hildenbrand (2):
>   mm/madvise: make MADV_POPULATE_(READ|WRITE) handle VM_FAULT_RETRY
>     properly
>   mm/madvise: don't perform madvise VMA walk for
>     MADV_POPULATE_(READ|WRITE)
> 
>  mm/gup.c      | 54 ++++++++++++++++++++++++++++++---------------------
>  mm/internal.h | 10 ++++++----
>  mm/madvise.c  | 43 +++++++++++++---------------------------
>  3 files changed, 52 insertions(+), 55 deletions(-)
> 
> 
> base-commit: f48159f866f422371bb1aad10eb4d05b29ca4d8c
> -- 
> 2.43.2
> 