Re: [PATCH 0/1] RESEND fix page_cache_next/prev_miss off by one error

Re: [PATCH 0/1] RESEND fix page_cache_next/prev_miss off by one error

[Andrew Morton]

On Fri,  2 Jun 2023 15:57:46 -0700 Mike Kravetz <mike.kravetz@oracle.com> wrote:

> In commits d0ce0e47b323 and 91a2fb956ad99, hugetlb code was changed to
> use page_cache_next_miss to determine if a page was present in the page
> cache.  However, the current implementation of page_cache_next_miss will
> always return the passed index if max_scan is 1 as in the hugetlb code.
> As a result, hugetlb code will always thing a page is present in the
> cache, even if that is not the case.
> 
> The patch which follows addresses the issue by changing the implementation
> of page_cache_next_miss and for consistency page_cache_prev_miss.  Since
> such a patch also impacts the readahead code, I would suggest using the
> patch by Sidhartha Kumar [1] to fix the issue in 6.3 and this patch moving
> forward.

Well this is tricky.

This patch applies cleanly to 6.3, so if we add cc:stable to this
patch, it will get backported, against your suggestion.

Sidhartha's patch [1] (which you recommend for -stable) is quite
different from this patch.  And Sidhartha's patch has no route to being
tested in linux-next nor to being merged by Linus.

So problems.  The preferable approach is to just backport this patch
into -stable in the usual fashion.  What are the risks in doing this?

> If we would rather not modify page_cache_next/prev_miss, then a new
> interface as suggested by Ackerley Tng [2] could also be used.
> 
> Comments on the best way to fix moving forward would be appreciated.
> 
> [1] https://lore.kernel.org/linux-mm/20230505185301.534259-1-sidhartha.kumar@oracle.com/
> [2] https://lore.kernel.org/linux-mm/98624c2f481966492b4eb8272aef747790229b73.1683069252.git.ackerleytng@google.com/
> 
> Mike Kravetz (1):
>   page cache: fix page_cache_next/prev_miss off by one
> 
>  mm/filemap.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 

Re: [PATCH 0/1] RESEND fix page_cache_next/prev_miss off by one error

[Mike Kravetz]

On 06/02/23 17:55, Andrew Morton wrote:
> On Fri,  2 Jun 2023 15:57:46 -0700 Mike Kravetz <mike.kravetz@oracle.com> wrote:
> 
> > In commits d0ce0e47b323 and 91a2fb956ad99, hugetlb code was changed to
> > use page_cache_next_miss to determine if a page was present in the page
> > cache.  However, the current implementation of page_cache_next_miss will
> > always return the passed index if max_scan is 1 as in the hugetlb code.
> > As a result, hugetlb code will always thing a page is present in the
> > cache, even if that is not the case.
> > 
> > The patch which follows addresses the issue by changing the implementation
> > of page_cache_next_miss and for consistency page_cache_prev_miss.  Since
> > such a patch also impacts the readahead code, I would suggest using the
> > patch by Sidhartha Kumar [1] to fix the issue in 6.3 and this patch moving
> > forward.
> 
> Well this is tricky.
> 
> This patch applies cleanly to 6.3, so if we add cc:stable to this
> patch, it will get backported, against your suggestion.
> 
> Sidhartha's patch [1] (which you recommend for -stable) is quite
> different from this patch.  And Sidhartha's patch has no route to being
> tested in linux-next nor to being merged by Linus.
> 
> So problems.  The preferable approach is to just backport this patch
> into -stable in the usual fashion.  What are the risks in doing this?

Really hoping to get some comments from Matthew on this.

The only other user is the readahead code and I have little
experience/knowledge there.

Unless I totally screwed up the code, page_cache_next/prev_miss will now
correctly indicate the lack of a page in the cache in these edge cases.
Since readahead is more about performance than correctness (not trying
to minimize), the risk should be small.
-- 
Mike Kravetz
>