Return-Path: Received: from mail-pz0-f46.google.com ([209.85.210.46]:38822 "EHLO mail-pz0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752269Ab0HSX4H (ORCPT ); Thu, 19 Aug 2010 19:56:07 -0400 Date: Fri, 20 Aug 2010 07:55:53 +0800 From: Wu Fengguang To: Christoph Hellwig Cc: Jeff Layton , linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, Chris Mason , Jens Axboe Subject: Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? Message-ID: <20100819235553.GB22747@localhost> References: <20100819101525.076831ad@barsoom.rdu.redhat.com> <20100819143710.GA4752@infradead.org> Content-Type: text/plain; charset=us-ascii In-Reply-To: <20100819143710.GA4752@infradead.org> Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 On Thu, Aug 19, 2010 at 10:37:10AM -0400, Christoph Hellwig wrote: > On Thu, Aug 19, 2010 at 10:15:25AM -0400, Jeff Layton wrote: > > I'm looking at backporting some upstream changes to earlier kernels, > > and ran across something I don't quite understand... > > > > In nfs_commit_unstable_pages, we set the flags to FLUSH_SYNC. We then > > zero out the flags if wbc->nonblocking or wbc->for_background is set. > > > > Shouldn't we also clear it out if wbc->sync_mode == WB_SYNC_NONE ? > > WB_SYNC_NONE means "don't wait on anything", so shouldn't that include > > not waiting on the COMMIT to complete? > > I've been trying to figure out what the nonblocking flag is supposed > to mean for a while now. > > It basically disappeared in commit 0d99519efef15fd0cf84a849492c7b1deee1e4b7 > > "writeback: remove unused nonblocking and congestion checks" > > from Wu. What's left these days is a couple of places in local copies > of write_cache_pages (afs, cifs), and a couple of checks in random > writepages instances (afs, block_write_full_page, ceph, nfs, reiserfs, xfs) > and the use in nfs_write_inode. In principle all nonblocking checks in ->writepages should be removed. (My original patch does have chunks for afs/cifs that somehow get dropped in the process, and missed ceph because it's not upstream when I started patch..) > It's only actually set for memory > migration and pageout, that is VM writeback. > > To me it really doesn't make much sense, but maybe someone has a better > idea what it is for. Since migration and pageout still set nonblocking for ->writepage, we may keep them in the near future, until VM does not start IO on itself. > > + if (wbc->nonblocking || wbc->for_background || > > + wbc->sync_mode == WB_SYNC_NONE) > > You could remove the nonblocking and for_background checks as > these impliy WB_SYNC_NONE. Agreed. Thanks, Fengguang --- writeback: remove useless nonblocking checks in ->writepages This removes more deadcode that was somehow missed by commit 0d99519efef (writeback: remove unused nonblocking and congestion checks). The nonblocking checks in ->writepages are no longer used because the flusher now prefer to block on get_request_wait() than to skip inodes on IO congestion. The latter will lead to more seeky IO. CC: Chris Mason CC: Jens Axboe CC: Christoph Hellwig Signed-off-by: Wu Fengguang --- fs/afs/write.c | 16 +--------------- fs/cifs/file.c | 10 ---------- 2 files changed, 1 insertion(+), 25 deletions(-) --- linux-next.orig/fs/afs/write.c 2010-06-24 14:32:01.000000000 +0800 +++ linux-next/fs/afs/write.c 2010-08-20 07:03:01.000000000 +0800 @@ -455,8 +455,6 @@ int afs_writepage(struct page *page, str } wbc->nr_to_write -= ret; - if (wbc->nonblocking && bdi_write_congested(bdi)) - wbc->encountered_congestion = 1; _leave(" = 0"); return 0; @@ -529,11 +527,6 @@ static int afs_writepages_region(struct wbc->nr_to_write -= ret; - if (wbc->nonblocking && bdi_write_congested(bdi)) { - wbc->encountered_congestion = 1; - break; - } - cond_resched(); } while (index < end && wbc->nr_to_write > 0); @@ -554,18 +547,11 @@ int afs_writepages(struct address_space _enter(""); - if (wbc->nonblocking && bdi_write_congested(bdi)) { - wbc->encountered_congestion = 1; - _leave(" = 0 [congest]"); - return 0; - } - if (wbc->range_cyclic) { start = mapping->writeback_index; end = -1; ret = afs_writepages_region(mapping, wbc, start, end, &next); - if (start > 0 && wbc->nr_to_write > 0 && ret == 0 && - !(wbc->nonblocking && wbc->encountered_congestion)) + if (start > 0 && wbc->nr_to_write > 0 && ret == 0) ret = afs_writepages_region(mapping, wbc, 0, start, &next); mapping->writeback_index = next; --- linux-next.orig/fs/cifs/file.c 2010-08-20 06:57:11.000000000 +0800 +++ linux-next/fs/cifs/file.c 2010-08-20 07:03:01.000000000 +0800 @@ -1379,16 +1379,6 @@ static int cifs_writepages(struct addres return generic_writepages(mapping, wbc); - /* - * BB: Is this meaningful for a non-block-device file system? - * If it is, we should test it again after we do I/O - */ - if (wbc->nonblocking && bdi_write_congested(bdi)) { - wbc->encountered_congestion = 1; - kfree(iov); - return 0; - } - xid = GetXid(); pagevec_init(&pvec, 0);