[PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]

[PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]

[Anton Salikhmetov]

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;

2) the file metadata are updated using the page modification time
  instead of the time of syncing data;

3) a few small corrections according to the latest feedback.

Brief explanation of these changes as well as some design considerations
are given below.

2. The case of retouching an already-dirtied page

Miklos Szeredi gave the following feedback on the previous version:

> 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.

This version adds handling of the case of multiple msync() calls. Before
going on with the explanaion, I'll quote a remark by Peter Zijlstra:

> 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.

Indeed, the following logic of the __set_pages_dirty_nobuffers() function:

if (!TestSetPageDirty(page)) {
       mapping = page_mapping(page);

       if (!mapping)
               return 1;

       /* critical section */

       if (mapping->host) {
               __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
               set_bit(AS_MCTIME, &mapping->flags);
       }
       return 1;
}
return 0;

made it difficult to account for the case of the already-dirty page
retouch after the call to msync(MS_ASYNC).

In this version of my solution, I redesigned the logic of the same
function as follows:

mapping = page_mapping(page);

if (!mapping)
       return 1;

set_bit(AS_MCTIME, &mapping->flags);

if (TestSetPageDirty(page))
       return 0;

/* critical section */

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

return 1;

This allows us to set the AS_MCTIME bit independently of whether the page
had already been dirtied or not. Besides, such change makes the logic of
the topmost "if" in this function straight thus improving readability.
Finally, we already have the __set_page_dirty() routine with almost
identical functionality. My redesign of __set_pages_dirty_nobuffers()
is based on how the __set_page_dirty() routine is implemented.

Miklos gave an example of a scenario, where the previous version of
my solution would fail:

http://lkml.org/lkml/2008/1/14/100

Here is how it looks in the version I am sending now:

 1 page is dirtied through mapping
       => the AS_MCTIME bit turned on
 2 app calls msync(MS_ASYNC)
       => inode's times updated, the AS_MCTIME bit turned off
 3 page is written again through mapping
       => the AS_MCTIME bit turned on again
 4 app calls msync(MS_ASYNC)
       => inode's times updated, the AS_MCTIME bit turned off
 5 ...
 6 page is written back
       => ... by this moment, the either the msync(MS_ASYNC) has
          taken care of updating the file times, or the AS_MCTIME
          bit is on.

I think that the feedback about writes after the first msync(MS_ASYNC)
has thereby been addressed.

3. Updating the time stamps of the block device special files

As for the block device case, let's start from the following assumption:

if the block device data changes, we should do our best to tell the world
that this has happened.

This is how I approach this requirement:

1) if the block device is active, this is done at next *sync() through
  calling the bd_inode_update_time() helper function.

2) if the block device is not active, this is done during the block
  device file deactivation in the unlink_file_vma() routine.

Removing either of these actions would leave a possibility of losing
information about the block device data update. That is why I am keeping
both.

4. Recording the time was the file data changed

Finally, I noticed yet another issue with the previous version of my patch.
Specifically, the time stamps were set to the current time of the moment
when syncing but not the write reference was being done. This led to the
following adverse effect on my development system:

1) a text file A was updated by process B;
2) process B exits without calling any of the *sync() functions;
3) vi editor opens the file A;
4) file data synced, file times updated;
5) vi is confused by "thinking" that the file was changed after 3).

This version overcomes this problem by introducing another field into the
address_space structure. This field is used to "remember" the time of
dirtying, and then this time value is propagated to the file metadata.

This approach is based upon the following suggestion given by Peter
Staubach during one of our previous discussions:

http://lkml.org/lkml/2008/1/9/267

> 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.

The solution I propose now proves the viability of the latter
approach.

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

[PATCH 1/2] Massive code cleanup of sys_msync()

[Anton Salikhmetov]

Substantial code cleanup of the sys_msync() function:

1) using the PAGE_ALIGN() macro instead of "manual" alignment;
2) improved readability of the loop traversing the process memory regions.

Signed-off-by: Anton Salikhmetov <salikhmetov@gmail.com>
---
 mm/msync.c |   79 ++++++++++++++++++++++++++++--------------------------------
 1 files changed, 37 insertions(+), 42 deletions(-)

diff --git a/mm/msync.c b/mm/msync.c
index 144a757..3270caa 100644
--- a/mm/msync.c
+++ b/mm/msync.c
@@ -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 <salikhmetov@gmail.com>
  */
 
-/*
- * The msync() system call.
- */
+#include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/mm.h>
 #include <linux/mman.h>
-#include <linux/file.h>
-#include <linux/syscalls.h>
 #include <linux/sched.h>
+#include <linux/syscalls.h>
 
 /*
  * 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 marks the relevant pages dirty (it used to up to 2.6.17).
+ * 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.
  *
  * The application may now run fsync() to
@@ -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;
 
 	if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
-		goto out;
+		return -EINVAL;
 	if (start & ~PAGE_MASK)
-		goto out;
+		return -EINVAL;
 	if ((flags & MS_ASYNC) && (flags & MS_SYNC))
-		goto out;
-	error = -ENOMEM;
-	len = (len + ~PAGE_MASK) & PAGE_MASK;
+		return -EINVAL;
+
+	len = PAGE_ALIGN(len);
 	end = start + len;
 	if (end < start)
-		goto out;
-	error = 0;
+		return -ENOMEM;
 	if (end == start)
-		goto out;
+		return 0;
+
 	/*
 	 * If the interval [start,end) covers some unmapped address ranges,
 	 * just ignore them, but return -ENOMEM at the end.
 	 */
 	down_read(&mm->mmap_sem);
 	vma = find_vma(mm, start);
