MB-24246: Combine 'writeLock' & 'highSeqnosLock' into one in BasicLinkedList 58/77958/7
authorManu Dhundi <manu@couchbase.com>
Fri, 12 May 2017 16:25:52 +0000 (09:25 -0700)
committerManu Dhundi <manu@couchbase.com>
Fri, 12 May 2017 17:49:45 +0000 (17:49 +0000)
Functionally 'writeLock' and 'highSeqnosLock' both result in the
serialization of the BasicLinkedList write and other list operations.
Hence this commit combines the 2 locks into one lock.

As a consequence this commit ensures that the writeLock is held on
the BasicLinkedList until the sequence number for the newly added,
updated and soft-deleted is generated. While this is strictly not
needed in new add, it is necessary in update and soft delete as
explained below.

(a) New Add :
   A1, B2, C3 and we are trying to add D4
   A1, B2, C3, D? added but no seqno yet.
   Now we don't really care if range read starts here as it can read
   only A1, B2, C3 in the snapshot.

   But to maintain uniformity (hence simpler code) with update
   and soft-delete we grab writeLock such that we append to list
   + update highSeqno as an atomic operation.

(b) Update (and Soft-delete):
   A1, B2, C3 and we are trying to update B to B4
   A1, B2, C3, B? added but no seqno yet and/or no B2_stale yet
   Now we cannot start the range read here because we do not wait
   for range read to finish to mark the item stale (note that we
   are planning to not send stale(duplicate) items in a snapshot).

   This was already in case (b) using highSeqnosLock, this commit
   uses just writeLock for the same.

The commit also adds documentation regarding the desired heirarchy of
the lock acquisition in BasicLinkedList.

Change-Id: I10a80f55a763d1496adec24fa12bc75d83ea1573
Reviewed-on: http://review.couchbase.org/77958
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
src/ephemeral_tombstone_purger.cc
src/ephemeral_vb.cc
src/linked_list.cc
src/linked_list.h
src/seqlist.h
tests/mock/mock_basic_ll.h
tests/module_tests/basic_ll_test.cc
tests/module_tests/ephemeral_vb_test.cc

index 9e4a1f6..034353e 100644 (file)
@@ -46,7 +46,11 @@ void EphemeralVBucket::HTTombstonePurger::visit(
     // This item should be purged. Remove from the HashTable and move over to
     // being owned by the sequence list.
     auto ownedSV = vbucket.ht.unlocked_release(hbl, v->getKey());
-    vbucket.seqList->markItemStale(std::move(ownedSV));
+    {
+        std::lock_guard<std::mutex> listWriteLg(
+                vbucket.seqList->getListWriteLock());
+        vbucket.seqList->markItemStale(listWriteLg, std::move(ownedSV));
+    }
     ++numPurgedItems;
 }
 
