On 09/05/2020 02:53, Thomas Munro wrote:
> On Sat, May 9, 2020 at 9:21 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> I noticed that commit 3eb77eba5a changed the logic in
>> ProcessSyncRequests() (formerly mdsync()) so that if you have fsync=off,
>> the entries are not removed from the pendingOps hash table. I don't
>> think that was intended.
>
> Perhaps we got confused about what the comment "... so that changing
> fsync on the fly behaves sensibly" means. Fsyncing everything you
> missed when you turn it back on after a period running with it off
> does sound a bit like behaviour that someone might want or expect,
> though it probably isn't really enough to guarantee durability,
> because requests queued here aren't the only fsyncs you missed while
> you had it off, among other problems.
Yeah, you need to run "sync" after turning fsync=on to be safe, there's
no way around it.
> Unfortunately, if you try that on an assertion build, you get a
> failure anyway, so that probably wasn't deliberate:
>
> TRAP: FailedAssertion("(CycleCtr) (entry->cycle_ctr + 1) ==
> sync_cycle_ctr", File: "sync.c", Line: 335)
Ah, I didn't notice that.
>> I propose the attached patch to move the test for enableFsync so that
>> the entries are removed from pendingOps again. It looks larger than it
>> really is because it re-indents the block of code that is now inside the
>> "if (enableFsync)" condition.
>
> Yeah, I found that git diff/show -w made it easier to understand that
> change. LGTM, though I'd be tempted to use "goto skip" instead of
> incurring that much indentation but up to you ...
I considered a goto too, but I found it confusing. If we need any more
nesting here in the future, I think extracting the inner parts into a
function would be good.
Committed, thanks!
- Heikki