-	for (;;) {
+	do {
 		struct file *file;
 
-		/* Still start < end. */
-		error = -ENOMEM;
-		if (!vma)
-			goto out_unlock;
-		/* Here start < vma->vm_end. */
+		if (!vma) {
+			error = -ENOMEM;
+			break;
+		}
 		if (start < vma->vm_start) {
 			start = vma->vm_start;
-			if (start >= end)
-				goto out_unlock;
+			if (start >= end) {
+				error = -ENOMEM;
+				break;
+			}
 			unmapped_error = -ENOMEM;
 		}
-		/* Here vma->vm_start <= start < vma->vm_end. */
-		if ((flags & MS_INVALIDATE) &&
-				(vma->vm_flags & VM_LOCKED)) {
+		if ((flags & MS_INVALIDATE) && (vma->vm_flags & VM_LOCKED)) {
 			error = -EBUSY;
-			goto out_unlock;
+			break;
 		}
-		file = vma->vm_file;
 		start = vma->vm_end;
-		if ((flags & MS_SYNC) && file &&
-				(vma->vm_flags & VM_SHARED)) {
+
+		file = vma->vm_file;
+		if ((flags & MS_SYNC) && file && (vma->vm_flags & VM_SHARED)) {
 			get_file(file);
 			up_read(&mm->mmap_sem);
 			error = do_fsync(file, 0);
 			fput(file);
-			if (error || start >= end)
-				goto out;
+			if (error)
+				return error;
 			down_read(&mm->mmap_sem);
 			vma = find_vma(mm, start);
-		} else {
-			if (start >= end) {
-				error = 0;
-				goto out_unlock;
-			}
-			vma = vma->vm_next;
+			continue;
 		}
-	}
-out_unlock:
+
+		vma = vma->vm_next;
+	} while (start < end);
 	up_read(&mm->mmap_sem);
-out:
+
 	return error ? : unmapped_error;
 }
-- 
1.4.4.4

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

Re: [PATCH 1/2] Massive code cleanup of sys_msync()

[Christoph Hellwig]

On Tue, Jan 15, 2008 at 07:02:44PM +0300, Anton Salikhmetov wrote:
> +++ b/mm/msync.c
> @@ -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 <salikhmetov@gmail.com>

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 <salikhmetov@gmail.com>
 */

> @@ -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;
>  
>  	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.

> +		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.

> +			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.

>  	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;

?

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

Re: [PATCH 1/2] Massive code cleanup of sys_msync()

[Anton Salikhmetov]

2008/1/15, Christoph Hellwig <hch@infradead.org>:
> On Tue, Jan 15, 2008 at 07:02:44PM +0300, Anton Salikhmetov wrote:
> > +++ b/mm/msync.c
> > @@ -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 <salikhmetov@gmail.com>
>
> 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 <salikhmetov@gmail.com>
>  */

Thanks!

I'll take into account your recommendation.

>
> > @@ -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;
> >
> >       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.

>
> > +             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.

>
> > +                     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.

>
> >       return error ? : unmapped_error;
>
> Care to get rid of this odd GNU extension while you're at it and use
> the proper

I do also think that this GNU extension is not readable,
so I'll take your recommentation into account.

>
>         return error ? error : unmapped_error;
>
> ?
>
>

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

Re: [PATCH 1/2] Massive code cleanup of sys_msync()

[Randy Dunlap]

On Tue, 15 Jan 2008 22:02:54 +0300 Anton Salikhmetov wrote:

> 2008/1/15, Christoph Hellwig <hch@infradead.org>:
> > On Tue, Jan 15, 2008 at 07:02:44PM +0300, Anton Salikhmetov wrote:

> > > @@ -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;
> > >
> > >       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.

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.


> The second part of requirement does not hold true for the sys_msync() routine.
> 
> >
> > > +             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.
> 
> >
> > > +                     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.


---
~Randy

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

Re: [PATCH 1/2] Massive code cleanup of sys_msync()

[Anton Salikhmetov]

2008/1/15, Randy Dunlap <randy.dunlap@oracle.com>:
> On Tue, 15 Jan 2008 22:02:54 +0300 Anton Salikhmetov wrote:
>
> > 2008/1/15, Christoph Hellwig <hch@infradead.org>:
> > > On Tue, Jan 15, 2008 at 07:02:44PM +0300, Anton Salikhmetov wrote:
>
> > > > @@ -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;
> > > >
> > > >       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.
>
> 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;
}

>
>
> > The second part of requirement does not hold true for the sys_msync() routine.
> >
> > >
> > > > +             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.
> >
> > >
> > > > +                     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.
>
>
> ---
> ~Randy
>

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

Re: [PATCH 1/2] Massive code cleanup of sys_msync()

[Peter Zijlstra]

On Tue, 2008-01-15 at 22:26 +0300, Anton Salikhmetov wrote:
> 2008/1/15, Randy Dunlap <randy.dunlap@oracle.com>:
> > On Tue, 15 Jan 2008 22:02:54 +0300 Anton Salikhmetov wrote:
> >
> > > 2008/1/15, Christoph Hellwig <hch@infradead.org>:
> > > > On Tue, Jan 15, 2008 at 07:02:44PM +0300, Anton Salikhmetov wrote:
> >
> > > > > @@ -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;
> > > > >
> > > > >       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.
> >
> > 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.

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

Re: [PATCH 1/2] Massive code cleanup of sys_msync()

