Message-ID: <Yw01bD00XO+QQbFL@monkey>
Date:   Mon, 29 Aug 2022 14:53:48 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Miaohe Lin <linmiaohe@...wei.com>
Cc:     Muchun Song <songmuchun@...edance.com>,
        David Hildenbrand <david@...hat.com>,
        Michal Hocko <mhocko@...e.com>, Peter Xu <peterx@...hat.com>,
        Naoya Horiguchi <naoya.horiguchi@...ux.dev>,
        "Aneesh Kumar K . V" <aneesh.kumar@...ux.vnet.ibm.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Prakash Sangappa <prakash.sangappa@...cle.com>,
        James Houghton <jthoughton@...gle.com>,
        Mina Almasry <almasrymina@...gle.com>,
        Pasha Tatashin <pasha.tatashin@...een.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Ray Fucillo <Ray.Fucillo@...ersystems.com>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/8] hugetlb: handle truncate racing with page faults

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@...cle.com>
> > ---
> >  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@...wei.com>
> 
> Thanks,
> Miaohe Lin
> 

Powered by blists - more mailing lists