Re: [PATCH 6/6] writeback: merge for_kupdate and !for_kupdate cases

[Dave Chinner <david@fromorbit.com>]

On Mon, Jul 12, 2010 at 11:52:39PM +0800, Wu Fengguang wrote:
> On Mon, Jul 12, 2010 at 10:08:42AM +0800, Dave Chinner wrote:
> > On Sun, Jul 11, 2010 at 10:07:02AM +0800, Wu Fengguang wrote:
> > > -			/*
> > > -			 * akpm: if the caller was the kupdate function we put
> > > -			 * this inode at the head of b_dirty so it gets first
> > > -			 * consideration.  Otherwise, move it to the tail, for
> > > -			 * the reasons described there.  I'm not really sure
> > > -			 * how much sense this makes.  Presumably I had a good
> > > -			 * reasons for doing it this way, and I'd rather not
> > > -			 * muck with it at present.
> > > -			 */
> > > -			if (wbc->for_kupdate) {
> > > +			inode->i_state |= I_DIRTY_PAGES;
> > > +			if (wbc->nr_to_write <= 0) {
> > >  				/*
> > > -				 * For the kupdate function we move the inode
> > > -				 * to b_more_io so it will get more writeout as
> > > -				 * soon as the queue becomes uncongested.
> > > +				 * slice used up: queue for next turn
> > >  				 */
> > > -				inode->i_state |= I_DIRTY_PAGES;
> > > -				if (wbc->nr_to_write <= 0) {
> > > -					/*
> > > -					 * slice used up: queue for next turn
> > > -					 */
> > > -					requeue_io(inode);
> > > -				} else {
> > > -					/*
> > > -					 * somehow blocked: retry later
> > > -					 */
> > > -					redirty_tail(inode);
> > > -				}
> > > +				requeue_io(inode);
> > >  			} else {
> > >  				/*
> > > -				 * Otherwise fully redirty the inode so that
> > > -				 * other inodes on this superblock will get some
> > > -				 * writeout.  Otherwise heavy writing to one
> > > -				 * file would indefinitely suspend writeout of
> > > -				 * all the other files.
> > > +				 * somehow blocked: retry later
> > >  				 */
> > > -				inode->i_state |= I_DIRTY_PAGES;
> > >  				redirty_tail(inode);
> > >  			}
> > 
> > This means that congestion will always trigger redirty_tail(). Is
> > that really what we want for that case?
> 
> This patch actually converts some redirty_tail() cases to use
> requeue_io(), so are reducing the use of redirty_tail(). Also
> recent kernels are blocked _inside_ get_request() on congestion
> instead of returning to writeback_single_inode() on congestion.
> So the "somehow blocked" comment for redirty_tail() no longer includes
> the congestion case.

Shouldn't some of this be in the comment explain why the tail is
redirtied rather than requeued?

> > Also, I'd prefer that the
> > comments remain somewhat more descriptive of the circumstances that
> > we are operating under. Comments like "retry later to avoid blocking
> > writeback of other inodes" is far, far better than "retry later"
> > because it has "why" component that explains the reason for the
> > logic. You may remember why, but I sure won't in a few months time....
> 
> Ah yes the comment is too simple. However the redirty_tail() is not to
> avoid blocking writeback of other inodes, but to avoid eating 100% CPU
> on busy retrying a dirty inode/page that cannot perform writeback for
> a while. (In theory redirty_tail() can still busy retry though, when
> there is only one single dirty inode.) So how about
> 
>         /*
>          * somehow blocked: avoid busy retrying
>          */

IMO, no better than "somehow blocked: retry later" because it
desont' include any of the explanation for the code you just gave
me.  The comment needs to tell us _why_ we are calling
redirty_tail, not what redirty_tail does. Perhaps something like:

	/*
	 * Writeback blocked by something other than congestion.
	 * Redirty the inode to avoid spinning on the CPU retrying
	 * writeback of the dirty page/inode that cannot be
	 * performed immediately. This allows writeback of other
	 * inodes until the blocking condition clears.
	 */

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com