Fixing the issue with memory-mapped file times

mm/msync.c line 35

Quoted 9 lines

      unsigned long end;

mm/msync.c +3 -3

-     struct mm_struct *mm = current->mm;
+     int error, unmapped_error;
      struct vm_area_struct *vma;
-     int unmapped_error = 0;
-     int error = -EINVAL;
+     struct mm_struct *mm;
 
+     error = -EINVAL;

I think you may have misunderstood my last comment. These are OK:

        struct mm_struct *mm = current->mm;
        int unmapped_error = 0;
        int error = -EINVAL;

This is not so good:

        int error, unmapped_error;

This is the worst:

        int error = -EINVAL, unmapped_error = 0;

So I think the original code is fine as it is.

Othewise patch looks OK now.

Miklos

Updating file times at write references to memory-mapped files and forcing file times update at the next write reference after calling the msync() system call with the MS_ASYNC flag.

Signed-off-by Anton Salikhmetov <[email protected]>

---
 mm/memory.c |    6 ++++++
 mm/msync.c  |   52 +++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 45 insertions(+), 13 deletions(-)

mm/memory.c +6 -0

index 4bf0b6d..13d5bbf 100644
@@ -1668,6 +1668,9 @@ gotten:
 unlock:
 	pte_unmap_unlock(page_table, ptl);
 	if (dirty_page) {
+		if (vma->vm_file)
+			file_update_time(vma->vm_file);
+
 		/*
 		 * Yes, Virginia, this is actually required to prevent a race
 		 * with clear_page_dirty_for_io() from clearing the page dirty

@@ -2341,6 +2344,9 @@ out_unlocked:
 	if (anon)
 		page_cache_release(vmf.page);
 	else if (dirty_page) {
+		if (vma->vm_file)
+			file_update_time(vma->vm_file);
+
 		set_page_dirty_balance(dirty_page, page_mkwrite);
 		put_page(dirty_page);
 	}

mm/msync.c +9 -0

index a4de868..a49af28 100644
@@ -13,11 +13,33 @@
 #include <linux/syscalls.h>
 
 /*
+ * Scan the PTEs for pages belonging to the VMA and mark them read-only.
+ * It will force a pagefault on the next write access.
+ */
+static void vma_wrprotect(struct vm_area_struct *vma)
+{
+	unsigned long addr;
+
+	for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
+		spinlock_t *ptl;

mm/msync.c +20 -13

+
+/*
  * MS_SYNC syncs the entire file - including mappings.
  *
- * MS_ASYNC does not start I/O (it used to, up to 2.5.67).
- * Nor does it mark the relevant pages dirty (it used to up to 2.6.17).
- * Now it doesn't do anything, since dirty pages are properly tracked.
+ * MS_ASYNC does not start I/O. Instead, it marks the relevant pages
+ * read-only by calling vma_wrprotect(). This is needed to catch the next
+ * write reference to the mapped region and update the file times
+ * accordingly.
  *
  * 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

@@ -77,16 +99,20 @@ asmlinkage long sys_msync(unsigned long start, size_t len, int flags)
 		error = 0;
 		start = vma->vm_end;
 		file = vma->vm_file;
-		if (file && (vma->vm_flags & VM_SHARED) && (flags & MS_SYNC)) {
-			get_file(file);
-			up_read(&mm->mmap_sem);
-			error = do_fsync(file, 0);
-			fput(file);
-			if (error || start >= end)
-				goto out;
-			down_read(&mm->mmap_sem);
-			vma = find_vma(mm, start);
-			continue;
+		if (file && (vma->vm_flags & VM_SHARED)) {
+			if (flags & MS_ASYNC)
+				vma_wrprotect(vma);
+			if (flags & MS_SYNC) {
+				get_file(file);
+				up_read(&mm->mmap_sem);
+				error = do_fsync(file, 0);
+				fput(file);
+				if (error || start >= end)
+					goto out;
+				down_read(&mm->mmap_sem);
+				vma = find_vma(mm, start);
+				continue;
+			}
 		}
 
 		vma = vma->vm_next;

Signature · 1 line

1.4.4.4

Quoted 26 lines

mm/msync.c +18 -0

index a4de868..a49af28 100644
@@ -13,11 +13,33 @@
 #include <linux/syscalls.h>
 
 /*
+ * Scan the PTEs for pages belonging to the VMA and mark them read-only.
+ * It will force a pagefault on the next write access.
+ */
+static void vma_wrprotect(struct vm_area_struct *vma)
+{
+	unsigned long addr;
+
+	for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
+		spinlock_t *ptl;
+		pgd_t *pgd = pgd_offset(vma->vm_mm, addr);
+		pud_t *pud = pud_offset(pgd, addr);
+		pmd_t *pmd = pmd_offset(pud, addr);
+		pte_t *pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
+
+		if (pte_dirty(*pte) && pte_write(*pte))
+			*pte = pte_wrprotect(*pte);
+		pte_unmap_unlock(pte, ptl);
+	}

mm/msync.c line 34

Quoted 1 line

mm/msync.c +1 -0

+}

What about ram based filesystems? They don't start out with read-only pte's, so I think they don't want them read-protected now either. Unless this is essential for correct mtime/ctime accounting on these filesystems (I don't think it really is). But then the mapping should start out read-only as well, otherwise the time update will only work after an msync(MS_ASYNC).

