init: Take lock on blocks directory in BlockManager ctor #31860
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31860. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Concept ACK on:
- making LockDirectory more strict, it confused me recently that I was able to acquire a lock multiple times (within the same process)
- making BlockManager manage the lock instead of init
As per TheCharlatan#25, I would be a fan of going further and making the BlockManagerOptions ctor take a BlockDirLock (or, as per my comment a DirectoryLock) to allow more granular error feedback, but this is already a step in the right direction regardless.
In the future, this approach would also make it more straightforward to e.g. let BlockManager give other threads read-only access to the blocksdir via something like:
class DirectoryLock {
private:
std::shared_mutex m_mutex;
public:
const fs::path m_path;
DirectoryLock(const fs::path& path)
: m_path(path)
{
assert(util::LockDirectory(m_path, ".lock", false) == util::LockResult::Success);
}
~DirectoryLock() { UnlockDirectory(m_path, ".lock"); }
DirectoryLock(const DirectoryLock&) = delete;
DirectoryLock& operator=(const DirectoryLock&) = delete;
std::unique_lock<std::shared_mutex> lock_exclusive()
{
return std::unique_lock<std::shared_mutex>(m_mutex);
}
std::shared_lock<std::shared_mutex> lock_shared()
{
return std::shared_lock<std::shared_mutex>(m_mutex);
}
};
src/node/blockstorage.h
Outdated
| @@ -127,6 +127,14 @@ struct BlockfileCursor { | |||
|
|
|||
| std::ostream& operator<<(std::ostream& os, const BlockfileCursor& cursor); | |||
|
|
|||
| class BlockDirLock | |||
There was a problem hiding this comment.
It seems like this class could easily be reused for chainstate, datadir, wallet, ... directories in future commits. Perhaps a class DirectoryLock would make sense?
src/node/blockstorage.h
Outdated
| { | ||
| fs::path m_path; | ||
|
|
||
| public: |
There was a problem hiding this comment.
Copy ctor and assignment operator should probably be deleted?
There was a problem hiding this comment.
And have BlockManager::m_lock be a std::unique_ptr<BlockDirLock> instead?
There was a problem hiding this comment.
I've been thinking about allocating it on the heap instead, but I kind of like that declaring it as the first field forces us to initialize it first, which a pointer type would not necessarily. Then again, moving the lock as a unique pointer from the options to the actual class would also probably be easier than defining a move constructor the directory lock class.
src/init.cpp
Outdated
| // Only allow probing the blocks directory, the BlockManager takes the lock internally. | ||
| return LockDirectory(gArgs.GetDataDirNet(), probeOnly) && | ||
| (probeOnly ? LockDirectory(gArgs.GetBlocksDirPath(), probeOnly) : true); |
There was a problem hiding this comment.
At this point, I don't see the benefit of this function anymore. Let's just inline it?
git diff on 436df34
diff --git a/src/init.cpp b/src/init.cpp
index 579669712e..3a984488af 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1107,12 +1107,6 @@ static bool LockDirectory(const fs::path& dir, bool probeOnly)
} // no default case, so the compiler can warn about missing cases
assert(false);
}
-static bool LockDirectories(bool probeOnly)
-{
- // Only allow probing the blocks directory, the BlockManager takes the lock internally.
- return LockDirectory(gArgs.GetDataDirNet(), probeOnly) &&
- (probeOnly ? LockDirectory(gArgs.GetBlocksDirPath(), probeOnly) : true);
-}
bool AppInitSanityChecks(const kernel::Context& kernel)
{
@@ -1130,7 +1124,8 @@ bool AppInitSanityChecks(const kernel::Context& kernel)
// Probe the directory locks to give an early error message, if possible
// We cannot hold the directory locks here, as the forking for daemon() hasn't yet happened,
// and a fork will cause weird behavior to them.
- return LockDirectories(true);
+ return LockDirectory(gArgs.GetDataDirNet(), /*probeOnly=*/true)
+ && LockDirectory(gArgs.GetBlocksDirPath(), /*probeOnly=*/true);
}
bool AppInitLockDirectories()
@@ -1138,11 +1133,8 @@ bool AppInitLockDirectories()
// After daemonization get the directory locks again and hold on to them until exit
// This creates a slight window for a race condition to happen, however this condition is harmless: it
// will at most make us exit without printing a message to console.
- if (!LockDirectories(false)) {
- // Detailed error printed inside LockDirectory
- return false;
- }
- return true;
+ // Detailed error printed inside LockDirectory
+ return LockDirectory(gArgs.GetDataDirNet(), /*probeOnly=*/false);
}
bool AppInitInterfaces(NodeContext& node)
b1f16d3 to
62ad1d6
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
aab070a to
51436f8
Compare
I attempted implementing this, but giving the options the lock starts to introduce non-trivial additional semantics to the options: Since the lock needs to be moved from the options to the block manager, the options cannot be re-used. I'm not sure how to implement that and make it clear to the user what is going on. |
I haven't tested this, but it may have been required in the past, because BDB wallets may just be files, not folders, so they could all sit in the same walletdir (which could even be the datadir)? If true, this requirement may go away with dropping bdb? |
Previously LockDirectory only prevented concurrent locks across different processes, but allowed the same process to re-lock on the same directory. This change is not immediately relevant for its current use, where the lock is only supposed to protect against a different process writing on the same resources. This change is relevant for future use by the kernel library, where users of the library might mistakenly create multiple instances of an object that seek to write to a common resource.
This makes it easier for a class or a struct to own a lock on a directory for the duration of its lifetime. It is used in the next commit.
This moves the responsibility of taking the lock for the blocks directory into the BlockManager. Use the DirectoryLock wrapper to ensure it is the first resource to be acquired and is released again after use. This is relevant for the kernel library where the lock should be taken even if the user fails to explicitly do so.
fc12a89 to
6975427
Compare
This ensures consistent directory locking for users of the kernel library by disallowing mistakenly creating multiple BlockManagers that might write into the same block or undo files.
The change here makes
LockDirectorymore strict: It now returns an error even if it is called again on the same directory from the same process. However from the description in #12422 , where this feature was introduced, it is not immediately clear what its usage was at the time. Supporting multiple wallets in the same directory is mentioned there, but it is not said why that might be a desirable feature.