[PATCH] flush inode when changing atime.

[PATCH] flush inode when changing atime.

[Utako Kusaka]

Hi,

The atime is changed for reading but it returns to a previous value
after unmount. i_update_core is still off after reading a file using
read(), readdir() and readlink(). So an inode isn't flushed to disk.

I referred to following fix when I made a patch:
TAKE 946679 - fix, speedup and simplify atime handling
http://marc.info/?l=linux-xfs&m=113398234310217&w=4

Example:
# ls -lu mpnt/log3
-rw-r--r--  1 root root 63494 2007-04-05 16:35 mpnt/log3
# less mpnt/log3
# ls -lu mpnt/log3
-rw-r--r--  1 root root 63494 2007-10-05 15:27 mpnt/log3
# umount mpnt/
# mount -t xfs /dev/sda6 mpnt
# ls -lu mpnt/log3
-rw-r--r--  1 root root 63494 2007-04-05 16:35 mpnt/log3

--
Utako

Signed-off-by: Utako Kusaka <u-kusaka@wm.jp.nec.com>
----------
--- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2007-10-15 15:50:10.000000000 +0900
+++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2007-10-15 16:49:35.000000000 +0900
@@ -1003,6 +1003,8 @@ xfs_readlink(
 		error = xfs_readlink_bmap(ip, link);
 	}
 
+	ip->i_update_core = 1;
+
  out:
 	xfs_iunlock(ip, XFS_ILOCK_SHARED);
 	return error;
--- linux-2.6-xfs.orig/fs/xfs/xfs_dir2.c	2007-10-15 15:50:09.000000000 +0900
+++ linux-2.6-xfs/fs/xfs/xfs_dir2.c	2007-10-15 16:48:47.000000000 +0900
@@ -318,6 +318,9 @@ xfs_readdir(
 	else
 		rval = xfs_dir2_leaf_getdents(dp, dirent, bufsize, offset,
 					      filldir);
+
+	dp->i_update_core = 1;
+
 	return rval;
 }
 
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_lrw.c	2007-10-15 15:50:10.000000000 +0900
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_lrw.c	2007-10-15 16:50:50.000000000 +0900
@@ -275,6 +275,9 @@ xfs_read(
 		XFS_STATS_ADD(xs_read_bytes, ret);
 
 	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
+
+	if (likely(!(ioflags & IO_INVIS)))
+		ip->i_update_core = 1;
 	return ret;
 }
 
----------

Re: [PATCH] flush inode when changing atime.

[David Chinner]

On Wed, Oct 17, 2007 at 01:56:00PM +0900, Utako Kusaka wrote:
> Hi,
> 
> The atime is changed for reading but it returns to a previous value
> after unmount. i_update_core is still off after reading a file using
> read(), readdir() and readlink(). So an inode isn't flushed to disk.

I think this was done by design - Christoph? I can't remember exactly
as it was more than two years ago this change was made. It is effectively
equivalent to using the relatime mount option.

The question is whether we want to change the default behaviour or
whether we need to supply an "atimeisatime" mount option for those
that really need atime to be updated on every access.

> I referred to following fix when I made a patch:
> TAKE 946679 - fix, speedup and simplify atime handling
> http://marc.info/?l=linux-xfs&m=113398234310217&w=4

Yes, that was the patch that removed the explicit atime updates
in the XFS code. Your fix is reverting part of that change.

> --- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2007-10-15 15:50:10.000000000 +0900
> +++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2007-10-15 16:49:35.000000000 +0900
> @@ -1003,6 +1003,8 @@ xfs_readlink(
>  		error = xfs_readlink_bmap(ip, link);
>  	}
>  
> +	ip->i_update_core = 1;
> +

If we are going to put these back in, then they should be
calls to xfs_ichgtime_fast() so that we know what the reason
for marking the core dirty is.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [PATCH] flush inode when changing atime.

[Lachlan McIlroy]

David Chinner wrote:
> On Wed, Oct 17, 2007 at 01:56:00PM +0900, Utako Kusaka wrote:
>> Hi,
>>
>> The atime is changed for reading but it returns to a previous value
>> after unmount. i_update_core is still off after reading a file using
>> read(), readdir() and readlink(). So an inode isn't flushed to disk.
> 
> I think this was done by design - Christoph? I can't remember exactly
> as it was more than two years ago this change was made. It is effectively
> equivalent to using the relatime mount option.
> 
> The question is whether we want to change the default behaviour or
> whether we need to supply an "atimeisatime" mount option for those
> that really need atime to be updated on every access.
> 
If we change it back then will anything that scans the filesystem cause
inodes to be dirtied and create a lot of inode flush traffic that we
don't currently have?

