Re: [PATCH 14/18] writeback: make writeback_control.nr_to_writestraight

[Dave Chinner]

On Fri, May 20, 2011 at 12:07:40PM +0800, Wu Fengguang wrote:
> 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.

<sigh>

That's not what I was referring to. The wbc.pages_skipped check is
fine where it is.

> 
> 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>

No, I didn't propose the change you've made in this patch. I've been
asking you to fix the original patch, not proposing new changes to
some other code.  Please don't add my name to random tags in patches
without asking me first.

So, for the third time, please fix the original patch by moving the
new "inode now clean" accounting to the "inode-now-clean" logic
branch in writeback_single_inode().

        if (!(inode->i_state & I_FREEING)) {
                if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
.....
                } else if (inode->i_state & I_DIRTY) {
.....
                } else {
			/*
			 * account for it here with all the other
			 * inode-now-clean manipulations that we need
			 * to do!
			 */
.....
                }
        }

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/