mm: tracking dirty pages -v5

I hacked up a new version last night.

Its now based on top of David's patches, Hugh's approach of using the MAP_PRIVATE protections instead of the MAP_SHARED seems far superior indeed.

Q: would it be feasable to do so for al shared mappings so we can remove the MAP_SHARED protections all together?

They survive my simple testing, but esp. the msync cleanup might need some more attention.

I post them now instead of after a little more testing because I'll not have much time the coming few days to do so, and hoarding them does nobody any good.

Peter

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

The attached patch adds a new VMA operation to notify a filesystem or other driver about the MMU generating a fault because userspace attempted to write to a page mapped through a read-only PTE.

This facility permits the filesystem or driver to:

(*) Implement storage allocation/reservation on attempted write, and so to
deal with problems such as ENOSPC more gracefully (perhaps by generating SIGBUS).

(*) Delay making the page writable until the contents have been written to a
backing cache. This is useful for NFS/AFS when using FS-Cache/CacheFS. It permits the filesystem to have some guarantee about the state of the cache.

(*) Account and limit number of dirty pages. This is one piece of the puzzle
needed to make shared writable mapping work safely in FUSE.

---

 include/linux/mm.h |    4 ++
 mm/memory.c        |   99 +++++++++++++++++++++++++++++++++++++++-------------
 mm/mmap.c          |   12 +++++-
 mm/mprotect.c      |   11 +++++-
 4 files changed, 98 insertions(+), 28 deletions(-)

