Support concurrent get/iterate/createIterator in dummy persistence

3 files changed, 51 insertions, 30 deletions

diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp
index d3883744229..25c6b71f7ff 100644
--- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp
+++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp
@@ -528,7 +528,7 @@ DummyPersistence::get(const Bucket& b,
         b.toString().c_str(),
         did.toString().c_str());
     assert(b.getBucketSpace() == FixedBucketSpaces::default_space());
-    BucketContentGuard::UP bc(acquireBucketWithLock(b));
+    BucketContentGuard::UP bc(acquireBucketWithLock(b, LockMode::Shared));
     if (!bc.get()) {
     } else {
         DocEntry::SP entry((*bc)->getEntry(did));
@@ -568,7 +568,7 @@ DummyPersistence::createIterator(
                     "Got invalid/unparseable document selection string");
         }
     }
-    BucketContentGuard::UP bc(acquireBucketWithLock(b));
+    BucketContentGuard::UP bc(acquireBucketWithLock(b, LockMode::Shared));
     if (!bc.get()) {
         return CreateIteratorResult(Result::TRANSIENT_ERROR, "Bucket not found");
     }
@@ -656,7 +656,7 @@ DummyPersistence::iterate(IteratorId id, uint64_t maxByteSize, Context& ctx) con
         it = iter->second.get();
     }
 