[Christoph Hellwig]

On Tue, Jan 15, 2008 at 08:28:48PM +0100, Peter Zijlstra wrote:
> 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.

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

Re: [PATCH 1/2] Massive code cleanup of sys_msync()

[Anton Salikhmetov]

2008/1/15, Christoph Hellwig <hch@infradead.org>:
> On Tue, Jan 15, 2008 at 08:28:48PM +0100, Peter Zijlstra wrote:
> > 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.
>

OK, I will do as you suggest. Thank you for your answers.

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

Re: [PATCH 1/2] Massive code cleanup of sys_msync()

[Matt Mackall]

On Tue, 2008-01-15 at 11:10 -0800, Randy Dunlap wrote:
> On Tue, 15 Jan 2008 22:02:54 +0300 Anton Salikhmetov wrote:
> 
> > 2008/1/15, Christoph Hellwig <hch@infradead.org>:
> > > On Tue, Jan 15, 2008 at 07:02:44PM +0300, Anton Salikhmetov wrote:
> 
> > > > @@ -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;
> > > >
> > > >       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.
> 
> 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.

-- 
Mathematics is the supreme nostalgia of our time.

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

Re: [PATCH 1/2] Massive code cleanup of sys_msync()

[Randy Dunlap]

On Tue, 15 Jan 2008 14:46:57 -0600 Matt Mackall wrote:

> 
> On Tue, 2008-01-15 at 11:10 -0800, Randy Dunlap wrote:
> > On Tue, 15 Jan 2008 22:02:54 +0300 Anton Salikhmetov wrote:
> > 
> > > 2008/1/15, Christoph Hellwig <hch@infradead.org>:
> > > > On Tue, Jan 15, 2008 at 07:02:44PM +0300, Anton Salikhmetov wrote:
> > 
> > > > > @@ -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;
> > > > >
> > > > >       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.
> > 
> > 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.

OTOH, I think that those are fine reasons for it.

---
~Randy

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

[PATCH 2/2] Updating ctime and mtime at syncing

[Anton Salikhmetov]

Changes for updating the ctime and mtime fields for memory-mapped files:

1) new flag triggering update of the inode data;
2) new field in the address_space structure for saving modification time;
3) new function to update ctime and mtime for block device files;
4) new helper function to update ctime and mtime when needed;
5) updating time stamps for mapped files in sys_msync() and do_fsync();
6) implementing the feature of auto-updating ctime and mtime;
7) account for the case of retouching an already-dirtied page.

