Re: [PATCH] fs: Fix busyloop in wb_writeback()

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

On Mon, Sep 21, 2009 at 10:45:51PM +0800, Chris Mason wrote:
> On Mon, Sep 21, 2009 at 10:31:07PM +0800, Wu Fengguang wrote:
> > On Mon, Sep 21, 2009 at 10:19:10PM +0800, Chris Mason wrote:
> > > On Mon, Sep 21, 2009 at 10:11:09PM +0800, Wu Fengguang wrote:
> > > > On Mon, Sep 21, 2009 at 09:45:11PM +0800, Jan Kara wrote:
> > > > > On Mon 21-09-09 09:08:59, Wu Fengguang wrote:
> > > > > > On Mon, Sep 21, 2009 at 01:43:56AM +0800, Jan Kara wrote:
> > > > > > >   So when we see inode under writeback, we put it to b_more_io. So I think
> > > > > > > my patch really fixes the issue when two threads are racing on writing the
> > > > > > > same inode.
> > > > > > 
> > > > > > Ah OK. So it busy loops when there are more syncing threads than dirty
> > > > > > files. For example, one bdi flush thread plus one process running
> > > > > > balance_dirty_pages().
> > > > >   Yes.
> > > > > 
> > > > > > > > The busy loop does exists, when bdi is congested.
> > > > > > > > In this case, write_cache_pages() will refuse to write anything,
> > > > > > > > we used to be calling congestion_wait() to take a breath, but now
> > > > > > > > wb_writeback() purged that call and thus created a busy loop.
> > > > > > >   I don't think congestion is an issue here. The device needen't be
> > > > > > > congested for the busyloop to happen.
> > > > > > 
> > > > > > bdi congestion is a different case. When there are only one syncing
> > > > > > thread, b_more_io inodes won't have I_SYNC, so your patch is a no-op.
> > > > > > wb_writeback() or any of its sub-routines must wait/yield for a while
> > > > > > to avoid busy looping on the congestion. Where is the wait with Jens'
> > > > > > new code?
> > > > >   I agree someone must wait when we bail out due to congestion. But we bail
> > > > > out only when wbc->nonblocking is set.
> > > > 
> > > > Here is another problem. wbc->nonblocking used to be set for kupdate
> > > > and background writebacks, but now it's gone. So they will be blocked
> > > > in get_request_wait(). That's fine, no busy loops.
> > > > 
> > > > However this inverts the priority. pageout() still have nonblocking=1.
> > > > So now vmscan can easily be live locked by heavy background writebacks.
> > > 
> > > The important part of the nonblocking check for pageout is really to
> > > make sure that it doesn't get stuck locking a buffer that is actually
> > > under IO, which happens in ext3/reiserfs data=ordered mode.
> > 
> > OK.
> > 
> > > Having pageout wait for a request is fine.  Its just as likely to wait
> > > for a request when it does actually start the IO, regardless of the
> > > congestion checks earlier in the call chain.
> > 
> > There are fundamental differences. The congestion wait is live lock for
> > pageout, while wait_on_page_writeback() will finish in bounded time.

Ah sorry for making silly mistakes! According Jan Kara, live lock is
not possible because pageout calls ->writepage() directly without
congestion wait.

> > > I'd drop any congestion checks in the nooks and crannies of the
> > > writeback paths.
> > 
> > Let's work on a better solution then?
> 
> Today, wbc->nonblocking and congestion are checked together:
> 
> 1) in writeback_inodes_wb before we call writeback_single_inode
> 2) in write_cache_pages, before we call writepage
> 3) in write_cache_pages, after we call writepage
> 
> If we delete all 3, we get rid of the livelock but keep the check that
> makes sure we don't wait on locked buffers that are under IO.
> 
> If we delete #1 and #2, we'll get rid of the livelock but pageout will
> still stop trying to do IO on this backing dev once it finds some
> congestion.
> 
> I think either way is fine ;)

To remove all these blocks? Looks like good cleanups because now no
one is passing nonblocking to these functions.

Thanks,
Fengguang

---
 fs/fs-writeback.c   |    9 +--------
 mm/page-writeback.c |   11 -----------
 2 files changed, 1 insertion(+), 19 deletions(-)

--- linux.orig/fs/fs-writeback.c	2009-09-22 16:29:58.000000000 +0800
+++ linux/fs/fs-writeback.c	2009-09-22 17:09:25.000000000 +0800
@@ -566,14 +567,6 @@ rescan:
 			continue;
 		}
 
-		if (wbc->nonblocking && bdi_write_congested(wb->bdi)) {
-			wbc->encountered_congestion = 1;
-			if (!is_blkdev_sb)
-				break;		/* Skip a congested fs */
-			requeue_io(inode);
-			continue;		/* Skip a congested blockdev */
-		}
-
 		if (inode_dirtied_after(inode, wbc->older_than_this)) {
 			if (list_empty(&wb->b_more_io))
 				break;
--- linux.orig/mm/page-writeback.c	2009-09-22 17:09:28.000000000 +0800
+++ linux/mm/page-writeback.c	2009-09-22 17:09:47.000000000 +0800
@@ -827,11 +827,6 @@ int write_cache_pages(struct address_spa
 	int range_whole = 0;
 	long nr_to_write = wbc->nr_to_write;
 
-	if (wbc->nonblocking && bdi_write_congested(bdi)) {
-		wbc->encountered_congestion = 1;
-		return 0;
-	}
-
 	pagevec_init(&pvec, 0);
 	if (wbc->range_cyclic) {
 		writeback_index = mapping->writeback_index; /* prev offset */
@@ -950,12 +945,6 @@ continue_unlock:
 					break;
 				}
 			}
-
-			if (wbc->nonblocking && bdi_write_congested(bdi)) {
-				wbc->encountered_congestion = 1;
-				done = 1;
-				break;
-			}
 		}
 		pagevec_release(&pvec);
 		cond_resched();