Fix a race condition between recovery and backup

[jowlyzhang]

A race condition between recovery and backup can happen with error messages like this:
Failure in BackupEngine::CreateNewBackup with: IO error: No such file or directory: While opening a file for sequentially reading: /dev/shm/rocksdb_test/rocksdb_crashtest_whitebox/002653.log: No such file or directory

PR #6949 introduced disabling file deletion during error handling of manifest IO errors. Aformentioned race condition is caused by this chain of event:

[Backup engine] disable file deletion
[Recovery] disable file deletion <= this is optional for the race condition, it may or may not get called
[Backup engine] get list of file to copy/link
[Recovery] force enable file deletion
.... some files refered by backup engine get deleted
[Backup engine] copy/link file <= error no file found

This PR fixes this with:

1. Recovery thread is currently forcing enabling file deletion as long as file deletion is disabled. Regardless of whether the previous error handling is for manifest IO error and that disabled it in the first place. This means it could incorrectly enabling file deletions intended by other threads like backup threads, file snapshotting threads. This PR does this check explicitly before making the call.

2. disable_delete_obsolete_files_ is designed as a counter to allow different threads to enable and disable file deletion separately. The recovery thread currently does a force enable file deletion, because ErrorHandler::SetBGError() can be called multiple times by different threads when they receive a manifest IO error(details per PR First step towards handling MANIFEST write error #6949), resulting in DBImpl::DisableFileDeletions to be called multiple times too. Making a force enable file deletion call that resets the counter disable_delete_obsolete_files_ to zero is a workaround for this. However, as it shows in the race condition, it can incorrectly suppress other threads like a backup thread's intention to keep the file deletion disabled. This PR adds a std::atomic<int> disable_file_deletion_count_ to the error handler to track the needed counter decrease more precisely. This PR tracks and caps file deletion enabling/disabling in error handler.

3. for recovery, the section to find obsolete files and purge them was moved to be done after the attempt to enable file deletion. The actual finding and purging is more likely to happen if file deletion was previously disabled and get re-enabled now. An internal function DBImpl::EnableFileDeletionsWithLock was added to support change 2) and 3). Some useful logging was explicitly added to keep those log messages around.

Test plan:
existing unit tests

[facebook-github-bot]

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

[ajkr]

Nice finding! I think ideally we wouldn't rely on disable_delete_obsolete_files_ for preserving SST files whose MANIFEST append/sync returned an error. In case of an error, we don't know the state of the MANIFEST - it might refer to the newly created files, in which case they're not really obsolete, or it might not.

Is there a way that file deletion can check with the ErrorHandler whether seemingly obsolete files are needed due to manifest recovery?

[anand1976]

Is there a need to track the number of times file deletion is disabled by the error handler? I wonder if it would be simpler to check and do it once in SetBGError if not done already, and reenable it in ClearBGError?

[jowlyzhang]

That's a good point. The main motivation is for the recovery and other threads like backup thread to express their need to disable file deletion equally, so that the recovery thread cannot re-enable file deletion against the will of the backup engine thread.

Quoting the PR that added this file deletion disabling in the first place #6949
multiple threads can call LogAndApply() at the same time, though only one of them will be going through the process MANIFEST write, possibly batching the version edits of other threads. When the leading MANIFEST writer finishes, all of the MANIFEST writing threads in this batch will have the same IOError. They will all call ErrorHandler::SetBGError() in which file deletion will be disabled

So SetBGError can potentially be called multiple times, while ClearBGError should only be called once by the recovery thread. The original implementation work around this with a EnableFileDeletions(/*force*/true). However, as this race condition shows, it can trump other thread's attempt to keep file deletion disabled. So here we track this number more precisely to avoid this.

[anand1976]

Sure, I understand the problem. I'm just suggesting that EnableFileDeletions be called from ClearBGError, so the details are not exposed here to DBImpl.

[jowlyzhang]

Sorry I missed an important detail in your original comment, which is to "do it once in SetGBError if not done already. A boolean type to cap the counter per recovery to 1 makes sense to me and is more efficient. Moving the EnableFileDeletions to be within error handler so that the details are contained is an improvement too. EnableFileDeletions will acquire the mutex while ClearBGError already holds the mutex, so we still needed a EnableFileDeletionsWithLock version of the API, plus there are some useful db info logging related to enabling to keep around that ideally should be done outside of the mutex. I wonder for these reasons, maybe it's still worth keeping this in DBImpl, WDYT?

[anand1976]

We do log info in SetBGError and other places in error_handler.cc while holding the mutex. A retryable error and recovery from it is relatively rare, so should be ok I think. I agree with keeping EnableFileDeletionsWithLock.

