linux-kernel - Re: Sync writeback still broken

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: <20101030131518.GA8234@infradead.org>
Date:	Sat, 30 Oct 2010 09:15:19 -0400
From:	Christoph Hellwig <hch@...radead.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Jan Engelhardt <jengelh@...ozas.de>, Jan Kara <jack@...e.cz>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>, stable@...nel.org,
	Greg KH <gregkh@...e.de>, Jens Axboe <jaxboe@...ionio.com>
Subject: Re: Sync writeback still broken

On Fri, Oct 29, 2010 at 06:16:48PM -0700, Linus Torvalds wrote:
> Btw, is the problem just that insane WB_SYNC_ALL thing?
> 
> The problem with WB_SYNC_ALL seems to be that it synchrnously writes
> out one inode at a time. And it's not just the data, it's the inode
> itself.
> 
> So rather than write out all pages for _all_ inodes, and then wait for
> them, and write out _all_ metadata, and then wait for that, it seems
> like the WB_SYNC_ALL code does the truly stupid thing, which is to
> "write out some data for one inode, then _synchronously_ wait for
> that, then write out the metadata for that single inode, then
> _synchronously_ wait for THAT" and then rinse and repeat for each
> inode.
> 
> The sane thing for "WB_SYNC_ALL" would be to:
>  - for_each_inode: write out all data (no waiting)
>  - for_each_inode: wait for the data for that inode, write out the inode
>  - for_each_inode: wait for the inode

What we do currently at a high level is:

	for_each_inode: write data (no waiting)
	for_each_inode: write metadata (no waiting)
	for_each_inode: write and wait on data
	for_each_inode: write and wait on metadata

except that as pointed out in the earlier thread we switch around inodes
in a really dumb way.  It's still not quite as efficient as your version
above.  I'd like to get to something similar to your by splitting the
data and metadata list, but for now we have a fairly nice workaround in
XFS for the inefficient metadata writes.

We basically do not start any real I/O at all in ->write_inode but only
put the inode changes in the log, and do we single force of the log
contents at the end of the sync process.  Dimitri has implemented
something very similar for ext4 as well recently.  Together with fixing
the behaviour of randomly switching between the inodes that should get
us to a point where we are doing pretty well.

I'd still like to see the split data/metadata dirty tracking and
writeout in common code eventually.

> 
> so that you avoid the whole synchronous wait thing, and can do all
> inodes in one go.
> 
> I dunno. Who even uses WB_SYNC_ALL? It's just "sync()" itself, isn't
> it? And "umount()", I guess. I didn't actually look at the code.

It's sync/umount as for the writeback threads are concerned, we of
course use the flag for synchronous writeout of single inodes, but the
issue does not apply there.

--
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/