Quoted 26 lines · Miklos Szeredi

Quoted 26 lines

mm/msync.c +18 -0

index a4de868..a49af28 100644
@@ -13,11 +13,33 @@
 #include <linux/syscalls.h>
 
 /*
+ * Scan the PTEs for pages belonging to the VMA and mark them read-only.
+ * It will force a pagefault on the next write access.
+ */
+static void vma_wrprotect(struct vm_area_struct *vma)
+{
+	unsigned long addr;
+
+	for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
+		spinlock_t *ptl;
+		pgd_t *pgd = pgd_offset(vma->vm_mm, addr);
+		pud_t *pud = pud_offset(pgd, addr);
+		pmd_t *pmd = pmd_offset(pud, addr);
+		pte_t *pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
+
+		if (pte_dirty(*pte) && pte_write(*pte))
+			*pte = pte_wrprotect(*pte);
+		pte_unmap_unlock(pte, ptl);
+	}

Quoted 8 lines

mm/msync.c line 34

Quoted 1 line

mm/msync.c +1 -0

+}

What about ram based filesystems? They don't start out with read-only pte's, so I think they don't want them read-protected now either. Unless this is essential for correct mtime/ctime accounting on these filesystems (I don't think it really is). But then the mapping should start out read-only as well, otherwise the time update will only work after an msync(MS_ASYNC).

page_mkclean() has all the needed logic for this, it also walks the rmap and cleans out all other users, which I think is needed too for consistencies sake:

Process A Process B

mmap(foo.txt) mmap(foo.txt)

dirty page
dirty page

msync(MS_ASYNC)

dirty page

msync(MS_ASYNC) <--- now what?!

So what I would suggest is using the page table walkers from mm, and walks the page range, obtain the page using vm_normal_page() and call page_mkclean(). (Oh, and ensure you don't nest the pte lock :-)

All in all, that sounds rather expensive..

Quoted 55 lines · Miklos Szeredi

Quoted 27 lines

Quoted 27 lines

mm/msync.c +18 -0

index a4de868..a49af28 100644
@@ -13,11 +13,33 @@
 #include <linux/syscalls.h>
 
 /*
+ * Scan the PTEs for pages belonging to the VMA and mark them read-only.
+ * It will force a pagefault on the next write access.
+ */
+static void vma_wrprotect(struct vm_area_struct *vma)
+{
+ unsigned long addr;
+
+ for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
+         spinlock_t *ptl;
+         pgd_t *pgd = pgd_offset(vma->vm_mm, addr);
+         pud_t *pud = pud_offset(pgd, addr);
+         pmd_t *pmd = pmd_offset(pud, addr);
+         pte_t *pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
+
+         if (pte_dirty(*pte) && pte_write(*pte))
+                 *pte = pte_wrprotect(*pte);
+         pte_unmap_unlock(pte, ptl);
+ }

Quoted 8 lines

mm/msync.c line 34

Quoted 1 line

mm/msync.c +1 -0

+}

What about ram based filesystems? They don't start out with read-only pte's, so I think they don't want them read-protected now either. Unless this is essential for correct mtime/ctime accounting on these filesystems (I don't think it really is). But then the mapping should start out read-only as well, otherwise the time update will only work after an msync(MS_ASYNC).

page_mkclean() has all the needed logic for this, it also walks the rmap and cleans out all other users, which I think is needed too for consistencies sake:

Process A Process B

mmap(foo.txt) mmap(foo.txt)

dirty page
dirty page

msync(MS_ASYNC)

dirty page

msync(MS_ASYNC) <--- now what?!

Quoted 5 lines

