Re: [External] Re: [PATCH 2/6] hugetlbfs: fix cannot migrate the fallocated HugeTLB page

[Muchun Song <songmuchun@bytedance.com>]

On Wed, Jan 6, 2021 at 6:29 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> On 1/4/21 6:44 PM, Muchun Song wrote:
> > On Tue, Jan 5, 2021 at 6:40 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
> >>
> >> On 1/3/21 10:58 PM, Muchun Song wrote:
> >>> Because we only can isolate a active page via isolate_huge_page()
> >>> and hugetlbfs_fallocate() forget to mark it as active, we cannot
> >>> isolate and migrate those pages.
> >>>
> >>> Fixes: 70c3547e36f5 (hugetlbfs: add hugetlbfs_fallocate())
> >>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> >>> ---
> >>>  fs/hugetlbfs/inode.c | 5 +++--
> >>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> Good catch.  This is indeed an issue.
> >>
> >>>
> >>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> >>> index b5c109703daa..2aceb085d202 100644
> >>> --- a/fs/hugetlbfs/inode.c
> >>> +++ b/fs/hugetlbfs/inode.c
> >>> @@ -737,10 +737,11 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
> >>>
> >>>               /*
> >>>                * unlock_page because locked by add_to_page_cache()
> >>> -              * page_put due to reference from alloc_huge_page()
> >>> +              * put_page() (which is in the putback_active_hugepage())
> >>> +              * due to reference from alloc_huge_page()
> >>
> >> Thanks for fixing the comment.
> >>
> >>>                */
> >>>               unlock_page(page);
> >>> -             put_page(page);
> >>> +             putback_active_hugepage(page);
> >>
> >> I'm curious why you used putback_active_hugepage() here instead of simply
> >> calling set_page_huge_active() before the put_page()?
> >>
> >> When the page was allocated, it was placed on the active list (alloc_huge_page).
> >> Therefore, the hugetlb_lock locking and list movement should not be necessary.
> >
> > I agree with you. Because set_page_huge_active is not exported (static
> > function). Only exporting set_page_huge_active seems strange (leaving
> > clear_page_huge_active not export). This is just my opinion. What's
> > yours, Mike?
>
> I'm thinking that we should export (make external) set_page_huge_active.
> We can leave clear_page_huge_active as static and just add something to
> the commit log noting that there are no external users.
>
> My primary reason for doing this is to eliminate the extra and unnecessary
> per-page lock/unlock cycle.  I believe there are some applications that
> use fallocate to pre-allocate very large hugetlbfs files.  They may notice
> the extra overhead.

Agree. Will do in the next version. Thanks.

> --
> Mike Kravetz