lkml.org 
[lkml]   [1998]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
Subject[PATCH] for lost mail due to NFS cache/locking non-atomicity
Linus,

I didn't see any NFS client maintainer so I'm sending this directly to
you.

There is a problem with the aggressive file cacheing of the NFS
filesystem. It's a generic problem to do with not honouring the
expected atomicity of reads and writes around locks, and there's one
very important application that it affects: Mail over NFS.

Symptom
=======

Specifically, after some new mail arrives, if you exit a mail reader in
the next 60 seconds, the mail can be lost. If it is a shorter interval,
e.g. 10 seconds, the mail is very likely to be lost. This is easily
tested. You must have modified the mail folder in the reader in some
way, e.g. changed an "N" status to "O" or deleted a message, so the
folder is written back.

This happens because when the mail reader closes the file, it locks the
file, checks the timestamp and/or reads the file, writes its idea of the
new file, then unlocks the file. The bug is that after locking the
file, the cached attributes and data are returned. These can be up to
60 seconds old with the default settings. Sometimes the settings are
even longer.

Tests
=====

I have tested for the problem and it is very easy to lose mail when you
know what order to do things in. I now believe I have lost mail in the
past because of this. (I didn't know it was likely before...)

It's easily tested. This is what I did with Mutt (should be similar
with Elm, Pine etc.):

1. Send myself a mail. Wait at least 60 seconds.
2. Start Mutt, see the mail with "N" flag.
3. Read mail, clears "N" flag.
4. Send myself a mail from within Mutt.
5. Synchronise mail folder ("$" key).
6. See that the second mail has got lost.
7. Repeat until convinced.

Extra steps include:

4a. Read mail on another machine, prove that the second mail did arrive.
6a. See that the second mail isn't visible on the other machine either.
8. Fix kernel (see below).

I also did a whole bunch of other tests on other files.

Some side issues
================

- I know mail over NFS is not recommended but some of us do it anyway.
- Some people have no choice.
- This bug can also bite mail folders kept in a user's home directory,
which are often mounted over NFS. Depending on what the user is up to.
- Other applications that use locking to synchronise operations are
affected, e.g. databases etc.
- Mail was easily lost with NFS and 2.0.x kernels because they don't
implement the locking protocol, and don't even report this to the
applications! But 2.1.x and eventually 2.2 do implement locking
over NFS. So applications that have a use for locking ought to
work! :-)
- The NFS cache coherency protocol is still broken w.r.t. atomic
operations taking less than a microsecond (1 second if the client or
server is Linux), if the file size stays the same. But this is not
a problem for mail delivery.

Patch to fix
============

The enclosed patch fixes the problem, by resynchronising the cache after
a file lock is acquired. Flushing dirty buffers already happens before
the lock is released.

The 60 second loss window is closed by adding just a couple of lines to
fs/nfs/file.c: if (condition) NFS_CACHEINV(inode). But that still
leaves a small possible window due to asynchronous requests, that shows
up only occasionally.

A bit more code fixes the in-flight-requests loss too. I would like
someone to check if I have missed any possibility, but even if I have
this code should reduce the likelihood of lost data significantly.

Also fixed is another atomicity problem, when a signal is received while
a file is being unlocked. The existing code could silently decide not
to flush the dirty buffers, which was bad if the signal is handled or
ignored. The only useful case for this was when a process died
unexpectedly, but the implemented test would never catch that anyway.
The change to fs/open.c is part of this.

