MB-19113: Address false positive lock inversion seen with test_mb16357 67/62567/7
authorabhinavdangeti <abhinav@couchbase.com>
Fri, 8 Apr 2016 20:57:42 +0000 (13:57 -0700)
committerChiyoung Seo <chiyoung@couchbase.com>
Mon, 11 Apr 2016 17:11:47 +0000 (17:11 +0000)
The inversion being pointed out by this test case is between the snapshot
lock and the hash table lock and this scenario would never happen
during regular operation, this is because the first thread points out
that the vbucket is active while the second indicates that the vbucket
is replica. These 2 operations can never occur simulataneously.

ThreadSanitizer assumes that this is a lock inversion because the 2
operations are done by different threads (main_thread and dcp_thread).

The fix here suppresses this thread sanitizer warning by getting rid of
the dcp_thread, and instead have the main_thread perform the activity
that the dcp thread is responsible for.

WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=5899)
  Cycle in lock order graph: M21372 (0x7d780000f510) => M21408 (0x7d640000f920) => M21372

  Mutex M21408 acquired here while holding mutex M21372 in main thread:
    #0 pthread_mutex_lock <null> (engine_testapp+0x00000047e970)
    #1 cb_mutex_enter <null> (libplatform.so.0.1.0+0x000000003870)
    #2 Mutex::acquire() /home/couchbase/couchbase/ep-engine/src/mutex.cc:31 (ep.so+0x0000001e287e)
    #3 LockHolder::lock() /home/couchbase/couchbase/ep-engine/src/locks.h:71 (ep.so+0x000000082543)
    #4 LockHolder::LockHolder(Mutex&, bool) /home/couchbase/couchbase/ep-engine/src/locks.h:48 (ep.so+0x0000000821b2)
    #5 VBucket::getSnapshotLock() /home/couchbase/couchbase/ep-engine/src/vbucket.h:212 (ep.so+0x000000104c72)
    #6 EventuallyPersistentStore::queueDirty(RCPtr<VBucket>&, StoredValue*, LockHolder*, bool, bool, bool) /home/couchbase/couchbase/ep-engine/src/ep.cc:2863 (ep.so+0x0000000d7123)
    #7 EventuallyPersistentStore::set(Item const&, void const*, bool, unsigned char) /home/couchbase/couchbase/ep-engine/src/ep.cc:683 (ep.so+0x0000000d9dfa)
    #8 EventuallyPersistentEngine::store(void const*, void*, unsigned long*, ENGINE_STORE_OPERATION, unsigned short) /home/couchbase/couchbase/ep-engine/src/ep_engine.cc:2128 (ep.so+0x00000013d538)
    #9 EvpStore(engine_interface*, void const*, void*, unsigned long*, ENGINE_STORE_OPERATION, unsigned short) /home/couchbase/couchbase/ep-engine/src/ep_engine.cc:229 (ep.so+0x00000013712d)
    #10 mock_store /home/couchbase/couchbase/memcached/programs/engine_testapp/engine_testapp.c (engine_testapp+0x0000004c7304)
    #11 storeCasVb11(engine_interface*, engine_interface_v1*, void const*, ENGINE_STORE_OPERATION, char const*, char const*, unsigned long, unsigned int, void**, unsigned long, unsigned short, unsigned int, unsigned char) /home/couchbase/couchbase/ep-engine/tests/ep_test_apis.cc:659 (ep_testsuite.so+0x0000000e8d17)
    #12 store(engine_interface*, engine_interface_v1*, void const*, ENGINE_STORE_OPERATION, char const*, char const*, void**, unsigned long, unsigned short, unsigned int, unsigned char) /home/couchbase/couchbase/ep-engine/tests/ep_test_apis.cc:631 (ep_testsuite.so+0x0000000e654a)
    #13 test_mb16357(engine_interface*, engine_interface_v1*) /home/couchbase/couchbase/ep-engine/tests/ep_testsuite.cc:11713 (ep_testsuite.so+0x0000000afc36)
    #14 execute_test /home/couchbase/couchbase/memcached/programs/engine_testapp/engine_testapp.c (engine_testapp+0x0000004c4e2f)
    #15 main crtstuff.c (engine_testapp+0x0000004c2d91)

  Mutex M21372 acquired here while holding mutex M21408 in thread T10:
    #0 pthread_mutex_lock <null> (engine_testapp+0x00000047e970)
    #1 cb_mutex_enter <null> (libplatform.so.0.1.0+0x000000003870)
    #2 Mutex::acquire() /home/couchbase/couchbase/ep-engine/src/mutex.cc:31 (ep.so+0x0000001e287e)
    #3 LockHolder::lock() /home/couchbase/couchbase/ep-engine/src/locks.h:71 (ep.so+0x000000082543)
    #4 LockHolder::LockHolder(Mutex&, bool) /home/couchbase/couchbase/ep-engine/src/locks.h:48 (ep.so+0x0000000821b2)
    #5 HashTable::getLockedBucket(int, int*) /home/couchbase/couchbase/ep-engine/src/stored-value.h:1266 (ep.so+0x00000008418a)
    #6 HashTable::getLockedBucket(std::string const&, int*) /home/couchbase/couchbase/ep-engine/src/stored-value.h:1295 (ep.so+0x00000007df9b)
    #7 EventuallyPersistentStore::setWithMeta(Item const&, unsigned long, void const*, bool, bool, unsigned char, bool, bool) /home/couchbase/couchbase/ep-engine/src/ep.cc:1827 (ep.so+0x0000000e6b4f)
    #8 PassiveStream::commitMutation(MutationResponse*, bool) /home/couchbase/couchbase/ep-engine/src/dcp-stream.cc:1369 (ep.so+0x00000029ba21)
    #9 PassiveStream::processMutation(MutationResponse*) /home/couchbase/couchbase/ep-engine/src/dcp-stream.cc:1341 (ep.so+0x00000029a7a0)
    #10 PassiveStream::processBufferedMessages(unsigned int&) /home/couchbase/couchbase/ep-engine/src/dcp-stream.cc:1281 (ep.so+0x00000029a0f2)
    #11 DcpConsumer::processBufferedItems() /home/couchbase/couchbase/ep-engine/src/dcp-consumer.cc:599 (ep.so+0x000000262a23)
    #12 Processer::run() /home/couchbase/couchbase/ep-engine/src/dcp-consumer.cc:48 (ep.so+0x0000002625ff)
    #13 ExecutorThread::run() /home/couchbase/couchbase/ep-engine/src/executorthread.cc:110 (ep.so+0x0000001e3dd9)
    #14 launch_executor_thread(void*) /home/couchbase/couchbase/ep-engine/src/executorthread.cc:34 (ep.so+0x0000001e32ea)
    #15 platform_thread_wrap /home/couchbase/couchbase/platform/src/cb_pthreads.c (libplatform.so.0.1.0+0x00000000362c)