Signed-off-by: Anton Salikhmetov <salikhmetov@gmail.com>
---
 fs/buffer.c             |    3 ++
 fs/fs-writeback.c       |    2 +
 fs/inode.c              |   53 ++++++++++++++++++++++++++++++++++-----------
 fs/sync.c               |    2 +
 include/linux/fs.h      |   13 ++++++++++-
 include/linux/pagemap.h |    3 +-
 mm/mmap.c               |    3 ++
 mm/msync.c              |   29 ++++++++++++++++--------
 mm/page-writeback.c     |   54 +++++++++++++++++++++++++---------------------
 9 files changed, 112 insertions(+), 50 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 7249e01..3967aa7 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -701,6 +701,9 @@ static int __set_page_dirty(struct page *page,
 	if (unlikely(!mapping))
 		return !TestSetPageDirty(page);
 
+	mapping->mtime = CURRENT_TIME;
+	set_bit(AS_MCTIME, &mapping->flags);
+
 	if (TestSetPageDirty(page))
 		return 0;
 
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 300324b..affd291 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -243,6 +243,8 @@ __sync_single_inode(struct inode *inode, struct writeback_control *wbc)
 
 	spin_unlock(&inode_lock);
 
+	mapping_update_time(mapping);
+
 	ret = do_writepages(mapping, wbc);
 
 	/* Don't write the inode if only I_DIRTY_PAGES was set */
diff --git a/fs/inode.c b/fs/inode.c
index ed35383..5997046 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1243,8 +1243,9 @@ void touch_atime(struct vfsmount *mnt, struct dentry *dentry)
 EXPORT_SYMBOL(touch_atime);
 
 /**
- *	file_update_time	-	update mtime and ctime time
- *	@file: file accessed
+ *	inode_update_time	-	update mtime and ctime time
+ *	@inode: inode accessed
+ *	@ts: time when inode was accessed
  *
  *	Update the mtime and ctime members of an inode and mark the inode
  *	for writeback.  Note that this function is meant exclusively for
@@ -1253,11 +1254,8 @@ EXPORT_SYMBOL(touch_atime);
  *	S_NOCTIME inode flag, e.g. for network filesystem where these
  *	timestamps are handled by the server.
  */
-
-void file_update_time(struct file *file)
+void inode_update_time(struct inode *inode, struct timespec *ts)
 {
-	struct inode *inode = file->f_path.dentry->d_inode;
-	struct timespec now;
 	int sync_it = 0;
 
 	if (IS_NOCMTIME(inode))
@@ -1265,22 +1263,52 @@ void file_update_time(struct file *file)
 	if (IS_RDONLY(inode))
 		return;
 
-	now = current_fs_time(inode->i_sb);
-	if (!timespec_equal(&inode->i_mtime, &now)) {
-		inode->i_mtime = now;
+	if (timespec_compare(&inode->i_mtime, ts) < 0) {
+		inode->i_mtime = *ts;
 		sync_it = 1;
 	}
 
-	if (!timespec_equal(&inode->i_ctime, &now)) {
-		inode->i_ctime = now;
+	if (timespec_compare(&inode->i_ctime, ts) < 0) {
+		inode->i_ctime = *ts;
 		sync_it = 1;
 	}
 
 	if (sync_it)
 		mark_inode_dirty_sync(inode);
 }
+EXPORT_SYMBOL(inode_update_time);
+
+/*
+ * 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;
+
+	mutex_lock(&bdev->bd_mutex);
+	list_for_each(p, &bdev->bd_inodes) {
+		inode = list_entry(p, struct inode, i_devices);
+		inode_update_time(inode, ts);
+	}
+	mutex_unlock(&bdev->bd_mutex);
+}
 
-EXPORT_SYMBOL(file_update_time);
+/*
+ * Update the ctime and mtime stamps after checking if they are to be updated.
+ */
+void mapping_update_time(struct address_space *mapping)
+{
+	if (test_and_clear_bit(AS_MCTIME, &mapping->flags)) {
+		if (S_ISBLK(mapping->host->i_mode))
+			bd_inode_update_time(mapping->host, &mapping->mtime);
+		else
+			inode_update_time(mapping->host, &mapping->mtime);
+	}
+}
 
 int inode_needs_sync(struct inode *inode)
 {
@@ -1290,7 +1318,6 @@ int inode_needs_sync(struct inode *inode)
 		return 1;
 	return 0;
 }
-
 EXPORT_SYMBOL(inode_needs_sync);
 
 int inode_wait(void *word)
diff --git a/fs/sync.c b/fs/sync.c
index 7cd005e..5561464 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -87,6 +87,8 @@ long do_fsync(struct file *file, int datasync)
 		goto out;
 	}
 
+	mapping_update_time(mapping);
+
 	ret = filemap_fdatawrite(mapping);
 
 	/*
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b3ec4a4..f0d3ced 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -511,6 +511,7 @@ struct address_space {
 	spinlock_t		private_lock;	/* for use by the address_space */
 	struct list_head	private_list;	/* ditto */
 	struct address_space	*assoc_mapping;	/* ditto */
+	struct timespec		mtime;		/* modification time */
 } __attribute__((aligned(sizeof(long))));
 	/*
 	 * On most architectures that alignment is already the case; but
@@ -1977,7 +1978,17 @@ extern int buffer_migrate_page(struct address_space *,
 extern int inode_change_ok(struct inode *, struct iattr *);
 extern int __must_check inode_setattr(struct inode *, struct iattr *);
 
-extern void file_update_time(struct file *file);
+extern void inode_update_time(struct inode *, struct timespec *);
+
+static inline void file_update_time(struct file *file)
+{
+	struct inode *inode = file->f_dentry->d_inode;
+	struct timespec ts = current_fs_time(inode->i_sb);
+
+	inode_update_time(inode, &ts);
+}
+
+extern void mapping_update_time(struct address_space *);
 
 static inline ino_t parent_ino(struct dentry *dentry)
 {
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index db8a410..bf0f9e7 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -17,8 +17,9 @@
  * Bits in mapping->flags.  The lower __GFP_BITS_SHIFT bits are the page
  * allocation mode flags.
  */
-#define	AS_EIO		(__GFP_BITS_SHIFT + 0)	/* IO error on async write */
+#define AS_EIO		(__GFP_BITS_SHIFT + 0)	/* IO error on async write */
 #define AS_ENOSPC	(__GFP_BITS_SHIFT + 1)	/* ENOSPC on async write */
+#define AS_MCTIME	(__GFP_BITS_SHIFT + 2)	/* mtime and ctime to update */
 
 static inline void mapping_set_error(struct address_space *mapping, int error)
 {
diff --git a/mm/mmap.c b/mm/mmap.c
index 15678aa..f659733 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -210,9 +210,12 @@ void unlink_file_vma(struct vm_area_struct *vma)
 
 	if (file) {
 		struct address_space *mapping = file->f_mapping;
+
 		spin_lock(&mapping->i_mmap_lock);
 		__remove_shared_vm_struct(vma, file, mapping);
 		spin_unlock(&mapping->i_mmap_lock);
+
+		mapping_update_time(mapping);
 	}
 }
 
diff --git a/mm/msync.c b/mm/msync.c
index 3270caa..80ca1cc 100644
--- a/mm/msync.c
+++ b/mm/msync.c
@@ -5,6 +5,7 @@
  * Copyright (C) 1994-1999  Linus Torvalds
  *
  * Massive code cleanup.
+ * Updating the ctime and mtime stamps for memory-mapped files.
  * Copyright (C) 2008 Anton Salikhmetov <salikhmetov@gmail.com>
  */
 
@@ -22,6 +23,10 @@
  * 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.
  *
+ * The msync() system call updates the ctime and mtime fields for
+ * the mapped file when called with the MS_SYNC or MS_ASYNC flags
+ * according to the POSIX standard.
+ *
  * 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
@@ -78,16 +83,20 @@ asmlinkage long sys_msync(unsigned long start, size_t len, int flags)
 		start = vma->vm_end;
 
 		file = vma->vm_file;
-		if ((flags & MS_SYNC) && file && (vma->vm_flags & VM_SHARED)) {
-			get_file(file);
-			up_read(&mm->mmap_sem);
-			error = do_fsync(file, 0);
-			fput(file);
-			if (error)
-				return error;
-			down_read(&mm->mmap_sem);
-			vma = find_vma(mm, start);
-			continue;
+		if (file && (vma->vm_flags & VM_SHARED)) {
+			if (flags & MS_ASYNC)
+				mapping_update_time(file->f_mapping);
+			if (flags & MS_SYNC) {
+				get_file(file);
+				up_read(&mm->mmap_sem);
+				error = do_fsync(file, 0);
+				fput(file);
+				if (error)
+					return error;
+				down_read(&mm->mmap_sem);
+				vma = find_vma(mm, start);
+				continue;
+			}
 		}
 
 		vma = vma->vm_next;
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 3d3848f..53d0e34 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -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);
-			}
-			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);
+
+	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);
 
