Re: frequent softlockups with 3.10rc6.

[Linus Torvalds <torvalds@linux-foundation.org>]

On Tue, Jul 2, 2013 at 9:57 AM, Jan Kara <jack@suse.cz> wrote:
>
>   sync(2) was always slow in presence of heavy concurrent IO so I don't
> think this is a stable material.

It's not the "sync being slow" part I personally react to. I don't
care that much about that.

It's the "sync slows down other things" part that makes me go "Hmm,
this may be due to interactions with the flushing changes". A slow
sync is fine - a sync that causes the global disk throughput to go
down by also stopping *other* writeback is not.

So it's the effect on the normal background writeback that makes me go
"hmm - have we really always had that, or is this an effect of the old
sync logic _mixed_ with all the bdflush -> worker changes"

The thing is, it used to be that bdflush didn't much care what a sync
by another user was doing. But bdflush doesn't exist any more, it's
all worker threads..

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

My point was - why don't we move that sync thing into the caller (so
write_inode_now() in this case)?

IOW, I'm not disputing the need for filemap_fdatawait() in the data
paths. I'm just saying that maybe we could split things up - including
that whole "write_inode()" call. Some users clearly want to do this in
different orders.

That said, we might also just want to change the "sync_mode" thing.
The thing that I dislike about this patch (even though I applied it)
is that odd

        if (wbc->sync_mode == WB_SYNC_ALL && !wbc->for_sync) {

test. It doesn't make sense to me. It's a hack saying "I know that
'sync' does something special and doesn't actually want this
particular WB_SYNC_ALL behavior at all". That's hacky. Moving that
kind of "I know what the caller *really* meant" logic into the callers
- by splitting up the logic - would get rid of the hacky part.

But another approach of getting rid of the hacky part might be to
simple split - and rename - that "WB_SYNC_ALL" thing, and simply say
"clearly 'sync()' and individual callers of 'write_inode_now()' have
totally different expectations of the semantics of WB_SYNC_ALL". Which
means that they really shouldn't share the same "sync_mode" at all.

So maybe we could just extend that "sync_mode", and have the ones that
want to do _one_ inode synchronously use "WB_SYNC_SINGLE" to make it
clear that they are syncing a single inode. Vs "WB_SYNC_ALL" that
would be used for "I'm syncing all inodes, and I'll do a separate
second pass for syncing".

Then that test would become

        if (wbc->sync_mode == WB_SYNC_SINGLE) {

instead, and now "sync_mode" would actually describe what mode of
syncing the caller wants, without that hacky special "we know what the
caller _really_ meant by looking at *which* caller it is".

See what my objection to the code is? And maybe there is yet another
solution to the oddity, I've just outlined two possible ones..

                   Linus