[PATCH 2/2] ext3: Don't update ctime in ext3_splice_branch()

[PATCH 2/2] ext3: Don't update ctime in ext3_splice_branch()

[Kazuya Mio]

VFS handles updating ctime, so we don't need to update i_ctime
in ext3_splace_branch().

I backport the following patch for ext3:
http://marc.info/?l=linux-ext4&m=124505184027078&w=4

Signed-off-by: Kazuya Mio <k-mio@sx.jp.nec.com>
---
 fs/ext3/inode.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
index 2d0afec..95cb0d1 100644
--- a/fs/ext3/inode.c
+++ b/fs/ext3/inode.c
@@ -795,10 +795,6 @@ static int ext3_splice_branch(handle_t *handle, struct inode *inode,
 	}
 
 	/* We are done with atomic stuff, now do the rest of housekeeping */

Re: [PATCH 2/2] ext3: Don't update ctime in ext3_splice_branch()

[Jan Kara]

On Mon 30-01-12 17:41:33, Kazuya Mio wrote:
> VFS handles updating ctime, so we don't need to update i_ctime
> in ext3_splace_branch().
  Thanks for the patches.  This is true for ordinary writes but not true
when you write via mmap. We call file_update_time() during page fault so
ctime won't be completely wrong but still we should update it after block
is allocated during writeback to reflect that new block is allocated to
the inode.

								Honza

> I backport the following patch for ext3:
> http://marc.info/?l=linux-ext4&m=124505184027078&w=4
> 
> Signed-off-by: Kazuya Mio <k-mio@sx.jp.nec.com>
> ---
>  fs/ext3/inode.c |    6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
> index 2d0afec..95cb0d1 100644
> --- a/fs/ext3/inode.c
> +++ b/fs/ext3/inode.c
> @@ -795,10 +795,6 @@ static int ext3_splice_branch(handle_t *handle, struct inode *inode,
>  	}
>  
>  	/* We are done with atomic stuff, now do the rest of housekeeping */
> -
> -	inode->i_ctime = CURRENT_TIME_SEC;
> -	ext3_mark_inode_dirty(handle, inode);
> -	/* ext3_mark_inode_dirty already updated i_sync_tid */
>  	atomic_set(&ei->i_datasync_tid, handle->h_transaction->t_tid);
>  
>  	/* had we spliced it onto indirect block? */
> @@ -819,9 +815,9 @@ static int ext3_splice_branch(handle_t *handle, struct inode *inode,
>  	} else {
>  		/*
>  		 * OK, we spliced it into the inode itself on a direct block.
> -		 * Inode was dirtied above.
>  		 */
>  		jbd_debug(5, "splicing direct\n");
> +		ext3_mark_inode_dirty(handle, inode);
>  	}
>  	return err;
>  
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 2/2] ext3: Don't update ctime in ext3_splice_branch()

[Kazuya Mio]

2012/01/31 2:52, Jan Kara wrote:
>   Thanks for the patches.  This is true for ordinary writes but not true
> when you write via mmap. We call file_update_time() during page fault so
> ctime won't be completely wrong but still we should update it after block
> is allocated during writeback to reflect that new block is allocated to
> the inode.

Should we update ctime whenever a block is allocated?
If so, ordinary write in ext4 with indirect block mapping has the same problem
due to the following patch, right?
http://marc.info/?l=linux-ext4&m=124505184027078&w=4

Regards,
Kazuya Mio

Re: [PATCH 2/2] ext3: Don't update ctime in ext3_splice_branch()

[Jan Kara]

On Wed 01-02-12 16:56:17, Kazuya Mio wrote:
> 2012/01/31 2:52, Jan Kara wrote:
> >  Thanks for the patches.  This is true for ordinary writes but not true
> >when you write via mmap. We call file_update_time() during page fault so
> >ctime won't be completely wrong but still we should update it after block
> >is allocated during writeback to reflect that new block is allocated to
> >the inode.
> 
> Should we update ctime whenever a block is allocated?
  Since ctime should be updated whenever inode is changed, and allocating
block updates i_blocks (via dquot_alloc_block()), we should update ctime.

> If so, ordinary write in ext4 with indirect block mapping has the same problem
> due to the following patch, right?
> http://marc.info/?l=linux-ext4&m=124505184027078&w=4
  It's kind of the same problem. But things are more complicated by the
fact that ext4 also does delayed allocation so, as changelog of the patch
you reference says, it's kind of unexpected from users to see ctime
suddently update when VM decides to do writeout triggering block
allocation. I'd think updating ctime is a correct (although unexpected)
thing to do but I can understand Ted's position as well and he's ext4
maintainer :).

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 2/2] ext3: Don't update ctime in ext3_splice_branch()

[Kazuya Mio]

2012/02/01 19:46, Jan Kara wrote:
> On Wed 01-02-12 16:56:17, Kazuya Mio wrote:
>> 2012/01/31 2:52, Jan Kara wrote:
>>>   Thanks for the patches.  This is true for ordinary writes but not true
>>> when you write via mmap. We call file_update_time() during page fault so
>>> ctime won't be completely wrong but still we should update it after block
>>> is allocated during writeback to reflect that new block is allocated to
>>> the inode.
>>
>> Should we update ctime whenever a block is allocated?
>    Since ctime should be updated whenever inode is changed, and allocating
> block updates i_blocks (via dquot_alloc_block()), we should update ctime.
>
>> If so, ordinary write in ext4 with indirect block mapping has the same problem
>> due to the following patch, right?
>> http://marc.info/?l=linux-ext4&m=124505184027078&w=4
>    It's kind of the same problem. But things are more complicated by the
> fact that ext4 also does delayed allocation so, as changelog of the patch
> you reference says, it's kind of unexpected from users to see ctime
> suddently update when VM decides to do writeout triggering block
> allocation. I'd think updating ctime is a correct (although unexpected)
> thing to do but I can understand Ted's position as well and he's ext4
> maintainer :).

I see. But I think we don't need to update ctime every time at this point.
ext3 doesn't support nanosecond timestamp, so it is wasteful to update ctime
whenever we allocate one block. How about the following patch?

http://patchwork.ozlabs.org/patch/140073/

The result of my measurement shows that the above patch can reduce the number of
calling ext3_mark_inode_dirty() almost the same as the previous one.

Regards,
Kazuya Mio