Re: [PATCH RFC 1/1] NFS: Allow nfs_updatepage to extend a write to cover a full page when we have a lock that covers the entire file

[Scott Mayhew <smayhew@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3290 bytes --]

On Fri, 24 May 2013, Jeff Layton wrote:

> On Thu, 23 May 2013 22:30:10 +0000
> "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
> 
> > On Thu, 2013-05-23 at 18:24 -0400, Jeff Layton wrote:
> > > On Thu, 23 May 2013 17:53:41 -0400
> > > Scott Mayhew <smayhew@redhat.com> wrote:
> > > 
> > > > Currently nfs_updatepage allows a write to be extended to cover a full
> > > > page only if we don't have a byte range lock on the file... but if we've
> > > > got the whole file locked, then we should be allowed to extend the
> > > > write.
> > > > 
> > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > > > ---
> > > >  fs/nfs/write.c | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> > > > index a2c7c28..f35fb4f 100644
> > > > --- a/fs/nfs/write.c
> > > > +++ b/fs/nfs/write.c
> > > > @@ -908,13 +908,16 @@ int nfs_updatepage(struct file *file, struct page *page,
> > > >  		file->f_path.dentry->d_name.name, count,
> > > >  		(long long)(page_file_offset(page) + offset));
> > > >  
> > > > -	/* If we're not using byte range locks, and we know the page
> > > > +	/* If we're not using byte range locks (or if the range of the
> > > > +	 * lock covers the entire file), and we know the page
> > > >  	 * is up to date, it may be more efficient to extend the write
> > > >  	 * to cover the entire page in order to avoid fragmentation
> > > >  	 * inefficiencies.
> > > >  	 */
> > > >  	if (nfs_write_pageuptodate(page, inode) &&
> > > > -			inode->i_flock == NULL &&
> > > > +			(inode->i_flock == NULL ||
> > > > +			(inode->i_flock->fl_start == 0 &&
> > > > +			inode->i_flock->fl_end == OFFSET_MAX)) &&
> > > >  			!(file->f_flags & O_DSYNC)) {
> > > >  		count = max(count + offset, nfs_page_length(page));
> > > >  		offset = 0;
> > > 
> > > Sounds like a reasonable proposition, but I think you might need to do
> > > more vetting of the locks...
> > > 
> > > For instance, does it make sense to do this if it's a F_RDLCK? Also,
> > > you're only looking at the first lock in the i_flock list. Might it
> > > make more sense to walk the list and see whether the page might be
> > > entirely covered by a lock that doesn't extend over the whole file?
> > > 
> > 
> > I'm guessing that the answer is to both these questions are "no":
> > - Anybody who is writing while holding a F_RDLCK is likely doing
> > something wrong.
> 
> Right, so I think we ought to be conservative here and not extend the
> write if this is an F_RDLCK.
> 
> > - Walking the lock list on every write can quickly get painful if we
> > have lots of small locks.
> > 
> 
> True, but it's probably still preferable to do that than to do a bunch
> of small I/Os to the server. But, that's an optimization that can be
> done later. Hardly anyone does real byte-range locking so I'm fine with
> this approach for now.
> 
> > However it may make a lot of sense to look at whether or not we hold a
> > NFSv4 write delegation.
> > 
> 
> Yes, that would be a good thing too. Having a helper function like you
> suggested should make it easier to encapsulate that logic sanely.
> 
Here's an updated patch that moves the logic to a helper function,
checks to see if we have a write delegation, and checks the lock type.

-Scott

[-- Attachment #2: 0001-NFS-Allow-nfs_updatepage-to-extend-a-write-under-add.patch --]
[-- Type: text/plain, Size: 2417 bytes --]

>From 3938f17ef84f5c4889fd7f827109f89c932df569 Mon Sep 17 00:00:00 2001
From: Scott Mayhew <smayhew@redhat.com>
Date: Wed, 22 May 2013 17:03:17 -0400
Subject: [PATCH RFC] NFS: Allow nfs_updatepage to extend a write under
 additional circumstances

Currently nfs_updatepage allows a write to be extended to cover a full
page only if we don't have a byte range lock lock on the file... but if
we have a write delegation on the file or if we have the whole file
locked for writing then we should be allowed to extend the write as
well.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 fs/nfs/write.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index a2c7c28..c8a1bcc 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -888,6 +888,28 @@ out:
 	return PageUptodate(page) != 0;
 }
 
+/* If we know the page is up to date, and we're not using byte range locks (or
+ * if we have the whole file locked for writing), it may be more efficient to
+ * extend the write to cover the entire page in order to avoid fragmentation
+ * inefficiencies.
+ *
+ * If the file is opened for synchronous writes or if we have a write delegation
+ * from the server then we can just skip the rest of the checks.
+ */
+static int nfs_can_extend_write(struct file *file, struct page *page, struct inode *inode)
+{
+	if (file->f_flags & O_DSYNC)
+		return 0;
+	if (nfs_have_delegation(inode, FMODE_WRITE))
+		return 1;
+	if (nfs_write_pageuptodate(page, inode) && (inode->i_flock == NULL ||
+			(inode->i_flock->fl_start == 0 &&
+			inode->i_flock->fl_end == OFFSET_MAX &&
+			inode->i_flock->fl_type != F_RDLCK)))
+		return 1;
+	return 0;
+}
+
 /*
  * Update and possibly write a cached page of an NFS file.
  *
@@ -908,14 +930,7 @@ int nfs_updatepage(struct file *file, struct page *page,
 		file->f_path.dentry->d_name.name, count,
 		(long long)(page_file_offset(page) + offset));
 
-	/* If we're not using byte range locks, and we know the page
-	 * is up to date, it may be more efficient to extend the write
-	 * to cover the entire page in order to avoid fragmentation
-	 * inefficiencies.
-	 */
-	if (nfs_write_pageuptodate(page, inode) &&
-			inode->i_flock == NULL &&
-			!(file->f_flags & O_DSYNC)) {
+	if (nfs_can_extend_write(file, page, inode)) {
 		count = max(count + offset, nfs_page_length(page));
 		offset = 0;
 	}
-- 
1.7.11.7