Change-Id: I6c7b1fadf76529a044341a4a9b6ed0ea829c4999
Reviewed-on: http://review.couchbase.org/62567
Well-Formed: buildbot <build@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
Reviewed-by: Chiyoung Seo <chiyoung@couchbase.com>
tests/ep_testsuite.cc

index 92dae11..d0e303e 100644 (file)
@@ -7543,7 +7543,7 @@ static enum test_result test_all_keys_api(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1
     protocol_binary_request_header *pkt1 =
         createPacket(CMD_GET_KEYS, 0, 0, ext, extlen,
                      "key_10", keylen, NULL, 0, 0x00);
-    delete ext;
+    delete[] ext;
 
     check(h1->unknown_command(h, NULL, pkt1, add_response) == ENGINE_SUCCESS,
             "Failed to get all_keys, sort: ascending");
@@ -11643,57 +11643,6 @@ struct mb16357_ctx {
 };
 
 extern "C" {
-    static void dcp_thread_func(void *args) {
-        struct mb16357_ctx *ctx = static_cast<mb16357_ctx *>(args);
-
-        const void *cookie = testHarness.create_cookie();
-        uint32_t opaque = 0xFFFF0000;
-        uint32_t flags = 0;
-        std::string name = "unittest";
-
-        while (get_int_stat(ctx->h, ctx->h1, "ep_pending_compactions") == 0);
-
-        // Switch to replica
-        check(set_vbucket_state(ctx->h, ctx->h1, 0, vbucket_state_replica),
-                "Failed to set vbucket state.");
-
-        // Open consumer connection
-        checkeq(ctx->h1->dcp.open(ctx->h, cookie, opaque, 0, flags,
-                                  (void*)name.c_str(), name.length()),
-                ENGINE_SUCCESS,
-                "Failed dcp Consumer open connection.");
-
-        add_stream_for_consumer(ctx->h, ctx->h1, cookie, opaque++, 0, 0,
-                PROTOCOL_BINARY_RESPONSE_SUCCESS);
-
-
-        uint32_t stream_opaque = get_int_stat(ctx->h, ctx->h1,
-                                              "eq_dcpq:unittest:stream_0_opaque",
-                                              "dcp");
-
-
-        for (int i = 1; i <= ctx->items; i++) {
-            std::stringstream ss;
-            ss << "kamakeey-" << i;
-
-            // send mutations in single mutation snapshots to race more with compaction
-            checkeq(ctx->h1->dcp.snapshot_marker(ctx->h, cookie,
-                                                 stream_opaque, 0/*vbid*/,
-                                                 ctx->items, ctx->items + i, 2),
-                    ENGINE_SUCCESS,
-                    "Failed to send snapshot marker");
-            checkeq(ctx->h1->dcp.mutation(ctx->h, cookie, stream_opaque,
-                                          ss.str().c_str(), ss.str().length(),
-                                          "value", 5, i * 3, 0, 0, 0,
-                                          i + ctx->items, i + ctx->items,
-                                          0, 0, "", 0, INITIAL_NRU_VALUE),
-                    ENGINE_SUCCESS,
-                    "Failed to send dcp mutation");
-        }
-
-        testHarness.destroy_cookie(cookie);
-    }
-
     static void compact_thread_func(void *args) {
         struct mb16357_ctx *ctx = static_cast<mb16357_ctx *>(args);
         compact_db(ctx->h, ctx->h1, 0, 99, ctx->items, 1);
@@ -11721,17 +11670,59 @@ static enum test_result test_mb16357(ENGINE_HANDLE *h,
     testHarness.time_travel(3617); // force expiry pushing time forward.
 
     struct mb16357_ctx ctx(h, h1, num_items);
-    cb_thread_t cp_thread, dcp_thread;
+    cb_thread_t cp_thread;
 
     cb_assert(cb_create_thread(&cp_thread,
                                compact_thread_func,
                                &ctx, 0) == 0);
-    cb_assert(cb_create_thread(&dcp_thread,
-                               dcp_thread_func,
-                               &ctx, 0) == 0);
+
+    const void *cookie = testHarness.create_cookie();
+    uint32_t opaque = 0xFFFF0000;
+    uint32_t flags = 0;
+    std::string name = "unittest";
+
+    while (get_int_stat(h, h1, "ep_pending_compactions") == 0);
+
+    // Switch to replica
+    check(set_vbucket_state(h, h1, 0, vbucket_state_replica),
+          "Failed to set vbucket state.");
+
+    // Open consumer connection
+    checkeq(h1->dcp.open(h, cookie, opaque, 0, flags,
+                         (void*)name.c_str(), name.length()),
+            ENGINE_SUCCESS,
+            "Failed dcp Consumer open connection.");
+
+    add_stream_for_consumer(h, h1, cookie, opaque++, 0, 0,
+                            PROTOCOL_BINARY_RESPONSE_SUCCESS);
+
+    uint32_t stream_opaque = get_int_stat(h, h1,
+                                          "eq_dcpq:unittest:stream_0_opaque",
+                                          "dcp");
+
+
+    for (int i = 1; i <= num_items; i++) {
+        std::stringstream ss;
+        ss << "kamakeey-" << i;
+
+        // send mutations in single mutation snapshots to race more with compaction
+        checkeq(h1->dcp.snapshot_marker(h, cookie,
+                                        stream_opaque, 0/*vbid*/,
+                                        num_items, num_items + i, 2),
+                ENGINE_SUCCESS,
+                "Failed to send snapshot marker");
+        checkeq(h1->dcp.mutation(h, cookie, stream_opaque,
+                                 ss.str().c_str(), ss.str().length(),
+                                 "value", 5, i * 3, 0, 0, 0,
+                                 i + num_items, i + num_items,
+                                 0, 0, "", 0, INITIAL_NRU_VALUE),
+                ENGINE_SUCCESS,
+                "Failed to send dcp mutation");
+    }
+
+    testHarness.destroy_cookie(cookie);
 
     cb_assert(cb_join_thread(cp_thread) == 0);
-    cb_assert(cb_join_thread(dcp_thread) == 0);
 
     return SUCCESS;
 }