So what I would suggest is using the page table walkers from mm, and walks the page range, obtain the page using vm_normal_page() and call page_mkclean(). (Oh, and ensure you don't nest the pte lock :-)

All in all, that sounds rather expensive..

Bah, and will break on s390... so we'd need a page_mkclean() variant that doesn't actually clear dirty.

Bah, and will break on s390... so we'd need a page_mkclean() variant that doesn't actually clear dirty.

And even in that four-liner, I suspect that the *last* two lines are actually incorrect: there's no point in updating the file time when the page *becomes* dirty,

we should update the file time when it is marked clean, and "msync(MS_SYNC)" should update it as part of *that*.

Quoted 3 lines

And even in that four-liner, I suspect that the *last* two lines are actually incorrect: there's no point in updating the file time when the page *becomes* dirty,

Actually all four lines do that. The first two for a write access on a present, read-only pte, the other two for a write on a non-present pte.

Quoted 2 lines

we should update the file time when it is marked clean, and "msync(MS_SYNC)" should update it as part of *that*.

That would need a new page flag (PG_mmap_dirty?). Do we have one available?

Quoted 8 lines

Quoted 3 lines

And even in that four-liner, I suspect that the *last* two lines are actually incorrect: there's no point in updating the file time when the page *becomes* dirty,

Actually all four lines do that. The first two for a write access on a present, read-only pte, the other two for a write on a non-present pte.

Quoted 5 lines

Quoted 2 lines

we should update the file time when it is marked clean, and "msync(MS_SYNC)" should update it as part of *that*.

That would need a new page flag (PG_mmap_dirty?). Do we have one available?

I thought the page writing stuff looked at (and cleared) the pte dirty bit, too?

That would need a new page flag (PG_mmap_dirty?). Do we have one available?

Quoted 2 lines

That would need a new page flag (PG_mmap_dirty?). Do we have one available?

Yeah, that would be bad. We probably have flags free, but those page flags are always a pain. Scratch that.

How about just setting a per-vma dirty flag, and then instead of updating the mtime when taking the dirty-page fault, we just set that flag?

Then, on unmap and msync, we just do

	if (vma->dirty-flag) {
		vma->dirty_flag = 0;
		update_file_times(vma->vm_file);
	}

and be done with it?

But then background writeout, sync(2), etc, wouldn't update the times.

Quoted 2 lines

But then background writeout, sync(2), etc, wouldn't update the times.

Sure it would, but only when doing the final unmap.

Did you miss the "on unmap and msync" part?

What I'm saying is that the times could be left un-updated for a long time if program doesn't do munmap() or msync(MS_SYNC) for a long time.

Quoted 3 lines

What I'm saying is that the times could be left un-updated for a long time if program doesn't do munmap() or msync(MS_SYNC) for a long time.

Sure.

But in those circumstances, the programmer cannot depend on the mtime *anyway* (because there is no synchronization), so what's the downside?

Let's face it, there's exactly three possible solutions:

- the insane one: trap EVERY SINGLE instruction that does a write to the page, and update mtime each and every time.

This one is so obviously STUPID that it's not even worth discussing further, except to say that "yes, there is an 'exact' algorithm, but no, we are never EVER going to use it".

- the non-exact solutions that don't give you mtime updates every time a write to the page happens, but give *some* guarantees for things that will update it.

This is the one I think we can do, and the only things a programmer can impact using it is "msync()" and "munmap()", since no other operations really have any thing to do with it in a programmer-visible way (ie a normal "sync" operation may happen in the background and has no progam-relevant timing information)

Other things *may* or may not update mtime (some filesystems - take most networked one as an example - will *always* update mtime on the server on writeback, so we cannot ever guarantee that nothing but msync/munmap does so), but at least we'll have a minimum set of things that people can depend on.

- the "we don't care at all solutions".

mmap(MAP_WRITE) doesn't really update times reliably after the write has happened (but might do it *before* - maybe the mmap() itself does).

Those are the three choices, I think. We currently approximate #3. We *can* do #2 (and there are various flavors of it). And even *aiming* for #1 is totally insane and stupid.

Linus

The current solution doesn't hit the performance at all when compared to the competitor POSIX-compliant systems. It is faster and does even more than the POSIX standard requires.

Quoted 4 lines

The current solution doesn't hit the performance at all when compared to the competitor POSIX-compliant systems. It is faster and does even more than the POSIX standard requires.

Your current patches have two problems:
- they are simply unnecessarily invasive for a relatively simple issue
- all versions I've looked at closer are buggy too

Example:

+ if (pte_dirty(*pte) && pte_write(*pte))
+ *pte = pte_wrprotect(*pte);

Uhhuh. Looks simple enough. Except it does a non-atomic pte access while other CPU's may be accessing it and updating it from their hw page table walkers. What will happen? Who knows? I can see lost access bits at a minimum.

IOW, this isn't simple code. It's code that it is simple to screw up. In this case, you really need to use ptep_set_wrprotect(), for example.

So why not do it in many fewer lines with that simpler vma->dirty flag?

Linus

Before using pte_wrprotect() the vma_wrprotect() routine uses the pte_offset_map_lock() macro to get the PTE and to acquire the ptl spinlock. Why did you say that this code was not SMP-safe? It should be atomic, I think.

The POSIX standard requires the ctime and mtime stamps to be updated not later than at the second call to msync() with the MS_ASYNC flag.

Quoted 5 lines

Before using pte_wrprotect() the vma_wrprotect() routine uses the pte_offset_map_lock() macro to get the PTE and to acquire the ptl spinlock. Why did you say that this code was not SMP-safe? It should be atomic, I think.

It's atomic WITH RESPECT TO OTHER PEOPLE WHO GET THE LOCK.

Guess how much another x86 CPU cares when it sets the accessed bit in hardware?

Quoted 3 lines

The POSIX standard requires the ctime and mtime stamps to be updated not later than at the second call to msync() with the MS_ASYNC flag.

.. and that is no excuse for bad code.

Linus

The page_check_address() function is called from the page_mkclean_one() routine as follows:

                entry = ptep_clear_flush(vma, address, pte);
                entry = pte_wrprotect(entry);
                entry = pte_mkclean(entry);
                set_pte_at(mm, address, pte, entry);

Quoted 3 lines

The page_check_address() function is called from the page_mkclean_one() routine as follows:

.. and the page_mkclean_one() function is totally different.

Lookie here, this is the correct and complex sequence:

Quoted 4 lines

                entry = ptep_clear_flush(vma, address, pte);
                entry = pte_wrprotect(entry);
                entry = pte_mkclean(entry);
                set_pte_at(mm, address, pte, entry);

That's a rather expensive sequence, but it's done exactly because it has to be done that way. What it does is to

- *atomically* load the pte entry _and_ clear the old one in memory.

That's the

        entry = ptep_clear_flush(vma, address, pte);

thing, and it basically means that it's doing some architecture-specific magic to make sure that another CPU that accesses the PTE at the same time will never actually modify the pte (because it's clear and not valid)