-- 
1.4.4.4

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

Re: [PATCH 2/2] Updating ctime and mtime at syncing

[Anton Salikhmetov]

2008/1/15, Peter Zijlstra <a.p.zijlstra@chello.nl>:
>
> On Tue, 2008-01-15 at 19:02 +0300, Anton Salikhmetov wrote:
>
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index 3d3848f..53d0e34 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -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);
> > -                     }
> > -                     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?
>
> > +
> > +     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);

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.

> > +
> > +     return 1;
> >  }
> >  EXPORT_SYMBOL(__set_page_dirty_nobuffers);
> >
>
>

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

Re: [PATCH 2/2] Updating ctime and mtime at syncing

[Peter Zijlstra]

On Tue, 2008-01-15 at 20:18 +0300, Anton Salikhmetov wrote:
> 2008/1/15, Peter Zijlstra <a.p.zijlstra@chello.nl>:
> >
> > On Tue, 2008-01-15 at 19:02 +0300, Anton Salikhmetov wrote:
> >
> > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > > index 3d3848f..53d0e34 100644
> > > --- a/mm/page-writeback.c
> > > +++ b/mm/page-writeback.c
> > > @@ -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);
> > > -                     }
> > > -                     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?
> >
> > > +
> > > +     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);
> 
> 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.

Ah, right, so that would be a resounding no to my previous question :-)

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

Re: [PATCH 2/2] Updating ctime and mtime at syncing

[Christoph Hellwig]

On Tue, Jan 15, 2008 at 07:02:45PM +0300, Anton Salikhmetov wrote:
> +/*
> + * 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.

> +
> +	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.

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

Re: [PATCH 2/2] Updating ctime and mtime at syncing

[Anton Salikhmetov]

2008/1/15, Christoph Hellwig <hch@infradead.org>:
> On Tue, Jan 15, 2008 at 07:02:45PM +0300, Anton Salikhmetov wrote:
> > +/*
> > + * 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.
>
> > +
> > +     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.
>
>

Thank you very much for your recommenations. I'll take them into account.

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

Re: [PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]

[Miklos Szeredi]

> 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.

Miklos
----

Index: linux/mm/memory.c
===================================================================
--- linux.orig/mm/memory.c	2008-01-09 21:16:30.000000000 +0100
+++ linux/mm/memory.c	2008-01-15 21:16:14.000000000 +0100
@@ -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
@@ -2313,6 +2315,8 @@ 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);
 	}

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

Re: [PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]

[Peter Zijlstra]

On Tue, 2008-01-15 at 21:27 +0100, Miklos Szeredi wrote:
> > 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.

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

But yeah, its simpler than fudging set_page_dirty().

> Index: linux/mm/memory.c
> ===================================================================
> --- linux.orig/mm/memory.c	2008-01-09 21:16:30.000000000 +0100
> +++ linux/mm/memory.c	2008-01-15 21:16:14.000000000 +0100
> @@ -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
> @@ -2313,6 +2315,8 @@ 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);
>  	}
> 

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

Re: [PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]

[Miklos Szeredi]

> On Tue, 2008-01-15 at 21:27 +0100, Miklos Szeredi wrote:
> > > 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.
> 
> It has the same problem as Anton's in that it won't get triggered again
> for an already dirty mapped page.

Yes, it's not better in this respect, than Anton's patch.  And it
might be worse performance-wise, since file_update_time() is sure to
be slower, than set_bit().  According to Andrew, this may not actually
matter in practice, but that would have to be benchmarked, I guess.

Miklos

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

Re: [PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]

[Anton Salikhmetov]

2008/1/15, Miklos Szeredi <miklos@szeredi.hu>:
> > 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 just tried your test program. Alas, my assumption appears to be wrong.

Thank you for your comment!

Now I start thinking that it is better not to care about
the MS_ASYNC case whatsoever.

>
> 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
> ===================================================================
> --- linux.orig/mm/memory.c      2008-01-09 21:16:30.000000000 +0100
> +++ linux/mm/memory.c   2008-01-15 21:16:14.000000000 +0100
> @@ -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
> @@ -2313,6 +2315,8 @@ 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);
>         }
>
>

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

[PATCH 0/2] yet another attempt to fix the ctime and mtime issue

[Anton Salikhmetov]

The POSIX standard requires that the ctime and mtime fields
for memory-mapped files should be updated after a write
reference to the memory region where the file data is mapped.
At least FreeBSD 6.2 and HP-UX 11i implement this properly.
Linux does not, which leads to data loss problems in database
backup applications.

Kernel Bug Tracker contains more information about the problem:

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

There have been several attempts in the past to address this
issue. Following are a few links to LKML discussions related
to this bug:

http://lkml.org/lkml/2006/5/17/138
http://lkml.org/lkml/2007/2/21/242
http://lkml.org/lkml/2008/1/7/234

All earlier solutions were criticized. Some solutions did not
handle memory-mapped block devices properly. Some led to forcing
applications to explicitly call msync() to update file metadata.
Some contained errors in using kernel synchronization primitives.

In the two patches that follow, I would like to propose a new
solution.

This is the third version of my changes. This version takes
into account all feedback I received for the two previous versions.
The overall design remains basically the same as the one that
was acked by Rick van Riel:

http://lkml.org/lkml/2008/1/11/208

To the best of my knowledge, these patches are free of all the
drawbacks found during previous attempts by Peter Staubach,
Miklos Szeredi and myself.

New since the previous version:

1) no need to explicitly call msync() to update file times;
2) changing block device data is visible to all device files
   associated with the block device;
3) in the cleanup part, the error checks are separated out as
   suggested by Rik van Riel;
4) some small refinements accodring to the LKML comments.

This is how I tested the patches.

1. To test the features mentioned above, I wrote a unit test
   available from

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

   I verified that the unit test passed successfully for both
   regular files and block device files. For the unit test I
   used the following architectures: 32-bit x86, x86_64 and
   MIPS32 (cross-compiled from x86_64).

2. I did build tests with allmodconfig and allyesconfig on x86_64.

3. I ran the following test cases from the LTP test suite:

   msync01
   msync02
   msync03
   msync04
   msync05
   mmapstress01
   mmapstress09
   mmapstress10

   No regressions were found by these test cases.

I think that the bug #2645 is resolved by these patches.

Please apply.

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

[PATCH 1/2] massive code cleanup of sys_msync()

[Anton Salikhmetov]

Substantial code cleanup of the sys_msync() function:

1) using the PAGE_ALIGN() macro instead of "manual" alignment;
2) improved readability of the loop traversing the process memory regions.

Signed-off-by: Anton Salikhmetov <salikhmetov@gmail.com>
---
 mm/msync.c |   78 +++++++++++++++++++++++++++---------------------------------
 1 files changed, 35 insertions(+), 43 deletions(-)

diff --git a/mm/msync.c b/mm/msync.c
index 144a757..ff654c9 100644
--- a/mm/msync.c
+++ b/mm/msync.c
@@ -1,24 +1,25 @@
 /*
  *	linux/mm/msync.c
  *
+ * The msync() system call.
  * Copyright (C) 1994-1999  Linus Torvalds
+ *
+ * Substantial code cleanup.
+ * Copyright (C) 2008 Anton Salikhmetov <salikhmetov@gmail.com>
  */
 
