Messages in this thread | | | Date | Mon, 21 Sep 2009 22:31:07 +0800 | From | Wu Fengguang <> | Subject | Re: [PATCH] fs: Fix busyloop in wb_writeback() |
| |
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: > > > > > On Sun 20-09-09 10:35:28, Wu Fengguang wrote: > > > > > > On Thu, Sep 17, 2009 at 01:22:48AM +0800, Jan Kara wrote: > > > > > > > If all inodes are under writeback (e.g. in case when there's only one inode > > > > > > > with dirty pages), wb_writeback() with WB_SYNC_NONE work basically degrades > > > > > > > to busylooping until I_SYNC flags of the inode is cleared. Fix the problem by > > > > > > > waiting on I_SYNC flags of an inode on b_more_io list in case we failed to > > > > > > > write anything. > > > > > > > > > > > > Sorry, I realized that inode_wait_for_writeback() waits for I_SYNC. > > > > > > But inodes in b_more_io are not expected to have I_SYNC set. So your > > > > > > patch looks like a big no-op? > > > > > Hmm, I don't think so. writeback_single_inode() does: > > > > > if (inode->i_state & I_SYNC) { > > > > > /* > > > > > * If this inode is locked for writeback and we are not > > > > > * doing > > > > > * writeback-for-data-integrity, move it to b_more_io so > > > > > * that > > > > > * writeback can proceed with the other inodes on s_io. > > > > > * > > > > > * We'll have another go at writing back this inode when we > > > > > * completed a full scan of b_io. > > > > > */ > > > > > if (!wait) { > > > > > requeue_io(inode); > > > > > return 0; > > > > > } > > > > > > > > > > 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.
> I'd drop any congestion checks in the nooks and crannies of the > writeback paths.
Let's work on a better solution then?
Thanks, Fengguang
| |