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

[Wu Fengguang <fengguang.wu@intel.com>]

On Fri, May 20, 2011 at 03:15:18PM +0800, Wu Fengguang wrote:
> On Fri, May 20, 2011 at 02:52:07PM +0800, Dave Chinner wrote:
> > 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@fromorbit.com>
> > 
> > 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.
> 
> OK, sorry, I'll keep that in mind in future.
> 
> > 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!
> > 			 */
> 
> That's what the original "writeback: introduce
> writeback_control.inodes_cleaned" does. Given that it's opposed to add
> writeback_control.inodes_cleaned, the only option remained is to add
> one more argument "long *inode_cleaned" to writeback_single_inode()
> like this.
> 
> Well it looks ugly and I wonder if you have any prettier version in
> mind. This ugliness is the main reason I resist to do the change.

The other option is to make use of a *bit* field wbc->inode_cleaned.
It still adds one more writeback_control field *logically* and several
new lines of code, but kills a bit stack overheads.

Thanks,
Fengguang