- it then - while the page table is actually clear and invalid - takes the old value and turns it into the new one:

        entry = pte_wrprotect(entry);
        entry = pte_mkclean(entry);

- and finally, it replaces the entry with the new one:

        set_pte_at(mm, address, pte, entry);

which takes care to write the new entry in some specific way that is atomic wrt other CPU's (ie on 32-bit x86 with a 64-bit page table entry it writes the high word first, see the write barriers in "native_set_pte()" in include/asm-x86/pgtable-3level.h

Now, compare that subtle and correct thing with what is *not* correct:

        if (pte_dirty(*pte) && pte_write(*pte))
                *pte = pte_wrprotect(*pte);

which makes no effort at all to make sure that it's safe in case another CPU updates the accessed bit.

Now, arguably it's unlikely to cause horrible problems at least on x86, because:

- we only do this if the pte is already marked dirty, so while we can lose the accessed bit, we can *not* lose the dirty bit. And the accessed bit isn't such a big deal.

- it's not doing any of the "be careful about" ordering things, but since the really important bits aren't changing, ordering probably won't practically matter.

But the problem is that we have something like 24 different architectures, it's hard to make sure that none of them have issues.

In other words: it may well work in practice. But when these things go subtly wrong, they are *really* nasty to find, and the unsafe sequence is really not how it's supposed to be done. For example, you don't even flush the TLB, so even if there are no cross-CPU issues, there's probably going to be writable entries in the TLB that now don't match the page tables.

Will it matter? Again, probably impossible to see in practice. But ...

Linus

Quoted 3 lines · Miklos Szeredi

What I'm saying is that the times could be left un-updated for a long time if program doesn't do munmap() or msync(MS_SYNC) for a long time.

Sure.

But in those circumstances, the programmer cannot depend on the mtime *anyway* (because there is no synchronization), so what's the downside?

Can we get "if the write to the page hits the disk, the mtime has hit the disk already no less than SOME_GRANULARITY before"?

That is very important for computer forensics. Esp. in saving your ass!

Ok, now back again to making that fast :-)

- keep it simple. Let's face it, Linux has never ever given those guarantees before, and it's not is if anybody has really cared. Even now, the issue seems to be more about paper standards conformance than anything else.

Quoted 4 lines · On Fri, 18 Jan 2008 14:47:33 -0800 (PST) Linus Torvalds…

- keep it simple. Let's face it, Linux has never ever given those guarantees before, and it's not is if anybody has really cared. Even now, the issue seems to be more about paper standards conformance than anything else.

