Updating ctime and mtime for memory-mapped files [try #4]

I suspect your patch is ignoring writes after the first msync, but then why care about msync at all? What's so special about the _first_ msync? Is it just that most test programs only check this, and not what happens if msync is called more than once? That would be a bug in the test cases.

I must agree, doing the mmap dirty, MS_ASYNC, mmap retouch, MS_ASYNC case correctly would need a lot more code which I doubt is worth the effort.

It would require scanning the PTEs and marking them read-only again on MS_ASYNC, and some more logic in set_page_dirty() because that currently bails out early if the page in question is already dirty.

A better architecture would be to arrange for the file times to be updated when the page makes the transition from being unmodified to modified. This is not straightforward due to the current locking, but should be doable, I think. Perhaps recording the current time and then using it to update the file times at a more suitable time (no pun intended) might work.

mm/msync.c +0 -0

@@ -33,71 +34,65 @@ asmlinkage long sys_msync(unsigned long start, size_t len, int flags)
 	unsigned long end;
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;

mm/msync.c line 8

Quoted 10 lines

+++ b/mm/msync.c

mm/msync.c +4 -0

@@ -1,24 +1,25 @@
 /*
  *   linux/mm/msync.c
  *
+ * The msync() system call.
  * Copyright (C) 1994-1999  Linus Torvalds
+ *
+ * Massive code cleanup.
+ * Copyright (C) 2008 Anton Salikhmetov <[email protected]>

Please don't put the changelog in here, that's what the log in the SCM is for. And while you're at it remove the confusing file name comment. This should now look something like:

/*
* The msync() system call.
*
* Copyright (C) 1994-1999 Linus Torvalds
* Copyright (C) 2008 Anton Salikhmetov <[email protected]> */

Quoted 9 lines

mm/msync.c +1 -2

@@ -33,71 +34,65 @@ asmlinkage long sys_msync(unsigned long start, size_t len, int flags)
      unsigned long end;
      struct mm_struct *mm = current->mm;
      struct vm_area_struct *vma;
-     int unmapped_error = 0;
-     int error = -EINVAL;
+     int error = 0, unmapped_error = 0;
 

mm/msync.c asmlinkage long sys_msync(unsigned long start, size_t len, int flags) · line 42

Quoted 6 lines

mm/msync.c +2 -2

      if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
-             goto out;
+             return -EINVAL;
      if (start & ~PAGE_MASK)
-             goto out;
+             return -EINVAL;

The goto out for a simple return style is used quite commonly in kernel code to have a single return statement which makes code maintaince, e.g. adding locks or allocations simpler. Not sure that getting rid of it makes a lot of sense.

mm/msync.c asmlinkage long sys_msync(unsigned long start, size_t len, int flags) · line 81

Quoted 3 lines

mm/msync.c +2 -0

+             file = vma->vm_file;
+             if ((flags & MS_SYNC) && file && (vma->vm_flags & VM_SHARED)) {

Given that file is assigned just above it would be more readable to test it first.

mm/msync.c asmlinkage long sys_msync(unsigned long start, size_t len, int flags) · line 87

Quoted 3 lines

mm/msync.c +2 -0

+                     if (error)
+                             return error;

This should be an goto out, returns out of the middle of the function make reading and maintaining the code not so nice.

mm/msync.c asmlinkage long sys_msync(unsigned long start, size_t len, int flags) · line 97

Quoted 2 lines

mm/msync.c +0 -0

      return error ? : unmapped_error;

Care to get rid of this odd GNU extension while you're at it and use the proper

        return error ? error : unmapped_error;

?

2008/1/15, Christoph Hellwig <[email protected]>:

Quoted 10 lines · Anton Salikhmetov

Quoted 9 lines

mm/msync.c +1 -2

@@ -33,71 +34,65 @@ asmlinkage long sys_msync(unsigned long start, size_t len, int flags)
      unsigned long end;
      struct mm_struct *mm = current->mm;
      struct vm_area_struct *vma;
-     int unmapped_error = 0;
-     int error = -EINVAL;
+     int error = 0, unmapped_error = 0;
 

Quoted 11 lines

mm/msync.c asmlinkage long sys_msync(unsigned long start, size_t len, int flags) · line 42

Quoted 6 lines

mm/msync.c +2 -2

      if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
-             goto out;
+             return -EINVAL;
      if (start & ~PAGE_MASK)
-             goto out;
+             return -EINVAL;

The goto out for a simple return style is used quite commonly in kernel code to have a single return statement which makes code maintaince, e.g. adding locks or allocations simpler. Not sure that getting rid of it makes a lot of sense.

Sorry, I can't agree. That's what is written in the CodingStyle document:

The goto statement comes in handy when a function exits from multiple locations and some common work such as cleanup has to be done.

The second part of requirement does not hold true for the sys_msync() routine.

Quoted 6 lines

mm/msync.c asmlinkage long sys_msync(unsigned long start, size_t len, int flags) · line 81

Quoted 3 lines

mm/msync.c +2 -0

+             file = vma->vm_file;
+             if ((flags & MS_SYNC) && file && (vma->vm_flags & VM_SHARED)) {

Given that file is assigned just above it would be more readable to test it first.

The second part of my solution changes this code, anyway.

Quoted 1 line

Quoted 5 lines

mm/msync.c asmlinkage long sys_msync(unsigned long start, size_t len, int flags) · line 87

Quoted 2 lines

mm/msync.c +2 -0

+                     if (error)
+                             return error;

This should be an goto out, returns out of the middle of the function make reading and maintaining the code not so nice.

Sorry, I don't think so. No "common cleanup" is needed here.

Quoted 26 lines

2008/1/15, Christoph Hellwig <[email protected]>:

Quoted 10 lines · Anton Salikhmetov

Quoted 9 lines

mm/msync.c +1 -2

@@ -33,71 +34,65 @@ asmlinkage long sys_msync(unsigned long start, size_t len, int flags)
      unsigned long end;
      struct mm_struct *mm = current->mm;
      struct vm_area_struct *vma;
-     int unmapped_error = 0;
-     int error = -EINVAL;
+     int error = 0, unmapped_error = 0;
 

Quoted 11 lines

mm/msync.c asmlinkage long sys_msync(unsigned long start, size_t len, int flags) · line 42

Quoted 6 lines

mm/msync.c +2 -2

      if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
-             goto out;
+             return -EINVAL;
      if (start & ~PAGE_MASK)
-             goto out;
+             return -EINVAL;

The goto out for a simple return style is used quite commonly in kernel code to have a single return statement which makes code maintaince, e.g. adding locks or allocations simpler. Not sure that getting rid of it makes a lot of sense.

Sorry, I can't agree. That's what is written in the CodingStyle document:

Quoted 2 lines

The goto statement comes in handy when a function exits from multiple locations and some common work such as cleanup has to be done.

CodingStyle does not try to cover Everything. Nor do we want it to.

At any rate, there is a desire for functions to have a single point of return, regardless of the amount of cleanup to be done, so I agree with Christoph's comments.

Quoted 17 lines

The second part of requirement does not hold true for the sys_msync() routine.

Quoted 6 lines

mm/msync.c asmlinkage long sys_msync(unsigned long start, size_t len, int flags) · line 81

Quoted 3 lines

mm/msync.c +2 -0

+             file = vma->vm_file;
+             if ((flags & MS_SYNC) && file && (vma->vm_flags & VM_SHARED)) {

Given that file is assigned just above it would be more readable to test it first.

The second part of my solution changes this code, anyway.

Quoted 4 lines

Quoted 3 lines

mm/msync.c +2 -0

+                     if (error)
+                             return error;

Quoted 4 lines

Quoted 2 lines

This should be an goto out, returns out of the middle of the function make reading and maintaining the code not so nice.

Sorry, I don't think so. No "common cleanup" is needed here.

---
~Randy

2008/1/15, Randy Dunlap <[email protected]>:

Quoted 27 lines · On Tue, 15 Jan 2008 22:02:54 +0300 Anton Salikhmetov wrote:

Quoted 26 lines

2008/1/15, Christoph Hellwig <[email protected]>:

Quoted 10 lines · Anton Salikhmetov

Quoted 9 lines

mm/msync.c +1 -2

@@ -33,71 +34,65 @@ asmlinkage long sys_msync(unsigned long start, size_t len, int flags)
      unsigned long end;
      struct mm_struct *mm = current->mm;
      struct vm_area_struct *vma;
-     int unmapped_error = 0;
-     int error = -EINVAL;
+     int error = 0, unmapped_error = 0;
 

Quoted 11 lines

mm/msync.c asmlinkage long sys_msync(unsigned long start, size_t len, int flags) · line 42

Quoted 6 lines

mm/msync.c +2 -2

      if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
-             goto out;
+             return -EINVAL;
      if (start & ~PAGE_MASK)
-             goto out;
+             return -EINVAL;

The goto out for a simple return style is used quite commonly in kernel code to have a single return statement which makes code maintaince, e.g. adding locks or allocations simpler. Not sure that getting rid of it makes a lot of sense.

Sorry, I can't agree. That's what is written in the CodingStyle document:

Quoted 8 lines

Quoted 2 lines

The goto statement comes in handy when a function exits from multiple locations and some common work such as cleanup has to be done.

CodingStyle does not try to cover Everything. Nor do we want it to.

At any rate, there is a desire for functions to have a single point of return, regardless of the amount of cleanup to be done, so I agree with Christoph's comments.

Should I replace "return -EINVAL;" statement with the following?

{
    error = -EINVAL;
    goto out;
}

Notice that error is already -EINVAL, so a simple goto should suffice.

Quoted 1 line

Notice that error is already -EINVAL, so a simple goto should suffice.

Yes, for the start of the function you can basically leave it as-is.

Quoted 25 lines · On Tue, 15 Jan 2008 22:02:54 +0300 Anton Salikhmetov wrote:

2008/1/15, Christoph Hellwig <[email protected]>:

Quoted 10 lines · Anton Salikhmetov

Quoted 9 lines

mm/msync.c +1 -2

@@ -33,71 +34,65 @@ asmlinkage long sys_msync(unsigned long start, size_t len, int flags)
      unsigned long end;
      struct mm_struct *mm = current->mm;
      struct vm_area_struct *vma;
-     int unmapped_error = 0;
-     int error = -EINVAL;
+     int error = 0, unmapped_error = 0;
 

Quoted 11 lines

mm/msync.c asmlinkage long sys_msync(unsigned long start, size_t len, int flags) · line 42

Quoted 6 lines

mm/msync.c +2 -2

      if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
-             goto out;
+             return -EINVAL;
      if (start & ~PAGE_MASK)
-             goto out;
+             return -EINVAL;

The goto out for a simple return style is used quite commonly in kernel code to have a single return statement which makes code maintaince, e.g. adding locks or allocations simpler. Not sure that getting rid of it makes a lot of sense.

Sorry, I can't agree. That's what is written in the CodingStyle document:

Quoted 2 lines

The goto statement comes in handy when a function exits from multiple locations and some common work such as cleanup has to be done.

CodingStyle does not try to cover Everything. Nor do we want it to.

At any rate, there is a desire for functions to have a single point of return, regardless of the amount of cleanup to be done, so I agree with Christoph's comments.

Quoted 27 lines · Randy Dunlap

Quoted 25 lines · On Tue, 15 Jan 2008 22:02:54 +0300 Anton Salikhmetov wrote:

2008/1/15, Christoph Hellwig <[email protected]>:

Quoted 10 lines · Anton Salikhmetov

Quoted 9 lines

mm/msync.c +1 -2

@@ -33,71 +34,65 @@ asmlinkage long sys_msync(unsigned long start, size_t len, int flags)
      unsigned long end;
      struct mm_struct *mm = current->mm;
      struct vm_area_struct *vma;
-     int unmapped_error = 0;
-     int error = -EINVAL;
+     int error = 0, unmapped_error = 0;
 

Quoted 11 lines

mm/msync.c asmlinkage long sys_msync(unsigned long start, size_t len, int flags) · line 42

Quoted 6 lines

mm/msync.c +2 -2

      if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
-             goto out;
+             return -EINVAL;
      if (start & ~PAGE_MASK)
-             goto out;
+             return -EINVAL;

The goto out for a simple return style is used quite commonly in kernel code to have a single return statement which makes code maintaince, e.g. adding locks or allocations simpler. Not sure that getting rid of it makes a lot of sense.

Sorry, I can't agree. That's what is written in the CodingStyle document:

Quoted 8 lines

Quoted 2 lines

The goto statement comes in handy when a function exits from multiple locations and some common work such as cleanup has to be done.

CodingStyle does not try to cover Everything. Nor do we want it to.

At any rate, there is a desire for functions to have a single point of return, regardless of the amount of cleanup to be done, so I agree with Christoph's comments.

When we're not cleaning up resources, the main advantage of having a single point of return is that you can trace backwards from the return point through the function's logic. But that advantage flies right out the window when you use gotos. You still have to figure out how you got to the return statement by tracing back and looking at all the possible gotos. And the "goto out" style adds bulk and non-negligible complexity when we've got to search back for what the last explicitly set value of "ret" or "error" or whatever the function in question is using was. Sometimes people get this wrong ("retval is already -EINVAL, so I don't need to explicitly set it"), and create bugs.

So I think if we're not actually going to use "structured programming" (no gotos) or "stack cleanup" styles, the single return point style is more trouble than it's worth.

A lesser advantage of the single return point is that you can set a breakpoint or put a printk at the end of a function. But I don't think that's much justification.

mm/page-writeback.c +4 -15

index 3d3848f..53d0e34 100644
@@ -997,35 +997,39 @@ int __set_page_dirty_no_writeback(struct page *page)
  */
 int __set_page_dirty_nobuffers(struct page *page)
 {
-	if (!TestSetPageDirty(page)) {
-		struct address_space *mapping = page_mapping(page);
-		struct address_space *mapping2;
+	struct address_space *mapping = page_mapping(page);
+	struct address_space *mapping2;
 
-		if (!mapping)
-			return 1;
+	if (!mapping)
+		return 1;
 
-		write_lock_irq(&mapping->tree_lock);
-		mapping2 = page_mapping(page);
-		if (mapping2) { /* Race with truncate? */
-			BUG_ON(mapping2 != mapping);
-			WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
-			if (mapping_cap_account_dirty(mapping)) {
-				__inc_zone_page_state(page, NR_FILE_DIRTY);
-				__inc_bdi_stat(mapping->backing_dev_info,
-						BDI_RECLAIMABLE);
-				task_io_account_write(PAGE_CACHE_SIZE);

mm/page-writeback.c +23 -2

+
+	if (TestSetPageDirty(page))
+		return 0;
+
+	write_lock_irq(&mapping->tree_lock);
+	mapping2 = page_mapping(page);
+	if (mapping2) {
+		/* Race with truncate? */
+		BUG_ON(mapping2 != mapping);
+		WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
+		if (mapping_cap_account_dirty(mapping)) {
+			__inc_zone_page_state(page, NR_FILE_DIRTY);
+			__inc_bdi_stat(mapping->backing_dev_info,
+					BDI_RECLAIMABLE);
+			task_io_account_write(PAGE_CACHE_SIZE);
 		}
-		return 1;
+		radix_tree_tag_set(&mapping->page_tree,
+				page_index(page), PAGECACHE_TAG_DIRTY);
 	}
-	return 0;
+	write_unlock_irq(&mapping->tree_lock);
+
+	if (mapping->host)
+		__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
+
+	return 1;
 }
 EXPORT_SYMBOL(__set_page_dirty_nobuffers);
 

Quoted 30 lines

mm/page-writeback.c +4 -15

index 3d3848f..53d0e34 100644
@@ -997,35 +997,39 @@ int __set_page_dirty_no_writeback(struct page *page)
  */
 int __set_page_dirty_nobuffers(struct page *page)
 {
-     if (!TestSetPageDirty(page)) {
-             struct address_space *mapping = page_mapping(page);
-             struct address_space *mapping2;
+     struct address_space *mapping = page_mapping(page);
+     struct address_space *mapping2;
 
-             if (!mapping)
-                     return 1;
+     if (!mapping)
+             return 1;
 
-             write_lock_irq(&mapping->tree_lock);
-             mapping2 = page_mapping(page);
-             if (mapping2) { /* Race with truncate? */
-                     BUG_ON(mapping2 != mapping);
-                     WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
-                     if (mapping_cap_account_dirty(mapping)) {
-                             __inc_zone_page_state(page, NR_FILE_DIRTY);
-                             __inc_bdi_stat(mapping->backing_dev_info,
-                                             BDI_RECLAIMABLE);
-                             task_io_account_write(PAGE_CACHE_SIZE);

mm/page-writeback.c int __set_page_dirty_no_writeback(struct page *page) · line 1007

Quoted 10 lines

mm/page-writeback.c +2 -8

-                     }
-                     radix_tree_tag_set(&mapping->page_tree,
-                             page_index(page), PAGECACHE_TAG_DIRTY);
-             }
-             write_unlock_irq(&mapping->tree_lock);
-             if (mapping->host) {
-                     /* !PageAnon && !swapper_space */
-                     __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
+     mapping->mtime = CURRENT_TIME;
+     set_bit(AS_MCTIME, &mapping->flags);

This seems vulnerable to the race we have against truncate, handled by the mapping2 magic below. Do we care?

Quoted 17 lines

mm/page-writeback.c +15 -1

+
+     if (TestSetPageDirty(page))
+             return 0;
+
+     write_lock_irq(&mapping->tree_lock);
+     mapping2 = page_mapping(page);
+     if (mapping2) {
+             /* Race with truncate? */
+             BUG_ON(mapping2 != mapping);
+             WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
+             if (mapping_cap_account_dirty(mapping)) {
+                     __inc_zone_page_state(page, NR_FILE_DIRTY);
+                     __inc_bdi_stat(mapping->backing_dev_info,
+                                     BDI_RECLAIMABLE);
+                     task_io_account_write(PAGE_CACHE_SIZE);
              }
-             return 1;

mm/page-writeback.c int __set_page_dirty_no_writeback(struct page *page) · line 1030

Quoted 8 lines

mm/page-writeback.c +6 -1

+             radix_tree_tag_set(&mapping->page_tree,
+                             page_index(page), PAGECACHE_TAG_DIRTY);
      }
-     return 0;
+     write_unlock_irq(&mapping->tree_lock);
+
+     if (mapping->host)
+             __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);

mm/page-writeback.c int __set_page_dirty_no_writeback(struct page *page) · line 1035

Quoted 5 lines

mm/page-writeback.c +2 -0

+
+     return 1;
 }
 EXPORT_SYMBOL(__set_page_dirty_nobuffers);
 

2008/1/15, Peter Zijlstra <[email protected]>:

Quoted 68 lines · Anton Salikhmetov

Quoted 30 lines

mm/page-writeback.c +4 -15

index 3d3848f..53d0e34 100644
@@ -997,35 +997,39 @@ int __set_page_dirty_no_writeback(struct page *page)
  */
 int __set_page_dirty_nobuffers(struct page *page)
 {
-     if (!TestSetPageDirty(page)) {
-             struct address_space *mapping = page_mapping(page);
-             struct address_space *mapping2;
+     struct address_space *mapping = page_mapping(page);
+     struct address_space *mapping2;
 
-             if (!mapping)
-                     return 1;
+     if (!mapping)
+             return 1;
 
-             write_lock_irq(&mapping->tree_lock);
-             mapping2 = page_mapping(page);
-             if (mapping2) { /* Race with truncate? */
-                     BUG_ON(mapping2 != mapping);
-                     WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
-                     if (mapping_cap_account_dirty(mapping)) {
-                             __inc_zone_page_state(page, NR_FILE_DIRTY);
-                             __inc_bdi_stat(mapping->backing_dev_info,
-                                             BDI_RECLAIMABLE);
-                             task_io_account_write(PAGE_CACHE_SIZE);

mm/page-writeback.c int __set_page_dirty_no_writeback(struct page *page) · line 1007

Quoted 10 lines

mm/page-writeback.c +2 -8

-                     }
-                     radix_tree_tag_set(&mapping->page_tree,
-                             page_index(page), PAGECACHE_TAG_DIRTY);
-             }
-             write_unlock_irq(&mapping->tree_lock);
-             if (mapping->host) {
-                     /* !PageAnon && !swapper_space */
-                     __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
+     mapping->mtime = CURRENT_TIME;
+     set_bit(AS_MCTIME, &mapping->flags);

This seems vulnerable to the race we have against truncate, handled by the mapping2 magic below. Do we care?

Quoted 22 lines

mm/page-writeback.c +18 -2

+
+     if (TestSetPageDirty(page))
+             return 0;
+
+     write_lock_irq(&mapping->tree_lock);
+     mapping2 = page_mapping(page);
+     if (mapping2) {
+             /* Race with truncate? */
+             BUG_ON(mapping2 != mapping);
+             WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
+             if (mapping_cap_account_dirty(mapping)) {
+                     __inc_zone_page_state(page, NR_FILE_DIRTY);
+                     __inc_bdi_stat(mapping->backing_dev_info,
+                                     BDI_RECLAIMABLE);
+                     task_io_account_write(PAGE_CACHE_SIZE);
              }
-             return 1;
+             radix_tree_tag_set(&mapping->page_tree,
+                             page_index(page), PAGECACHE_TAG_DIRTY);
      }
-     return 0;
+     write_unlock_irq(&mapping->tree_lock);

Quoted 3 lines

mm/page-writeback.c int __set_page_dirty_no_writeback(struct page *page) · line 1030

Quoted 3 lines

mm/page-writeback.c +3 -0

+
+     if (mapping->host)
+             __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);

The inode gets marked dirty using the same "mapping" variable as my code does. So, AFAIU, my change does not introduce any new vulnerabilities. I would nevertherless be grateful to you for a scenario where the race would be triggered.

fs/inode.c void file_update_time(struct file *file) · line 1290

Quoted 10 lines

fs/inode.c +10 -0

+/*
+ * Update the ctime and mtime stamps for memory-mapped block device files.
+ */
+static void bd_inode_update_time(struct inode *inode, struct timespec *ts)
+{
+     struct block_device *bdev = inode->i_bdev;
+     struct list_head *p;
+
+     if (bdev == NULL)
+             return;

inode->i_bdev is never NULL for inodes currently beeing written to.

fs/inode.c void file_update_time(struct file *file) · line 1294

Quoted 4 lines

fs/inode.c +4 -0

+
+     mutex_lock(&bdev->bd_mutex);
+     list_for_each(p, &bdev->bd_inodes) {
+             inode = list_entry(p, struct inode, i_devices);

this should use list_for_each_entry.

1. Introduction

This is the fourth version of my solution for the bug #2645:

http://bugzilla.kernel.org/show_bug.cgi?id=2645

Changes since the previous version:

1) the case of retouching an already-dirty page pointed out by Miklos Szeredi has been addressed;

Quoted 10 lines

1. Introduction

This is the fourth version of my solution for the bug #2645:

http://bugzilla.kernel.org/show_bug.cgi?id=2645

Changes since the previous version:

1) the case of retouching an already-dirty page pointed out by Miklos Szeredi has been addressed;

I'm a bit sceptical, as we've also pointed out, that this is not possible without messing with the page tables.

Did you try my test program on the patched kernel?

I've refreshed the patch, where we left this issue last time. It should basically have equivalent functionality to your patch, and is a lot simpler. There might be performance issues with it, but it's a good starting point.

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

mm/memory.c +2 -0

@@ -1680,6 +1680,8 @@ 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

Quoted 16 lines

Quoted 10 lines

1. Introduction

This is the fourth version of my solution for the bug #2645:

http://bugzilla.kernel.org/show_bug.cgi?id=2645

Changes since the previous version:

1) the case of retouching an already-dirty page pointed out by Miklos Szeredi has been addressed;

I'm a bit sceptical, as we've also pointed out, that this is not possible without messing with the page tables.

Did you try my test program on the patched kernel?

Quoted 4 lines

I've refreshed the patch, where we left this issue last time. It should basically have equivalent functionality to your patch, and is a lot simpler. There might be performance issues with it, but it's a good starting point.

It has the same problem as Anton's in that it won't get triggered again for an already dirty mapped page.

Quoted 8 lines

1. Introduction

This is the fourth version of my solution for the bug #2645:

http://bugzilla.kernel.org/show_bug.cgi?id=2645

Changes since the previous version:

Quoted 2 lines

1) the case of retouching an already-dirty page pointed out by Miklos Szeredi has been addressed;

I'm a bit sceptical, as we've also pointed out, that this is not possible without messing with the page tables.

Did you try my test program on the patched kernel?

I've refreshed the patch, where we left this issue last time. It should basically have equivalent functionality to your patch, and is a lot simpler. There might be performance issues with it, but it's a good starting point.

Miklos
----

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

mm/memory.c 2008-01-15 21:16:14.000000000 +0100 renamed from mm/memory.c 2008-01-09 21:16:30.000000000 +0100 +2 -0

@@ -1680,6 +1680,8 @@ 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