[xfs] c91c46c12: xfstests generic/313 regression

[xfs] c91c46c12: xfstests generic/313 regression

[fengguang.wu]

Hi Christoph,

We find this commit failed xfstests generic/313.
Attached is our kconfig.

c91c46c12768daac8486dff0f74bc52c2ec974cd is the first bad commit
commit c91c46c12768daac8486dff0f74bc52c2ec974cd
Author:     Christoph Hellwig <hch@infradead.org>
AuthorDate: Mon Nov 18 05:10:52 2013 -0800
Commit:     Ben Myers <bpm@sgi.com>
CommitDate: Fri Dec 6 17:26:19 2013 -0600

    xfs: add xfs_setattr_time
    
    Split out a xfs_setattr_time helper to share code between truncate and
    regular setattr similar to xfs_setattr_mode.  I might also have another
    caller growing for this in the near future.
    
    Signed-off-by: Christoph Hellwig <hch@lst.de>
    Reviewed-by: Brian Foster <bfoster@redhat.com>
    Signed-off-by: Ben Myers <bpm@sgi.com>

 fs/xfs/xfs_iops.c | 66 +++++++++++++++++++++++++------------------------------
 1 file changed, 30 insertions(+), 36 deletions(-)

Thanks,
Fengguang

[xfs] c91c46c12: xfstests generic/313 regression

[fengguang.wu]

Hi Christoph,

We find this commit failed xfstests generic/313.
Attached is our kconfig.

c91c46c12768daac8486dff0f74bc52c2ec974cd is the first bad commit
commit c91c46c12768daac8486dff0f74bc52c2ec974cd
Author:     Christoph Hellwig <hch@infradead.org>
AuthorDate: Mon Nov 18 05:10:52 2013 -0800
Commit:     Ben Myers <bpm@sgi.com>
CommitDate: Fri Dec 6 17:26:19 2013 -0600

    xfs: add xfs_setattr_time
    
    Split out a xfs_setattr_time helper to share code between truncate and
    regular setattr similar to xfs_setattr_mode.  I might also have another
    caller growing for this in the near future.
    
    Signed-off-by: Christoph Hellwig <hch@lst.de>
    Reviewed-by: Brian Foster <bfoster@redhat.com>
    Signed-off-by: Ben Myers <bpm@sgi.com>

 fs/xfs/xfs_iops.c | 66 +++++++++++++++++++++++++------------------------------
 1 file changed, 30 insertions(+), 36 deletions(-)

Thanks,
Fengguang

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [xfs] c91c46c12: xfstests generic/313 regression

[Jeff Liu]

Hi Fengguang,

Thanks for help catching up this.  I think the below patch can fix it
up, but maybe there would have a neater solution once Christoph is back.

Thanks,
-Jeff

From: Jie Liu <jeff.liu@oracle.com>
Subject: xfs: fix ctime and mtime update for truncate(2)

There is a semantic difference between truncate(2) and ftruncate(2) as
per VFS implementation, that is the truncate(2) is called without both
ATTR_CTIME and ATTR_MTIME flags in inode attributes (iattr->ia_valid),
and this is a special case where we need to update the times despite
not having these flags set.  However, this was broken by:

    Commit: c91c46c12768daac8486dff0f74bc52c2ec974cd
    xfs: add xfs_setattr_time

Since those flags are not set in iattr->ia_valid, and this problem can
be reproduced via xfstests/generic/313.

