Return-Path: linux-nfs-owner@vger.kernel.org Received: from mx1.redhat.com ([209.132.183.28]:44919 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751823Ab3FYTPc (ORCPT ); Tue, 25 Jun 2013 15:15:32 -0400 Date: Tue, 25 Jun 2013 15:15:29 -0400 From: Jeff Layton To: Scott Mayhew Cc: "Myklebust, Trond" , "linux-nfs@vger.kernel.org" Subject: 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 Message-ID: <20130625151529.219acce9@tlielax.poochiereds.net> In-Reply-To: <20130604132149.GL55330@tonberry.usersys.redhat.com> References: <1369346021-20041-1-git-send-email-smayhew@redhat.com> <1369346021-20041-2-git-send-email-smayhew@redhat.com> <20130523182450.18adbcd8@tlielax.poochiereds.net> <1369348209.8861.12.camel@leira.trondhjem.org> <20130524072403.6b814585@corrin.poochiereds.net> <20130604132149.GL55330@tonberry.usersys.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-nfs-owner@vger.kernel.org List-ID: On Tue, 4 Jun 2013 09:21:49 -0400 Scott Mayhew wrote: > From: Scott Mayhew > To: Jeff Layton > Cc: "Myklebust, Trond" , "linux-nfs@vger.kernel.org" > Subject: 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 > Date: Tue, 4 Jun 2013 09:21:49 -0400 > Sender: linux-nfs-owner@vger.kernel.org > User-Agent: Mutt/1.5.20 (2009-06-14) > > On Fri, 24 May 2013, Jeff Layton wrote: > > > On Thu, 23 May 2013 22:30:10 +0000 > > "Myklebust, Trond" wrote: > > > > > On Thu, 2013-05-23 at 18:24 -0400, Jeff Layton wrote: > > > > On Thu, 23 May 2013 17:53:41 -0400 > > > > Scott Mayhew 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 > > > > > --- > > > > > 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 > > From 3938f17ef84f5c4889fd7f827109f89c932df569 Mon Sep 17 00:00:00 2001 > From: Scott Mayhew > 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 > --- > 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; > } Sorry I didn't chime in on this before. Looks sane to me... Reviewed-by: Jeff Layton