use shared mutex to guard against block files being removed before read #26316

andrewtoth · 2022-10-15T00:35:59Z

Potentially fixed #17161.

It seems there's a race in ReadBlockFromDisk and UndoReadFromDisk where after the block/undo file position is retrieved the actual block/undo file could be removed during prune. This could just cause the read to fail, or it could prevent the file from being removed if the file is already open depending on the platform (https://en.cppreference.com/w/cpp/io/c/remove). This PR uses a shared mutex to lock the removal of files until any reads are completed. This way it doesn't block cs_main while reading so ReadBlockFromDisk usage can be moved outside of cs_main in some cases like #26308.

The first commit also updates the function signature of ReadRawBlockFromDisk to take a block index instead of the file position, and then the file position is retrieved while locking cs_main inside the function. That way it unifies it with ReadBlockFromDisk and can be moved outside cs_main scope inside net_processing in a follow up PR like #26326.

Also see #25232.

DrahtBot · 2022-10-17T18:29:07Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	dergoegge

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26415 (rpc,rest: faster getblock and rest_block by reading raw block by andrewtoth)
#26288 (Enable -Wstring-concatenation and -Wstring-conversion on clang builds by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

dergoegge

Concept ACK

I am wondering if the current approach actually fixes the race mentioned in #17161. Reading the block file will fail if (a) the file was removed or (b) if CBlockIndex::nStatus does not have the BLOCK_HAVE_DATA bit set at the time of calling GetBlockPos() in ReadBlockFromDisk. In this PR you cover (a) but not (b), is that right? BlockManager::PruneOneBlockFile unsets the bit during pruning before UnlinkPrunedFiles is called, so it seems to like the race could still happen.

dergoegge · 2022-10-19T10:05:47Z

src/node/blockstorage.cpp

 std::atomic_bool fReindex(false);
 bool fPruneMode = false;
 uint64_t nPruneTarget = 0;
+std::shared_mutex cs_UnlinkFiles;


Suggested change

std::shared_mutex cs_UnlinkFiles;

std::shared_mutex g_unlinkfiles_mutex;

Would be more inline with our naming conventions.

But generally I think we should be removing globals instead of adding more. In this case, we could make ReadBlockFromDisk, ReadRawBlockFromDisk, UndoReadFromDisk, UnlinkPrunedFiles methods of BlockManager and have the mutex owned by that as well. I have not scoped out how big of a refactor that would be but I think that would be preferable.

All places have access to the BlockManager fairly easily so it should be straightforward refactor, with the exception of CZMQPublishRawBlockNotifier::NotifyBlock. I have no idea how I will get a handle to the BlockManager from there :/.

I created #26375 to remove needing to read a block from CZMQPublishRawBlockNotifier::NotifyBlock entirely. After that or something like it the refactor can be done much more easily. Marking as draft until that is merged.

The refactor should be after and separate from the fix.

andrewtoth · 2022-10-19T14:11:00Z

Reading the block file will fail if (a) the file was removed or (b) if CBlockIndex::nStatus does not have the BLOCK_HAVE_DATA bit set at the time of calling GetBlockPos() in ReadBlockFromDisk. In this PR you cover (a) but not (b), is that right? BlockManager::PruneOneBlockFile unsets the bit during pruning before UnlinkPrunedFiles is called, so it seems to like the race could still happen.

Yes, that sounds right to me. However, after this change #17161 will not really be a problem. Since getting the block position inside ReadBlockFromDisk will return an empty position if nStatus does not have BLOCK_HAVE_DATA, the read will fail. Having the read fail should be fine as long as the error can be handled gracefully (like RPC/REST/ZMQ/net_processing) but in validation code cs_main should remain locked so it can't happen.

The real issue this solves is on Windows where if after pindex->GetBlockPos() is called and the file is opened then the files are unlinked. This will cause the file to not be removed. From https://en.cppreference.com/w/cpp/io/c/remove

If the file is currently open by the current or another process, the behavior of this function is implementation-defined

I think we should just avoid that happening altogether since "implementation-defined" sounds scary. This was suggested in #11913 (review) and mentioned in #13903 (comment) as well.

