Messages in this thread | | | Date | Tue, 2 Jul 2013 20:28:42 -0700 | Subject | Re: frequent softlockups with 3.10rc6. | From | Linus Torvalds <> |
| |
On Tue, Jul 2, 2013 at 8:07 PM, Dave Chinner <david@fromorbit.com> wrote: >> >> 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". > > The problem is that all the code that currently looks for > WB_SYNC_ALL for it's behavioural cue during writeback now has > multiple different modes they have to handle. IOWs, it's not a > straight forward conversion process. WB_SYNC_ALL reaches right down > into filesystem ->writepages implementations and they all need to be > changed if we make up a new sync_mode behaviour.
I have to admit that I absolutely detest our current "sync_mode" to begin with, so I'd personally be happy to see some major surgery in this area.
For example, maybe we'd be much better off with something that has various behavioral flags rather than distinct "mode values". So instead of being an enum of different reasons for syncing, it would be a set of bitmasks for specific sync behavior. We have a much better sync model in our sync_file_range() model, where we have flags like SYNC_FILE_RANGE_WAIT_xxx (where xxx is BEFORE, WRITE, AFTER to describe whether you should wait for old writes, start new writes, or wait after the newly started writes).
That's a very powerful model, and it's also much more easy to think about. So the above test could become
if (wbc->sync_mode & WB_SYNC_AFTER) { int err = filemap_fdatawait(mapping); ....
in that kind of model, and the code actually looks sensible. It reads like "if the caller asked us to synchronize after writing, then we do an fdatawait on the mapping".
So I think something like that might make sense. And there aren't _that_ many users of WB_SYNC_xxx, and the patch should be pretty straightforward. WB_SYNC_NONE semantics would presumably be "just start writeout" (so it would become WB_SYNC_WRITE), while WB_SYNC_ALL would become (WB_SYNC_BEFORE | WB_SYNC_WRITE | WB_SYNC_AFTER), but then the "for_sync" case would remove WB_SYNC_AFTER, because it does its own waiting after.
Sounds fairly sensible and straightforward to me. Much more self-explanatory than the current "WB_SYNC_NONE/ALL" distinction, methinks (well, you'd still have to explain what the point of BEFORE/AFTER is, and how it interacts with starting writeout, but especially since we already have that concept for file_sync_range(), I think that's not too bad).
Linus
| |