NFS: fix the handling of NFS_INO_INVALID_DATA flag in nfs_revalidate_mapping

commit d529ef83c355f97027ff85298a9709fe06216a66 upstream.

There is a possible race in how the nfs_invalidate_mapping function is
handled.  Currently, we go and invalidate the pages in the file and then
clear NFS_INO_INVALID_DATA.

The problem is that it's possible for a stale page to creep into the
mapping after the page was invalidated (i.e., via readahead). If another
writer comes along and sets the flag after that happens but before
invalidate_inode_pages2 returns then we could clear the flag
without the cache having been properly invalidated.

So, we must clear the flag first and then invalidate the pages. Doing
this however, opens another race:

It's possible to have two concurrent read() calls that end up in
nfs_revalidate_mapping at the same time. The first one clears the
NFS_INO_INVALID_DATA flag and then goes to call nfs_invalidate_mapping.

Just before calling that though, the other task races in, checks the
flag and finds it cleared. At that point, it trusts that the mapping is
good and gets the lock on the page, allowing the read() to be satisfied
from the cache even though the data is no longer valid.

These effects are easily manifested by running diotest3 from the LTP
test suite on NFS. That program does a series of DIO writes and buffered
reads. The operations are serialized and page-aligned but the existing
code fails the test since it occasionally allows a read to come out of
the cache incorrectly. While mixing direct and buffered I/O isn't
recommended, I believe it's possible to hit this in other ways that just
use buffered I/O, though that situation is much harder to reproduce.

The problem is that the checking/clearing of that flag and the
invalidation of the mapping really need to be atomic. Fix this by
serializing concurrent invalidations with a bitlock.

At the same time, we also need to allow other places that check
NFS_INO_INVALID_DATA to check whether we might be in the middle of
invalidating the file, so fix up a couple of places that do that
to look for the new NFS_INO_INVALIDATING flag.