Another strategy could be to not unset CBlockIndex::nFile and CBlockIndex::nDataPos when calling BlockManager::PruneOneBlockFile and then not check for BLOCK_HAVE_DATA in CBlockIndex::GetBlockPos(). That way a read will still succeed even if the block is set to be pruned but the file has not been removed. Then nFile and nDataPos can be zeroed in UnlinkPrunedFiles. But that seems like a bigger change for not much more benefit.

luke-jr · 2022-11-03T22:54:04Z

This seems like only a partial fix, since we should be checking BLOCK_HAVE_DATA within the lock. Otherwise, there's still a race after we check it and the block is read, where the block could be pruned.

OTOH, maybe we should just handle missing block files more gracefully than a failed assertion...

andrewtoth · 2022-11-18T16:07:31Z

Closing in favor of #26533.

andrewtoth mentioned this pull request Oct 15, 2022

rpc/rest/zmq: reduce LOCK(cs_main) scope: ~6 times as many requests per second #26308

Merged

andrewtoth force-pushed the block-read-shared-mutex branch from 1be79d5 to eccc18e Compare October 16, 2022 22:14

andrewtoth mentioned this pull request Oct 17, 2022

rpc: Faster getblock API #25232

Closed

pass block index to ReadRawBlockFromDisk and lock inside

a3c7da9

andrewtoth force-pushed the block-read-shared-mutex branch from eccc18e to 5806421 Compare October 17, 2022 14:44

use shared mutex to guard against files being removed before read

ad67092

andrewtoth force-pushed the block-read-shared-mutex branch from 5806421 to ad67092 Compare October 17, 2022 14:46

andrewtoth mentioned this pull request Oct 17, 2022

net: don't lock cs_main while reading blocks in net processing #26326

Merged

This was referenced Oct 17, 2022

Enable -Wstring-concatenation and -Wstring-conversion on clang builds #26288

Closed

Drop IO priority to idle while reading blocks for peer requests and startup verification #9245

Closed

dergoegge reviewed Oct 19, 2022

View reviewed changes

This was referenced Oct 19, 2022

rpc: Return accurate results for scanblocks #26325

Merged

validation, zmq: pass block in UpdatedBlockTip to avoid duplicate read #26375

Closed

andrewtoth marked this pull request as draft October 24, 2022 18:33

andrewtoth mentioned this pull request Oct 28, 2022

rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block #26415

Merged

andrewtoth closed this Nov 18, 2022

andrewtoth deleted the block-read-shared-mutex branch August 17, 2023 20:40

bitcoin locked and limited conversation to collaborators Aug 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

use shared mutex to guard against block files being removed before read #26316

use shared mutex to guard against block files being removed before read #26316

Uh oh!

andrewtoth commented Oct 15, 2022 •

edited

Loading

Uh oh!

DrahtBot commented Oct 17, 2022 •

edited

Loading

Uh oh!

dergoegge left a comment

Uh oh!

dergoegge Oct 19, 2022

Uh oh!

andrewtoth Oct 20, 2022

Uh oh!

andrewtoth Oct 24, 2022

Uh oh!

luke-jr Nov 3, 2022

Uh oh!

andrewtoth commented Oct 19, 2022 •

edited

Loading

Uh oh!

luke-jr commented Nov 3, 2022

Uh oh!

andrewtoth commented Nov 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	std::shared_mutex cs_UnlinkFiles;
	std::shared_mutex g_unlinkfiles_mutex;

use shared mutex to guard against block files being removed before read #26316

use shared mutex to guard against block files being removed before read #26316

Uh oh!

Conversation

andrewtoth commented Oct 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

dergoegge left a comment

Choose a reason for hiding this comment

Uh oh!

dergoegge Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

andrewtoth Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

andrewtoth Oct 24, 2022

Choose a reason for hiding this comment

Uh oh!

luke-jr Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

andrewtoth commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Nov 3, 2022

Uh oh!

andrewtoth commented Nov 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andrewtoth commented Oct 15, 2022 •

edited

Loading

DrahtBot commented Oct 17, 2022 •

edited

Loading

andrewtoth commented Oct 19, 2022 •

edited

Loading