linux-kernel - Re: frequent softlockups with 3.10rc6.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130702165752.GA12179@quack.suse.cz>
Date:	Tue, 2 Jul 2013 18:57:52 +0200
From:	Jan Kara <jack@...e.cz>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Jan Kara <jack@...e.cz>, Dave Chinner <david@...morbit.com>,
	Dave Jones <davej@...hat.com>, Oleg Nesterov <oleg@...hat.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Andrey Vagin <avagin@...nvz.org>,
	Steven Rostedt <rostedt@...dmis.org>
Subject: Re: frequent softlockups with 3.10rc6.

On Tue 02-07-13 09:13:43, Linus Torvalds wrote:
> On Tue, Jul 2, 2013 at 7:05 AM, Jan Kara <jack@...e.cz> wrote:
> > On Tue 02-07-13 22:38:35, Dave Chinner wrote:
> >>
> >> IOWs, sync is 7-8x faster on a busy filesystem and does not have an
> >> adverse impact on ongoing async data write operations.
> >   The patch looks good. You can add:
> > Reviewed-by: Jan Kara <jack@...e.cz>
> 
> Ok, I'm going to take this patch asap. Should we also mark it for
> stable? It doesn't look like a regression in that particular code, but
> it sounds like it might be a regression when paired with the way the
> flusher threads interact. Or is this really some long-time performance
> problem?
  sync(2) was always slow in presence of heavy concurrent IO so I don't
think this is a stable material.

> I'm also wondering if we should just change all callers - remove that
> "wait for writeback to complete" from writeback_one_inode()
> completely, and just make sure that *all* callers that use WB_SYNC_ALL
> do the "wait for writeback" in a separate stage, the way "sync()"
> already does?
  The trouble is with callers like write_inode_now() from iput_final().
For write_inode_now() to work correctly in that place, you must make sure
page writeback is finished before calling ->write_inode() because
filesystems may (and do) dirty the inode in their ->end_io callbacks. If
you don't wait you risk calling ->evict_inode() on a dirty inode and thus
loosing some updates.

> That whole
> 
>         if (wbc->sync_mode == WB_SYNC_ALL && !wbc->for_sync) {
> 
> test doesn't really look all that sane (..so thanks Dave for adding a
> comment above it)
  I agree the condition looks a bit fishy so it definitely deserves that
comment. The only way I see to avoid this strange condition is to move
do_writepages() from __writeback_single_inode() into the callers
(writeback_single_inode() and writeback_sb_inodes()) and the condition with
the wait would then be only in writeback_single_inode(). But we would also
have to duplicate the trace points so current solution looked a tad bit
better to me.

								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
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/