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.

src/blobstore/implementations/onblocks/BlobStoreOnBlocks.cpp

@@ -52,6 +52,10 @@ void BlobStoreOnBlocks::remove(unique_ref<Blob> blob) {
     _dataTreeStore->remove((*_blob)->releaseTree());
 }
 
+void BlobStoreOnBlocks::remove(const Key &key) {
+    _dataTreeStore->remove(key);
+}
+
 uint64_t BlobStoreOnBlocks::virtualBlocksizeBytes() const {
     return _dataTreeStore->virtualBlocksizeBytes();
 }

src/blobstore/implementations/onblocks/BlobStoreOnBlocks.h

@@ -23,6 +23,7 @@ public:
   boost::optional<cpputils::unique_ref<Blob>> load(const blockstore::Key &key) override;
 
   void remove(cpputils::unique_ref<Blob> 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, ...)

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<DataNode> DataNodeStore::overwriteNodeWith(unique_ref<DataNode> targe
 }
 
 void DataNodeStore::remove(unique_ref<DataNode> 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<DataNode> 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<DataInnerNode*>(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<DataInnerNode>(*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<DataInnerNode> 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<DataInnerNode>(*child);
+      ASSERT(inner != none, "Expected inner node as child");
+      _removeSubtree(std::move(*inner));
     }
   }
   remove(std::move(node));

src/blobstore/implementations/onblocks/datanodestore/DataNodeStore.h

@@ -38,7 +38,8 @@ public:
   cpputils::unique_ref<DataNode> overwriteNodeWith(cpputils::unique_ref<DataNode> target, const DataNode &source);
 
   void remove(cpputils::unique_ref<DataNode> node);
-  void removeSubtree(cpputils::unique_ref<DataNode> 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<DataNode> load(cpputils::unique_ref<blockstore::Block> block);
+  void _removeSubtree(cpputils::unique_ref<DataInnerNode> node);
 
   cpputils::unique_ref<blockstore::BlockStore> _blockstore;
   const DataNodeLayout _layout;

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();
       }
   };

src/blobstore/implementations/onblocks/datatreestore/DataTreeStore.cpp

@@ -35,10 +35,13 @@ unique_ref<DataTree> DataTreeStore::createNewTree() {
 }
 
 void DataTreeStore::remove(unique_ref<DataTree> 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);
 }
 
 }

src/blobstore/implementations/onblocks/datatreestore/DataTreeStore.h

@@ -24,6 +24,7 @@ public:
   cpputils::unique_ref<DataTree> createNewTree();
 
   void remove(cpputils::unique_ref<DataTree> tree);
+  void remove(const blockstore::Key &key);
 
   //TODO Test blocksizeBytes/numBlocks/estimateSpaceForNumBlocksLeft
   uint64_t virtualBlocksizeBytes() const;

src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStore.cpp

@@ -41,6 +41,9 @@ void ParallelAccessDataTreeStore::remove(unique_ref<DataTreeRef> tree) {
   return _parallelAccessStore.remove(key, std::move(tree));
 }
 
+void ParallelAccessDataTreeStore::remove(const Key &key) {
+  return _parallelAccessStore.remove(key);
+}
 
 
 }

src/blobstore/implementations/onblocks/parallelaccessdatatreestore/ParallelAccessDataTreeStore.h

@@ -25,6 +25,7 @@ public:
   cpputils::unique_ref<DataTreeRef> createNewTree();
 
   void remove(cpputils::unique_ref<DataTreeRef> tree);
+  void remove(const blockstore::Key &key);
 
   //TODO Test blocksizeBytes/numBlocks/estimateSpaceForNumBlocksLeft
   uint64_t virtualBlocksizeBytes() const;

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;
 

src/blobstore/interface/BlobStore.h

@@ -19,6 +19,7 @@ public:
   virtual cpputils::unique_ref<Blob> create() = 0;
   virtual boost::optional<cpputils::unique_ref<Blob>> load(const blockstore::Key &key) = 0;
   virtual void remove(cpputils::unique_ref<Blob> blob) = 0;
+  virtual void remove(const blockstore::Key &key) = 0;
 
   virtual uint64_t numBlocks() const = 0;
   virtual uint64_t estimateSpaceForNumBlocksLeft() const = 0;

src/blockstore/implementations/caching/CachingBlockStore.cpp

@@ -61,6 +61,15 @@ void CachingBlockStore::remove(cpputils::unique_ref<Block> block) {
   }
 }
 