index 68e573c..a69ff4d 100644 (file)
@@ -310,11 +310,12 @@ EphemeralVBucket::updateStoredValue(const HashTable::HashBucketLock& hbl,
         // highSeqno and highestDedupedSeqno are both incorrect. We have to hold
         // this lock to prevent a new rangeRead starting, and covering an
         // inconsistent range.
-        std::lock_guard<std::mutex> highSeqnoLh(seqList->getHighSeqnosLock());
+        std::lock_guard<std::mutex> listWriteLg(seqList->getListWriteLock());
 
         /* Update the OrderedStoredValue in hash table + Ordered data structure
            (list) */
-        res = seqList->updateListElem(lh, *(v.toOrderedStoredValue()));
+        res = seqList->updateListElem(
+                lh, listWriteLg, *(v.toOrderedStoredValue()));
 
         switch (res) {
         case SequenceList::UpdateStatus::Success:
@@ -341,7 +342,8 @@ EphemeralVBucket::updateStoredValue(const HashTable::HashBucketLock& hbl,
             /* Add a new storedvalue for the item */
             newSv = ht.unlocked_addNewStoredValue(hbl, itm);
 
-            seqList->appendToList(lh, *(newSv->toOrderedStoredValue()));
+            seqList->appendToList(
+                    lh, listWriteLg, *(newSv->toOrderedStoredValue()));
         } break;
         }
 
@@ -352,8 +354,19 @@ EphemeralVBucket::updateStoredValue(const HashTable::HashBucketLock& hbl,
 
         /* Update the high seqno in the sequential storage */
         auto& osv = *(newSv->toOrderedStoredValue());
-        seqList->updateHighSeqno(highSeqnoLh, osv);
-        seqList->updateHighestDedupedSeqno(highSeqnoLh, osv);
+        seqList->updateHighSeqno(listWriteLg, osv);
+        seqList->updateHighestDedupedSeqno(listWriteLg, osv);
+
+        if (res == SequenceList::UpdateStatus::Append) {
+            /* Mark the un-updated storedValue as stale. This must be done after
+             the new storedvalue for the item is visible for range read in the
+             list. This is because we do not want the seqlist to delete the
+             stale
+             item before its latest copy is added to the list.
+             (item becomes visible for range read only after updating the list
+             with the seqno of the item) */
+            seqList->markItemStale(listWriteLg, std::move(ownedSv));
+        }
     }
 
     if (recreatingDeletedItem) {
@@ -364,15 +377,6 @@ EphemeralVBucket::updateStoredValue(const HashTable::HashBucketLock& hbl,
 
     seqList->updateNumDeletedItems(oldValueDeleted, itm.isDeleted());
 
-    if (res == SequenceList::UpdateStatus::Append) {
-        /* Mark the un-updated storedValue as stale. This must be done after
-           the new storedvalue for the item is visible for range read in the
-           list. This is because we do not want the seqlist to delete the stale
-           item before its latest copy is added to the list.
-           (item becomes visible for range read only after updating the list
-            with the seqno of the item) */
-        seqList->markItemStale(std::move(ownedSv));
-    }
     return std::make_tuple(newSv, status, notifyCtx);
 }
 
@@ -384,9 +388,9 @@ std::pair<StoredValue*, VBNotifyCtx> EphemeralVBucket::addNewStoredValue(
 
     std::lock_guard<std::mutex> lh(sequenceLock);
 
-    /* Add to the sequential storage */
+    OrderedStoredValue* osv;
     try {
-        seqList->appendToList(lh, *(v->toOrderedStoredValue()));
+        osv = v->toOrderedStoredValue();
     } catch (const std::bad_cast& e) {
         throw std::logic_error(
                 "EphemeralVBucket::addNewStoredValue(): Error " +
@@ -397,14 +401,20 @@ std::pair<StoredValue*, VBNotifyCtx> EphemeralVBucket::addNewStoredValue(
     }
 
     VBNotifyCtx notifyCtx;
-    if (queueItmCtx) {
-        /* Put on checkpoint mgr */
-        notifyCtx = queueDirty(*v, *queueItmCtx);
-    }
+    {
+        std::lock_guard<std::mutex> listWriteLg(seqList->getListWriteLock());
 
-    /* Update the high seqno in the sequential storage */
-    std::lock_guard<std::mutex> highSeqnoLh(seqList->getHighSeqnosLock());
-    seqList->updateHighSeqno(highSeqnoLh, *(v->toOrderedStoredValue()));
+        /* Add to the sequential storage */
+        seqList->appendToList(lh, listWriteLg, *osv);
+
+        if (queueItmCtx) {
+            /* Put on checkpoint mgr */
+            notifyCtx = queueDirty(*v, *queueItmCtx);
+        }
+
+        /* Update the high seqno in the sequential storage */
+        seqList->updateHighSeqno(listWriteLg, *osv);
+    }
     ++opsCreate;
 
     seqList->updateNumDeletedItems(false, itm.isDeleted());
@@ -432,11 +442,12 @@ std::tuple<StoredValue*, VBNotifyCtx> EphemeralVBucket::softDeleteStoredValue(
         // highSeqno and highestDedupedSeqno are both incorrect. We have to hold
         // this lock to prevent a new rangeRead starting, and covering an
         // inconsistent range.
-        std::lock_guard<std::mutex> highSeqnoLh(seqList->getHighSeqnosLock());
+        std::lock_guard<std::mutex> listWriteLg(seqList->getListWriteLock());
 
         /* Update the OrderedStoredValue in hash table + Ordered data structure
            (list) */
-        res = seqList->updateListElem(lh, *(v.toOrderedStoredValue()));
+        res = seqList->updateListElem(
+                lh, listWriteLg, *(v.toOrderedStoredValue()));
 
         switch (res) {
         case SequenceList::UpdateStatus::Success:
@@ -459,7 +470,8 @@ std::tuple<StoredValue*, VBNotifyCtx> EphemeralVBucket::softDeleteStoredValue(
                */
             std::tie(newSv, ownedSv) = ht.unlocked_replaceByCopy(hbl, v);
 
-            seqList->appendToList(lh, *(newSv->toOrderedStoredValue()));
+            seqList->appendToList(
+                    lh, listWriteLg, *(newSv->toOrderedStoredValue()));
         } break;
         }
 
@@ -474,23 +486,25 @@ std::tuple<StoredValue*, VBNotifyCtx> EphemeralVBucket::softDeleteStoredValue(
 
         /* Update the high seqno in the sequential storage */
         auto& osv = *(newSv->toOrderedStoredValue());
-        seqList->updateHighSeqno(highSeqnoLh, osv);
-        seqList->updateHighestDedupedSeqno(highSeqnoLh, osv);
+        seqList->updateHighSeqno(listWriteLg, osv);
+        seqList->updateHighestDedupedSeqno(listWriteLg, osv);
+
+        if (res == SequenceList::UpdateStatus::Append) {
+            /* Mark the un-updated storedValue as stale. This must be done after
+             the new storedvalue for the item is visible for range read in the
+             list. This is because we do not want the seqlist to delete the
+             stale
+             item before its latest copy is added to the list.
+             (item becomes visible for range read only after updating the list
+             with the seqno of the item) */
+            seqList->markItemStale(listWriteLg, std::move(ownedSv));
+        }
     }
 
     ++opsDelete;
 
     seqList->updateNumDeletedItems(oldValueDeleted, true);
 
-    if (res == SequenceList::UpdateStatus::Append) {
-        /* Mark the un-updated storedValue as stale. This must be done after
-           the new storedvalue for the item is visible for range read in the
-           list. This is because we do not want the seqlist to delete the stale
-           item before its latest copy is added to the list.
-           (item becomes visible for range read only after updating the list
-           with the seqno of the item) */
-        seqList->markItemStale(std::move(ownedSv));
-    }
     return std::make_tuple(newSv, notifyCtx);
 }
 
index 0e91a4f..f021915 100644 (file)
@@ -35,7 +35,7 @@ BasicLinkedList::BasicLinkedList(uint16_t vbucketId, EPStats& st)
 BasicLinkedList::~BasicLinkedList() {
     /* Delete stale items here, other items are deleted by the hash
        table */
-    std::lock_guard<std::mutex> writeGuard(writeLock);
+    std::lock_guard<std::mutex> writeGuard(getListWriteLock());
     seqList.remove_and_dispose_if(
             [&writeGuard](const OrderedStoredValue& v) {
                 return v.isStale(writeGuard);
@@ -51,18 +51,15 @@ BasicLinkedList::~BasicLinkedList() {
 }
 
 void BasicLinkedList::appendToList(std::lock_guard<std::mutex>& seqLock,
+                                   std::lock_guard<std::mutex>& writeLock,
                                    OrderedStoredValue& v) {
-    /* Allow only one write to the list at a time */
-    std::lock_guard<std::mutex> lckGd(writeLock);
-
     seqList.push_back(v);
 }
 
 SequenceList::UpdateStatus BasicLinkedList::updateListElem(
-        std::lock_guard<std::mutex>& seqLock, OrderedStoredValue& v) {
-    /* Allow only one write to the list at a time */
-    std::lock_guard<std::mutex> lckGd(writeLock);
-
+        std::lock_guard<std::mutex>& seqLock,
+        std::lock_guard<std::mutex>& writeLock,
+        OrderedStoredValue& v) {
     /* Lock that needed for consistent read of SeqRange 'readRange' */
     std::lock_guard<SpinLock> lh(rangeLock);
 
@@ -102,6 +99,7 @@ BasicLinkedList::rangeRead(seqno_t start, seqno_t end) {
     std::lock_guard<std::mutex> lckGd(rangeReadLock);
 
     {
+        std::lock_guard<std::mutex> listWriteLg(getListWriteLock());
         std::lock_guard<SpinLock> lh(rangeLock);
         if (start > highSeqno) {
             LOG(EXTENSION_LOG_WARNING,
@@ -169,18 +167,17 @@ BasicLinkedList::rangeRead(seqno_t start, seqno_t end) {
     return std::make_tuple(ENGINE_SUCCESS, std::move(items), end);
 }
 
-void BasicLinkedList::updateHighSeqno(
-        std::lock_guard<std::mutex>& highSeqnoLock,
-        const OrderedStoredValue& v) {
+void BasicLinkedList::updateHighSeqno(std::lock_guard<std::mutex>& listWriteLg,
+                                      const OrderedStoredValue& v) {
     highSeqno = v.getBySeqno();
 }
 void BasicLinkedList::updateHighestDedupedSeqno(
-        std::lock_guard<std::mutex>& highSeqnoLock,
-        const OrderedStoredValue& v) {
+        std::lock_guard<std::mutex>& listWriteLg, const OrderedStoredValue& v) {
     highestDedupedSeqno = v.getBySeqno();
 }
 
-void BasicLinkedList::markItemStale(StoredValue::UniquePtr ownedSv) {
+void BasicLinkedList::markItemStale(std::lock_guard<std::mutex>& listWriteLg,
+                                    StoredValue::UniquePtr ownedSv) {
     /* Release the StoredValue as BasicLinkedList does not want it to be of
        owned type */
     StoredValue* v = ownedSv.release();
@@ -191,10 +188,7 @@ void BasicLinkedList::markItemStale(StoredValue::UniquePtr ownedSv) {
     st.currentSize.fetch_add(v->metaDataSize());
 
     ++numStaleItems;
-    {
-        std::lock_guard<std::mutex> writeGuard(writeLock);
-        v->toOrderedStoredValue()->markStale(writeGuard);
-    }
+    v->toOrderedStoredValue()->markStale(listWriteLg);
 }
 
 size_t BasicLinkedList::purgeTombstones() {
@@ -232,7 +226,7 @@ size_t BasicLinkedList::purgeTombstones() {
     OrderedLL::iterator startIt;
     OrderedLL::iterator endIt;
     {
-        std::lock_guard<std::mutex> writeGuard(writeLock);
+        std::lock_guard<std::mutex> writeGuard(getListWriteLock());
         if (seqList.empty()) {
             // Nothing in sequence list - nothing to purge.
             return 0;
@@ -267,7 +261,7 @@ size_t BasicLinkedList::purgeTombstones() {
     bool stale;
     for (auto it = startIt; it != endIt;) {
         {
-            std::lock_guard<std::mutex> writeGuard(writeLock);
+            std::lock_guard<std::mutex> writeGuard(getListWriteLock());
             stale = it->isStale(writeGuard);
         }
         // Only stale items are purged.
@@ -282,7 +276,7 @@ size_t BasicLinkedList::purgeTombstones() {
     }
     // Handle the last element.
     {
-        std::lock_guard<std::mutex> writeGuard(writeLock);
+        std::lock_guard<std::mutex> writeGuard(getListWriteLock());
         stale = endIt->isStale(writeGuard);
     }
     if (stale) {
@@ -319,22 +313,22 @@ size_t BasicLinkedList::getStaleMetadataBytes() const {
 }
 
 uint64_t BasicLinkedList::getNumDeletedItems() const {
-    std::lock_guard<std::mutex> lckGd(writeLock);
+    std::lock_guard<std::mutex> lckGd(getListWriteLock());
     return numDeletedItems;
 }
 
 uint64_t BasicLinkedList::getNumItems() const {
-    std::lock_guard<std::mutex> lckGd(writeLock);
+    std::lock_guard<std::mutex> lckGd(getListWriteLock());
     return seqList.size();
 }
 
 uint64_t BasicLinkedList::getHighSeqno() const {
-    std::lock_guard<std::mutex> lckGd(highSeqnosLock);
+    std::lock_guard<std::mutex> lckGd(getListWriteLock());
     return highSeqno;
 }
 
 uint64_t BasicLinkedList::getHighestDedupedSeqno() const {
-    std::lock_guard<std::mutex> lckGd(highSeqnosLock);
+    std::lock_guard<std::mutex> lckGd(getListWriteLock());
     return highestDedupedSeqno;
 }
 
@@ -351,8 +345,8 @@ uint64_t BasicLinkedList::getRangeReadEnd() const {
     std::lock_guard<SpinLock> lh(rangeLock);
     return readRange.getEnd();
 }
-std::mutex& BasicLinkedList::getHighSeqnosLock() const {
-    return highSeqnosLock;
+std::mutex& BasicLinkedList::getListWriteLock() const {
+    return writeLock;
 }
 
 SequenceList::RangeIterator BasicLinkedList::makeRangeIterator() {
@@ -382,7 +376,7 @@ std::ostream& operator <<(std::ostream& os, const BasicLinkedList& ll) {
 OrderedLL::iterator BasicLinkedList::purgeListElem(OrderedLL::iterator it) {
     StoredValue::UniquePtr purged(&*it);
     {
-        std::lock_guard<std::mutex> lckGd(writeLock);
+        std::lock_guard<std::mutex> lckGd(getListWriteLock());
         it = seqList.erase(it);
     }
 
@@ -406,6 +400,7 @@ OrderedLL::iterator BasicLinkedList::purgeListElem(OrderedLL::iterator it) {
 
 BasicLinkedList::RangeIteratorLL::RangeIteratorLL(BasicLinkedList& ll)
     : list(ll), readLockHolder(list.rangeReadLock), itrRange(0, 0) {
+    std::lock_guard<std::mutex> listWriteLg(list.getListWriteLock());
     std::lock_guard<SpinLock> lh(list.rangeLock);
     if (list.highSeqno < 1) {
         /* No need of holding a lock for the snapshot as there are no items;
index b3e0ff4..8662a6c 100644 (file)
@@ -129,6 +129,21 @@ private:
  *      VBucket delete or rollback, we first remove the element from
  *      BasicLinkedList (invalidate next, prev links) and then delete from the
  *      hashtable.
+ *
+ * Ordering/Hierarchy of Locks:
+ * ===========================
+ * BasicLinkedList has 3 locks namely:
+ * (i) writeLock (ii) rangeLock (iii) rangeReadLock
+ * Description of each lock can be found below in the class declaration, here
+ * we describe in what order the locks should be grabbed
+ *
+ * rangeReadLock ==> writeLock ==> rangeLock is the valid lock hierarchy.
+ *
+ * Preferred/Expected Lock Duration:
+ * ================================
+ * 'writeLock' and 'rangeLock' are held for short durations, typically for
+ * single list element writes and reads.
+ * 'rangeReadLock' is held for longer duration on the list (for entire range).
  */
 class BasicLinkedList : public SequenceList {
 public:
@@ -137,22 +152,25 @@ public:
     ~BasicLinkedList();
 
     void appendToList(std::lock_guard<std::mutex>& seqLock,
+                      std::lock_guard<std::mutex>& writeLock,
                       OrderedStoredValue& v) override;
 
     SequenceList::UpdateStatus updateListElem(
             std::lock_guard<std::mutex>& seqLock,
+            std::lock_guard<std::mutex>& writeLock,
             OrderedStoredValue& v) override;
 
     std::tuple<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>, seqno_t>
     rangeRead(seqno_t start, seqno_t end) override;
 
-    void updateHighSeqno(std::lock_guard<std::mutex>& highSeqnoLock,
+    void updateHighSeqno(std::lock_guard<std::mutex>& listWriteLg,
                          const OrderedStoredValue& v) override;
 
-    void updateHighestDedupedSeqno(std::lock_guard<std::mutex>& highSeqnoLock,
+    void updateHighestDedupedSeqno(std::lock_guard<std::mutex>& listWriteLg,
                                    const OrderedStoredValue& v) override;
 
-    void markItemStale(StoredValue::UniquePtr ownedSv) override;
+    void markItemStale(std::lock_guard<std::mutex>& listWriteLg,
+                       StoredValue::UniquePtr ownedSv) override;
 
     size_t purgeTombstones() override;
 
@@ -178,7 +196,7 @@ public:
 
     uint64_t getRangeReadEnd() const override;
 
-    std::mutex& getHighSeqnosLock() const override;
+    std::mutex& getListWriteLock() const override;
 
     SequenceList::RangeIterator makeRangeIterator() override;
 
@@ -190,7 +208,8 @@ protected:
 
     /**
      * Lock that serializes writes (append, update, purgeTombstones) on
-     * 'seqList'
+     * 'seqList' + the updation of the corresponding highSeqno or the
+     * highestDedupedSeqno atomic
      */
     mutable std::mutex writeLock;
 
@@ -209,11 +228,6 @@ protected:
     mutable SpinLock rangeLock;
 
     /**
-     * Lock protecting the highSeqno and highestDedupedSeqno.
-     */
-    mutable std::mutex highSeqnosLock;
-
-    /**
      * Lock that serializes range reads on the 'seqList' - i.e. serializes
      * the addition / removal of range reads from the set in-flight.
      * We need to serialize range reads because, range reads set a list level
index db2b7f8..bf5cc1e 100644 (file)
@@ -189,10 +189,12 @@ public:
      * Add a new item at the end of the list.
      *
      * @param seqLock A sequence lock the calling module is expected to hold.
+     * @param writeLock Write lock of the sequenceList from getListWriteLock()
      * @param v Ref to orderedStoredValue which will placed into the linked list
      *          Its intrusive list links will be updated.
      */
     virtual void appendToList(std::lock_guard<std::mutex>& seqLock,
+                              std::lock_guard<std::mutex>& writeLock,
                               OrderedStoredValue& v) = 0;
 
     /**
@@ -201,6 +203,7 @@ public:
      * we do not allow the update and indicate the caller to do an append.
      *
      * @param seqLock A sequence lock the calling module is expected to hold.
+     * @param writeLock Write lock of the sequenceList from getListWriteLock()
      * @param v Ref to orderedStoredValue which will placed into the linked list
      *          Its intrusive list links will be updated.
      *
@@ -210,7 +213,9 @@ public:
      *                              handle the append.
      */
     virtual SequenceList::UpdateStatus updateListElem(
-            std::lock_guard<std::mutex>& seqLock, OrderedStoredValue& v) = 0;
+            std::lock_guard<std::mutex>& seqLock,
+            std::lock_guard<std::mutex>& writeLock,
+            OrderedStoredValue& v) = 0;
 
     /**
      * Provides point-in-time snapshots which can be used for incremental
@@ -237,24 +242,24 @@ public:
      * Updates the highSeqno in the list. Since seqno is generated and managed
      * outside the list, the module managing it must update this after the seqno
      * is generated for the item already put in the list.
-     * @param highSeqnoLock The lock protecting the high seqnos the caller is
-     * expected to hold
+     *
+     * @param listWriteLg Write lock of the sequenceList from getListWriteLock()
      * @param v Ref to orderedStoredValue
      */
-    virtual void updateHighSeqno(std::lock_guard<std::mutex>& highSeqnoLock,
+    virtual void updateHighSeqno(std::lock_guard<std::mutex>& listWriteLg,
                                  const OrderedStoredValue& v) = 0;
 
     /**
      * Updates the highestDedupedSeqno in the list. Since seqno is generated and
      * managed outside the list, the module managing it must update this after
      * the seqno is generated for the item already put in the list.
-     * @param highSeqnoLock The lock protecting the high seqnos the caller is
-     * expected to hold
+     *
+     * @param listWriteLg Write lock of the sequenceList from getListWriteLock()
      * @param v Ref to orderedStoredValue
      *
      */
     virtual void updateHighestDedupedSeqno(
-            std::lock_guard<std::mutex>& highSeqnoLock,
+            std::lock_guard<std::mutex>& listWriteLg,
             const OrderedStoredValue& v) = 0;
 
     /**
@@ -263,10 +268,12 @@ public:
      *       wants to own the OrderedStoredValue (as owned type vs non-owned
      *       type)
      *
+     * @param listWriteLg Write lock of the sequenceList from getListWriteLock()
      * @param ownedSv StoredValue whose ownership is passed to the sequential
      *                data structure.
      */
-    virtual void markItemStale(StoredValue::UniquePtr ownedSv) = 0;
+    virtual void markItemStale(std::lock_guard<std::mutex>& listWriteLg,
+                               StoredValue::UniquePtr ownedSv) = 0;
 
     /**
      * Remove from sequence list and delete all OSVs which are purgable.
@@ -344,10 +351,11 @@ public:
     virtual uint64_t getRangeReadEnd() const = 0;
 
     /**
-     * Returns the lock which must be held to modify the highSeqno or the
-     * highestDedupedSeqno
+     * Returns the lock which must be held to make append/update to the seqList
+     * + the updation of the corresponding highSeqno or the
+     * highestDedupedSeqno atomic
      */
-    virtual std::mutex& getHighSeqnosLock() const = 0;
+    virtual std::mutex& getListWriteLock() const = 0;
 
     /**
      * Returns a range iterator for the underlying SequenceList obj
index 8211e72..98db58f 100644 (file)
@@ -51,11 +51,6 @@ public:
         return rangeReadLock;
     }
 
-    /// Expose the writeLock for testing.
-    std::mutex& getWriteLock() {
-        return writeLock;
-    }
-
     /* Register fake read range for testing */
     void registerFakeReadRange(seqno_t start, seqno_t end) {
         std::lock_guard<SpinLock> lh(rangeLock);
index 3934a2b..75ee162 100644 (file)
@@ -86,10 +86,11 @@ protected:
 
             sv = ht.find(key, TrackReference::Yes, WantsDeleted::No)
                          ->toOrderedStoredValue();
-            basicLL->appendToList(lg, *sv);
-            std::lock_guard<std::mutex> highSeqnoLh(
-                    basicLL->getHighSeqnosLock());
-            basicLL->updateHighSeqno(highSeqnoLh, *sv);
+
+            std::lock_guard<std::mutex> listWriteLg(
+                    basicLL->getListWriteLock());
+            basicLL->appendToList(lg, listWriteLg, *sv);
+            basicLL->updateHighSeqno(listWriteLg, *sv);
             expectedSeqno.push_back(i);
         }
         return expectedSeqno;
@@ -108,11 +109,12 @@ protected:
                                           TrackReference::No,
                                           WantsDeleted::Yes)
                                           ->toOrderedStoredValue();
+
+        std::lock_guard<std::mutex> listWriteLg(basicLL->getListWriteLock());
         EXPECT_EQ(SequenceList::UpdateStatus::Success,
-                  basicLL->updateListElem(lg, *osv));
+                  basicLL->updateListElem(lg, listWriteLg, *osv));
         osv->setBySeqno(highSeqno + 1);
-        std::lock_guard<std::mutex> highSeqnoLh(basicLL->getHighSeqnosLock());
-        basicLL->updateHighSeqno(highSeqnoLh, *osv);
+        basicLL->updateHighSeqno(listWriteLg, *osv);
     }
 
     /**
@@ -131,14 +133,15 @@ protected:
                                           WantsDeleted::Yes)
                                           ->toOrderedStoredValue();
 
+        std::lock_guard<std::mutex> listWriteLg(basicLL->getListWriteLock());
         EXPECT_EQ(SequenceList::UpdateStatus::Append,
-                  basicLL->updateListElem(lg, *osv));
+                  basicLL->updateListElem(lg, listWriteLg, *osv));
 
         /* Release the current sv from the HT */
         StoredDocKey sKey = makeStoredDocKey(key);
         auto hbl = ht.getLockedBucket(sKey);
         auto ownedSv = ht.unlocked_release(hbl, osv->getKey());
-        basicLL->markItemStale(std::move(ownedSv));
+        basicLL->markItemStale(listWriteLg, std::move(ownedSv));
 
         /* Add a new storedvalue for the append */
         Item itm(sKey,
@@ -152,9 +155,9 @@ protected:
                  /*bySeqno*/ highSeqno + 1);
         auto newSv = ht.unlocked_addNewStoredValue(hbl, itm);
 
-        basicLL->appendToList(lg, *(newSv->toOrderedStoredValue()));
-        std::lock_guard<std::mutex> highSeqnoLh(basicLL->getHighSeqnosLock());
-        basicLL->updateHighSeqno(highSeqnoLh, *(newSv->toOrderedStoredValue()));
+        basicLL->appendToList(
+                lg, listWriteLg, *(newSv->toOrderedStoredValue()));
+        basicLL->updateHighSeqno(listWriteLg, *(newSv->toOrderedStoredValue()));
     }
 
     /**
@@ -411,11 +414,14 @@ TEST_F(BasicLinkedListTest, MarkStale) {
     size_t svMetaDataSize = ownedSv->metaDataSize();
 
     /* Mark the item stale */
-    basicLL->markItemStale(std::move(ownedSv));
+    {
+        std::lock_guard<std::mutex> writeGuard(basicLL->getListWriteLock());
+        basicLL->markItemStale(writeGuard, std::move(ownedSv));
+    }
 
     /* Check if the StoredValue is marked stale */
     {
-        std::lock_guard<std::mutex> writeGuard(basicLL->getWriteLock());
+        std::lock_guard<std::mutex> writeGuard(basicLL->getListWriteLock());
         EXPECT_TRUE(nonOwnedSvPtr->isStale(writeGuard));
     }
 
index ad9478e..da60b55 100644 (file)
@@ -381,7 +381,7 @@ TEST_F(EphTombstoneTest, ImmediatePurgeOfAliveStale) {
         ASSERT_EQ(staleIt->getKey(), newIt->getKey());
         {
             std::lock_guard<std::mutex> writeGuard(
-                    mockEpheVB->getLL()->getWriteLock());
+                    mockEpheVB->getLL()->getListWriteLock());
             ASSERT_TRUE(staleIt->isStale(writeGuard));
             ASSERT_FALSE(newIt->isStale(writeGuard));
         }
@@ -529,4 +529,4 @@ TEST_F(EphemeralVBucketTest, AppendUpdatesHighestDedupedSeqno) {
     EXPECT_EQ(3, mockEpheVB->purgeTombstones(0));
 
     EXPECT_EQ(6, mockEpheVB->getLL()->getHighestDedupedSeqno());
-}
\ No newline at end of file
+}