Doing this requires us to be careful not to set the bitlock
unnecessarily, so this code only does that if it believes it will
be doing an invalidation.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Signed-off-by: Jiri Slaby <jslaby@suse.cz>

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index d0325d7db6438ce099c36c4db83421c5158a3f04..4d9f098614bc6c955b348b12b6494ecf7d5fb9bc 100644 (file)
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -290,7 +290,8 @@ int nfs_readdir_search_for_cookie(struct nfs_cache_array *array, nfs_readdir_des
 
                        new_pos = desc->current_index + i;
                        if (ctx->attr_gencount != nfsi->attr_gencount
-                           || (nfsi->cache_validity & (NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA))) {
+                           || (nfsi->cache_validity & (NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA))
+                           || test_bit(NFS_INO_INVALIDATING, &nfsi->flags)) {
                                ctx->duped = 0;
                                ctx->attr_gencount = nfsi->attr_gencount;
                        } else if (new_pos < desc->ctx->pos) {

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 0ee22ab9ef97a1c1ee44955bee6d27e12a573af2..9b6c90a8a311a6826196948635616e13022953be 100644 (file)
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -988,11 +988,11 @@ static int nfs_invalidate_mapping(struct inode *inode, struct address_space *map
                if (ret < 0)
                        return ret;
        }
-       spin_lock(&inode->i_lock);
-       nfsi->cache_validity &= ~NFS_INO_INVALID_DATA;
-       if (S_ISDIR(inode->i_mode))
+       if (S_ISDIR(inode->i_mode)) {
+               spin_lock(&inode->i_lock);
                memset(nfsi->cookieverf, 0, sizeof(nfsi->cookieverf));
-       spin_unlock(&inode->i_lock);
+               spin_unlock(&inode->i_lock);
+       }
        nfs_inc_stats(inode, NFSIOS_DATAINVALIDATE);
        nfs_fscache_wait_on_invalidate(inode);
 
@@ -1018,6 +1018,7 @@ static bool nfs_mapping_need_revalidate_inode(struct inode *inode)
 int nfs_revalidate_mapping(struct inode *inode, struct address_space *mapping)
 {
        struct nfs_inode *nfsi = NFS_I(inode);
+       unsigned long *bitlock = &nfsi->flags;
        int ret = 0;
 
        /* swapfiles are not supposed to be shared. */
@@ -1029,12 +1030,45 @@ int nfs_revalidate_mapping(struct inode *inode, struct address_space *mapping)
                if (ret < 0)
                        goto out;
        }
+
+       /*
+        * We must clear NFS_INO_INVALID_DATA first to ensure that
+        * invalidations that come in while we're shooting down the mappings
+        * are respected. But, that leaves a race window where one revalidator
+        * can clear the flag, and then another checks it before the mapping
+        * gets invalidated. Fix that by serializing access to this part of
+        * the function.
+        *
+        * At the same time, we need to allow other tasks to see whether we
+        * might be in the middle of invalidating the pages, so we only set
+        * the bit lock here if it looks like we're going to be doing that.
+        */
+       for (;;) {
+               ret = wait_on_bit(bitlock, NFS_INO_INVALIDATING,
+                                 nfs_wait_bit_killable, TASK_KILLABLE);
+               if (ret)
+                       goto out;
+               if (!(nfsi->cache_validity & NFS_INO_INVALID_DATA))
+                       goto out;
+               if (!test_and_set_bit_lock(NFS_INO_INVALIDATING, bitlock))
+                       break;
+       }
+
+       spin_lock(&inode->i_lock);
        if (nfsi->cache_validity & NFS_INO_INVALID_DATA) {
+               nfsi->cache_validity &= ~NFS_INO_INVALID_DATA;
+               spin_unlock(&inode->i_lock);
                trace_nfs_invalidate_mapping_enter(inode);
                ret = nfs_invalidate_mapping(inode, mapping);
                trace_nfs_invalidate_mapping_exit(inode, ret);
+       } else {
+               /* something raced in and cleared the flag */
+               spin_unlock(&inode->i_lock);
        }
 
+       clear_bit_unlock(NFS_INO_INVALIDATING, bitlock);
+       smp_mb__after_clear_bit();
+       wake_up_bit(bitlock, NFS_INO_INVALIDATING);
 out:
        return ret;
 }

diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
index 89fe741e58b1f7280f44e882f640259b8a8af144..59f838cdc009307f539bb94ee75f3447bfd79a56 100644 (file)
--- a/fs/nfs/nfstrace.h
+++ b/fs/nfs/nfstrace.h
@@ -36,6 +36,7 @@
        __print_flags(v, "|", \
                        { 1 << NFS_INO_ADVISE_RDPLUS, "ADVISE_RDPLUS" }, \
                        { 1 << NFS_INO_STALE, "STALE" }, \
+                       { 1 << NFS_INO_INVALIDATING, "INVALIDATING" }, \
                        { 1 << NFS_INO_FLUSHING, "FLUSHING" }, \
                        { 1 << NFS_INO_FSCACHE, "FSCACHE" }, \
                        { 1 << NFS_INO_COMMIT, "COMMIT" }, \

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 28466be64eeb069cce4aff1634cfa73c8fca5fd4..d69e789f19083590ae624edbad8c8a7f40beadb0 100644 (file)
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -909,9 +909,13 @@ bool nfs_ctx_key_to_expire(struct nfs_open_context *ctx)
  */
 static bool nfs_write_pageuptodate(struct page *page, struct inode *inode)
 {
+       struct nfs_inode *nfsi = NFS_I(inode);
+
        if (nfs_have_delegated_attributes(inode))
                goto out;
-       if (NFS_I(inode)->cache_validity & (NFS_INO_INVALID_DATA|NFS_INO_REVAL_PAGECACHE))
+       if (nfsi->cache_validity & (NFS_INO_INVALID_DATA|NFS_INO_REVAL_PAGECACHE))
+               return false;
+       if (test_bit(NFS_INO_INVALIDATING, &nfsi->flags))
                return false;
 out:
        return PageUptodate(page) != 0;

diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 3ea4cde8701ce1e97d6a39a4a1264452468a5254..a632498d42fa061fa9f4c85b91d10a27f53eedb4 100644 (file)
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -215,6 +215,7 @@ struct nfs_inode {
 #define NFS_INO_ADVISE_RDPLUS  (0)             /* advise readdirplus */
 #define NFS_INO_STALE          (1)             /* possible stale inode */
 #define NFS_INO_ACL_LRU_SET    (2)             /* Inode is on the LRU list */
+#define NFS_INO_INVALIDATING   (3)             /* inode is being invalidated */
 #define NFS_INO_FLUSHING       (4)             /* inode is flushing out data */
 #define NFS_INO_FSCACHE                (5)             /* inode can be cached by FS-Cache */
 #define NFS_INO_FSCACHE_LOCK   (6)             /* FS-Cache cookie management lock */