Re: [PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]
2008/1/15, Miklos Szeredi <[EMAIL PROTECTED]>:
> > 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.0 +0100
> +++ linux/mm/memory.c 2008-01-15 21:16:14.0 +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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]
> 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 from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]
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.c2008-01-09 21:16:30.0 +0100
> +++ linux/mm/memory.c 2008-01-15 21:16:14.0 +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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]
> 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.0 +0100
+++ linux/mm/memory.c 2008-01-15 21:16:14.0 +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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]
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, >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, >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
[PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]
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
Re: [PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]
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.0 +0100
+++ linux/mm/memory.c 2008-01-15 21:16:14.0 +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 from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]
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.c2008-01-09 21:16:30.0 +0100
+++ linux/mm/memory.c 2008-01-15 21:16:14.0 +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 from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]
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 from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] Updating ctime and mtime for memory-mapped files [try #4]
2008/1/15, Miklos Szeredi [EMAIL PROTECTED]:
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.0 +0100
+++ linux/mm/memory.c 2008-01-15 21:16:14.0 +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 from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