+void CachingBlockStore::remove(const Key &key) {
+  auto fromCache = _cache.pop(key);
+  if (fromCache != none) {
+    remove(make_unique_ref<CachedBlock>(std::move(*fromCache), this));
+  } else {
+    _baseBlockStore->remove(key);
+  }
+}
+
 uint64_t CachingBlockStore::numBlocks() const {
   return _baseBlockStore->numBlocks() + _newBlocks.size();
 }

src/blockstore/implementations/caching/CachingBlockStore.h

@@ -19,6 +19,7 @@ public:
   Key createKey() override;
   boost::optional<cpputils::unique_ref<Block>> tryCreate(const Key &key, cpputils::Data data) override;
   boost::optional<cpputils::unique_ref<Block>> load(const Key &key) override;
+  void remove(const Key &key) override;
   void remove(cpputils::unique_ref<Block> block) override;
   uint64_t numBlocks() const override;
   uint64_t estimateNumFreeBytes() const override;

src/blockstore/implementations/compressing/CompressingBlockStore.h

@@ -17,7 +17,7 @@ public:
     Key createKey() override;
     boost::optional<cpputils::unique_ref<Block>> tryCreate(const Key &key, cpputils::Data data) override;
     boost::optional<cpputils::unique_ref<Block>> load(const Key &key) override;
-    void remove(cpputils::unique_ref<Block> 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<cpputils::unique_ref<Block>> CompressingBlockStore<Compressor>::
 }
 
 template<class Compressor>
