Re: [PATCH] memory mapped files not updating timestamps

[Peter Staubach <staubach@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1649 bytes --]

Eric Dumazet wrote:

> Peter Staubach a écrit :
>
>> --- linux-2.6.16.i686/mm/msync.c.org
>> +++ linux-2.6.16.i686/mm/msync.c
>> @@ -206,12 +206,16 @@ asmlinkage long sys_msync(unsigned long 
>>          file = vma->vm_file;
>>          start = vma->vm_end;
>>          if ((flags & MS_ASYNC) && file && nr_pages_dirtied) {
>> +            struct address_space *mapping = file->f_mapping;
>> +
>>              get_file(file);
>>              up_read(&current->mm->mmap_sem);
>> -            balance_dirty_pages_ratelimited_nr(file->f_mapping,
>> +            balance_dirty_pages_ratelimited_nr(mapping,
>>                              nr_pages_dirtied);
>>              fput(file);
>
>
> <here>, another thread can perform an munmap(), and the file can be 
> totally dismantled.
>
>>              down_read(&current->mm->mmap_sem);
>
>
> So referencing 'mapping' is *buggy* here.
> I believe that you have to move 'fput(file);' *after* the folloging 
> two lines.
>
>> +            if (test_and_clear_bit(AS_MCTIME, &mapping->flags))
>> +                inode_update_time(mapping->host);
>>              vma = find_vma(current->mm, start);
>>          } else if ((flags & MS_SYNC) && file &&
>>                  (vma->vm_flags & VM_SHARED)) {
>
>
>
> Eric


Yes, sorry, you mentioned that before and I meant to address that.  I almost
reordered those operations originally because I think that it is cleaner
when acquires and releases are done in the opposite order.  However, I was
also trying to change as little as possible too.

Anyway, attached is an updated patch.

    Thanx...

       ps

Signed-off-by: Peter Staubach <staubach@redhat.com>

[-- Attachment #2: mctime.devel --]
[-- Type: text/plain, Size: 5187 bytes --]

--- linux-2.6.16.x86_64/fs/inode.c.org
+++ linux-2.6.16.x86_64/fs/inode.c
@@ -1211,8 +1211,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
@@ -1222,9 +1222,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_dentry->d_inode;
 	struct timespec now;
 	int sync_it = 0;
 
@@ -1246,7 +1245,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.16.x86_64/fs/fs-writeback.c.org
+++ linux-2.6.16.x86_64/fs/fs-writeback.c
@@ -168,6 +168,9 @@ __sync_single_inode(struct inode *inode,
 
 	spin_unlock(&inode_lock);
 
+	if (test_and_clear_bit(AS_MCTIME, &mapping->flags))
+		inode_update_time(inode);
+
 	ret = do_writepages(mapping, wbc);
 
 	/* Don't write the inode if only I_DIRTY_PAGES was set */
--- linux-2.6.16.x86_64/fs/buffer.c.org
+++ linux-2.6.16.x86_64/fs/buffer.c
@@ -347,6 +347,10 @@ long do_fsync(struct file *file, int dat
 	if (!ret)
 		ret = err;
 	current->flags &= ~PF_SYNCWRITE;
+
+	if (test_and_clear_bit(AS_MCTIME, &mapping->flags))
+		inode_update_time(mapping->host);
+
 out:
 	return ret;
 }
@@ -837,6 +841,7 @@ EXPORT_SYMBOL(mark_buffer_dirty_inode);
 int __set_page_dirty_buffers(struct page *page)
 {
 	struct address_space * const mapping = page->mapping;
+	int ret = 0;
 
 	spin_lock(&mapping->private_lock);
 	if (page_has_buffers(page)) {
@@ -861,9 +866,13 @@ 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;
 	}
-	return 0;
+
+	if (page_mapped(page))
+		set_bit(AS_MCTIME, &mapping->flags);
+
+	return ret;
 }
 EXPORT_SYMBOL(__set_page_dirty_buffers);
 
--- linux-2.6.16.x86_64/include/linux/fs.h.org
+++ linux-2.6.16.x86_64/include/linux/fs.h
@@ -1778,7 +1778,12 @@ extern int buffer_migrate_page(struct pa
 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 *);
+
+static inline void file_update_time(struct file *file)
+{
+	inode_update_time(file->f_dentry->d_inode);
+}
 
 static inline ino_t parent_ino(struct dentry *dentry)
 {
--- linux-2.6.16.x86_64/include/linux/pagemap.h.org
+++ linux-2.6.16.x86_64/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.16.x86_64/mm/page-writeback.c.org
+++ linux-2.6.16.x86_64/mm/page-writeback.c
@@ -627,8 +627,10 @@ EXPORT_SYMBOL(write_one_page);
  */
 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) {
@@ -648,9 +650,11 @@ int __set_page_dirty_nobuffers(struct pa
 							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.16.x86_64/mm/msync.c.org
+++ linux-2.6.16.x86_64/mm/msync.c
@@ -206,12 +206,16 @@ asmlinkage long sys_msync(unsigned long 
 		file = vma->vm_file;
 		start = vma->vm_end;
 		if ((flags & MS_ASYNC) && file && nr_pages_dirtied) {
+			struct address_space *mapping = file->f_mapping;
+
 			get_file(file);
 			up_read(&current->mm->mmap_sem);
-			balance_dirty_pages_ratelimited_nr(file->f_mapping,
+			balance_dirty_pages_ratelimited_nr(mapping,
 							nr_pages_dirtied);
-			fput(file);
 			down_read(&current->mm->mmap_sem);
+			if (test_and_clear_bit(AS_MCTIME, &mapping->flags))
+				inode_update_time(mapping->host);
+			fput(file);
 			vma = find_vma(current->mm, start);
 		} else if ((flags & MS_SYNC) && file &&
 				(vma->vm_flags & VM_SHARED)) {
--- linux-2.6.16.x86_64/mm/mmap.c.org
+++ linux-2.6.16.x86_64/mm/mmap.c
@@ -203,6 +203,8 @@ void unlink_file_vma(struct vm_area_stru
 		spin_lock(&mapping->i_mmap_lock);
 		__remove_shared_vm_struct(vma, file, mapping);
 		spin_unlock(&mapping->i_mmap_lock);
+		if (test_and_clear_bit(AS_MCTIME, &mapping->flags))
+			inode_update_time(mapping->host);
 	}
 }