On Fri, 20 Aug 2010 08:33:08 +0800
Wu Fengguang <fengguang.wu@xxxxxxxxx> wrote:
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>
> Thanks,
> Fengguang
Either way. I figured it would be slightly more efficient to just check
WB_SYNC_NONE once in that function. I suppose we could just fix up the
comments instead. Let me know if I should respin the patch with updated
comments...
Thanks for the review...
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>