Re: [PATCH -v5 2/2] Updating ctime and mtime at syncing
2008/1/17, Miklos Szeredi <[EMAIL PROTECTED]>: > > The do_wp_page() function is called in mm/memory.c after locking PTE. > > And the file_update_time() routine calls the filesystem operation that can > > sleep. It's not accepted, I guess. > > do_wp_page() is called with the pte lock but drops it, so that's fine. OK, I agree. I'll take into account your suggestion to move updating time stamps from the __set_page_dirty() and __set_page_dirty_nobuffers() routines to do_wp_page(). Thank you! > > 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 -v5 2/2] Updating ctime and mtime at syncing
> The do_wp_page() function is called in mm/memory.c after locking PTE. > And the file_update_time() routine calls the filesystem operation that can > sleep. It's not accepted, I guess. do_wp_page() is called with the pte lock but drops it, so that's fine. 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 -v5 2/2] Updating ctime and mtime at syncing
2008/1/17, Miklos Szeredi <[EMAIL PROTECTED]>:
> > > I'm not sure this auto-updating is really needed (POSIX doesn't
> > > mandate it).
> >
> > Peter Shtaubach, author of the first solution for this bug,
> > and Jacob Ostergaard, the reporter of this bug, insist the "auto-update"
> > feature to be implemented.
>
> Can they state their reasons for the insistence?
>
> > 1) a base patch: update time just from fsync() and remove_vma()
> > 2) update time on sync(2) as well
> > 3) update time on MS_ASYNC as well
>
> Oh, and the four-liner I posted the other day will give you 1) + 2) +
> even more at a small fraction of the complexity. And tacking on the
> reprotect code will solve the MS_ASYNC issue just the same.
>
> I agree, that having the timestamp updated on sync() is nice, and that
> trivial patch will give you that, and will also update the timestamp
> at least each 30 seconds if the file is being constantly modified,
> even if no explicit syncing is done.
>
> So maybe it's worth a little effort benchmarking how much that patch
> affects the cost of writing to a page.
>
> You could write a little test program like this (if somebody hasn't
> yet done so):
>
> - do some preparation:
>
>echo 80 > dirty_ratio
>echo 80 > dirty_background_ratio
>echo 3 > dirty_expire_centisecs
>sync
>
> - map a large file, one that fits comfortably into free memory
> - bring the whole file in, by reading a byte from each page
> - start the timer
> - write a byte to each page
> - stop the timer
>
> It would be most interesting to try this on a filesystem supporting
> nanosecond timestamps. Anyone know which these are?
The do_wp_page() function is called in mm/memory.c after locking PTE.
And the file_update_time() routine calls the filesystem operation that can
sleep. It's not accepted, I guess.
>
> 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 -v5 2/2] Updating ctime and mtime at syncing
> > I'm not sure this auto-updating is really needed (POSIX doesn't
> > mandate it).
>
> Peter Shtaubach, author of the first solution for this bug,
> and Jacob Ostergaard, the reporter of this bug, insist the "auto-update"
> feature to be implemented.
Can they state their reasons for the insistence?
> 1) a base patch: update time just from fsync() and remove_vma()
> 2) update time on sync(2) as well
> 3) update time on MS_ASYNC as well
Oh, and the four-liner I posted the other day will give you 1) + 2) +
even more at a small fraction of the complexity. And tacking on the
reprotect code will solve the MS_ASYNC issue just the same.
I agree, that having the timestamp updated on sync() is nice, and that
trivial patch will give you that, and will also update the timestamp
at least each 30 seconds if the file is being constantly modified,
even if no explicit syncing is done.
So maybe it's worth a little effort benchmarking how much that patch
affects the cost of writing to a page.
You could write a little test program like this (if somebody hasn't
yet done so):
- do some preparation:
echo 80 > dirty_ratio
echo 80 > dirty_background_ratio
echo 3 > dirty_expire_centisecs
sync
- map a large file, one that fits comfortably into free memory
- bring the whole file in, by reading a byte from each page
- start the timer
- write a byte to each page
- stop the timer
It would be most interesting to try this on a filesystem supporting
nanosecond timestamps. Anyone know which these are?
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 -v5 2/2] Updating ctime and mtime at syncing
2008/1/17, Miklos Szeredi <[EMAIL PROTECTED]>: > > 2008/1/17, Miklos Szeredi <[EMAIL PROTECTED]>: > > > > > 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). > > > > > > Updating the time in remove_vma() would fix this, no? > > > > We need to save modification time. Otherwise, updating time stamps > > will be confusing the vi editor. > > remove_vma() will be called when process B exits, so if the times are > updated there, and the flag is cleared, the times won't be updated > later. > > > > > > > > > All these changes to inode.c are unnecessary, I think. > > > > > > > > The first part is necessary to account for "remembering" the > > > > modification time. > > > > > > > > The second part is for handling block device files. I cannot see any > > > > other > > > > sane way to update file times for them. > > > > > > Use file_update_time(), which will do the right thing. It will in > > > fact do the same thing as write(2) on the device, which is really what > > > we want. > > > > > > Block devices being mapped for write through different device > > > nodes..., well, I don't think we really need to handle such weird > > > corner cases 100% acurately. > > > > The file_update_time() cannot be used for implementing > > the "auto-update" feature, because the sync() system call > > doesn't "know" about the file which was memory-mapped. > > I'm not sure this auto-updating is really needed (POSIX doesn't > mandate it). Peter Shtaubach, author of the first solution for this bug, and Jacob Ostergaard, the reporter of this bug, insist the "auto-update" feature to be implemented. > > At least split it out into a separate patch, so it can be considered > separately on it's own merit. > > I think doing the same with the page-table reprotecting in MS_ASYNC is > also a good idea. That will leave us with > > 1) a base patch: update time just from fsync() and remove_vma() > 2) update time on sync(2) as well > 3) update time on MS_ASYNC as well > > I'd happily ack the first one, which would solve the most serious > issues, but have some reservations about the other two. > > 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 -v5 2/2] Updating ctime and mtime at syncing
2008/1/17, Rogier Wolff <[EMAIL PROTECTED]>: > On Thu, Jan 17, 2008 at 04:16:47PM +0300, Anton Salikhmetov wrote: > > 2008/1/17, Miklos Szeredi <[EMAIL PROTECTED]>: > > > > > 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). > > > > > > Updating the time in remove_vma() would fix this, no? > > > > We need to save modification time. Otherwise, updating time stamps > > will be confusing the vi editor. > > If process B exits before vi opens the file, the timestamp should at > the latest be the time that process B exits. There is no excuse for > setting the timestamp later than the time that B exits. > > If process B no longer modifies the file, but still keeps it mapped > until after vi starts, then the system can't help the > situation. Wether or not B acesses those pages is unknown to the > system. So you get what you deserve. The exit() system call closes the file memory-mapped file. Therefore, it's better to catch the close() system call. However, the close() system call does not trigger unmapping the file. The mapped area for the file may be used after closing the file. So, we should catch only the unmap() call, not close() or exit(). > > Roger. > > -- > ** [EMAIL PROTECTED] ** http://www.BitWizard.nl/ ** +31-15-2600998 ** > **Delftechpark 26 2628 XH Delft, The Netherlands. KVK: 27239233** > *-- BitWizard writes Linux device drivers for any device you may have! --* > Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. > Does it sit on the couch all day? Is it unemployed? Please be specific! > Define 'it' and what it isn't doing. - Adapted from lxrbot FAQ > -- 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 -v5 2/2] Updating ctime and mtime at syncing
> 2008/1/17, Miklos Szeredi <[EMAIL PROTECTED]>: > > > > 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). > > > > Updating the time in remove_vma() would fix this, no? > > We need to save modification time. Otherwise, updating time stamps > will be confusing the vi editor. remove_vma() will be called when process B exits, so if the times are updated there, and the flag is cleared, the times won't be updated later. > > > > > > All these changes to inode.c are unnecessary, I think. > > > > > > The first part is necessary to account for "remembering" the modification > > > time. > > > > > > The second part is for handling block device files. I cannot see any other > > > sane way to update file times for them. > > > > Use file_update_time(), which will do the right thing. It will in > > fact do the same thing as write(2) on the device, which is really what > > we want. > > > > Block devices being mapped for write through different device > > nodes..., well, I don't think we really need to handle such weird > > corner cases 100% acurately. > > The file_update_time() cannot be used for implementing > the "auto-update" feature, because the sync() system call > doesn't "know" about the file which was memory-mapped. I'm not sure this auto-updating is really needed (POSIX doesn't mandate it). At least split it out into a separate patch, so it can be considered separately on it's own merit. I think doing the same with the page-table reprotecting in MS_ASYNC is also a good idea. That will leave us with 1) a base patch: update time just from fsync() and remove_vma() 2) update time on sync(2) as well 3) update time on MS_ASYNC as well I'd happily ack the first one, which would solve the most serious issues, but have some reservations about the other two. 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 -v5 2/2] Updating ctime and mtime at syncing
On Thu, Jan 17, 2008 at 04:16:47PM +0300, Anton Salikhmetov wrote: > 2008/1/17, Miklos Szeredi <[EMAIL PROTECTED]>: > > > > 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). > > > > Updating the time in remove_vma() would fix this, no? > > We need to save modification time. Otherwise, updating time stamps > will be confusing the vi editor. If process B exits before vi opens the file, the timestamp should at the latest be the time that process B exits. There is no excuse for setting the timestamp later than the time that B exits. If process B no longer modifies the file, but still keeps it mapped until after vi starts, then the system can't help the situation. Wether or not B acesses those pages is unknown to the system. So you get what you deserve. Roger. -- ** [EMAIL PROTECTED] ** http://www.BitWizard.nl/ ** +31-15-2600998 ** **Delftechpark 26 2628 XH Delft, The Netherlands. KVK: 27239233** *-- BitWizard writes Linux device drivers for any device you may have! --* Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. Does it sit on the couch all day? Is it unemployed? Please be specific! Define 'it' and what it isn't doing. - Adapted from lxrbot FAQ -- 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 -v5 2/2] Updating ctime and mtime at syncing
2008/1/17, Miklos Szeredi <[EMAIL PROTECTED]>: > > > 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). > > Updating the time in remove_vma() would fix this, no? We need to save modification time. Otherwise, updating time stamps will be confusing the vi editor. > > > > All these changes to inode.c are unnecessary, I think. > > > > The first part is necessary to account for "remembering" the modification > > time. > > > > The second part is for handling block device files. I cannot see any other > > sane way to update file times for them. > > Use file_update_time(), which will do the right thing. It will in > fact do the same thing as write(2) on the device, which is really what > we want. > > Block devices being mapped for write through different device > nodes..., well, I don't think we really need to handle such weird > corner cases 100% acurately. The file_update_time() cannot be used for implementing the "auto-update" feature, because the sync() system call doesn't "know" about the file which was memory-mapped. > > 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 -v5 2/2] Updating ctime and mtime at syncing
On Thu, Jan 17, 2008 at 01:45:43PM +0100, Miklos Szeredi wrote: > > > 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). > > Updating the time in remove_vma() would fix this, no? That sounds to me as the right thing to do. Although not explcitly mentioned in the standard, it is the logical (latest allowable) timestamp to put on the modifications by process B. Roger. -- ** [EMAIL PROTECTED] ** http://www.BitWizard.nl/ ** +31-15-2600998 ** **Delftechpark 26 2628 XH Delft, The Netherlands. KVK: 27239233** *-- BitWizard writes Linux device drivers for any device you may have! --* Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. Does it sit on the couch all day? Is it unemployed? Please be specific! Define 'it' and what it isn't doing. - Adapted from lxrbot FAQ -- 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 -v5 2/2] Updating ctime and mtime at syncing
> > 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). Updating the time in remove_vma() would fix this, no? > > All these changes to inode.c are unnecessary, I think. > > The first part is necessary to account for "remembering" the modification > time. > > The second part is for handling block device files. I cannot see any other > sane way to update file times for them. Use file_update_time(), which will do the right thing. It will in fact do the same thing as write(2) on the device, which is really what we want. Block devices being mapped for write through different device nodes..., well, I don't think we really need to handle such weird corner cases 100% acurately. 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 -v5 2/2] Updating ctime and mtime at syncing
2008/1/17, Miklos Szeredi <[EMAIL PROTECTED]>: > > http://bugzilla.kernel.org/show_bug.cgi?id=2645 > > > > Changes for updating the ctime and mtime fields for memory-mapped files: > > > > 1) a new flag triggering update of the inode data; > > 2) a new field in the address_space structure for saving modification time; > > 3) a new helper function to update ctime and mtime when needed; > > 4) updating time stamps for mapped files in sys_msync() and do_fsync(); > > 5) implementing lazy ctime and mtime update. > > OK, the functionality seems to be there now. As a next step, I think > you should try to simplify the patch, removing everything, that is not > strictly necessary. > > > > > Signed-off-by: Anton Salikhmetov <[EMAIL PROTECTED]> > > --- > > fs/buffer.c |3 ++ > > fs/fs-writeback.c |2 + > > fs/inode.c | 43 +++-- > > fs/sync.c |2 + > > include/linux/fs.h | 13 +- > > include/linux/pagemap.h |3 +- > > mm/msync.c | 61 > > +- > > mm/page-writeback.c | 54 ++--- > > 8 files changed, 124 insertions(+), 57 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; > > Why is this needed? POSIX explicitly states, that the modification > time can be set to anywhere between the first write and the msync. I've already described this change in one of my previous emails: > 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. See: http://lkml.org/lkml/2008/1/15/202 > > > + set_bit(AS_MCTIME, >flags); > > A bigger problem is that doing this in __set_page_dirty() and friends > will mean, that the flag will be set for non-mapped writes as well, > which we definitely don't want. > > A better place to put it is do_wp_page and __do_fault, where > set_page_dirty_balance() is called. Thanks for your suggestion, I'll consider how I can apply it. > > > + > > 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(_lock); > > > > + mapping_update_time(mapping); > > + > > I think this is unnecessary. Rather put the update into remove_vma(). Thanks for your suggestion, I'll consider how I can apply it. However, I suspect that this is a bit problematic. Let me check it out. > > > 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..edd5bf4 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -1243,8 +1243,10 @@ 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 > > + * @sync: whether to do synchronous update > > * > > * Update the mtime and ctime
Re: [PATCH -v5 2/2] Updating ctime and mtime at syncing
> http://bugzilla.kernel.org/show_bug.cgi?id=2645
>
> Changes for updating the ctime and mtime fields for memory-mapped files:
>
> 1) a new flag triggering update of the inode data;
> 2) a new field in the address_space structure for saving modification time;
> 3) a new helper function to update ctime and mtime when needed;
> 4) updating time stamps for mapped files in sys_msync() and do_fsync();
> 5) implementing lazy ctime and mtime update.
OK, the functionality seems to be there now. As a next step, I think
you should try to simplify the patch, removing everything, that is not
strictly necessary.
>
> Signed-off-by: Anton Salikhmetov <[EMAIL PROTECTED]>
> ---
> fs/buffer.c |3 ++
> fs/fs-writeback.c |2 +
> fs/inode.c | 43 +++--
> fs/sync.c |2 +
> include/linux/fs.h | 13 +-
> include/linux/pagemap.h |3 +-
> mm/msync.c | 61 +-
> mm/page-writeback.c | 54 ++---
> 8 files changed, 124 insertions(+), 57 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;
Why is this needed? POSIX explicitly states, that the modification
time can be set to anywhere between the first write and the msync.
> + set_bit(AS_MCTIME, >flags);
A bigger problem is that doing this in __set_page_dirty() and friends
will mean, that the flag will be set for non-mapped writes as well,
which we definitely don't want.
A better place to put it is do_wp_page and __do_fault, where
set_page_dirty_balance() is called.
> +
> 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(_lock);
>
> + mapping_update_time(mapping);
> +
I think this is unnecessary. Rather put the update into remove_vma().
> 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..edd5bf4 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1243,8 +1243,10 @@ 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
> + * @sync: whether to do synchronous update
> *
> * 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 +1255,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 +1264,41 @@ void file_update_time(struct file *file)
> if (IS_RDONLY(inode))
> return;
>
> - now = current_fs_time(inode->i_sb);
> - if (!timespec_equal(>i_mtime, )) {
> - inode->i_mtime = now;
> + if (timespec_compare(>i_mtime, ts) < 0) {
> + inode->i_mtime = *ts;
> sync_it = 1;
> }
>
> - if (!timespec_equal(>i_ctime, )) {
> - inode->i_ctime = now;
> + if (timespec_compare(>i_ctime, ts) < 0) {
> + inode->i_ctime = *ts;
> sync_it = 1;
> }
>
> if (sync_it)
> mark_inode_dirty_sync(inode);
> }
> +EXPORT_SYMBOL(inode_update_time);
>
> -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, >flags)) {
> + struct inode *inode = mapping->host;
> + struct timespec *ts = >mtime;
> +
> + if (S_ISBLK(inode->i_mode)) {
> + struct block_device *bdev = inode->i_bdev;
> +
> + mutex_lock(>bd_mutex);
> + list_for_each_entry(inode, >bd_inodes, i_devices)
> + inode_update_time(inode, ts);
> +
Re: [PATCH -v5 2/2] Updating ctime and mtime at syncing
http://bugzilla.kernel.org/show_bug.cgi?id=2645
Changes for updating the ctime and mtime fields for memory-mapped files:
1) a new flag triggering update of the inode data;
2) a new field in the address_space structure for saving modification time;
3) a new helper function to update ctime and mtime when needed;
4) updating time stamps for mapped files in sys_msync() and do_fsync();
5) implementing lazy ctime and mtime update.
OK, the functionality seems to be there now. As a next step, I think
you should try to simplify the patch, removing everything, that is not
strictly necessary.
Signed-off-by: Anton Salikhmetov [EMAIL PROTECTED]
---
fs/buffer.c |3 ++
fs/fs-writeback.c |2 +
fs/inode.c | 43 +++--
fs/sync.c |2 +
include/linux/fs.h | 13 +-
include/linux/pagemap.h |3 +-
mm/msync.c | 61 +-
mm/page-writeback.c | 54 ++---
8 files changed, 124 insertions(+), 57 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;
Why is this needed? POSIX explicitly states, that the modification
time can be set to anywhere between the first write and the msync.
+ set_bit(AS_MCTIME, mapping-flags);
A bigger problem is that doing this in __set_page_dirty() and friends
will mean, that the flag will be set for non-mapped writes as well,
which we definitely don't want.
A better place to put it is do_wp_page and __do_fault, where
set_page_dirty_balance() is called.
+
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);
+
I think this is unnecessary. Rather put the update into remove_vma().
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..edd5bf4 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1243,8 +1243,10 @@ 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
+ * @sync: whether to do synchronous update
*
* 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 +1255,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 +1264,41 @@ 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);
-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)) {
+ struct inode *inode = mapping-host;
+ struct timespec *ts = mapping-mtime;
+
+ if (S_ISBLK(inode-i_mode)) {
+ struct block_device *bdev = inode-i_bdev;
+
+ mutex_lock(bdev-bd_mutex);
+ list_for_each_entry(inode, bdev-bd_inodes, i_devices)
+ inode_update_time(inode, ts);
+ mutex_unlock(bdev-bd_mutex);
+ } else
+ inode_update_time(inode,
Re: [PATCH -v5 2/2] Updating ctime and mtime at syncing
2008/1/17, Miklos Szeredi [EMAIL PROTECTED]: http://bugzilla.kernel.org/show_bug.cgi?id=2645 Changes for updating the ctime and mtime fields for memory-mapped files: 1) a new flag triggering update of the inode data; 2) a new field in the address_space structure for saving modification time; 3) a new helper function to update ctime and mtime when needed; 4) updating time stamps for mapped files in sys_msync() and do_fsync(); 5) implementing lazy ctime and mtime update. OK, the functionality seems to be there now. As a next step, I think you should try to simplify the patch, removing everything, that is not strictly necessary. Signed-off-by: Anton Salikhmetov [EMAIL PROTECTED] --- fs/buffer.c |3 ++ fs/fs-writeback.c |2 + fs/inode.c | 43 +++-- fs/sync.c |2 + include/linux/fs.h | 13 +- include/linux/pagemap.h |3 +- mm/msync.c | 61 +- mm/page-writeback.c | 54 ++--- 8 files changed, 124 insertions(+), 57 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; Why is this needed? POSIX explicitly states, that the modification time can be set to anywhere between the first write and the msync. I've already described this change in one of my previous emails: 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. See: http://lkml.org/lkml/2008/1/15/202 + set_bit(AS_MCTIME, mapping-flags); A bigger problem is that doing this in __set_page_dirty() and friends will mean, that the flag will be set for non-mapped writes as well, which we definitely don't want. A better place to put it is do_wp_page and __do_fault, where set_page_dirty_balance() is called. Thanks for your suggestion, I'll consider how I can apply it. + 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); + I think this is unnecessary. Rather put the update into remove_vma(). Thanks for your suggestion, I'll consider how I can apply it. However, I suspect that this is a bit problematic. Let me check it out. 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..edd5bf4 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1243,8 +1243,10 @@ 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 + * @sync: whether to do synchronous update * * 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 +1255,8 @@ EXPORT_SYMBOL(touch_atime); * S_NOCTIME inode
Re: [PATCH -v5 2/2] Updating ctime and mtime at syncing
On Thu, Jan 17, 2008 at 01:45:43PM +0100, Miklos Szeredi wrote: 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). Updating the time in remove_vma() would fix this, no? That sounds to me as the right thing to do. Although not explcitly mentioned in the standard, it is the logical (latest allowable) timestamp to put on the modifications by process B. Roger. -- ** [EMAIL PROTECTED] ** http://www.BitWizard.nl/ ** +31-15-2600998 ** **Delftechpark 26 2628 XH Delft, The Netherlands. KVK: 27239233** *-- BitWizard writes Linux device drivers for any device you may have! --* Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. Does it sit on the couch all day? Is it unemployed? Please be specific! Define 'it' and what it isn't doing. - Adapted from lxrbot FAQ -- 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 -v5 2/2] Updating ctime and mtime at syncing
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). Updating the time in remove_vma() would fix this, no? All these changes to inode.c are unnecessary, I think. The first part is necessary to account for remembering the modification time. The second part is for handling block device files. I cannot see any other sane way to update file times for them. Use file_update_time(), which will do the right thing. It will in fact do the same thing as write(2) on the device, which is really what we want. Block devices being mapped for write through different device nodes..., well, I don't think we really need to handle such weird corner cases 100% acurately. 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 -v5 2/2] Updating ctime and mtime at syncing
On Thu, Jan 17, 2008 at 04:16:47PM +0300, Anton Salikhmetov wrote: 2008/1/17, Miklos Szeredi [EMAIL PROTECTED]: 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). Updating the time in remove_vma() would fix this, no? We need to save modification time. Otherwise, updating time stamps will be confusing the vi editor. If process B exits before vi opens the file, the timestamp should at the latest be the time that process B exits. There is no excuse for setting the timestamp later than the time that B exits. If process B no longer modifies the file, but still keeps it mapped until after vi starts, then the system can't help the situation. Wether or not B acesses those pages is unknown to the system. So you get what you deserve. Roger. -- ** [EMAIL PROTECTED] ** http://www.BitWizard.nl/ ** +31-15-2600998 ** **Delftechpark 26 2628 XH Delft, The Netherlands. KVK: 27239233** *-- BitWizard writes Linux device drivers for any device you may have! --* Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. Does it sit on the couch all day? Is it unemployed? Please be specific! Define 'it' and what it isn't doing. - Adapted from lxrbot FAQ -- 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 -v5 2/2] Updating ctime and mtime at syncing
2008/1/17, Miklos Szeredi [EMAIL PROTECTED]: 2008/1/17, Miklos Szeredi [EMAIL PROTECTED]: 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). Updating the time in remove_vma() would fix this, no? We need to save modification time. Otherwise, updating time stamps will be confusing the vi editor. remove_vma() will be called when process B exits, so if the times are updated there, and the flag is cleared, the times won't be updated later. All these changes to inode.c are unnecessary, I think. The first part is necessary to account for remembering the modification time. The second part is for handling block device files. I cannot see any other sane way to update file times for them. Use file_update_time(), which will do the right thing. It will in fact do the same thing as write(2) on the device, which is really what we want. Block devices being mapped for write through different device nodes..., well, I don't think we really need to handle such weird corner cases 100% acurately. The file_update_time() cannot be used for implementing the auto-update feature, because the sync() system call doesn't know about the file which was memory-mapped. I'm not sure this auto-updating is really needed (POSIX doesn't mandate it). Peter Shtaubach, author of the first solution for this bug, and Jacob Ostergaard, the reporter of this bug, insist the auto-update feature to be implemented. At least split it out into a separate patch, so it can be considered separately on it's own merit. I think doing the same with the page-table reprotecting in MS_ASYNC is also a good idea. That will leave us with 1) a base patch: update time just from fsync() and remove_vma() 2) update time on sync(2) as well 3) update time on MS_ASYNC as well I'd happily ack the first one, which would solve the most serious issues, but have some reservations about the other two. 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 -v5 2/2] Updating ctime and mtime at syncing
2008/1/17, Miklos Szeredi [EMAIL PROTECTED]: 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). Updating the time in remove_vma() would fix this, no? We need to save modification time. Otherwise, updating time stamps will be confusing the vi editor. All these changes to inode.c are unnecessary, I think. The first part is necessary to account for remembering the modification time. The second part is for handling block device files. I cannot see any other sane way to update file times for them. Use file_update_time(), which will do the right thing. It will in fact do the same thing as write(2) on the device, which is really what we want. Block devices being mapped for write through different device nodes..., well, I don't think we really need to handle such weird corner cases 100% acurately. The file_update_time() cannot be used for implementing the auto-update feature, because the sync() system call doesn't know about the file which was memory-mapped. 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 -v5 2/2] Updating ctime and mtime at syncing
2008/1/17, Miklos Szeredi [EMAIL PROTECTED]:
I'm not sure this auto-updating is really needed (POSIX doesn't
mandate it).
Peter Shtaubach, author of the first solution for this bug,
and Jacob Ostergaard, the reporter of this bug, insist the auto-update
feature to be implemented.
Can they state their reasons for the insistence?
1) a base patch: update time just from fsync() and remove_vma()
2) update time on sync(2) as well
3) update time on MS_ASYNC as well
Oh, and the four-liner I posted the other day will give you 1) + 2) +
even more at a small fraction of the complexity. And tacking on the
reprotect code will solve the MS_ASYNC issue just the same.
I agree, that having the timestamp updated on sync() is nice, and that
trivial patch will give you that, and will also update the timestamp
at least each 30 seconds if the file is being constantly modified,
even if no explicit syncing is done.
So maybe it's worth a little effort benchmarking how much that patch
affects the cost of writing to a page.
You could write a little test program like this (if somebody hasn't
yet done so):
- do some preparation:
echo 80 dirty_ratio
echo 80 dirty_background_ratio
echo 3 dirty_expire_centisecs
sync
- map a large file, one that fits comfortably into free memory
- bring the whole file in, by reading a byte from each page
- start the timer
- write a byte to each page
- stop the timer
It would be most interesting to try this on a filesystem supporting
nanosecond timestamps. Anyone know which these are?
The do_wp_page() function is called in mm/memory.c after locking PTE.
And the file_update_time() routine calls the filesystem operation that can
sleep. It's not accepted, I guess.
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 -v5 2/2] Updating ctime and mtime at syncing
2008/1/17, Miklos Szeredi [EMAIL PROTECTED]: 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). Updating the time in remove_vma() would fix this, no? We need to save modification time. Otherwise, updating time stamps will be confusing the vi editor. remove_vma() will be called when process B exits, so if the times are updated there, and the flag is cleared, the times won't be updated later. All these changes to inode.c are unnecessary, I think. The first part is necessary to account for remembering the modification time. The second part is for handling block device files. I cannot see any other sane way to update file times for them. Use file_update_time(), which will do the right thing. It will in fact do the same thing as write(2) on the device, which is really what we want. Block devices being mapped for write through different device nodes..., well, I don't think we really need to handle such weird corner cases 100% acurately. The file_update_time() cannot be used for implementing the auto-update feature, because the sync() system call doesn't know about the file which was memory-mapped. I'm not sure this auto-updating is really needed (POSIX doesn't mandate it). At least split it out into a separate patch, so it can be considered separately on it's own merit. I think doing the same with the page-table reprotecting in MS_ASYNC is also a good idea. That will leave us with 1) a base patch: update time just from fsync() and remove_vma() 2) update time on sync(2) as well 3) update time on MS_ASYNC as well I'd happily ack the first one, which would solve the most serious issues, but have some reservations about the other two. 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 -v5 2/2] Updating ctime and mtime at syncing
The do_wp_page() function is called in mm/memory.c after locking PTE. And the file_update_time() routine calls the filesystem operation that can sleep. It's not accepted, I guess. do_wp_page() is called with the pte lock but drops it, so that's fine. 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 -v5 2/2] Updating ctime and mtime at syncing
I'm not sure this auto-updating is really needed (POSIX doesn't
mandate it).
Peter Shtaubach, author of the first solution for this bug,
and Jacob Ostergaard, the reporter of this bug, insist the auto-update
feature to be implemented.
Can they state their reasons for the insistence?
1) a base patch: update time just from fsync() and remove_vma()
2) update time on sync(2) as well
3) update time on MS_ASYNC as well
Oh, and the four-liner I posted the other day will give you 1) + 2) +
even more at a small fraction of the complexity. And tacking on the
reprotect code will solve the MS_ASYNC issue just the same.
I agree, that having the timestamp updated on sync() is nice, and that
trivial patch will give you that, and will also update the timestamp
at least each 30 seconds if the file is being constantly modified,
even if no explicit syncing is done.
So maybe it's worth a little effort benchmarking how much that patch
affects the cost of writing to a page.
You could write a little test program like this (if somebody hasn't
yet done so):
- do some preparation:
echo 80 dirty_ratio
echo 80 dirty_background_ratio
echo 3 dirty_expire_centisecs
sync
- map a large file, one that fits comfortably into free memory
- bring the whole file in, by reading a byte from each page
- start the timer
- write a byte to each page
- stop the timer
It would be most interesting to try this on a filesystem supporting
nanosecond timestamps. Anyone know which these are?
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 -v5 2/2] Updating ctime and mtime at syncing
2008/1/17, Rogier Wolff [EMAIL PROTECTED]: On Thu, Jan 17, 2008 at 04:16:47PM +0300, Anton Salikhmetov wrote: 2008/1/17, Miklos Szeredi [EMAIL PROTECTED]: 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). Updating the time in remove_vma() would fix this, no? We need to save modification time. Otherwise, updating time stamps will be confusing the vi editor. If process B exits before vi opens the file, the timestamp should at the latest be the time that process B exits. There is no excuse for setting the timestamp later than the time that B exits. If process B no longer modifies the file, but still keeps it mapped until after vi starts, then the system can't help the situation. Wether or not B acesses those pages is unknown to the system. So you get what you deserve. The exit() system call closes the file memory-mapped file. Therefore, it's better to catch the close() system call. However, the close() system call does not trigger unmapping the file. The mapped area for the file may be used after closing the file. So, we should catch only the unmap() call, not close() or exit(). Roger. -- ** [EMAIL PROTECTED] ** http://www.BitWizard.nl/ ** +31-15-2600998 ** **Delftechpark 26 2628 XH Delft, The Netherlands. KVK: 27239233** *-- BitWizard writes Linux device drivers for any device you may have! --* Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. Does it sit on the couch all day? Is it unemployed? Please be specific! Define 'it' and what it isn't doing. - Adapted from lxrbot FAQ -- 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 -v5 2/2] Updating ctime and mtime at syncing
2008/1/17, Miklos Szeredi [EMAIL PROTECTED]: The do_wp_page() function is called in mm/memory.c after locking PTE. And the file_update_time() routine calls the filesystem operation that can sleep. It's not accepted, I guess. do_wp_page() is called with the pte lock but drops it, so that's fine. OK, I agree. I'll take into account your suggestion to move updating time stamps from the __set_page_dirty() and __set_page_dirty_nobuffers() routines to do_wp_page(). Thank you! 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/
[PATCH -v5 2/2] Updating ctime and mtime at syncing
http://bugzilla.kernel.org/show_bug.cgi?id=2645
Changes for updating the ctime and mtime fields for memory-mapped files:
1) a new flag triggering update of the inode data;
2) a new field in the address_space structure for saving modification time;
3) a new helper function to update ctime and mtime when needed;
4) updating time stamps for mapped files in sys_msync() and do_fsync();
5) implementing lazy ctime and mtime update.
Signed-off-by: Anton Salikhmetov <[EMAIL PROTECTED]>
---
fs/buffer.c |3 ++
fs/fs-writeback.c |2 +
fs/inode.c | 43 +++--
fs/sync.c |2 +
include/linux/fs.h | 13 +-
include/linux/pagemap.h |3 +-
mm/msync.c | 61 +-
mm/page-writeback.c | 54 ++---
8 files changed, 124 insertions(+), 57 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, >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(_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..edd5bf4 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1243,8 +1243,10 @@ 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
+ * @sync: whether to do synchronous update
*
* 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 +1255,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 +1264,41 @@ void file_update_time(struct file *file)
if (IS_RDONLY(inode))
return;
- now = current_fs_time(inode->i_sb);
- if (!timespec_equal(>i_mtime, )) {
- inode->i_mtime = now;
+ if (timespec_compare(>i_mtime, ts) < 0) {
+ inode->i_mtime = *ts;
sync_it = 1;
}
- if (!timespec_equal(>i_ctime, )) {
- inode->i_ctime = now;
+ if (timespec_compare(>i_ctime, ts) < 0) {
+ inode->i_ctime = *ts;
sync_it = 1;
}
if (sync_it)
mark_inode_dirty_sync(inode);
}
+EXPORT_SYMBOL(inode_update_time);
-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, >flags)) {
+ struct inode *inode = mapping->host;
+ struct timespec *ts = >mtime;
+
+ if (S_ISBLK(inode->i_mode)) {
+ struct block_device *bdev = inode->i_bdev;
+
+ mutex_lock(>bd_mutex);
+ list_for_each_entry(inode, >bd_inodes, i_devices)
+ inode_update_time(inode, ts);
+ mutex_unlock(>bd_mutex);
+ } else
+ inode_update_time(inode, ts);
+ }
+}
int inode_needs_sync(struct inode *inode)
{
@@ -1290,7 +1308,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 {