-void CompressingBlockStore<Compressor>::remove(cpputils::unique_ref<Block> block) {
-    auto _block = cpputils::dynamic_pointer_move<CompressedBlock<Compressor>>(block);
-    ASSERT(_block != boost::none, "Wrong block type");
-    auto baseBlock = (*_block)->releaseBaseBlock();
-    return _baseBlockStore->remove(std::move(baseBlock));
+void CompressingBlockStore<Compressor>::remove(const Key &key) {
+    return _baseBlockStore->remove(key);
 }
 
 template<class Compressor>

src/blockstore/implementations/encrypted/EncryptedBlockStore.h

@@ -20,7 +20,7 @@ public:
   Key createKey() override;
   boost::optional<cpputils::unique_ref<Block>> tryCreate(const Key &key, cpputils::Data data) override;
   boost::optional<cpputils::unique_ref<Block>> load(const Key &key) override;
-  void remove(cpputils::unique_ref<Block> 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<cpputils::unique_ref<Block>> EncryptedBlockStore<Cipher>::load(c
 }
 
 template<class Cipher>
-void EncryptedBlockStore<Cipher>::remove(cpputils::unique_ref<Block> block) {
-  auto encryptedBlock = cpputils::dynamic_pointer_move<EncryptedBlock<Cipher>>(block);
-  ASSERT(encryptedBlock != boost::none, "Block is not an EncryptedBlock");
-  auto baseBlock = (*encryptedBlock)->releaseBlock();
-  return _baseBlockStore->remove(std::move(baseBlock));
+void EncryptedBlockStore<Cipher>::remove(const Key &key) {
+  return _baseBlockStore->remove(key);
 }
 
 template<class Cipher>

src/blockstore/implementations/inmemory/InMemoryBlockStore.cpp

@@ -42,9 +42,7 @@ optional<unique_ref<Block>> InMemoryBlockStore::load(const Key &key) {
   }
 }
 
-void InMemoryBlockStore::remove(unique_ref<Block> 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");
 }

src/blockstore/implementations/inmemory/InMemoryBlockStore.h

@@ -19,7 +19,7 @@ public:
 
   boost::optional<cpputils::unique_ref<Block>> tryCreate(const Key &key, cpputils::Data data) override;
   boost::optional<cpputils::unique_ref<Block>> load(const Key &key) override;
-  void remove(cpputils::unique_ref<Block> 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;

src/blockstore/implementations/ondisk/OnDiskBlockStore.cpp

@@ -69,9 +69,7 @@ optional<unique_ref<Block>> OnDiskBlockStore::load(const Key &key) {
   return optional<unique_ref<Block>>(OnDiskBlock::LoadFromDisk(_rootdir, key));
 }
 
-void OnDiskBlockStore::remove(unique_ref<Block> block) {
-  Key key = block->key();
-  cpputils::destruct(std::move(block));
+void OnDiskBlockStore::remove(const Key &key) {
   OnDiskBlock::RemoveFromDisk(_rootdir, key);
 }
 

src/blockstore/implementations/ondisk/OnDiskBlockStore.h

@@ -17,7 +17,7 @@ public:
   boost::optional<cpputils::unique_ref<Block>> tryCreate(const Key &key, cpputils::Data data) override;
   boost::optional<cpputils::unique_ref<Block>> 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> 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;

src/blockstore/implementations/parallelaccess/ParallelAccessBlockStore.cpp

@@ -52,6 +52,10 @@ void ParallelAccessBlockStore::remove(unique_ref<Block> 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();
 }

src/blockstore/implementations/parallelaccess/ParallelAccessBlockStore.h

@@ -18,7 +18,8 @@ public:
   Key createKey() override;
   boost::optional<cpputils::unique_ref<Block>> tryCreate(const Key &key, cpputils::Data data) override;
   boost::optional<cpputils::unique_ref<Block>> load(const Key &key) override;
-  void remove(cpputils::unique_ref<Block> block) override;
+  void remove(const Key &key) override;
+  void remove(cpputils::unique_ref<Block> node) override;
   uint64_t numBlocks() const override;
   uint64_t estimateNumFreeBytes() const override;
   uint64_t blockSizeFromPhysicalBlockSize(uint64_t blockSize) const override;

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;
 

src/blockstore/implementations/testfake/FakeBlockStore.cpp

@@ -45,9 +45,7 @@ optional<unique_ref<Block>> FakeBlockStore::_load(const Key &key) {
   }
 }
 
-void FakeBlockStore::remove(unique_ref<Block> block) {
-  Key key = block->key();
-  cpputils::destruct(std::move(block));
+void FakeBlockStore::remove(const Key &key) {
   std::unique_lock<std::mutex> lock(_mutex);
   int numRemoved = _blocks.erase(key);
   ASSERT(numRemoved == 1, "Block not found");

src/blockstore/implementations/testfake/FakeBlockStore.h

@@ -33,7 +33,7 @@ public:
 
   boost::optional<cpputils::unique_ref<Block>> tryCreate(const Key &key, cpputils::Data data) override;
   boost::optional<cpputils::unique_ref<Block>> load(const Key &key) override;
-  void remove(cpputils::unique_ref<Block> 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;

src/blockstore/implementations/versioncounting/VersionCountingBlockStore.cpp

@@ -58,13 +58,9 @@ namespace blockstore {
             throw IntegrityViolationError(reason);
         }
 
-        void VersionCountingBlockStore::remove(unique_ref<Block> block) {
-            Key key = block->key();
-            auto versionCountingBlock = cpputils::dynamic_pointer_move<VersionCountingBlock>(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 {

src/blockstore/implementations/versioncounting/VersionCountingBlockStore.h

@@ -18,7 +18,7 @@ public:
   Key createKey() override;
   boost::optional<cpputils::unique_ref<Block>> tryCreate(const Key &key, cpputils::Data data) override;
   boost::optional<cpputils::unique_ref<Block>> load(const Key &key) override;
-  void remove(cpputils::unique_ref<Block> 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;

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<cpputils::unique_ref<Block>> load(const Key &key) = 0;
-  virtual void remove(cpputils::unique_ref<Block> 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<void (const Key &)> callback) const = 0;
 
+  virtual void remove(cpputils::unique_ref<Block> block) {
+    Key key = block->key();
+    cpputils::destruct(std::move(block));
+    remove(key);
+  }
+
   cpputils::unique_ref<Block> create(const cpputils::Data &data) {
     while(true) {
       //TODO Copy (data.copy()) necessary?

src/cryfs/filesystem/cachingfsblobstore/CachingFsBlobStore.h

@@ -24,6 +24,7 @@ namespace cryfs {
             cpputils::unique_ref<SymlinkBlobRef> createSymlinkBlob(const boost::filesystem::path &target, const blockstore::Key &parent);
             boost::optional<cpputils::unique_ref<FsBlobRef>> load(const blockstore::Key &key);
             void remove(cpputils::unique_ref<FsBlobRef> 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<fsblobstore::FsBlob> baseBlob) {
             blockstore::Key key = baseBlob->key();
             _cache.push(key, std::move(baseBlob));

src/cryfs/filesystem/fsblobstore/FsBlobStore.h

@@ -22,6 +22,7 @@ namespace cryfs {
             cpputils::unique_ref<SymlinkBlob> createSymlinkBlob(const boost::filesystem::path &target, const blockstore::Key &parent);
             boost::optional<cpputils::unique_ref<FsBlob>> load(const blockstore::Key &key);
             void remove(cpputils::unique_ref<FsBlob> 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<off_t (const blockstore::Key &)> FsBlobStore::_getLstatSize() {
             return [this] (const blockstore::Key &key) {
                 auto blob = load(key);

src/cryfs/filesystem/parallelaccessfsblobstore/ParallelAccessFsBlobStoreAdapter.h

@@ -11,20 +11,24 @@ namespace parallelaccessfsblobstore {
 
 class ParallelAccessFsBlobStoreAdapter final: public parallelaccessstore::ParallelAccessBaseStore<cachingfsblobstore::FsBlobRef, blockstore::Key> {
 public:
-  explicit ParallelAccessFsBlobStoreAdapter(cachingfsblobstore::CachingFsBlobStore *baseBlockStore)
-    :_baseBlockStore(std::move(baseBlockStore)) {
+  explicit ParallelAccessFsBlobStoreAdapter(cachingfsblobstore::CachingFsBlobStore *baseBlobStore)
+    :_baseBlobStore(std::move(baseBlobStore)) {
   }
 
   boost::optional<cpputils::unique_ref<cachingfsblobstore::FsBlobRef>> loadFromBaseStore(const blockstore::Key &key) override {
-	return _baseBlockStore->load(key);
+	return _baseBlobStore->load(key);
   }
 
   void removeFromBaseStore(cpputils::unique_ref<cachingfsblobstore::FsBlobRef> 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);
 };

src/parallelaccessstore/ParallelAccessBaseStore.h

@@ -4,6 +4,7 @@
 
 #include <cpp-utils/pointer/unique_ref.h>
 #include <boost/optional.hpp>
+#include <blockstore/utils/Key.h>
 
 namespace parallelaccessstore {
 
@@ -13,6 +14,7 @@ public:
   virtual ~ParallelAccessBaseStore() {}
   virtual boost::optional<cpputils::unique_ref<Resource>> loadFromBaseStore(const Key &key) = 0;
   virtual void removeFromBaseStore(cpputils::unique_ref<Resource> block) = 0;
+  virtual void removeFromBaseStore(const blockstore::Key &key) = 0;
 };
 
 }

src/parallelaccessstore/ParallelAccessStore.h

@@ -54,6 +54,7 @@ public:
   boost::optional<cpputils::unique_ref<ResourceRef>> load(const Key &key);
   boost::optional<cpputils::unique_ref<ResourceRef>> load(const Key &key, std::function<cpputils::unique_ref<ResourceRef>(Resource*)> createResourceRef);
   void remove(const Key &key, cpputils::unique_ref<ResourceRef> block);
+  void remove(const Key &key);
 
 private:
   class OpenResource final {
@@ -93,6 +94,9 @@ private:
   template<class ActualResourceRef>
   cpputils::unique_ref<ActualResourceRef> _add(const Key &key, cpputils::unique_ref<Resource> resource, std::function<cpputils::unique_ref<ActualResourceRef>(Resource*)> createResourceRef);
 
+  std::future<cpputils::unique_ref<Resource>> _resourceToRemoveFuture(const Key &key);
+  cpputils::unique_ref<Resource> _waitForResourceToRemove(const Key &key, std::future<cpputils::unique_ref<Resource>> resourceToRemoveFuture);
+
   void release(const Key &key);
   friend class CachedResource;
 
@@ -167,22 +171,45 @@ boost::optional<cpputils::unique_ref<ResourceRef>> ParallelAccessStore<Resource,
 
 template<class Resource, class ResourceRef, class Key>
 void ParallelAccessStore<Resource, ResourceRef, Key>::remove(const Key &key, cpputils::unique_ref<ResourceRef> resource) {
-  std::future<cpputils::unique_ref<Resource>> 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<class Resource, class ResourceRef, class Key>
+std::future<cpputils::unique_ref<Resource>> ParallelAccessStore<Resource, ResourceRef, Key>::_resourceToRemoveFuture(const Key &key) {
     std::lock_guard <std::mutex> 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<std::mutex> 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<class Resource, class ResourceRef, class Key>
+cpputils::unique_ref<Resource> ParallelAccessStore<Resource, ResourceRef, Key>::_waitForResourceToRemove(const Key &key, std::future<cpputils::unique_ref<Resource>> resourceToRemoveFuture) {
+    auto resourceToRemove = resourceToRemoveFuture.get();
 
-  _baseStore->removeFromBaseStore(std::move(resourceToRemove));
-}
+    std::lock_guard<std::mutex> 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<class Resource, class ResourceRef, class Key>
+void ParallelAccessStore<Resource, ResourceRef, Key>::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<class Resource, class ResourceRef, class Key>
 void ParallelAccessStore<Resource, ResourceRef, Key>::release(const Key &key) {

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();

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));
+}

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<VersionCountingBlockStore> 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<VersionCountingBlockStore> 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<VersionCountingBlockStore> 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<VersionCountingBlockStore> 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

test/blockstore/interface/helpers/BlockStoreWithRandomKeysTest.cpp

@@ -29,6 +29,7 @@ public:
   }
   MOCK_METHOD1(do_load, Block*(const Key &));
   void remove(unique_ref<Block> 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));

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,