There is one issue which is way more than just standards conformance.

When a program changes file data through mmap(), at some point the mtime needs to be update so that backup programs know to back up the new version of the file.

Backup programs not seeing an updated mtime is a really big deal.

Quoted 1 line · Rik van Riel

Backup programs not seeing an updated mtime is a really big deal.

And that's fixed with the 4-line approach.

Reminds me, I've got a patch here for addressing that problem with loop mounts:

Writes to loop should update the mtime of the underlying file.

Signed-off-by Matt Mackall <[email protected]>

Reminds me, I've got a patch here for addressing that problem with loop mounts:

Writes to loop should update the mtime of the underlying file.

Signed-off-by Matt Mackall <[email protected]>

Index: l/drivers/block/loop.c
===================================================================

drivers/block/loop.c +1 -0

@@ -221,6 +221,7 @@ static int do_lo_send_aops(struct loop_d
 	offset = pos & ((pgoff_t)PAGE_CACHE_SIZE - 1);
 	bv_offs = bvec->bv_offset;
 	len = bvec->bv_len;
+	file_update_time(file);
 	while (len > 0) {
 		sector_t IV;
 		unsigned size;

Quoted 20 lines

Reminds me, I've got a patch here for addressing that problem with loop mounts:

Writes to loop should update the mtime of the underlying file.

Signed-off-by Matt Mackall <[email protected]>

Index: l/drivers/block/loop.c
===================================================================

drivers/block/loop.c +1 -0

@@ -221,6 +221,7 @@ static int do_lo_send_aops(struct loop_d
 	offset = pos & ((pgoff_t)PAGE_CACHE_SIZE - 1);
 	bv_offs = bvec->bv_offset;
 	len = bvec->bv_len;
+	file_update_time(file);
 	while (len > 0) {
 		sector_t IV;
 		unsigned size;

@@ -299,6 +300,7 @@ static int __do_lo_send_write(struct fil
 

drivers/block/loop.c static int __do_lo_send_write(struct fil · line 303

Quoted 3 lines

drivers/block/loop.c +1 -0

 	set_fs(get_ds());
 	bw = file->f_op->write(file, buf, len, &pos);
+	file_update_time(file);

->write should have already updated the times, no?

Quoted 7 lines · Ingo Oeser

Can we get "if the write to the page hits the disk, the mtime has hit the disk already no less than SOME_GRANULARITY before"?

That is very important for computer forensics. Esp. in saving your ass!

Ok, now back again to making that fast :-)

I certainly don't mind it if we have some tighter guarantees, but what I'd want is:

- keep it simple. Let's face it, Linux has never ever given those guarantees before, and it's not is if anybody has really cared. Even now, the issue seems to be more about paper standards conformance than anything else.

- I get worried about people playing around with the dirty bit in particular. We have had some really rather nasty bugs here. Most of which are totally impossible to trigger under normal loads (for example the old random-access utorrent writable mmap issue from about a year ago).

So these two issues - the big red danger signs flashing in my brain, coupled with the fact that no application has apparently ever really noticed in the last 15 years - just makes it a case where I'd like each step of the way to be obvious and simple and no larger than really absolutely necessary.

Quoted 10 lines · Linus Torvalds

Quoted 7 lines · Ingo Oeser

Can we get "if the write to the page hits the disk, the mtime has hit the disk already no less than SOME_GRANULARITY before"?

That is very important for computer forensics. Esp. in saving your ass!

Ok, now back again to making that fast :-)

Quoted 9 lines

I certainly don't mind it if we have some tighter guarantees, but what I'd want is:

- keep it simple. Let's face it, Linux has never ever given those guarantees before, and it's not is if anybody has really cared. Even now, the issue seems to be more about paper standards conformance than anything else.

I have been working on getting something supported here for because I have some very large Wall Street customers who do care about getting the mtime updated because their backups are getting corrupted. They are incomplete because although their applications update files, they don't get backed up because the mtime never changes.

Quoted 11 lines

- I get worried about people playing around with the dirty bit in particular. We have had some really rather nasty bugs here. Most of which are totally impossible to trigger under normal loads (for example the old random-access utorrent writable mmap issue from about a year ago).

So these two issues - the big red danger signs flashing in my brain, coupled with the fact that no application has apparently ever really noticed in the last 15 years - just makes it a case where I'd like each step of the way to be obvious and simple and no larger than really absolutely necessary.

Simple is good. However, too simple is not good. I would suggest that we implement file time updates which make sense and if they happen to follow POSIX, then nifty, otherwise, oh well.

    Thanx...

       ps

Quoted 27 lines

Quoted 27 lines

mm/msync.c +18 -0

index a4de868..a49af28 100644
@@ -13,11 +13,33 @@
 #include <linux/syscalls.h>
 
 /*
+ * Scan the PTEs for pages belonging to the VMA and mark them read-only.
+ * It will force a pagefault on the next write access.
+ */
+static void vma_wrprotect(struct vm_area_struct *vma)
+{
+	unsigned long addr;
+
+	for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
+		spinlock_t *ptl;
+		pgd_t *pgd = pgd_offset(vma->vm_mm, addr);
+		pud_t *pud = pud_offset(pgd, addr);
+		pmd_t *pmd = pmd_offset(pud, addr);
+		pte_t *pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
+
+		if (pte_dirty(*pte) && pte_write(*pte))
+			*pte = pte_wrprotect(*pte);
+		pte_unmap_unlock(pte, ptl);
+	}

Quoted 8 lines

mm/msync.c line 34

Quoted 1 line

mm/msync.c +1 -0

+}

What about ram based filesystems? They don't start out with read-only pte's, so I think they don't want them read-protected now either. Unless this is essential for correct mtime/ctime accounting on these filesystems (I don't think it really is). But then the mapping should start out read-only as well, otherwise the time update will only work after an msync(MS_ASYNC).

page_mkclean() has all the needed logic for this, it also walks the rmap and cleans out all other users, which I think is needed too for consistencies sake:

Process A Process B

mmap(foo.txt) mmap(foo.txt)

dirty page
dirty page

msync(MS_ASYNC)

dirty page

msync(MS_ASYNC) <--- now what?!

All in all, that sounds rather expensive..

Quoted 45 lines · Miklos Szeredi

Quoted 27 lines

Quoted 27 lines

mm/msync.c +18 -0

index a4de868..a49af28 100644
@@ -13,11 +13,33 @@
 #include <linux/syscalls.h>
 
 /*
+ * Scan the PTEs for pages belonging to the VMA and mark them read-only.
+ * It will force a pagefault on the next write access.
+ */
+static void vma_wrprotect(struct vm_area_struct *vma)
+{
+	unsigned long addr;
+
+	for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
+		spinlock_t *ptl;
+		pgd_t *pgd = pgd_offset(vma->vm_mm, addr);
+		pud_t *pud = pud_offset(pgd, addr);
+		pmd_t *pmd = pmd_offset(pud, addr);
+		pte_t *pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
+
+		if (pte_dirty(*pte) && pte_write(*pte))
+			*pte = pte_wrprotect(*pte);
+		pte_unmap_unlock(pte, ptl);
+	}

Quoted 8 lines

mm/msync.c line 34

Quoted 1 line

mm/msync.c +1 -0

+}

What about ram based filesystems? They don't start out with read-only pte's, so I think they don't want them read-protected now either. Unless this is essential for correct mtime/ctime accounting on these filesystems (I don't think it really is). But then the mapping should start out read-only as well, otherwise the time update will only work after an msync(MS_ASYNC).

