| Message ID | 20220824175757.20590-2-mike.kravetz@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hugetlb: Use new vma mutex for huge pmd sharing synchronization | expand |
On 2022/8/25 1:57, Mike Kravetz wrote: > Commit c0d0381ade79 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing > synchronization") added code to take i_mmap_rwsem in read mode for the > duration of fault processing. The use of i_mmap_rwsem to prevent > fault/truncate races depends on this. However, this has been shown to > cause performance/scaling issues. As a result, that code will be > reverted. Since the use i_mmap_rwsem to address page fault/truncate races > depends on this, it must also be reverted. > > In a subsequent patch, code will be added to detect the fault/truncate > race and back out operations as required. LGTM. Thanks. Reviewed-by: Miaohe Lin <linmiaohe@huawei.com> Thanks, Miaohe Lin > > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> > --- > fs/hugetlbfs/inode.c | 30 +++++++++--------------------- > mm/hugetlb.c | 23 ++++++++++++----------- > 2 files changed, 21 insertions(+), 32 deletions(-) > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > index f7a5b5124d8a..a32031e751d1 100644 > --- a/fs/hugetlbfs/inode.c > +++ b/fs/hugetlbfs/inode.c > @@ -419,9 +419,10 @@ hugetlb_vmdelete_list(struct rb_root_cached *root, pgoff_t start, pgoff_t end, > * In this case, we first scan the range and release found pages. > * After releasing pages, hugetlb_unreserve_pages cleans up region/reserve > * maps and global counts. Page faults can not race with truncation > - * in this routine. hugetlb_no_page() holds i_mmap_rwsem and prevents > - * page faults in the truncated range by checking i_size. i_size is > - * modified while holding i_mmap_rwsem. > + * in this routine. hugetlb_no_page() prevents page faults in the > + * truncated range. It checks i_size before allocation, and again after > + * with the page table lock for the page held. The same lock must be > + * acquired to unmap a page. > * hole punch is indicated if end is not LLONG_MAX > * In the hole punch case we scan the range and release found pages. > * Only when releasing a page is the associated region/reserve map > @@ -451,16 +452,8 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart, > u32 hash = 0; > > index = folio->index; > - if (!truncate_op) { > - /* > - * Only need to hold the fault mutex in the > - * hole punch case. This prevents races with > - * page faults. Races are not possible in the > - * case of truncation. > - */ > - hash = hugetlb_fault_mutex_hash(mapping, index); > - mutex_lock(&hugetlb_fault_mutex_table[hash]); > - } > + hash = hugetlb_fault_mutex_hash(mapping, index); > + mutex_lock(&hugetlb_fault_mutex_table[hash]); > > /* > * If folio is mapped, it was faulted in after being > @@ -504,8 +497,7 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart, > } > > folio_unlock(folio); > - if (!truncate_op) > - mutex_unlock(&hugetlb_fault_mutex_table[hash]); > + mutex_unlock(&hugetlb_fault_mutex_table[hash]); > } > folio_batch_release(&fbatch); > cond_resched(); > @@ -543,8 +535,8 @@ static void hugetlb_vmtruncate(struct inode *inode, loff_t offset) > BUG_ON(offset & ~huge_page_mask(h)); > pgoff = offset >> PAGE_SHIFT; > > - i_mmap_lock_write(mapping); > i_size_write(inode, offset); > + i_mmap_lock_write(mapping); > if (!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root)) > hugetlb_vmdelete_list(&mapping->i_mmap, pgoff, 0, > ZAP_FLAG_DROP_MARKER); > @@ -703,11 +695,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, > /* addr is the offset within the file (zero based) */ > addr = index * hpage_size; > > - /* > - * fault mutex taken here, protects against fault path > - * and hole punch. inode_lock previously taken protects > - * against truncation. > - */ > + /* mutex taken here, fault path and hole punch */ > hash = hugetlb_fault_mutex_hash(mapping, index); > mutex_lock(&hugetlb_fault_mutex_table[hash]); > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 9a72499486c1..70bc7f867bc0 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -5575,18 +5575,17 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, > } > > /* > - * We can not race with truncation due to holding i_mmap_rwsem. > - * i_size is modified when holding i_mmap_rwsem, so check here > - * once for faults beyond end of file. > + * Use page lock to guard against racing truncation > + * before we get page_table_lock. > */ > - size = i_size_read(mapping->host) >> huge_page_shift(h); > - if (idx >= size) > - goto out; > - > retry: > new_page = false; > page = find_lock_page(mapping, idx); > if (!page) { > + size = i_size_read(mapping->host) >> huge_page_shift(h); > + if (idx >= size) > + goto out; > + > /* Check for page in userfault range */ > if (userfaultfd_missing(vma)) { > ret = hugetlb_handle_userfault(vma, mapping, idx, > @@ -5677,6 +5676,10 @@ 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)) > @@ -5785,10 +5788,8 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma, > > /* > * Acquire i_mmap_rwsem before calling huge_pte_alloc and hold > - * until finished with ptep. This serves two purposes: > - * 1) It prevents huge_pmd_unshare from being called elsewhere > - * and making the ptep no longer valid. > - * 2) It synchronizes us with i_size modifications during truncation. > + * until finished with ptep. This prevents huge_pmd_unshare from > + * being called elsewhere and making the ptep no longer valid. > * > * ptep could have already be assigned via huge_pte_offset. That > * is OK, as huge_pte_alloc will return the same value unless >
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index f7a5b5124d8a..a32031e751d1 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -419,9 +419,10 @@ hugetlb_vmdelete_list(struct rb_root_cached *root, pgoff_t start, pgoff_t end, * In this case, we first scan the range and release found pages. * After releasing pages, hugetlb_unreserve_pages cleans up region/reserve * maps and global counts. Page faults can not race with truncation - * in this routine. hugetlb_no_page() holds i_mmap_rwsem and prevents - * page faults in the truncated range by checking i_size. i_size is - * modified while holding i_mmap_rwsem. + * in this routine. hugetlb_no_page() prevents page faults in the + * truncated range. It checks i_size before allocation, and again after + * with the page table lock for the page held. The same lock must be + * acquired to unmap a page. * hole punch is indicated if end is not LLONG_MAX * In the hole punch case we scan the range and release found pages. * Only when releasing a page is the associated region/reserve map @@ -451,16 +452,8 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart, u32 hash = 0; index = folio->index; - if (!truncate_op) { - /* - * Only need to hold the fault mutex in the - * hole punch case. This prevents races with - * page faults. Races are not possible in the - * case of truncation. - */ - hash = hugetlb_fault_mutex_hash(mapping, index); - mutex_lock(&hugetlb_fault_mutex_table[hash]); - } + hash = hugetlb_fault_mutex_hash(mapping, index); + mutex_lock(&hugetlb_fault_mutex_table[hash]); /* * If folio is mapped, it was faulted in after being @@ -504,8 +497,7 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart, } folio_unlock(folio); - if (!truncate_op) - mutex_unlock(&hugetlb_fault_mutex_table[hash]); + mutex_unlock(&hugetlb_fault_mutex_table[hash]); } folio_batch_release(&fbatch); cond_resched(); @@ -543,8 +535,8 @@ static void hugetlb_vmtruncate(struct inode *inode, loff_t offset) BUG_ON(offset & ~huge_page_mask(h)); pgoff = offset >> PAGE_SHIFT; - i_mmap_lock_write(mapping); i_size_write(inode, offset); + i_mmap_lock_write(mapping); if (!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root)) hugetlb_vmdelete_list(&mapping->i_mmap, pgoff, 0, ZAP_FLAG_DROP_MARKER); @@ -703,11 +695,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, /* addr is the offset within the file (zero based) */ addr = index * hpage_size; - /* - * fault mutex taken here, protects against fault path - * and hole punch. inode_lock previously taken protects - * against truncation. - */ + /* mutex taken here, fault path and hole punch */ hash = hugetlb_fault_mutex_hash(mapping, index); mutex_lock(&hugetlb_fault_mutex_table[hash]); diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 9a72499486c1..70bc7f867bc0 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -5575,18 +5575,17 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, } /* - * We can not race with truncation due to holding i_mmap_rwsem. - * i_size is modified when holding i_mmap_rwsem, so check here - * once for faults beyond end of file. + * Use page lock to guard against racing truncation + * before we get page_table_lock. */ - size = i_size_read(mapping->host) >> huge_page_shift(h); - if (idx >= size) - goto out; - retry: new_page = false; page = find_lock_page(mapping, idx); if (!page) { + size = i_size_read(mapping->host) >> huge_page_shift(h); + if (idx >= size) + goto out; + /* Check for page in userfault range */ if (userfaultfd_missing(vma)) { ret = hugetlb_handle_userfault(vma, mapping, idx, @@ -5677,6 +5676,10 @@ 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)) @@ -5785,10 +5788,8 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma, /* * Acquire i_mmap_rwsem before calling huge_pte_alloc and hold - * until finished with ptep. This serves two purposes: - * 1) It prevents huge_pmd_unshare from being called elsewhere - * and making the ptep no longer valid. - * 2) It synchronizes us with i_size modifications during truncation. + * until finished with ptep. This prevents huge_pmd_unshare from + * being called elsewhere and making the ptep no longer valid. * * ptep could have already be assigned via huge_pte_offset. That * is OK, as huge_pte_alloc will return the same value unless
Commit c0d0381ade79 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing synchronization") added code to take i_mmap_rwsem in read mode for the duration of fault processing. The use of i_mmap_rwsem to prevent fault/truncate races depends on this. However, this has been shown to cause performance/scaling issues. As a result, that code will be reverted. Since the use i_mmap_rwsem to address page fault/truncate races depends on this, it must also be reverted. In a subsequent patch, code will be added to detect the fault/truncate race and back out operations as required. Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> --- fs/hugetlbfs/inode.c | 30 +++++++++--------------------- mm/hugetlb.c | 23 ++++++++++++----------- 2 files changed, 21 insertions(+), 32 deletions(-)