nfs-backed mmap file results in 1000s of WRITEs per second

Commit Message

Trond Myklebust Sept. 6, 2013, 3:39 p.m. UTC

On Fri, 2013-09-06 at 11:04 -0400, Jeff Layton wrote:
> On Fri, 6 Sep 2013 15:00:56 +0000

> "Myklebust, Trond" <[e-mail]> wrote:

> > > Also, do you need to do a similar fix to nfs_can_coalesce_requests?

> > 

> > Yes. Good point!

> > 

> 

> Cool. FWIW, here's the original bug that was opened against RHEL5:

> 

>     https://bugzilla.redhat.com/show_bug.cgi?id=736578

> 

> ...the reproducer that Max cooked up is not doing mmapped I/O so there

> may be a difference there, but I haven't looked closely at why that is.


Here is the patch...

There should be no big differences between mmapped I/O and ordinary I/O
now that we have page_mkwrite() to set up the request. The only
difference that I can think of offhand is when you use locking: for
mmap() writes, NFS can only guarantee data integrity if you lock the
entire page.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
[e-mail]
www.netapp.com

Comments

William Dauchy Sept. 8, 2013, 2:25 p.m. UTC | #1

On Fri, Sep 6, 2013 at 5:39 PM, Myklebust, Trond
<[e-mail]> wrote:
> There should be no big differences between mmapped I/O and ordinary I/O
> now that we have page_mkwrite() to set up the request. The only
> difference that I can think of offhand is when you use locking: for
> mmap() writes, NFS can only guarantee data integrity if you lock the
> entire page.

Tested the two patches on top of a 3.10.x. Looks ok so far.

Patch

From 4109bb7496640aa97a12904527ba8e3a19b7ce7a Mon Sep 17 00:00:00 2001
From: Trond Myklebust <[e-mail]>
Date: Fri, 6 Sep 2013 11:09:38 -0400
Subject: [PATCH] NFS: Don't check lock owner compatability unless file is
 locked (part 2)

When coalescing requests into a single READ or WRITE RPC call, and there
is no file locking involved, we don't have to refuse coalescing for
requests where the lock owner information doesn't match.

Reported-by: Jeff Layton <[e-mail]>
Signed-off-by: Trond Myklebust <[e-mail]>
---
 fs/nfs/pagelist.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 29cfb7a..2ffebf2 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -328,6 +328,19 @@  void nfs_pageio_init(struct nfs_pageio_descriptor *desc,
 }
 EXPORT_SYMBOL_GPL(nfs_pageio_init);
 
+static bool nfs_match_open_context(const struct nfs_open_context *ctx1,
+		const struct nfs_open_context *ctx2)
+{
+	return ctx1->cred == ctx2->cred && ctx1->state == ctx2->state;
+}
+
+static bool nfs_match_lock_context(const struct nfs_lock_context *l1,
+		const struct nfs_lock_context *l2)
+{
+	return l1->lockowner.l_owner == l2->lockowner.l_owner
+		&& l1->lockowner.l_pid == l2->lockowner.l_pid;
+}
+
 /**
  * nfs_can_coalesce_requests - test two requests for compatibility
  * @prev: pointer to nfs_page
@@ -343,13 +356,10 @@  static bool nfs_can_coalesce_requests(struct nfs_page *prev,
 				      struct nfs_page *req,
 				      struct nfs_pageio_descriptor *pgio)
 {
-	if (req->wb_context->cred != prev->wb_context->cred)
-		return false;
-	if (req->wb_lock_context->lockowner.l_owner != prev->wb_lock_context->lockowner.l_owner)
-		return false;
-	if (req->wb_lock_context->lockowner.l_pid != prev->wb_lock_context->lockowner.l_pid)
+	if (!nfs_match_open_context(req->wb_context, prev->wb_context))
 		return false;
-	if (req->wb_context->state != prev->wb_context->state)
+	if (req->wb_context->dentry->d_inode->i_flock != NULL &&
+	    !nfs_match_lock_context(req->wb_lock_context, prev->wb_lock_context))
 		return false;
 	if (req->wb_pgbase != 0)
 		return false;
-- 
1.8.3.1