From 9e9369b9ed157d05b8ab25e5692067dd209bbc2e Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Wed, 13 Jul 2016 11:06:37 +0200 Subject: [PATCH] Performance optimization: When removing or shrinking a tree, we don't load/read leaves anymore. Loading inner nodes is enough to get all block IDs and then we can delete the leaves from their IDs without loading them. --- .../onblocks/BlobStoreOnBlocks.cpp | 4 ++ .../onblocks/BlobStoreOnBlocks.h | 1 + .../onblocks/datanodestore/DataNodeStore.cpp | 45 ++++++++++++----- .../onblocks/datanodestore/DataNodeStore.h | 4 +- .../onblocks/datatreestore/DataTree.cpp | 4 +- .../onblocks/datatreestore/DataTreeStore.cpp | 11 +++-- .../onblocks/datatreestore/DataTreeStore.h | 1 + .../ParallelAccessDataTreeStore.cpp | 3 ++ .../ParallelAccessDataTreeStore.h | 1 + .../ParallelAccessDataTreeStoreAdapter.h | 4 ++ src/blobstore/interface/BlobStore.h | 1 + .../caching/CachingBlockStore.cpp | 9 ++++ .../caching/CachingBlockStore.h | 1 + .../compressing/CompressingBlockStore.h | 9 ++-- .../encrypted/EncryptedBlockStore.h | 9 ++-- .../inmemory/InMemoryBlockStore.cpp | 4 +- .../inmemory/InMemoryBlockStore.h | 2 +- .../ondisk/OnDiskBlockStore.cpp | 4 +- .../implementations/ondisk/OnDiskBlockStore.h | 2 +- .../ParallelAccessBlockStore.cpp | 4 ++ .../parallelaccess/ParallelAccessBlockStore.h | 3 +- .../ParallelAccessBlockStoreAdapter.h | 4 ++ .../testfake/FakeBlockStore.cpp | 4 +- .../implementations/testfake/FakeBlockStore.h | 2 +- .../VersionCountingBlockStore.cpp | 8 +-- .../VersionCountingBlockStore.h | 2 +- src/blockstore/interface/BlockStore.h | 8 ++- .../cachingfsblobstore/CachingFsBlobStore.h | 10 ++++ .../filesystem/fsblobstore/FsBlobStore.h | 5 ++ .../ParallelAccessFsBlobStoreAdapter.h | 14 ++++-- .../ParallelAccessBaseStore.h | 2 + src/parallelaccessstore/ParallelAccessStore.h | 49 ++++++++++++++----- .../onblocks/BlobStoreTest.cpp | 6 +++ .../datatreestore/DataTreeStoreTest.cpp | 25 ++++++++-- ...VersionCountingBlockStoreTest_Specific.cpp | 10 ++-- .../helpers/BlockStoreWithRandomKeysTest.cpp | 1 + test/blockstore/testutils/BlockStoreTest.h | 37 +++++++++++--- 37 files changed, 230 insertions(+), 83 deletions(-) diff --git a/src/blobstore/implementations/onblocks/BlobStoreOnBlocks.cpp b/src/blobstore/implementations/onblocks/BlobStoreOnBlocks.cpp index 31647c60..03166ab8 100644 --- a/src/blobstore/implementations/onblocks/BlobStoreOnBlocks.cpp +++ b/src/blobstore/implementations/onblocks/BlobStoreOnBlocks.cpp @@ -52,6 +52,10 @@ void BlobStoreOnBlocks::remove(unique_ref blob) { _dataTreeStore->remove((*_blob)->releaseTree()); } +void BlobStoreOnBlocks::remove(const Key &key) { + _dataTreeStore->remove(key); +} + uint64_t BlobStoreOnBlocks::virtualBlocksizeBytes() const { return _dataTreeStore->virtualBlocksizeBytes(); } diff --git a/src/blobstore/implementations/onblocks/BlobStoreOnBlocks.h b/src/blobstore/implementations/onblocks/BlobStoreOnBlocks.h index 92234543..53fac8b4 100644 --- a/src/blobstore/implementations/onblocks/BlobStoreOnBlocks.h +++ b/src/blobstore/implementations/onblocks/BlobStoreOnBlocks.h @@ -23,6 +23,7 @@ public: boost::optional> load(const blockstore::Key &key) override; void remove(cpputils::unique_ref blob) override; + void remove(const blockstore::Key &key) override; //TODO Test blocksizeBytes/numBlocks/estimateSpaceForNumBlocksLeft //virtual means "space we can use" as opposed to "space it takes on the disk" (i.e. virtual is without headers, checksums, ...) diff --git a/src/blobstore/implementations/onblocks/datanodestore/DataNodeStore.cpp b/src/blobstore/implementations/onblocks/datanodestore/DataNodeStore.cpp index 11ab6e6a..d244fcb2 100644 --- a/src/blobstore/implementations/onblocks/datanodestore/DataNodeStore.cpp +++ b/src/blobstore/implementations/onblocks/datanodestore/DataNodeStore.cpp @@ -12,6 +12,7 @@ using blockstore::Key; using cpputils::Data; using cpputils::unique_ref; using cpputils::make_unique_ref; +using cpputils::dynamic_pointer_move; using std::runtime_error; using boost::optional; using boost::none; @@ -83,20 +84,42 @@ unique_ref DataNodeStore::overwriteNodeWith(unique_ref targe } void DataNodeStore::remove(unique_ref node) { - auto block = node->node().releaseBlock(); - cpputils::destruct(std::move(node)); // Call destructor - _blockstore->remove(std::move(block)); + Key key = node->key(); + cpputils::destruct(std::move(node)); + remove(key); +} + +void DataNodeStore::remove(const Key &key) { + _blockstore->remove(key); } -void DataNodeStore::removeSubtree(unique_ref node) { - //TODO Make this faster by not loading the leaves but just deleting them. Can be recognized, because of the depth of their parents. - DataInnerNode *inner = dynamic_cast(node.get()); - if (inner != nullptr) { - for (uint32_t i = 0; i < inner->numChildren(); ++i) { - auto child = load(inner->getChild(i)->key()); - ASSERT(child != none, "Couldn't load child node"); - removeSubtree(std::move(*child)); +void DataNodeStore::removeSubtree(const Key &key) { + auto node = load(key); + ASSERT(node != none, "Node for removeSubtree not found"); + + auto inner = dynamic_pointer_move(*node); + if (inner == none) { + ASSERT((*node)->depth() == 0, "If it's not an inner node, it has to be a leaf."); + remove(std::move(*node)); + } else { + _removeSubtree(std::move(*inner)); + } +} + +void DataNodeStore::_removeSubtree(cpputils::unique_ref node) { + if (node->depth() == 1) { + for (uint32_t i = 0; i < node->numChildren(); ++i) { + remove(node->getChild(i)->key()); + } + } else { + ASSERT(node->depth() > 1, "This if branch is only called when our children are inner nodes."); + for (uint32_t i = 0; i < node->numChildren(); ++i) { + auto child = load(node->getChild(i)->key()); + ASSERT(child != none, "Child not found"); + auto inner = dynamic_pointer_move(*child); + ASSERT(inner != none, "Expected inner node as child"); + _removeSubtree(std::move(*inner)); } } remove(std::move(node)); diff --git a/src/blobstore/implementations/onblocks/datanodestore/DataNodeStore.h b/src/blobstore/implementations/onblocks/datanodestore/DataNodeStore.h index 36acf56c..f48efdfe 100644 --- a/src/blobstore/implementations/onblocks/datanodestore/DataNodeStore.h +++ b/src/blobstore/implementations/onblocks/datanodestore/DataNodeStore.h @@ -38,7 +38,8 @@ public: cpputils::unique_ref overwriteNodeWith(cpputils::unique_ref target, const DataNode &source); void remove(cpputils::unique_ref node); - void removeSubtree(cpputils::unique_ref node); + void remove(const blockstore::Key &key); + void removeSubtree(const blockstore::Key &key); //TODO Test blocksizeBytes/numBlocks/estimateSpaceForNumBlocksLeft uint64_t virtualBlocksizeBytes() const; @@ -48,6 +49,7 @@ public: private: cpputils::unique_ref load(cpputils::unique_ref block); + void _removeSubtree(cpputils::unique_ref node); cpputils::unique_ref _blockstore; const DataNodeLayout _layout; diff --git a/src/blobstore/implementations/onblocks/datatreestore/DataTree.cpp b/src/blobstore/implementations/onblocks/datatreestore/DataTree.cpp index a6e9e6ce..cc27fd39 100644 --- a/src/blobstore/implementations/onblocks/datatreestore/DataTree.cpp +++ b/src/blobstore/implementations/onblocks/datatreestore/DataTree.cpp @@ -178,9 +178,7 @@ void DataTree::resizeNumBytes(uint64_t newNumBytes) { ASSERT(neededChildrenForRightBorderNode <= node->numChildren(), "Node has too few children"); // All children to the right of the new right-border-node are removed including their subtree. while(node->numChildren() > neededChildrenForRightBorderNode) { - // TODO removeSubtree() should get the key, I shouldn't load the block here. - // TODO removeSubtree() needs perf optimization: Don't load leaves. - _nodeStore->removeSubtree(_nodeStore->load(node->LastChild()->key()).value()); + _nodeStore->removeSubtree(node->LastChild()->key()); node->removeLastChild(); } }; diff --git a/src/blobstore/implementations/onblocks/datatreestore/DataTreeStore.cpp b/src/blobstore/implementations/onblocks/datatreestore/DataTreeStore.cpp index f5581816..cbbd344f 100644 --- a/src/blobstore/implementations/onblocks/datatreestore/DataTreeStore.cpp +++ b/src/blobstore/implementations/onblocks/datatreestore/DataTreeStore.cpp @@ -35,10 +35,13 @@ unique_ref DataTreeStore::createNewTree() { } void DataTreeStore::remove(unique_ref tree) { - // Remove all nodes except for the root, which will be a leaf. - tree->resizeNumBytes(0); - // Then remove the root node - _nodeStore->remove(tree->releaseRootNode()); + auto key = tree->key(); + cpputils::destruct(std::move(tree)); + remove(key); +} + +void DataTreeStore::remove(const blockstore::Key &key) { + _nodeStore->removeSubtree(key); } } diff --git a/src/blobstore/implementations/onblocks/datatreestore/DataTreeStore.h b/src/blobstore/implementations/onblocks/datatreestore/DataTreeStore.h index 78d0455d..1af464ee 100644 --- a/src/blobstore/implementations/onblocks/datatreestore/DataTreeStore.h +++ b/src/blobstore/implementations/onblocks/datatreestore/DataTreeStore.h @@ -24,6 +24,7 @@ public: cpputils::unique_ref createNewTree(); void remove(cpputils::unique_ref tree); + void remove(const blockstore::Key &key); //TODO Test blocksizeBytes/numBlocks/estimateSpaceForNumBlocksLeft uint64_t virtualBlocksizeBytes() const; diff --git a/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStore.cpp b/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStore.cpp index 8ec82c09..b7431f1f 100644 --- a/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStore.cpp +++ b/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStore.cpp @@ -41,6 +41,9 @@ void ParallelAccessDataTreeStore::remove(unique_ref tree) { return _parallelAccessStore.remove(key, std::move(tree)); } +void ParallelAccessDataTreeStore::remove(const Key &key) { + return _parallelAccessStore.remove(key); +} } diff --git a/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStore.h b/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStore.h index beea020e..ef4e5408 100644 --- a/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStore.h +++ b/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStore.h @@ -25,6 +25,7 @@ public: cpputils::unique_ref createNewTree(); void remove(cpputils::unique_ref tree); + void remove(const blockstore::Key &key); //TODO Test blocksizeBytes/numBlocks/estimateSpaceForNumBlocksLeft uint64_t virtualBlocksizeBytes() const; diff --git a/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStoreAdapter.h b/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStoreAdapter.h index 6e8b1861..ad1cf487 100644 --- a/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStoreAdapter.h +++ b/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStoreAdapter.h @@ -25,6 +25,10 @@ public: return _baseDataTreeStore->remove(std::move(dataTree)); } + void removeFromBaseStore(const blockstore::Key &key) override { + return _baseDataTreeStore->remove(key); + } + private: datatreestore::DataTreeStore *_baseDataTreeStore; diff --git a/src/blobstore/interface/BlobStore.h b/src/blobstore/interface/BlobStore.h index 9110f9dd..5175ec61 100644 --- a/src/blobstore/interface/BlobStore.h +++ b/src/blobstore/interface/BlobStore.h @@ -19,6 +19,7 @@ public: virtual cpputils::unique_ref create() = 0; virtual boost::optional> load(const blockstore::Key &key) = 0; virtual void remove(cpputils::unique_ref blob) = 0; + virtual void remove(const blockstore::Key &key) = 0; virtual uint64_t numBlocks() const = 0; virtual uint64_t estimateSpaceForNumBlocksLeft() const = 0; diff --git a/src/blockstore/implementations/caching/CachingBlockStore.cpp b/src/blockstore/implementations/caching/CachingBlockStore.cpp index 546cacbf..df35bd59 100644 --- a/src/blockstore/implementations/caching/CachingBlockStore.cpp +++ b/src/blockstore/implementations/caching/CachingBlockStore.cpp @@ -61,6 +61,15 @@ void CachingBlockStore::remove(cpputils::unique_ref block) { } } +void CachingBlockStore::remove(const Key &key) { + auto fromCache = _cache.pop(key); + if (fromCache != none) { + remove(make_unique_ref(std::move(*fromCache), this)); + } else { + _baseBlockStore->remove(key); + } +} + uint64_t CachingBlockStore::numBlocks() const { return _baseBlockStore->numBlocks() + _newBlocks.size(); } diff --git a/src/blockstore/implementations/caching/CachingBlockStore.h b/src/blockstore/implementations/caching/CachingBlockStore.h index 5b1a865a..6596fdea 100644 --- a/src/blockstore/implementations/caching/CachingBlockStore.h +++ b/src/blockstore/implementations/caching/CachingBlockStore.h @@ -19,6 +19,7 @@ public: Key createKey() override; boost::optional> tryCreate(const Key &key, cpputils::Data data) override; boost::optional> load(const Key &key) override; + void remove(const Key &key) override; void remove(cpputils::unique_ref block) override; uint64_t numBlocks() const override; uint64_t estimateNumFreeBytes() const override; diff --git a/src/blockstore/implementations/compressing/CompressingBlockStore.h b/src/blockstore/implementations/compressing/CompressingBlockStore.h index c9bfff90..8a877e6e 100644 --- a/src/blockstore/implementations/compressing/CompressingBlockStore.h +++ b/src/blockstore/implementations/compressing/CompressingBlockStore.h @@ -17,7 +17,7 @@ public: Key createKey() override; boost::optional> tryCreate(const Key &key, cpputils::Data data) override; boost::optional> load(const Key &key) override; - void remove(cpputils::unique_ref block) override; + void remove(const Key &key) override; uint64_t numBlocks() const override; uint64_t estimateNumFreeBytes() const override; uint64_t blockSizeFromPhysicalBlockSize(uint64_t blockSize) const override; @@ -62,11 +62,8 @@ boost::optional> CompressingBlockStore:: } template -void CompressingBlockStore::remove(cpputils::unique_ref block) { - auto _block = cpputils::dynamic_pointer_move>(block); - ASSERT(_block != boost::none, "Wrong block type"); - auto baseBlock = (*_block)->releaseBaseBlock(); - return _baseBlockStore->remove(std::move(baseBlock)); +void CompressingBlockStore::remove(const Key &key) { + return _baseBlockStore->remove(key); } template diff --git a/src/blockstore/implementations/encrypted/EncryptedBlockStore.h b/src/blockstore/implementations/encrypted/EncryptedBlockStore.h index 01dd82a6..e921088f 100644 --- a/src/blockstore/implementations/encrypted/EncryptedBlockStore.h +++ b/src/blockstore/implementations/encrypted/EncryptedBlockStore.h @@ -20,7 +20,7 @@ public: Key createKey() override; boost::optional> tryCreate(const Key &key, cpputils::Data data) override; boost::optional> load(const Key &key) override; - void remove(cpputils::unique_ref block) override; + void remove(const Key &key) override; uint64_t numBlocks() const override; uint64_t estimateNumFreeBytes() const override; uint64_t blockSizeFromPhysicalBlockSize(uint64_t blockSize) const override; @@ -70,11 +70,8 @@ boost::optional> EncryptedBlockStore::load(c } template -void EncryptedBlockStore::remove(cpputils::unique_ref block) { - auto encryptedBlock = cpputils::dynamic_pointer_move>(block); - ASSERT(encryptedBlock != boost::none, "Block is not an EncryptedBlock"); - auto baseBlock = (*encryptedBlock)->releaseBlock(); - return _baseBlockStore->remove(std::move(baseBlock)); +void EncryptedBlockStore::remove(const Key &key) { + return _baseBlockStore->remove(key); } template diff --git a/src/blockstore/implementations/inmemory/InMemoryBlockStore.cpp b/src/blockstore/implementations/inmemory/InMemoryBlockStore.cpp index b3907acb..20a66b2a 100644 --- a/src/blockstore/implementations/inmemory/InMemoryBlockStore.cpp +++ b/src/blockstore/implementations/inmemory/InMemoryBlockStore.cpp @@ -42,9 +42,7 @@ optional> InMemoryBlockStore::load(const Key &key) { } } -void InMemoryBlockStore::remove(unique_ref block) { - Key key = block->key(); - cpputils::destruct(std::move(block)); +void InMemoryBlockStore::remove(const Key &key) { int numRemoved = _blocks.erase(key); ASSERT(1==numRemoved, "Didn't find block to remove"); } diff --git a/src/blockstore/implementations/inmemory/InMemoryBlockStore.h b/src/blockstore/implementations/inmemory/InMemoryBlockStore.h index 4069cf3e..aeaf14a1 100644 --- a/src/blockstore/implementations/inmemory/InMemoryBlockStore.h +++ b/src/blockstore/implementations/inmemory/InMemoryBlockStore.h @@ -19,7 +19,7 @@ public: boost::optional> tryCreate(const Key &key, cpputils::Data data) override; boost::optional> load(const Key &key) override; - void remove(cpputils::unique_ref block) override; + void remove(const Key &key) override; uint64_t numBlocks() const override; uint64_t estimateNumFreeBytes() const override; uint64_t blockSizeFromPhysicalBlockSize(uint64_t blockSize) const override; diff --git a/src/blockstore/implementations/ondisk/OnDiskBlockStore.cpp b/src/blockstore/implementations/ondisk/OnDiskBlockStore.cpp index 1384be25..a055d90c 100644 --- a/src/blockstore/implementations/ondisk/OnDiskBlockStore.cpp +++ b/src/blockstore/implementations/ondisk/OnDiskBlockStore.cpp @@ -69,9 +69,7 @@ optional> OnDiskBlockStore::load(const Key &key) { return optional>(OnDiskBlock::LoadFromDisk(_rootdir, key)); } -void OnDiskBlockStore::remove(unique_ref block) { - Key key = block->key(); - cpputils::destruct(std::move(block)); +void OnDiskBlockStore::remove(const Key &key) { OnDiskBlock::RemoveFromDisk(_rootdir, key); } diff --git a/src/blockstore/implementations/ondisk/OnDiskBlockStore.h b/src/blockstore/implementations/ondisk/OnDiskBlockStore.h index c59a3ae2..26ecdb01 100644 --- a/src/blockstore/implementations/ondisk/OnDiskBlockStore.h +++ b/src/blockstore/implementations/ondisk/OnDiskBlockStore.h @@ -17,7 +17,7 @@ public: boost::optional> tryCreate(const Key &key, cpputils::Data data) override; boost::optional> load(const Key &key) override; //TODO Can we make this faster by allowing to delete blocks by only having theiy Key? So we wouldn't have to load it first? - void remove(cpputils::unique_ref block) override; + void remove(const Key &key) override; uint64_t numBlocks() const override; uint64_t estimateNumFreeBytes() const override; uint64_t blockSizeFromPhysicalBlockSize(uint64_t blockSize) const override; diff --git a/src/blockstore/implementations/parallelaccess/ParallelAccessBlockStore.cpp b/src/blockstore/implementations/parallelaccess/ParallelAccessBlockStore.cpp index b85ec31a..7cca43e4 100644 --- a/src/blockstore/implementations/parallelaccess/ParallelAccessBlockStore.cpp +++ b/src/blockstore/implementations/parallelaccess/ParallelAccessBlockStore.cpp @@ -52,6 +52,10 @@ void ParallelAccessBlockStore::remove(unique_ref block) { return _parallelAccessStore.remove(key, std::move(*block_ref)); } +void ParallelAccessBlockStore::remove(const Key &key) { + return _parallelAccessStore.remove(key); +} + uint64_t ParallelAccessBlockStore::numBlocks() const { return _baseBlockStore->numBlocks(); } diff --git a/src/blockstore/implementations/parallelaccess/ParallelAccessBlockStore.h b/src/blockstore/implementations/parallelaccess/ParallelAccessBlockStore.h index 6574ca9b..6fd694b9 100644 --- a/src/blockstore/implementations/parallelaccess/ParallelAccessBlockStore.h +++ b/src/blockstore/implementations/parallelaccess/ParallelAccessBlockStore.h @@ -18,7 +18,8 @@ public: Key createKey() override; boost::optional> tryCreate(const Key &key, cpputils::Data data) override; boost::optional> load(const Key &key) override; - void remove(cpputils::unique_ref block) override; + void remove(const Key &key) override; + void remove(cpputils::unique_ref node) override; uint64_t numBlocks() const override; uint64_t estimateNumFreeBytes() const override; uint64_t blockSizeFromPhysicalBlockSize(uint64_t blockSize) const override; diff --git a/src/blockstore/implementations/parallelaccess/ParallelAccessBlockStoreAdapter.h b/src/blockstore/implementations/parallelaccess/ParallelAccessBlockStoreAdapter.h index 679b30d5..4f0ed8f9 100644 --- a/src/blockstore/implementations/parallelaccess/ParallelAccessBlockStoreAdapter.h +++ b/src/blockstore/implementations/parallelaccess/ParallelAccessBlockStoreAdapter.h @@ -23,6 +23,10 @@ public: return _baseBlockStore->remove(std::move(block)); } + void removeFromBaseStore(const Key &key) override { + return _baseBlockStore->remove(key); + } + private: BlockStore *_baseBlockStore; diff --git a/src/blockstore/implementations/testfake/FakeBlockStore.cpp b/src/blockstore/implementations/testfake/FakeBlockStore.cpp index ec94cf52..7453a07d 100644 --- a/src/blockstore/implementations/testfake/FakeBlockStore.cpp +++ b/src/blockstore/implementations/testfake/FakeBlockStore.cpp @@ -45,9 +45,7 @@ optional> FakeBlockStore::_load(const Key &key) { } } -void FakeBlockStore::remove(unique_ref block) { - Key key = block->key(); - cpputils::destruct(std::move(block)); +void FakeBlockStore::remove(const Key &key) { std::unique_lock lock(_mutex); int numRemoved = _blocks.erase(key); ASSERT(numRemoved == 1, "Block not found"); diff --git a/src/blockstore/implementations/testfake/FakeBlockStore.h b/src/blockstore/implementations/testfake/FakeBlockStore.h index 26991089..365914f1 100644 --- a/src/blockstore/implementations/testfake/FakeBlockStore.h +++ b/src/blockstore/implementations/testfake/FakeBlockStore.h @@ -33,7 +33,7 @@ public: boost::optional> tryCreate(const Key &key, cpputils::Data data) override; boost::optional> load(const Key &key) override; - void remove(cpputils::unique_ref block) override; + void remove(const Key &key) override; uint64_t numBlocks() const override; uint64_t estimateNumFreeBytes() const override; uint64_t blockSizeFromPhysicalBlockSize(uint64_t blockSize) const override; diff --git a/src/blockstore/implementations/versioncounting/VersionCountingBlockStore.cpp b/src/blockstore/implementations/versioncounting/VersionCountingBlockStore.cpp index a735f0f8..6f3738ad 100644 --- a/src/blockstore/implementations/versioncounting/VersionCountingBlockStore.cpp +++ b/src/blockstore/implementations/versioncounting/VersionCountingBlockStore.cpp @@ -58,13 +58,9 @@ namespace blockstore { throw IntegrityViolationError(reason); } - void VersionCountingBlockStore::remove(unique_ref block) { - Key key = block->key(); - auto versionCountingBlock = cpputils::dynamic_pointer_move(block); - ASSERT(versionCountingBlock != boost::none, "Block is not an VersionCountingBlock"); + void VersionCountingBlockStore::remove(const Key &key) { _knownBlockVersions.markBlockAsDeleted(key); - auto baseBlock = (*versionCountingBlock)->releaseBlock(); - _baseBlockStore->remove(std::move(baseBlock)); + _baseBlockStore->remove(key); } uint64_t VersionCountingBlockStore::numBlocks() const { diff --git a/src/blockstore/implementations/versioncounting/VersionCountingBlockStore.h b/src/blockstore/implementations/versioncounting/VersionCountingBlockStore.h index 06d0ceb3..7ee91900 100644 --- a/src/blockstore/implementations/versioncounting/VersionCountingBlockStore.h +++ b/src/blockstore/implementations/versioncounting/VersionCountingBlockStore.h @@ -18,7 +18,7 @@ public: Key createKey() override; boost::optional> tryCreate(const Key &key, cpputils::Data data) override; boost::optional> load(const Key &key) override; - void remove(cpputils::unique_ref block) override; + void remove(const Key &key) override; uint64_t numBlocks() const override; uint64_t estimateNumFreeBytes() const override; uint64_t blockSizeFromPhysicalBlockSize(uint64_t blockSize) const override; diff --git a/src/blockstore/interface/BlockStore.h b/src/blockstore/interface/BlockStore.h index 56361579..c7704b9f 100644 --- a/src/blockstore/interface/BlockStore.h +++ b/src/blockstore/interface/BlockStore.h @@ -20,7 +20,7 @@ public: //TODO Use boost::optional (if key doesn't exist) // Return nullptr if block with this key doesn't exists virtual boost::optional> load(const Key &key) = 0; - virtual void remove(cpputils::unique_ref block) = 0; + virtual void remove(const Key &key) = 0; virtual uint64_t numBlocks() const = 0; //TODO Test estimateNumFreeBytes in all block stores virtual uint64_t estimateNumFreeBytes() const = 0; @@ -31,6 +31,12 @@ public: virtual void forEachBlock(std::function callback) const = 0; + virtual void remove(cpputils::unique_ref block) { + Key key = block->key(); + cpputils::destruct(std::move(block)); + remove(key); + } + cpputils::unique_ref create(const cpputils::Data &data) { while(true) { //TODO Copy (data.copy()) necessary? diff --git a/src/cryfs/filesystem/cachingfsblobstore/CachingFsBlobStore.h b/src/cryfs/filesystem/cachingfsblobstore/CachingFsBlobStore.h index 37c0ddc9..31f98d62 100644 --- a/src/cryfs/filesystem/cachingfsblobstore/CachingFsBlobStore.h +++ b/src/cryfs/filesystem/cachingfsblobstore/CachingFsBlobStore.h @@ -24,6 +24,7 @@ namespace cryfs { cpputils::unique_ref createSymlinkBlob(const boost::filesystem::path &target, const blockstore::Key &parent); boost::optional> load(const blockstore::Key &key); void remove(cpputils::unique_ref blob); + void remove(const blockstore::Key &key); uint64_t virtualBlocksizeBytes() const; uint64_t numBlocks() const; uint64_t estimateSpaceForNumBlocksLeft() const; @@ -76,6 +77,15 @@ namespace cryfs { return _baseBlobStore->remove(std::move(baseBlob)); } + inline void CachingFsBlobStore::remove(const blockstore::Key &key) { + auto fromCache = _cache.pop(key); + if (fromCache != boost::none) { + remove(_makeRef(std::move(*fromCache))); + } else { + _baseBlobStore->remove(key); + } + } + inline void CachingFsBlobStore::releaseForCache(cpputils::unique_ref baseBlob) { blockstore::Key key = baseBlob->key(); _cache.push(key, std::move(baseBlob)); diff --git a/src/cryfs/filesystem/fsblobstore/FsBlobStore.h b/src/cryfs/filesystem/fsblobstore/FsBlobStore.h index cd058ad0..53cb3bed 100644 --- a/src/cryfs/filesystem/fsblobstore/FsBlobStore.h +++ b/src/cryfs/filesystem/fsblobstore/FsBlobStore.h @@ -22,6 +22,7 @@ namespace cryfs { cpputils::unique_ref createSymlinkBlob(const boost::filesystem::path &target, const blockstore::Key &parent); boost::optional> load(const blockstore::Key &key); void remove(cpputils::unique_ref blob); + void remove(const blockstore::Key &key); uint64_t numBlocks() const; uint64_t estimateSpaceForNumBlocksLeft() const; @@ -75,6 +76,10 @@ namespace cryfs { _baseBlobStore->remove(blob->releaseBaseBlob()); } + inline void FsBlobStore::remove(const blockstore::Key &key) { + _baseBlobStore->remove(key); + } + inline std::function FsBlobStore::_getLstatSize() { return [this] (const blockstore::Key &key) { auto blob = load(key); diff --git a/src/cryfs/filesystem/parallelaccessfsblobstore/ParallelAccessFsBlobStoreAdapter.h b/src/cryfs/filesystem/parallelaccessfsblobstore/ParallelAccessFsBlobStoreAdapter.h index 46c7dcd3..36df2ae2 100644 --- a/src/cryfs/filesystem/parallelaccessfsblobstore/ParallelAccessFsBlobStoreAdapter.h +++ b/src/cryfs/filesystem/parallelaccessfsblobstore/ParallelAccessFsBlobStoreAdapter.h @@ -11,20 +11,24 @@ namespace parallelaccessfsblobstore { class ParallelAccessFsBlobStoreAdapter final: public parallelaccessstore::ParallelAccessBaseStore { public: - explicit ParallelAccessFsBlobStoreAdapter(cachingfsblobstore::CachingFsBlobStore *baseBlockStore) - :_baseBlockStore(std::move(baseBlockStore)) { + explicit ParallelAccessFsBlobStoreAdapter(cachingfsblobstore::CachingFsBlobStore *baseBlobStore) + :_baseBlobStore(std::move(baseBlobStore)) { } boost::optional> loadFromBaseStore(const blockstore::Key &key) override { - return _baseBlockStore->load(key); + return _baseBlobStore->load(key); } void removeFromBaseStore(cpputils::unique_ref block) override { - return _baseBlockStore->remove(std::move(block)); + return _baseBlobStore->remove(std::move(block)); + } + + void removeFromBaseStore(const blockstore::Key &key) override { + return _baseBlobStore->remove(key); } private: - cachingfsblobstore::CachingFsBlobStore *_baseBlockStore; + cachingfsblobstore::CachingFsBlobStore *_baseBlobStore; DISALLOW_COPY_AND_ASSIGN(ParallelAccessFsBlobStoreAdapter); }; diff --git a/src/parallelaccessstore/ParallelAccessBaseStore.h b/src/parallelaccessstore/ParallelAccessBaseStore.h index 4a215fb4..d42c49b6 100644 --- a/src/parallelaccessstore/ParallelAccessBaseStore.h +++ b/src/parallelaccessstore/ParallelAccessBaseStore.h @@ -4,6 +4,7 @@ #include #include +#include namespace parallelaccessstore { @@ -13,6 +14,7 @@ public: virtual ~ParallelAccessBaseStore() {} virtual boost::optional> loadFromBaseStore(const Key &key) = 0; virtual void removeFromBaseStore(cpputils::unique_ref block) = 0; + virtual void removeFromBaseStore(const blockstore::Key &key) = 0; }; } diff --git a/src/parallelaccessstore/ParallelAccessStore.h b/src/parallelaccessstore/ParallelAccessStore.h index 28946e8d..96fa5afc 100644 --- a/src/parallelaccessstore/ParallelAccessStore.h +++ b/src/parallelaccessstore/ParallelAccessStore.h @@ -54,6 +54,7 @@ public: boost::optional> load(const Key &key); boost::optional> load(const Key &key, std::function(Resource*)> createResourceRef); void remove(const Key &key, cpputils::unique_ref block); + void remove(const Key &key); private: class OpenResource final { @@ -93,6 +94,9 @@ private: template cpputils::unique_ref _add(const Key &key, cpputils::unique_ref resource, std::function(Resource*)> createResourceRef); + std::future> _resourceToRemoveFuture(const Key &key); + cpputils::unique_ref _waitForResourceToRemove(const Key &key, std::future> resourceToRemoveFuture); + void release(const Key &key); friend class CachedResource; @@ -167,22 +171,45 @@ boost::optional> ParallelAccessStore void ParallelAccessStore::remove(const Key &key, cpputils::unique_ref resource) { - std::future> resourceToRemoveFuture; - { + auto resourceToRemoveFuture = _resourceToRemoveFuture(key); + + cpputils::destruct(std::move(resource)); + + //Wait for last resource user to release it + auto resourceToRemove = _waitForResourceToRemove(key, std::move(resourceToRemoveFuture)); + _baseStore->removeFromBaseStore(std::move(resourceToRemove)); +} + +template +std::future> ParallelAccessStore::_resourceToRemoveFuture(const Key &key) { std::lock_guard lock(_mutex); // TODO Lock needed for _resourcesToRemove? auto insertResult = _resourcesToRemove.emplace(key, std::promise < cpputils::unique_ref < Resource >> ()); ASSERT(true == insertResult.second, "Inserting failed"); - resourceToRemoveFuture = insertResult.first->second.get_future(); - } - cpputils::destruct(std::move(resource)); - //Wait for last resource user to release it - auto resourceToRemove = resourceToRemoveFuture.get(); + return insertResult.first->second.get_future(); +}; - std::lock_guard lock(_mutex); // TODO Just added this as a precaution on a whim, but I seriously need to rethink locking here. - _resourcesToRemove.erase(key); //TODO Is this erase causing a race condition? +template +cpputils::unique_ref ParallelAccessStore::_waitForResourceToRemove(const Key &key, std::future> resourceToRemoveFuture) { + auto resourceToRemove = resourceToRemoveFuture.get(); - _baseStore->removeFromBaseStore(std::move(resourceToRemove)); -} + std::lock_guard lock(_mutex); // TODO Just added this as a precaution on a whim, but I seriously need to rethink locking here. + _resourcesToRemove.erase(key); //TODO Is this erase causing a race condition? + + return resourceToRemove; +}; + +template +void ParallelAccessStore::remove(const Key &key) { + auto found = _openResources.find(key); + if (found != _openResources.end()) { + auto resourceToRemoveFuture = _resourceToRemoveFuture(key); + //Wait for last resource user to release it + auto resourceToRemove = _waitForResourceToRemove(key, std::move(resourceToRemoveFuture)); + _baseStore->removeFromBaseStore(std::move(resourceToRemove)); + } else { + _baseStore->removeFromBaseStore(key); + } +}; template void ParallelAccessStore::release(const Key &key) { diff --git a/test/blobstore/implementations/onblocks/BlobStoreTest.cpp b/test/blobstore/implementations/onblocks/BlobStoreTest.cpp index 870ffe10..05bf9d26 100644 --- a/test/blobstore/implementations/onblocks/BlobStoreTest.cpp +++ b/test/blobstore/implementations/onblocks/BlobStoreTest.cpp @@ -30,6 +30,12 @@ TEST_F(BlobStoreTest, BlobIsNotLoadableAfterDeletion_DeleteDirectly) { EXPECT_FALSE((bool)blobStore->load(key)); } +TEST_F(BlobStoreTest, BlobIsNotLoadableAfterDeletion_DeleteByKey) { + auto key = blobStore->create()->key(); + blobStore->remove(key); + EXPECT_FALSE((bool)blobStore->load(key)); +} + TEST_F(BlobStoreTest, BlobIsNotLoadableAfterDeletion_DeleteAfterLoading) { auto blob = blobStore->create(); Key key = blob->key(); diff --git a/test/blobstore/implementations/onblocks/datatreestore/DataTreeStoreTest.cpp b/test/blobstore/implementations/onblocks/datatreestore/DataTreeStoreTest.cpp index 01da3611..865857db 100644 --- a/test/blobstore/implementations/onblocks/datatreestore/DataTreeStoreTest.cpp +++ b/test/blobstore/implementations/onblocks/datatreestore/DataTreeStoreTest.cpp @@ -33,7 +33,7 @@ TEST_F(DataTreeStoreTest, NewTreeIsLeafOnly) { EXPECT_IS_LEAF_NODE(tree->key()); } -TEST_F(DataTreeStoreTest, TreeIsNotLoadableAfterRemove) { +TEST_F(DataTreeStoreTest, TreeIsNotLoadableAfterRemove_DeleteByTree) { Key key = treeStore.createNewTree()->key(); auto tree = treeStore.load(key); EXPECT_NE(none, tree); @@ -41,14 +41,31 @@ TEST_F(DataTreeStoreTest, TreeIsNotLoadableAfterRemove) { EXPECT_EQ(none, treeStore.load(key)); } -TEST_F(DataTreeStoreTest, RemovingTreeRemovesAllNodesOfTheTree) { - auto key = CreateThreeLevelMinData()->key(); - auto tree1 = treeStore.load(key).value(); +TEST_F(DataTreeStoreTest, TreeIsNotLoadableAfterRemove_DeleteByKey) { + Key key = treeStore.createNewTree()->key(); + treeStore.remove(key); + EXPECT_EQ(none, treeStore.load(key)); +} + +TEST_F(DataTreeStoreTest, RemovingTreeRemovesAllNodesOfTheTree_DeleteByTree) { + auto tree1_key = CreateThreeLevelMinData()->key(); auto tree2_key = treeStore.createNewTree()->key(); + auto tree1 = treeStore.load(tree1_key).value(); treeStore.remove(std::move(tree1)); //Check that the only remaining node is tree2 EXPECT_EQ(1u, nodeStore->numNodes()); EXPECT_NE(none, treeStore.load(tree2_key)); } + +TEST_F(DataTreeStoreTest, RemovingTreeRemovesAllNodesOfTheTree_DeleteByKey) { + auto tree1_key = CreateThreeLevelMinData()->key(); + auto tree2_key = treeStore.createNewTree()->key(); + + treeStore.remove(tree1_key); + + //Check that the only remaining node is tree2 + EXPECT_EQ(1u, nodeStore->numNodes()); + EXPECT_NE(none, treeStore.load(tree2_key)); +} diff --git a/test/blockstore/implementations/versioncounting/VersionCountingBlockStoreTest_Specific.cpp b/test/blockstore/implementations/versioncounting/VersionCountingBlockStoreTest_Specific.cpp index d79534d1..e86039ca 100644 --- a/test/blockstore/implementations/versioncounting/VersionCountingBlockStoreTest_Specific.cpp +++ b/test/blockstore/implementations/versioncounting/VersionCountingBlockStoreTest_Specific.cpp @@ -105,7 +105,7 @@ public: } void deleteBlock(const blockstore::Key &key) { - blockStore->remove(blockStore->load(key).value()); + blockStore->remove(key); } void insertBaseBlock(const blockstore::Key &key, Data data) { @@ -198,7 +198,7 @@ TEST_F(VersionCountingBlockStoreTest, DeletionPrevention_AllowsDeletingBlocksWhe unique_ptr blockStore; std::tie(baseBlockStore, blockStore) = makeBlockStoreWithoutDeletionPrevention(); auto key = blockStore->create(Data(0))->key(); - baseBlockStore->remove(baseBlockStore->load(key).value()); + baseBlockStore->remove(key); EXPECT_EQ(boost::none, blockStore->load(key)); } @@ -208,7 +208,7 @@ TEST_F(VersionCountingBlockStoreTest, DeletionPrevention_DoesntAllowDeletingBloc unique_ptr blockStore; std::tie(baseBlockStore, blockStore) = makeBlockStoreWithDeletionPrevention(); auto key = blockStore->create(Data(0))->key(); - baseBlockStore->remove(baseBlockStore->load(key).value()); + baseBlockStore->remove(key); EXPECT_THROW( blockStore->load(key), IntegrityViolationError @@ -221,7 +221,7 @@ TEST_F(VersionCountingBlockStoreTest, DeletionPrevention_InForEachBlock_AllowsDe unique_ptr blockStore; std::tie(baseBlockStore, blockStore) = makeBlockStoreWithoutDeletionPrevention(); auto key = blockStore->create(Data(0))->key(); - baseBlockStore->remove(baseBlockStore->load(key).value()); + baseBlockStore->remove(key); int count = 0; blockStore->forEachBlock([&count] (const blockstore::Key &) { ++count; @@ -235,7 +235,7 @@ TEST_F(VersionCountingBlockStoreTest, DeletionPrevention_InForEachBlock_DoesntAl unique_ptr blockStore; std::tie(baseBlockStore, blockStore) = makeBlockStoreWithDeletionPrevention(); auto key = blockStore->create(Data(0))->key(); - baseBlockStore->remove(baseBlockStore->load(key).value()); + baseBlockStore->remove(key); EXPECT_THROW( blockStore->forEachBlock([] (const blockstore::Key &) {}), IntegrityViolationError diff --git a/test/blockstore/interface/helpers/BlockStoreWithRandomKeysTest.cpp b/test/blockstore/interface/helpers/BlockStoreWithRandomKeysTest.cpp index a34ccf45..d4f7a8fd 100644 --- a/test/blockstore/interface/helpers/BlockStoreWithRandomKeysTest.cpp +++ b/test/blockstore/interface/helpers/BlockStoreWithRandomKeysTest.cpp @@ -29,6 +29,7 @@ public: } MOCK_METHOD1(do_load, Block*(const Key &)); void remove(unique_ref block) {UNUSED(block);} + MOCK_METHOD1(remove, void(const Key &)); MOCK_CONST_METHOD0(numBlocks, uint64_t()); MOCK_CONST_METHOD0(estimateNumFreeBytes, uint64_t()); MOCK_CONST_METHOD1(blockSizeFromPhysicalBlockSize, uint64_t(uint64_t)); diff --git a/test/blockstore/testutils/BlockStoreTest.h b/test/blockstore/testutils/BlockStoreTest.h index de5672b5..abc32b01 100644 --- a/test/blockstore/testutils/BlockStoreTest.h +++ b/test/blockstore/testutils/BlockStoreTest.h @@ -78,7 +78,7 @@ TYPED_TEST_P(BlockStoreTest, TwoCreatedBlocksHaveDifferentKeys) { EXPECT_NE(block1->key(), block2->key()); } -TYPED_TEST_P(BlockStoreTest, BlockIsNotLoadableAfterDeleting) { +TYPED_TEST_P(BlockStoreTest, BlockIsNotLoadableAfterDeleting_DeleteByBlock) { auto blockStore = this->fixture.createBlockStore(); auto blockkey = blockStore->create(cpputils::Data(1024))->key(); auto block = blockStore->load(blockkey); @@ -87,6 +87,13 @@ TYPED_TEST_P(BlockStoreTest, BlockIsNotLoadableAfterDeleting) { EXPECT_EQ(boost::none, blockStore->load(blockkey)); } +TYPED_TEST_P(BlockStoreTest, BlockIsNotLoadableAfterDeleting_DeleteByKey) { + auto blockStore = this->fixture.createBlockStore(); + auto blockkey = blockStore->create(cpputils::Data(1024))->key(); + blockStore->remove(blockkey); + EXPECT_EQ(boost::none, blockStore->load(blockkey)); +} + TYPED_TEST_P(BlockStoreTest, NumBlocksIsCorrectOnEmptyBlockstore) { auto blockStore = this->fixture.createBlockStore(); EXPECT_EQ(0u, blockStore->numBlocks()); @@ -104,13 +111,20 @@ TYPED_TEST_P(BlockStoreTest, NumBlocksIsCorrectAfterAddingOneBlock_AfterClosingB EXPECT_EQ(1u, blockStore->numBlocks()); } -TYPED_TEST_P(BlockStoreTest, NumBlocksIsCorrectAfterRemovingTheLastBlock) { +TYPED_TEST_P(BlockStoreTest, NumBlocksIsCorrectAfterRemovingTheLastBlock_DeleteByBlock) { auto blockStore = this->fixture.createBlockStore(); auto block = blockStore->create(cpputils::Data(1)); blockStore->remove(std::move(block)); EXPECT_EQ(0u, blockStore->numBlocks()); } +TYPED_TEST_P(BlockStoreTest, NumBlocksIsCorrectAfterRemovingTheLastBlock_DeleteByKey) { + auto blockStore = this->fixture.createBlockStore(); + auto key = blockStore->create(cpputils::Data(1))->key(); + blockStore->remove(key); + EXPECT_EQ(0u, blockStore->numBlocks()); +} + TYPED_TEST_P(BlockStoreTest, NumBlocksIsCorrectAfterAddingTwoBlocks) { auto blockStore = this->fixture.createBlockStore(); auto block1 = blockStore->create(cpputils::Data(1)); @@ -139,7 +153,7 @@ TYPED_TEST_P(BlockStoreTest, NumBlocksIsCorrectAfterAddingTwoBlocks_AfterClosing EXPECT_EQ(2u, blockStore->numBlocks()); } -TYPED_TEST_P(BlockStoreTest, NumBlocksIsCorrectAfterRemovingABlock) { +TYPED_TEST_P(BlockStoreTest, NumBlocksIsCorrectAfterRemovingABlock_DeleteByBlock) { auto blockStore = this->fixture.createBlockStore(); auto block = blockStore->create(cpputils::Data(1)); blockStore->create(cpputils::Data(1)); @@ -147,6 +161,14 @@ TYPED_TEST_P(BlockStoreTest, NumBlocksIsCorrectAfterRemovingABlock) { EXPECT_EQ(1u, blockStore->numBlocks()); } +TYPED_TEST_P(BlockStoreTest, NumBlocksIsCorrectAfterRemovingABlock_DeleteByKey) { + auto blockStore = this->fixture.createBlockStore(); + auto key = blockStore->create(cpputils::Data(1))->key(); + blockStore->create(cpputils::Data(1)); + blockStore->remove(key); + EXPECT_EQ(1u, blockStore->numBlocks()); +} + TYPED_TEST_P(BlockStoreTest, CanRemoveModifiedBlock) { auto blockStore = this->fixture.createBlockStore(); auto block = blockStore->create(cpputils::Data(5)); @@ -281,16 +303,19 @@ REGISTER_TYPED_TEST_CASE_P(BlockStoreTest, AfterLoad_FlushesWhenDestructed, LoadNonExistingBlock, TwoCreatedBlocksHaveDifferentKeys, - BlockIsNotLoadableAfterDeleting, + BlockIsNotLoadableAfterDeleting_DeleteByBlock, + BlockIsNotLoadableAfterDeleting_DeleteByKey, NumBlocksIsCorrectOnEmptyBlockstore, NumBlocksIsCorrectAfterAddingOneBlock, NumBlocksIsCorrectAfterAddingOneBlock_AfterClosingBlock, - NumBlocksIsCorrectAfterRemovingTheLastBlock, + NumBlocksIsCorrectAfterRemovingTheLastBlock_DeleteByBlock, + NumBlocksIsCorrectAfterRemovingTheLastBlock_DeleteByKey, NumBlocksIsCorrectAfterAddingTwoBlocks, NumBlocksIsCorrectAfterAddingTwoBlocks_AfterClosingFirstBlock, NumBlocksIsCorrectAfterAddingTwoBlocks_AfterClosingSecondBlock, NumBlocksIsCorrectAfterAddingTwoBlocks_AfterClosingBothBlocks, - NumBlocksIsCorrectAfterRemovingABlock, + NumBlocksIsCorrectAfterRemovingABlock_DeleteByBlock, + NumBlocksIsCorrectAfterRemovingABlock_DeleteByKey, WriteAndReadImmediately, WriteAndReadAfterLoading, OverwriteAndRead,