On 09/05/2020 02:53, Thomas Munro wrote:
On Sat, May 9, 2020 at 9:21 AM Heikki Linnakangas <hlinn...@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


Reply via email to