hugetlb: Fix up filesystem quota accounting

mm/hugetlb.c +3 -1

@@ -685,7 +685,17 @@ void __unmap_hugepage_range(struct vm_area_struct *vma, unsigned long start,
 	flush_tlb_range(vma, start, end);
 	list_for_each_entry_safe(page, tmp, &page_list, lru) {
 		list_del(&page->lru);
-		put_page(page);
+		if (put_page_testzero(page)) {
+			/*
+			 * When releasing the last reference to a page we must

Quoted 10 lines · Adam Litke

mm/hugetlb.c +3 -1

@@ -685,7 +685,17 @@ void __unmap_hugepage_range(struct vm_area_struct *vma, unsigned long start,
 	flush_tlb_range(vma, start, end);
 	list_for_each_entry_safe(page, tmp, &page_list, lru) {
 		list_del(&page->lru);
-		put_page(page);
+		if (put_page_testzero(page)) {
+			/*
+			 * When releasing the last reference to a page we must

mm/hugetlb.c void __unmap_hugepage_range(struct vm_area_struct *vma, unsigned long start, · line 700

Quoted 10 lines

mm/hugetlb.c +8 -0

+			 * credit the quota.  For MAP_PRIVATE pages this occurs
+			 * when the last PTE is cleared, for MAP_SHARED pages
+			 * this occurs when the file is truncated.
+			 */
+			VM_BUG_ON(PageMapping(page));
+			hugetlb_put_quota(vma->vm_file->f_mapping);
+			free_huge_page(page);
+		}
 	}
 }

That's a pretty good mechanism to use.

This particular nugget is for MAP_PRIVATE pages only, right? The shared ones should have another ref out on them for the 'mapping' too, so won't get released at unmap, right?

It isn't obvious from the context here, but how did free_huge_page() get called for these pages before?

Can you use free_pages_check() here to get some more comprehensive page tests? Just a cursory look at it makes me think that it should work.

mm/hugetlb.c +1 -0

index eaade8c..5fc075e 100644
@@ -769,6 +769,7 @@ static int hugetlb_no_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	struct page *page;
 	struct address_space *mapping;
 	pte_t new_pte;
+	int shared_page = vma->vm_flags & VM_SHARED;
 
 	mapping = vma->vm_file->f_mapping;
 	idx = ((address - vma->vm_start) >> HPAGE_SHIFT)

mm/hugetlb.c +2 -2

 		if (!page) {
-			hugetlb_put_quota(mapping, 1);
+			if (!shared_page)
+				hugetlb_put_quota(mapping, 1);
 			ret = VM_FAULT_OOM;
 			goto out;
 		}
 		clear_huge_page(page, address);
 
