[patch] fix truncate inode time modification breakage

[patch] fix truncate inode time modification breakage

[Nick Piggin]

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?

---
 fs/ext2/inode.c       |    1 -
 fs/open.c             |    3 +++
 fs/ramfs/file-nommu.c |    5 -----
 mm/shmem.c            |    5 +++--
 4 files changed, 6 insertions(+), 8 deletions(-)

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,11 @@ 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 (S_ISREG(inode->i_mode) && (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;
 		}
 	}
 

Re: [patch] fix truncate inode time modification breakage

[Christoph Hellwig]

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.

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

Re: [patch] fix truncate inode time modification breakage

[Nick Piggin]

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.

[patch v2] fix truncate inode time modification breakage

[Nick Piggin]

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;
 	}

Filesystem setattr/truncate notes and problems

[Nick Piggin]

On Thu, Jun 03, 2010 at 05:55:38AM +1000, Nick Piggin wrote:
> 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.

Looking through the filesystems, I thought I found a number of issues.
I'll try to list what I understand to be important steps required in
setattr. Unless I'm way off, a lot of filesystems get these wrong, so
we may want to add some guideline to docs?

Anyway feedback on these would be appreciated. Below this there are
some questions about specific filesystems too.

Filesystems setattr methods should:

- Perform idempotent error checks as early as possible. Perform actual
  i_size change and pagecache truncation last[*]. Truncate means dirty
  pagecache is fine to be thrown out at any time[**], what is important
  is that it is not thrown out before the operation can complete (eg. by
  doing the on-disk truncation).

  [*] Pagecache truncation is irreversable unless the filesystem can
  guarantee that the page is not mapped and has no dangling
  get_user_pages references (it's almost impossible to check this
  properly unless get_user_pages is disallowed from the start) or the
  file has been readonly. Just writing back the pagecache does not
  guarantee no data loss window. So it's probably easiest to do
  pagecache truncation last.

  [**] Be careful, this might mean the page is subject to writeout
  inside i_size after the on-disk truncation. Buffer based filesystems
  will probably need rework to handle this properly.

- Update mtime and ctime IFF ATTR_MTIME and ATTR_CTIME are set in
  setattr.

- Update mode before owner or size if mode update can fail. Otherwise
  killing of suid could fail after owner/size has changed (ideally
  these would be all done atomically). Although we do already have weird
  races in permission checking in core vfs code anyway.

- Perform time modifications iff others have/will succeeded (these are
  applicable for chmod, chown, etc too). Do not succeed a truncate and
  then error out because the time cannot be set. Do not set the time but
  fail the truncate or chown etc.

- If ATTR_SIZE change cannot succeed, don't ignore it, return something
  (like -EPERM or -EINVAL), right? (lots of filesystems just mask it
  off, maybe that is preferable behaviour sometimes?)

- Remember to actually trim off disk blocks or underlying object past
  i_size in response to setattr shrinking the file.

- In case of write_begin/write_end/direct_IO failure, trim object past
  i_size if it was possible to have been allocated before the operation
  fails.


Interesting specific filesystem questions: (not comprehensive)

ecryptfs improve mtime/ctime handling for truncate of lower inode

fuse, should be setting ctime on truncate?

gfs2, mtime and ctime should only be set in truncate if the ATTR_?TIME
are set.

hostfs, don't ignore ATTR_SIZE. ftruncate/truncate have interesting
different semantics for timestamp updates, so don't use fd != -1 for
these (could use utimes for this, but it causes an atomicity/error
handling issue).

hpfs could use cont_expand to do expanding truncates?

ncpfs, nfs smbfs, cifs seems to perform inode size checks
(inode_newsize_ok) after truncating the file on the server??

ntfs does not do inode size checks properly.

logfs logfs_truncate without doing inode_change_ok, inode_newsize_ok etc
can fail inode_setattr after successful truncate (truncate(2) would
return failure and yet the file would be truncated).

procfs why not return -EPERM rather than setting size?

reiserfs should do all setattr checks early as possible
(inode_change_ok, inode_newsize_ok).

