Skip to contentSkip to conversation

c91c46c12: xfstests generic/313 regression

xfs In review [email protected] 5 participants 14 messages Jan 10, 2014 – Feb 16, 2014 XFS filesystem

lore
Share & tools
lore

[email protected]

author Reviewed-by Brian Foster
#0 lore raw

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 <[email protected]>
AuthorDate: Mon Nov 18 05:10:52 2013 -0800
Commit:     Ben Myers <[email protected]>
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 <[email protected]>
Reviewed-by Brian Foster <[email protected]>
Signed-off-by Ben Myers <[email protected]>
 fs/xfs/xfs_iops.c | 66 +++++++++++++++++++++++++------------------------------
 1 file changed, 30 insertions(+), 36 deletions(-)

Thanks,
Fengguang

#1 lore raw

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 <[email protected]>
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 <[email protected]>
Signed-off-by Jie Liu <[email protected]>
---
 fs/xfs/xfs_iops.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
fs/xfs/xfs_iops.c +7 -6
index 0ce1d75..defd47e 100644
@@ -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);
Signature · 1 line
1.8.3.2
Quoted 25 lines · On 01/10 2014 20:27 PM, [email protected] 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 <[email protected]>
AuthorDate: Mon Nov 18 05:10:52 2013 -0800
Commit:     Ben Myers <[email protected]>
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 <[email protected]>
Reviewed-by Brian Foster <[email protected]>
Signed-off-by Ben Myers <[email protected]>
 fs/xfs/xfs_iops.c | 66 +++++++++++++++++++++++++------------------------------
 1 file changed, 30 insertions(+), 36 deletions(-)
Quoted 8 lines

Thanks,
Fengguang

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#2 lore raw
Quoted 4 lines · 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.

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

fs/xfs/xfs_iops.c +8 -8
index 0ce1d75..ce966c5 100644
@@ -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);
Reviewed-by
#3 lore raw
Quoted 8 lines · Christoph Hellwig
Quoted 4 lines · 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.

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 <[email protected]>

Thanks,
-Jeff

Quoted 65 lines
fs/xfs/xfs_iops.c +8 -8
index 0ce1d75..ce966c5 100644
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#4 lore raw
Quoted 4 lines · Jeff Liu

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!

#5 lore raw
Quoted 4 lines · Christoph Hellwig
Quoted 3 lines · Jeff Liu

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

Quoted 9 lines
Quoted 1 line

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

Quoted 5 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#7 lore raw

PATCH 1/1 xfs: ensure correct timestamp updates from truncate

In reply to Brian Foster

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 <[email protected]>
Reported-by Fengguang Wu <[email protected]>
fs/xfs/xfs_iops.c +8 -8
index 0ce1d75..ce966c5 100644
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Reviewed-by on v1 1/1
#8 lore raw

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

Quoted 9 lines · 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 <[email protected]>
Reported-by Fengguang Wu <[email protected]>
Reviewed-by Jie Liu <[email protected]>

Thanks,
-Jeff

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Reviewed-by on v1 1/1
#9 lore raw

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

In reply to Christoph Hellwig
Quoted 10 lines · 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 <[email protected]>
Reported-by Fengguang Wu <[email protected]>

Thanks Christoph!

Reviewed-by Brian Foster <[email protected]>
Quoted 59 lines
fs/xfs/xfs_iops.c +8 -8
index 0ce1d75..ce966c5 100644
@@ -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);
Quoted 5 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#10 lore raw

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

In reply to Christoph Hellwig
Quoted 9 lines · 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 <[email protected]>
Reported-by Fengguang Wu <[email protected]>

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

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#12 lore raw

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

Quoted 3 lines · Christoph Hellwig
Quoted 2 lines · Ben Myers

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#13 lore raw

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

Quoted 6 lines · Christoph Hellwig
Quoted 3 lines · Christoph Hellwig
Quoted 2 lines · Ben Myers

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs