On Fri, May 20, 2011 at 07:29:10AM +0800, Dave Chinner wrote:
> On Fri, May 20, 2011 at 06:06:44AM +0800, Wu Fengguang wrote:
> > : writeback_single_inode(inode, wb, &wbc);
> > : work->nr_pages -= write_chunk - wbc.nr_to_write;
> > : wrote += write_chunk - wbc.nr_to_write;
> > : if (wbc.pages_skipped) {
> > : /*
> > : * writeback is not making progress due to locked
> > : * buffers. Skip this inode for now.
> > : */
> > : redirty_tail(inode, wb);
> > : - }
> > : + } else if (!(inode->i_state & I_DIRTY))
> > : + wrote++;
> >
> > It looks a bit more clean to do
> >
> > : wrote += write_chunk - wbc.nr_to_write;
> > : + if (!(inode->i_state & I_DIRTY))
> > : + wrote++;
> > : if (wbc.pages_skipped) {
> > : /*
> > : * writeback is not making progress due to locked
> > : * buffers. Skip this inode for now.
> > : */
> > : redirty_tail(inode, wb);
> > : }
>
> But it's still in the wrong place - such post-write inode dirty
> processing is supposed to be isolated to writeback_single_inode().
> Spreading it across multiple locations is not, IMO, the nicest thing
> to do...
Strictly speaking, it's post inspecting :)
It does look reasonable and safe to move the pages_skipped post
processing into writeback_single_inode(). See the below patch.
When doing this chunk,
- if (wbc->nr_to_write <= 0) {
+ if (wbc->nr_to_write <= 0 && wbc->pages_skipped == 0) {
I wonder in general sense (without knowing enough FS internals)
whether ->pages_skipped is that useful: if some locked buffer is
blocking all subsequent pages, then ->nr_to_write won't be able to
drop to zero. So the (wbc->pages_skipped == 0) test seems redundant..
Thanks,
Fengguang
---
Subject: writeback: move pages_skipped post processing into writeback_single_inode()
Date: Fri May 20 11:42:42 CST 2011
It's more logical to isolate post-write processings in writeback_single_inode().
Note that it slightly changes behavior for write_inode_now() and sync_inode(),
which used to ignore pages_skipped.
Proposed-by: Dave Chinner <david@xxxxxxxxxxxxx>
Signed-off-by: Wu Fengguang <fengguang.wu@xxxxxxxxx>
---
fs/fs-writeback.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
--- linux-next.orig/fs/fs-writeback.c 2011-05-20 11:26:19.000000000 +0800
+++ linux-next/fs/fs-writeback.c 2011-05-20 11:42:30.000000000 +0800
@@ -404,6 +404,7 @@ writeback_single_inode(struct inode *ino
spin_unlock(&inode->i_lock);
spin_unlock(&wb->list_lock);
+ wbc->pages_skipped = 0;
ret = do_writepages(mapping, wbc);
/*
@@ -443,7 +444,7 @@ writeback_single_inode(struct inode *ino
* sometimes bales out without doing anything.
*/
inode->i_state |= I_DIRTY_PAGES;
- if (wbc->nr_to_write <= 0) {
+ if (wbc->nr_to_write <= 0 && wbc->pages_skipped == 0) {
/*
* slice used up: queue for next turn
*/
@@ -602,7 +603,6 @@ static long writeback_sb_inodes(struct s
__iget(inode);
write_chunk = writeback_chunk_size(work);
wbc.nr_to_write = write_chunk;
- wbc.pages_skipped = 0;
writeback_single_inode(inode, wb, &wbc);
@@ -610,13 +610,6 @@ static long writeback_sb_inodes(struct s
wrote += write_chunk - wbc.nr_to_write;
if (!(inode->i_state & I_DIRTY))
wrote++;
- if (wbc.pages_skipped) {
- /*
- * writeback is not making progress due to locked
- * buffers. Skip this inode for now.
- */
- redirty_tail(inode, wb);
- }
spin_unlock(&inode->i_lock);
spin_unlock(&wb->list_lock);
iput(inode);
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
