* 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
* 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.
^ 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
@ 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
* 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.
^ 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 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
^ 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 ?
[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
^ 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 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-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: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
* 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>
^ 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 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,
};
^ 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 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 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 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
^ 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
(?)
@ 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
^ 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
@ 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
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.