replace inode_update_time with file_update_time

Review

Quoted 4 lines · Christoph Hellwig

(quoted diff) +1 -0

 	struct timespec now;
...
+	inode->i_ctime = now;

hm, that nonatomic assignment is racy wrt readers and, conceivably, other writers.

To allow various options to work per-mount instead of per-sb we need

a struct vfsmount when updating ctime and mtime. This preparation patch replaces the inode_update_time routine with a file_update_atime routine so we can easily get at the vfsmount. (and the file makes more sense in this context anyway). Also get rid of the unused second argument - we always want to update the ctime when calling this routine.

Signed-off-by Christoph Hellwig <[email protected]>

Index: linux-2.6/fs/inode.c
===================================================================

fs/inode.c +11 -12

@@ -1203,16 +1203,16 @@
 EXPORT_SYMBOL(update_atime);
 
 /**
- *	inode_update_time	-	update mtime and ctime time
- *	@inode: inode accessed
- *	@ctime_too: update ctime too
+ *	file_update_time	-	update mtime and ctime time
+ *	@file: file accessed
  *
- *	Update the mtime time on an inode and mark it for writeback.
- *	When ctime_too is specified update the ctime too.
+ *	Update the mtime and ctime members of an inode and mark the inode
+ *	for writeback.
  */
 
-void inode_update_time(struct inode *inode, int ctime_too)
+void file_update_time(struct file *file)
 {
+	struct inode *inode = file->f_dentry->d_inode;
 	struct timespec now;
 	int sync_it = 0;
 

@@ -1226,16 +1226,15 @@
 		sync_it = 1;
 	inode->i_mtime = now;
 
-	if (ctime_too) {
-		if (!timespec_equal(&inode->i_ctime, &now))
-			sync_it = 1;
-		inode->i_ctime = now;
-	}
+	if (!timespec_equal(&inode->i_ctime, &now))
+		sync_it = 1;
+	inode->i_ctime = now;
+
 	if (sync_it)
 		mark_inode_dirty_sync(inode);
 }
 
-EXPORT_SYMBOL(inode_update_time);
+EXPORT_SYMBOL(file_update_time);
 
 int inode_needs_sync(struct inode *inode)
 {

Index: linux-2.6/fs/ncpfs/file.c
===================================================================

fs/ncpfs/file.c +1 -1

@@ -262,7 +262,7 @@
 	}
 	vfree(bouncebuffer);
 
-	inode_update_time(inode, 1);
+	file_update_time(file);
 
 	*ppos = pos;
 

Index: linux-2.6/fs/pipe.c
===================================================================

fs/pipe.c +1 -1

@@ -347,7 +347,7 @@
 		kill_fasync(PIPE_FASYNC_READERS(*inode), SIGIO, POLL_IN);
 	}
 	if (ret > 0)
-		inode_update_time(inode, 1);	/* mtime and ctime */
+		file_update_time(filp);
 	return ret;
 }
 

Index: linux-2.6/fs/reiserfs/file.c
===================================================================

fs/reiserfs/file.c +1 -1

@@ -1360,7 +1360,7 @@
 	if (res)
 		goto out;
 
-	inode_update_time(inode, 1);	/* Both mtime and ctime */
+	file_update_time(file);
 
 	// Ok, we are done with all the checks.
 

Index: linux-2.6/fs/xfs/linux-2.6/xfs_lrw.c
===================================================================

fs/xfs/linux-2.6/xfs_lrw.c +1 -1

