| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 22 Sep 2009 17:14:13 +0800
From: Wu Fengguang <fengguang.wu@...el.com>
To: Chris Mason <chris.mason@...cle.com>, Jan Kara <jack@...e.cz>,
"jens.axboe@...cle.com" <jens.axboe@...cle.com>,
LKML <linux-kernel@...r.kernel.org>,
Dave Chinner <david@...morbit.com>
Subject: Re: [PATCH] fs: Fix busyloop in wb_writeback()
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();
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists