On 08/27/22 16:02, Miaohe Lin wrote:
> On 2022/8/25 1:57, Mike Kravetz wrote:
> > When page fault code needs to allocate and instantiate a new hugetlb
> > page (huegtlb_no_page), it checks early to determine if the fault is
> > beyond i_size. When discovered early, it is easy to abort the fault and
> > return an error. However, it becomes much more difficult to handle when
> > discovered later after allocating the page and consuming reservations
> > and adding to the page cache. Backing out changes in such instances
> > becomes difficult and error prone.
> >
> > Instead of trying to catch and backout all such races, use the hugetlb
> > fault mutex to handle truncate racing with page faults. The most
> > significant change is modification of the routine remove_inode_hugepages
> > such that it will take the fault mutex for EVERY index in the truncated
> > range (or hole in the case of hole punch). Since remove_inode_hugepages
> > is called in the truncate path after updating i_size, we can experience
> > races as follows.
> > - truncate code updates i_size and takes fault mutex before a racing
> > fault. After fault code takes mutex, it will notice fault beyond
> > i_size and abort early.
> > - fault code obtains mutex, and truncate updates i_size after early
> > checks in fault code. fault code will add page beyond i_size.
> > When truncate code takes mutex for page/index, it will remove the
> > page.
> > - truncate updates i_size, but fault code obtains mutex first. If
> > fault code sees updated i_size it will abort early. If fault code
> > does not see updated i_size, it will add page beyond i_size and
> > truncate code will remove page when it obtains fault mutex.
> >
> > Note, for performance reasons remove_inode_hugepages will still use
> > filemap_get_folios for bulk folio lookups. For indicies not returned in
> > the bulk lookup, it will need to lookup individual folios to check for
> > races with page fault.
> >
> > Signed-off-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
> > ---
> > fs/hugetlbfs/inode.c | 184 +++++++++++++++++++++++++++++++------------
> > mm/hugetlb.c | 41 +++++-----
> > 2 files changed, 152 insertions(+), 73 deletions(-)
> >
> > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > index d98c6edbd1a4..e83fd31671b3 100644
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -411,6 +411,95 @@ hugetlb_vmdelete_list(struct rb_root_cached *root, pgoff_t start, pgoff_t end,
> > }
> > }
> >
> > +/*
> > + * Called with hugetlb fault mutex held.
> > + * Returns true if page was actually removed, false otherwise.
> > + */
> > +static bool remove_inode_single_folio(struct hstate *h, struct inode *inode,
> > + struct address_space *mapping,
> > + struct folio *folio, pgoff_t index,
> > + bool truncate_op)
> > +{
> > + bool ret = false;
> > +
> > + /*
> > + * If folio is mapped, it was faulted in after being
> > + * unmapped in caller. Unmap (again) while holding
> > + * the fault mutex. The mutex will prevent faults
> > + * until we finish removing the folio.
> > + */
> > + if (unlikely(folio_mapped(folio))) {
> > + i_mmap_lock_write(mapping);
> > + hugetlb_vmdelete_list(&mapping->i_mmap,
> > + index * pages_per_huge_page(h),
> > + (index + 1) * pages_per_huge_page(h),
> > + ZAP_FLAG_DROP_MARKER);
> > + i_mmap_unlock_write(mapping);
> > + }
> > +
> > + folio_lock(folio);
> > + /*
> > + * After locking page, make sure mapping is the same.
> > + * We could have raced with page fault populate and
> > + * backout code.
> > + */
> > + if (folio_mapping(folio) == mapping) {
>
> Could you explain this more? IIUC, page fault won't remove the hugetlb page from page
> cache anymore. So this check is unneeded? Or we should always check this in case future
> code changing?
>
You are correct, with the updated code we should never hit this condition.
The faulting code will not remove pages from the page cache.
It can be removed.
> > + /*
> > + * We must remove the folio from page cache before removing
> > + * the region/ reserve map (hugetlb_unreserve_pages). In
> > + * rare out of memory conditions, removal of the region/reserve
> > + * map could fail. Correspondingly, the subpool and global
> > + * reserve usage count can need to be adjusted.
> > + */
> > + VM_BUG_ON(HPageRestoreReserve(&folio->page));
> > + hugetlb_delete_from_page_cache(&folio->page);
> > + ret = true;
> > + if (!truncate_op) {
> > + if (unlikely(hugetlb_unreserve_pages(inode, index,
> > + index + 1, 1)))
> > + hugetlb_fix_reserve_counts(inode);
> > + }
> > + }
> > +
> > + folio_unlock(folio);
> > + return ret;
> > +}
>
> <snip>
> > @@ -5584,9 +5585,13 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
> > clear_huge_page(page, address, pages_per_huge_page(h));
> > __SetPageUptodate(page);
> > new_page = true;
> > + if (HPageRestoreReserve(page))
> > + reserve_alloc = true;
> >
> > if (vma->vm_flags & VM_MAYSHARE) {
> > - int err = hugetlb_add_to_page_cache(page, mapping, idx);
> > + int err;
> > +
> > + err = hugetlb_add_to_page_cache(page, mapping, idx);
> > if (err) {
> > restore_reserve_on_error(h, vma, haddr, page);
> > put_page(page);
> > @@ -5642,10 +5647,6 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
> > }
> >
> > ptl = huge_pte_lock(h, mm, ptep);
> > - size = i_size_read(mapping->host) >> huge_page_shift(h);
> > - if (idx >= size)
> > - goto backout;
> > -
> > ret = 0;
> > /* If pte changed from under us, retry */
> > if (!pte_same(huge_ptep_get(ptep), old_pte))
> > @@ -5689,10 +5690,18 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
> > backout:
> > spin_unlock(ptl);
> > backout_unlocked:
> > - unlock_page(page);
> > - /* restore reserve for newly allocated pages not in page cache */
> > - if (new_page && !new_pagecache_page)
> > + if (new_page && !new_pagecache_page) {
> > + /*
> > + * If reserve was consumed, make sure flag is set so that it
> > + * will be restored in free_huge_page().
> > + */
> > + if (reserve_alloc)
> > + SetHPageRestoreReserve(page);
>
> If code reaches here, it should be a newly allocated page and it's not added to the hugetlb page cache.
> Note that failing to add the page to hugetlb page cache should have returned already. So the page must be
> anon? If so, HPageRestoreReserve isn't cleared yet as it's cleared right before set_huge_pte. Thus above
> check can be removed?
You are correct again. The above check can be removed.
Thanks!
I will remove them in a v2 series.
--
Mike Kravetz
>
> Anyway, the patch looks good to me.
>
> Reviewed-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
>
> Thanks,
> Miaohe Lin
>