@@ -740,7 +740,7 @@
 	 */
 	if (!(ioflags & IO_INVIS)) {
 		xfs_ichgtime(xip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
-		inode_update_time(inode, 1);
+		file_update_time(file);
 	}
 
 	/*

Index: linux-2.6/include/linux/fs.h
===================================================================

include/linux/fs.h +1 -1

@@ -1651,7 +1651,7 @@
 extern int inode_change_ok(struct inode *, struct iattr *);
 extern int __must_check inode_setattr(struct inode *, struct iattr *);
 
-extern void inode_update_time(struct inode *inode, int ctime_too);
+extern void file_update_time(struct file *file);
 
 static inline ino_t parent_ino(struct dentry *dentry)
 {

Index: linux-2.6/mm/filemap.c
===================================================================

mm/filemap.c +1 -1

@@ -2066,7 +2066,7 @@
 	if (err)
 		goto out;
 
-	inode_update_time(inode, 1);
+	file_update_time(file);
 
 	/* coalesce the iovecs and go direct-to-BIO for O_DIRECT */
 	if (unlikely(file->f_flags & O_DIRECT)) {

Index: linux-2.6/mm/filemap_xip.c
===================================================================

mm/filemap_xip.c +1 -2

@@ -381,7 +381,7 @@
 	if (ret)
 		goto out_backing;
 
-	inode_update_time(inode, 1);
+	file_update_time(filp);
 
 	ret = __xip_file_write (filp, buf, count, pos, ppos);
 
-

To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 1 line · Christoph Hellwig

To allow various options to work per-mount instead of per-sb we need

What are those various options? Please spell them out. (I mean it! I really do not know what you have in mind and I cannot see anything that would require a vfs mount wrt cmtime updates.)

Quoted 6 lines · Anton Altaparmakov

Quoted 1 line · Christoph Hellwig

To allow various options to work per-mount instead of per-sb we need

What are those various options? Please spell them out. (I mean it! I really do not know what you have in mind and I cannot see anything that would require a vfs mount wrt cmtime updates.)

I'm thinking you should think of things like read only bind mounts, where even meta data wont be updated.

Quoted 8 lines · Shaya Potter

Quoted 6 lines · Anton Altaparmakov

Quoted 1 line · Christoph Hellwig

To allow various options to work per-mount instead of per-sb we need

What are those various options? Please spell them out. (I mean it! I really do not know what you have in mind and I cannot see anything that would require a vfs mount wrt cmtime updates.)

Quoted 2 lines

I'm thinking you should think of things like read only bind mounts, where even meta data wont be updated.

But that is my point! A read-only bind mount is just like any other read-only mount and should never even try to update metadata.

Quoted 9 lines · Anton Altaparmakov

Quoted 8 lines · Shaya Potter

Quoted 6 lines · Anton Altaparmakov

Quoted 1 line · Christoph Hellwig

To allow various options to work per-mount instead of per-sb we need

What are those various options? Please spell them out. (I mean it! I really do not know what you have in mind and I cannot see anything that would require a vfs mount wrt cmtime updates.)

Quoted 5 lines

Quoted 2 lines

I'm thinking you should think of things like read only bind mounts, where even meta data wont be updated.

But that is my point! A read-only bind mount is just like any other read-only mount and should never even try to update metadata.

there's no such thing as a read only bind mount right now, I believe this is to enable support of such a thing, but I could be wrong.

But that is my point! A read-only bind mount is just like any other read-only mount and should never even try to update metadata.

Which codepaths cause inode/file_update_time() to be called for a read-only mount?

I do not believe there are any!

And assuming that I am correct this makes the IS_RDONLY() check pointless and this in turn makes the whole patch pointless

and given it
breaks existing file systems it should be sent to the nirvana of useless patches.

Quoted 5 lines · Anton Altaparmakov

But that is my point! A read-only bind mount is just like any other read-only mount and should never even try to update metadata.

Which codepaths cause inode/file_update_time() to be called for a read-only mount?

The callers right now are the write methods of the various filesystems, so you are right indeed that we shouldn't need it.

Quoted 4 lines

I do not believe there are any!

And assuming that I am correct this makes the IS_RDONLY() check pointless and this in turn makes the whole patch pointless

Yes, I think you're right here that we can removed it.

Quoted 3 lines

and given it
breaks existing file systems it should be sent to the nirvana of useless patches.

and here I disagree. Given that the only intended use of this function is to be used in the ->write methods the file is a useful argument. The

only other use that sneaked in recently is ntfs ->truncate that shouldn't use and has been fixed by a patch I sent offlist.

Quoted 1 line · Christoph Hellwig

To allow various options to work per-mount instead of per-sb we need

What are those various options? Please spell them out. (I mean it! I really do not know what you have in mind and I cannot see anything that would require a vfs mount wrt cmtime updates.)

I believe your patch is wrong/unnecessary because the only option that is per superblock (or in your new world per vfsmount) is the IS_RDONLY(inode) check and if a mount is read-only the VFS should never have allowed inode/file_update_time() to be called. The codepath should have been aborted _much_ earlier than that so it does not make any sense to test for read-only here thus it is not necessary to do a per mountpoint check. Instead we should remove the readonly check altogether and if there are any places where an update of cmtime is attempted on a read-only mount we should fix those instead...

A more detailed justification for your patch is IMO required before it be applied to mainline, especially as it breaks existing file systems like ntfs (as discussed offlist).

Quoted 6 lines · Anton Altaparmakov

Quoted 1 line · Christoph Hellwig

To allow various options to work per-mount instead of per-sb we need

What are those various options? Please spell them out. (I mean it! I really do not know what you have in mind and I cannot see anything that would require a vfs mount wrt cmtime updates.)

The ones I work now are noatime and read-only.

Quoted 10 lines

I believe your patch is wrong/unnecessary because the only option that is per superblock (or in your new world per vfsmount) is the IS_RDONLY(inode) check and if a mount is read-only the VFS should never have allowed inode/file_update_time() to be called. The codepath should have been aborted _much_ earlier than that so it does not make any sense to test for read-only here thus it is not necessary to do a per mountpoint check. Instead we should remove the readonly check altogether and if there are any places where an update of cmtime is attempted on a read-only mount we should fix those instead...

Quoted 3 lines

A more detailed justification for your patch is IMO required before it be applied to mainline, especially as it breaks existing file systems like ntfs (as discussed offlist).

Besides the above usage it's a nice cleanup and already found bugs like the API abuse in ntfs ->truncate.