-/*
- * The msync() system call.
- */
+#include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/mm.h>
 #include <linux/mman.h>
-#include <linux/file.h>
-#include <linux/syscalls.h>
 #include <linux/sched.h>
+#include <linux/syscalls.h>
 
 /*
  * 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 marks the relevant pages dirty (it used to up to 2.6.17).
+ * 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.
  *
  * The application may now run fsync() to
@@ -33,71 +34,62 @@ 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;
 
 	if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
-		goto out;
+		return -EINVAL;
 	if (start & ~PAGE_MASK)
-		goto out;
+		return -EINVAL;
 	if ((flags & MS_ASYNC) && (flags & MS_SYNC))
-		goto out;
-	error = -ENOMEM;
-	len = (len + ~PAGE_MASK) & PAGE_MASK;
+		return -EINVAL;
+
+	len = PAGE_ALIGN(len);
 	end = start + len;
 	if (end < start)
-		goto out;
-	error = 0;
+		return -ENOMEM;
 	if (end == start)
-		goto out;
+		return 0;
+
 	/*
 	 * If the interval [start,end) covers some unmapped address ranges,
 	 * just ignore them, but return -ENOMEM at the end.
 	 */
 	down_read(&mm->mmap_sem);
 	vma = find_vma(mm, start);
-	for (;;) {
+	do {
 		struct file *file;
 
-		/* Still start < end. */
-		error = -ENOMEM;
-		if (!vma)
-			goto out_unlock;
-		/* Here start < vma->vm_end. */
+		if (!vma) {
+			error = -ENOMEM;
+			break;
+		}
 		if (start < vma->vm_start) {
 			start = vma->vm_start;
-			if (start >= end)
-				goto out_unlock;
+			if (start >= end) {
+				error = -ENOMEM;
+				break;
+			}
 			unmapped_error = -ENOMEM;
 		}
-		/* Here vma->vm_start <= start < vma->vm_end. */
-		if ((flags & MS_INVALIDATE) &&
-				(vma->vm_flags & VM_LOCKED)) {
+		if ((flags & MS_INVALIDATE) && (vma->vm_flags & VM_LOCKED)) {
 			error = -EBUSY;
-			goto out_unlock;
+			break;
 		}
 		file = vma->vm_file;
-		start = vma->vm_end;
-		if ((flags & MS_SYNC) && file &&
-				(vma->vm_flags & VM_SHARED)) {
+		if ((flags & MS_SYNC) && file && (vma->vm_flags & VM_SHARED)) {
 			get_file(file);
 			up_read(&mm->mmap_sem);
 			error = do_fsync(file, 0);
 			fput(file);
-			if (error || start >= end)
-				goto out;
+			if (error)
+				return error;
 			down_read(&mm->mmap_sem);
-			vma = find_vma(mm, start);
-		} else {
-			if (start >= end) {
-				error = 0;
-				goto out_unlock;
-			}
-			vma = vma->vm_next;
 		}
-	}
-out_unlock:
+
+		start = vma->vm_end;
+		vma = vma->vm_next;
+	} while (start < end);
 	up_read(&mm->mmap_sem);
-out:
+
 	return error ? : unmapped_error;
 }
-- 
1.4.4.4

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

Re: [PATCH 1/2] massive code cleanup of sys_msync()

[Rik van Riel]

On Sun, 13 Jan 2008 07:39:58 +0300
Anton Salikhmetov <salikhmetov@gmail.com> wrote:

> Substantial code cleanup of the sys_msync() function:
> 
> 1) using the PAGE_ALIGN() macro instead of "manual" alignment;
> 2) improved readability of the loop traversing the process memory regions.
> 
> Signed-off-by: Anton Salikhmetov <salikhmetov@gmail.com>

