From: Wu Fengguang <fengguang.wu@gmail.com> To: Christoph Hellwig <hch@infradead.org> Cc: Jeff Layton <jlayton@redhat.com>, linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, Chris Mason <chris.mason@oracle.com>, Jens Axboe <jens.axboe@oracle.com> Subject: Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? Date: Fri, 20 Aug 2010 07:55:53 +0800 [thread overview] Message-ID: <20100819235553.GB22747@localhost> (raw) In-Reply-To: <20100819143710.GA4752@infradead.org> 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 <chris.mason@oracle.com> CC: Jens Axboe <jens.axboe@oracle.com> CC: Christoph Hellwig <hch@infradead.org> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> --- 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); -- 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>
WARNING: multiple messages have this Message-ID (diff)
From: Wu Fengguang <fengguang.wu@gmail.com> To: Christoph Hellwig <hch@infradead.org> Cc: Jeff Layton <jlayton@redhat.com>, linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, Chris Mason <chris.mason@oracle.com>, Jens Axboe <jens.axboe@oracle.com> Subject: Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? Date: Fri, 20 Aug 2010 07:55:53 +0800 [thread overview] Message-ID: <20100819235553.GB22747@localhost> (raw) In-Reply-To: <20100819143710.GA4752@infradead.org> 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 <chris.mason@oracle.com> CC: Jens Axboe <jens.axboe@oracle.com> CC: Christoph Hellwig <hch@infradead.org> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> --- 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);
next prev parent reply other threads:[~2010-08-19 23:55 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 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 [this message] 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=20100819235553.GB22747@localhost \ --to=fengguang.wu@gmail.com \ --cc=chris.mason@oracle.com \ --cc=hch@infradead.org \ --cc=jens.axboe@oracle.com \ --cc=jlayton@redhat.com \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linux-nfs@vger.kernel.org \ /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.