* [xfs] c91c46c12: xfstests generic/313 regression
@ 2014-01-10 12:27 fengguang.wu
2014-01-10 13:22 ` Jeff Liu
0 siblings, 1 reply; 5+ messages in thread
From: fengguang.wu @ 2014-01-10 12:27 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: LKML, xfs@oss.sgi.com
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [xfs] c91c46c12: xfstests generic/313 regression 2014-01-10 12:27 [xfs] c91c46c12: xfstests generic/313 regression fengguang.wu @ 2014-01-10 13:22 ` Jeff Liu 2014-01-10 13:33 ` Christoph Hellwig 0 siblings, 1 reply; 5+ messages in thread From: Jeff Liu @ 2014-01-10 13:22 UTC (permalink / raw) To: fengguang.wu, Christoph Hellwig; +Cc: LKML, xfs@oss.sgi.com 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 > ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [xfs] c91c46c12: xfstests generic/313 regression 2014-01-10 13:22 ` Jeff Liu @ 2014-01-10 13:33 ` Christoph Hellwig 2014-01-10 14:01 ` Jeff Liu 0 siblings, 1 reply; 5+ messages in thread From: Christoph Hellwig @ 2014-01-10 13:33 UTC (permalink / raw) To: Jeff Liu; +Cc: fengguang.wu, Christoph Hellwig, LKML, xfs@oss.sgi.com 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); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [xfs] c91c46c12: xfstests generic/313 regression 2014-01-10 13:33 ` Christoph Hellwig @ 2014-01-10 14:01 ` Jeff Liu 2014-01-11 11:10 ` Christoph Hellwig 0 siblings, 1 reply; 5+ messages in thread From: Jeff Liu @ 2014-01-10 14:01 UTC (permalink / raw) To: Christoph Hellwig; +Cc: fengguang.wu, LKML, xfs@oss.sgi.com 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [xfs] c91c46c12: xfstests generic/313 regression 2014-01-10 14:01 ` Jeff Liu @ 2014-01-11 11:10 ` Christoph Hellwig 0 siblings, 0 replies; 5+ messages in thread From: Christoph Hellwig @ 2014-01-11 11:10 UTC (permalink / raw) To: Jeff Liu; +Cc: Christoph Hellwig, fengguang.wu, LKML, xfs@oss.sgi.com 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! ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-01-11 11:10 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-10 12:27 [xfs] c91c46c12: xfstests generic/313 regression fengguang.wu 2014-01-10 13:22 ` Jeff Liu 2014-01-10 13:33 ` Christoph Hellwig 2014-01-10 14:01 ` Jeff Liu 2014-01-11 11:10 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox