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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
