possible lock_page fix for Andrea's nopage vs invalidate race?

Hi,

Just like to get some thoughts on another possible approach to this problem, and whether my changelog and implementation actually capture the problem. This fix is actually something Andrea had proposed, so credit really goes to him.

I suppose we should think about fixing it some day?

Quoted 1 line · Nick Piggin

I suppose we should think about fixing it some day?

I was thinking about this every few days too, but I already submitted two fixes and I got somewhat contradictory reviews of them, so I wasn't sure what to do given that for mainline it's mostly a DoS because the VM lacks the proper bugchecks in the objrmap layer to autodetect the leak (the bugchecks I'm talking about only exists only in the sles9 VM, Hugh removed them while merging objrmap into mainline, and the fact they existed in sles9 is why we noticed and tracked down this leak). We already fixed the bug in sles9 a while ago with my second fix, but I obviously agree we have to fix it in mainline as well some day too, infact I wouldn't mind to add the bugchecks too to be sure something like this doesn't go unnoticed again (especially now that in sles10 we're in VM sync with mainline).

I really appreciate this third way being implemented. It looks quite nice. Great work.

Just like to get some thoughts on another possible approach to this problem, and whether my changelog and implementation actually capture

the problem. This fix is actually something Andrea had proposed, so credit really goes to him.

I suppose we should think about fixing it some day?

Fix the race between invalidate_inode_pages and do_no_page.

Andrea Arcangeli identified a subtle race between invalidation of pages from pagecache with userspace mappings, and do_no_page.

The issue is that invalidation has to shoot down all mappings to the page, before it can be discarded from the pagecache. Between shooting down ptes to a particular page, and actually dropping the struct page from the pagecache, do_no_page from any process might fault on that page and establish a new mapping to the page just before it gets discarded from the pagecache.

The most common case where such invalidation is used is in file truncation. This case was catered for by doing a sort of open-coded seqlock between the file's i_size, and its truncate_count.

Truncation will decrease i_size, then increment truncate_count before unmapping userspace pages; do_no_page will read truncate_count, then find the page if it is within i_size, and then check truncate_count under the page table lock and back out and retry if it had subsequently been changed (ptl will serialise against unmapping, and ensure a potentially updated truncate_count is actually visible).

Complexity and documentation issues aside, the locking protocol fails in the case where we would like to invalidate pagecache inside i_size. do_no_page can come in anytime and filemap_nopage is not aware of the invalidation in progress (as it is when it is outside i_size). The end result is that dangling (->mapping == NULL) pages that appear to be from a particular file may be mapped into userspace with nonsense data. Valid mappings to the same place will see a different page.

Andrea implemented two working fixes, one using a real seqlock, another using a page->flags bit. He also proposed using the page lock in do_no_page, but that was initially considered too heavyweight. However, it is not a global or per-file lock, and the page cacheline is modified in do_no_page to increment _count and _mapcount anyway, so a further modification should not be a large performance hit. Scalability is not an issue.

This patch implements this latter approach. ->nopage implementations return with the page locked if it is possible for their underlying file to be invalidated (in that case, they must set a special vm_flags bit to indicate so). do_no_page only unlocks the page after setting up the mapping completely. invalidation is excluded because it holds the page lock during invalidation of each page.

This allows significant simplifications in do_no_page.

kbuild performance is, surprisingly, maybe slightly improved:

2xP4 Xeon 3.6GHz with HT:
kbuild -j8 in shmfs
original:
505.16user 28.59system 2:15.87elapsed 392%CPU
505.08user 28.28system 2:15.81elapsed 392%CPU
504.16user 29.28system 2:15.85elapsed 392%CPU

patched:
502.35user 26.86system 2:14.77elapsed 392%CPU
501.89user 27.15system 2:14.86elapsed 392%CPU
502.22user 27.01system 2:14.87elapsed 392%CPU

