Linux NFS development
 help / color / mirror / Atom feed
From: NeilBrown <neilb@suse.com>
To: Trond Myklebust <trond.myklebust@primarydata.com>,
	Anna Schumaker <anna.schumaker@netapp.com>
Cc: linux-nfs@vger.kernel.org
Subject: [PATCH 6/8] NFS: flush data when locking a file to ensure cache coherence for mmap.
Date: Fri, 18 Aug 2017 17:12:52 +1000	[thread overview]
Message-ID: <150304037202.30218.1015616186302440289.stgit@noble> (raw)
In-Reply-To: <150304014011.30218.1636255532744321171.stgit@noble>

When a byte range lock (or flock) is taken out on an NFS file, the
validity of the cached data is checked and the inode is marked
NFS_INODE_INVALID_DATA.  However the cached data isn't flushed from
the page cache.

This is sufficient for future read() requests or mmap() requests as
they call nfs_revalidate_mapping() which performs the flush if
necessary.

However an existing mapping is not affected.  Accessing data through
that mapping will continue to return old data even though the inode is
marked NFS_INODE_INVALID_DATA.

This can easily be confirmed using the 'nfs' tool in
  git://github.com/okirch/twopence-nfs.git
and running

   nfs coherence FILENAME
on one client, and
   nfs coherence -r FILENAME
on another client.

It appears that prior to Linux 2.6.0 this worked correctly.

However commit:

http://git.kernel.org/cgit/linux/kernel/git/history/history.git/commit/?id=ca9268fe3ddd075714005adecd4afbd7f9ab87d0

removed the call to inode_invalidate_pages() from nfs_zap_caches().  I
haven't tested this code, but inspection suggests that prior to this
commit, file locking would invalidate all inode pages.

This patch adds a call to nfs_revalidate_mapping() after a
successful SETLK so that invalid data is flushed.  With this patch the
above test passes.  To minimize impact (and possibly avoid a GETATTR
call) this only happens if the mapping might be mapped into
userspace.

Cc: Olaf Kirch <okir@suse.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfs/file.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index aa883d8b24e6..d67cd3dbfa67 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -750,15 +750,18 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 		goto out;
 
 	/*
-	 * Revalidate the cache if the server has time stamps granular
-	 * enough to detect subsecond changes.  Otherwise, clear the
-	 * cache to prevent missing any changes.
+	 * Invalidate cache to prevent missing any changes.  If
+	 * the file is mapped, clear the page cache as well so
+	 * those mappings will be loaded.
 	 *
 	 * This makes locking act as a cache coherency point.
 	 */
 	nfs_sync_mapping(filp->f_mapping);
-	if (!NFS_PROTO(inode)->have_delegation(inode, FMODE_READ))
+	if (!NFS_PROTO(inode)->have_delegation(inode, FMODE_READ)) {
 		nfs_zap_caches(inode);
+		if (mapping_mapped(filp->f_mapping))
+			nfs_revalidate_mapping(inode, filp->f_mapping);
+	}
 out:
 	return status;
 }



  parent reply	other threads:[~2017-08-18  7:14 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-18  7:12 [PATCH 0/8] Assorted NFS patches NeilBrown
2017-08-18  7:12 ` [PATCH 4/8] NFS: flush out dirty data on file fput() NeilBrown
2017-08-18 19:15   ` Trond Myklebust
2017-08-19  1:02     ` NeilBrown
2017-08-19  1:27       ` Trond Myklebust
2017-08-21  1:35         ` NeilBrown
2017-08-23 11:12           ` Jeff Layton
2017-08-18  7:12 ` [PATCH 1/8] SUNRPC: ECONNREFUSED should cause a rebind NeilBrown
2017-08-18 14:40   ` Chuck Lever
2017-08-18  7:12 ` [PATCH 2/8] NFSv4: don't let hanging mounts block other mounts NeilBrown
2017-08-18 19:05   ` Trond Myklebust
2017-08-19  0:50     ` NeilBrown
2017-08-18  7:12 ` [PATCH 3/8] NFS: don't expect errors from mempool_alloc() NeilBrown
2017-08-18  7:12 ` NeilBrown [this message]
2017-08-18  7:12 ` [PATCH 8/8] NFSv4.1: don't use machine credentials for CLOSE when using 'sec=sys' NeilBrown
2017-08-18  7:12 ` [PATCH 5/8] SUNRPC: remove some dead code NeilBrown
2017-08-18  7:12 ` [PATCH 7/8] NFS: remove error handling for callers of rpc_new_task() NeilBrown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=150304037202.30218.1015616186302440289.stgit@noble \
    --to=neilb@suse.com \
    --cc=anna.schumaker@netapp.com \
    --cc=linux-nfs@vger.kernel.org \
    --cc=trond.myklebust@primarydata.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox