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/
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/
[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/