index 1154684..cd3c2cf 100644
@@ -200,6 +200,10 @@ struct vm_operations_struct {
 	void (*close)(struct vm_area_struct * area);
 	struct page * (*nopage)(struct vm_area_struct * area, unsigned long address, int *type);
 	int (*populate)(struct vm_area_struct * area, unsigned long address, unsigned long len, pgprot_t prot, unsigned long pgoff, int nonblock);
+
+	/* notification that a previously read-only page is about to become
+	 * writable, if an error is returned it will cause a SIGBUS */
+	int (*page_mkwrite)(struct vm_area_struct *vma, struct page *page);
 #ifdef CONFIG_NUMA
 	int (*set_policy)(struct vm_area_struct *vma, struct mempolicy *new);
 	struct mempolicy *(*get_policy)(struct vm_area_struct *vma,

index e6ee123..6446c61 100644
@@ -1065,7 +1065,8 @@ munmap_back:
 	vma->vm_start = addr;
 	vma->vm_end = addr + len;
 	vma->vm_flags = vm_flags;
-	vma->vm_page_prot = protection_map[vm_flags & 0x0f];
+	vma->vm_page_prot = protection_map[vm_flags &
+				(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)];
 	vma->vm_pgoff = pgoff;
 
 	if (file) {

@@ -1089,6 +1090,12 @@ munmap_back:
 			goto free_vma;
 	}
 
+	/* Don't make the VMA automatically writable if it's shared, but the
+	 * backer wishes to know when pages are first written to */
+	if (vma->vm_ops && vma->vm_ops->page_mkwrite)
+		vma->vm_page_prot =
+			protection_map[vm_flags & (VM_READ|VM_WRITE|VM_EXEC)];
+
 	/* We set VM_ACCOUNT in a shared mapping's vm_flags, to inform
 	 * shmem_zero_setup (perhaps called through /dev/zero's ->mmap)
 	 * that memory reservation must be checked; but that reservation

@@ -1921,7 +1928,8 @@ unsigned long do_brk(unsigned long addr,
 	vma->vm_end = addr + len;
 	vma->vm_pgoff = pgoff;
 	vma->vm_flags = flags;
-	vma->vm_page_prot = protection_map[flags & 0x0f];
+	vma->vm_page_prot = protection_map[flags &
+				(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)];
 	vma_link(mm, vma, prev, rb_link, rb_parent);
 out:
 	mm->total_vm += len >> PAGE_SHIFT;

index 4c14d42..2697abd 100644
@@ -106,6 +106,7 @@ mprotect_fixup(struct vm_area_struct *vm
 	unsigned long oldflags = vma->vm_flags;
 	long nrpages = (end - start) >> PAGE_SHIFT;
 	unsigned long charged = 0;
+	unsigned int mask;
 	pgprot_t newprot;
 	pgoff_t pgoff;
 	int error;

@@ -132,8 +133,6 @@ mprotect_fixup(struct vm_area_struct *vm
 		}
 	}
 
-	newprot = protection_map[newflags & 0xf];
-
 	/*
 	 * First try to merge with previous and/or next vma.
 	 */

@@ -160,6 +159,14 @@ mprotect_fixup(struct vm_area_struct *vm
 	}
 
 success:
+	/* Don't make the VMA automatically writable if it's shared, but the
+	 * backer wishes to know when pages are first written to */
+	mask = VM_READ|VM_WRITE|VM_EXEC|VM_SHARED;
+	if (vma->vm_ops && vma->vm_ops->page_mkwrite)
+		mask &= ~VM_SHARED;
+
+	newprot = protection_map[newflags & mask];
+
 	/*
 	 * vm_flags and vm_page_prot are protected by the mmap_sem
 	 * held in write mode.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

From: Peter Zijlstra <[email protected]>

People expressed the need to track dirty pages in shared mappings.

Linus outlined the general idea of doing that through making clean writable pages write-protected and taking the write fault.

This patch does exactly that, it makes pages in a shared writable mapping write-protected. On write-fault the pages are marked dirty and made writable. When the pages get synced with their backing store, the write-protection is re-instated.

It survives a simple test and shows the dirty pages in /proc/vmstat.

Changes in -v5

- rename page_wrprotect() to page_mkclean() (suggested by Nick Piggin)
- added comment to test_clear_page_dirty() (Andrew Morton)
- cleanup page_wrprotect() (Andrew Morton)
- renamed VM_SharedWritable() to is_shared_writable()
- fs/buffers.c try_to_free_buffers(): remove clear_page_dirty() from under ->private_lock. This seems to be save, since ->private_lock is used to serialize access to the buffers, not the page itself.
- rebased on top of David Howells' page_mkwrite() patch.

Changes in -v4:

- small cleanup as suggested by Christoph Lameter.

Changes in -v3:

- move set_page_dirty() outside pte lock (suggested by Christoph Lameter)

Changes in -v2:

- only wrprotect pages from dirty capable mappings. (Nick Piggin)
- move the writefault handling from do_wp_page() into handle_pte_fault(). (Nick Piggin)
- revert to the old install_page interface. (Nick Piggin)
- also clear the pte dirty bit when we make pages read-only again. (spotted by Rik van Riel)
- make page_wrprotect() return the number of reprotected ptes.

 fs/buffer.c          |    2 -
 include/linux/mm.h   |    5 +++
 include/linux/rmap.h |    8 ++++++
 mm/fremap.c          |    4 +--
 mm/memory.c          |   20 +++++++++++++++
 mm/mmap.c            |    7 ++++-
 mm/mprotect.c        |    7 ++++-
 mm/page-writeback.c  |   13 ++++++++--
 mm/rmap.c            |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 122 insertions(+), 8 deletions(-)

Index: linux-2.6/include/linux/mm.h
===================================================================
@@ -183,6 +183,11 @@ extern unsigned int kobjsize(const void 
 #define VM_SequentialReadHint(v)	((v)->vm_flags & VM_SEQ_READ)
 #define VM_RandomReadHint(v)		((v)->vm_flags & VM_RAND_READ)
 
+static inline int is_shared_writable(struct vm_area_struct *vma)
+{
+	return (vma->vm_flags & (VM_SHARED|VM_WRITE)) == (VM_SHARED|VM_WRITE);
+}
+
 /*
  * mapping from the currently active vm_flags protection bits (the
  * low four bits) to a page protection mask..

Index: linux-2.6/mm/fremap.c
===================================================================
@@ -79,9 +79,9 @@ int install_page(struct mm_struct *mm, s
 		inc_mm_counter(mm, file_rss);
 
 	flush_icache_page(vma, page);
-	set_pte_at(mm, addr, pte, mk_pte(page, prot));
+	pte_val = mk_pte(page, prot);
+	set_pte_at(mm, addr, pte, pte_val);
 	page_add_file_rmap(page);
-	pte_val = *pte;
 	update_mmu_cache(vma, addr, pte_val);
 	err = 0;
 unlock:

Index: linux-2.6/mm/memory.c
===================================================================
@@ -48,6 +48,7 @@
 #include <linux/rmap.h>
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/backing-dev.h>
 
 #include <asm/pgalloc.h>
 #include <asm/uaccess.h>

@@ -1446,12 +1447,13 @@ static int do_wp_page(struct mm_struct *
 	struct page *old_page, *new_page;
 	pte_t entry;
 	int reuse, ret = VM_FAULT_MINOR;
+	struct page *dirty_page = NULL;
 
 	old_page = vm_normal_page(vma, address, orig_pte);
 	if (!old_page)
 		goto gotten;
 
-	if (unlikely(vma->vm_flags & VM_SHARED)) {
+	if (vma->vm_flags & VM_SHARED) {
 		if (vma->vm_ops && vma->vm_ops->page_mkwrite) {
 			/*
 			 * Notify the address space that the page is about to

@@ -1482,6 +1484,9 @@ static int do_wp_page(struct mm_struct *
 		}
 
 		reuse = 1;
+
+		dirty_page = old_page;
+		get_page(dirty_page);
 	} else if (PageAnon(old_page) && !TestSetPageLocked(old_page)) {
 		reuse = can_share_swap_page(old_page);
 		unlock_page(old_page);

@@ -1552,6 +1557,10 @@ gotten:
 		page_cache_release(old_page);
 unlock:
 	pte_unmap_unlock(page_table, ptl);
+	if (dirty_page) {
+		set_page_dirty(dirty_page);
+		put_page(dirty_page);
+	}
 	return ret;
 oom:
 	if (old_page)

@@ -2084,6 +2093,7 @@ static int do_no_page(struct mm_struct *
 	unsigned int sequence = 0;
 	int ret = VM_FAULT_MINOR;
 	int anon = 0;
+	struct page *dirty_page = NULL;
 
 	pte_unmap(page_table);
 	BUG_ON(vma->vm_flags & VM_PFNMAP);

@@ -2178,6 +2188,10 @@ retry:
 		} else {
 			inc_mm_counter(mm, file_rss);
 			page_add_file_rmap(new_page);
+			if (write_access) {
+				dirty_page = new_page;
+				get_page(dirty_page);
+			}
 		}
 	} else {
 		/* One of our sibling threads was faster, back out. */

@@ -2190,6 +2204,10 @@ retry:
 	lazy_mmu_prot_update(entry);
 unlock:
 	pte_unmap_unlock(page_table, ptl);
+	if (dirty_page) {
+		set_page_dirty(dirty_page);
+		put_page(dirty_page);
+	}
 	return ret;
 oom:
 	page_cache_release(new_page);

Index: linux-2.6/mm/page-writeback.c
===================================================================
@@ -29,6 +29,7 @@
 #include <linux/sysctl.h>
 #include <linux/cpu.h>
 #include <linux/syscalls.h>
+#include <linux/rmap.h>
 
 /*
  * The maximum number of pages to writeout in a single bdflush/kupdate

@@ -725,8 +726,14 @@ int test_clear_page_dirty(struct page *p
 						page_index(page),
 						PAGECACHE_TAG_DIRTY);
 			write_unlock_irqrestore(&mapping->tree_lock, flags);
-			if (mapping_cap_account_dirty(mapping))
+			/*
+			 * We can continue to use `mapping' here because the
+			 * page is locked, which pins the address_space
+			 */
+			if (mapping_cap_account_dirty(mapping)) {
+				page_mkclean(page);
 				dec_page_state(nr_dirty);
+			}
 			return 1;
 		}
 		write_unlock_irqrestore(&mapping->tree_lock, flags);

@@ -756,8 +763,10 @@ int clear_page_dirty_for_io(struct page 
 
 	if (mapping) {
 		if (TestClearPageDirty(page)) {
-			if (mapping_cap_account_dirty(mapping))
+			if (mapping_cap_account_dirty(mapping)) {
+				page_mkclean(page);
 				dec_page_state(nr_dirty);
+			}
 			return 1;
 		}
 		return 0;

Index: linux-2.6/include/linux/rmap.h
===================================================================
@@ -105,6 +105,14 @@ pte_t *page_check_address(struct page *,
  */
 unsigned long page_address_in_vma(struct page *, struct vm_area_struct *);
 
+/*
+ * Cleans the PTEs of shared mappings.
+ * (and since clean PTEs should also be readonly, write protects them too)
+ *
+ * returns the number of cleaned PTEs.
+ */
+int page_mkclean(struct page *);
+
 #else	/* !CONFIG_MMU */
 
 #define anon_vma_init()		do {} while (0)

Index: linux-2.6/fs/buffer.c
===================================================================
@@ -2985,6 +2985,7 @@ int try_to_free_buffers(struct page *pag
 
 	spin_lock(&mapping->private_lock);
 	ret = drop_buffers(page, &buffers_to_free);
+	spin_unlock(&mapping->private_lock);
 	if (ret) {
 		/*
 		 * If the filesystem writes its buffers by hand (eg ext3)

@@ -2996,7 +2997,6 @@ int try_to_free_buffers(struct page *pag
 		 */
 		clear_page_dirty(page);
 	}
-	spin_unlock(&mapping->private_lock);
 out:
 	if (buffers_to_free) {
 		struct buffer_head *bh = buffers_to_free;

Index: linux-2.6/mm/mmap.c
===================================================================
@@ -25,6 +25,7 @@
 #include <linux/mount.h>
 #include <linux/mempolicy.h>
 #include <linux/rmap.h>
+#include <linux/backing-dev.h>
 
 #include <asm/uaccess.h>
 #include <asm/cacheflush.h>

@@ -888,6 +889,7 @@ unsigned long do_mmap_pgoff(struct file 
 	struct rb_node ** rb_link, * rb_parent;
 	int accountable = 1;
 	unsigned long charged = 0, reqprot = prot;
+	struct address_space *mapping = NULL;
 
 	if (file) {
 		if (is_file_hugepages(file))

@@ -1092,7 +1094,10 @@ munmap_back:
 
 	/* Don't make the VMA automatically writable if it's shared, but the
 	 * backer wishes to know when pages are first written to */
-	if (vma->vm_ops && vma->vm_ops->page_mkwrite)
+	if (is_shared_writable(vma) && vma->vm_file)
+		mapping = vma->vm_file->f_mapping;
+	if ((mapping && mapping_cap_account_dirty(mapping)) ||
+			(vma->vm_ops && vma->vm_ops->page_mkwrite))
 		vma->vm_page_prot =
 			protection_map[vm_flags & (VM_READ|VM_WRITE|VM_EXEC)];
 

Index: linux-2.6/mm/mprotect.c
===================================================================
@@ -19,6 +19,7 @@
 #include <linux/mempolicy.h>
 #include <linux/personality.h>
 #include <linux/syscalls.h>
+#include <linux/backing-dev.h>
 
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>

@@ -107,6 +108,7 @@ mprotect_fixup(struct vm_area_struct *vm
 	long nrpages = (end - start) >> PAGE_SHIFT;
 	unsigned long charged = 0;
 	unsigned int mask;
+	struct address_space *mapping = NULL;
 	pgprot_t newprot;
 	pgoff_t pgoff;
 	int error;

@@ -162,7 +164,10 @@ success:
 	/* Don't make the VMA automatically writable if it's shared, but the
 	 * backer wishes to know when pages are first written to */
 	mask = VM_READ|VM_WRITE|VM_EXEC|VM_SHARED;
-	if (vma->vm_ops && vma->vm_ops->page_mkwrite)
+	if (is_shared_writable(vma) && vma->vm_file)
+		mapping = vma->vm_file->f_mapping;
+	if ((mapping && mapping_cap_account_dirty(mapping)) ||
+			(vma->vm_ops && vma->vm_ops->page_mkwrite))
 		mask &= ~VM_SHARED;
 
 	newprot = protection_map[newflags & mask];

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

I'm not sure I follow you, are you suggesting that we'll find the condition to be unlikely still, even with most of the shared mappings trapping this branch?

OK, will check.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

No, I just saw the opposite in a later patch. It was the -1 patch that does

+ if (unlikely(vma->vm_flags & VM_SHARED)) {

but thats a different context?
\

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

I am a bit confused about the need for Davids patch. set_page_dirty() is already a notification that a page is to be dirtied. Why do we need it twice? set_page_dirty could return an error code and the file system can use the set_page_dirty() hook to get its notification. What we would need to do is to make sure that set_page_dirty can sleep.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Ah, I see what you're saying here. Good point, David, Hugh?

The reason I did it was because of Hugh's trick to use MAP_SHARED protection and building on top of it naturally solves the patch conflict Andrew would have had to resolve otherwise.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

I guess what we wanted is a patch that addresses the concern of both patches and not a combination of the patches. IMHO the dirty notification of David's patch is possible with the shared dirty pages patch if we allow the set_page_dirty method in address operations to sleep and return an error code. However, this may raise some additional issues because we have to check whenever we dirty a page.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

set_page_dirty() is actually called fairly late in the game by zap_pte_range() and follow_page(). Thus, it is a notification that a page HAS BEEN dirtied and needs a writeback.

What is needed (at least for log structured or copy-on-write filesystems) is a function that is called during the page fault BEFORE the pte's writeable bit is set so that the pages' mapping can either be unmapped or remapped. That prevents possible readers of the old (read-only) version of the page from seeing any changes via the page map cache. If the page was unmapped, a second call is needed later to map it to a new set of blocks on the device. For a log structured filesystem it can makes sense to defer the remapping until late in the game when the block is about to be queued for i/o, in case it gets deleted before it is ever flushed from the page cache.

I looked at using set_page_dirty for the first of these hooks, but it comes too late. It might be OK for the second (remapping) hook though.

do_wp_page() is called by handle_pte_fault() in exactly the case where this would apply: if write access is needed and !pte_write(entry). This patch appears to address the necessary issue in a way that set_page_dirty cannot.

Jeff Anderson-Lee

[I'm a little late in coming in on this thread, and not 100% familiar with the latest kernels, so please pardon me if I'm not fully up to speed yet. I'm trying to catch up as quickly as I can though!]

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

The tracking patch changes that behavior. set_page_dirty is called before the write to the page occurs and so its similar to the new method introduced by David's patch.

page_mkwrite() is called just before the _PTE_ is dirtied. Take do_wp_page() for example, set_page_dirty() is called after a lot of stuff, including some stuff that marks the PTE dirty... by which time it's too late as another thread sharing the page tables can come along and modify the page before the first thread calls set_page_dirty().

Furthermore, by that point, it's pointless to have set_page_dirty() return an error by then. The deed is effectively done: the PTE is marked dirty and writable.

And also as you pointed out, set_page_dirty() needs to be able to sleep. There are places where it's called still, even with Peter's patch, with the page table lock held - zap_pte_range() for example. In that particular case, dropping the lock for each PTE would be bad for performance.

Basically, you can look at it as page_mkwrite() is called upfront, and set_page_dirty() is called at the end.

David

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Since we are terminating the application with extreme prejudice on an error (SIGBUS) it does not matter if another process has written to the page in the meantime.

zap_pte_range would only have to dirty anonymous pages. The pages of shared mappings would already be dirty.

The end is that the page is written back. I think we can still solve this with set_page_dirty being called when a page is about to be dirtied or was dirtied.

The page_mkwrite() method does not really allow the tracking of dirty pages. It is a way to track the potentially dirty pages that is useful if one is not able to track dirty pages. Moreover, unmapped dirtied pages do not factor into that scheme probably because it was thought that they are already sufficiently tracked by nr_dirty. However, having two methods of accounting for dirty pages creates problems in correlating the number of dirty pages. This is unnecessarily complex.

In order to consistently reach the goal of of tracking dirty pages we have to deal with set_page_dirty(). In the first stage lets just be satified with being able to throttle dirty pages by having an accurate nr_dirty.

We can then avoid doing too many things on set_page_dirty so that we do not have to sleep or return an error. Maybe add the support for errors (SIGBUS) later. But then we should consistently check everytime we dirty a page be it mapped or unmapped.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Erm... Yes, it does matter, at least for AFS or NFS using FS-Cache, and whether or not we're generating a SIGBUS or just proceeding normally. There are two cases I've come across:

Firstly I use page_mkwrite() to make sure that the page is written to the cache before we let anyone modify it, just so that we've got a reasonable idea of what's in the cache.

What we currently have is:

invoke page_mkwrite()
- wait for page to be written to the cache
lock
modify PTE
unlock
invoke set_page_dirty()

What your suggestion gives is:

lock
modify PTE
unlock
invoke set_page_dirty()
- wait for page to be written to the cache

But what can happen is:

CPU 1 CPU 2
======================= =======================
write to page (faults)
lock
modify PTE
unlock
write to page (succeeds)
invoke set_page_dirty()
- wait for page to be written to the cache
write to page (succeeds)

That potentially lets data of uncertain state into the cache, which means we can't trust what's in the cache any longer.

Secondly some filesystems want to use page_mkwrite() to prevent a write from occurring if a write to a shared writable mapping would require an allocation from a filesystem that's currently in an ENOSPC state. That means that we may not change the PTE until we're sure that the allocation is guaranteed to succeed, and that means that the kernel isn't left with dirty pages attached to inodes it'd like to dispose of but can't because there's nowhere to write the data.

David

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Maybe I do not understand properly. I thought page_mkwrite is called before a page is made writable not before it is dirtied. If its only called before the page is dirtied then a better name maybe before_dirty or so?

What do you mean by "written to the cache"? It cannot be written back since the page has been dirtied yet. So "written to the cache" means that the FS does some reservation, right?

I am not sure what the point would be to write a page that is not dirty. So I guess this reserves space on disk for the page.

Regardless of what "written to the cache" exactly means here we have the page marked dirty in the mapping and queued for write back.

Correct.

It continues established usage and usage that even with your patch continues at least for anonymous pages. The page dirty state may also be reflected by any dirty bit set in a pte pointing to the page even if the dirty state is not set on the page itself.

If set_page_dirty cannot reserve the page then we know that some severe action is required. The FS method set_page_dirty() could:

1. Determine the ENOSPC condition before it sets the page dirty. That leaves the potential that some writes to the page have occurred by other processes.

2. Track down all processes that use the mapping (or maybe less
severe: processes that have set the dirty bit in the pte) and terminate them with SIGBUS.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

page_mkwrite() is called before any of the PTEs referring to a page are made writable. This must precede a page being dirtied by writing to it directly through an mmap'd section. It does not catch write() and co. dirtying pages, but then there's no need since prepare_write() is available for that.

See the FS-Cache patches posted to LKML on the 19th of May, in particular the documentation included in the patch with the subject:

[PATCH 10/14] FS-Cache: Generic filesystem caching facility [try #10]

These patches permit data retrieved from network filesystems (NFS and AFS for now) to be cached locally on disk.

The page is fetched from the server and then written to the cache. We don't let the clean page be modified or released until the write to the cache is complete. This permits us to keep track of what state the cache is in.

But by the time set_page_dirty() is called, it's too late as the code currently stands. We've already marked the PTE writable and dirty. The page_mkwrite() op is called _first_.

You have to detect ENOSPC before you modify the PTE, otherwise someone else can jump through your hoop and dirty the page before you can stop them.

That's bad, even if you restrict it to those that have MAP_SHARED and PROT_WRITE set. They should not be terminated if they haven't attempted to write to the mapping.

Brrrr.

What's wrong with my suggestion anyway?

David

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

We are in set_page_dirty and this would be part of set_page_dirty processing.

Its bad but the out of space situation is an exceptional situation. We do similar contortions when we run out of memory space. As I said: One can track down the processes that have dirtied the pte to the page in question and just terminate those and remove the page.

Adds yet another method with functionality that for the most part is the same as set_page_dirty().

The advantage of such a method seems to be that it reserves filesystem space for pages that could potentially be written to. This allows the filesystem to accurately deal with out of space situations (a very rare condition. Is this really justifiable?). Maybe having already reserved space could speed up the real dirtying of pages?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Your original question, whether they could be combined, was a good one; and I hoped you'd be right. But I agree with David, they cannot, unless we sacrifice the guarantee that one or the other is there to give. It's much like the relationship between ->prepare_write and ->commit_write.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Ok, so separate patch sets?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

They are separate patch sets (and rc5-mm1 has David's without Peter's). But they trample on nearby areas and share infrastructure, so I'm happy that they're looked at together. Peter has helpfully arranged his to go on top of David's: that can be reversed later if it's decided that Peter's is wanted but David's not.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Eh? What do you mean "We are in set_page_dirty"?

The main caller of page_mkwrite() is do_wp_page(). With my patch, this function does the following steps in order:

(1) Invoke page_mkwrite() if available, giving SIGBUS on error.

(2) Mark the PTE writable and dirty.

(3) Unlock the page table.

Then with Peter's patch:

(4) Invoke set_page_dirty_balance().

We shouldn't be having to handle an error at step (4), and we shouldn't be marking the page dirty before step (2).

In fact, it could be argued that we shouldn't be marking the page dirty until it is actually dirty, but I can't see any way to do that atomically short of emulating the store instruction without hardware support.

In general, we really really shouldn't just remove the page. The only place I would consider it appropriate to destroy a dirty page is if we can't write it back because the backing store is no longer available due to an event we cannot predict (network filesystems for example).

We should operate on preventative measures rather than curative ones: we should detect ENOSPC in advance and signal the process doing the write that that it's not permitted to proceed. We shouldn't go and kill all the processes that might write to that mapping.

Prevention is almost always cleaner than trying to fix things up afterwards.

It's not exactly the same. It's almost the same difference as between prepare_write() and commit_write(). Currently it's very much the same because, as I understand it, the PTE dirty flag is transferred to the page struct when the PTE is destroyed (in zap_pte_range()).

Actually, I'm not sure that calling set_page_dirty() at the bottom of do_wp_page() is necessarily a good idea. It's possible that the page will be marked dirty in do_wp_page() and then will get written back before the write actually succeeds. In other words the page may be marked dirty and cleaned up all before the modification _actually_ occurs. On the other hand, the common case is probably that the store instruction will beat the writeback.

Of course, if you're proposing to call set_page_dirty() before the PTE is modified, that would work. But you still have to make sure that it isn't called under spinlock, and doing that would make zap_pte_range() potentially indulge in lock thrashing.

It's better than having unwritable pages sat around in the pagecache because there's no space on the backing device to write them back. At least this way gives a graceful way to handle the situation.

Destroying uncommitted data isn't really an acceptable answer.

David

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

We could do the reservation in as part of the set_page_dirty FS method.

Yes we are aware of that case.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

From: Peter Zijlstra <[email protected]>

Now that we can detect writers of shared mappings, throttle them. Avoids OOM by surprise.

Changes -v2:

- small helper function (Andrew Morton)

 include/linux/writeback.h |    1 +
 mm/memory.c               |    5 +++--
 mm/page-writeback.c       |   10 ++++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/memory.c
===================================================================
@@ -49,6 +49,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/backing-dev.h>
+#include <linux/writeback.h>
 
 #include <asm/pgalloc.h>
 #include <asm/uaccess.h>

@@ -1558,7 +1559,7 @@ gotten:
 unlock:
 	pte_unmap_unlock(page_table, ptl);
 	if (dirty_page) {
-		set_page_dirty(dirty_page);
+		set_page_dirty_balance(dirty_page);
 		put_page(dirty_page);
 	}
 	return ret;

@@ -2205,7 +2206,7 @@ retry:
 unlock:
 	pte_unmap_unlock(page_table, ptl);
 	if (dirty_page) {
-		set_page_dirty(dirty_page);
+		set_page_dirty_balance(dirty_page);
 		put_page(dirty_page);
 	}
 	return ret;

Index: linux-2.6/include/linux/writeback.h
===================================================================
@@ -114,6 +114,7 @@ int sync_page_range(struct inode *inode,
 			loff_t pos, loff_t count);
 int sync_page_range_nolock(struct inode *inode, struct address_space *mapping,
 			   loff_t pos, loff_t count);
+void set_page_dirty_balance(struct page *page);
 
 /* pdflush.c */
 extern int nr_pdflush_threads;	/* Global so it can be exported to sysctl

Index: linux-2.6/mm/page-writeback.c
===================================================================
@@ -255,6 +255,16 @@ static void balance_dirty_pages(struct a
 		pdflush_operation(background_writeout, 0);
 }
 
+void set_page_dirty_balance(struct page *page)
+{
+	if (set_page_dirty(page)) {
+		struct address_space *mapping = page_mapping(page);
+
+		if (mapping)
+			balance_dirty_pages_ratelimited(mapping);
+	}
+}
+
 /**
  * balance_dirty_pages_ratelimited_nr - balance dirty memory state
  * @mapping: address_space which was dirtied

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

From: Peter Zijlstra <[email protected]>

With the tracking of dirty pages properly done now, msync doesn't need to scan the pte's anymore to determine the dirty status.

 mm/msync.c |  145 ++++---------------------------------------------------------
 1 file changed, 10 insertions(+), 135 deletions(-)

Index: linux-2.6/mm/msync.c
===================================================================
@@ -20,128 +20,20 @@
 #include <asm/pgtable.h>
 #include <asm/tlbflush.h>
 
-static unsigned long msync_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
-				unsigned long addr, unsigned long end)
-{
-	pte_t *pte;
-	spinlock_t *ptl;
-	int progress = 0;
-	unsigned long ret = 0;
-
-again:
-	pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
-	do {
-		struct page *page;
-
-		if (progress >= 64) {
-			progress = 0;
-			if (need_resched() || need_lockbreak(ptl))
-				break;
-		}
-		progress++;
-		if (!pte_present(*pte))
-			continue;
-		if (!pte_maybe_dirty(*pte))
-			continue;
-		page = vm_normal_page(vma, addr, *pte);
-		if (!page)
-			continue;
-		if (ptep_clear_flush_dirty(vma, addr, pte) ||
-				page_test_and_clear_dirty(page))
-			ret += set_page_dirty(page);
-		progress += 3;
-	} while (pte++, addr += PAGE_SIZE, addr != end);
-	pte_unmap_unlock(pte - 1, ptl);
-	cond_resched();
-	if (addr != end)
-		goto again;
-	return ret;
-}
-
-static inline unsigned long msync_pmd_range(struct vm_area_struct *vma,
-			pud_t *pud, unsigned long addr, unsigned long end)
-{
-	pmd_t *pmd;
-	unsigned long next;
-	unsigned long ret = 0;
-
-	pmd = pmd_offset(pud, addr);
-	do {
-		next = pmd_addr_end(addr, end);
-		if (pmd_none_or_clear_bad(pmd))
-			continue;
-		ret += msync_pte_range(vma, pmd, addr, next);
-	} while (pmd++, addr = next, addr != end);
-	return ret;
-}
-
-static inline unsigned long msync_pud_range(struct vm_area_struct *vma,
-			pgd_t *pgd, unsigned long addr, unsigned long end)
-{
-	pud_t *pud;
-	unsigned long next;
-	unsigned long ret = 0;
-
-	pud = pud_offset(pgd, addr);
-	do {
-		next = pud_addr_end(addr, end);
-		if (pud_none_or_clear_bad(pud))
-			continue;
-		ret += msync_pmd_range(vma, pud, addr, next);
-	} while (pud++, addr = next, addr != end);
-	return ret;
-}
-
-static unsigned long msync_page_range(struct vm_area_struct *vma,
-				unsigned long addr, unsigned long end)
-{
-	pgd_t *pgd;
-	unsigned long next;
-	unsigned long ret = 0;
-
-	/* For hugepages we can't go walking the page table normally,
-	 * but that's ok, hugetlbfs is memory based, so we don't need
-	 * to do anything more on an msync().
-	 */
-	if (vma->vm_flags & VM_HUGETLB)
-		return 0;
-
-	BUG_ON(addr >= end);
-	pgd = pgd_offset(vma->vm_mm, addr);
-	flush_cache_range(vma, addr, end);
-	do {
-		next = pgd_addr_end(addr, end);
-		if (pgd_none_or_clear_bad(pgd))
-			continue;
-		ret += msync_pud_range(vma, pgd, addr, next);
-	} while (pgd++, addr = next, addr != end);
-	return ret;
-}
-
 /*
  * MS_SYNC syncs the entire file - including mappings.
  *
- * MS_ASYNC does not start I/O (it used to, up to 2.5.67).  Instead, it just
- * marks the relevant pages dirty.  The application may now run fsync() to
+ * MS_ASYNC does not start I/O (it used to, up to 2.5.67).
+ * Nor does it just marks the relevant pages dirty (it used to up to 2.6.17).
+ * Now it doesn't do anything, since dirty pages are properly tracked.
+ *
+ * The application may now run fsync() to
  * write out the dirty pages and wait on the writeout and check the result.
  * Or the application may run fadvise(FADV_DONTNEED) against the fd to start
  * async writeout immediately.
  * So by _not_ starting I/O in MS_ASYNC we provide complete flexibility to
  * applications.
  */
-static int msync_interval(struct vm_area_struct *vma, unsigned long addr,
-			unsigned long end, int flags,
-			unsigned long *nr_pages_dirtied)
-{
-	struct file *file = vma->vm_file;
-
-	if ((flags & MS_INVALIDATE) && (vma->vm_flags & VM_LOCKED))
-		return -EBUSY;
-
-	if (file && (vma->vm_flags & VM_SHARED))
-		*nr_pages_dirtied = msync_page_range(vma, addr, end);
-	return 0;
-}
 
 asmlinkage long sys_msync(unsigned long start, size_t len, int flags)
 {

@@ -157,6 +49,10 @@ asmlinkage long sys_msync(unsigned long 
 		goto out;
 	if ((flags & MS_ASYNC) && (flags & MS_SYNC))
 		goto out;
+	if (flags & MS_ASYNC) {
+		error = 0;
+		goto out;
+	}
 	error = -ENOMEM;
 	len = (len + ~PAGE_MASK) & PAGE_MASK;
 	end = start + len;

@@ -178,7 +74,6 @@ asmlinkage long sys_msync(unsigned long 
 		goto out_unlock;
 	}
 	do {
-		unsigned long nr_pages_dirtied = 0;
 		struct file *file;
 
 		/* Here start < vma->vm_end. */

@@ -188,32 +83,12 @@ asmlinkage long sys_msync(unsigned long 
 		}
 		/* Here vma->vm_start <= start < vma->vm_end. */
 		if (end <= vma->vm_end) {
-			if (start < end) {
-				error = msync_interval(vma, start, end, flags,
-							&nr_pages_dirtied);
-				if (error)
-					goto out_unlock;
-			}
 			error = unmapped_error;
 			done = 1;
-		} else {
-			/* Here vma->vm_start <= start < vma->vm_end < end. */
-			error = msync_interval(vma, start, vma->vm_end, flags,
-						&nr_pages_dirtied);
-			if (error)
-				goto out_unlock;
 		}
 		file = vma->vm_file;
 		start = vma->vm_end;
-		if ((flags & MS_ASYNC) && file && nr_pages_dirtied) {
-			get_file(file);
-			up_read(&current->mm->mmap_sem);
-			balance_dirty_pages_ratelimited_nr(file->f_mapping,
-							nr_pages_dirtied);
-			fput(file);
-			down_read(&current->mm->mmap_sem);
-			vma = find_vma(current->mm, start);
-		} else if ((flags & MS_SYNC) && file &&
+		if ((flags & MS_SYNC) && file &&
 				(vma->vm_flags & VM_SHARED)) {
 			get_file(file);
 			up_read(&current->mm->mmap_sem);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Although I like this approach, and your patches implementing it, I have realized a snag with it, see below. I hope someone can make a suggestion that shows up my stupidity and melts away the difficulty, but for now I'm stumped.

That would impose the overhead of additional write faults on those cases (notably shared memory) where we just don't need it.

Yes, I think you've cleaned up slightly too far, see remarks below.

I'll mention the minor points first, then after come to the tiresome issue I've spent so much time failing to resolve.

You tend to use get_page/put_page amidst code using page_cache_get/ page_cache_release. Carry on: it sometimes looks odd, but I can't see any way to impose consistency, short of abolishing one or the other throughout the tree. So don't worry about it.

It's irritating to have to be setting "dirty_page" near the page_mkwrite code, then doing the set_page_dirty_balance separately at the end. But I agree with you and David: the page_mkwrite needs to be checked before, and the set_page_dirty needs to be done after (when the pte is exposed in the page table): otherwise page_mkclean would miss instances, and the nr_dirty count drift slowly downwards away from reality. Or, am I mistaken? Maybe, but the way you have it is exact (one could add a BUG in zap_pte_range, that a VM_SHARED page with pte dirty is already PageDirty; though I wouldn't recommend doing so unless testing), let's keep it like that for now, maybe look into relaxing at a future date.

You've got a minor cleanup to install_page, left over from an earlier
iteration: the cleanup looked okay, but of no relevance to your patchset now, is it? Just cut mm/fremap.c out of the patchset I think.

You've taken the simplification of sys_msync a little too far, I believe:
you ought to try to reproduce the same errors as before, so MS_ASYNC should be winding through the separate vmas like MS_SYNC, just to report -ENOMEM if it crosses an unmapped area; and MS_INVALIDATE used to say -EBUSY if VM_LOCKED, but that has disappeared. (Perhaps I've missed other such details, please recheck.) Your comment should say "Nor does it mark" instead of "Nor does it just marks".

I think remove the update_mmu_cache from page_mkclean_one as Christoph suggested, leaving the lazy_mmu_prot_update: I cannot then justify why do_wp_page's reuse case does update_mmu_cache when it's merely changing the protection on the page, but it always has so I suppose it should continue to do so; what you have in page_mkclean_one is more like mprotecting than faulting.

Your is_shared_writable(vma) in mprotect_fixup is along the right lines, but wrong: because at that point vma->vm_flags is the old one, and may be omitting VM_WRITE when that is about to be added. Perhaps you should move the "vma->vm_flags = newflags" above it, or perhaps you should change is_shared_writable to work on flags rather than vma (as Linus' is_cow_mapping does).

And whenever I see that call to change_protection, I wonder whether we should optimize it out when newprot is the same as the old vma->vm_page_prot: we've not done so in the past, but perhaps now we're adding cases when it's likely to be the same as before, we should add that little optimization.

But the tiresome issue is the similar mapping_cap_account_dirty check in do_mmap_pgoff. Whereas page_mkwrite defaults to unset, so that the restriction on vm_page_prot only applies to drivers or filesystems which have opted in by declaring a page_mkwrite, mapping->backing_dev_info (initialized in alloc_inode) defaults to default_backing_dev_info, which does not have BDI_CAP_NO_ACCT_DIRTY.

Which is fine for ordinary filesystems, but means that all the strange drivers with their own ->mmap are by default mapping_cap_account_dirty, and therefore you will be adjusting their vm_page_prot; and some of them have set vm_page_prot rather carefully e.g. with a NOCACHE bit.

This shouldn't be a problem for remap_pfn_range and vm_insert_page users (vmas now marked VM_PFNMAP or VM_INSERTPAGE), since they should have already set up their ptes in the ->mmap above, and have no further use for vm_page_prot - though there might be odd cases, and it would be preferable not to risk changing behaviour on them.

Where it's really a problem is for the various drivers with .nopage methods. In the old scheme (before I diverted you to follow page_mkwrite), that could have been handled in do_no_page, by checking page->mapping (there is then a question of race with truncate on ordinary filesystem pages, which also lose page->mapping, but I think that works out not to matter - though I'm not sure); but in do_mmap_pgoff we do not know whether the filesystem or driver will be dealing in pagecache pages or not.

There are several ways forward, but I don't know which to advise:
I'm hoping someone else will see an easy and safe solution which I've been missing.

The "right" solution is for us to declare another backing_dev_info with BDI_CAP_NO_ACCT_DIRTY set, and all those drivers point their mapping->backing_dev_info to that instead in their .mmap. But that means your work becomes dependent on changes to a variety of drivers; an obscure further requirement on drivers trying to enable mmap, when there's too many things to get right already; and obscure bugs in any drivers forgotten or outside the tree.

Or resurrect the VM_RESERVED flag we were thinking of scrapping, add it in whichever drivers it's needed but missing, and check that?

Or is there anything we can key off instead, to default to this other backing_dev_info from the core, without changing drivers?

Make character devices use the BDI_CAP_NO_ACCT_DIRTY info, leaving block devices and files with the current default? Perhaps, but I'm not sure if that apportions all the cases correctly, and it would sit better in UNIX than in Linux (which doesn't key much off the block/character distinction, I think).

Check for ->fsync in the file_operations, use BDI_CAP_NO_ACCT_DIRTY backing_dev_info when it's NULL? That looks to me like it would work pretty well; but definitely a hack, and again I'm not certain it would get all the cases right (I haven't the faintest idea what should happen with arch/powerpc/platforms/cell/spufs, for example).

Or give up, apologize, and ask you to go back to testing in do_no_page, instead of following the page_mkwrite vm_page_prot technique?

I don't know: sorry.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Noticed that myself too, came to the same conclusion, thanks for the confirmation thought.

OK, unless we go back to the previous way I'll send this tiny cleanup as a separate patch to Andrew.

Ah, yes, I've noticed this too, I just wasn't sure on if this would be wanted or not. Is fixed, thanks!

Hehe, thanks, please do point out my mistakes with the English language. I have good enough control to convey most of what I intent but I'm not a native.

How odd, I have the distinct recollection of actually moving that assignment upwards a few lines, must have been late or something. Thanks for pointing this out, would've never found it again; with me thinking it was done with.

<snip: big tiresome story/>

Well, you got me. I don't know either, the only thing I can come up with is making the breakage compile-time (for 3rd-party modules) instead of subtle run-time, but its still not pretty.

/me looks around at assorted VM gurus; any ideas out there?

If by tomorrow morning CET nobody has spoken up, I'll just go ahead and accept Hugh's apology :-), that is revert back to my original way of doing it. (I can always go back to this scheme if some smart but slower working brain manages a solution)

Peter

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

From: Peter Zijlstra <[email protected]>

People expressed the need to track dirty pages in shared mappings.

Linus outlined the general idea of doing that through making clean writable pages write-protected and taking the write fault.

This patch does exactly that, it makes pages in a shared writable mapping write-protected. On write-fault the pages are marked dirty and made writable. When the pages get synced with their backing store, the write-protection is re-instated.

It survives a simple test and shows the dirty pages in /proc/vmstat.

Changes in -v6

- make page_mkclean_one() modify the pte more like change_pte_range() (suggested by Christoph Lameter)
- made is_shared_writable() take vm_flags, it now resembles is_cow_mapping().
- fixed the mprotect() bug (spotted by Hugh Dickins)
- hopefully fixed the tiresome issue of do_mmap_pgoff() trampling on driver specific vm_page_prot settings (spotted by Hugh Dickins)

This does add the requirement that f_op->mmap() may not 'reset' vm_page_prot from vm_flags. I checked all in-kernel users and non seem to do this.

- made a new version of the page_mkwrite() patch to go on top of all this. This so that Linus could merge this very early on in 2.6.18.

Changes in -v5

- rename page_wrprotect() to page_mkclean() (suggested by Nick Piggin)
- added comment to test_clear_page_dirty() (Andrew Morton)
- cleanup page_wrprotect() (Andrew Morton)
- renamed VM_SharedWritable() to is_shared_writable()
- fs/buffers.c try_to_free_buffers(): remove clear_page_dirty() from under ->private_lock. This seems to be save, since ->private_lock is used to serialize access to the buffers, not the page itself.
- rebased on top of David Howells' page_mkwrite() patch.

Changes in -v4:

- small cleanup as suggested by Christoph Lameter.

Changes in -v3:

- move set_page_dirty() outside pte lock (suggested by Christoph Lameter)

Changes in -v2:

- only wrprotect pages from dirty capable mappings. (Nick Piggin)
- move the writefault handling from do_wp_page() into handle_pte_fault(). (Nick Piggin)
- revert to the old install_page interface. (Nick Piggin)
- also clear the pte dirty bit when we make pages read-only again. (spotted by Rik van Riel)
- make page_wrprotect() return the number of reprotected ptes.

 drivers/char/drm/drm_vm.c |    5 ++-
 fs/buffer.c               |    2 -
 include/linux/mm.h        |    5 +++
 include/linux/rmap.h      |    8 +++++
 mm/memory.c               |   45 +++++++++++++++++++++++---------
 mm/mmap.c                 |   28 ++++++++++++++++++--
 mm/mprotect.c             |   14 ++++++++--
 mm/page-writeback.c       |   13 +++++++--
 mm/rmap.c                 |   64 ++++++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 163 insertions(+), 21 deletions(-)

Index: linux-2.6/include/linux/mm.h
===================================================================
@@ -183,6 +183,11 @@ extern unsigned int kobjsize(const void 
 #define VM_SequentialReadHint(v)	((v)->vm_flags & VM_SEQ_READ)
 #define VM_RandomReadHint(v)		((v)->vm_flags & VM_RAND_READ)
 
+static inline int is_shared_writable(unsigned int flags)
+{
+	return (flags & (VM_SHARED|VM_WRITE)) == (VM_SHARED|VM_WRITE);
+}
+
 /*
  * mapping from the currently active vm_flags protection bits (the
  * low four bits) to a page protection mask..

Index: linux-2.6/mm/mmap.c
===================================================================
@@ -25,6 +25,7 @@
 #include <linux/mount.h>
 #include <linux/mempolicy.h>
 #include <linux/rmap.h>
+#include <linux/backing-dev.h>
 
 #include <asm/uaccess.h>
 #include <asm/cacheflush.h>

@@ -1065,10 +1066,13 @@ munmap_back:
 	vma->vm_start = addr;
 	vma->vm_end = addr + len;
 	vma->vm_flags = vm_flags;
-	vma->vm_page_prot = protection_map[vm_flags & 0x0f];
+	vma->vm_page_prot = protection_map[vm_flags &
+				(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)];
 	vma->vm_pgoff = pgoff;
 
 	if (file) {
+		struct address_space *mapping = NULL;
+
 		error = -EINVAL;
 		if (vm_flags & (VM_GROWSDOWN|VM_GROWSUP))
 			goto free_vma;

@@ -1080,6 +1084,25 @@ munmap_back:
 		}
 		vma->vm_file = file;
 		get_file(file);
+
+		/* Trap writes to shared writable mappings that have
+		 * dirty page accounting.
+		 *
+		 * f_op->open() - sets the backing_dev_info
+		 * f_op->mmap() - modifies vm_page_prot; but will not reset
+		 *                from vm_flags.
+		 *
+		 * Hence between the two calls (here) it is save to modify
+		 * vm_page_prot depending on backing_dev_info capabilities.
+		 *
+		 * shmem_backing_dev_info does have BDI_CAP_NO_ACCT_DIRTY.
+		 */
+		if (is_shared_writable(vm_flags))
+			mapping = file->f_mapping;
+		if (mapping && mapping_cap_account_dirty(mapping))
+			vma->vm_page_prot = protection_map[vm_flags &
+				(VM_READ|VM_WRITE|VM_EXEC)];
+
 		error = file->f_op->mmap(file, vma);
 		if (error)
 			goto unmap_and_free_vma;

@@ -1921,7 +1944,8 @@ unsigned long do_brk(unsigned long addr,
 	vma->vm_end = addr + len;
 	vma->vm_pgoff = pgoff;
 	vma->vm_flags = flags;
-	vma->vm_page_prot = protection_map[flags & 0x0f];
+	vma->vm_page_prot = protection_map[flags &
+				(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)];
 	vma_link(mm, vma, prev, rb_link, rb_parent);
 out:
 	mm->total_vm += len >> PAGE_SHIFT;

Index: linux-2.6/mm/mprotect.c
===================================================================
@@ -19,6 +19,7 @@
 #include <linux/mempolicy.h>
 #include <linux/personality.h>
 #include <linux/syscalls.h>
+#include <linux/backing-dev.h>
 
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>

@@ -106,6 +107,8 @@ mprotect_fixup(struct vm_area_struct *vm
 	unsigned long oldflags = vma->vm_flags;
 	long nrpages = (end - start) >> PAGE_SHIFT;
 	unsigned long charged = 0;
+	unsigned int mask;
+	struct address_space *mapping = NULL;
 	pgprot_t newprot;
 	pgoff_t pgoff;
 	int error;

@@ -132,8 +135,6 @@ mprotect_fixup(struct vm_area_struct *vm
 		}
 	}
 
-	newprot = protection_map[newflags & 0xf];
-
 	/*
 	 * First try to merge with previous and/or next vma.
 	 */

@@ -160,6 +161,15 @@ mprotect_fixup(struct vm_area_struct *vm
 	}
 
 success:
+	/* Don't make the VMA automatically writable if it's shared. */
+	mask = VM_READ|VM_WRITE|VM_EXEC|VM_SHARED;
+	if (is_shared_writable(newflags) && vma->vm_file)
+		mapping = vma->vm_file->f_mapping;
+	if (mapping && mapping_cap_account_dirty(mapping))
+		mask &= ~VM_SHARED;
+
+	newprot = protection_map[newflags & mask];
+
 	/*
 	 * vm_flags and vm_page_prot are protected by the mmap_sem
 	 * held in write mode.

Index: linux-2.6/mm/page-writeback.c
===================================================================
@@ -29,6 +29,7 @@
 #include <linux/sysctl.h>
 #include <linux/cpu.h>
 #include <linux/syscalls.h>
+#include <linux/rmap.h>
 
 /*
  * The maximum number of pages to writeout in a single bdflush/kupdate

@@ -725,8 +726,14 @@ int test_clear_page_dirty(struct page *p
 						page_index(page),
 						PAGECACHE_TAG_DIRTY);
 			write_unlock_irqrestore(&mapping->tree_lock, flags);
-			if (mapping_cap_account_dirty(mapping))
+			/*
+			 * We can continue to use `mapping' here because the
+			 * page is locked, which pins the address_space
+			 */
+			if (mapping_cap_account_dirty(mapping)) {
+				page_mkclean(page);
 				dec_page_state(nr_dirty);
+			}
 			return 1;
 		}
 		write_unlock_irqrestore(&mapping->tree_lock, flags);

@@ -756,8 +763,10 @@ int clear_page_dirty_for_io(struct page 
 
 	if (mapping) {
 		if (TestClearPageDirty(page)) {
-			if (mapping_cap_account_dirty(mapping))
+			if (mapping_cap_account_dirty(mapping)) {
+				page_mkclean(page);
 				dec_page_state(nr_dirty);
+			}
 			return 1;
 		}
 		return 0;

Index: linux-2.6/include/linux/rmap.h
===================================================================
@@ -105,6 +105,14 @@ pte_t *page_check_address(struct page *,
  */
 unsigned long page_address_in_vma(struct page *, struct vm_area_struct *);
 
+/*
+ * Cleans the PTEs of shared mappings.
+ * (and since clean PTEs should also be readonly, write protects them too)
+ *
+ * returns the number of cleaned PTEs.
+ */
+int page_mkclean(struct page *);
+
 #else	/* !CONFIG_MMU */
 
 #define anon_vma_init()		do {} while (0)

Index: linux-2.6/fs/buffer.c
===================================================================
@@ -2985,6 +2985,7 @@ int try_to_free_buffers(struct page *pag
 
 	spin_lock(&mapping->private_lock);
 	ret = drop_buffers(page, &buffers_to_free);
+	spin_unlock(&mapping->private_lock);
 	if (ret) {
 		/*
 		 * If the filesystem writes its buffers by hand (eg ext3)

@@ -2996,7 +2997,6 @@ int try_to_free_buffers(struct page *pag
 		 */
 		clear_page_dirty(page);
 	}
-	spin_unlock(&mapping->private_lock);
 out:
 	if (buffers_to_free) {
 		struct buffer_head *bh = buffers_to_free;

Index: linux-2.6/drivers/char/drm/drm_vm.c
===================================================================
@@ -574,7 +574,10 @@ int drm_mmap(struct file *filp, struct v
 #else
 		/* Ye gads this is ugly.  With more thought
 		   we could move this up higher and use
-		   `protection_map' instead.  */
+		   `protection_map' instead.
+
+		   One cannot; drivers are not allowed to 'reset'
+		   vm_page_prot from vm_flags. */
 		vma->vm_page_prot =
 		    __pgprot(pte_val
 			     (pte_wrprotect

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

If this one still has some problems there is on more thing we could try before going back to the old way of doing things.

I found this 'gem' in the drm code:

        vma->vm_page_prot =
            __pgprot(pte_val
                     (pte_wrprotect
                      (__pte(pgprot_val(vma->vm_page_prot)))));

which does exactly what is needed.

OTOH, my alternate version of the mprotect fix leaves dirty pages writable, which saves a few faults.

Peter

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

1. The case that a vma is shared is not likely.

2. Typo fix in mmap.c

3. This may be a bit controversial but it does not seem to make sense to use the update_mmu_cache macro when we reuse the page. We are only fiddling around with the protections, the dirty and accessed bits.

With the call to update_mmu_cache the way of using the macros would be different from mprotect() and page_mkclean(). I'd rather have everything work the same way. If this breaks on some arches then also mprotect and page_mkclean() are broken. The use of mprotect() is rare, we may have breakage in some arches that we just have not seen yet.

Index: linux-2.6/mm/memory.c
===================================================================
@@ -1452,7 +1452,7 @@ static int do_wp_page(struct mm_struct *
 	if (!old_page)
 		goto gotten;
 
-	if (vma->vm_flags & VM_SHARED) {
+	if (unlikely(vma->vm_flags & VM_SHARED)) {
 		reuse = 1;
 		dirty_page = old_page;
 		get_page(dirty_page);

@@ -1466,7 +1466,6 @@ static int do_wp_page(struct mm_struct *
 		entry = pte_mkyoung(orig_pte);
 		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
 		ptep_set_access_flags(vma, address, page_table, entry, 1);
-		update_mmu_cache(vma, address, entry);
 		lazy_mmu_prot_update(entry);
 		ret |= VM_FAULT_WRITE;
 		goto unlock;

Index: linux-2.6/mm/mmap.c
===================================================================
@@ -1092,7 +1092,7 @@ munmap_back:
 		 * f_op->mmap() - modifies vm_page_prot; but will not reset
 		 *                from vm_flags.
 		 *
-		 * Hence between the two calls (here) it is save to modify
+		 * Hence between the two calls (here) it is safe to modify
 		 * vm_page_prot depending on backing_dev_info capabilities.
 		 *
 		 * shmem_backing_dev_info does have BDI_CAP_NO_ACCT_DIRTY.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Does this mean that processes dirtying pages via mmap are now subject to write throttling? That could dramatically change the performance for tasks with a working set larger than 10% of memory.

NATE

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Exactly. Except it's not a "working set", it's a "dirty set".

Which is the whole (and only) point of the whole patch.

If you want to live on the edge, you can set the dirty_balance trigger to something much higher, it's entirely configurable if I remember correctly.

Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>