Refactor SyncWAL and SyncClosedLogs for code sharing

[pdillinger]

Summary: These functions were very similar and did not make sense for maintaining separately. This is not a pure refactor but I think bringing the behaviors closer together should reduce long term risk of unintentionally divergent behavior. This change is motivated by some forthcoming WAL handling fixes for Checkpoint and Backups.

• Sync() is always used on closed WALs, like the old SyncClosedWals. SyncWithoutFlush() is only used on the active (maybe) WAL. Perhaps SyncWithoutFlush() should be used whenever available, but I don't know which is preferred, as the previous state of the code was inconsistent.
• Syncing the WAL dir is selective based on need, like old SyncWAL, rather than done always like old SyncClosedLogs. This could be a performance improvement that was never applied to SyncClosedLogs but now is. We might still sync the dir more times than necessary in the case of parallel SyncWAL variants, but on a good FileSystem that's probably not too different performance-wise from us implementing something to have threads wait on each other.

Cosmetic changes:

• Rename internal function SyncClosedLogs to SyncClosedWals
• Merging the sync points into the common implementation between the two entry points isn't pretty, but should be fine.

Recommended follow-up:

• Clean up more confusing naming like log_dirsynced

Test Plan: existing tests

[facebook-github-bot]

@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[anand1976]

Sync() is always used on closed WALs, like the old SyncClosedWals. SyncWithoutFlush() is only used on the active (maybe) WAL.

Flush() is not safe to call concurrently. That's probably why SyncWAL() calls SyncWithoutFlush() and SyncClosedLogs() calls Sync().

[pdillinger]

Looks good. I think we need to call SyncWithoutFlush() in the SyncWAL() path. The rest looks good.

DBImpl::SwitchMemtable() guarantees that WALs before the current are flushed (cur_log_writer->WriteBuffer(write_options)). I don't see the problem with using Sync on such WALs, as SyncClosedLogs() already did before this change.

[anand1976]

DBImpl::SwitchMemtable() guarantees that WALs before the current are flushed (cur_log_writer->WriteBuffer(write_options)).

Indeed it does. Thanks for digging into it. So calling Sync concurrently shouldn't be a problem in practice, although it still makes me a little uncomfortable.

I don't see the problem with using Sync on such WALs, as SyncClosedLogs() already did before this change.

Can SyncClosedLogs() be called concurrently? I believe not, since its only called during Flush. But I'm not 100% sure.

Also, why not always call SyncWithoutFlush() to be on the safe side if there's nothing to be flushed?

[anand1976]

Can SyncClosedLogs() be called concurrently? I believe not, since its only called during Flush. But I'm not 100% sure.

Never mind. Its called from the background thread, so I guess it can be.

[facebook-github-bot]

@pdillinger merged this pull request in facebook/rocksdb@7127119ae9021fc1ebe8ec7b45c2b9c7c825f102.