* why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-19 14:15 Jeff Layton [not found] ` <20100819101525.076831ad-xSBYVWDuneFaJnirhKH9O4GKTjYczspe@public.gmane.org> 0 siblings, 1 reply; 47+ messages in thread From: Jeff Layton @ 2010-08-19 14:15 UTC (permalink / raw) To: linux-nfs 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? For discussion purposes, here's a patch that shows what I'm talking about (completely untested): diff --git a/fs/nfs/write.c b/fs/nfs/write.c index 874972d..2fca906 100644 --- a/fs/nfs/write.c +++ b/fs/nfs/write.c @@ -1440,7 +1440,8 @@ static int nfs_commit_unstable_pages(struct inode *inode, struct writeback_contr nfsi->ncommit <= (nfsi->npages >> 1)) goto out_mark_dirty; - if (wbc->nonblocking || wbc->for_background) + if (wbc->nonblocking || wbc->for_background || + wbc->sync_mode == WB_SYNC_NONE) flags = 0; ret = nfs_commit_inode(inode, flags); if (ret >= 0) { -- Jeff Layton <jlayton@redhat.com> ^ permalink raw reply related [flat|nested] 47+ messages in thread
[parent not found: <20100819101525.076831ad-xSBYVWDuneFaJnirhKH9O4GKTjYczspe@public.gmane.org>]
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 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 0 siblings, 0 replies; 47+ messages in thread From: Christoph Hellwig @ 2010-08-19 14:37 UTC (permalink / raw) To: Jeff Layton, fengguang.wu-Re5JQEeQqe8AvxtiuMwx3w Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg 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. 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. > + 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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-19 14:37 ` Christoph Hellwig 0 siblings, 0 replies; 47+ messages in thread From: Christoph Hellwig @ 2010-08-19 14:37 UTC (permalink / raw) To: Jeff Layton, fengguang.wu; +Cc: linux-nfs, linux-fsdevel, linux-mm 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. 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. > + 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. -- 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-19 14:37 ` Christoph Hellwig 0 siblings, 0 replies; 47+ messages in thread From: Christoph Hellwig @ 2010-08-19 14:37 UTC (permalink / raw) To: Jeff Layton, fengguang.wu; +Cc: linux-nfs, linux-fsdevel, linux-mm 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. 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. > + 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-19 14:37 ` Christoph Hellwig @ 2010-08-19 14:58 ` Trond Myklebust -1 siblings, 0 replies; 47+ messages in thread From: Trond Myklebust @ 2010-08-19 14:58 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, fengguang.wu, linux-nfs, linux-fsdevel, linux-mm On Thu, 2010-08-19 at 10:37 -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. 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. > > > + 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. To me that sounds fine. I've also been trying to wrap my head around the differences between 'nonblocking', 'for_background', 'for_reclaim' and 'for_kupdate' and how the filesystem is supposed to treat them. Aside from the above, I've used 'for_reclaim', 'for_kupdate' and 'for_background' in order to adjust the RPC request's queuing priority (high in the case of 'for_reclaim' and low for the other two). Cheers Trond ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-19 14:58 ` Trond Myklebust 0 siblings, 0 replies; 47+ messages in thread From: Trond Myklebust @ 2010-08-19 14:58 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, fengguang.wu, linux-nfs, linux-fsdevel, linux-mm On Thu, 2010-08-19 at 10:37 -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. 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. > > > + 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. To me that sounds fine. I've also been trying to wrap my head around the differences between 'nonblocking', 'for_background', 'for_reclaim' and 'for_kupdate' and how the filesystem is supposed to treat them. Aside from the above, I've used 'for_reclaim', 'for_kupdate' and 'for_background' in order to adjust the RPC request's queuing priority (high in the case of 'for_reclaim' and low for the other two). Cheers Trond -- 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-19 14:58 ` Trond Myklebust @ 2010-08-19 15:11 ` Jeff Layton -1 siblings, 0 replies; 47+ messages in thread From: Jeff Layton @ 2010-08-19 15:11 UTC (permalink / raw) To: Trond Myklebust Cc: Christoph Hellwig, fengguang.wu, linux-nfs, linux-fsdevel, linux-mm On Thu, 19 Aug 2010 10:58:25 -0400 Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > On Thu, 2010-08-19 at 10:37 -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. 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. > > > > > + 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. > > To me that sounds fine. I've also been trying to wrap my head around the > differences between 'nonblocking', 'for_background', 'for_reclaim' and > 'for_kupdate' and how the filesystem is supposed to treat them. > > Aside from the above, I've used 'for_reclaim', 'for_kupdate' and > 'for_background' in order to adjust the RPC request's queuing priority > (high in the case of 'for_reclaim' and low for the other two). > Ok, I don't really have a great way to test the above change though aside from sticking it into the backport I'm working on for RHEL5 (2.6.18). I suspect that the existing flag checks probably cover a lot of the WB_SYNC_NONE cases already. Changing it to a check for WB_SYNC_NONE would help me as RHEL5 doesn't have the for_background flag... Cheers, -- Jeff Layton <jlayton@redhat.com> -- 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-19 15:11 ` Jeff Layton 0 siblings, 0 replies; 47+ messages in thread From: Jeff Layton @ 2010-08-19 15:11 UTC (permalink / raw) To: Trond Myklebust Cc: Christoph Hellwig, fengguang.wu, linux-nfs, linux-fsdevel, linux-mm On Thu, 19 Aug 2010 10:58:25 -0400 Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > On Thu, 2010-08-19 at 10:37 -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. 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. > > > > > + 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. > > To me that sounds fine. I've also been trying to wrap my head around the > differences between 'nonblocking', 'for_background', 'for_reclaim' and > 'for_kupdate' and how the filesystem is supposed to treat them. > > Aside from the above, I've used 'for_reclaim', 'for_kupdate' and > 'for_background' in order to adjust the RPC request's queuing priority > (high in the case of 'for_reclaim' and low for the other two). > Ok, I don't really have a great way to test the above change though aside from sticking it into the backport I'm working on for RHEL5 (2.6.18). I suspect that the existing flag checks probably cover a lot of the WB_SYNC_NONE cases already. Changing it to a check for WB_SYNC_NONE would help me as RHEL5 doesn't have the for_background flag... Cheers, -- Jeff Layton <jlayton@redhat.com> ^ permalink raw reply [flat|nested] 47+ messages in thread
[parent not found: <1282229905.6199.19.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org>]
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-19 14:58 ` Trond Myklebust (?) @ 2010-08-19 15:24 ` Christoph Hellwig -1 siblings, 0 replies; 47+ messages in thread From: Christoph Hellwig @ 2010-08-19 15:24 UTC (permalink / raw) To: Trond Myklebust Cc: Christoph Hellwig, Jeff Layton, fengguang.wu-Re5JQEeQqe8AvxtiuMwx3w, linux-nfs-u79uwXL29TY76Z2rM5mHXA, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg, hughd-hpIqsD4AKlfQT0dZR+AlfA, chris.mason-QHcLZuEGTsvQT0dZR+AlfA, konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg On Thu, Aug 19, 2010 at 10:58:25AM -0400, Trond Myklebust wrote: > To me that sounds fine. I've also been trying to wrap my head around the > differences between 'nonblocking', 'for_background', 'for_reclaim' and > 'for_kupdate' and how the filesystem is supposed to treat them. Yeah, it's not clear to me either. for_background is in fact only used in nfs, for the priority and the nfs_commit_inode flags, for_kupdate is only used in nfs, and in a really weird spot in btrfs, and for_reclaim is used in nfs, and two places in nilfs2 and in shmemfs. > Aside from the above, I've used 'for_reclaim', 'for_kupdate' and > 'for_background' in order to adjust the RPC request's queuing priority > (high in the case of 'for_reclaim' and low for the other two). Right now writepage calls to the filesystem can come from various places: - the flusher threads - VM reclaim (kswapd, memcg, direct reclaim) - memory migration - filemap_fdatawrite & other calls directly from FS code, also including fsync We have WB_SYNC_ALL set for the second, data integrity pass when doing a sync from the flusher threads, and when doing data integrity writes from fs context (most fsync but also a few others). All these obviously are high priority. It's not too easy to set priorities for the others in my opinion. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-19 15:24 ` Christoph Hellwig 0 siblings, 0 replies; 47+ messages in thread From: Christoph Hellwig @ 2010-08-19 15:24 UTC (permalink / raw) To: Trond Myklebust Cc: Christoph Hellwig, Jeff Layton, fengguang.wu, linux-nfs, linux-fsdevel, linux-mm, hughd, chris.mason, konishi.ryusuke On Thu, Aug 19, 2010 at 10:58:25AM -0400, Trond Myklebust wrote: > To me that sounds fine. I've also been trying to wrap my head around the > differences between 'nonblocking', 'for_background', 'for_reclaim' and > 'for_kupdate' and how the filesystem is supposed to treat them. Yeah, it's not clear to me either. for_background is in fact only used in nfs, for the priority and the nfs_commit_inode flags, for_kupdate is only used in nfs, and in a really weird spot in btrfs, and for_reclaim is used in nfs, and two places in nilfs2 and in shmemfs. > Aside from the above, I've used 'for_reclaim', 'for_kupdate' and > 'for_background' in order to adjust the RPC request's queuing priority > (high in the case of 'for_reclaim' and low for the other two). Right now writepage calls to the filesystem can come from various places: - the flusher threads - VM reclaim (kswapd, memcg, direct reclaim) - memory migration - filemap_fdatawrite & other calls directly from FS code, also including fsync We have WB_SYNC_ALL set for the second, data integrity pass when doing a sync from the flusher threads, and when doing data integrity writes from fs context (most fsync but also a few others). All these obviously are high priority. It's not too easy to set priorities for the others in my opinion. -- 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-19 15:24 ` Christoph Hellwig 0 siblings, 0 replies; 47+ messages in thread From: Christoph Hellwig @ 2010-08-19 15:24 UTC (permalink / raw) To: Trond Myklebust Cc: Christoph Hellwig, Jeff Layton, fengguang.wu, linux-nfs, linux-fsdevel, linux-mm, hughd, chris.mason, konishi.ryusuke On Thu, Aug 19, 2010 at 10:58:25AM -0400, Trond Myklebust wrote: > To me that sounds fine. I've also been trying to wrap my head around the > differences between 'nonblocking', 'for_background', 'for_reclaim' and > 'for_kupdate' and how the filesystem is supposed to treat them. Yeah, it's not clear to me either. for_background is in fact only used in nfs, for the priority and the nfs_commit_inode flags, for_kupdate is only used in nfs, and in a really weird spot in btrfs, and for_reclaim is used in nfs, and two places in nilfs2 and in shmemfs. > Aside from the above, I've used 'for_reclaim', 'for_kupdate' and > 'for_background' in order to adjust the RPC request's queuing priority > (high in the case of 'for_reclaim' and low for the other two). Right now writepage calls to the filesystem can come from various places: - the flusher threads - VM reclaim (kswapd, memcg, direct reclaim) - memory migration - filemap_fdatawrite & other calls directly from FS code, also including fsync We have WB_SYNC_ALL set for the second, data integrity pass when doing a sync from the flusher threads, and when doing data integrity writes from fs context (most fsync but also a few others). All these obviously are high priority. It's not too easy to set priorities for the others in my opinion. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-19 14:58 ` Trond Myklebust (?) @ 2010-08-19 19:16 ` Jeff Layton -1 siblings, 0 replies; 47+ messages in thread From: Jeff Layton @ 2010-08-19 19:16 UTC (permalink / raw) To: Trond Myklebust Cc: Christoph Hellwig, fengguang.wu-Re5JQEeQqe8AvxtiuMwx3w, linux-nfs-u79uwXL29TY76Z2rM5mHXA, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg On Thu, 19 Aug 2010 10:58:25 -0400 Trond Myklebust <trond.myklebust-41N18TsMXrtuMpJDpNschA@public.gmane.org> wrote: > On Thu, 2010-08-19 at 10:37 -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. 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. > > > > > + 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. > > To me that sounds fine. I've also been trying to wrap my head around the > differences between 'nonblocking', 'for_background', 'for_reclaim' and > 'for_kupdate' and how the filesystem is supposed to treat them. > > Aside from the above, I've used 'for_reclaim', 'for_kupdate' and > 'for_background' in order to adjust the RPC request's queuing priority > (high in the case of 'for_reclaim' and low for the other two). > 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? ------------------[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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> --- 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; + } + ret = nfs_commit_inode(inode, flags); if (ret >= 0) { if (wbc->sync_mode == WB_SYNC_NONE) { -- 1.5.5.6 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-19 19:16 ` Jeff Layton 0 siblings, 0 replies; 47+ messages in thread From: Jeff Layton @ 2010-08-19 19:16 UTC (permalink / raw) To: Trond Myklebust Cc: Christoph Hellwig, fengguang.wu, linux-nfs, linux-fsdevel, linux-mm On Thu, 19 Aug 2010 10:58:25 -0400 Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > On Thu, 2010-08-19 at 10:37 -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. 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. > > > > > + 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. > > To me that sounds fine. I've also been trying to wrap my head around the > differences between 'nonblocking', 'for_background', 'for_reclaim' and > 'for_kupdate' and how the filesystem is supposed to treat them. > > Aside from the above, I've used 'for_reclaim', 'for_kupdate' and > 'for_background' in order to adjust the RPC request's queuing priority > (high in the case of 'for_reclaim' and low for the other two). > 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? ------------------[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; + } + ret = nfs_commit_inode(inode, flags); if (ret >= 0) { if (wbc->sync_mode == WB_SYNC_NONE) { -- 1.5.5.6 -- 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> ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-19 19:16 ` Jeff Layton 0 siblings, 0 replies; 47+ messages in thread From: Jeff Layton @ 2010-08-19 19:16 UTC (permalink / raw) To: Trond Myklebust Cc: Christoph Hellwig, fengguang.wu, linux-nfs, linux-fsdevel, linux-mm On Thu, 19 Aug 2010 10:58:25 -0400 Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > On Thu, 2010-08-19 at 10:37 -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. 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. > > > > > + 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. > > To me that sounds fine. I've also been trying to wrap my head around the > differences between 'nonblocking', 'for_background', 'for_reclaim' and > 'for_kupdate' and how the filesystem is supposed to treat them. > > Aside from the above, I've used 'for_reclaim', 'for_kupdate' and > 'for_background' in order to adjust the RPC request's queuing priority > (high in the case of 'for_reclaim' and low for the other two). > 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? ------------------[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; + } + ret = nfs_commit_inode(inode, flags); if (ret >= 0) { if (wbc->sync_mode == WB_SYNC_NONE) { -- 1.5.5.6 ^ permalink raw reply related [flat|nested] 47+ messages in thread
[parent not found: <20100819151618.5f769dc9-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>]
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? [not found] ` <20100819151618.5f769dc9-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org> 2010-08-19 19:43 ` Trond Myklebust @ 2010-08-19 19:43 ` Trond Myklebust 0 siblings, 0 replies; 47+ messages in thread From: Trond Myklebust @ 2010-08-19 19:43 UTC (permalink / raw) To: Jeff Layton Cc: Christoph Hellwig, fengguang.wu-Re5JQEeQqe8AvxtiuMwx3w, linux-nfs-u79uwXL29TY76Z2rM5mHXA, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg On Thu, 2010-08-19 at 15:16 -0400, Jeff Layton wrote: > On Thu, 19 Aug 2010 10:58:25 -0400 > Trond Myklebust <trond.myklebust-41N18TsMXrtuMpJDpNschA@public.gmane.org> wrote: > > > On Thu, 2010-08-19 at 10:37 -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. 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. > > > > > > > + 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. > > > > To me that sounds fine. I've also been trying to wrap my head around the > > differences between 'nonblocking', 'for_background', 'for_reclaim' and > > 'for_kupdate' and how the filesystem is supposed to treat them. > > > > Aside from the above, I've used 'for_reclaim', 'for_kupdate' and > > 'for_background' in order to adjust the RPC request's queuing priority > > (high in the case of 'for_reclaim' and low for the other two). > > > > 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? Looks fine to me. I'll queue it up for the post-2.6.36 merge window... Cheers Trond -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-19 19:43 ` Trond Myklebust 0 siblings, 0 replies; 47+ messages in thread From: Trond Myklebust @ 2010-08-19 19:43 UTC (permalink / raw) To: Jeff Layton Cc: Christoph Hellwig, fengguang.wu, linux-nfs, linux-fsdevel, linux-mm On Thu, 2010-08-19 at 15:16 -0400, Jeff Layton wrote: > On Thu, 19 Aug 2010 10:58:25 -0400 > Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > > > On Thu, 2010-08-19 at 10:37 -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. 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. > > > > > > > + 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. > > > > To me that sounds fine. I've also been trying to wrap my head around the > > differences between 'nonblocking', 'for_background', 'for_reclaim' and > > 'for_kupdate' and how the filesystem is supposed to treat them. > > > > Aside from the above, I've used 'for_reclaim', 'for_kupdate' and > > 'for_background' in order to adjust the RPC request's queuing priority > > (high in the case of 'for_reclaim' and low for the other two). > > > > 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? Looks fine to me. I'll queue it up for the post-2.6.36 merge window... Cheers Trond -- 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-19 19:43 ` Trond Myklebust 0 siblings, 0 replies; 47+ messages in thread From: Trond Myklebust @ 2010-08-19 19:43 UTC (permalink / raw) To: Jeff Layton Cc: Christoph Hellwig, fengguang.wu, linux-nfs, linux-fsdevel, linux-mm On Thu, 2010-08-19 at 15:16 -0400, Jeff Layton wrote: > On Thu, 19 Aug 2010 10:58:25 -0400 > Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > > > On Thu, 2010-08-19 at 10:37 -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. 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. > > > > > > > + 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. > > > > To me that sounds fine. I've also been trying to wrap my head around the > > differences between 'nonblocking', 'for_background', 'for_reclaim' and > > 'for_kupdate' and how the filesystem is supposed to treat them. > > > > Aside from the above, I've used 'for_reclaim', 'for_kupdate' and > > 'for_background' in order to adjust the RPC request's queuing priority > > (high in the case of 'for_reclaim' and low for the other two). > > > > 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? Looks fine to me. I'll queue it up for the post-2.6.36 merge window... Cheers Trond ^ permalink raw reply [flat|nested] 47+ messages in thread
[parent not found: <1282246999.7799.66.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org>]
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-19 19:43 ` Trond Myklebust (?) @ 2010-08-20 13:23 ` Wu Fengguang -1 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 13:23 UTC (permalink / raw) To: Trond Myklebust Cc: Jeff Layton, Christoph Hellwig, linux-nfs-u79uwXL29TY76Z2rM5mHXA, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg > > 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? > > Looks fine to me. I'll queue it up for the post-2.6.36 merge window... Trond, I just created a patch that removes the wbc->nonblocking definition and all its references except NFS. So there will be merge dependencies. What should we do? To push both patches to Andrew's -mm tree? Thanks, Fengguang -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-20 13:23 ` Wu Fengguang 0 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 13:23 UTC (permalink / raw) To: Trond Myklebust Cc: Jeff Layton, Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm > > 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? > > Looks fine to me. I'll queue it up for the post-2.6.36 merge window... Trond, I just created a patch that removes the wbc->nonblocking definition and all its references except NFS. So there will be merge dependencies. What should we do? To push both patches to Andrew's -mm tree? 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-20 13:23 ` Wu Fengguang 0 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 13:23 UTC (permalink / raw) To: Trond Myklebust Cc: Jeff Layton, Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm > > 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? > > Looks fine to me. I'll queue it up for the post-2.6.36 merge window... Trond, I just created a patch that removes the wbc->nonblocking definition and all its references except NFS. So there will be merge dependencies. What should we do? To push both patches to Andrew's -mm tree? Thanks, Fengguang ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-20 13:23 ` Wu Fengguang (?) @ 2010-08-30 19:22 ` Trond Myklebust -1 siblings, 0 replies; 47+ messages in thread From: Trond Myklebust @ 2010-08-30 19:22 UTC (permalink / raw) To: Wu Fengguang Cc: Jeff Layton, Christoph Hellwig, linux-nfs-u79uwXL29TY76Z2rM5mHXA, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg On Fri, 2010-08-20 at 21:23 +0800, Wu Fengguang 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? > > > > Looks fine to me. I'll queue it up for the post-2.6.36 merge window... > > Trond, I just created a patch that removes the wbc->nonblocking > definition and all its references except NFS. So there will be merge > dependencies. What should we do? To push both patches to Andrew's -mm > tree? > > Thanks, > Fengguang Do you want to include it as part of your series? Just remember to add an Acked-by: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> Cheers Trond -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-30 19:22 ` Trond Myklebust 0 siblings, 0 replies; 47+ messages in thread From: Trond Myklebust @ 2010-08-30 19:22 UTC (permalink / raw) To: Wu Fengguang Cc: Jeff Layton, Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm On Fri, 2010-08-20 at 21:23 +0800, Wu Fengguang 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? > > > > Looks fine to me. I'll queue it up for the post-2.6.36 merge window... > > Trond, I just created a patch that removes the wbc->nonblocking > definition and all its references except NFS. So there will be merge > dependencies. What should we do? To push both patches to Andrew's -mm > tree? > > Thanks, > Fengguang Do you want to include it as part of your series? Just remember to add an Acked-by: Trond Myklebust <Trond.Myklebust@netapp.com> Cheers Trond -- 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-30 19:22 ` Trond Myklebust 0 siblings, 0 replies; 47+ messages in thread From: Trond Myklebust @ 2010-08-30 19:22 UTC (permalink / raw) To: Wu Fengguang Cc: Jeff Layton, Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm On Fri, 2010-08-20 at 21:23 +0800, Wu Fengguang 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? > > > > Looks fine to me. I'll queue it up for the post-2.6.36 merge window... > > Trond, I just created a patch that removes the wbc->nonblocking > definition and all its references except NFS. So there will be merge > dependencies. What should we do? To push both patches to Andrew's -mm > tree? > > Thanks, > Fengguang Do you want to include it as part of your series? Just remember to add an Acked-by: Trond Myklebust <Trond.Myklebust@netapp.com> Cheers Trond ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-30 19:22 ` Trond Myklebust @ 2010-08-30 23:53 ` Wu Fengguang -1 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-30 23:53 UTC (permalink / raw) To: Trond Myklebust Cc: Jeff Layton, Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm On Mon, Aug 30, 2010 at 03:22:54PM -0400, Trond Myklebust wrote: > On Fri, 2010-08-20 at 21:23 +0800, Wu Fengguang 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? > > > > > > Looks fine to me. I'll queue it up for the post-2.6.36 merge window... > > > > Trond, I just created a patch that removes the wbc->nonblocking > > definition and all its references except NFS. So there will be merge > > dependencies. What should we do? To push both patches to Andrew's -mm > > tree? > > > > Thanks, > > Fengguang > > Do you want to include it as part of your series? Just remember to add > an > > Acked-by: Trond Myklebust <Trond.Myklebust@netapp.com> Thanks. Please keep the NFS patches in your tree. I've send a patch to Andrew Morton which removes the other references but keeps the definitions. So that there won't be compile errors when the patches are pushed at different time. Thanks, Fengguang ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-30 23:53 ` Wu Fengguang 0 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-30 23:53 UTC (permalink / raw) To: Trond Myklebust Cc: Jeff Layton, Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm On Mon, Aug 30, 2010 at 03:22:54PM -0400, Trond Myklebust wrote: > On Fri, 2010-08-20 at 21:23 +0800, Wu Fengguang 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? > > > > > > Looks fine to me. I'll queue it up for the post-2.6.36 merge window... > > > > Trond, I just created a patch that removes the wbc->nonblocking > > definition and all its references except NFS. So there will be merge > > dependencies. What should we do? To push both patches to Andrew's -mm > > tree? > > > > Thanks, > > Fengguang > > Do you want to include it as part of your series? Just remember to add > an > > Acked-by: Trond Myklebust <Trond.Myklebust@netapp.com> Thanks. Please keep the NFS patches in your tree. I've send a patch to Andrew Morton which removes the other references but keeps the definitions. So that there won't be compile errors when the patches are pushed at different time. 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-19 19:16 ` Jeff Layton @ 2010-08-20 0:33 ` Wu Fengguang -1 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 0:33 UTC (permalink / raw) To: Jeff Layton Cc: Trond Myklebust, Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm > 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-20 0:33 ` Wu Fengguang 0 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 0:33 UTC (permalink / raw) To: Jeff Layton Cc: Trond Myklebust, Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm > 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-20 0:33 ` Wu Fengguang @ 2010-08-20 0:53 ` Jeff Layton -1 siblings, 0 replies; 47+ messages in thread From: Jeff Layton @ 2010-08-20 0:53 UTC (permalink / raw) To: Wu Fengguang Cc: Trond Myklebust, Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm On Fri, 20 Aug 2010 08:33:08 +0800 Wu Fengguang <fengguang.wu@gmail.com> 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@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 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@redhat.com> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-20 0:53 ` Jeff Layton 0 siblings, 0 replies; 47+ messages in thread From: Jeff Layton @ 2010-08-20 0:53 UTC (permalink / raw) To: Wu Fengguang Cc: Trond Myklebust, Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm On Fri, 20 Aug 2010 08:33:08 +0800 Wu Fengguang <fengguang.wu@gmail.com> 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@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 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@redhat.com> -- 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-20 0:53 ` Jeff Layton @ 2010-08-20 13:20 ` Wu Fengguang -1 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 13:20 UTC (permalink / raw) To: Jeff Layton Cc: Trond Myklebust, Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm On Thu, Aug 19, 2010 at 08:53:32PM -0400, Jeff Layton wrote: > On Fri, 20 Aug 2010 08:33:08 +0800 > Wu Fengguang <fengguang.wu@gmail.com> 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@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 > > 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... OK, please do the comments. > Thanks for the review... You are welcome. Thanks, Fengguang ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-20 13:20 ` Wu Fengguang 0 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 13:20 UTC (permalink / raw) To: Jeff Layton Cc: Trond Myklebust, Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm On Thu, Aug 19, 2010 at 08:53:32PM -0400, Jeff Layton wrote: > On Fri, 20 Aug 2010 08:33:08 +0800 > Wu Fengguang <fengguang.wu@gmail.com> 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@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 > > 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... OK, please do the comments. > Thanks for the review... You are welcome. 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-19 14:37 ` Christoph Hellwig @ 2010-08-19 23:55 ` Wu Fengguang -1 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-19 23:55 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, linux-nfs, linux-fsdevel, linux-mm, Chris Mason, Jens Axboe 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-19 23:55 ` Wu Fengguang 0 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-19 23:55 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, linux-nfs, linux-fsdevel, linux-mm, Chris Mason, Jens Axboe 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); ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-19 23:55 ` Wu Fengguang @ 2010-08-20 0:02 ` Wu Fengguang -1 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 0:02 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, linux-nfs-u79uwXL29TY76Z2rM5mHXA, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Chris Mason, Jens Axboe, David Howells, Sage Weil, Steve French [-- Attachment #1: Type: text/plain, Size: 2091 bytes --] [add CC to afs/cifs/ceph maintainers] On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > 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 [-- Attachment #2: writeback-remove-congested-checks.patch --] [-- Type: text/x-diff, Size: 2939 bytes --] Subject: writeback: remove useless nonblocking checks in ->writepages From: Wu Fengguang <fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> Date: Fri Aug 20 07:04:54 CST 2010 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: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> CC: Sage Weil <sage-BnTBU8nroG7k1uMJSBkQmQ@public.gmane.org> CC: Steve French <sfrench-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> CC: Chris Mason <chris.mason-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> CC: Jens Axboe <axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org> CC: Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> Signed-off-by: Wu Fengguang <fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> --- 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); ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-20 0:02 ` Wu Fengguang 0 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 0:02 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, linux-nfs, linux-fsdevel, linux-mm, Chris Mason, Jens Axboe, David Howells, Sage Weil, Steve French [-- Attachment #1: Type: text/plain, Size: 2091 bytes --] [add CC to afs/cifs/ceph maintainers] On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > 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 [-- Attachment #2: writeback-remove-congested-checks.patch --] [-- Type: text/x-diff, Size: 2708 bytes --] Subject: writeback: remove useless nonblocking checks in ->writepages From: Wu Fengguang <fengguang.wu@intel.com> Date: Fri Aug 20 07:04:54 CST 2010 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: David Howells <dhowells@redhat.com> CC: Sage Weil <sage@newdream.net> CC: Steve French <sfrench@samba.org> CC: Chris Mason <chris.mason@oracle.com> CC: Jens Axboe <axboe@kernel.dk> 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); ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-20 0:02 ` Wu Fengguang @ 2010-08-20 2:36 ` Sage Weil -1 siblings, 0 replies; 47+ messages in thread From: Sage Weil @ 2010-08-20 2:36 UTC (permalink / raw) To: Wu Fengguang Cc: Christoph Hellwig, Jeff Layton, linux-nfs, linux-fsdevel, linux-mm, Chris Mason, Jens Axboe, David Howells, Steve French On Fri, 20 Aug 2010, Wu Fengguang wrote: > [add CC to afs/cifs/ceph maintainers] > > On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > > 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..) I'll queue up a fix for Ceph's ->writepages in my tree. Thanks! sage > > > > > 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 > ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-20 2:36 ` Sage Weil 0 siblings, 0 replies; 47+ messages in thread From: Sage Weil @ 2010-08-20 2:36 UTC (permalink / raw) To: Wu Fengguang Cc: Christoph Hellwig, Jeff Layton, linux-nfs, linux-fsdevel, linux-mm, Chris Mason, Jens Axboe, David Howells, Steve French On Fri, 20 Aug 2010, Wu Fengguang wrote: > [add CC to afs/cifs/ceph maintainers] > > On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > > 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..) I'll queue up a fix for Ceph's ->writepages in my tree. Thanks! sage > > > > > 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 > -- 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-19 23:55 ` Wu Fengguang @ 2010-08-20 9:19 ` Christoph Hellwig -1 siblings, 0 replies; 47+ messages in thread From: Christoph Hellwig @ 2010-08-20 9:19 UTC (permalink / raw) To: Wu Fengguang Cc: Christoph Hellwig, Jeff Layton, linux-nfs, linux-fsdevel, linux-mm, Chris Mason, Jens Axboe On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > 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. Why does pageout() and memory migration need to be even more non-blocking than the already non-blockig WB_SYNC_NONE writeout? ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-20 9:19 ` Christoph Hellwig 0 siblings, 0 replies; 47+ messages in thread From: Christoph Hellwig @ 2010-08-20 9:19 UTC (permalink / raw) To: Wu Fengguang Cc: Christoph Hellwig, Jeff Layton, linux-nfs, linux-fsdevel, linux-mm, Chris Mason, Jens Axboe On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > 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. Why does pageout() and memory migration need to be even more non-blocking than the already non-blockig WB_SYNC_NONE writeout? -- 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
[parent not found: <20100820091904.GB20138-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>]
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-20 9:19 ` Christoph Hellwig (?) @ 2010-08-20 11:27 ` Jeff Layton -1 siblings, 0 replies; 47+ messages in thread From: Jeff Layton @ 2010-08-20 11:27 UTC (permalink / raw) To: Christoph Hellwig Cc: Wu Fengguang, linux-nfs-u79uwXL29TY76Z2rM5mHXA, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Chris Mason, Jens Axboe On Fri, 20 Aug 2010 05:19:04 -0400 Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote: > On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > > 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. > > Why does pageout() and memory migration need to be even more > non-blocking than the already non-blockig WB_SYNC_NONE writeout? > Just an idle thought on this... I think a lot of the confusion here comes from the fact that we have sync_mode and a bunch of flags, and it's not at all clear how filesystems are supposed to treat the union of them. There are also possible unions of flags/sync_modes that never happen in practice. It's not always obvious though and as filesystem implementors we have to consider the possibility that they might occur (consider WB_SYNC_ALL + for_background). Perhaps a lot of this confusion could be lifted by getting rid of the extra flags and adding new sync_mode's. Maybe something like: WB_SYNC_ALL /* wait on everything to complete */ WB_SYNC_NONE /* don't wait on anything */ WB_SYNC_FOR_RECLAIM /* sync for reclaim */ WB_SYNC_FOR_KUPDATED /* sync by kupdate */ ...etc... That does mean that all of the filesystem specific code may need to be touched when new modes are added and removed. I think it would be clearer though about what you're supposed to do in ->writepages. -- Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-20 11:27 ` Jeff Layton 0 siblings, 0 replies; 47+ messages in thread From: Jeff Layton @ 2010-08-20 11:27 UTC (permalink / raw) To: Christoph Hellwig Cc: Wu Fengguang, linux-nfs, linux-fsdevel, linux-mm, Chris Mason, Jens Axboe On Fri, 20 Aug 2010 05:19:04 -0400 Christoph Hellwig <hch@infradead.org> wrote: > On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > > 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. > > Why does pageout() and memory migration need to be even more > non-blocking than the already non-blockig WB_SYNC_NONE writeout? > Just an idle thought on this... I think a lot of the confusion here comes from the fact that we have sync_mode and a bunch of flags, and it's not at all clear how filesystems are supposed to treat the union of them. There are also possible unions of flags/sync_modes that never happen in practice. It's not always obvious though and as filesystem implementors we have to consider the possibility that they might occur (consider WB_SYNC_ALL + for_background). Perhaps a lot of this confusion could be lifted by getting rid of the extra flags and adding new sync_mode's. Maybe something like: WB_SYNC_ALL /* wait on everything to complete */ WB_SYNC_NONE /* don't wait on anything */ WB_SYNC_FOR_RECLAIM /* sync for reclaim */ WB_SYNC_FOR_KUPDATED /* sync by kupdate */ ...etc... That does mean that all of the filesystem specific code may need to be touched when new modes are added and removed. I think it would be clearer though about what you're supposed to do in ->writepages. -- Jeff Layton <jlayton@redhat.com> -- 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-20 11:27 ` Jeff Layton 0 siblings, 0 replies; 47+ messages in thread From: Jeff Layton @ 2010-08-20 11:27 UTC (permalink / raw) To: Christoph Hellwig Cc: Wu Fengguang, linux-nfs, linux-fsdevel, linux-mm, Chris Mason, Jens Axboe On Fri, 20 Aug 2010 05:19:04 -0400 Christoph Hellwig <hch@infradead.org> wrote: > On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > > 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. > > Why does pageout() and memory migration need to be even more > non-blocking than the already non-blockig WB_SYNC_NONE writeout? > Just an idle thought on this... I think a lot of the confusion here comes from the fact that we have sync_mode and a bunch of flags, and it's not at all clear how filesystems are supposed to treat the union of them. There are also possible unions of flags/sync_modes that never happen in practice. It's not always obvious though and as filesystem implementors we have to consider the possibility that they might occur (consider WB_SYNC_ALL + for_background). Perhaps a lot of this confusion could be lifted by getting rid of the extra flags and adding new sync_mode's. Maybe something like: WB_SYNC_ALL /* wait on everything to complete */ WB_SYNC_NONE /* don't wait on anything */ WB_SYNC_FOR_RECLAIM /* sync for reclaim */ WB_SYNC_FOR_KUPDATED /* sync by kupdate */ ...etc... That does mean that all of the filesystem specific code may need to be touched when new modes are added and removed. I think it would be clearer though about what you're supposed to do in ->writepages. -- Jeff Layton <jlayton@redhat.com> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-20 11:27 ` Jeff Layton @ 2010-08-20 12:44 ` Wu Fengguang -1 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 12:44 UTC (permalink / raw) To: Jeff Layton Cc: Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm, Chris Mason, Jens Axboe On Fri, Aug 20, 2010 at 07:27:57AM -0400, Jeff Layton wrote: > On Fri, 20 Aug 2010 05:19:04 -0400 > Christoph Hellwig <hch@infradead.org> wrote: > > > On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > > > 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. > > > > Why does pageout() and memory migration need to be even more > > non-blocking than the already non-blockig WB_SYNC_NONE writeout? > > > > Just an idle thought on this... > > I think a lot of the confusion here comes from the fact that we have > sync_mode and a bunch of flags, and it's not at all clear how > filesystems are supposed to treat the union of them. There are also > possible unions of flags/sync_modes that never happen in practice. It's > not always obvious though and as filesystem implementors we have to > consider the possibility that they might occur (consider WB_SYNC_ALL + > for_background). > > Perhaps a lot of this confusion could be lifted by getting rid of the > extra flags and adding new sync_mode's. Maybe something like: > > WB_SYNC_ALL /* wait on everything to complete */ > WB_SYNC_NONE /* don't wait on anything */ > WB_SYNC_FOR_RECLAIM /* sync for reclaim */ > WB_SYNC_FOR_KUPDATED /* sync by kupdate */ > ...etc... > > That does mean that all of the filesystem specific code may need to be > touched when new modes are added and removed. I think it would be > clearer though about what you're supposed to do in ->writepages. No, we are moving towards the other direction :) I just removed the definition of wbc->nonblocking and wbc->encountered_congestion and all of the references. Sorry for the confusion! 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> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-20 12:44 ` Wu Fengguang 0 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 12:44 UTC (permalink / raw) To: Jeff Layton Cc: Christoph Hellwig, linux-nfs, linux-fsdevel, linux-mm, Chris Mason, Jens Axboe On Fri, Aug 20, 2010 at 07:27:57AM -0400, Jeff Layton wrote: > On Fri, 20 Aug 2010 05:19:04 -0400 > Christoph Hellwig <hch@infradead.org> wrote: > > > On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > > > 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. > > > > Why does pageout() and memory migration need to be even more > > non-blocking than the already non-blockig WB_SYNC_NONE writeout? > > > > Just an idle thought on this... > > I think a lot of the confusion here comes from the fact that we have > sync_mode and a bunch of flags, and it's not at all clear how > filesystems are supposed to treat the union of them. There are also > possible unions of flags/sync_modes that never happen in practice. It's > not always obvious though and as filesystem implementors we have to > consider the possibility that they might occur (consider WB_SYNC_ALL + > for_background). > > Perhaps a lot of this confusion could be lifted by getting rid of the > extra flags and adding new sync_mode's. Maybe something like: > > WB_SYNC_ALL /* wait on everything to complete */ > WB_SYNC_NONE /* don't wait on anything */ > WB_SYNC_FOR_RECLAIM /* sync for reclaim */ > WB_SYNC_FOR_KUPDATED /* sync by kupdate */ > ...etc... > > That does mean that all of the filesystem specific code may need to be > touched when new modes are added and removed. I think it would be > clearer though about what you're supposed to do in ->writepages. No, we are moving towards the other direction :) I just removed the definition of wbc->nonblocking and wbc->encountered_congestion and all of the references. Sorry for the confusion! Thanks, Fengguang ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? 2010-08-20 9:19 ` Christoph Hellwig (?) @ 2010-08-20 12:26 ` Wu Fengguang -1 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 12:26 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, linux-nfs-u79uwXL29TY76Z2rM5mHXA, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Chris Mason, Jens Axboe On Fri, Aug 20, 2010 at 05:19:04AM -0400, Christoph Hellwig wrote: > On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > > 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. > > Why does pageout() and memory migration need to be even more > non-blocking than the already non-blockig WB_SYNC_NONE writeout? Good question! So the other nonblocking checks in ->writepage are also redundant code, let's rip them all! Thanks, Fengguang --- diff --git a/include/linux/writeback.h b/include/linux/writeback.h index 72a5d64..bd11c0e 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -45,8 +45,6 @@ struct writeback_control { loff_t range_start; loff_t range_end; - unsigned nonblocking:1; /* Don't get stuck on request queues */ - unsigned encountered_congestion:1; /* An output: a queue is full */ unsigned for_kupdate:1; /* A kupdate writeback */ unsigned for_background:1; /* A background writeback */ unsigned for_reclaim:1; /* Invoked from the page allocator */ diff --git a/fs/buffer.c b/fs/buffer.c index 3e7dca2..c9a7a80 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -1706,7 +1706,7 @@ static int __block_write_full_page(struct inode *inode, struct page *page, * and kswapd activity, but those code paths have their own * higher-level throttling. */ - if (wbc->sync_mode != WB_SYNC_NONE || !wbc->nonblocking) { + if (wbc->sync_mode != WB_SYNC_NONE) { lock_buffer(bh); } else if (!trylock_buffer(bh)) { redirty_page_for_writepage(wbc, page); diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c index f3b071f..939739c 100644 --- a/fs/gfs2/meta_io.c +++ b/fs/gfs2/meta_io.c @@ -55,7 +55,7 @@ static int gfs2_aspace_writepage(struct page *page, struct writeback_control *wb * activity, but those code paths have their own higher-level * throttling. */ - if (wbc->sync_mode != WB_SYNC_NONE || !wbc->nonblocking) { + if (wbc->sync_mode != WB_SYNC_NONE) { lock_buffer(bh); } else if (!trylock_buffer(bh)) { redirty_page_for_writepage(wbc, page); diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c index caa7583..c1f9389 100644 --- a/fs/reiserfs/inode.c +++ b/fs/reiserfs/inode.c @@ -2438,7 +2438,7 @@ static int reiserfs_write_full_page(struct page *page, /* from this point on, we know the buffer is mapped to a * real block and not a direct item */ - if (wbc->sync_mode != WB_SYNC_NONE || !wbc->nonblocking) { + if (wbc->sync_mode != WB_SYNC_NONE) { lock_buffer(bh); } else { if (!trylock_buffer(bh)) { diff --git a/fs/xfs/linux-2.6/xfs_aops.c b/fs/xfs/linux-2.6/xfs_aops.c index 15412fe..ec495a4 100644 --- a/fs/xfs/linux-2.6/xfs_aops.c +++ b/fs/xfs/linux-2.6/xfs_aops.c @@ -1139,8 +1139,7 @@ xfs_vm_writepage( type = IO_DELAY; flags = BMAPI_ALLOCATE; - if (wbc->sync_mode == WB_SYNC_NONE && - wbc->nonblocking) + if (wbc->sync_mode == WB_SYNC_NONE) flags |= BMAPI_TRYLOCK; } diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h index f345f66..0bb01ab 100644 --- a/include/trace/events/writeback.h +++ b/include/trace/events/writeback.h @@ -96,8 +96,6 @@ DECLARE_EVENT_CLASS(wbc_class, __field(long, nr_to_write) __field(long, pages_skipped) __field(int, sync_mode) - __field(int, nonblocking) - __field(int, encountered_congestion) __field(int, for_kupdate) __field(int, for_background) __field(int, for_reclaim) diff --git a/mm/migrate.c b/mm/migrate.c index 38e7cad..1e5914a 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -431,7 +431,6 @@ static int writeout(struct address_space *mapping, struct page *page) .nr_to_write = 1, .range_start = 0, .range_end = LLONG_MAX, - .nonblocking = 1, .for_reclaim = 1 }; int rc; diff --git a/mm/vmscan.c b/mm/vmscan.c index 225a759..3520523 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -376,7 +376,6 @@ static pageout_t pageout(struct page *page, struct address_space *mapping, .nr_to_write = SWAP_CLUSTER_MAX, .range_start = 0, .range_end = LLONG_MAX, - .nonblocking = 1, .for_reclaim = 1, }; -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-20 12:26 ` Wu Fengguang 0 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 12:26 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, linux-nfs, linux-fsdevel, linux-mm, Chris Mason, Jens Axboe On Fri, Aug 20, 2010 at 05:19:04AM -0400, Christoph Hellwig wrote: > On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > > 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. > > Why does pageout() and memory migration need to be even more > non-blocking than the already non-blockig WB_SYNC_NONE writeout? Good question! So the other nonblocking checks in ->writepage are also redundant code, let's rip them all! Thanks, Fengguang --- diff --git a/include/linux/writeback.h b/include/linux/writeback.h index 72a5d64..bd11c0e 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -45,8 +45,6 @@ struct writeback_control { loff_t range_start; loff_t range_end; - unsigned nonblocking:1; /* Don't get stuck on request queues */ - unsigned encountered_congestion:1; /* An output: a queue is full */ unsigned for_kupdate:1; /* A kupdate writeback */ unsigned for_background:1; /* A background writeback */ unsigned for_reclaim:1; /* Invoked from the page allocator */ diff --git a/fs/buffer.c b/fs/buffer.c index 3e7dca2..c9a7a80 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -1706,7 +1706,7 @@ static int __block_write_full_page(struct inode *inode, struct page *page, * and kswapd activity, but those code paths have their own * higher-level throttling. */ - if (wbc->sync_mode != WB_SYNC_NONE || !wbc->nonblocking) { + if (wbc->sync_mode != WB_SYNC_NONE) { lock_buffer(bh); } else if (!trylock_buffer(bh)) { redirty_page_for_writepage(wbc, page); diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c index f3b071f..939739c 100644 --- a/fs/gfs2/meta_io.c +++ b/fs/gfs2/meta_io.c @@ -55,7 +55,7 @@ static int gfs2_aspace_writepage(struct page *page, struct writeback_control *wb * activity, but those code paths have their own higher-level * throttling. */ - if (wbc->sync_mode != WB_SYNC_NONE || !wbc->nonblocking) { + if (wbc->sync_mode != WB_SYNC_NONE) { lock_buffer(bh); } else if (!trylock_buffer(bh)) { redirty_page_for_writepage(wbc, page); diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c index caa7583..c1f9389 100644 --- a/fs/reiserfs/inode.c +++ b/fs/reiserfs/inode.c @@ -2438,7 +2438,7 @@ static int reiserfs_write_full_page(struct page *page, /* from this point on, we know the buffer is mapped to a * real block and not a direct item */ - if (wbc->sync_mode != WB_SYNC_NONE || !wbc->nonblocking) { + if (wbc->sync_mode != WB_SYNC_NONE) { lock_buffer(bh); } else { if (!trylock_buffer(bh)) { diff --git a/fs/xfs/linux-2.6/xfs_aops.c b/fs/xfs/linux-2.6/xfs_aops.c index 15412fe..ec495a4 100644 --- a/fs/xfs/linux-2.6/xfs_aops.c +++ b/fs/xfs/linux-2.6/xfs_aops.c @@ -1139,8 +1139,7 @@ xfs_vm_writepage( type = IO_DELAY; flags = BMAPI_ALLOCATE; - if (wbc->sync_mode == WB_SYNC_NONE && - wbc->nonblocking) + if (wbc->sync_mode == WB_SYNC_NONE) flags |= BMAPI_TRYLOCK; } diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h index f345f66..0bb01ab 100644 --- a/include/trace/events/writeback.h +++ b/include/trace/events/writeback.h @@ -96,8 +96,6 @@ DECLARE_EVENT_CLASS(wbc_class, __field(long, nr_to_write) __field(long, pages_skipped) __field(int, sync_mode) - __field(int, nonblocking) - __field(int, encountered_congestion) __field(int, for_kupdate) __field(int, for_background) __field(int, for_reclaim) diff --git a/mm/migrate.c b/mm/migrate.c index 38e7cad..1e5914a 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -431,7 +431,6 @@ static int writeout(struct address_space *mapping, struct page *page) .nr_to_write = 1, .range_start = 0, .range_end = LLONG_MAX, - .nonblocking = 1, .for_reclaim = 1 }; int rc; diff --git a/mm/vmscan.c b/mm/vmscan.c index 225a759..3520523 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -376,7 +376,6 @@ static pageout_t pageout(struct page *page, struct address_space *mapping, .nr_to_write = SWAP_CLUSTER_MAX, .range_start = 0, .range_end = LLONG_MAX, - .nonblocking = 1, .for_reclaim = 1, }; -- 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> ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: why are WB_SYNC_NONE COMMITs being done with FLUSH_SYNC set ? @ 2010-08-20 12:26 ` Wu Fengguang 0 siblings, 0 replies; 47+ messages in thread From: Wu Fengguang @ 2010-08-20 12:26 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, linux-nfs, linux-fsdevel, linux-mm, Chris Mason, Jens Axboe On Fri, Aug 20, 2010 at 05:19:04AM -0400, Christoph Hellwig wrote: > On Fri, Aug 20, 2010 at 07:55:53AM +0800, Wu Fengguang wrote: > > 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. > > Why does pageout() and memory migration need to be even more > non-blocking than the already non-blockig WB_SYNC_NONE writeout? Good question! So the other nonblocking checks in ->writepage are also redundant code, let's rip them all! Thanks, Fengguang --- diff --git a/include/linux/writeback.h b/include/linux/writeback.h index 72a5d64..bd11c0e 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -45,8 +45,6 @@ struct writeback_control { loff_t range_start; loff_t range_end; - unsigned nonblocking:1; /* Don't get stuck on request queues */ - unsigned encountered_congestion:1; /* An output: a queue is full */ unsigned for_kupdate:1; /* A kupdate writeback */ unsigned for_background:1; /* A background writeback */ unsigned for_reclaim:1; /* Invoked from the page allocator */ diff --git a/fs/buffer.c b/fs/buffer.c index 3e7dca2..c9a7a80 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -1706,7 +1706,7 @@ static int __block_write_full_page(struct inode *inode, struct page *page, * and kswapd activity, but those code paths have their own * higher-level throttling. */ - if (wbc->sync_mode != WB_SYNC_NONE || !wbc->nonblocking) { + if (wbc->sync_mode != WB_SYNC_NONE) { lock_buffer(bh); } else if (!trylock_buffer(bh)) { redirty_page_for_writepage(wbc, page); diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c index f3b071f..939739c 100644 --- a/fs/gfs2/meta_io.c +++ b/fs/gfs2/meta_io.c @@ -55,7 +55,7 @@ static int gfs2_aspace_writepage(struct page *page, struct writeback_control *wb * activity, but those code paths have their own higher-level * throttling. */ - if (wbc->sync_mode != WB_SYNC_NONE || !wbc->nonblocking) { + if (wbc->sync_mode != WB_SYNC_NONE) { lock_buffer(bh); } else if (!trylock_buffer(bh)) { redirty_page_for_writepage(wbc, page); diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c index caa7583..c1f9389 100644 --- a/fs/reiserfs/inode.c +++ b/fs/reiserfs/inode.c @@ -2438,7 +2438,7 @@ static int reiserfs_write_full_page(struct page *page, /* from this point on, we know the buffer is mapped to a * real block and not a direct item */ - if (wbc->sync_mode != WB_SYNC_NONE || !wbc->nonblocking) { + if (wbc->sync_mode != WB_SYNC_NONE) { lock_buffer(bh); } else { if (!trylock_buffer(bh)) { diff --git a/fs/xfs/linux-2.6/xfs_aops.c b/fs/xfs/linux-2.6/xfs_aops.c index 15412fe..ec495a4 100644 --- a/fs/xfs/linux-2.6/xfs_aops.c +++ b/fs/xfs/linux-2.6/xfs_aops.c @@ -1139,8 +1139,7 @@ xfs_vm_writepage( type = IO_DELAY; flags = BMAPI_ALLOCATE; - if (wbc->sync_mode == WB_SYNC_NONE && - wbc->nonblocking) + if (wbc->sync_mode == WB_SYNC_NONE) flags |= BMAPI_TRYLOCK; } diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h index f345f66..0bb01ab 100644 --- a/include/trace/events/writeback.h +++ b/include/trace/events/writeback.h @@ -96,8 +96,6 @@ DECLARE_EVENT_CLASS(wbc_class, __field(long, nr_to_write) __field(long, pages_skipped) __field(int, sync_mode) - __field(int, nonblocking) - __field(int, encountered_congestion) __field(int, for_kupdate) __field(int, for_background) __field(int, for_reclaim) diff --git a/mm/migrate.c b/mm/migrate.c index 38e7cad..1e5914a 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -431,7 +431,6 @@ static int writeout(struct address_space *mapping, struct page *page) .nr_to_write = 1, .range_start = 0, .range_end = LLONG_MAX, - .nonblocking = 1, .for_reclaim = 1 }; int rc; diff --git a/mm/vmscan.c b/mm/vmscan.c index 225a759..3520523 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -376,7 +376,6 @@ static pageout_t pageout(struct page *page, struct address_space *mapping, .nr_to_write = SWAP_CLUSTER_MAX, .range_start = 0, .range_end = LLONG_MAX, - .nonblocking = 1, .for_reclaim = 1, }; ^ permalink raw reply related [flat|nested] 47+ messages in thread
end of thread, other threads:[~2010-08-30 23:53 UTC | newest] Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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.