Re: [PATCH 2/2][RFC][BUG] msync: updating ctime and mtime at syncing
From: Anton Salikhmetov
Date: Fri Jan 11 2008 - 16:40:39 EST
2008/1/11, Peter Staubach <staubach@xxxxxxxxxx>:
> Anton Salikhmetov wrote:
> > From: Anton Salikhmetov <salikhmetov@xxxxxxxxx>
> >
> > The patch contains changes for updating the ctime and mtime fields for memory mapped files:
> >
> > 1) adding a new flag triggering update of the inode data;
> > 2) implementing a helper function for checking that flag and updating ctime and mtime;
> > 3) updating time stamps for mapped files in sys_msync() and do_fsync().
>
> Sorry, one other issue to throw out too -- an mmap'd block device
> should also have its inode time fields updated. This is a little
> tricky because the inode referenced via mapping->host isn't the
> one that needs to have the time fields updated on.
>
> I have attached the patch that I submitted last. It is quite out
> of date, but does show my attempt to resolve some of these issues.
Thanks for your feedback!
Now I'm looking at your solution and thinking about which parts of it
I could adapt to the infrastructure I'm trying to develop.
However, I would like to address the block device case within
a separate project. But for now, I want the msync() and fsync()
system calls to update ctime and mtime at least for memory-mapped
regular files properly. I feel that even this little improvement could address
many customer's troubles such as the one Jacob Oestergaard reported
in the bug #2645.
>
> Thanx...
>
> ps
>
> --- linux-2.6.20.i686/fs/buffer.c.org
> +++ linux-2.6.20.i686/fs/buffer.c
> @@ -710,6 +710,7 @@ EXPORT_SYMBOL(mark_buffer_dirty_inode);
> int __set_page_dirty_buffers(struct page *page)
> {
> struct address_space * const mapping = page_mapping(page);
> + int ret = 0;
>
> if (unlikely(!mapping))
> return !TestSetPageDirty(page);
> @@ -727,7 +728,7 @@ int __set_page_dirty_buffers(struct page
> spin_unlock(&mapping->private_lock);
>
> if (TestSetPageDirty(page))
> - return 0;
> + goto out;
>
> write_lock_irq(&mapping->tree_lock);
> if (page->mapping) { /* Race with truncate? */
> @@ -740,7 +741,11 @@ int __set_page_dirty_buffers(struct page
> }
> write_unlock_irq(&mapping->tree_lock);
> __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
> - return 1;
> + ret = 1;
> +out:
> + if (page_mapped(page))
> + set_bit(AS_MCTIME, &mapping->flags);
> + return ret;
> }
> EXPORT_SYMBOL(__set_page_dirty_buffers);
>
> --- linux-2.6.20.i686/fs/fs-writeback.c.org
> +++ linux-2.6.20.i686/fs/fs-writeback.c
> @@ -167,6 +167,13 @@ __sync_single_inode(struct inode *inode,
>
> spin_unlock(&inode_lock);
>
> + if (test_and_clear_bit(AS_MCTIME, &mapping->flags)) {
> + if (S_ISBLK(inode->i_mode))
> + bd_inode_update_time(inode);
> + else
> + inode_update_time(inode);
> + }
> +
> ret = do_writepages(mapping, wbc);
>
> /* Don't write the inode if only I_DIRTY_PAGES was set */
> --- linux-2.6.20.i686/fs/inode.c.org
> +++ linux-2.6.20.i686/fs/inode.c
> @@ -1201,8 +1201,8 @@ void touch_atime(struct vfsmount *mnt, s
> EXPORT_SYMBOL(touch_atime);
>
> /**
> - * file_update_time - update mtime and ctime time
> - * @file: file accessed
> + * inode_update_time - update mtime and ctime time
> + * @inode: file accessed
> *
> * Update the mtime and ctime members of an inode and mark the inode
> * for writeback. Note that this function is meant exclusively for
> @@ -1212,9 +1212,8 @@ EXPORT_SYMBOL(touch_atime);
> * timestamps are handled by the server.
> */
>
> -void file_update_time(struct file *file)
> +void inode_update_time(struct inode *inode)
> {
> - struct inode *inode = file->f_path.dentry->d_inode;
> struct timespec now;
> int sync_it = 0;
>
> @@ -1238,7 +1237,7 @@ void file_update_time(struct file *file)
> mark_inode_dirty_sync(inode);
> }
>
> -EXPORT_SYMBOL(file_update_time);
> +EXPORT_SYMBOL(inode_update_time);
>
> int inode_needs_sync(struct inode *inode)
> {
> --- linux-2.6.20.i686/fs/block_dev.c.org
> +++ linux-2.6.20.i686/fs/block_dev.c
> @@ -608,6 +608,22 @@ void bdput(struct block_device *bdev)
>
> EXPORT_SYMBOL(bdput);
>
> +void bd_inode_update_time(struct inode *inode)
> +{
> + struct block_device *bdev = inode->i_bdev;
> + struct list_head *p;
> +
> + if (bdev == NULL)
> + return;
> +
> + spin_lock(&bdev_lock);
> + list_for_each(p, &bdev->bd_inodes) {
> + inode = list_entry(p, struct inode, i_devices);
> + inode_update_time(inode);
> + }
> + spin_unlock(&bdev_lock);
> +}
> +
> static struct block_device *bd_acquire(struct inode *inode)
> {
> struct block_device *bdev;
> --- linux-2.6.20.i686/include/linux/fs.h.org
> +++ linux-2.6.20.i686/include/linux/fs.h
> @@ -1488,6 +1488,7 @@ extern struct block_device *bdget(dev_t)
> extern void bd_set_size(struct block_device *, loff_t size);
> extern void bd_forget(struct inode *inode);
> extern void bdput(struct block_device *);
> +extern void bd_inode_update_time(struct inode *);
> extern struct block_device *open_by_devnum(dev_t, unsigned);
> extern const struct address_space_operations def_blk_aops;
> #else
> @@ -1892,7 +1893,11 @@ extern int buffer_migrate_page(struct ad
> 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 *inode);
> +static inline void file_update_time(struct file *file)
> +{
> + inode_update_time(file->f_path.dentry->d_inode);
> +}
>
> static inline ino_t parent_ino(struct dentry *dentry)
> {
> --- linux-2.6.20.i686/include/linux/pagemap.h.org
> +++ linux-2.6.20.i686/include/linux/pagemap.h
> @@ -16,8 +16,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) /* need m/ctime change */
>
> static inline gfp_t mapping_gfp_mask(struct address_space * mapping)
> {
> --- linux-2.6.20.i686/mm/page-writeback.c.org
> +++ linux-2.6.20.i686/mm/page-writeback.c
> @@ -760,8 +760,10 @@ int __set_page_dirty_no_writeback(struct
> */
> int __set_page_dirty_nobuffers(struct page *page)
> {
> + struct address_space *mapping = page_mapping(page);
> + int ret = 0;
> +
> if (!TestSetPageDirty(page)) {
> - struct address_space *mapping = page_mapping(page);
> struct address_space *mapping2;
>
> if (!mapping)
> @@ -783,9 +785,11 @@ int __set_page_dirty_nobuffers(struct pa
> /* !PageAnon && !swapper_space */
> __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
> }
> - return 1;
> + ret = 1;
> }
> - return 0;
> + if (page_mapped(page))
> + set_bit(AS_MCTIME, &mapping->flags);
> + return ret;
> }
> EXPORT_SYMBOL(__set_page_dirty_nobuffers);
>
> --- linux-2.6.20.i686/mm/msync.c.org
> +++ linux-2.6.20.i686/mm/msync.c
> @@ -12,6 +12,7 @@
> #include <linux/mman.h>
> #include <linux/file.h>
> #include <linux/syscalls.h>
> +#include <linux/pagemap.h>
>
> /*
> * MS_SYNC syncs the entire file - including mappings.
> @@ -77,11 +78,16 @@ asmlinkage long sys_msync(unsigned long
> }
> file = vma->vm_file;
> start = vma->vm_end;
> - if ((flags & MS_SYNC) && file &&
> - (vma->vm_flags & VM_SHARED)) {
> + if ((flags & (MS_SYNC|MS_ASYNC)) &&
> + file && (vma->vm_flags & VM_SHARED)) {
> get_file(file);
> up_read(&mm->mmap_sem);
> - error = do_fsync(file, 0);
> + error = 0;
> + if (flags & MS_SYNC)
> + error = do_fsync(file, 0);
> + else if (test_and_clear_bit(AS_MCTIME,
> + &file->f_mapping->flags))
> + file_update_time(file);
> fput(file);
> if (error || start >= end)
> goto out;
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/