| Message ID | alpine.LSU.2.11.2005231837500.1766@eggly.anvils (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm,thp: stop leaking unreleased file pages | expand |
> On May 23, 2020, at 6:50 PM, Hugh Dickins <hughd@google.com> wrote: > > When collapse_file() calls try_to_release_page(), it has already > isolated the page: so if releasing buffers happens to fail (as it > sometimes does), remember to putback_lru_page(): otherwise that page is > left unreclaimable and unfreeable, and the file extent uncollapsible. > > Signed-off-by: Hugh Dickins <hughd@google.com> > Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS") > Cc: stable@vger.kernel.org # v5.4+ Acked-by: Song Liu <songliubraving@fb.com> Thanks for the fix! > --- > > mm/khugepaged.c | 1 + > 1 file changed, 1 insertion(+) > > --- 5.7-rc6/mm/khugepaged.c 2020-04-12 16:24:37.710999073 -0700 > +++ linux/mm/khugepaged.c 2020-05-10 17:06:21.788398646 -0700 > @@ -1692,6 +1692,7 @@ static void collapse_file(struct mm_stru > if (page_has_private(page) && > !try_to_release_page(page, GFP_KERNEL)) { > result = SCAN_PAGE_HAS_PRIVATE; > + putback_lru_page(page); > goto out_unlock; > } >
On Sat, May 23, 2020 at 06:50:15PM -0700, Hugh Dickins wrote: > When collapse_file() calls try_to_release_page(), it has already > isolated the page: so if releasing buffers happens to fail (as it > sometimes does), remember to putback_lru_page(): otherwise that page is > left unreclaimable and unfreeable, and the file extent uncollapsible. > > Signed-off-by: Hugh Dickins <hughd@google.com> > Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS") > Cc: stable@vger.kernel.org # v5.4+ Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
On Sat, May 23, 2020 at 06:50:15PM -0700, Hugh Dickins wrote: > When collapse_file() calls try_to_release_page(), it has already > isolated the page: so if releasing buffers happens to fail (as it > sometimes does), remember to putback_lru_page(): otherwise that page is > left unreclaimable and unfreeable, and the file extent uncollapsible. Oof, I could imagine that was painful to debug (unless you already suspected file THP due to a targeted test or similar). Kudos. > Signed-off-by: Hugh Dickins <hughd@google.com> > Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS") > Cc: stable@vger.kernel.org # v5.4+ Acked-by: Johannes Weiner <hannes@cmpxchg.org>
On Tue, 26 May 2020, Johannes Weiner wrote: > On Sat, May 23, 2020 at 06:50:15PM -0700, Hugh Dickins wrote: > > When collapse_file() calls try_to_release_page(), it has already > > isolated the page: so if releasing buffers happens to fail (as it > > sometimes does), remember to putback_lru_page(): otherwise that page is > > left unreclaimable and unfreeable, and the file extent uncollapsible. > > Oof, I could imagine that was painful to debug (unless you already > suspected file THP due to a targeted test or similar). Kudos. Thanks, but I have to refuse both your admiration and sympathy: mercifully, it was just something I noticed by source inspection when working there. I did then put in a debug count to see if it ever got hit in practice, and checked after big multi-testing runs: it was sometimes hit, but certainly not often, and I don't know what it was racing with when it happened - would depend on filesystem anyway (ext4 in our case). Saying "source inspection" reminds me: there is another funny in there, but I don't think it matters very much in practice, and might need rather a lot of testing to justify any particular patch: where page_cache_sync_readahead() asks for PAGE_SIZE pages! "end - index" seems a more reasonable number to me: but then we might find that reading ahead into the next huge extent had actually been a useful optimization (and those readahead functions impose their own caps, so PAGE_SIZE shouldn't work out too outrageously). Hugh > > > Signed-off-by: Hugh Dickins <hughd@google.com> > > Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS") > > Cc: stable@vger.kernel.org # v5.4+ > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
On Thu, 28 May 2020 at 02:58, Hugh Dickins <hughd@google.com> wrote: > > On Tue, 26 May 2020, Johannes Weiner wrote: > > On Sat, May 23, 2020 at 06:50:15PM -0700, Hugh Dickins wrote: > > > When collapse_file() calls try_to_release_page(), it has already > > > isolated the page: so if releasing buffers happens to fail (as it > > > sometimes does), remember to putback_lru_page(): otherwise that page is > > > left unreclaimable and unfreeable, and the file extent uncollapsible. > > > > Oof, I could imagine that was painful to debug (unless you already > > suspected file THP due to a targeted test or similar). Kudos. > > Thanks, but I have to refuse both your admiration and sympathy: > mercifully, it was just something I noticed by source inspection > when working there. > > I did then put in a debug count to see if it ever got hit in practice, > and checked after big multi-testing runs: it was sometimes hit, but > certainly not often, and I don't know what it was racing with when > it happened - would depend on filesystem anyway (ext4 in our case). > > Saying "source inspection" reminds me: there is another funny in there, > but I don't think it matters very much in practice, and might need > rather a lot of testing to justify any particular patch: where > page_cache_sync_readahead() asks for PAGE_SIZE pages! > > "end - index" seems a more reasonable number to me: but then we > might find that reading ahead into the next huge extent had actually > been a useful optimization (and those readahead functions impose > their own caps, so PAGE_SIZE shouldn't work out too outrageously). My two cents, Do you have a test case / test steps to validate this patch ? - Naresh
On Wed, May 27, 2020 at 02:28:10PM -0700, Hugh Dickins wrote: > Saying "source inspection" reminds me: there is another funny in there, > but I don't think it matters very much in practice, and might need > rather a lot of testing to justify any particular patch: where > page_cache_sync_readahead() asks for PAGE_SIZE pages! > > "end - index" seems a more reasonable number to me: but then we > might find that reading ahead into the next huge extent had actually > been a useful optimization (and those readahead functions impose > their own caps, so PAGE_SIZE shouldn't work out too outrageously). That readahead was only added in 99cb0dbd47a15d395bf3faa78dc122bc5efe3fc0 so it probably hasn't really been performance tested yet.
On Wed, 3 Jun 2020, Matthew Wilcox wrote: > On Wed, May 27, 2020 at 02:28:10PM -0700, Hugh Dickins wrote: > > Saying "source inspection" reminds me: there is another funny in there, > > but I don't think it matters very much in practice, and might need > > rather a lot of testing to justify any particular patch: where > > page_cache_sync_readahead() asks for PAGE_SIZE pages! > > > > "end - index" seems a more reasonable number to me: but then we > > might find that reading ahead into the next huge extent had actually > > been a useful optimization (and those readahead functions impose > > their own caps, so PAGE_SIZE shouldn't work out too outrageously). > > That readahead was only added in 99cb0dbd47a15d395bf3faa78dc122bc5efe3fc0 > so it probably hasn't really been performance tested yet. I can well imagine that an entirely new body of code, exercised in the background by khugepaged, will not be a leading candidate for performance comparisons - rightly so; but arguing that by the commit being in the tree for only eight months seems... odd :) Hugh
--- 5.7-rc6/mm/khugepaged.c 2020-04-12 16:24:37.710999073 -0700 +++ linux/mm/khugepaged.c 2020-05-10 17:06:21.788398646 -0700 @@ -1692,6 +1692,7 @@ static void collapse_file(struct mm_stru if (page_has_private(page) && !try_to_release_page(page, GFP_KERNEL)) { result = SCAN_PAGE_HAS_PRIVATE; + putback_lru_page(page); goto out_unlock; }
When collapse_file() calls try_to_release_page(), it has already isolated the page: so if releasing buffers happens to fail (as it sometimes does), remember to putback_lru_page(): otherwise that page is left unreclaimable and unfreeable, and the file extent uncollapsible. Signed-off-by: Hugh Dickins <hughd@google.com> Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS") Cc: stable@vger.kernel.org # v5.4+ --- mm/khugepaged.c | 1 + 1 file changed, 1 insertion(+)