[patch v2] fix truncate inode time modification breakage

[Nick Piggin <npiggin@suse.de>]

On Tue, Jun 01, 2010 at 11:56:55PM +1000, Nick Piggin wrote:
> On Tue, Jun 01, 2010 at 03:48:01PM +0200, Christoph Hellwig wrote:
> > On Tue, Jun 01, 2010 at 11:39:23PM +1000, Nick Piggin wrote:
> > > It appears that I've broken inode time modifications on tmpfs/ext2.
> > > While ftruncate always updates these attributes, truncate must not
> > > unless size is changed. I hadn't actually understood that until
> > > Christoph told me.
> > > 
> > > Confusion is increased because other filesystems get this wrong.
> > > Those without ->setattr or ->truncate get it wrong by default.
> > > Others appear to have problems too.
> > > 
> > > I haven't gone through many yet, but is there any reason not to
> > > just do it in the vfs?
> > 
> > Doing it in the VFS is fine with me, we still have the the file
> > pointer in struct iatta to indicate a ftruncate / open O_TRUNC
> > if any filesystem really cares.  But I think you need to audit
> > all instances if they care about this.  And while you're at it
> > also remove the code handling this and the comments about it in
> > XFS.
> 
> Yes I was starting to look at that. Just want to see if I'm on the
> right track.
> 
>  
> > > -	if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
> > > -		loff_t newsize = attr->ia_size;
> > > +	if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE) &&
> > > +						newsize != inode->i_size) {
> > 
> > Btw, the S_ISREG is superflous - we only ever set ATTR_SIZE for
> > regular files from the upper layer code.
> 
> OK I'll get rid of it in the same patch.

Well I looked through filesystems and I cannot seem to find anywhere
they can use ATTR_SIZE|ATTR_[MC]TIME to do anything strange that would
let them distinguish truncate from ftruncate and O_TRUNC.

I couldn't quite understand what FUSE was trying to do, or whether I
improved it.
---

Index: linux-2.6/fs/ext2/inode.c
===================================================================
--- linux-2.6.orig/fs/ext2/inode.c
+++ linux-2.6/fs/ext2/inode.c
@@ -1203,7 +1203,6 @@ int ext2_setsize(struct inode *inode, lo
 
 	__ext2_truncate_blocks(inode, newsize);
 
-	inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
 	if (inode_needs_sync(inode)) {
 		sync_mapping_buffers(inode->i_mapping);
 		ext2_sync_inode (inode);
Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c
+++ linux-2.6/fs/open.c
@@ -55,6 +55,9 @@ int do_truncate(struct dentry *dentry, l
 		newattrs.ia_valid |= ret | ATTR_FORCE;
 
 	mutex_lock(&dentry->d_inode->i_mutex);
+	/* Unlike ftruncate, truncate only updates times when size changes */
+	if (length != dentry->d_inode->i_size)
+		newattrs.ia_valid |= ATTR_MTIME|ATTR_CTIME;
 	ret = notify_change(dentry, &newattrs);
 	mutex_unlock(&dentry->d_inode->i_mutex);
 	return ret;
Index: linux-2.6/mm/shmem.c
===================================================================
--- linux-2.6.orig/mm/shmem.c
+++ linux-2.6/mm/shmem.c
@@ -764,10 +764,10 @@ done2:
 static int shmem_notify_change(struct dentry *dentry, struct iattr *attr)
 {
 	struct inode *inode = dentry->d_inode;
+	loff_t newsize = attr->ia_size;
 	int error;
 
-	if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
-		loff_t newsize = attr->ia_size;
+	if ((attr->ia_valid & ATTR_SIZE) && newsize != inode->i_size) {
 		struct page *page = NULL;
 
 		if (newsize < inode->i_size) {
Index: linux-2.6/fs/ramfs/file-nommu.c
===================================================================
--- linux-2.6.orig/fs/ramfs/file-nommu.c
+++ linux-2.6/fs/ramfs/file-nommu.c
@@ -175,11 +175,6 @@ static int ramfs_nommu_setattr(struct de
 			ret = ramfs_nommu_resize(inode, ia->ia_size, size);
 			if (ret < 0 || ia->ia_valid == ATTR_SIZE)
 				goto out;
-		} else {
-			/* we skipped the truncate but must still update
-			 * timestamps
-			 */
-			ia->ia_valid |= ATTR_MTIME|ATTR_CTIME;
 		}
 	}
 
Index: linux-2.6/fs/xfs/xfs_vnodeops.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_vnodeops.c
+++ linux-2.6/fs/xfs/xfs_vnodeops.c
@@ -286,25 +286,6 @@ xfs_setattr(
 		xfs_trans_ijoin(tp, ip, lock_flags);
 		xfs_trans_ihold(tp, ip);
 
-		/*
-		 * Only change the c/mtime if we are changing the size
-		 * or we are explicitly asked to change it. This handles
-		 * the semantic difference between truncate() and ftruncate()
-		 * as implemented in the VFS.
-		 *
-		 * The regular truncate() case without ATTR_CTIME and ATTR_MTIME
-		 * is a special case where we need to update the times despite
-		 * not having these flags set.  For all other operations the
-		 * VFS set these flags explicitly if it wants a timestamp
-		 * update.
-		 */
-		if (iattr->ia_size != ip->i_size &&
-		    (!(mask & (ATTR_CTIME | ATTR_MTIME)))) {
-			iattr->ia_ctime = iattr->ia_mtime =
-				current_fs_time(inode->i_sb);
-			mask |= ATTR_CTIME | ATTR_MTIME;
-		}
-
 		if (iattr->ia_size > ip->i_size) {
 			ip->i_d.di_size = iattr->ia_size;
 			ip->i_size = iattr->ia_size;
Index: linux-2.6/fs/fuse/dir.c
===================================================================
--- linux-2.6.orig/fs/fuse/dir.c
+++ linux-2.6/fs/fuse/dir.c
@@ -1161,20 +1161,6 @@ static int fuse_dir_fsync(struct file *f
 	return fuse_fsync_common(file, datasync, 1);
 }
 
-static bool update_mtime(unsigned ivalid)
-{
-	/* Always update if mtime is explicitly set  */
-	if (ivalid & ATTR_MTIME_SET)
-		return true;
-
-	/* If it's an open(O_TRUNC) or an ftruncate(), don't update */
-	if ((ivalid & ATTR_SIZE) && (ivalid & (ATTR_OPEN | ATTR_FILE)))
-		return false;
-
-	/* In all other cases update */
-	return true;
-}
-
 static void iattr_to_fattr(struct iattr *iattr, struct fuse_setattr_in *arg)
 {
 	unsigned ivalid = iattr->ia_valid;
@@ -1194,7 +1180,7 @@ static void iattr_to_fattr(struct iattr
 		if (!(ivalid & ATTR_ATIME_SET))
 			arg->valid |= FATTR_ATIME_NOW;
 	}
-	if ((ivalid & ATTR_MTIME) && update_mtime(ivalid)) {
+	if (ivalid & ATTR_MTIME) {
 		arg->valid |= FATTR_MTIME;
 		arg->mtime = iattr->ia_mtime.tv_sec;
 		arg->mtimensec = iattr->ia_mtime.tv_nsec;
Index: linux-2.6/fs/ntfs/inode.c
===================================================================
--- linux-2.6.orig/fs/ntfs/inode.c
+++ linux-2.6/fs/ntfs/inode.c
@@ -2917,12 +2917,6 @@ int ntfs_setattr(struct dentry *dentry,
 				err = vmtruncate(vi, attr->ia_size);
 			if (err || ia_valid == ATTR_SIZE)
 				goto out;
-		} else {
-			/*
-			 * We skipped the truncate but must still update
-			 * timestamps.
-			 */
-			ia_valid |= ATTR_MTIME | ATTR_CTIME;
 		}
 	}
 	if (ia_valid & ATTR_ATIME)
Index: linux-2.6/fs/reiserfs/inode.c
===================================================================
--- linux-2.6.orig/fs/reiserfs/inode.c
+++ linux-2.6/fs/reiserfs/inode.c
@@ -3105,11 +3105,6 @@ int reiserfs_setattr(struct dentry *dent
 			}
 			if (error)
 				goto out;
-			/*
-			 * file size is changed, ctime and mtime are
-			 * to be updated
-			 */
-			attr->ia_valid |= (ATTR_MTIME | ATTR_CTIME);
 		}
 	}
 
Index: linux-2.6/fs/ubifs/file.c
===================================================================
--- linux-2.6.orig/fs/ubifs/file.c
+++ linux-2.6/fs/ubifs/file.c
@@ -1175,8 +1175,6 @@ static int do_truncation(struct ubifs_in
 
 	mutex_lock(&ui->ui_mutex);
 	ui->ui_size = inode->i_size;
-	/* Truncation changes inode [mc]time */
-	inode->i_mtime = inode->i_ctime = ubifs_current_time(inode);
 	/* Other attributes may be changed at the same time as well */
 	do_attr_changes(inode, attr);
 	err = ubifs_jnl_truncate(c, inode, old_size, new_size);
@@ -1224,8 +1222,6 @@ static int do_setattr(struct ubifs_info
 
 	mutex_lock(&ui->ui_mutex);
 	if (attr->ia_valid & ATTR_SIZE) {
-		/* Truncation changes inode [mc]time */
-		inode->i_mtime = inode->i_ctime = ubifs_current_time(inode);
 		/* 'simple_setsize()' changed @i_size, update @ui_size */
 		ui->ui_size = inode->i_size;
 	}