ubifs should not update access times unconditionally when ATTR_SIZE is
set

ufs simple_setsize still destroys the pagecache and causes concurrent
reads to go EOF past updated i_size. Restoring old i_size is too late
I think. Should do those checks first (in fairness lots of filesystems
have bit problems with error handling, but ufs attempts a little bit and
gets it wrong).

xfs in the 0 length file shortcut, isn't this missing suid kill case?
(ftruncate mandatory mtime update seems like it wasn't working right
there either before this patch).

nfsd should not change attributes in the case of truncated file with no
size change?

Re: Filesystem setattr/truncate notes and problems

[Christoph Hellwig]

On Thu, Jun 03, 2010 at 06:08:55AM +1000, Nick Piggin wrote:
> xfs in the 0 length file shortcut, isn't this missing suid kill case?
> (ftruncate mandatory mtime update seems like it wasn't working right
> there either before this patch).

The wording from Posix for truncate is:

"Upon successful completion, if the file size is changed, this function
 shall mark for update the st_ctime and st_mtime fields of the file, and
 the S_ISUID and S_ISGID bits of the file mode may be cleared."

and for truncate:

"Upon successful completion, if fildes refers to a regular file, the
 ftruncate() function shall mark for update the st_ctime and st_mtime
 fields of the file and the S_ISUID and S_ISGID bits of the file mode
 may be cleared."

Which translates to me that the suid/sgid clearing behaviour is
exactly the same as the c/mtime update and it can be skipped for
truncate only if there is no size change.  Except that the wording
is rather nasty as the suid/sgid bits are only accepted into Posix
as an ugly headed stepchild and may not actually be present on
some systems.

Which means XFS behaviour for truncate while not strictly against
Posix is at least unexpected.  I'll fix it up.

Re: Filesystem setattr/truncate notes and problems

[Christoph Hellwig]

On Thu, Jun 03, 2010 at 09:28:12AM +0200, Christoph Hellwig wrote:
> Which means XFS behaviour for truncate while not strictly against
> Posix is at least unexpected.  I'll fix it up.

Actually I'd prefer if you could throw this into your do_truncate
patch so that filesystems don't get the clear suid request if
the file size doesn't change.  That way I can just change XFS
to do everything requested but the actual size update for that case.
In fact just clearing out ATTR_SIZE in that case in do_truncate would
be nice.

Which brings up the question: are we guaranteed to have stable
and uptodate i_size in do_truncate?  I think normally we'd need
a ->getattr first to stabilize it.

Re: Filesystem setattr/truncate notes and problems

[Nick Piggin]

On Thu, Jun 03, 2010 at 09:32:38AM +0200, Christoph Hellwig wrote:
> On Thu, Jun 03, 2010 at 09:28:12AM +0200, Christoph Hellwig wrote:
> > Which means XFS behaviour for truncate while not strictly against
> > Posix is at least unexpected.  I'll fix it up.
> 
> Actually I'd prefer if you could throw this into your do_truncate
> patch so that filesystems don't get the clear suid request if
> the file size doesn't change.

Yes it should be done the same way as mtime/ctime updates are
done.


>  That way I can just change XFS
> to do everything requested but the actual size update for that case.
> In fact just clearing out ATTR_SIZE in that case in do_truncate would
> be nice.

I thought I spotted a reason why why should not do that... I guess it
is because some filesystems were choosing to do things like update ctime
even when file size does not change. But if those all get stamped out,
it can probably go away.

 
> Which brings up the question: are we guaranteed to have stable
> and uptodate i_size in do_truncate?  I think normally we'd need
> a ->getattr first to stabilize it.

Good point actually. I wonder if it would be better then to pass down
specific flags from the truncate code, where filesystems can call a
helper to turn them into CTIME|MTIME|MODE flags when they have a
stable i_size?

Re: Filesystem setattr/truncate notes and problems

[Nick Piggin]