Acked-by: Rik van Riel <riel@redhat.com>


-- 
All rights reversed.

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

Re: [PATCH 1/2] massive code cleanup of sys_msync()

[Miklos Szeredi]

> Substantial code cleanup of the sys_msync() function:
> 
> 1) using the PAGE_ALIGN() macro instead of "manual" alignment;
> 2) improved readability of the loop traversing the process memory regions.

Thanks for doing this.  See comments below.

> Signed-off-by: Anton Salikhmetov <salikhmetov@gmail.com>
> ---
>  mm/msync.c |   78 +++++++++++++++++++++++++++---------------------------------
>  1 files changed, 35 insertions(+), 43 deletions(-)
> 
> diff --git a/mm/msync.c b/mm/msync.c
> index 144a757..ff654c9 100644
> --- a/mm/msync.c
> +++ b/mm/msync.c
> @@ -1,24 +1,25 @@
>  /*
>   *	linux/mm/msync.c
>   *
> + * The msync() system call.
>   * Copyright (C) 1994-1999  Linus Torvalds
> + *
> + * Substantial code cleanup.
> + * Copyright (C) 2008 Anton Salikhmetov <salikhmetov@gmail.com>
>   */
>  
> -/*
> - * The msync() system call.
> - */
> +#include <linux/file.h>
>  #include <linux/fs.h>
>  #include <linux/mm.h>
>  #include <linux/mman.h>
> -#include <linux/file.h>
> -#include <linux/syscalls.h>
>  #include <linux/sched.h>
> +#include <linux/syscalls.h>
>  
>  /*
>   * 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 marks the relevant pages dirty (it used to up to 2.6.17).
> + * 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.
>   *
>   * The application may now run fsync() to
> @@ -33,71 +34,62 @@ 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;
>  
>  	if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
> -		goto out;
> +		return -EINVAL;
>  	if (start & ~PAGE_MASK)
> -		goto out;
> +		return -EINVAL;
>  	if ((flags & MS_ASYNC) && (flags & MS_SYNC))
> -		goto out;
> -	error = -ENOMEM;
> -	len = (len + ~PAGE_MASK) & PAGE_MASK;
> +		return -EINVAL;
> +
> +	len = PAGE_ALIGN(len);
>  	end = start + len;
>  	if (end < start)
> -		goto out;
> -	error = 0;
> +		return -ENOMEM;
>  	if (end == start)
> -		goto out;
> +		return 0;
> +
>  	/*
>  	 * If the interval [start,end) covers some unmapped address ranges,
>  	 * just ignore them, but return -ENOMEM at the end.
>  	 */
>  	down_read(&mm->mmap_sem);
>  	vma = find_vma(mm, start);
> -	for (;;) {
> +	do {
>  		struct file *file;
>  
> -		/* Still start < end. */
> -		error = -ENOMEM;
> -		if (!vma)
> -			goto out_unlock;
> -		/* Here start < vma->vm_end. */
> +		if (!vma) {
> +			error = -ENOMEM;
> +			break;
> +		}
>  		if (start < vma->vm_start) {
>  			start = vma->vm_start;
> -			if (start >= end)
> -				goto out_unlock;
> +			if (start >= end) {
> +				error = -ENOMEM;
> +				break;
> +			}
>  			unmapped_error = -ENOMEM;
>  		}
> -		/* Here vma->vm_start <= start < vma->vm_end. */
> -		if ((flags & MS_INVALIDATE) &&
> -				(vma->vm_flags & VM_LOCKED)) {
> +		if ((flags & MS_INVALIDATE) && (vma->vm_flags & VM_LOCKED)) {
>  			error = -EBUSY;
> -			goto out_unlock;
> +			break;
>  		}
>  		file = vma->vm_file;
> -		start = vma->vm_end;
> -		if ((flags & MS_SYNC) && file &&
> -				(vma->vm_flags & VM_SHARED)) {
> +		if ((flags & MS_SYNC) && file && (vma->vm_flags & VM_SHARED)) {
>  			get_file(file);
>  			up_read(&mm->mmap_sem);
>  			error = do_fsync(file, 0);
>  			fput(file);
> -			if (error || start >= end)
> -				goto out;
> +			if (error)
> +				return error;
>  			down_read(&mm->mmap_sem);
> -			vma = find_vma(mm, start);

Where did this line go?  It's needed because after releasing and
reacquiring the mmap sem, the old vma may have gone away.

I suggest, that when doing such a massive cleanup, that you split it
up even further into easily understandable pieces, so such bugs cannot
creep in.

> -		} else {
> -			if (start >= end) {
> -				error = 0;
> -				goto out_unlock;
> -			}
> -			vma = vma->vm_next;
>  		}
> -	}
> -out_unlock:
> +
> +		start = vma->vm_end;
> +		vma = vma->vm_next;
> +	} while (start < end);
>  	up_read(&mm->mmap_sem);
> -out:
> +
>  	return error ? : unmapped_error;
>  }
> -- 
> 1.4.4.4

Miklos

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

Re: [PATCH 1/2] massive code cleanup of sys_msync()

[Anton Salikhmetov]

2008/1/14, Miklos Szeredi <miklos@szeredi.hu>:
> > Substantial code cleanup of the sys_msync() function:
> >
> > 1) using the PAGE_ALIGN() macro instead of "manual" alignment;
> > 2) improved readability of the loop traversing the process memory regions.
>
> Thanks for doing this.  See comments below.
>
> > Signed-off-by: Anton Salikhmetov <salikhmetov@gmail.com>
> > ---
> >  mm/msync.c |   78 +++++++++++++++++++++++++++---------------------------------
> >  1 files changed, 35 insertions(+), 43 deletions(-)
> >
> > diff --git a/mm/msync.c b/mm/msync.c
> > index 144a757..ff654c9 100644
> > --- a/mm/msync.c
> > +++ b/mm/msync.c
> > @@ -1,24 +1,25 @@
> >  /*
> >   *   linux/mm/msync.c
> >   *
> > + * The msync() system call.
> >   * Copyright (C) 1994-1999  Linus Torvalds
> > + *
> > + * Substantial code cleanup.
> > + * Copyright (C) 2008 Anton Salikhmetov <salikhmetov@gmail.com>
> >   */
> >
> > -/*
> > - * The msync() system call.
> > - */
> > +#include <linux/file.h>
> >  #include <linux/fs.h>
> >  #include <linux/mm.h>
> >  #include <linux/mman.h>
> > -#include <linux/file.h>
> > -#include <linux/syscalls.h>
> >  #include <linux/sched.h>
> > +#include <linux/syscalls.h>
> >
> >  /*
> >   * 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 marks the relevant pages dirty (it used to up to 2.6.17).
> > + * 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.
> >   *
> >   * The application may now run fsync() to
> > @@ -33,71 +34,62 @@ 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;
> >
> >       if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
> > -             goto out;
> > +             return -EINVAL;
> >       if (start & ~PAGE_MASK)
> > -             goto out;
> > +             return -EINVAL;
> >       if ((flags & MS_ASYNC) && (flags & MS_SYNC))
> > -             goto out;
> > -     error = -ENOMEM;
> > -     len = (len + ~PAGE_MASK) & PAGE_MASK;
> > +             return -EINVAL;
> > +
> > +     len = PAGE_ALIGN(len);
> >       end = start + len;
> >       if (end < start)
> > -             goto out;
> > -     error = 0;
> > +             return -ENOMEM;
> >       if (end == start)
> > -             goto out;
> > +             return 0;
> > +
> >       /*
> >        * If the interval [start,end) covers some unmapped address ranges,
> >        * just ignore them, but return -ENOMEM at the end.
> >        */
> >       down_read(&mm->mmap_sem);
> >       vma = find_vma(mm, start);
> > -     for (;;) {
> > +     do {
> >               struct file *file;
> >
> > -             /* Still start < end. */
> > -             error = -ENOMEM;
> > -             if (!vma)
> > -                     goto out_unlock;
> > -             /* Here start < vma->vm_end. */
> > +             if (!vma) {
> > +                     error = -ENOMEM;
> > +                     break;
> > +             }
> >               if (start < vma->vm_start) {
> >                       start = vma->vm_start;
> > -                     if (start >= end)
> > -                             goto out_unlock;
> > +                     if (start >= end) {
> > +                             error = -ENOMEM;
> > +                             break;
> > +                     }
> >                       unmapped_error = -ENOMEM;
> >               }
> > -             /* Here vma->vm_start <= start < vma->vm_end. */
> > -             if ((flags & MS_INVALIDATE) &&
> > -                             (vma->vm_flags & VM_LOCKED)) {
> > +             if ((flags & MS_INVALIDATE) && (vma->vm_flags & VM_LOCKED)) {
> >                       error = -EBUSY;
> > -                     goto out_unlock;
> > +                     break;
> >               }
> >               file = vma->vm_file;
> > -             start = vma->vm_end;
> > -             if ((flags & MS_SYNC) && file &&
> > -                             (vma->vm_flags & VM_SHARED)) {
> > +             if ((flags & MS_SYNC) && file && (vma->vm_flags & VM_SHARED)) {
> >                       get_file(file);
> >                       up_read(&mm->mmap_sem);
> >                       error = do_fsync(file, 0);
> >                       fput(file);
> > -                     if (error || start >= end)
> > -                             goto out;
> > +                     if (error)
> > +                             return error;
> >                       down_read(&mm->mmap_sem);
> > -                     vma = find_vma(mm, start);
>
> Where did this line go?  It's needed because after releasing and
> reacquiring the mmap sem, the old vma may have gone away.
>
> I suggest, that when doing such a massive cleanup, that you split it
> up even further into easily understandable pieces, so such bugs cannot
> creep in.

Thanks for your review. I overlooked this problem. I'll redo my cleanup soon.

>
> > -             } else {
> > -                     if (start >= end) {
> > -                             error = 0;
> > -                             goto out_unlock;
> > -                     }
> > -                     vma = vma->vm_next;
> >               }
> > -     }
> > -out_unlock:
> > +
> > +             start = vma->vm_end;
> > +             vma = vma->vm_next;
> > +     } while (start < end);
> >       up_read(&mm->mmap_sem);
> > -out:
> > +
> >       return error ? : unmapped_error;
> >  }
> > --
> > 1.4.4.4
>
> Miklos
>

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