lkml.org 
[lkml]   [1996]   [Jul]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: NFS caching patch.

Ok, this is _exactly_ why I didn't check the mtime in the first place,
and just to explain WHY this patch is bad bad bad, I'll walk you through
it.

On Sun, 14 Jul 1996, A.N.Kuznetsov wrote:
> diff -ur linux/fs/nfs/file.c linux-obj/fs/nfs/file.c
> --- linux/fs/nfs/file.c Sun Jul 14 21:03:44 1996
> +++ linux-obj/fs/nfs/file.c Sun Jul 14 21:11:36 1996
> @@ -147,8 +147,23 @@
> if (pos > inode->i_size)
> inode->i_size = pos;
> /* Avoid possible Solaris 2.5 nfsd bug */
> - if (inode->i_ino == fattr.fileid)
> + if (inode->i_ino == fattr.fileid) {
> +
> + /*
> + * It is hack, that cannot be avoided for NFSv2,
> + * because it has no "weak cache consistency" mechanism.
> + * We are deemed to believe, that the file
> + * was not changed since previous getattr (or
> + * another operation returning gratuitous attributes).
> + * Another solution would be to call getattr before
> + * each write :-) --ANK
> + * (see also nfs_notify_change() in inode.c)
> + */
> + inode->i_mtime = fattr.mtime.seconds;
> + NFS_MTIME_USECS(inode) = fattr.mtime.useconds;
> +
> nfs_refresh_inode(inode, &fattr);
> + }
> return written;
> }
>

The above "hack" is the obvious solution, but it's WRONG.

Think of when we write to the file, and have it cached. When the write
succeeds, we now update the "mtime" of the local cached data from the
resulting file attributes, and _really_ really bad things happen if
somebody else has happened to write to the file just before we wrote to
it.

In short, the above will update "mtime" to a new time _without_
guaranteeing that no changes have taken place in the file between the two
dates. Your logic is that "WE changed the file, thus we don't need to
invalidate", but the case may be that "WE AND SOMEBODY ELSE changed the
file".

Now, the above patch will result in us not necessarily flushing the cache
_ever_ even if it's full of data that is stale. That's a lot worse than the
current behaviour, where we don't flush the cache immediately, but we do
flush it eventually when it times out. The current behaviour is at least
correct (according to NFS specs you can cache read data for some time), the
patch introduces a bug (and your comment about calling "getattr()" before
each write is equally broken - you _cannot_ make the race window go away, and
it's fundamental to the whole NFSv2 design).

Linus


\
 
 \ /
  Last update: 2005-03-22 13:38    [from the cache]
©2003-2011 Jasper Spaans