On Thu, Jun 03, 2010 at 09:28:12AM +0200, Christoph Hellwig wrote:
> On Thu, Jun 03, 2010 at 06:08:55AM +1000, Nick Piggin wrote:
> > xfs in the 0 length file shortcut, isn't this missing suid kill case?
> > (ftruncate mandatory mtime update seems like it wasn't working right
> > there either before this patch).
> 
> The wording from Posix for truncate is:
> 
> "Upon successful completion, if the file size is changed, this function
>  shall mark for update the st_ctime and st_mtime fields of the file, and
>  the S_ISUID and S_ISGID bits of the file mode may be cleared."
> 
> and for truncate:
> 
> "Upon successful completion, if fildes refers to a regular file, the
>  ftruncate() function shall mark for update the st_ctime and st_mtime
>  fields of the file and the S_ISUID and S_ISGID bits of the file mode
>  may be cleared."
> 
> Which translates to me that the suid/sgid clearing behaviour is
> exactly the same as the c/mtime update and it can be skipped for
> truncate only if there is no size change.  Except that the wording
> is rather nasty as the suid/sgid bits are only accepted into Posix
> as an ugly headed stepchild and may not actually be present on
> some systems.

Well at least we should do suid bit clearing consistently with ctime/mtime
updates if we do them at all, that part of the wording seems clear.

Re: Filesystem setattr/truncate notes and problems

[Nick Piggin]

On Thu, Jun 03, 2010 at 06:08:55AM +1000, Nick Piggin wrote:
> Interesting specific filesystem questions: (not comprehensive)
> 
> ecryptfs improve mtime/ctime handling for truncate of lower inode
> 
> fuse, should be setting ctime on truncate?
> 
> gfs2, mtime and ctime should only be set in truncate if the ATTR_?TIME
> are set.
> 
> hostfs, don't ignore ATTR_SIZE. ftruncate/truncate have interesting
> different semantics for timestamp updates, so don't use fd != -1 for
> these (could use utimes for this, but it causes an atomicity/error
> handling issue).
> 
> hpfs could use cont_expand to do expanding truncates?
> 
> ncpfs, nfs smbfs, cifs seems to perform inode size checks
> (inode_newsize_ok) after truncating the file on the server??
> 
> ntfs does not do inode size checks properly.
> 
> logfs logfs_truncate without doing inode_change_ok, inode_newsize_ok etc
> can fail inode_setattr after successful truncate (truncate(2) would
> return failure and yet the file would be truncated).
> 
> procfs why not return -EPERM rather than setting size?
> 
> reiserfs should do all setattr checks early as possible
> (inode_change_ok, inode_newsize_ok).
> 
> ubifs should not update access times unconditionally when ATTR_SIZE is
> set
> 
> ufs simple_setsize still destroys the pagecache and causes concurrent
> reads to go EOF past updated i_size. Restoring old i_size is too late
> I think. Should do those checks first (in fairness lots of filesystems
> have bit problems with error handling, but ufs attempts a little bit and
> gets it wrong).
> 
> xfs in the 0 length file shortcut, isn't this missing suid kill case?
> (ftruncate mandatory mtime update seems like it wasn't working right
> there either before this patch).
> 
> nfsd should not change attributes in the case of truncated file with no
> size change?

ceph doesn't seem to call inode_newsize_ok (so it's missing the rlimit
check).

Re: [patch v2] fix truncate inode time modification breakage

[Miklos Szeredi]

On Thu, 3 Jun 2010, Nick Piggin wrote:
> 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;
> -}
> -

Fuse philosophy is: each operation itself has to update times on files
if necessary.  So it basically moves the responsibility to update
[amc]time from the VFS into the filesystem.

This means the only place fuse is interested in ATTR_ATIME or
ATTR_MTIME is for the utime* syscalls.

It also means that fuse always ignores ATTR_CTIME which is never set
explicitly.

So I believe the current fuse code is correct.

Thanks,
Miklos

Re: [patch v2] fix truncate inode time modification breakage

[Boaz Harrosh]