-		if (vma->vm_flags & VM_SHARED) {

backout:

mm/hugetlb.c +4 -1

 	spin_unlock(&mm->page_table_lock);
-	hugetlb_put_quota(mapping, 1);
+	if (!shared_page)
+		hugetlb_put_quota(mapping, 1);
 	unlock_page(page);
 	put_page(page);
 	goto out;

@@ -1144,6 +1147,8 @@ int hugetlb_reserve_pages(struct inode *inode, long from, long to)
 	if (chg < 0)
 		return chg;
 
+	if (hugetlb_get_quota(inode->i_mapping, chg))
+		return -ENOSPC;
 	ret = hugetlb_acct_memory(chg);
 	if (ret < 0)
 		return ret;

Quoted 15 lines · Dave Hansen

Quoted 4 lines · Adam Litke

mm/hugetlb.c +0 -0

@@ -685,7 +685,17 @@ void __unmap_hugepage_range(struct vm_area_struct *vma, unsigned long start,
 	flush_tlb_range(vma, start, end);

mm/hugetlb.c void __unmap_hugepage_range(struct vm_area_struct *vma, unsigned long start, · line 694

Quoted 10 lines

mm/hugetlb.c +7 -1

 	list_for_each_entry_safe(page, tmp, &page_list, lru) {
 		list_del(&page->lru);
-		put_page(page);
+		if (put_page_testzero(page)) {
+			/*
+			 * When releasing the last reference to a page we must
+			 * credit the quota.  For MAP_PRIVATE pages this occurs
+			 * when the last PTE is cleared, for MAP_SHARED pages
+			 * this occurs when the file is truncated.
+			 */

Quoted 9 lines

mm/hugetlb.c void __unmap_hugepage_range(struct vm_area_struct *vma, unsigned long start, · line 700

Quoted 6 lines

mm/hugetlb.c +4 -0

+			VM_BUG_ON(PageMapping(page));
+			hugetlb_put_quota(vma->vm_file->f_mapping);
+			free_huge_page(page);
+		}
 	}
 }

That's a pretty good mechanism to use.

Quoted 3 lines

This particular nugget is for MAP_PRIVATE pages only, right? The shared ones should have another ref out on them for the 'mapping' too, so won't get released at unmap, right?

Yep that's right. Shared pages are released by truncate_hugepages() when the ref for the mapping is dropped.

Quoted 2 lines

It isn't obvious from the context here, but how did free_huge_page() get called for these pages before?

put_page() calls put_compound_page() which looks up the page dtor (free_huge_page) and calls it.

Quoted 2 lines

Can you use free_pages_check() here to get some more comprehensive page tests? Just a cursory look at it makes me think that it should work.

Yeah, that should work but I am a little hesitant to use free_pages_check() because it may happen to "just work." None of the other hugetlb code concerns itself with all of those page flags so it seems a little out of place to start caring only here.

Quoted 3 lines · Dave Hansen

This particular nugget is for MAP_PRIVATE pages only, right? The shared ones should have another ref out on them for the 'mapping' too, so won't get released at unmap, right?

Yep that's right. Shared pages are released by truncate_hugepages() when the ref for the mapping is dropped.

Quoted 13 lines · Adam Litke

mm/hugetlb.c +1 -0

index eaade8c..5fc075e 100644
@@ -769,6 +769,7 @@ static int hugetlb_no_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	struct page *page;
 	struct address_space *mapping;
 	pte_t new_pte;
+	int shared_page = vma->vm_flags & VM_SHARED;
 
 	mapping = vma->vm_file->f_mapping;
 	idx = ((address - vma->vm_start) >> HPAGE_SHIFT)

mm/hugetlb.c retry: · line 791

Quoted 8 lines

mm/hugetlb.c +2 -1

@@ -784,23 +785,24 @@ retry:
 		size = i_size_read(mapping->host) >> HPAGE_SHIFT;
 		if (idx >= size)
 			goto out;
-		if (hugetlb_get_quota(mapping, 1))
+		/* Shared pages are quota-accounted at reservation/mmap time */
+		if (!shared_page && hugetlb_get_quota(mapping, 1))
 			goto out;
 		page = alloc_huge_page(vma, address);

Since alloc_huge_page() gets the VMA it could, in theory, be doing the accounting. The other user, hugetlb_cow(), seems to have a similar code path. But, it doesn't have to worry about shared_page, right? We can only have COWs on MAP_PRIVATE.

I'm just trying to find ways to future-proof the quotas since they already got screwed up once. The fewer call sites we have for them, the fewer places they can get screwed up. :)

Quoted 13 lines

mm/hugetlb.c +3 -2

 		if (!page) {
-			hugetlb_put_quota(mapping, 1);
+			if (!shared_page)
+				hugetlb_put_quota(mapping, 1);
 			ret = VM_FAULT_OOM;
 			goto out;
 		}
 		clear_huge_page(page, address);
 
-		if (vma->vm_flags & VM_SHARED) {
+		if (shared_page) {
 			int err;
 

mm/hugetlb.c retry: · line 808

Quoted 7 lines

mm/hugetlb.c +0 -1

 			err = add_to_page_cache(page, mapping, idx, GFP_KERNEL);
 			if (err) {
 				put_page(page);
-				hugetlb_put_quota(mapping, 1);
 				if (err == -EEXIST)
 					goto retry;
 				goto out;

To where was this quota put moved? Is it because we're in a fault path here, and shared pages don't modify quotas during faults, only at mmap/truncate() time now?

Quoted 22 lines

backout:

mm/hugetlb.c +4 -2

 	spin_unlock(&mm->page_table_lock);
-	hugetlb_put_quota(mapping, 1);
+	if (!shared_page)
+		hugetlb_put_quota(mapping, 1);
 	unlock_page(page);
 	put_page(page);
 	goto out;

@@ -1144,6 +1147,8 @@ int hugetlb_reserve_pages(struct inode *inode, long from, long to)
 	if (chg < 0)
 		return chg;
 
+	if (hugetlb_get_quota(inode->i_mapping, chg))
+		return -ENOSPC;
 	ret = hugetlb_acct_memory(chg);
 	if (ret < 0)
 		return ret;

@@ -1154,5 +1159,6 @@ int hugetlb_reserve_pages(struct inode *inode, long from, long to)
 void hugetlb_unreserve_pages(struct inode *inode, long offset, long freed)
 {
 	long chg = region_truncate(&inode->i_mapping->private_list, offset);
-	hugetlb_acct_memory(freed - chg);

mm/hugetlb.c int hugetlb_reserve_pages(struct inode *inode, long from, long to) · line 1164

Quoted 3 lines

mm/hugetlb.c +2 -0

+	hugetlb_put_quota(inode->i_mapping, (chg - freed));
+	hugetlb_acct_memory(-(chg - freed));
 }

Would it be any easier to just do all of the quota operations in _either_ truncate_hugepages() or in here? Could you skip the quota operation in truncate_hugepages()'s while() loop, and just put the quota for the entire region in hugetlb_unreserve_pages()?

void hugetlb_unreserve_pages(struct inode *inode, long offset, long already_freed) {

 	long total_truncated = region_truncate(&inode->i_mapping->private_list, offset);
	long newly_freed = total_truncated - already_freed;
	hugetlb_put_quota(inode->i_mapping, newly_freed);
	hugetlb_acct_memory(-newly_freed);
}

I do see several hugetlb_put_quota()/hugetlb_acct_memory() pairs next to each other. Do they deserve to be lumped together in one helper?

Quoted 9 lines

Since alloc_huge_page() gets the VMA it could, in theory, be doing the accounting. The other user, hugetlb_cow(), seems to have a similar code path. But, it doesn't have to worry about shared_page, right? We can only have COWs on MAP_PRIVATE.

I'm just trying to find ways to future-proof the quotas since they already got screwed up once. The fewer call sites we have for them, the fewer places they can get screwed up. :)

Yep. Originally I wanted to put the hugetlb_get_quota() call inside alloc_huge_page() but the devil is in the details. Failure to get quota needs to result in a SIGBUS whereas a standard allocation failure is OOM. Because of this, we'd still need special handling of the alloc_huge_page() return value. While that can be done easily enough, I didn't think it was worth it.

Quoted 5 lines · Adam Litke

Quoted 3 lines · Dave Hansen

This particular nugget is for MAP_PRIVATE pages only, right? The shared ones should have another ref out on them for the 'mapping' too, so won't get released at unmap, right?

Quoted 2 lines

Yep that's right. Shared pages are released by truncate_hugepages() when the ref for the mapping is dropped.

I think as a follow up patch, we should debit the quota in free_huge_page(), so you don't have to open code it like this and also consolidate calls to hugetlb_put_quota() in one place. It's cleaner that way.

Quoted 4 lines

I think as a follow up patch, we should debit the quota in free_huge_page(), so you don't have to open code it like this and also consolidate calls to hugetlb_put_quota() in one place. It's cleaner that way.

At free_huge_page() time, you can't associate the page with a struct address_space so it becomes hard to credit the proper filesystem. When freeing the page, page->mapping is no longer valid (even for shared pages).

But, I think what I'm realizing is that the free paths for shared vs. private are actually quite distinct. Even more now after your patches abolish using and actual put_page() (and the destructors) on private pages losing their last mapping.

I think it may make a lot of sense to have {alloc,free}_{private,shared}_huge_page(). It'll really help readability, and I _think_ it gives you a handy dandy place to add the different quota operations needed.

Quoted 5 lines · Dave Hansen

But, I think what I'm realizing is that the free paths for shared vs. private are actually quite distinct. Even more now after your patches abolish using and actual put_page() (and the destructors) on private pages losing their last mapping.

Quoted 4 lines

I think it may make a lot of sense to have {alloc,free}_{private,shared}_huge_page(). It'll really help readability, and I _think_ it gives you a handy dandy place to add the different quota operations needed.

Here is my version of re-factoring hugetlb_put_quota() into free_huge_page. Not exactly what Dave suggested, but at least consolidate quota credit in one place.