kbuild -j8 in ext3
original:
505.65user 27.72system 2:15.75elapsed 392%CPU
506.38user 26.73system 2:16.38elapsed 390%CPU
507.00user 26.21system 2:15.76elapsed 392%CPU

patched:
501.03user 28.67system 2:14.98elapsed 392%CPU
500.84user 29.34system 2:14.99elapsed 392%CPU
501.18user 28.76system 2:15.05elapsed 392%CPU

Signed-off-by Nick Piggin <[email protected]>

Index: linux-2.6/include/linux/mm.h
===================================================================

include/linux/mm.h +0 -0

 struct mmu_gather;

Index: linux-2.6/mm/filemap.c
===================================================================

mm/filemap.c +4 -16

@@ -1279,6 +1279,8 @@ struct page *filemap_nopage(struct vm_ar
 	unsigned long size, pgoff;
 	int did_readaround = 0, majmin = VM_FAULT_MINOR;
 
+	BUG_ON(!(area->vm_flags & VM_CAN_INVLD));
+
 	pgoff = ((address-area->vm_start) >> PAGE_CACHE_SHIFT) + area->vm_pgoff;
 
 retry_all:

@@ -1303,7 +1305,7 @@ retry_all:
 	 * Do we have something in the page cache already?
 	 */
 retry_find:
-	page = find_get_page(mapping, pgoff);
+	page = find_lock_page(mapping, pgoff);
 	if (!page) {
 		unsigned long ra_pages;
 

@@ -1337,7 +1339,7 @@ retry_find:
 				start = pgoff - ra_pages / 2;
 			do_page_cache_readahead(mapping, file, start, ra_pages);
 		}
-		page = find_get_page(mapping, pgoff);
+		page = find_lock_page(mapping, pgoff);
 		if (!page)
 			goto no_cached_page;
 	}

@@ -1399,30 +1401,6 @@ page_not_uptodate:
 		majmin = VM_FAULT_MAJOR;
 		inc_page_state(pgmajfault);
 	}
-	lock_page(page);
-
-	/* Did it get unhashed while we waited for it? */
-	if (!page->mapping) {
-		unlock_page(page);
-		page_cache_release(page);
-		goto retry_all;
-	}
-
-	/* Did somebody else get it up-to-date? */
-	if (PageUptodate(page)) {
-		unlock_page(page);
-		goto success;
-	}

mm/filemap.c +0 -4

 	/*
 	 * Umm, take care of errors if the page isn't up-to-date.

@@ -1430,20 +1408,6 @@ page_not_uptodate:
 	 * because there really aren't any performance issues here
 	 * and we need to check for errors.
 	 */
-	lock_page(page);
-
-	/* Somebody truncated the page on us? */
-	if (!page->mapping) {

mm/filemap.c +1 -1

 }
