Re: [patch] fix truncate inode time modification breakage

[Boaz Harrosh <bharrosh@panasas.com>]

On 06/01/2010 04:39 PM, 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?
> 
> ---
>  fs/ext2/inode.c       |    1 -
>  fs/open.c             |    3 +++
>  fs/ramfs/file-nommu.c |    5 -----
>  mm/shmem.c            |    5 +++--
>  4 files changed, 6 insertions(+), 8 deletions(-)
> 
> 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;

OK I was just investigating this. Please forgive my noviceness here:

So i_mtime is modifications time. i_ctime is creation time?

if I do ftrunc() (like dd skip=x) don't I want modification time changed
but creation time unchanged?

if I do chmod/chown do I want modification-time changed creation not?

open+truncate both m an c changed?

modification-time: is it any aspect of the file has changed? or just the actual data / size?

Confused?

Boaz

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