-    BucketContentGuard::UP bc(acquireBucketWithLock(it->_bucket));
+    BucketContentGuard::UP bc(acquireBucketWithLock(it->_bucket, LockMode::Shared));
     if (!bc.get()) {
         ctx.trace(9, "finished iterate(); bucket not found");
         return IterateResult(Result::TRANSIENT_ERROR, "Bucket not found");
@@ -942,11 +942,11 @@ DummyPersistence::isActive(const Bucket& b) const
 
 BucketContentGuard::~BucketContentGuard()
 {
-    _persistence.releaseBucketNoLock(_content);
+    _persistence.releaseBucketNoLock(_content, _lock_mode);
 }
 
 BucketContentGuard::UP
-DummyPersistence::acquireBucketWithLock(const Bucket& b) const
+DummyPersistence::acquireBucketWithLock(const Bucket& b, LockMode lock_mode) const
 {
     assert(b.getBucketSpace() == FixedBucketSpaces::default_space());
     vespalib::MonitorGuard lock(_monitor);
@@ -955,28 +955,32 @@ DummyPersistence::acquireBucketWithLock(const Bucket& b) const
     if (it == ncp._content[b.getPartition()].end()) {
         return BucketContentGuard::UP();
     }
-    // Sanity check that SPI-level locking is doing its job correctly.
-    // Atomic CAS might be a bit overkill, but since we "release" the bucket
-    // outside of the mutex, we want to ensure the write is visible across all
-    // threads.
-    bool my_false(false);
-    bool bucketNotInUse(it->second->_inUse.compare_exchange_strong(my_false, true));
-    if (!bucketNotInUse) {
-        LOG(error, "Attempted to acquire %s, but it was already marked as being in use!",
-            b.toString().c_str());
-        LOG_ABORT("should not reach here");
+    if (lock_mode == LockMode::Exclusive) {
+        // Sanity check that SPI-level locking is doing its job correctly.
+        // Atomic CAS might be a bit overkill, but since we "release" the bucket
+        // outside of the mutex, we want to ensure the write is visible across all
+        // threads.
+        bool my_false(false);
+        bool bucketNotInUse(it->second->_inUse.compare_exchange_strong(my_false, true));
+        if (!bucketNotInUse) {
+            LOG(error, "Attempted to acquire %s, but it was already marked as being in use!",
+                b.toString().c_str());
+            LOG_ABORT("dummy persistence bucket locking invariant violation");
+        }
     }
 
-    return BucketContentGuard::UP(new BucketContentGuard(ncp, *it->second));
+    return std::make_unique<BucketContentGuard>(ncp, *it->second, lock_mode);
 }
 
 void
-DummyPersistence::releaseBucketNoLock(const BucketContent& bc) const
+DummyPersistence::releaseBucketNoLock(const BucketContent& bc, LockMode lock_mode) const noexcept
 {
-    bool my_true(true);
-    bool bucketInUse(bc._inUse.compare_exchange_strong(my_true, false));
-    assert(bucketInUse);
-    (void) bucketInUse;
+    if (lock_mode == LockMode::Exclusive) {
+        bool my_true(true);
+        bool bucketInUse(bc._inUse.compare_exchange_strong(my_true, false));
+        assert(bucketInUse);
+        (void) bucketInUse;
+    }
 }
 
 }
diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h
index c93b7fd22c7..c97aab822ac 100644
--- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h
+++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h
@@ -24,13 +24,18 @@ class DocumentTypeRepo;
 
 namespace storage::spi::dummy {
 
+enum class LockMode {
+    Exclusive,
+    Shared
+};
+
 struct BucketEntry
 {
     DocEntry::SP entry;
     GlobalId gid;
 
     BucketEntry(DocEntry::SP e, const GlobalId& g)
-        : entry(e),
+        : entry(std::move(e)),
           gid(g)
     { }
 };
@@ -98,30 +103,33 @@ class BucketContentGuard
     BucketContentGuard(const BucketContentGuard&);
     BucketContentGuard& operator=(const BucketContentGuard&);
 public:
-    typedef std::unique_ptr<BucketContentGuard> UP;
+    using UP = std::unique_ptr<BucketContentGuard>;
 
     BucketContentGuard(DummyPersistence& persistence,
-                       BucketContent& content)
+                       BucketContent& content,
+                       LockMode lock_mode)
         : _persistence(persistence),
-          _content(content)
+          _content(content),
+          _lock_mode(lock_mode)
     {
     }
     ~BucketContentGuard();
 
-    BucketContent& getContent() {
+    BucketContent& getContent() noexcept {
         return _content;
     }
 
-    BucketContent* operator->() {
+    BucketContent* operator->() noexcept {
         return &_content;
     }
 
-    BucketContent& operator*() {
+    BucketContent& operator*() noexcept {
         return _content;
     }
 private:
     DummyPersistence& _persistence;
     BucketContent& _content;
+    LockMode _lock_mode;
 };
 
 class DummyPersistence : public AbstractPersistenceProvider
@@ -207,8 +215,8 @@ public:
 private:
     friend class BucketContentGuard;
     // Const since funcs only alter mutable field in BucketContent
-    BucketContentGuard::UP acquireBucketWithLock(const Bucket& b) const;
-    void releaseBucketNoLock(const BucketContent& bc) const;
+    BucketContentGuard::UP acquireBucketWithLock(const Bucket& b, LockMode lock_mode = LockMode::Exclusive) const;
+    void releaseBucketNoLock(const BucketContent& bc, LockMode lock_mode = LockMode::Exclusive) const noexcept;
 
     mutable bool _initialized;
     std::shared_ptr<const document::DocumentTypeRepo> _repo;
diff --git a/persistence/src/vespa/persistence/spi/persistenceprovider.h b/persistence/src/vespa/persistence/spi/persistenceprovider.h
index b5f2fc198c4..96b3d385b87 100644
--- a/persistence/src/vespa/persistence/spi/persistenceprovider.h
+++ b/persistence/src/vespa/persistence/spi/persistenceprovider.h
@@ -232,6 +232,9 @@ struct PersistenceProvider
      * document id. If no versions were found, or the document was removed,
      * the result should be successful, but contain no document (see GetResult).
      *
+     * Concurrency note: may be called concurrently with other read-only
+     * operations.
+     *
      * @param fieldSet A set of fields that should be retrieved.
      * @param id The document id to retrieve.
      */
@@ -253,6 +256,9 @@ struct PersistenceProvider
      * iteration progress and selection criteria. destroyIterator will NOT
      * be called when createIterator returns an error.
      *
+     * Concurrency note: may be called concurrently with other read-only
+     * operations.
+     *
      * @param selection Selection criteria used to limit the subset of
      *   the bucket's documents that will be returned by the iterator. The
      *   provider implementation may use these criteria to optimize its
@@ -323,6 +329,9 @@ struct PersistenceProvider
      * iterator must only set this flag on the result and return without any
      * documents.
      *
+     * Concurrency note: may be called concurrently with other read-only
+     * operations.
+     *
      * @param id An iterator ID returned by a previous call to createIterator
      * @param maxByteSize An indication of the maximum number of bytes that
      * should be returned.