On 06/03/2010 11:18 AM, Miklos Szeredi wrote:
> On Thu, 3 Jun 2010, Nick Piggin wrote:
>> 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;
>> -}
>> -
> 
> Fuse philosophy is: each operation itself has to update times on files
> if necessary.  So it basically moves the responsibility to update
> [amc]time from the VFS into the filesystem.
> 
> This means the only place fuse is interested in ATTR_ATIME or
> ATTR_MTIME is for the utime* syscalls.
> 
> It also means that fuse always ignores ATTR_CTIME which is never set
> explicitly.
> 
> So I believe the current fuse code is correct.
> 

It might be correct, but there were reports it has problems with NFS export.
Why let the filesystems be broken? Why not do the common stuff common? (In VFS)

Boaz

> Thanks,
> Miklos
> --
> 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

Re: [patch v2] fix truncate inode time modification breakage

[Miklos Szeredi]

On Thu, 03 Jun 2010, Boaz Harrosh wrote:
> > Fuse philosophy is: each operation itself has to update times on files
> > if necessary.  So it basically moves the responsibility to update
> > [amc]time from the VFS into the filesystem.
> > 
> > This means the only place fuse is interested in ATTR_ATIME or
> > ATTR_MTIME is for the utime* syscalls.
> > 
> > It also means that fuse always ignores ATTR_CTIME which is never set
> > explicitly.
> > 
> > So I believe the current fuse code is correct.
> > 
> 
> It might be correct, but there were reports it has problems with NFS export.

Do you have details?  I can't remember any report related to time
modification and NFS export.

> Why let the filesystems be broken? Why not do the common stuff
> common? (In VFS)

For consistency and simplicity.  The fuse API looks much like the
syscall API, which means that for a truncate() call on a fuse file the
truncate() method will be called in the filesystem, not a truncate() +
utimes().  And so on...

Thanks,
Miklos

Re: [patch v2] fix truncate inode time modification breakage

[Boaz Harrosh]

On 06/03/2010 12:05 PM, Miklos Szeredi wrote:
> On Thu, 03 Jun 2010, Boaz Harrosh wrote:
>>> Fuse philosophy is: each operation itself has to update times on files
>>> if necessary.  So it basically moves the responsibility to update
>>> [amc]time from the VFS into the filesystem.
>>>
>>> This means the only place fuse is interested in ATTR_ATIME or
>>> ATTR_MTIME is for the utime* syscalls.
>>>
>>> It also means that fuse always ignores ATTR_CTIME which is never set
>>> explicitly.
>>>
>>> So I believe the current fuse code is correct.
>>>
>>
>> It might be correct, but there were reports it has problems with NFS export.
> 
> Do you have details?  I can't remember any report related to time
> modification and NFS export.
> 
>> Why let the filesystems be broken? Why not do the common stuff
>> common? (In VFS)
> 
> For consistency and simplicity.  The fuse API looks much like the
> syscall API, which means that for a truncate() call on a fuse file the
> truncate() method will be called in the filesystem, not a truncate() +
> utimes().  And so on...
> 

OK So that has now changed there is a ->setattr from VFS ->truncate is
been killed. And the VFS could be trusted to do what it knows how to do?
(I think)

> Thanks,
> Miklos

Boaz

Re: [patch v2] fix truncate inode time modification breakage

[Nick Piggin]

On Thu, Jun 03, 2010 at 10:18:24AM +0200, Miklos Szeredi wrote:
> On Thu, 3 Jun 2010, Nick Piggin wrote:
> > 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;
> > -}
> > -
> 
> Fuse philosophy is: each operation itself has to update times on files
> if necessary.  So it basically moves the responsibility to update
> [amc]time from the VFS into the filesystem.
> 
> This means the only place fuse is interested in ATTR_ATIME or
> ATTR_MTIME is for the utime* syscalls.

OK, makes sense. I wonder why you do ATTR_ATIME changes, though,
rather than just getting those too back from the filesystem?

Do you ever have a problem with inode's atime going backwards due
to differences between touch_atime and reading the atime from the
server?


> It also means that fuse always ignores ATTR_CTIME which is never set
> explicitly.
> 
> So I believe the current fuse code is correct.

