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