[jowlyzhang]

Thank you for the thorough check. This makes sense. I have updated the PR with these improvement suggestions.

[jowlyzhang]

Nice finding! I think ideally we wouldn't rely on disable_delete_obsolete_files_ for preserving SST files whose MANIFEST append/sync returned an error. In case of an error, we don't know the state of the MANIFEST - it might refer to the newly created files, in which case they're not really obsolete, or it might not.

Is there a way that file deletion can check with the ErrorHandler whether seemingly obsolete files are needed due to manifest recovery?

Thank you for the proposal. This is a more efficient and targeted way to handle the original issue. You are right that disabling file deletion could be an overkill. I will look into this and some other things in the obsolete file deletion procedures. I also saw a few Deleting non-existing... type of warning logs while checking this, will try to fix them too.

[facebook-github-bot]

@jowlyzhang has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@jowlyzhang has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@jowlyzhang has updated the pull request. You must reimport the pull request before landing.

[anand1976]

LGTM. Thanks for addressing all the comments!

[facebook-github-bot]

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

[facebook-github-bot]

@jowlyzhang has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

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

[jowlyzhang]

LGTM. Thanks for addressing all the comments!

Thank you for the detailed review and improvement suggestions!

[facebook-github-bot]

@jowlyzhang merged this pull request in 933ee29.

[ajkr]

Nice finding! I think ideally we wouldn't rely on disable_delete_obsolete_files_ for preserving SST files whose MANIFEST append/sync returned an error. In case of an error, we don't know the state of the MANIFEST - it might refer to the newly created files, in which case they're not really obsolete, or it might not.

Is there a way that file deletion can check with the ErrorHandler whether seemingly obsolete files are needed due to manifest recovery?

By the way I didn't fully explain the thought process here. There is one more step to show the potential problem. That is, user may call EnableFileDeletions(), which by default sets force=true, and interfere with recovery. It's actually pretty common API, for example, a physical shard transfer involves disabling file deletions, getting live file listing, copying the files, and then reenabling file deletions. If people do that without thinking about potential auto-recovery happening in the background (because how would they know :) ), the file deletion reenable step can cause necessary files to be deleted, which in turn causes crash-recovery to fail.

[jowlyzhang]

Nice finding! I think ideally we wouldn't rely on disable_delete_obsolete_files_ for preserving SST files whose MANIFEST append/sync returned an error. In case of an error, we don't know the state of the MANIFEST - it might refer to the newly created files, in which case they're not really obsolete, or it might not.
Is there a way that file deletion can check with the ErrorHandler whether seemingly obsolete files are needed due to manifest recovery?

By the way I didn't fully explain the thought process here. There is one more step to show the potential problem. That is, user may call EnableFileDeletions(), which by default sets force=true, and interfere with recovery. It's actually pretty common API, for example, a physical shard transfer involves disabling file deletions, getting live file listing, copying the files, and then reenabling file deletions. If people do that without thinking about potential auto-recovery happening in the background (because how would they know :) ), the file deletion reenable step can cause necessary files to be deleted, which in turn causes crash-recovery to fail.

Thank you for the example for how this can lead to a failure mode, not just inefficiency. This made me wonder though is there a strong reason to make the API EnableFileDeletions() having a force mode, let alone making it default to true. Assuming everyone has a good reason to keep the file deletion disabled, the rational thinking would be, let's disable file deletion as long as someone wants it disabled, and only enable it when everyone agrees to re-enable it. Not being able to re enable file deletion immediately (the force mode) would only result in inefficiency, not a correctness issue, right? Assuming every caller handle the re-enable in time, the inefficiency won't be a concern either. what do you think about making the API not having a forcing mode or not default to force?

[ajkr]

Sure. Ideally we change it in a way that breaks noticeably for force==true users and works as usual for force==false users. Also good would be break it noticeably for both.

Still, I wonder if FindObsoleteFiles() is considering these files obsolete too soon.

[jowlyzhang]

Sure. Ideally we change it in a way that breaks noticeably for force==true users and works as usual for force==false users. Also good would be break it noticeably for both.

Still, I wonder if FindObsoleteFiles() is considering these files obsolete too soon.

Thank you for helping check this. It's a good idea to augment FindObsoleteFiles to be considerate for this scenario. I'm thinking of building some context in error handler for FindObsoleteFiles to consult about these to-be-quarantined files on top of the minimum pending output file number mechanism. You may already be familiar with what this file number does, let me describe what I mean by iterating what this file number does. Currently when a background flush/compaction job starts and ends, it reserves and releases a minimum file number for the pending output. FindObsoleteFiles respects the registered live files in VersionSet and refrain from deleting any files with a file number equal to or bigger than this minimum file number. At the end of the background job, releasing this file number happens regardless of its success, the idea is that if it's successful, the temp files would now being registered in the VersionSet's live files, so it won't get deleted. If it fails, the temp files are garbage to be collected, it should be deleted now.

