Is nfs_flush_incompatible to heavy-handed.

Commit Message

NeilBrown May 28, 2013, 1:58 a.m. UTC

The nfs client code has a function "nfs_flush_incompatible" which is called
when a process writes to a page to see if the page is already dirty in a way
that means it should be flushed before the write is permitted.

As I understand it, this is needed so that if two different credentials are
used to write to a file, it is important that the server see the different
writes each with their own credentials.  Similarly if two different processes
have different locks, then the writes they send should be kept separate.

However if two processes both have the same credentials and neither are
holding any locks on the file, then there can be no need to flush writes from
one before writes from the other are allowed.

I have a customer who has hit exactly this problem.  It particularly involves
mmapped files.

Two different processes both mmap a file which happens to be stored on NFS.
They both update the same pages (with some independent synchronisation) and
when this happens each update requires that the change made by the other
process be flushed first.

This was did not happen before 2.6.24, but since the addition of
nfs_vm_page_mkwrite, it has a serious impact on performance.

So my question is: is it safe to make nfs_flush_incompatible" a bit less
heavy handed, and in particular allow different processes with the same
credentials to update the same page without flushing.

The patch I have been experimenting with is below.  It probably don't help on
NFSv4 because there probably needs to be some care taken with different
'state' values.  But my customer uses NFSv3 so that is what I focussed on.

Any ideas?

Thanks,
NeilBrown

Patch

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index c483cc5..df40c57 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -860,11 +860,16 @@  int nfs_flush_incompatible(struct file *file, struct page *page)
 		if (req == NULL)
 			return 0;
 		l_ctx = req->wb_lock_context;
-		do_flush = req->wb_page != page || req->wb_context != ctx;
-		if (l_ctx) {
+
+		do_flush = req->wb_page != page;
+		if (req->wb_context != ctx)
+			do_flush |=
+				(req->wb_context->dentry != ctx->dentry ||
+				 req->wb_context->cred != ctx->cred );
+		if (l_ctx && (req->wb_context->state || ctx->state))
 			do_flush |= l_ctx->lockowner.l_owner != current->files
 				|| l_ctx->lockowner.l_pid != current->tgid;
-		}
+
 		nfs_release_request(req);
 		if (!do_flush)
 			return 0;