Re: [PATCH] replace inode_update_time with file_update_time

[Anton Altaparmakov <aia21@cam.ac.uk>]

On Sat, 29 Oct 2005, Christoph Hellwig wrote:
> 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.)

> 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.

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).

Best regards,

	Anton

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6/fs/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/inode.c	2005-10-29 04:14:44.000000000 +0200
> +++ linux-2.6/fs/inode.c	2005-10-29 04:14:47.000000000 +0200
> @@ -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
> ===================================================================
> --- linux-2.6.orig/fs/ncpfs/file.c	2005-10-29 04:14:44.000000000 +0200
> +++ linux-2.6/fs/ncpfs/file.c	2005-10-29 04:14:47.000000000 +0200
> @@ -262,7 +262,7 @@
>  	}
>  	vfree(bouncebuffer);
>  
> -	inode_update_time(inode, 1);
> +	file_update_time(file);
>  
>  	*ppos = pos;
>  
> Index: linux-2.6/fs/pipe.c
> ===================================================================
> --- linux-2.6.orig/fs/pipe.c	2005-10-29 04:14:44.000000000 +0200
> +++ linux-2.6/fs/pipe.c	2005-10-29 04:14:47.000000000 +0200
> @@ -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
> ===================================================================
> --- linux-2.6.orig/fs/reiserfs/file.c	2005-10-29 04:14:44.000000000 +0200
> +++ linux-2.6/fs/reiserfs/file.c	2005-10-29 04:14:47.000000000 +0200
> @@ -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
> ===================================================================
> --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_lrw.c	2005-10-29 04:14:44.000000000 +0200
> +++ linux-2.6/fs/xfs/linux-2.6/xfs_lrw.c	2005-10-29 04:14:47.000000000 +0200
> @@ -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
> ===================================================================
> --- linux-2.6.orig/include/linux/fs.h	2005-10-29 04:14:44.000000000 +0200
> +++ linux-2.6/include/linux/fs.h	2005-10-29 04:15:01.000000000 +0200
> @@ -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
> ===================================================================
> --- linux-2.6.orig/mm/filemap.c	2005-10-29 04:14:44.000000000 +0200
> +++ linux-2.6/mm/filemap.c	2005-10-29 04:14:47.000000000 +0200
> @@ -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
> ===================================================================
> --- linux-2.6.orig/mm/filemap_xip.c	2005-10-29 04:14:44.000000000 +0200
> +++ linux-2.6/mm/filemap_xip.c	2005-10-29 04:14:47.000000000 +0200
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/