In this manifest IO error case, the job fails, but we want to postpone the deletion of the temp files to be after recovery succeeds. One idea would be, as we continue to release this job's file number from the pending output file pool, we also add this file number to the error handler as the minimum file number of files to quarantine. FindObsoleteFiles will consult this file number too when finding files to delete, and the error handler is responsible for clearing up this file number when recovery succeeds. When multiple background jobs all try to sets this quarantine file number, error handler just keeps the minimum one of them all. Let me know if this idea sounds feasible or if you have any concerns. Thanks for helping me check this!

[ajkr]

In this manifest IO error case, the job fails, but we want to postpone the deletion of the temp files to be after recovery succeeds. One idea would be, as we continue to release this job's file number from the pending output file pool, we also add this file number to the error handler as the minimum file number of files to quarantine. FindObsoleteFiles will consult this file number too when finding files to delete, and the error handler is responsible for clearing up this file number when recovery succeeds. When multiple background jobs all try to sets this quarantine file number, error handler just keeps the minimum one of them all. Let me know if this idea sounds feasible or if you have any concerns. Thanks for helping me check this!

Yes this sounds great. My only concern is space usage growing if flush/compaction happens while error recovery is ongoing. IIRC we disable background work during a hard error like MANIFEST failure. Is that right, and do we also avoid triggering auto-recovery flushes?

[jowlyzhang]

IIRC we disable background work during a hard error like MANIFEST failure. Is that right, and do we also avoid triggering auto-recovery flushes?

These are great questions, thank you for this perspective! I checked the error handling logic more. It's true that all MANIFEST errors are initially marked as either hard or fatal errors that will stop non recovery background work. There is a special case for handling the no WAL scenario that will mark it as soft error (Editted: want to add this important detail, this marking as soft error will also mark the flag soft_error_no_bg_work_ to be true too, so non recovery background work still cannot run) and attempts to trigger a recovery flush

rocksdb/db/error_handler.cc

Lines 477 to 492 in 2e514e4

	} else if (BackgroundErrorReason::kFlushNoWAL == reason ||
	BackgroundErrorReason::kManifestWriteNoWAL == reason) {
	// When the BG Retryable IO error reason is flush without WAL,
	// We map it to a soft error. At the same time, all the background work
	// should be stopped except the BG work from recovery. Therefore, we
	// set the soft_error_no_bg_work_ to true. At the same time, since DB
	// continues to receive writes when BG error is soft error, to avoid
	// to many small memtable being generated during auto resume, the flush
	// reason is set to kErrorRecoveryRetryFlush.
	Status bg_err(new_bg_io_err, Status::Severity::kSoftError);
	CheckAndSetRecoveryAndBGError(bg_err);
	soft_error_no_bg_work_ = true;
	context.flush_reason = FlushReason::kErrorRecoveryRetryFlush;
	recover_context_ = context;
	return StartRecoverFromRetryableBGIOError(bg_io_err);
	} else {

But it seems to me any recovery flush effort, either from manual DB::Resume or auto recovery only actually triggers flush when recovery succeeds:

rocksdb/db/db_impl/db_impl.cc

Lines 389 to 406 in 2e514e4

	if (s.ok()) {
	if (context.flush_reason == FlushReason::kErrorRecoveryRetryFlush) {
	s = RetryFlushesForErrorRecovery(FlushReason::kErrorRecoveryRetryFlush,
	true /* wait */);
	} else {
	// We cannot guarantee consistency of the WAL. So force flush Memtables of
	// all the column families
	FlushOptions flush_opts;
	// We allow flush to stall write since we are trying to resume from error.
	flush_opts.allow_write_stall = true;
	s = FlushAllColumnFamilies(flush_opts, context.flush_reason);
	}
	if (!s.ok()) {
	ROCKS_LOG_INFO(immutable_db_options_.info_log,
	"DB resume requested but failed due to Flush failure [%s]",
	s.ToString().c_str());
	}
	}

At that time, error handler should have cleaned up the quarantine file set or we need to make sure it does. So recovery flush shouldn't be a concern?

[ajkr]

Sure, a quarantine file set makes sense, and in that case recovery flush is not a concern. I was thinking if we prevent file deletion more broadly during recovery then failed recovery flushes could cause dead SST files to accumulate. But I guess your implementation approach will make that not a possibility.