>> I referred to following fix when I made a patch:
>> TAKE 946679 - fix, speedup and simplify atime handling
>> http://marc.info/?l=linux-xfs&m=113398234310217&w=4
> 
> Yes, that was the patch that removed the explicit atime updates
> in the XFS code. Your fix is reverting part of that change.
> 
>> --- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c	2007-10-15 15:50:10.000000000 +0900
>> +++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c	2007-10-15 16:49:35.000000000 +0900
>> @@ -1003,6 +1003,8 @@ xfs_readlink(
>>  		error = xfs_readlink_bmap(ip, link);
>>  	}
>>  
>> +	ip->i_update_core = 1;
>> +
> 
> If we are going to put these back in, then they should be
> calls to xfs_ichgtime_fast() so that we know what the reason
> for marking the core dirty is.
> 

xfs_ichgtime_fast() will also dirty the linux inode so that sync
will push out the change.

Re: [PATCH] flush inode when changing atime.

[David Chinner]

On Thu, Oct 18, 2007 at 01:52:49PM +1000, Lachlan McIlroy wrote:
> David Chinner wrote:
> >On Wed, Oct 17, 2007 at 01:56:00PM +0900, Utako Kusaka wrote:
> >>Hi,
> >>
> >>The atime is changed for reading but it returns to a previous value
> >>after unmount. i_update_core is still off after reading a file using
> >>read(), readdir() and readlink(). So an inode isn't flushed to disk.
> >
> >I think this was done by design - Christoph? I can't remember exactly
> >as it was more than two years ago this change was made. It is effectively
> >equivalent to using the relatime mount option.
> >
> >The question is whether we want to change the default behaviour or
> >whether we need to supply an "atimeisatime" mount option for those
> >that really need atime to be updated on every access.
> >
> If we change it back then will anything that scans the filesystem cause
> inodes to be dirtied and create a lot of inode flush traffic that we
> don't currently have?

Right. And given that it's taken over 2 years for anyone to notice
that atime only get updated when a file is otherwise dirtied....

> >If we are going to put these back in, then they should be
> >calls to xfs_ichgtime_fast() so that we know what the reason
> >for marking the core dirty is.
> 
> xfs_ichgtime_fast() will also dirty the linux inode so that sync
> will push out the change.

The VFS already calls filp_accessed, which marks the linux inode
dirty. We're telling sync that the inode really isn't dirty when
it tries to flush it, so it's not going to disk....

As Christoph pointed out last night to me the problem really is that
VFS atime updates don't have a callback into the filesystem(*) so if
we want to write back atime we've basically got to duplicate VFS
infrastructure. 

(*) i.e. call ->setattr to set the atime.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [PATCH] flush inode when changing atime.

[Lachlan McIlroy]

David Chinner wrote:
> On Thu, Oct 18, 2007 at 01:52:49PM +1000, Lachlan McIlroy wrote:
>> David Chinner wrote:
>>> On Wed, Oct 17, 2007 at 01:56:00PM +0900, Utako Kusaka wrote:
>>>> Hi,
>>>>
>>>> The atime is changed for reading but it returns to a previous value
>>>> after unmount. i_update_core is still off after reading a file using
>>>> read(), readdir() and readlink(). So an inode isn't flushed to disk.
>>> I think this was done by design - Christoph? I can't remember exactly
>>> as it was more than two years ago this change was made. It is effectively
>>> equivalent to using the relatime mount option.
>>>
>>> The question is whether we want to change the default behaviour or
>>> whether we need to supply an "atimeisatime" mount option for those
>>> that really need atime to be updated on every access.
>>>
>> If we change it back then will anything that scans the filesystem cause
>> inodes to be dirtied and create a lot of inode flush traffic that we
>> don't currently have?
> 
> Right. And given that it's taken over 2 years for anyone to notice
> that atime only get updated when a file is otherwise dirtied....
> 
>>> If we are going to put these back in, then they should be
>>> calls to xfs_ichgtime_fast() so that we know what the reason
>>> for marking the core dirty is.
>> xfs_ichgtime_fast() will also dirty the linux inode so that sync
>> will push out the change.
> 
> The VFS already calls filp_accessed, which marks the linux inode
> dirty. We're telling sync that the inode really isn't dirty when
> it tries to flush it, so it's not going to disk....
Ah, so it is.  We could compare the linux inode atime with the xfs
inode atime in the inode flush path and if they are different sync
them and dirty the xfs inode (set i_update_core).  But as you said
it should be a mount option cause flushing inodes just for atime
updates would hurt.

> 
> As Christoph pointed out last night to me the problem really is that
> VFS atime updates don't have a callback into the filesystem(*) so if
> we want to write back atime we've basically got to duplicate VFS
> infrastructure. 
> 
> (*) i.e. call ->setattr to set the atime.
> 
> Cheers,
> 
> Dave.