Re: [PATCH] mtime attribute is not being updated on client
On Fri, 2005-04-08 at 16:54, Linda Dunaphant wrote:
>Do you think it would be better for nfs_refresh_inode() to check the mtime,
>perform the mtime update if needed, and not set the NFS_INO_INVALID_ATTR
>flag if the data_unstable flag is set? This is how nfs_update_inode()
>handles its mtime check.
Hi again Trond,
I updated my first patch to nfs_refresh_inode() to be similar to the way
nfs_update_inode() does the check and update of the mtime. nfs_refresh_inode()
now checks to see if the mtime changed, and if so, it does the update of
the mtime. It only sets NFS_INO_INVALID_ATTR if data_unstable is not set.
I temporarily added some printk's to selected functions to monitor some of
the functions called after the data write to the server occurs. With this
latest patch, the sequence that I see with the test program is now the
same as it was originally without any patch - except the mtime is has been
updated:
nfs3_xdr_writeargs
xdr_decode_fattr<--- new mtime from server
nfs_refresh_inode <--- updates mtime in inode
nfs_attribute_timeout
nfs_attribute_timeout
xdr_decode_fattr
nfs_refresh_inode
With the first patch I proposed this sequence was:
nfs3_xdr_writeargs
xdr_decode_fattr<--- new mtime from server
nfs_refresh_inode <--- NFS_INO_INVALID_ATTR set
xdr_decode_fattr
nfs_update_inode<--- updates mtime in inode
nfs_attribute_timeout
xdr_decode_fattr
Thank you for pointing out that there may be other consequences if the
NFS_INO_INVALID_ATTR is always set by nfs_refresh_inode() when the mtime
values are different. I believe this second patch fixes the original
problem without affecting any other behaviour.
Cheers,
Linda
diff -ura base/fs/nfs/inode.c new/fs/nfs/inode.c
--- base/fs/nfs/inode.c 2005-04-07 16:04:40.0 -0400
+++ new/fs/nfs/inode.c 2005-04-08 19:23:44.151698674 -0400
@@ -1176,9 +1176,17 @@
}
/* Verify a few of the more important attributes */
+ if (!timespec_equal(>i_mtime, >mtime)) {
+ memcpy(>i_mtime, >mtime, sizeof(inode->i_mtime));
+#ifdef NFS_DEBUG_VERBOSE
+ printk(KERN_DEBUG "NFS: mtime change on %s/%ld\n",
inode->i_sb->s_id, inode->i_ino);
+#endif
+ if (!data_unstable)
+ nfsi->flags |= NFS_INO_INVALID_ATTR;
+ }
+
if (!data_unstable) {
- if (!timespec_equal(>i_mtime, >mtime)
- || cur_size != new_isize)
+ if (cur_size != new_isize)
nfsi->flags |= NFS_INO_INVALID_ATTR;
} else if (S_ISREG(inode->i_mode) && new_isize > cur_size)
nfsi->flags |= NFS_INO_INVALID_ATTR;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtime attribute is not being updated on client
On Fri, 2005-04-08 at 09:11, Trond Myklebust wrote: > I'm a bit unclear as to what your end-goal is here. Is it basically to >ensure that fstat() always return the correct value for the mtime? > > The reason I ask is that I think your change is likely to have nasty >consequences for the general performance in a lot of other syscalls that >use nfs_revalidate_inode(). I would expect a particularly nasty hit in >the of the write() syscalls themselves, and they really shouldn't have >to worry about the value of mtime in the close-to-open cache consistency >model. >I therefore think we should look for a more fine-grained solution that >addresses more precisely the issues you see. > >Cheers, > Trond Hi Trond, The goal wasn't to ensure that fstat() always return the correct value for mtime. The goal is to update the mtime within the bounds of the min and max attribute cache timeouts, which was not happening before if the test ran for more than a minute. nfs_refresh_inode() was already being called after every write to the server and fattr->mtime was already set to the server's updated mtime value. However, it didn't check for an updated mtime value if data_unstable was set. Since nfs_refresh_inode() always resets the attribute timer (even when it skipped the mtime check), and the calls to it occurred more frequently than the attribute timer could expire, nfs_update_inode() was never being called again to update the inode's mtime. With the change I proposed, the test shows an mtime change every ~32 secs which corresponds to when the client writes the data to the server. Before this change, the test only showed one mtime change, even when it was run for > 10 mins. I did not see any increase in the calls to either nfs_revalidate_inode() or __nfs_revalidate_inode(). Do you think it would be better for nfs_refresh_inode() to check the mtime, perform the mtime update if needed, and not set the NFS_INO_INVALID_ATTR flag if the data_unstable flag is set? This is how nfs_update_inode() handles its mtime check. Regards, Linda - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtime attribute is not being updated on client
to den 07.04.2005 Klokka 20:52 (-0400) skreiv Linda Dunaphant: > Hi Trond, > > The acregmin (default=3) and acregmax (default=60) NFS attributes that > control the min and max attribute cache lifetimes don't appear to be > working after the first few timeouts. Using a test program that loops > on the following sequence: > - write to a file on an NFS3 mounted filesystem from the client > - sleep for one second > - stat the file to get the mtime > you see the mtime change once after ~56 seconds, but no further mtime > changes are detected by the test. > > nfs_refresh_inode() currently bypasses the inode vs fattr mtime comparison > if data_unstable is true. At the end of nfs_refresh_inode(), it resets the > attribute timer. Since nfs_refresh_inode() is being called after every > write to the server (which occurs more often than the attribute timer is > set to expire), the attribute timer never expires again for this file past > the ~56 sec point. > > In nfs_refresh_inode() I believe the mtime comparison should be moved outside > the check for data_unstable. The server might already have a newer value for > mtime than the value on the client. With this change, the test sees the mtime > change after each write completes on the server. > > Regards, > Linda Hi Linda, I'm a bit unclear as to what your end-goal is here. Is it basically to ensure that fstat() always return the correct value for the mtime? The reason I ask is that I think your change is likely to have nasty consequences for the general performance in a lot of other syscalls that use nfs_revalidate_inode(). I would expect a particularly nasty hit in the of the write() syscalls themselves, and they really shouldn't have to worry about the value of mtime in the close-to-open cache consistency model. I therefore think we should look for a more fine-grained solution that addresses more precisely the issues you see. Cheers, Trond -- Trond Myklebust <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtime attribute is not being updated on client
to den 07.04.2005 Klokka 20:52 (-0400) skreiv Linda Dunaphant: Hi Trond, The acregmin (default=3) and acregmax (default=60) NFS attributes that control the min and max attribute cache lifetimes don't appear to be working after the first few timeouts. Using a test program that loops on the following sequence: - write to a file on an NFS3 mounted filesystem from the client - sleep for one second - stat the file to get the mtime you see the mtime change once after ~56 seconds, but no further mtime changes are detected by the test. nfs_refresh_inode() currently bypasses the inode vs fattr mtime comparison if data_unstable is true. At the end of nfs_refresh_inode(), it resets the attribute timer. Since nfs_refresh_inode() is being called after every write to the server (which occurs more often than the attribute timer is set to expire), the attribute timer never expires again for this file past the ~56 sec point. In nfs_refresh_inode() I believe the mtime comparison should be moved outside the check for data_unstable. The server might already have a newer value for mtime than the value on the client. With this change, the test sees the mtime change after each write completes on the server. Regards, Linda Hi Linda, I'm a bit unclear as to what your end-goal is here. Is it basically to ensure that fstat() always return the correct value for the mtime? The reason I ask is that I think your change is likely to have nasty consequences for the general performance in a lot of other syscalls that use nfs_revalidate_inode(). I would expect a particularly nasty hit in the of the write() syscalls themselves, and they really shouldn't have to worry about the value of mtime in the close-to-open cache consistency model. I therefore think we should look for a more fine-grained solution that addresses more precisely the issues you see. Cheers, Trond -- Trond Myklebust [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtime attribute is not being updated on client
On Fri, 2005-04-08 at 09:11, Trond Myklebust wrote: I'm a bit unclear as to what your end-goal is here. Is it basically to ensure that fstat() always return the correct value for the mtime? The reason I ask is that I think your change is likely to have nasty consequences for the general performance in a lot of other syscalls that use nfs_revalidate_inode(). I would expect a particularly nasty hit in the of the write() syscalls themselves, and they really shouldn't have to worry about the value of mtime in the close-to-open cache consistency model. I therefore think we should look for a more fine-grained solution that addresses more precisely the issues you see. Cheers, Trond Hi Trond, The goal wasn't to ensure that fstat() always return the correct value for mtime. The goal is to update the mtime within the bounds of the min and max attribute cache timeouts, which was not happening before if the test ran for more than a minute. nfs_refresh_inode() was already being called after every write to the server and fattr-mtime was already set to the server's updated mtime value. However, it didn't check for an updated mtime value if data_unstable was set. Since nfs_refresh_inode() always resets the attribute timer (even when it skipped the mtime check), and the calls to it occurred more frequently than the attribute timer could expire, nfs_update_inode() was never being called again to update the inode's mtime. With the change I proposed, the test shows an mtime change every ~32 secs which corresponds to when the client writes the data to the server. Before this change, the test only showed one mtime change, even when it was run for 10 mins. I did not see any increase in the calls to either nfs_revalidate_inode() or __nfs_revalidate_inode(). Do you think it would be better for nfs_refresh_inode() to check the mtime, perform the mtime update if needed, and not set the NFS_INO_INVALID_ATTR flag if the data_unstable flag is set? This is how nfs_update_inode() handles its mtime check. Regards, Linda - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtime attribute is not being updated on client
On Fri, 2005-04-08 at 16:54, Linda Dunaphant wrote:
Do you think it would be better for nfs_refresh_inode() to check the mtime,
perform the mtime update if needed, and not set the NFS_INO_INVALID_ATTR
flag if the data_unstable flag is set? This is how nfs_update_inode()
handles its mtime check.
Hi again Trond,
I updated my first patch to nfs_refresh_inode() to be similar to the way
nfs_update_inode() does the check and update of the mtime. nfs_refresh_inode()
now checks to see if the mtime changed, and if so, it does the update of
the mtime. It only sets NFS_INO_INVALID_ATTR if data_unstable is not set.
I temporarily added some printk's to selected functions to monitor some of
the functions called after the data write to the server occurs. With this
latest patch, the sequence that I see with the test program is now the
same as it was originally without any patch - except the mtime is has been
updated:
nfs3_xdr_writeargs
xdr_decode_fattr--- new mtime from server
nfs_refresh_inode --- updates mtime in inode
nfs_attribute_timeout
nfs_attribute_timeout
xdr_decode_fattr
nfs_refresh_inode
With the first patch I proposed this sequence was:
nfs3_xdr_writeargs
xdr_decode_fattr--- new mtime from server
nfs_refresh_inode --- NFS_INO_INVALID_ATTR set
xdr_decode_fattr
nfs_update_inode--- updates mtime in inode
nfs_attribute_timeout
xdr_decode_fattr
Thank you for pointing out that there may be other consequences if the
NFS_INO_INVALID_ATTR is always set by nfs_refresh_inode() when the mtime
values are different. I believe this second patch fixes the original
problem without affecting any other behaviour.
Cheers,
Linda
diff -ura base/fs/nfs/inode.c new/fs/nfs/inode.c
--- base/fs/nfs/inode.c 2005-04-07 16:04:40.0 -0400
+++ new/fs/nfs/inode.c 2005-04-08 19:23:44.151698674 -0400
@@ -1176,9 +1176,17 @@
}
/* Verify a few of the more important attributes */
+ if (!timespec_equal(inode-i_mtime, fattr-mtime)) {
+ memcpy(inode-i_mtime, fattr-mtime, sizeof(inode-i_mtime));
+#ifdef NFS_DEBUG_VERBOSE
+ printk(KERN_DEBUG NFS: mtime change on %s/%ld\n,
inode-i_sb-s_id, inode-i_ino);
+#endif
+ if (!data_unstable)
+ nfsi-flags |= NFS_INO_INVALID_ATTR;
+ }
+
if (!data_unstable) {
- if (!timespec_equal(inode-i_mtime, fattr-mtime)
- || cur_size != new_isize)
+ if (cur_size != new_isize)
nfsi-flags |= NFS_INO_INVALID_ATTR;
} else if (S_ISREG(inode-i_mode) new_isize cur_size)
nfsi-flags |= NFS_INO_INVALID_ATTR;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mtime attribute is not being updated on client
Hi Trond,
The acregmin (default=3) and acregmax (default=60) NFS attributes that
control the min and max attribute cache lifetimes don't appear to be
working after the first few timeouts. Using a test program that loops
on the following sequence:
- write to a file on an NFS3 mounted filesystem from the client
- sleep for one second
- stat the file to get the mtime
you see the mtime change once after ~56 seconds, but no further mtime
changes are detected by the test.
nfs_refresh_inode() currently bypasses the inode vs fattr mtime comparison
if data_unstable is true. At the end of nfs_refresh_inode(), it resets the
attribute timer. Since nfs_refresh_inode() is being called after every
write to the server (which occurs more often than the attribute timer is
set to expire), the attribute timer never expires again for this file past
the ~56 sec point.
In nfs_refresh_inode() I believe the mtime comparison should be moved outside
the check for data_unstable. The server might already have a newer value for
mtime than the value on the client. With this change, the test sees the mtime
change after each write completes on the server.
Regards,
Linda
The following patch is for 2.6.12-rc2:
diff -Nura base/fs/nfs/inode.c new/fs/nfs/inode.c
--- base/fs/nfs/inode.c 2005-04-07 16:04:40.933611205 -0400
+++ new/fs/nfs/inode.c 2005-04-07 16:12:34.484299540 -0400
@@ -1176,9 +1176,11 @@
}
/* Verify a few of the more important attributes */
+ if (!timespec_equal(>i_mtime, >mtime))
+ nfsi->flags |= NFS_INO_INVALID_ATTR;
+
if (!data_unstable) {
- if (!timespec_equal(>i_mtime, >mtime)
- || cur_size != new_isize)
+ if (cur_size != new_isize)
nfsi->flags |= NFS_INO_INVALID_ATTR;
} else if (S_ISREG(inode->i_mode) && new_isize > cur_size)
nfsi->flags |= NFS_INO_INVALID_ATTR;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mtime attribute is not being updated on client
Hi Trond,
The acregmin (default=3) and acregmax (default=60) NFS attributes that
control the min and max attribute cache lifetimes don't appear to be
working after the first few timeouts. Using a test program that loops
on the following sequence:
- write to a file on an NFS3 mounted filesystem from the client
- sleep for one second
- stat the file to get the mtime
you see the mtime change once after ~56 seconds, but no further mtime
changes are detected by the test.
nfs_refresh_inode() currently bypasses the inode vs fattr mtime comparison
if data_unstable is true. At the end of nfs_refresh_inode(), it resets the
attribute timer. Since nfs_refresh_inode() is being called after every
write to the server (which occurs more often than the attribute timer is
set to expire), the attribute timer never expires again for this file past
the ~56 sec point.
In nfs_refresh_inode() I believe the mtime comparison should be moved outside
the check for data_unstable. The server might already have a newer value for
mtime than the value on the client. With this change, the test sees the mtime
change after each write completes on the server.
Regards,
Linda
The following patch is for 2.6.12-rc2:
diff -Nura base/fs/nfs/inode.c new/fs/nfs/inode.c
--- base/fs/nfs/inode.c 2005-04-07 16:04:40.933611205 -0400
+++ new/fs/nfs/inode.c 2005-04-07 16:12:34.484299540 -0400
@@ -1176,9 +1176,11 @@
}
/* Verify a few of the more important attributes */
+ if (!timespec_equal(inode-i_mtime, fattr-mtime))
+ nfsi-flags |= NFS_INO_INVALID_ATTR;
+
if (!data_unstable) {
- if (!timespec_equal(inode-i_mtime, fattr-mtime)
- || cur_size != new_isize)
+ if (cur_size != new_isize)
nfsi-flags |= NFS_INO_INVALID_ATTR;
} else if (S_ISREG(inode-i_mode) new_isize cur_size)
nfsi-flags |= NFS_INO_INVALID_ATTR;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