After my patch to pass ATTR_MTIME|ATTR_CTIME from truncate(2), it is
not (because above won't return false, ie. it will change the mtime
for truncate).

Why not avoid all mtime updates except MTIME_SET?

Re: [patch v2] fix truncate inode time modification breakage

[Miklos Szeredi]

On Thu, 3 Jun 2010, Nick Piggin wrote:
> > Fuse philosophy is: each operation itself has to update times on files
> > if necessary.  So it basically moves the responsibility to update
> > [amc]time from the VFS into the filesystem.
> > 
> > This means the only place fuse is interested in ATTR_ATIME or
> > ATTR_MTIME is for the utime* syscalls.
> 
> OK, makes sense. I wonder why you do ATTR_ATIME changes, though,
> rather than just getting those too back from the filesystem?

It does that, through setting S_NOATIME and invalidating attributes
after each op.

> > It also means that fuse always ignores ATTR_CTIME which is never set
> > explicitly.
> > 
> > So I believe the current fuse code is correct.
> 
> After my patch to pass ATTR_MTIME|ATTR_CTIME from truncate(2), it is
> not (because above won't return false, ie. it will change the mtime
> for truncate).
> 
> Why not avoid all mtime updates except MTIME_SET?

Because utimes(NULL) will supply only ATTR_ATIME | ATTR_MTIME.

I guess fuse should do something like this:

	if (valid ^ (ATTR_MTIME | ATTR_CTIME | ATTR_SIZE) == 0)
		valid = ATTR_SIZE;

Thanks,
Miklos

Re: [patch v2] fix truncate inode time modification breakage

[Nick Piggin]

On Thu, Jun 03, 2010 at 11:28:25AM +0200, Miklos Szeredi wrote:
> On Thu, 3 Jun 2010, Nick Piggin wrote:
> > > Fuse philosophy is: each operation itself has to update times on files
> > > if necessary.  So it basically moves the responsibility to update
> > > [amc]time from the VFS into the filesystem.
> > > 
> > > This means the only place fuse is interested in ATTR_ATIME or
> > > ATTR_MTIME is for the utime* syscalls.
> > 
> > OK, makes sense. I wonder why you do ATTR_ATIME changes, though,
> > rather than just getting those too back from the filesystem?
> 
> It does that, through setting S_NOATIME and invalidating attributes
> after each op.

Oh ok.

 
> > > It also means that fuse always ignores ATTR_CTIME which is never set
> > > explicitly.
> > > 
> > > So I believe the current fuse code is correct.
> > 
> > After my patch to pass ATTR_MTIME|ATTR_CTIME from truncate(2), it is
> > not (because above won't return false, ie. it will change the mtime
> > for truncate).
> > 
> > Why not avoid all mtime updates except MTIME_SET?
> 
> Because utimes(NULL) will supply only ATTR_ATIME | ATTR_MTIME.

Good answer.

 
> I guess fuse should do something like this:
> 
> 	if (valid ^ (ATTR_MTIME | ATTR_CTIME | ATTR_SIZE) == 0)
> 		valid = ATTR_SIZE;

You'll have to be careful, truncate will pass other things down like
mode to get rid of suid.

If you just wanted to ignore mtime changes on truncate, then masking
it off would be the way to go I think.

if (valid & ATTR_SIZE)
  valid &= ~ATTR_SIZE;

Would you also want to do the same thing with suid kill bits from
truncate, then? Mask off ATTR_MODE and just read it back from the
server too?

Re: [patch v2] fix truncate inode time modification breakage

[Miklos Szeredi]

On Thu, 3 Jun 2010, Nick Piggin wrote:
> You'll have to be careful, truncate will pass other things down like
> mode to get rid of suid.

Oh, right.

> If you just wanted to ignore mtime changes on truncate, then masking
> it off would be the way to go I think.
> 
> if (valid & ATTR_SIZE)
>   valid &= ~ATTR_SIZE;
> 
> Would you also want to do the same thing with suid kill bits from
> truncate, then? Mask off ATTR_MODE and just read it back from the
> server too?

That would be logical, but no, it didn't used to do that, and now it
can't start doing it for fear of creating a security hole.

Also suid removal happens very rarely compared to mtime update, so a
rare additional chmod request (which might be superfluous for some
filesystems) in addition to write and truncate is I think acceptable.

(In fact I think it would be cleanest if truncate/ftruncate was a
separate operation from setattr on all levels, but that's a different
story.)

So for now something like

	if (valid & ATTR_SIZE)
		valid &= ~(ATTR_MTIME | ATTR_CTIME);

would work?

Thanks,
Miklos

Re: [patch v2] fix truncate inode time modification breakage

[Christoph Hellwig]

On Thu, Jun 03, 2010 at 12:58:22PM +0200, Miklos Szeredi wrote:
> (In fact I think it would be cleanest if truncate/ftruncate was a
> separate operation from setattr on all levels, but that's a different
> story.)

It might be.  Let's finish off the current transition after which
we should have all truncate code in a clean

	if (valid & ATTR_SIZE)

inside the setattr method.  We can think about re-introducing a
proper truncate method after that, although I'm not sure how much
duplication the timestamp and mode updates will mean.  The upside
of it is that many filesystems could remove the setattr method
after this, though.

[patch v3] fix truncate inode time modification breakage

[Nick Piggin]

I think this should be the best way to fix 2.6.35. I'll look at what
it takes to completely direct truncate time/mode changes from the vfs
probably on top of Christophs latest truncate patchsets.

--

mtime and ctime should be changed only if the file size has actually
changed. Patches changing ext2 and tmpfs from vmtruncate to new truncate
sequence has caused regressions where they always update timestamps.

There is some strange cases in POSIX where truncate(2) must not update
times unless the size has acutally changed, see 6e656be89.

This area is all still rather buggy in different ways in a lot of
filesystems and needs a cleanup and audit (ideally the vfs will provide
a simple attribute or call to direct all filesystems exactly which
attributes to change). But coming up with the best solution will take a
while and is not appropriate for rc anyway.

So fix recent regression for now.

Signed-off-by: Nick Piggin <npiggin@suse.de>

---
 fs/ext2/inode.c |    2 +-
 mm/shmem.c      |    5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

