From: Wu Fengguang <fengguang.wu@gmail.com> To: Jeff Layton <jlayton@redhat.com> Cc: Trond Myklebust <trond.myklebust@fys.uio.no>, Christoph Hellwig <hch@infradead.org>, linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org Subject: Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? Date: Fri, 20 Aug 2010 08:33:08 +0800 [thread overview] Message-ID: <20100820003308.GA30548@localhost> (raw) In-Reply-To: <20100819151618.5f769dc9@tlielax.poochiereds.net> > Here's a lightly tested patch that turns the check for the two flags > into a check for WB_SYNC_NONE. It seems to do the right thing, but I > don't have a clear testcase for it. Does this look reasonable? Yes, I don't see any problems. > ------------------[snip]------------------------ > > NFS: don't use FLUSH_SYNC on WB_SYNC_NONE COMMIT calls > > WB_SYNC_NONE is supposed to mean "don't wait on anything". That should > also include not waiting for COMMIT calls to complete. > > WB_SYNC_NONE is also implied when wbc->nonblocking or > wbc->for_background are set, so we can replace those checks in > nfs_commit_unstable_pages with a check for WB_SYNC_NONE. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/nfs/write.c | 10 +++++----- > 1 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/fs/nfs/write.c b/fs/nfs/write.c > index 874972d..35bd7d0 100644 > --- a/fs/nfs/write.c > +++ b/fs/nfs/write.c > @@ -1436,12 +1436,12 @@ static int nfs_commit_unstable_pages(struct inode *inode, struct writeback_contr > /* Don't commit yet if this is a non-blocking flush and there are > * lots of outstanding writes for this mapping. > */ > - if (wbc->sync_mode == WB_SYNC_NONE && > - nfsi->ncommit <= (nfsi->npages >> 1)) > - goto out_mark_dirty; > - > - if (wbc->nonblocking || wbc->for_background) > + if (wbc->sync_mode == WB_SYNC_NONE) { > + if (nfsi->ncommit <= (nfsi->npages >> 1)) > + goto out_mark_dirty; > flags = 0; > + } > + nitpick: I'd slightly prefer an one-line change - if (wbc->nonblocking || wbc->for_background) + if (wbc->sync_mode == WB_SYNC_NONE) flags = 0; That way the patch will look more obvious and "git blame" friendly, and the original "Don't commit.." comment will best match its code. Reviewed-by: Wu Fengguang <fengguang.wu@intel.com> Thanks, Fengguang
WARNING: multiple messages have this Message-ID (diff)
From: Wu Fengguang <fengguang.wu@gmail.com> To: Jeff Layton <jlayton@redhat.com> Cc: Trond Myklebust <trond.myklebust@fys.uio.no>, Christoph Hellwig <hch@infradead.org>, linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org Subject: Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? Date: Fri, 20 Aug 2010 08:33:08 +0800 [thread overview] Message-ID: <20100820003308.GA30548@localhost> (raw) In-Reply-To: <20100819151618.5f769dc9@tlielax.poochiereds.net> > Here's a lightly tested patch that turns the check for the two flags > into a check for WB_SYNC_NONE. It seems to do the right thing, but I > don't have a clear testcase for it. Does this look reasonable? Yes, I don't see any problems. > ------------------[snip]------------------------ > > NFS: don't use FLUSH_SYNC on WB_SYNC_NONE COMMIT calls > > WB_SYNC_NONE is supposed to mean "don't wait on anything". That should > also include not waiting for COMMIT calls to complete. > > WB_SYNC_NONE is also implied when wbc->nonblocking or > wbc->for_background are set, so we can replace those checks in > nfs_commit_unstable_pages with a check for WB_SYNC_NONE. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/nfs/write.c | 10 +++++----- > 1 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/fs/nfs/write.c b/fs/nfs/write.c > index 874972d..35bd7d0 100644 > --- a/fs/nfs/write.c > +++ b/fs/nfs/write.c > @@ -1436,12 +1436,12 @@ static int nfs_commit_unstable_pages(struct inode *inode, struct writeback_contr > /* Don't commit yet if this is a non-blocking flush and there are > * lots of outstanding writes for this mapping. > */ > - if (wbc->sync_mode == WB_SYNC_NONE && > - nfsi->ncommit <= (nfsi->npages >> 1)) > - goto out_mark_dirty; > - > - if (wbc->nonblocking || wbc->for_background) > + if (wbc->sync_mode == WB_SYNC_NONE) { > + if (nfsi->ncommit <= (nfsi->npages >> 1)) > + goto out_mark_dirty; > flags = 0; > + } > + nitpick: I'd slightly prefer an one-line change - if (wbc->nonblocking || wbc->for_background) + if (wbc->sync_mode == WB_SYNC_NONE) flags = 0; That way the patch will look more obvious and "git blame" friendly, and the original "Don't commit.." comment will best match its code. Reviewed-by: Wu Fengguang <fengguang.wu@intel.com> Thanks, Fengguang -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2010-08-20 0:33 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-08-19 14:15 why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? Jeff Layton [not found] ` <20100819101525.076831ad-xSBYVWDuneFaJnirhKH9O4GKTjYczspe@public.gmane.org> 2010-08-19 14:37 ` Christoph Hellwig 2010-08-19 14:37 ` Christoph Hellwig 2010-08-19 14:37 ` Christoph Hellwig 2010-08-19 14:58 ` Trond Myklebust 2010-08-19 14:58 ` Trond Myklebust 2010-08-19 15:11 ` Jeff Layton 2010-08-19 15:11 ` Jeff Layton [not found] ` <1282229905.6199.19.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org> 2010-08-19 15:24 ` Christoph Hellwig 2010-08-19 15:24 ` Christoph Hellwig 2010-08-19 15:24 ` Christoph Hellwig 2010-08-19 19:16 ` Jeff Layton 2010-08-19 19:16 ` Jeff Layton 2010-08-19 19:16 ` Jeff Layton [not found] ` <20100819151618.5f769dc9-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org> 2010-08-19 19:43 ` Trond Myklebust 2010-08-19 19:43 ` Trond Myklebust 2010-08-19 19:43 ` Trond Myklebust [not found] ` <1282246999.7799.66.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org> 2010-08-20 13:23 ` Wu Fengguang 2010-08-20 13:23 ` Wu Fengguang 2010-08-20 13:23 ` Wu Fengguang 2010-08-30 19:22 ` Trond Myklebust 2010-08-30 19:22 ` Trond Myklebust 2010-08-30 19:22 ` Trond Myklebust 2010-08-30 23:53 ` Wu Fengguang 2010-08-30 23:53 ` Wu Fengguang 2010-08-20 0:33 ` Wu Fengguang [this message] 2010-08-20 0:33 ` Wu Fengguang 2010-08-20 0:53 ` Jeff Layton 2010-08-20 0:53 ` Jeff Layton 2010-08-20 13:20 ` Wu Fengguang 2010-08-20 13:20 ` Wu Fengguang 2010-08-19 23:55 ` Wu Fengguang 2010-08-19 23:55 ` Wu Fengguang 2010-08-20 0:02 ` Wu Fengguang 2010-08-20 0:02 ` Wu Fengguang 2010-08-20 2:36 ` Sage Weil 2010-08-20 2:36 ` Sage Weil 2010-08-20 9:19 ` Christoph Hellwig 2010-08-20 9:19 ` Christoph Hellwig [not found] ` <20100820091904.GB20138-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> 2010-08-20 11:27 ` Jeff Layton 2010-08-20 11:27 ` Jeff Layton 2010-08-20 11:27 ` Jeff Layton 2010-08-20 12:44 ` Wu Fengguang 2010-08-20 12:44 ` Wu Fengguang 2010-08-20 12:26 ` Wu Fengguang 2010-08-20 12:26 ` Wu Fengguang 2010-08-20 12:26 ` Wu Fengguang
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=20100820003308.GA30548@localhost \ --to=fengguang.wu@gmail.com \ --cc=hch@infradead.org \ --cc=jlayton@redhat.com \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linux-nfs@vger.kernel.org \ --cc=trond.myklebust@fys.uio.no \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.