[patch] Don't change ctime in truncate is size does not change

[patch] Don't change ctime in truncate is size does not change

[David Chinner]

XFS changes the c/mtime of an inode when truncating it to the same
size. The c/mtime is only supposed to change if the size is changed.
Not to be confused with ftruncate, where the c/mtime is supposed to
be changed even if the size is not changed.

The Linux VFS encodes this semantic difference in the flags it sends
down to ->setattr, which XFS currently ignores. We need to make XFS
pay attention to the VFS flags and hence Do The Right Thing.

Signed-off-by: Dave Chinner <dgc@sgi.com>
---
 fs/xfs/xfs_vnodeops.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Index: 2.6.x-xfs-new/fs/xfs/xfs_vnodeops.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_vnodeops.c	2008-01-25 16:03:23.654971763 +1100
+++ 2.6.x-xfs-new/fs/xfs/xfs_vnodeops.c	2008-01-25 16:10:30.875397129 +1100
@@ -634,6 +634,15 @@ xfs_setattr(
 	 * Truncate file.  Must have write permission and not be a directory.
 	 */
 	if (mask & XFS_AT_SIZE) {
+		/*
+		 * 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.
+		 */
+		if (vap->va_size != ip->i_size || mask & XFS_AT_CTIME)
+			timeflags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
+
 		if (vap->va_size > ip->i_size) {
 			xfs_igrow_finish(tp, ip, vap->va_size,
 			    !(flags & ATTR_DMI));
@@ -662,10 +671,6 @@ xfs_setattr(
 			 */
 			xfs_iflags_set(ip, XFS_ITRUNCATED);
 		}
-		/*
-		 * Have to do this even if the file's size doesn't change.
-		 */
-		timeflags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
 	}
 
 	/*

Re: [patch] Don't change ctime in truncate is size does not change

[Mark Goodwin]

Did we ever discover what was behind the comment:
"Have to do this even if the file's size doesn't change"

Cheers
-- Mark

David Chinner wrote:
> XFS changes the c/mtime of an inode when truncating it to the same
> size. The c/mtime is only supposed to change if the size is changed.
> Not to be confused with ftruncate, where the c/mtime is supposed to
> be changed even if the size is not changed.
> 
> The Linux VFS encodes this semantic difference in the flags it sends
> down to ->setattr, which XFS currently ignores. We need to make XFS
> pay attention to the VFS flags and hence Do The Right Thing.
> 
> Signed-off-by: Dave Chinner <dgc@sgi.com>
> ---
>  fs/xfs/xfs_vnodeops.c |   13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> Index: 2.6.x-xfs-new/fs/xfs/xfs_vnodeops.c
> ===================================================================
> --- 2.6.x-xfs-new.orig/fs/xfs/xfs_vnodeops.c	2008-01-25 16:03:23.654971763 +1100
> +++ 2.6.x-xfs-new/fs/xfs/xfs_vnodeops.c	2008-01-25 16:10:30.875397129 +1100
> @@ -634,6 +634,15 @@ xfs_setattr(
>  	 * Truncate file.  Must have write permission and not be a directory.
>  	 */
>  	if (mask & XFS_AT_SIZE) {
> +		/*
> +		 * 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.
> +		 */
> +		if (vap->va_size != ip->i_size || mask & XFS_AT_CTIME)
> +			timeflags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
> +
>  		if (vap->va_size > ip->i_size) {
>  			xfs_igrow_finish(tp, ip, vap->va_size,
>  			    !(flags & ATTR_DMI));
> @@ -662,10 +671,6 @@ xfs_setattr(
>  			 */
>  			xfs_iflags_set(ip, XFS_ITRUNCATED);
>  		}
> -		/*
> -		 * Have to do this even if the file's size doesn't change.
> -		 */
> -		timeflags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
>  	}
>  
>  	/*

-- 

  Mark Goodwin                                  markgw@sgi.com
  Engineering Manager for XFS and PCP    Phone: +61-3-99631937
  SGI Australian Software Group           Cell: +61-4-18969583
-------------------------------------------------------------

Re: [patch] Don't change ctime in truncate is size does not change

[David Chinner]

On Tue, Feb 19, 2008 at 10:37:48AM +1100, Mark Goodwin wrote:
> Did we ever discover what was behind the comment:
> "Have to do this even if the file's size doesn't change"

It was put there when the patch was checked in - I can only
assume that it was done this way because the old test script
that was being run didn't discriminate between the truncate
and truncate differences.

Basically, both the code and the comment are wrong and needed
to be fixed....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [patch] Don't change ctime in truncate is size does not change

[Christoph Hellwig]

> +		if (vap->va_size != ip->i_size || mask & XFS_AT_CTIME)

Normal Linux style is to always have braces around the 'foo & bar'
statements to avoid operator precedance problems when re-ordering the
statements.  Otherwise this one looks good.

Re: [patch] Don't change ctime in truncate is size does not change

[David Chinner]

On Wed, Feb 20, 2008 at 06:26:15PM -0500, Christoph Hellwig wrote:
> > +		if (vap->va_size != ip->i_size || mask & XFS_AT_CTIME)
> 
> Normal Linux style is to always have braces around the 'foo & bar'
> statements to avoid operator precedance problems when re-ordering the
> statements.  Otherwise this one looks good.

Fixed up. Thank's for the review.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group