This patch fix it by adding a mask argument to xfs_setattr_time() as it
includes those flags in xfs_setattr_size() specially.

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
 fs/xfs/xfs_iops.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 0ce1d75..defd47e 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -477,23 +477,24 @@ xfs_setattr_mode(
 static void
 xfs_setattr_time(
 	struct xfs_inode	*ip,
-	struct iattr		*iattr)
+	struct iattr		*iattr,
+	int			mask)
 {
 	struct inode		*inode = VFS_I(ip);
 
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
-	if (iattr->ia_valid & ATTR_ATIME) {
+	if (mask & ATTR_ATIME) {
 		inode->i_atime = iattr->ia_atime;
 		ip->i_d.di_atime.t_sec = iattr->ia_atime.tv_sec;
 		ip->i_d.di_atime.t_nsec = iattr->ia_atime.tv_nsec;
 	}
-	if (iattr->ia_valid & ATTR_CTIME) {
+	if (mask & ATTR_CTIME) {
 		inode->i_ctime = iattr->ia_ctime;
 		ip->i_d.di_ctime.t_sec = iattr->ia_ctime.tv_sec;
 		ip->i_d.di_ctime.t_nsec = iattr->ia_ctime.tv_nsec;
 	}
-	if (iattr->ia_valid & ATTR_MTIME) {
+	if (mask & ATTR_MTIME) {
 		inode->i_mtime = iattr->ia_mtime;
 		ip->i_d.di_mtime.t_sec = iattr->ia_mtime.tv_sec;
 		ip->i_d.di_mtime.t_nsec = iattr->ia_mtime.tv_nsec;
@@ -657,7 +658,7 @@ xfs_setattr_nonsize(
 	if (mask & ATTR_MODE)
 		xfs_setattr_mode(ip, iattr);
 	if (mask & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
-		xfs_setattr_time(ip, iattr);
+		xfs_setattr_time(ip, iattr, mask);
 
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
@@ -875,7 +876,7 @@ xfs_setattr_size(
 	if (mask & ATTR_MODE)
 		xfs_setattr_mode(ip, iattr);
 	if (mask & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
-		xfs_setattr_time(ip, iattr);
+		xfs_setattr_time(ip, iattr, mask);
 
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
-- 
1.8.3.2

On 01/10 2014 20:27 PM, fengguang.wu@intel.com wrote:
> Hi Christoph,
> 
> We find this commit failed xfstests generic/313.
> Attached is our kconfig.
> 
> c91c46c12768daac8486dff0f74bc52c2ec974cd is the first bad commit
> commit c91c46c12768daac8486dff0f74bc52c2ec974cd
> Author:     Christoph Hellwig <hch@infradead.org>
> AuthorDate: Mon Nov 18 05:10:52 2013 -0800
> Commit:     Ben Myers <bpm@sgi.com>
> CommitDate: Fri Dec 6 17:26:19 2013 -0600
> 
>     xfs: add xfs_setattr_time
>     
>     Split out a xfs_setattr_time helper to share code between truncate and
>     regular setattr similar to xfs_setattr_mode.  I might also have another
>     caller growing for this in the near future.
>     
>     Signed-off-by: Christoph Hellwig <hch@lst.de>
>     Reviewed-by: Brian Foster <bfoster@redhat.com>
>     Signed-off-by: Ben Myers <bpm@sgi.com>
> 
>  fs/xfs/xfs_iops.c | 66 +++++++++++++++++++++++++------------------------------
>  1 file changed, 30 insertions(+), 36 deletions(-)
> 
> Thanks,
> Fengguang
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

Re: [xfs] c91c46c12: xfstests generic/313 regression

[Jeff Liu]

Hi Fengguang,

Thanks for help catching up this.  I think the below patch can fix it
up, but maybe there would have a neater solution once Christoph is back.

Thanks,
-Jeff

From: Jie Liu <jeff.liu@oracle.com>
Subject: xfs: fix ctime and mtime update for truncate(2)

There is a semantic difference between truncate(2) and ftruncate(2) as
per VFS implementation, that is the truncate(2) is called without both
ATTR_CTIME and ATTR_MTIME flags in inode attributes (iattr->ia_valid),
and this is a special case where we need to update the times despite
not having these flags set.  However, this was broken by:

    Commit: c91c46c12768daac8486dff0f74bc52c2ec974cd
    xfs: add xfs_setattr_time

Since those flags are not set in iattr->ia_valid, and this problem can
be reproduced via xfstests/generic/313.

This patch fix it by adding a mask argument to xfs_setattr_time() as it
includes those flags in xfs_setattr_size() specially.

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
 fs/xfs/xfs_iops.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 0ce1d75..defd47e 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -477,23 +477,24 @@ xfs_setattr_mode(
 static void
 xfs_setattr_time(
 	struct xfs_inode	*ip,
-	struct iattr		*iattr)
+	struct iattr		*iattr,
+	int			mask)
 {
 	struct inode		*inode = VFS_I(ip);
 
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
-	if (iattr->ia_valid & ATTR_ATIME) {
+	if (mask & ATTR_ATIME) {
 		inode->i_atime = iattr->ia_atime;
 		ip->i_d.di_atime.t_sec = iattr->ia_atime.tv_sec;
 		ip->i_d.di_atime.t_nsec = iattr->ia_atime.tv_nsec;
 	}
-	if (iattr->ia_valid & ATTR_CTIME) {
+	if (mask & ATTR_CTIME) {
 		inode->i_ctime = iattr->ia_ctime;
 		ip->i_d.di_ctime.t_sec = iattr->ia_ctime.tv_sec;
 		ip->i_d.di_ctime.t_nsec = iattr->ia_ctime.tv_nsec;
 	}
-	if (iattr->ia_valid & ATTR_MTIME) {
+	if (mask & ATTR_MTIME) {
 		inode->i_mtime = iattr->ia_mtime;
 		ip->i_d.di_mtime.t_sec = iattr->ia_mtime.tv_sec;
 		ip->i_d.di_mtime.t_nsec = iattr->ia_mtime.tv_nsec;
@@ -657,7 +658,7 @@ xfs_setattr_nonsize(
 	if (mask & ATTR_MODE)
 		xfs_setattr_mode(ip, iattr);
 	if (mask & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
-		xfs_setattr_time(ip, iattr);
+		xfs_setattr_time(ip, iattr, mask);
 
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
@@ -875,7 +876,7 @@ xfs_setattr_size(
 	if (mask & ATTR_MODE)
 		xfs_setattr_mode(ip, iattr);
 	if (mask & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
-		xfs_setattr_time(ip, iattr);
+		xfs_setattr_time(ip, iattr, mask);
 
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
-- 
1.8.3.2

On 01/10 2014 20:27 PM, fengguang.wu@intel.com wrote:
> Hi Christoph,
> 
> We find this commit failed xfstests generic/313.
> Attached is our kconfig.
> 
> c91c46c12768daac8486dff0f74bc52c2ec974cd is the first bad commit
> commit c91c46c12768daac8486dff0f74bc52c2ec974cd
> Author:     Christoph Hellwig <hch@infradead.org>
> AuthorDate: Mon Nov 18 05:10:52 2013 -0800
> Commit:     Ben Myers <bpm@sgi.com>
> CommitDate: Fri Dec 6 17:26:19 2013 -0600
> 
>     xfs: add xfs_setattr_time
>     
>     Split out a xfs_setattr_time helper to share code between truncate and
>     regular setattr similar to xfs_setattr_mode.  I might also have another
>     caller growing for this in the near future.
>     
>     Signed-off-by: Christoph Hellwig <hch@lst.de>
>     Reviewed-by: Brian Foster <bfoster@redhat.com>
>     Signed-off-by: Ben Myers <bpm@sgi.com>
> 
>  fs/xfs/xfs_iops.c | 66 +++++++++++++++++++++++++------------------------------
>  1 file changed, 30 insertions(+), 36 deletions(-)
> 
> Thanks,
> Fengguang
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [xfs] c91c46c12: xfstests generic/313 regression

[Christoph Hellwig]

On Fri, Jan 10, 2014 at 09:22:10PM +0800, Jeff Liu wrote:
> Hi Fengguang,
> 
> Thanks for help catching up this.  I think the below patch can fix it
> up, but maybe there would have a neater solution once Christoph is back.

I'd just remove the mask variable that caused the problem.  Untested
patch below:

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 0ce1d75..ce966c5 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -714,7 +714,6 @@ xfs_setattr_size(
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct inode		*inode = VFS_I(ip);
-	int			mask = iattr->ia_valid;
 	xfs_off_t		oldsize, newsize;
 	struct xfs_trans	*tp;
 	int			error;
@@ -735,8 +734,8 @@ xfs_setattr_size(
 
 	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
 	ASSERT(S_ISREG(ip->i_d.di_mode));
-	ASSERT((mask & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
-			ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
+	ASSERT((iattr->ia_valid & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
+		ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
 
 	oldsize = inode->i_size;
 	newsize = iattr->ia_size;
@@ -745,7 +744,7 @@ xfs_setattr_size(
 	 * Short circuit the truncate case for zero length files.
 	 */
 	if (newsize == 0 && oldsize == 0 && ip->i_d.di_nextents == 0) {
-		if (!(mask & (ATTR_CTIME|ATTR_MTIME)))
+		if (!(iattr->ia_valid & (ATTR_CTIME|ATTR_MTIME)))
 			return 0;
 
 		/*
@@ -833,10 +832,11 @@ xfs_setattr_size(
 	 * these flags set.  For all other operations the VFS set these flags
 	 * explicitly if it wants a timestamp update.
 	 */
-	if (newsize != oldsize && (!(mask & (ATTR_CTIME | ATTR_MTIME)))) {
+	if (newsize != oldsize &&
+	    !(iattr->ia_valid & (ATTR_CTIME | ATTR_MTIME))) {
 		iattr->ia_ctime = iattr->ia_mtime =
 			current_fs_time(inode->i_sb);
-		mask |= ATTR_CTIME | ATTR_MTIME;
+		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
 	}
 
 	/*
@@ -872,9 +872,9 @@ xfs_setattr_size(
 		xfs_inode_clear_eofblocks_tag(ip);
 	}
 
-	if (mask & ATTR_MODE)
+	if (iattr->ia_valid & ATTR_MODE)
 		xfs_setattr_mode(ip, iattr);
-	if (mask & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
+	if (iattr->ia_valid & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
 		xfs_setattr_time(ip, iattr);
 
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

Re: [xfs] c91c46c12: xfstests generic/313 regression

[Christoph Hellwig]

On Fri, Jan 10, 2014 at 09:22:10PM +0800, Jeff Liu wrote:
> Hi Fengguang,
> 
> Thanks for help catching up this.  I think the below patch can fix it
> up, but maybe there would have a neater solution once Christoph is back.

I'd just remove the mask variable that caused the problem.  Untested
patch below:

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 0ce1d75..ce966c5 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -714,7 +714,6 @@ xfs_setattr_size(
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct inode		*inode = VFS_I(ip);
-	int			mask = iattr->ia_valid;
 	xfs_off_t		oldsize, newsize;
 	struct xfs_trans	*tp;
 	int			error;
@@ -735,8 +734,8 @@ xfs_setattr_size(
 
 	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
 	ASSERT(S_ISREG(ip->i_d.di_mode));
-	ASSERT((mask & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
-			ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
+	ASSERT((iattr->ia_valid & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
+		ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
 
 	oldsize = inode->i_size;
 	newsize = iattr->ia_size;
@@ -745,7 +744,7 @@ xfs_setattr_size(
 	 * Short circuit the truncate case for zero length files.
 	 */
 	if (newsize == 0 && oldsize == 0 && ip->i_d.di_nextents == 0) {
-		if (!(mask & (ATTR_CTIME|ATTR_MTIME)))
+		if (!(iattr->ia_valid & (ATTR_CTIME|ATTR_MTIME)))
 			return 0;
 
 		/*
@@ -833,10 +832,11 @@ xfs_setattr_size(
 	 * these flags set.  For all other operations the VFS set these flags
 	 * explicitly if it wants a timestamp update.
 	 */
-	if (newsize != oldsize && (!(mask & (ATTR_CTIME | ATTR_MTIME)))) {
+	if (newsize != oldsize &&
+	    !(iattr->ia_valid & (ATTR_CTIME | ATTR_MTIME))) {
 		iattr->ia_ctime = iattr->ia_mtime =
 			current_fs_time(inode->i_sb);
-		mask |= ATTR_CTIME | ATTR_MTIME;
+		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
 	}
 
 	/*
@@ -872,9 +872,9 @@ xfs_setattr_size(
 		xfs_inode_clear_eofblocks_tag(ip);
 	}
 
-	if (mask & ATTR_MODE)
+	if (iattr->ia_valid & ATTR_MODE)
 		xfs_setattr_mode(ip, iattr);
-	if (mask & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
+	if (iattr->ia_valid & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
 		xfs_setattr_time(ip, iattr);
 
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [xfs] c91c46c12: xfstests generic/313 regression

[Jeff Liu]

On 01/10 2014 21:33 PM, Christoph Hellwig wrote:
> On Fri, Jan 10, 2014 at 09:22:10PM +0800, Jeff Liu wrote:
>> Hi Fengguang,
>>
>> Thanks for help catching up this.  I think the below patch can fix it
>> up, but maybe there would have a neater solution once Christoph is back.
> 
> I'd just remove the mask variable that caused the problem.  Untested
> patch below:
I also thought to fix this problem in this way, however I'm not sure
if those flags can be set back to iattr->ia_valid internally...

Otherwise, this fix looks good to me.

Reviewed-by: Jie Liu <jeff.liu@oracle.com>

Thanks,
-Jeff
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 0ce1d75..ce966c5 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -714,7 +714,6 @@ xfs_setattr_size(
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct inode		*inode = VFS_I(ip);
> -	int			mask = iattr->ia_valid;
>  	xfs_off_t		oldsize, newsize;
>  	struct xfs_trans	*tp;
>  	int			error;
> @@ -735,8 +734,8 @@ xfs_setattr_size(
>  
>  	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
>  	ASSERT(S_ISREG(ip->i_d.di_mode));
> -	ASSERT((mask & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
> -			ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
> +	ASSERT((iattr->ia_valid & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
> +		ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
>  
>  	oldsize = inode->i_size;
>  	newsize = iattr->ia_size;
> @@ -745,7 +744,7 @@ xfs_setattr_size(
>  	 * Short circuit the truncate case for zero length files.
>  	 */
>  	if (newsize == 0 && oldsize == 0 && ip->i_d.di_nextents == 0) {
> -		if (!(mask & (ATTR_CTIME|ATTR_MTIME)))
> +		if (!(iattr->ia_valid & (ATTR_CTIME|ATTR_MTIME)))
>  			return 0;
>  
>  		/*
> @@ -833,10 +832,11 @@ xfs_setattr_size(
>  	 * these flags set.  For all other operations the VFS set these flags
>  	 * explicitly if it wants a timestamp update.
>  	 */
> -	if (newsize != oldsize && (!(mask & (ATTR_CTIME | ATTR_MTIME)))) {
> +	if (newsize != oldsize &&
> +	    !(iattr->ia_valid & (ATTR_CTIME | ATTR_MTIME))) {
>  		iattr->ia_ctime = iattr->ia_mtime =
>  			current_fs_time(inode->i_sb);
> -		mask |= ATTR_CTIME | ATTR_MTIME;
> +		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
>  	}
>  
>  	/*
> @@ -872,9 +872,9 @@ xfs_setattr_size(
>  		xfs_inode_clear_eofblocks_tag(ip);
>  	}
>  
> -	if (mask & ATTR_MODE)
> +	if (iattr->ia_valid & ATTR_MODE)
>  		xfs_setattr_mode(ip, iattr);
> -	if (mask & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
> +	if (iattr->ia_valid & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
>  		xfs_setattr_time(ip, iattr);
>  
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

Re: [xfs] c91c46c12: xfstests generic/313 regression

[Jeff Liu]

On 01/10 2014 21:33 PM, Christoph Hellwig wrote:
> On Fri, Jan 10, 2014 at 09:22:10PM +0800, Jeff Liu wrote:
>> Hi Fengguang,
>>
>> Thanks for help catching up this.  I think the below patch can fix it
>> up, but maybe there would have a neater solution once Christoph is back.
> 
> I'd just remove the mask variable that caused the problem.  Untested
> patch below:
I also thought to fix this problem in this way, however I'm not sure
if those flags can be set back to iattr->ia_valid internally...

Otherwise, this fix looks good to me.

Reviewed-by: Jie Liu <jeff.liu@oracle.com>

Thanks,
-Jeff
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 0ce1d75..ce966c5 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -714,7 +714,6 @@ xfs_setattr_size(
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct inode		*inode = VFS_I(ip);
> -	int			mask = iattr->ia_valid;
>  	xfs_off_t		oldsize, newsize;
>  	struct xfs_trans	*tp;
>  	int			error;
> @@ -735,8 +734,8 @@ xfs_setattr_size(
>  
>  	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
>  	ASSERT(S_ISREG(ip->i_d.di_mode));
> -	ASSERT((mask & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
> -			ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
> +	ASSERT((iattr->ia_valid & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
> +		ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
>  
>  	oldsize = inode->i_size;
>  	newsize = iattr->ia_size;
> @@ -745,7 +744,7 @@ xfs_setattr_size(
>  	 * Short circuit the truncate case for zero length files.
>  	 */
>  	if (newsize == 0 && oldsize == 0 && ip->i_d.di_nextents == 0) {
> -		if (!(mask & (ATTR_CTIME|ATTR_MTIME)))
> +		if (!(iattr->ia_valid & (ATTR_CTIME|ATTR_MTIME)))
>  			return 0;
>  
>  		/*
> @@ -833,10 +832,11 @@ xfs_setattr_size(
>  	 * these flags set.  For all other operations the VFS set these flags
>  	 * explicitly if it wants a timestamp update.
>  	 */
> -	if (newsize != oldsize && (!(mask & (ATTR_CTIME | ATTR_MTIME)))) {
> +	if (newsize != oldsize &&
> +	    !(iattr->ia_valid & (ATTR_CTIME | ATTR_MTIME))) {
>  		iattr->ia_ctime = iattr->ia_mtime =
>  			current_fs_time(inode->i_sb);
> -		mask |= ATTR_CTIME | ATTR_MTIME;
> +		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
>  	}
>  
>  	/*
> @@ -872,9 +872,9 @@ xfs_setattr_size(
>  		xfs_inode_clear_eofblocks_tag(ip);
>  	}
>  
> -	if (mask & ATTR_MODE)
> +	if (iattr->ia_valid & ATTR_MODE)
>  		xfs_setattr_mode(ip, iattr);
> -	if (mask & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
> +	if (iattr->ia_valid & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
>  		xfs_setattr_time(ip, iattr);
>  
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [xfs] c91c46c12: xfstests generic/313 regression

[Christoph Hellwig]

On Fri, Jan 10, 2014 at 10:01:00PM +0800, Jeff Liu wrote:
> I also thought to fix this problem in this way, however I'm not sure
> if those flags can be set back to iattr->ia_valid internally...
> 
> Otherwise, this fix looks good to me.

Nothing in the truncate or open code path (or non-size setattr for that
matter) looks at ia_valid after calling the filesystem, and they really
have no business to.

In the meantime this has passed xfstests, so I'll send it along.  Thanks
for the first fix, btw!

Re: [xfs] c91c46c12: xfstests generic/313 regression

[Christoph Hellwig]

On Fri, Jan 10, 2014 at 10:01:00PM +0800, Jeff Liu wrote:
> I also thought to fix this problem in this way, however I'm not sure
> if those flags can be set back to iattr->ia_valid internally...
> 
> Otherwise, this fix looks good to me.

Nothing in the truncate or open code path (or non-size setattr for that
matter) looks at ia_valid after calling the filesystem, and they really
have no business to.

In the meantime this has passed xfstests, so I'll send it along.  Thanks
for the first fix, btw!

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [xfs] c91c46c12: xfstests generic/313 regression

[Brian Foster]

On 01/11/2014 06:10 AM, Christoph Hellwig wrote:
> On Fri, Jan 10, 2014 at 10:01:00PM +0800, Jeff Liu wrote:
>> I also thought to fix this problem in this way, however I'm not sure
>> if those flags can be set back to iattr->ia_valid internally...
>>
>> Otherwise, this fix looks good to me.
> 
> Nothing in the truncate or open code path (or non-size setattr for that
> matter) looks at ia_valid after calling the filesystem, and they really
> have no business to.
> 
> In the meantime this has passed xfstests, so I'll send it along.  Thanks
> for the first fix, btw!
> 

ping... any plans to repost this with a proper commit log and whatnot?
Thanks.

Brian

> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [xfs] c91c46c12: xfstests generic/313 regression

[Christoph Hellwig]

On Wed, Jan 29, 2014 at 08:13:37AM -0500, Brian Foster wrote:
> ping... any plans to repost this with a proper commit log and whatnot?
> Thanks.

Sorry, got sidetracked.  I'll try to get it out ASAP.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH] xfs: ensure correct timestamp updates from truncate

[Christoph Hellwig]

The VFS doesn't set the proper ATTR_CTIME and ATTR_MTIME values for truncate,
so filesystems have to manually add them.  The introduction of
xfs_setattr_time accidentally broke this special case an caused a
regression in generic/313.  Fix this by removing the local mask variable
in xfs_setattr_size so that we only have a single place to keep the
attribute information.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Fengguang Wu <fengguang.wu@intel.com>

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 0ce1d75..ce966c5 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -714,7 +714,6 @@ xfs_setattr_size(
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct inode		*inode = VFS_I(ip);
-	int			mask = iattr->ia_valid;
 	xfs_off_t		oldsize, newsize;
 	struct xfs_trans	*tp;
 	int			error;
@@ -735,8 +734,8 @@ xfs_setattr_size(
 
 	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
 	ASSERT(S_ISREG(ip->i_d.di_mode));
-	ASSERT((mask & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
-			ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
+	ASSERT((iattr->ia_valid & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
+		ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
 
 	oldsize = inode->i_size;
 	newsize = iattr->ia_size;
@@ -745,7 +744,7 @@ xfs_setattr_size(
 	 * Short circuit the truncate case for zero length files.
 	 */
 	if (newsize == 0 && oldsize == 0 && ip->i_d.di_nextents == 0) {
-		if (!(mask & (ATTR_CTIME|ATTR_MTIME)))
+		if (!(iattr->ia_valid & (ATTR_CTIME|ATTR_MTIME)))
 			return 0;
 
 		/*
@@ -833,10 +832,11 @@ xfs_setattr_size(
 	 * these flags set.  For all other operations the VFS set these flags
 	 * explicitly if it wants a timestamp update.
 	 */
-	if (newsize != oldsize && (!(mask & (ATTR_CTIME | ATTR_MTIME)))) {
+	if (newsize != oldsize &&
+	    !(iattr->ia_valid & (ATTR_CTIME | ATTR_MTIME))) {
 		iattr->ia_ctime = iattr->ia_mtime =
 			current_fs_time(inode->i_sb);
-		mask |= ATTR_CTIME | ATTR_MTIME;
+		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
 	}
 
 	/*
@@ -872,9 +872,9 @@ xfs_setattr_size(
 		xfs_inode_clear_eofblocks_tag(ip);
 	}
 
-	if (mask & ATTR_MODE)
+	if (iattr->ia_valid & ATTR_MODE)
 		xfs_setattr_mode(ip, iattr);
-	if (mask & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
+	if (iattr->ia_valid & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
 		xfs_setattr_time(ip, iattr);
 
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: ensure correct timestamp updates from truncate

[Jeff Liu]

On 01/30 2014 16:06 PM, Christoph Hellwig wrote:
> The VFS doesn't set the proper ATTR_CTIME and ATTR_MTIME values for truncate,
> so filesystems have to manually add them.  The introduction of
> xfs_setattr_time accidentally broke this special case an caused a
> regression in generic/313.  Fix this by removing the local mask variable
> in xfs_setattr_size so that we only have a single place to keep the
> attribute information.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>

Reviewed-by: Jie Liu <jeff.liu@oracle.com>

Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: ensure correct timestamp updates from truncate

[Brian Foster]

On 01/30/2014 03:06 AM, Christoph Hellwig wrote:
> The VFS doesn't set the proper ATTR_CTIME and ATTR_MTIME values for truncate,
> so filesystems have to manually add them.  The introduction of
> xfs_setattr_time accidentally broke this special case an caused a
> regression in generic/313.  Fix this by removing the local mask variable
> in xfs_setattr_size so that we only have a single place to keep the
> attribute information.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> 

Thanks Christoph!

Reviewed-by: Brian Foster <bfoster@redhat.com>

> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 0ce1d75..ce966c5 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -714,7 +714,6 @@ xfs_setattr_size(
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct inode		*inode = VFS_I(ip);
> -	int			mask = iattr->ia_valid;
>  	xfs_off_t		oldsize, newsize;
>  	struct xfs_trans	*tp;
>  	int			error;
> @@ -735,8 +734,8 @@ xfs_setattr_size(
>  
>  	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
>  	ASSERT(S_ISREG(ip->i_d.di_mode));
> -	ASSERT((mask & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
> -			ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
> +	ASSERT((iattr->ia_valid & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
> +		ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
>  
>  	oldsize = inode->i_size;
>  	newsize = iattr->ia_size;
> @@ -745,7 +744,7 @@ xfs_setattr_size(
>  	 * Short circuit the truncate case for zero length files.
>  	 */
>  	if (newsize == 0 && oldsize == 0 && ip->i_d.di_nextents == 0) {
> -		if (!(mask & (ATTR_CTIME|ATTR_MTIME)))
> +		if (!(iattr->ia_valid & (ATTR_CTIME|ATTR_MTIME)))
>  			return 0;
>  
>  		/*
> @@ -833,10 +832,11 @@ xfs_setattr_size(
>  	 * these flags set.  For all other operations the VFS set these flags
>  	 * explicitly if it wants a timestamp update.
>  	 */
> -	if (newsize != oldsize && (!(mask & (ATTR_CTIME | ATTR_MTIME)))) {
> +	if (newsize != oldsize &&
> +	    !(iattr->ia_valid & (ATTR_CTIME | ATTR_MTIME))) {
>  		iattr->ia_ctime = iattr->ia_mtime =
>  			current_fs_time(inode->i_sb);
> -		mask |= ATTR_CTIME | ATTR_MTIME;
> +		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
>  	}
>  
>  	/*
> @@ -872,9 +872,9 @@ xfs_setattr_size(
>  		xfs_inode_clear_eofblocks_tag(ip);
>  	}
>  
> -	if (mask & ATTR_MODE)
> +	if (iattr->ia_valid & ATTR_MODE)
>  		xfs_setattr_mode(ip, iattr);
> -	if (mask & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
> +	if (iattr->ia_valid & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
>  		xfs_setattr_time(ip, iattr);
>  
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: ensure correct timestamp updates from truncate

[Ben Myers]

On Thu, Jan 30, 2014 at 12:06:52AM -0800, Christoph Hellwig wrote:
> The VFS doesn't set the proper ATTR_CTIME and ATTR_MTIME values for truncate,
> so filesystems have to manually add them.  The introduction of
> xfs_setattr_time accidentally broke this special case an caused a
> regression in generic/313.  Fix this by removing the local mask variable
> in xfs_setattr_size so that we only have a single place to keep the
> attribute information.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>

That is v3.13-rc2-14-gc91c46c, so I believe we'll want to Cc: stable on this
one.  Correct?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: ensure correct timestamp updates from truncate

[Christoph Hellwig]

On Thu, Jan 30, 2014 at 09:47:01AM -0600, Ben Myers wrote:
> That is v3.13-rc2-14-gc91c46c, so I believe we'll want to Cc: stable on this
> one.  Correct?

Seems so.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: ensure correct timestamp updates from truncate

[Christoph Hellwig]

On Mon, Feb 03, 2014 at 02:23:23AM -0800, Christoph Hellwig wrote:
> On Thu, Jan 30, 2014 at 09:47:01AM -0600, Ben Myers wrote:
> > That is v3.13-rc2-14-gc91c46c, so I believe we'll want to Cc: stable on this
> > one.  Correct?

We'll need to get it to Linus for 3.14 first, though.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: ensure correct timestamp updates from truncate

[Christoph Hellwig]

On Fri, Feb 07, 2014 at 11:11:00PM -0800, Christoph Hellwig wrote:
> On Mon, Feb 03, 2014 at 02:23:23AM -0800, Christoph Hellwig wrote:
> > On Thu, Jan 30, 2014 at 09:47:01AM -0600, Ben Myers wrote:
> > > That is v3.13-rc2-14-gc91c46c, so I believe we'll want to Cc: stable on this
> > > one.  Correct?
> 
> We'll need to get it to Linus for 3.14 first, though.

Any chance to get it out any time soon? Same for the logvec alignment
fix from Dave.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs