truncate() or ftruncate shouldn't change mtime if size doesn't change.

According to Posix and SUS, truncate(2) and ftruncate(2) only update ctime and mtime if the size actually changes. Linux doesn't currently obey this.

There is no need to test the size under i_sem, as loosing any race will not make a noticable different the mtime or ctime.

(According to SUS, truncate and ftruncate 'may' clear setuid/setgid
as well, currently we don't. Should we?
)

Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./fs/open.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff ./fs/open.c~current~ ./fs/open.c

Quoted 6 lines · NeilBrown

According to Posix and SUS, truncate(2) and ftruncate(2) only update ctime and mtime if the size actually changes. Linux doesn't currently obey this.

There is no need to test the size under i_sem, as loosing any race will not make a noticable different the mtime or ctime.

Well if there's a race then the file may end up not being truncated after this patch is applied. But that could have happened anwyay, so I don't see a need for i_sem synchronisation either.

Quoted 15 lines

(According to SUS, truncate and ftruncate 'may' clear setuid/setgid
as well, currently we don't. Should we?
)

Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./fs/open.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff ./fs/open.c~current~ ./fs/open.c

fs/open.c static inline long do_sys_truncate(const · line 264

Quoted 6 lines

fs/open.c renamed from fs/open.c~current~ +2 -1

@@ -260,7 +260,8 @@ static inline long do_sys_truncate(const
 		goto dput_and_out;
 
 	error = locks_verify_truncate(inode, NULL, length);
-	if (!error) {
+	if (!error &&
+	    length != i_size_read(dentry->d_inode)) {

Odd code layout?

Quoted 6 lines

fs/open.c renamed from fs/open.c~current~ +0 -0

 		DQUOT_INIT(inode);
 		error = do_truncate(nd.dentry, length);
 	}

@@ -313,7 +314,8 @@ static inline long do_sys_ftruncate(unsi
 		goto out_putf;
 

fs/open.c static inline long do_sys_ftruncate(unsi · line 321

Quoted 7 lines

fs/open.c renamed from fs/open.c~current~ +2 -1

 	error = locks_verify_truncate(inode, file, length);
-	if (!error)
+	if (!error &&
+	    length != i_size_read(dentry->d_inode))
 		error = do_truncate(dentry, length);
 out_putf:
 	fput(file);

This partially obsoletes the similar optimisation in inode_setattr(). I guess the optimisation there retains some usefulness for O_TRUNC opens of zero-length files, but for symettry and micro-efficiency, perhaps we should remvoe the inode_setattr() test and check for i_size==0 in may_open()?

Quoted 42 lines · Andrew Morton

Quoted 6 lines · NeilBrown

According to Posix and SUS, truncate(2) and ftruncate(2) only update ctime and mtime if the size actually changes. Linux doesn't currently obey this.

There is no need to test the size under i_sem, as loosing any race will not make a noticable different the mtime or ctime.

Well if there's a race then the file may end up not being truncated after this patch is applied. But that could have happened anwyay, so I don't see a need for i_sem synchronisation either.

Quoted 15 lines

(According to SUS, truncate and ftruncate 'may' clear setuid/setgid
as well, currently we don't. Should we?
)

Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./fs/open.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff ./fs/open.c~current~ ./fs/open.c

fs/open.c static inline long do_sys_truncate(const · line 264

Quoted 6 lines

fs/open.c renamed from fs/open.c~current~ +2 -1

@@ -260,7 +260,8 @@ static inline long do_sys_truncate(const
 		goto dput_and_out;
 
 	error = locks_verify_truncate(inode, NULL, length);
-	if (!error) {
+	if (!error &&
+	    length != i_size_read(dentry->d_inode)) {

Odd code layout?

Quoted 6 lines

fs/open.c renamed from fs/open.c~current~ +0 -0

 		DQUOT_INIT(inode);
 		error = do_truncate(nd.dentry, length);
 	}

@@ -313,7 +314,8 @@ static inline long do_sys_ftruncate(unsi
 		goto out_putf;
 

Quoted 12 lines

fs/open.c static inline long do_sys_ftruncate(unsi · line 321

Quoted 7 lines

fs/open.c renamed from fs/open.c~current~ +2 -1

 	error = locks_verify_truncate(inode, file, length);
-	if (!error)
+	if (!error &&
+	    length != i_size_read(dentry->d_inode))
 		error = do_truncate(dentry, length);
 out_putf:
 	fput(file);

This partially obsoletes the similar optimisation in inode_setattr(). I guess the optimisation there retains some usefulness for O_TRUNC opens of zero-length files, but for symettry and micro-efficiency, perhaps we should remvoe the inode_setattr() test and check for i_size==0 in may_open()?

Sounds like a good idea. That does simplify inode_setattr() nicely...

Signed-off-by Anton Altaparmakov <[email protected]>

fs/attr.c renamed from fs/attr.c.old +3 -12

@@ -70,19 +70,10 @@ int inode_setattr(struct inode * inode, 
 	int error = 0;
 
 	if (ia_valid & ATTR_SIZE) {
-		if (attr->ia_size != i_size_read(inode)) {
-			error = vmtruncate(inode, attr->ia_size);
-			if (error || (ia_valid == ATTR_SIZE))
-				goto out;
-		} else {
-			/*
-			 * We skipped the truncate but must still update
-			 * timestamps
-			 */
-			ia_valid |= ATTR_MTIME|ATTR_CTIME;
-		}
+		error = vmtruncate(inode, attr->ia_size);
+		if (error || (ia_valid == ATTR_SIZE))
+			goto out;
 	}
-
 	if (ia_valid & ATTR_UID)
 		inode->i_uid = attr->ia_uid;
 	if (ia_valid & ATTR_GID)

btw. Is it actually correct that we "goto out;" when "ia_valid == ATTR_SIZE"? That way we skip the mark_inode_dirty() call just before the "out" label...

For ntfs at least that is fine because ntfs does an "inode_update_time(inode, 1)" unconditionally in ntfs_truncate() even when the size has not changed which calls mark_inode_dirty_sync() and when the size changes it also does a "__mark_inode_dirty(inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);" but I am not sure all filesystems are fine in that respect?

In may_open() only truncate when O_TRUNC is set and the file size is not zero.

fs/namei.c renamed from fs/namei.c.old +1 -1

@@ -1447,7 +1447,7 @@ int may_open(struct nameidata *nd, int a
 	if (error)
 		return error;
 
-	if (flag & O_TRUNC) {
+	if (flag & O_TRUNC && i_size_read(inode)) {
 		error = get_write_access(inode);
 		if (error)
 			return error;

Quoted 5 lines

This partially obsoletes the similar optimisation in inode_setattr(). I guess the optimisation there retains some usefulness for O_TRUNC opens of zero-length files, but for symettry and micro-efficiency, perhaps we should remvoe the inode_setattr() test and check for i_size==0 in may_open()?

Sounds like a good idea. That does simplify inode_setattr() nicely...

Signed-off-by Anton Altaparmakov <[email protected]>

fs/attr.c renamed from fs/attr.c.old +3 -12

@@ -70,19 +70,10 @@ int inode_setattr(struct inode * inode, 
 	int error = 0;
 
 	if (ia_valid & ATTR_SIZE) {
-		if (attr->ia_size != i_size_read(inode)) {
-			error = vmtruncate(inode, attr->ia_size);
-			if (error || (ia_valid == ATTR_SIZE))
-				goto out;
-		} else {
-			/*
-			 * We skipped the truncate but must still update
-			 * timestamps
-			 */
-			ia_valid |= ATTR_MTIME|ATTR_CTIME;
-		}
+		error = vmtruncate(inode, attr->ia_size);
+		if (error || (ia_valid == ATTR_SIZE))
+			goto out;
 	}
-
 	if (ia_valid & ATTR_UID)
 		inode->i_uid = attr->ia_uid;
 	if (ia_valid & ATTR_GID)

btw. Is it actually correct that we "goto out;" when "ia_valid == ATTR_SIZE"? That way we skip the mark_inode_dirty() call just before the "out" label...

For ntfs at least that is fine because ntfs does an "inode_update_time(inode, 1)" unconditionally in ntfs_truncate() even when the size has not changed which calls mark_inode_dirty_sync() and when the size changes it also does a "__mark_inode_dirty(inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);" but I am not sure all filesystems are fine in that respect?

if (ia_valid & ATTR_SIZE)

		error = vmtruncate(inode, attr->ia_size);

Quoted 40 lines · On Monday October 31, [email protected] wrote:

Quoted 5 lines

This partially obsoletes the similar optimisation in inode_setattr(). I guess the optimisation there retains some usefulness for O_TRUNC opens of zero-length files, but for symettry and micro-efficiency, perhaps we should remvoe the inode_setattr() test and check for i_size==0 in may_open()?

Sounds like a good idea. That does simplify inode_setattr() nicely...

Signed-off-by Anton Altaparmakov <[email protected]>

fs/attr.c renamed from fs/attr.c.old +3 -12

@@ -70,19 +70,10 @@ int inode_setattr(struct inode * inode, 
 	int error = 0;
 
 	if (ia_valid & ATTR_SIZE) {
-		if (attr->ia_size != i_size_read(inode)) {
-			error = vmtruncate(inode, attr->ia_size);
-			if (error || (ia_valid == ATTR_SIZE))
-				goto out;
-		} else {
-			/*
-			 * We skipped the truncate but must still update
-			 * timestamps
-			 */
-			ia_valid |= ATTR_MTIME|ATTR_CTIME;
-		}
+		error = vmtruncate(inode, attr->ia_size);
+		if (error || (ia_valid == ATTR_SIZE))
+			goto out;
 	}
-
 	if (ia_valid & ATTR_UID)
 		inode->i_uid = attr->ia_uid;
 	if (ia_valid & ATTR_GID)

btw. Is it actually correct that we "goto out;" when "ia_valid == ATTR_SIZE"? That way we skip the mark_inode_dirty() call just before the "out" label...

Quoted 6 lines

For ntfs at least that is fine because ntfs does an "inode_update_time(inode, 1)" unconditionally in ntfs_truncate() even when the size has not changed which calls mark_inode_dirty_sync() and when the size changes it also does a "__mark_inode_dirty(inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);" but I am not sure all filesystems are fine in that respect?

I think the 'goto' is fine as presumably an error from vmtruncate means that nothing was changed, and as there are no other attr bits, there is no need to mark_inode_dirty.

However, there always will be other ATTR bits as whenever we set ATTR_SIZE (do_truncate and nfsd_setattr) we also set ATTR_CTIME, so this "optimisation" is just a waste of space.
Best make it:

Quoted 2 lines

if (ia_valid & ATTR_SIZE)

		error = vmtruncate(inode, attr->ia_size);

and assume that the filesystem's ->truncate or ->setattr will still set mtime if the size doesn't change (->truncate would have a hard time knowing if it has changed or not, so it has to set mtime unconditionally if it exists .. ->setattr... probably does the right thing)

According to Posix and SUS, truncate(2) and ftruncate(2) only update ctime and mtime if the size actually changes. Linux doesn't currently obey this.

There is no need to test the size under i_sem, as loosing any race will not make a noticable different the mtime or ctime.

(According to SUS, truncate and ftruncate 'may' clear setuid/setgid
as well, currently we don't. Should we?
)

Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./fs/open.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff ./fs/open.c~current~ ./fs/open.c

This should normally be OK as far as NFS is concerned since we usually end up revalidating the attribute cache in the lookup() code, but you could imagine a networked filesystem that does not do this. I'd therefore prefer if such checks were made in the filesystem itself.

What we can, however, do is to ensure that truncate() and ftruncate() only set ATTR_SIZE, but ensure that may_open() sets ATTR_MTIME| ATTR_CTIME as well.

Quoted 4 lines · On Monday October 31, [email protected] wrote:

What we can, however, do is to ensure that truncate() and ftruncate() only set ATTR_SIZE, but ensure that may_open() sets ATTR_MTIME| ATTR_CTIME as well.

Thanks. This makes lots of sense, in that it gives power to the filesystem. We should keep the optimisations in placed where the filesystem can over-ride them, just as inode_setattr.

So, here is a revised patch.
Comment?