On Tue 02-07-13 09:13:43, Linus Torvalds wrote:
> On Tue, Jul 2, 2013 at 7:05 AM, Jan Kara <j...@suse.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 <j...@suse.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 <j...@suse.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
