lkml.org 
[lkml]   [2019]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] huegtlbfs: fix page leak during migration of file pages
Date
On Thu, Feb 07, 2019 at 10:50:55AM -0800, Mike Kravetz wrote:
> On 1/30/19 1:14 PM, Mike Kravetz wrote:
> > Files can be created and mapped in an explicitly mounted hugetlbfs
> > filesystem. If pages in such files are migrated, the filesystem
> > usage will not be decremented for the associated pages. This can
> > result in mmap or page allocation failures as it appears there are
> > fewer pages in the filesystem than there should be.
>
> Does anyone have a little time to take a look at this?
>
> While migration of hugetlb pages 'should' not be a common issue, we
> have seen it happen via soft memory errors/page poisoning in production
> environments. Didn't see a leak in that case as it was with pages in a
> Sys V shared mem segment. However, our DB code is starting to make use
> of files in explicitly mounted hugetlbfs filesystems. Therefore, we are
> more likely to hit this bug in the field.

Hi Mike,

Thank you for finding/reporting the problem.
# sorry for my late response.

>
> >
> > For example, a test program which hole punches, faults and migrates
> > pages in such a file (1G in size) will eventually fail because it
> > can not allocate a page. Reported counts and usage at time of failure:
> >
> > node0
> > 537 free_hugepages
> > 1024 nr_hugepages
> > 0 surplus_hugepages
> > node1
> > 1000 free_hugepages
> > 1024 nr_hugepages
> > 0 surplus_hugepages
> >
> > Filesystem Size Used Avail Use% Mounted on
> > nodev 4.0G 4.0G 0 100% /var/opt/hugepool
> >
> > Note that the filesystem shows 4G of pages used, while actual usage is
> > 511 pages (just under 1G). Failed trying to allocate page 512.
> >
> > If a hugetlb page is associated with an explicitly mounted filesystem,
> > this information in contained in the page_private field. At migration
> > time, this information is not preserved. To fix, simply transfer
> > page_private from old to new page at migration time if necessary. Also,
> > migrate_page_states() unconditionally clears page_private and PagePrivate
> > of the old page. It is unlikely, but possible that these fields could
> > be non-NULL and are needed at hugetlb free page time. So, do not touch
> > these fields for hugetlb pages.
> >
> > Cc: <stable@vger.kernel.org>
> > Fixes: 290408d4a250 ("hugetlb: hugepage migration core")
> > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> > ---
> > fs/hugetlbfs/inode.c | 10 ++++++++++
> > mm/migrate.c | 10 ++++++++--
> > 2 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > index 32920a10100e..fb6de1db8806 100644
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -859,6 +859,16 @@ static int hugetlbfs_migrate_page(struct address_space *mapping,
> > rc = migrate_huge_page_move_mapping(mapping, newpage, page);
> > if (rc != MIGRATEPAGE_SUCCESS)
> > return rc;
> > +
> > + /*
> > + * page_private is subpool pointer in hugetlb pages, transfer
> > + * if needed.
> > + */
> > + if (page_private(page) && !page_private(newpage)) {
> > + set_page_private(newpage, page_private(page));
> > + set_page_private(page, 0);

You don't have to copy PagePrivate flag?

> > + }
> > +
> > if (mode != MIGRATE_SYNC_NO_COPY)
> > migrate_page_copy(newpage, page);
> > else
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index f7e4bfdc13b7..0d9708803553 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -703,8 +703,14 @@ void migrate_page_states(struct page *newpage, struct page *page)
> > */
> > if (PageSwapCache(page))
> > ClearPageSwapCache(page);
> > - ClearPagePrivate(page);
> > - set_page_private(page, 0);
> > + /*
> > + * Unlikely, but PagePrivate and page_private could potentially
> > + * contain information needed at hugetlb free page time.
> > + */
> > + if (!PageHuge(page)) {
> > + ClearPagePrivate(page);
> > + set_page_private(page, 0);
> > + }

# This argument is mainly for existing code...

According to the comment on migrate_page():

/*
* Common logic to directly migrate a single LRU page suitable for
* pages that do not use PagePrivate/PagePrivate2.
*
* Pages are locked upon entry and exit.
*/
int migrate_page(struct address_space *mapping, ...

So this common logic assumes that page_private is not used, so why do
we explicitly clear page_private in migrate_page_states()?
buffer_migrate_page(), which is commonly used for the case when
page_private is used, does that clearing outside migrate_page_states().
So I thought that hugetlbfs_migrate_page() could do in the similar manner.
IOW, migrate_page_states() should not do anything on PagePrivate.
But there're a few other .migratepage callbacks, and I'm not sure all of
them are safe for the change, so this approach might not fit for a small fix.

# BTW, there seems a typo in $SUBJECT.

Thanks,
Naoya Horiguchi

\
 
 \ /
  Last update: 2019-02-08 03:35    [W:0.096 / U:3.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site