-
 EXPORT_SYMBOL(filemap_nopage);
 
 static struct page * filemap_getpage(struct file *file, unsigned long pgoff,

@@ -1641,6 +1604,7 @@ EXPORT_SYMBOL(filemap_populate);
 struct vm_operations_struct generic_file_vm_ops = {
 	.nopage		= filemap_nopage,
 	.populate	= filemap_populate,
+	.vm_flags	= VM_CAN_INVLD,
 };
 
 /* This is used for a general mmap of a disk file */

Index: linux-2.6/mm/memory.c
===================================================================

mm/memory.c +0 -2

 again:
 	restart_addr = vma->vm_truncate_count;
 	if (is_restart_addr(restart_addr) && start_addr < restart_addr) {

@@ -1707,17 +1714,8 @@ void unmap_mapping_range(struct address_
 
 	spin_lock(&mapping->i_mmap_lock);
 
-	/* serialize i_size write against truncate_count write */
-	smp_wmb();

mm/memory.c +1 -0

 	if (unlikely(is_restart_addr(mapping->truncate_count))) {
 		if (mapping->truncate_count == 0)
 			reset_vma_truncate_counts(mapping);

@@ -1729,6 +1727,7 @@ void unmap_mapping_range(struct address_
 		unmap_mapping_range_tree(&mapping->i_mmap, &details);
 	if (unlikely(!list_empty(&mapping->i_mmap_nonlinear)))
 		unmap_mapping_range_list(&mapping->i_mmap_nonlinear, &details);
+
 	spin_unlock(&mapping->i_mmap_lock);
 }
 EXPORT_SYMBOL(unmap_mapping_range);

mm/memory.c +0 -6

 	pte_t entry;
-	unsigned int sequence = 0;
 	int ret = VM_FAULT_MINOR;
 	int anon = 0;
 
 	pte_unmap(page_table);
 	BUG_ON(vma->vm_flags & VM_PFNMAP);
 
-	if (vma->vm_file) {
-		mapping = vma->vm_file->f_mapping;
-		sequence = mapping->truncate_count;
-		smp_rmb(); /* serializes i_size against truncate_count */
-	}

mm/memory.c +0 -3

@@ -2089,19 +2075,6 @@ retry:
 	}
 
 	page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
-	/*
-	 * For a file-backed vma, someone could have truncated or otherwise
-	 * invalidated this page.  If unmap_mapping_range got called,

mm/memory.c +3 -1

 	return ret;
+
 oom:
 	page_cache_release(new_page);
-	return VM_FAULT_OOM;
+	ret = VM_FAULT_OOM;
+	goto out;
 }
 
 /*

Index: linux-2.6/mm/shmem.c
===================================================================

mm/shmem.c +2 -1

 	idx = (address - vma->vm_start) >> PAGE_SHIFT;
 	idx += vma->vm_pgoff;
 	idx >>= PAGE_CACHE_SHIFT - PAGE_SHIFT;
 	if (((loff_t) idx << PAGE_CACHE_SHIFT) >= i_size_read(inode))
 		return NOPAGE_SIGBUS;
 
-	error = shmem_getpage(inode, idx, &page, SGP_CACHE, type);
+	error = shmem_getpage(inode, idx, &page, SGP_NOPAGE, type);
 	if (error)
 		return (error == -ENOMEM)? NOPAGE_OOM: NOPAGE_SIGBUS;
 

@@ -2230,6 +2235,7 @@ static struct vm_operations_struct shmem
 	.set_policy     = shmem_set_policy,
 	.get_policy     = shmem_get_policy,
 #endif
+	.vm_flags	= VM_CAN_INVLD,
 };
 
 

Index: linux-2.6/fs/ncpfs/mmap.c
===================================================================

fs/ncpfs/mmap.c +1 -0

@@ -100,6 +100,7 @@ static struct page* ncp_file_mmap_nopage
 static struct vm_operations_struct ncp_file_mmap =
 {
 	.nopage	= ncp_file_mmap_nopage,
+	.vm_flags = VM_CAN_INVLD,
 };
 
 

Index: linux-2.6/fs/ocfs2/mmap.c
===================================================================

fs/ocfs2/mmap.c +1 -0

@@ -78,6 +78,7 @@ out:
 
 static struct vm_operations_struct ocfs2_file_vm_ops = {
 	.nopage = ocfs2_nopage,
+	.vm_flags = VM_CAN_INVLD,
 };
 
 int ocfs2_mmap(struct file *file, struct vm_area_struct *vma)

Index: linux-2.6/fs/xfs/linux-2.6/xfs_file.c
===================================================================

fs/xfs/linux-2.6/xfs_file.c +2 -0

@@ -634,6 +634,7 @@ const struct file_operations xfs_dir_fil
 static struct vm_operations_struct xfs_file_vm_ops = {
 	.nopage		= filemap_nopage,
 	.populate	= filemap_populate,
+	.vm_flags	= VM_CAN_INVLD,
 };
 
 #ifdef CONFIG_XFS_DMAPI

@@ -643,5 +644,6 @@ static struct vm_operations_struct xfs_d
 #ifdef HAVE_VMOP_MPROTECT
 	.mprotect	= xfs_vm_mprotect,
 #endif
+	.vm_flags	= VM_CAN_INVLD,
 };
 #endif /* CONFIG_XFS_DMAPI */

Index: linux-2.6/mm/mmap.c
===================================================================

mm/memory.c static int do_no_page(struct mm_struct * · line 2061

Quoted 3 lines

mm/memory.c +0 -0

 	/*
 	 * Should we do an early C-O-W break?

Somewhere below here you're missing a hunk to deal with a failed page_mkwrite, needing to unlock_page(locked_page). We don't have an example of a page_mkwrite in tree at present, but it seems reasonable to suppose that we not it should unlock the page.

Hmm, David Howells has an afs_file_page_mkwrite which sits waiting for an FsMisc page flag to be cleared: might that deadlock with the page lock held? If so, it may need to unlock and relock the page, rechecking for truncation.

Hmmm, page_mkwrite when called from do_wp_page would not expect to be holding page lock: we don't want it called with in one case and without in the other. Maybe do_no_page needs to unlock_page before calling page_mkwrite, lock_page after, and check page->mapping when VM_NOPAGE_LOCKED??

(David, I've added you to CC because way down below there's an issue of interaction with page_mkwrite.)

Quoted 2 lines · Nick Piggin

Just like to get some thoughts on another possible approach to this problem, and whether my changelog and implementation actually capture

Good changelog, promising implementation.

(David, I've added you to CC because way down below there's an issue of interaction with page_mkwrite.)

Quoted 7 lines

Complexity and documentation issues aside, the locking protocol fails in the case where we would like to invalidate pagecache inside i_size. do_no_page can come in anytime and filemap_nopage is not aware of the invalidation in progress (as it is when it is outside i_size). The end result is that dangling (->mapping == NULL) pages that appear to be from a particular file may be mapped into userspace with nonsense data. Valid mappings to the same place will see a different page.

I think it was some NFS or cluster FS case that showed the problem, Andrea would know. But Badari's MADV_REMOVE, punching a hole within a file, has added another case which the i_size/truncate_count technique cannot properly guard against.

Quoted 9 lines

Andrea implemented two working fixes, one using a real seqlock, another using a page->flags bit. He also proposed using the page lock in do_no_page, but that was initially considered too heavyweight. However, it is not a global or per-file lock, and the page cacheline is modified in do_no_page to increment _count and _mapcount anyway, so a further modification should not be a large performance hit. Scalability is not an issue.

Scalability is not an issue, that's nice - but I don't see how you arrive at that certainty. Obviously the per-page lock means it's less of a scalability issue than global or per-file; and the fact that tmpfs' shmem_getpage has always taken page lock internally adds good evidence that it can't be too bad.

But I worry a little about shared libraries, and suspect that there will be cases degraded by the additional locking - perhaps benchmarks (with processes falling into lockstep) rather than real-life loads. I think it's fair to say "Scalability is unlikely to be much of an issue".

Quoted 9 lines

This patch implements this latter approach. ->nopage implementations return with the page locked if it is possible for their underlying file to be invalidated (in that case, they must set a special vm_flags bit to indicate so). do_no_page only unlocks the page after setting up the mapping completely. invalidation is excluded because it holds the page lock during invalidation of each page.

Quoted 3 lines

This allows significant simplifications in do_no_page.

kbuild performance is, surprisingly, maybe slightly improved:

Emphasis on maybe. It would be surprising, and your ext3 system times go the other way, and I find the reverse of what you find:
slightly regressed, say ~0.5%, on kbuilds and some lmbenchs.

I didn't care for "INVLD", and gather now that it's being changed to "INVALIDATE" (I'd have suggested "INVAL"). But actually, I'd rather a name that says what's actually being assumed: VM_NOPAGE_LOCKED?

With "revoke" in the air, I suspect that we're going to want to be able to invalidate the pages of _any_ mapping, whether the driver locks them in its nopage or not. (Or am I thereby just encouraging the idea of a racy revoke?)

include/linux/mm.h struct vm_operations_struct { · line 214

Quoted 11 lines

#ifndef VM_STACK_DEFAULT_FLAGS /* arch can override this */
#define VM_STACK_DEFAULT_FLAGS VM_DATA_DEFAULT_FLAGS

include/linux/mm.h +1 -0

@@ -205,6 +210,7 @@ struct vm_operations_struct {
 	struct mempolicy *(*get_policy)(struct vm_area_struct *vma,
 					unsigned long addr);
 #endif
+	unsigned long vm_flags; /* vm_flags to copy into any mapping vmas */
 };

I suppose this is quite efficient, but I find it confusing. We have lots and lots of drivers already setting vm_flags in their mmap methods, now you add an alternative way of doing the same thing. Can't you just set VM_NOPAGE_LOCKED in the relevant mmap methods? Or did you try it that way and it worked out messy?

Quoted 8 lines

include/linux/mm.h +0 -0

 struct mmu_gather;

Index: linux-2.6/mm/filemap.c
===================================================================

mm/filemap.c +0 -0

@@ -1279,6 +1279,8 @@ struct page *filemap_nopage(struct vm_ar

mm/filemap.c page_not_uptodate: · line 1427

Quoted 3 lines

mm/filemap.c +0 -0

@@ -1462,7 +1426,6 @@ page_not_uptodate:
 	page_cache_release(page);
 	return NULL;

But here I think you're missing something: the wait_on_page_locked after ->readpage needs to become a lock_page before going to success? with unlock_page if it doesn't.

Quoted 8 lines

Index: linux-2.6/mm/memory.c
===================================================================

mm/memory.c +0 -0

@@ -1577,6 +1577,13 @@ static int unmap_mapping_range_vma(struc
 	unsigned long restart_addr;
 	int need_break;
 

mm/memory.c static int unmap_mapping_range_vma(struc · line 1586

Quoted 7 lines

mm/memory.c +7 -0

+	/*
+	 * files that support invalidating or truncating portions of the
+	 * file from under mmaped areas must set the VM_CAN_INVLD flag, and
+	 * have their .nopage function return the page locked.
+	 */
+	BUG_ON(!(vma->vm_flags & VM_CAN_INVLD));
+

I think we shall end up wanting to apply unmap_mapping_range even to "unlocked nopage" vmas (the revoke idea) - unless we decide we have to make every nopage vma do the locking.

Would the BUG_ON be better as a WARN_ON, or nothing at all? It'll give trouble until out-of-tree filesystems/drivers are updated; or do we want to give them active trouble there, I'm not sure?

mm/memory.c static int do_no_page(struct mm_struct * · line 2042

Quoted 7 lines

mm/memory.c +1 -2

@@ -2040,36 +2039,23 @@ static int do_no_page(struct mm_struct *
 		int write_access)
 {
 	spinlock_t *ptl;
-	struct page *new_page;
-	struct address_space *mapping = NULL;
+	struct page *new_page, *old_page;

I think it's much clearer to call it "locked_page" than "old_page", particularly when you see it alonside Peter's "dirty_page".

mm/memory.c static int do_no_page(struct mm_struct * · line 2058

Quoted 1 line

mm/memory.c +1 -0

+	BUG_ON(vma->vm_flags & VM_CAN_INVLD && !PageLocked(new_page));

Maybe

	if (vma->vm_flags & VM_NOPAGE_LOCKED) {
		locked_page = new_page;
		BUG_ON(!PageLocked(locked_page));
	} else
		locked_page = NULL;

But what I hate about this do_no_page is that sometimes we're going through it with the page locked, and sometimes we're going through it with the page not locked. Now I've not noticed any actual problem from that (aside from where page_mkwrite fails), and it is well-defined which case is which, but it is confusing and does make do_no_page harder to audit at any time.

(I did toy with a separate do_no_page_locked, and nopage_locked methods for the filesystems; but duplicating so much code doesn't really solve anything.)

And when you factor in Peter's dirty_page stuff, it's a nuisance:
because he has had to get_page(dirty_page) then put_page(dirty_page), in case page already got freed by vmscan after ptl dropped: which is redundant if the page is locked throughout, but you can't rely on that because (for a while at least) some fs'es won't set VM_NOPAGE_LOCKED.

How about

	if (vma->vm_flags & VM_NOPAGE_LOCKED)
		BUG_ON(!PageLocked(new_page));
	else
		lock_page(new_page);
	locked_page = new_page;

?

And then proceed through the rest of do_no_page sure in the knowledge that we have the page locked, simplifying whatever might be simplified by that (removing Peter's get_page,put_page at least). I can see this adds a little overhead to some less important cases, but it does make the rules much easier to grasp.

mm/memory.c static int do_no_page(struct mm_struct * · line 2061

Quoted 5 lines

mm/memory.c +0 -0

 	/*
 	 * Should we do an early C-O-W break?

Somewhere below here you're missing a hunk to deal with a failed page_mkwrite, needing to unlock_page(locked_page). We don't have an example of a page_mkwrite in tree at present, but it seems reasonable to suppose that we not it should unlock the page.

Hmm, David Howells has an afs_file_page_mkwrite which sits waiting for an FsMisc page flag to be cleared: might that deadlock with the page lock held? If so, it may need to unlock and relock the page, rechecking for truncation.

Hmmm, page_mkwrite when called from do_wp_page would not expect to be holding page lock: we don't want it called with in one case and without in the other. Maybe do_no_page needs to unlock_page before calling page_mkwrite, lock_page after, and check page->mapping when VM_NOPAGE_LOCKED??

mm/shmem.c enum sgp_type { · line 84

Quoted 10 lines

Index: linux-2.6/mm/shmem.c
===================================================================

mm/shmem.c +1 -0

@@ -80,6 +80,7 @@ enum sgp_type {
 	SGP_READ,	/* don't exceed i_size, don't allocate page */
 	SGP_CACHE,	/* don't exceed i_size, may allocate page */
 	SGP_WRITE,	/* may exceed i_size, may allocate page */
+	SGP_NOPAGE,	/* same as SGP_CACHE, return with page locked */
 };

I don't think you need to add another type for this, SGP_CACHE should do.

Perhaps you avoided that because it's also used by shmem_populate. But another point I want to make is that you do need to update filemap_populate, shmem_populate, install_page and whatever to make use the same locked page fix: they've been relying on the i_size and page->mapping checks, which are not quite enough, isn't that right? (now my grasp of the race has fallen out of my left ear, and I'd better finish this mail before regrasping it)

Quoted 5 lines

mm/shmem.c +0 -0

 static int shmem_getpage(struct inode *inode, unsigned long idx,

@@ -1211,8 +1212,10 @@ repeat:
 	}

done:

	if (*pagep != filepage) {

mm/shmem.c repeat: · line 1218

Quoted 5 lines

mm/shmem.c +0 -1

-		unlock_page(filepage);

		*pagep = filepage;

mm/shmem.c +3 -0

+		if (sgp != SGP_NOPAGE)
+			unlock_page(filepage);
+

You've inserted that blank line just to upset me.

Quoted 8 lines

Index: linux-2.6/mm/mmap.c
===================================================================

mm/mmap.c +0 -0

@@ -1089,6 +1089,9 @@ munmap_back:
 			goto free_vma;
 	}
 

mm/mmap.c munmap_back: · line 1094

Quoted 3 lines

mm/mmap.c +3 -0

+	if (vma->vm_ops)
+		vma->vm_flags |= vma->vm_ops->vm_flags;
+

Mmm, I'd prefer not to have this additional way of setting vm_flags.

Quoted 5 lines · Hugh Dickins

I suppose this is quite efficient, but I find it confusing. We have lots and lots of drivers already setting vm_flags in their mmap methods, now you add an alternative way of doing the same thing. Can't you just set VM_NOPAGE_LOCKED in the relevant mmap methods? Or did you try it that way and it worked out messy?

Generic pagecache doesn't have an mmap method, which is where I stopped looking. I guess you could add the |= to filemap_nopage, but that's much uglier.

I don't find it at all confusing, just maybe a bit of a violation because the structure is technically only for "ops".

Quoted 4 lines · Nick Piggin

Generic pagecache doesn't have an mmap method, which is where I stopped looking. I guess you could add the |= to filemap_nopage, but that's much uglier.

Hmm, I guess adding a new mmap method solely to set that flag would actually be cleaner. And it would allow any filesystems that override .nopage bug end up calling filemap_nopage could equivalently override their mmap but still call filemap_mmap.

Quoted 5 lines · Nick Piggin

Quoted 3 lines · Nick Piggin

Generic pagecache doesn't have an mmap method, which is where I stopped looking. I guess you could add the |= to filemap_nopage, but that's much uglier.

You can't |= vm_flags in nopage, mmap_sem isn't exclusive there.

But what's the matter with generic_file_mmap?

Quoted 6 lines · Hugh Dickins

Hmmm, page_mkwrite when called from do_wp_page would not expect to be holding page lock: we don't want it called with in one case and without in the other. Maybe do_no_page needs to unlock_page before calling page_mkwrite, lock_page after, and check page->mapping when VM_NOPAGE_LOCKED??

That's pretty foul. I'll take a bit of a look. Is it really a problem to call in either state, if it is well documented? (we could even send a flag down if needed). I thought filesystem code loved this kind of spaghetti locking?

I don't think ->populate has ever particularly troubled itself with these kinds of theoretical races. I was really hoping to fix linear pagecache first before getting bogged down with nonlinear.

After thinking about it a bit more, I think I've found my filemap_nopage wanting. Suppose i_size is shrunk and the page truncated before the first find_lock_page. OK, no we'll allocate a new page, add it to the pagecache, and do a ->readpage().

Quoted 8 lines · Nick Piggin

Quoted 5 lines · Hugh Dickins

Hmmm, page_mkwrite when called from do_wp_page would not expect to be holding page lock: we don't want it called with in one case and without in the other. Maybe do_no_page needs to unlock_page before calling page_mkwrite, lock_page after, and check page->mapping when VM_NOPAGE_LOCKED??

Quoted 4 lines

That's pretty foul. I'll take a bit of a look. Is it really a problem to call in either state, if it is well documented? (we could even send a flag down if needed). I thought filesystem code loved this kind of spaghetti locking?

Agreed foul. David's helpful mail reassures not an immediate problem, but I'm pretty sure other future uses of page_mkwrite would need to know if the page is held locked or not. Yes, could be done by a flag, though that's not pretty (gives the ->page_mkwrite implementation much the same schizophrenia as I was disliking here in do_no_page).

Quoted 5 lines

I don't think ->populate has ever particularly troubled itself with these kinds of theoretical races. I was really hoping to fix linear pagecache first before getting bogged down with nonlinear.

install_page has had mapping & i_size check for quite a while, but perhaps by theoretical races you mean Andrea's invalidate case. The nonlinear case is much less a concern than MAP_POPULATE (though I don't know if anyone really uses that).

Quoted 6 lines

After thinking about it a bit more, I think I've found my filemap_nopage wanting. Suppose i_size is shrunk and the page truncated before the first find_lock_page. OK, no we'll allocate a new page, add it to the pagecache, and do a ->readpage().

I've got a bit lost between merges against different trees, I'll let you sort that one out.

I wonder if we should have the i_size check (under the page lock) in do_no_page or down in the ->nopage implementations?