page_mkclean() has all the needed logic for this, it also walks the rmap and cleans out all other users, which I think is needed too for consistencies sake:

Process A Process B

mmap(foo.txt) mmap(foo.txt)

Quoted 8 lines

dirty page
dirty page

msync(MS_ASYNC)

dirty page

msync(MS_ASYNC) <--- now what?!

mm/msync.c +9 -0

index 144a757..a1b3fc6 100644
@@ -14,6 +14,122 @@
 #include <linux/syscalls.h>
 #include <linux/sched.h>
 
+unsigned long masync_pte_range(struct vm_area_struct *vma, pmd_t *pdm,
+		unsigned long addr, unsigned long end)
+{
+	pte_t *pte;
+	spinlock_t *ptl;
+
+	pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
+	arch_enter_lazy_mmu_mode();
+	do {

mm/msync.c +83 -0

+			ptent = ptep_clear_flush(vma, addr, pte);
+			ptent = pte_wrprotect(ptent);
+			set_pte_at(vma->vm_mnm, addr, pte, ptent);
+		}
+	} while (pte++, addr += PAGE_SIZE, addr != end);
+	arch_leave_lazy_mmu_mode();
+	pte_unmap_unlock(pte - 1, ptl);
+
+	return addr;
+}
+
+unsigned long masync_pmd_range(struct vm_area_struct *vma, pud_t *pud,
+		unsigned long addr, unsigned long end)
+{
+	pmd_t *pmd;
+	unsigned long next;
+
+	pmd = pmd_offset(pud, addr);
+	do {
+		next = pmd_addr_end(addr, end);
+		if (pmd_none_or_clear_bad(pmd))
+			continue;
+		next = masync_pte_range(vma, pmd, addr, next);
+	} while (pmd++, addr = next, addr != end);
+
+	return addr;
+}
+
+unsigned long masync_pud_range(struct vm_area_struct *vma, pgd_t *pgd,
+	       	unsigned long addr, unsigned long end)
+{
+	pud_t *pud;
+	unsigned long next;
+
+	pud = pud_offset(pgd, addr);
+	do {
+		next = pud_addr_end(addr, end);
+		if (pud_none_or_clear_bad(pud))
+			continue;
+		next = masync_pmd_range(vma, pud, addr, next);
+	} while (pud++, addr = next, addr != end);
+
+	return addr;
+}
+
+unsigned long masync_pgd_range()
+{
+	pgd_t *pgd;
+	unsigned long next;
+
+	pgd = pgd_offset(vma->vm_mm, addr);
+	do {
+		next = pgd_addr_end(addr, end);
+		if (pgd_none_of_clear_bad(pgd))
+			continue;
+		next = masync_pud_range(vma, pgd, addr, next);
+	} while (pgd++, addr = next, addr != end);
+
+	return addr;
+}
+
+int masync_vma_one(struct vm_area_struct *vma,
+		unsigned long start, unsigned long end)
+{
+	if (start < vma->vm_start)
+		start = vma->vm_start;
+
+	if (end > vma->vm_end)
+		end = vma->vm_end;
+
+	masync_pgd_range(vma, start, end);
+
+	return 0;
+}
+
+int masync_vma(struct vm_area_struct *vma, 
+		unsigned long start, unsigned long end)
+{
+	struct address_space *mapping;
+	struct vm_area_struct *vma_iter;
+
+	if (!(vma->vm_flags & VM_SHARED))
+		return 0;

Quoted 22 lines

mm/msync.c +14 -0

index 144a757..a1b3fc6 100644
@@ -14,6 +14,122 @@
 #include <linux/syscalls.h>
 #include <linux/sched.h>
 
+unsigned long masync_pte_range(struct vm_area_struct *vma, pmd_t *pdm,
+		unsigned long addr, unsigned long end)
+{
+	pte_t *pte;
+	spinlock_t *ptl;
+
+	pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
+	arch_enter_lazy_mmu_mode();
+	do {
+		pte_t ptent = *pte;
+
+		if (pte_none(ptent))
+			continue;
+

mm/msync.c line 35

Quoted 5 lines

mm/msync.c +5 -0

+		if (!pte_present(ptent))
+			continue;
+
+		if (pte_dirty(ptent) && pte_write(ptent)) {
+			flush_cache_page(vma, addr, pte_pfn(ptent));

Hmm, I'm not sure flush_cache_page() is needed. Or does does dirty data in the cache somehow interfere with the page protection?

Quoted 88 lines

mm/msync.c +88 -0

+			ptent = ptep_clear_flush(vma, addr, pte);
+			ptent = pte_wrprotect(ptent);
+			set_pte_at(vma->vm_mnm, addr, pte, ptent);
+		}
+	} while (pte++, addr += PAGE_SIZE, addr != end);
+	arch_leave_lazy_mmu_mode();
+	pte_unmap_unlock(pte - 1, ptl);
+
+	return addr;
+}
+
+unsigned long masync_pmd_range(struct vm_area_struct *vma, pud_t *pud,
+		unsigned long addr, unsigned long end)
+{
+	pmd_t *pmd;
+	unsigned long next;
+
+	pmd = pmd_offset(pud, addr);
+	do {
+		next = pmd_addr_end(addr, end);
+		if (pmd_none_or_clear_bad(pmd))
+			continue;
+		next = masync_pte_range(vma, pmd, addr, next);
+	} while (pmd++, addr = next, addr != end);
+
+	return addr;
+}
+
+unsigned long masync_pud_range(struct vm_area_struct *vma, pgd_t *pgd,
+	       	unsigned long addr, unsigned long end)
+{
+	pud_t *pud;
+	unsigned long next;
+
+	pud = pud_offset(pgd, addr);
+	do {
+		next = pud_addr_end(addr, end);
+		if (pud_none_or_clear_bad(pud))
+			continue;
+		next = masync_pmd_range(vma, pud, addr, next);
+	} while (pud++, addr = next, addr != end);
+
+	return addr;
+}
+
+unsigned long masync_pgd_range()
+{
+	pgd_t *pgd;
+	unsigned long next;
+
+	pgd = pgd_offset(vma->vm_mm, addr);
+	do {
+		next = pgd_addr_end(addr, end);
+		if (pgd_none_of_clear_bad(pgd))
+			continue;
+		next = masync_pud_range(vma, pgd, addr, next);
+	} while (pgd++, addr = next, addr != end);
+
+	return addr;
+}
+
+int masync_vma_one(struct vm_area_struct *vma,
+		unsigned long start, unsigned long end)
+{
+	if (start < vma->vm_start)
+		start = vma->vm_start;
+
+	if (end > vma->vm_end)
+		end = vma->vm_end;
+
+	masync_pgd_range(vma, start, end);
+
+	return 0;
+}
+
+int masync_vma(struct vm_area_struct *vma, 
+		unsigned long start, unsigned long end)
+{
+	struct address_space *mapping;
+	struct vm_area_struct *vma_iter;
+
+	if (!(vma->vm_flags & VM_SHARED))
+		return 0;
+
+	mapping = vma->vm_file->f_mapping;
+
+	if (!mapping_cap_account_dirty(mapping))
+		return 0;

mm/msync.c line 128

Quoted 5 lines

mm/msync.c +5 -0

+
+	spin_lock(&mapping->i_mmap_lock);
+	vma_prio_tree_foreach(vma_iter, &iter, &mapping->i_mmap, start, end)
+		masync_vma_one(vma_iter, start, end);
+	spin_unlock(&mapping->i_mmap_lock);

This is hoding i_mmap_lock for possibly quite long. Isn't that going to cause problems?

From a quick glance at prio_tree.c the iterator isn't valid anymore

Possibly, I didn't see a quick way to break that iteration.

Quoted 1 line

From a quick glance at prio_tree.c the iterator isn't valid anymore

after releasing i_mmap_lock. Fixing that would be,.. 'fun'.

I also realized I forgot to copy/paste the prio_tree_iter declaration and ought to make all these functions static.

But for a quick draft it conveys the idea pretty well, I guess :-)

4. Performance test was done using the program available from the following link:

http://bugzilla.kernel.org/attachment.cgi?id=14493

Result: the impact of the changes was negligible for files of a few hundred megabytes.

Quoted 7 lines

4. Performance test was done using the program available from the following link:

http://bugzilla.kernel.org/attachment.cgi?id=14493

Result: the impact of the changes was negligible for files of a few hundred megabytes.

Could you also test with ext4 and post some numbers? Afaik, ext4 uses nanosecond timestamps, so the time updating code would be exercised more during the page faults.

What about performance impact on msync(MS_ASYNC)? Could you please do some measurment of that as well?

Thanks,
Miklos

Quoted 7 lines

4. Performance test was done using the program available from the following link:

http://bugzilla.kernel.org/attachment.cgi?id=14493

Result: the impact of the changes was negligible for files of a few hundred megabytes.

Could you also test with ext4 and post some numbers? Afaik, ext4 uses nanosecond timestamps, so the time updating code would be exercised more during the page faults.

What about performance impact on msync(MS_ASYNC)? Could you please do some measurment of that as well?

What about performance impact on msync(MS_ASYNC)? Could you please do some measurment of that as well?

Quoted 1 line

Quoted 1 line

Thanks,
Miklos

2008/1/18, Miklos Szeredi <[email protected]>:

Quoted 8 lines

Quoted 7 lines

4. Performance test was done using the program available from the following link:

http://bugzilla.kernel.org/attachment.cgi?id=14493

Result: the impact of the changes was negligible for files of a few hundred megabytes.

Quoted 6 lines

Could you also test with ext4 and post some numbers? Afaik, ext4 uses nanosecond timestamps, so the time updating code would be exercised more during the page faults.

What about performance impact on msync(MS_ASYNC)? Could you please do some measurment of that as well?

Did a quick test on an ext4 partition. This is how it looks like:

Table 1. Reference platforms.

------------------------------------------------------------
| | HP-UX/PA-RISC | HP-UX/Itanium | FreeBSD |
------------------------------------------------------------
| First run | 263405 usec | 202283 usec | 90 SECONDS |
------------------------------------------------------------
| Second run | 262253 usec | 172837 usec | 90 SECONDS |
------------------------------------------------------------
| Third run | 238465 usec | 238465 usec | 90 SECONDS |
------------------------------------------------------------

It looks like FreeBSD is a clear outsider here. Note that FreeBSD showed an almost liner depencence of the time spent in the msync(MS_ASYNC) call on the file size.

Table 2. The Qemu system. File size is 512M.

---------------------------------------------------
| | Before the patch | After the patch |
---------------------------------------------------
| First run | 35 usec | 5852 usec |
---------------------------------------------------
| Second run | 35 usec | 4444 usec |
---------------------------------------------------
| Third run | 35 usec | 6330 usec |
---------------------------------------------------