The patch has been tested by me in various ways and mail is now not lost
in the situations described. I have been running with it for a couple
of weeks to check it is stable, and it has been fine so far. General
performance seems as fast as ever (though I haven't measured it).

The patch is against 2.1.129, but should apply cleanly to 2.1.130. In
my opinion, some form of this patch is important before 2.2 is widely
deployed.

Enjoy,
-- Jamie

--- linux/fs/nfs/file.c.nfslock Fri Nov 20 14:21:29 1998
+++ linux/fs/nfs/file.c Sun Nov 22 20:48:47 1998
@@ -213,7 +213,8 @@
int
nfs_lock(struct file *filp, int cmd, struct file_lock *fl)
{
- struct inode * inode = filp->f_dentry->d_inode;
+ struct dentry * dentry = filp->f_dentry;
+ struct inode * inode = dentry->d_inode;
int status;

dprintk("NFS: nfs_lock(f=%4x/%ld, t=%x, fl=%x, r=%ld:%ld)\n",
@@ -242,10 +243,17 @@
if (!fl->fl_owner || (fl->fl_flags & (FL_POSIX|FL_BROKEN)) != FL_POSIX)
return -ENOLCK;

- /* If unlocking a file region, flush dirty pages (unless we've
- * been killed by a signal, that is). */
- if (cmd == F_SETLK && fl->fl_type == F_UNLCK
- && !signal_pending(current)) {
+ /* If unlocking a file region, flush dirty pages. This used
+ * to skip flushing when a signal was pending, but that was
+ * silently wrong for a handled or non-fatal signal. It was
+ * also inconsistent with the effect of a signal arriving
+ * _during_ the writeback.
+ *
+ * The flush() operation on file close means there should be
+ * nothing to flush here if this process is exiting, otherwise
+ * a pending signal will result in -ERESTARTSYS.
+ * */
+ if (cmd == F_SETLK && fl->fl_type == F_UNLCK) {
status = nfs_wb_area(inode, /* current->pid ?*/
fl->fl_start, fl->fl_end == NLM_OFFSET_MAX? 0 :
fl->fl_end - fl->fl_start + 1);
@@ -258,6 +266,41 @@

/* Here, we could turn off write-back of pages in the
* locked file region */
+
+ /* Cache coherency warning!
+ *
+ * (Previously) How to lose mail over NFS: read mail, flag it
+ * as read, receive more mail, quit your mail reader which
+ * writes the flagged message back. Even with locking working
+ * perfectly, the mail reader doesn't see the new mail and
+ * writes an old mailbox. Eek! The new mail is lost. Take
+ * your time, you have an easy 60 seconds to do this.
+ *
+ * (But now) For proper atomicity we have to invalidate cached
+ * data in the region after locking it. But the performance
+ * sucks. So invalidate the attribute cache and let the next
+ * remote mtime/size tell us if our data is stale.
+ *
+ * mtime isn't reliable because of its granularity, but
+ * there's one very important application that needs this:
+ * mail delivery and/or reading via NFS. When mail is
+ * delivered the size always changes.
+ *
+ * -- 17/Nov/1998, Jamie Lokier <jamie@imbolc.ucc.ie>
+ * Thanks to Mike Spivey for noticing the problem. */
+
+ if ((cmd == F_SETLK || cmd == F_SETLKW) && fl->fl_type != F_UNLCK) {
+ /* Must do a synchronous revalidation, not just
+ * NFS_CACHEINV, otherwise requests "in flight" can
+ * restore the attribute cache with information from
+ * before the lock. NFS_CACHEINV is still needed for
+ * some error cases.
+ *
+ * No need to reset NFS_ATTRTIMEO(inode), because no
+ * real change has been detected yet. */
+ NFS_CACHEINV(inode);
+ nfs_force_revalidate(NFS_DSERVER(dentry), dentry);
+ }

return 0;
}
--- linux/fs/nfs/inode.c.nfslock Fri Nov 20 14:21:29 1998
+++ linux/fs/nfs/inode.c Sun Nov 22 20:17:31 1998
@@ -649,16 +649,12 @@
* the cached attributes have to be refreshed.
*/
int
-_nfs_revalidate_inode(struct nfs_server *server, struct dentry *dentry)
+nfs_force_revalidate(struct nfs_server *server, struct dentry *dentry)
{
struct inode *inode = dentry->d_inode;
- int status = 0;
+ int status;
struct nfs_fattr fattr;

- /* Don't bother revalidating if we've done it recently */
- if (jiffies - NFS_READTIME(inode) < NFS_ATTRTIMEO(inode))
- goto out;
-
dfprintk(PAGECACHE, "NFS: revalidating %s/%s, ino=%ld\n",
dentry->d_parent->d_name.name, dentry->d_name.name,
inode->i_ino);
--- linux/fs/open.c.nfslock Fri Nov 20 14:21:29 1998
+++ linux/fs/open.c Sun Nov 22 20:41:33 1998
@@ -797,11 +797,11 @@
printk("VFS: Close: file count is 0\n");
return 0;
}
- if (dentry->d_inode)
- locks_remove_posix(filp, id);
retval = 0;
if (filp->f_op && filp->f_op->flush)
retval = filp->f_op->flush(filp);
+ if (dentry->d_inode)
+ locks_remove_posix(filp, id);
fput(filp);
return retval;
}
--- linux/include/linux/nfs_fs.h.nfslock Sat Nov 21 19:05:29 1998
+++ linux/include/linux/nfs_fs.h Sat Nov 21 19:05:29 1998
@@ -182,7 +182,7 @@
struct nfs_fattr *);
extern int nfs_refresh_inode(struct inode *, struct nfs_fattr *);
extern int nfs_revalidate(struct dentry *);
-extern int _nfs_revalidate_inode(struct nfs_server *, struct dentry *);
+extern int nfs_force_revalidate(struct nfs_server *, struct dentry *);

/*
* linux/fs/nfs/file.c
@@ -256,7 +256,7 @@
struct inode *inode = dentry->d_inode;
if (jiffies - NFS_READTIME(inode) < NFS_ATTRTIMEO(inode))
return 0;
- return _nfs_revalidate_inode(server, dentry);
+ return nfs_force_revalidate(server, dentry);
}

static inline int
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:45    [W:0.073 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site