Index: linux-2.6/mm/shmem.c
===================================================================
--- linux-2.6.orig/mm/shmem.c
+++ linux-2.6/mm/shmem.c
@@ -764,10 +764,11 @@ 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 (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)
+					&& newsize != inode->i_size) {
 		struct page *page = NULL;
 
 		if (newsize < inode->i_size) {
Index: linux-2.6/fs/ext2/inode.c
===================================================================
--- linux-2.6.orig/fs/ext2/inode.c
+++ linux-2.6/fs/ext2/inode.c
@@ -1552,7 +1552,7 @@ int ext2_setattr(struct dentry *dentry,
 		if (error)
 			return error;
 	}
-	if (iattr->ia_valid & ATTR_SIZE) {
+	if (iattr->ia_valid & ATTR_SIZE && iattr->ia_size != inode->i_size) {
 		error = ext2_setsize(inode, iattr->ia_size);
 		if (error)
 			return error;

Re: [patch v2] fix truncate inode time modification breakage

[Nick Piggin]

On Thu, Jun 03, 2010 at 12:58:22PM +0200, Miklos Szeredi wrote:
> On Thu, 3 Jun 2010, Nick Piggin wrote:
> > You'll have to be careful, truncate will pass other things down like
> > mode to get rid of suid.
> 
> Oh, right.
> 
> > If you just wanted to ignore mtime changes on truncate, then masking
> > it off would be the way to go I think.
> > 
> > if (valid & ATTR_SIZE)
> >   valid &= ~ATTR_SIZE;
> > 
> > Would you also want to do the same thing with suid kill bits from
> > truncate, then? Mask off ATTR_MODE and just read it back from the
> > server too?
> 
> That would be logical, but no, it didn't used to do that, and now it
> can't start doing it for fear of creating a security hole.

OK.

 
> Also suid removal happens very rarely compared to mtime update, so a
> rare additional chmod request (which might be superfluous for some
> filesystems) in addition to write and truncate is I think acceptable.

Yeah no big deal. One little thing to put on the list if you ever need
bump the protocol version.

 
> (In fact I think it would be cleanest if truncate/ftruncate was a
> separate operation from setattr on all levels, but that's a different
> story.)

Well it's possible. It is a combination of inode and address space
operation really. Because you really want to update mtime/ctime and
suid bits iff the truncate succeeds.

We did consider a new API for it, but it didn't seem to be an
obvious improvement. A filesystem can easily branch into another
function for ATTR_SIZE immediately on setattr entry.

 
> So for now something like
> 
> 	if (valid & ATTR_SIZE)
> 		valid &= ~(ATTR_MTIME | ATTR_CTIME);
> 
> would work?

That should do the trick, yes. But I think CTIME would just confuse
things seeing as you ignore it everywhere else.

Re: [patch v2] fix truncate inode time modification breakage

[Miklos Szeredi]

On Thu, 3 Jun 2010, Nick Piggin wrote:
> > (In fact I think it would be cleanest if truncate/ftruncate was a
> > separate operation from setattr on all levels, but that's a different
> > story.)
> 
> Well it's possible. It is a combination of inode and address space
> operation really. Because you really want to update mtime/ctime and
> suid bits iff the truncate succeeds.

Well, same as write() really.  Except truncate is an inode op, while
ftruncate is a file op just like write.

> We did consider a new API for it, but it didn't seem to be an
> obvious improvement. A filesystem can easily branch into another
> function for ATTR_SIZE immediately on setattr entry.
> 
>  
> > So for now something like
> > 
> > 	if (valid & ATTR_SIZE)
> > 		valid &= ~(ATTR_MTIME | ATTR_CTIME);
> > 
> > would work?
> 
> That should do the trick, yes. But I think CTIME would just confuse
> things seeing as you ignore it everywhere else.

Yes.

Thanks,
Miklos

Re: [patch] fix truncate inode time modification breakage

[Boaz Harrosh]

On 06/01/2010 04:39 PM, 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?
> 
> ---
>  fs/ext2/inode.c       |    1 -
>  fs/open.c             |    3 +++
>  fs/ramfs/file-nommu.c |    5 -----
>  mm/shmem.c            |    5 +++--
>  4 files changed, 6 insertions(+), 8 deletions(-)
> 
> 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;

OK I was just investigating this. Please forgive my noviceness here:

So i_mtime is modifications time. i_ctime is creation time?

if I do ftrunc() (like dd skip=x) don't I want modification time changed
but creation time unchanged?

if I do chmod/chown do I want modification-time changed creation not?

open+truncate both m an c changed?

modification-time: is it any aspect of the file has changed? or just the actual data / size?

Confused?

Boaz

>  	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,11 @@ 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 (S_ISREG(inode->i_mode) && (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;
>  		}
>  	}
>  
> 
> --
> 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

Re: [patch] fix truncate inode time modification breakage

[Nick Piggin]

On Tue, Jun 01, 2010 at 05:10:05PM +0300, Boaz Harrosh wrote:
> On 06/01/2010 04:39 PM, 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?
> > 
> > ---
> >  fs/ext2/inode.c       |    1 -
> >  fs/open.c             |    3 +++
> >  fs/ramfs/file-nommu.c |    5 -----
> >  mm/shmem.c            |    5 +++--
> >  4 files changed, 6 insertions(+), 8 deletions(-)
> > 
> > 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;
> 
> OK I was just investigating this. Please forgive my noviceness here:
> 
> So i_mtime is modifications time. i_ctime is creation time?
> 
> if I do ftrunc() (like dd skip=x) don't I want modification time changed
> but creation time unchanged?

It's change time and modification time. modification time is for data
modification. change time is for data and metadata as far as I know
(with various little historic quirks like truncate/ftruncate).