MDEV-25512 Deadlock between sux_lock::u_x_upgrade() and sux_lock::u_lock()

In the SUX_LOCK_GENERIC implementation, we can remember at most
one pending exclusive lock request. If multiple exclusive lock
requests are pending, the WRITER_WAITING flag will be cleared when
the first waiting writer acquires the exclusive lock.

ssux_lock_low::update_lock(): If WRITER_WAITING is set, wake up
the writer even if the UPDATER flag is set, because the waiting
writer may be in the process of upgrading its U lock to X.

rw_lock::read_unlock(): Also indicate that an X lock waiter must
be woken up if an U lock exists.

This fix may cause unnecessary wake-ups and system calls, but this
is the best that we can do. Ideally we would use the MDEV-25404
idea of a separate 'writer' mutex, but there is no portable way to
request that a non-recursive mutex be created, and InnoDB requires
the ability to transfer buf_block_t::lock ownership to an I/O thread.

To allow problems like this to be caught more reliably in the future,
we add a unit test for srw_mutex, srw_lock, ssux_lock, sux_lock.

storage/innobase/CMakeLists.txt

@@ -397,3 +397,7 @@ IF(NOT (PLUGIN_INNOBASE STREQUAL DYNAMIC))
   TARGET_LINK_LIBRARIES(innobase tpool mysys)
   ADD_SUBDIRECTORY(${CMAKE_SOURCE_DIR}/extra/mariabackup ${CMAKE_BINARY_DIR}/extra/mariabackup)
 ENDIF()
+
+IF(WITH_UNIT_TESTS)
+  ADD_SUBDIRECTORY(unittest)
+ENDIF()

storage/innobase/include/rw_lock.h

@@ -161,13 +161,14 @@ class rw_lock
   bool read_unlock()
   {
     auto l= lock.fetch_sub(1, std::memory_order_release);
+    DBUG_ASSERT(!(l & WRITER)); /* no write lock must have existed */
 #ifdef SUX_LOCK_GENERIC
     DBUG_ASSERT(~(WRITER_PENDING | UPDATER) & l); /* at least one read lock */
+    return (~(WRITER_PENDING | UPDATER) & l) == 1;
 #else /* SUX_LOCK_GENERIC */
     DBUG_ASSERT(~(WRITER_PENDING) & l); /* at least one read lock */
-#endif /* SUX_LOCK_GENERIC */
-    DBUG_ASSERT(!(l & WRITER)); /* no write lock must have existed */
     return (~WRITER_PENDING & l) == 1;
+#endif /* SUX_LOCK_GENERIC */
   }
 #ifdef SUX_LOCK_GENERIC
   /** Release an update lock */

storage/innobase/sync/srw_lock.cc

@@ -111,13 +111,13 @@ void ssux_lock_low::update_lock(uint32_t l)
 {
   do
   {
-    if (l == WRITER_WAITING)
+    if ((l | UPDATER) == (UPDATER | WRITER_WAITING))
     {
     wake_writer:
       pthread_mutex_lock(&mutex);
       for (;;)
       {
-        if (l == WRITER_WAITING)
+        if ((l | UPDATER) == (UPDATER | WRITER_WAITING))
           pthread_cond_signal(&cond_exclusive);
         l= value();
         if (!(l & WRITER_PENDING))
@@ -133,7 +133,7 @@ void ssux_lock_low::update_lock(uint32_t l)
         ut_delay(srv_spin_wait_delay);
         if (update_trylock(l))
           return;
-        else if (l == WRITER_WAITING)
+        else if ((l | UPDATER) == (UPDATER | WRITER_WAITING))
           goto wake_writer;
       }
 

storage/innobase/unittest/CMakeLists.txt

+# Copyright (c) 2021, MariaDB Corporation.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1335 USA
+
+INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/include
+                    ${CMAKE_SOURCE_DIR}/unittest/mytap
+                    ${CMAKE_SOURCE_DIR}/storage/innobase/include
+                    ${CMAKE_SOURCE_DIR}/tpool)
+ADD_EXECUTABLE(innodb_sync-t innodb_sync-t.cc ../sync/srw_lock.cc)
+TARGET_LINK_LIBRARIES(innodb_sync-t mysys mytap)
+ADD_DEPENDENCIES(innodb_sync-t GenError)
+MY_ADD_TEST(innodb_sync)

storage/innobase/unittest/innodb_sync-t.cc

+/* Copyright (c) 2021, MariaDB Corporation.
+
+   This program is free software; you can redistribute it and/or
+   modify it under the terms of the GNU General Public License as
+   published by the Free Software Foundation; version 2 of the License.
+
+   This program is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335  USA */
+
+#include <thread>
+#include "tap.h"
+#include "my_sys.h"
+#include "sux_lock.h"
+
+static std::atomic<bool> critical;
+
+ulong srv_n_spin_wait_rounds= 30;
+uint srv_spin_wait_delay= 4;
+
+constexpr unsigned N_THREADS= 30;
+constexpr unsigned N_ROUNDS= 100;
+constexpr unsigned M_ROUNDS= 100;
+
+static srw_mutex m;
+
+static void test_srw_mutex()
+{
+  for (auto i= N_ROUNDS * M_ROUNDS; i--; )
+  {
+    m.wr_lock();
+    assert(!critical);
+    critical= true;
+    critical= false;
+    m.wr_unlock();
+  }
+}
+
+static srw_lock_low l;
+
+static void test_srw_lock()
+{
+  for (auto i= N_ROUNDS; i--; )
+  {
+    l.wr_lock();
+    assert(!critical);
+    critical= true;
+    critical= false;
+    l.wr_unlock();
+
+    for (auto j= M_ROUNDS; j--; )
+    {
+      l.rd_lock();
+      assert(!critical);
+      l.rd_unlock();
+    }
+  }
+}
+
+static ssux_lock_low ssux;
+
+static void test_ssux_lock()
+{
+  for (auto i= N_ROUNDS; i--; )
+  {
+    ssux.wr_lock();
+    assert(!critical);
+    critical= true;
+    critical= false;
+    ssux.wr_unlock();
+
+    for (auto j= M_ROUNDS; j--; )
+    {
+      ssux.rd_lock();
+      assert(!critical);
+      ssux.rd_unlock();
+    }
+
+    for (auto j= M_ROUNDS; j--; )
+    {
+      ssux.u_lock();
+      assert(!critical);
+      ssux.u_wr_upgrade();
+      assert(!critical);
+      critical= true;
+      critical= false;
+      ssux.wr_u_downgrade();
+      ssux.u_unlock();
+    }
+  }
+}
+
+static sux_lock<ssux_lock_low> sux;
+
+static void test_sux_lock()
+{
+  for (auto i= N_ROUNDS; i--; )
+  {
+    sux.x_lock();
+    assert(!critical);
+    critical= true;
+    for (auto j= M_ROUNDS; j--; )
+      sux.x_lock();
+    critical= false;
+    for (auto j= M_ROUNDS + 1; j--; )
+      sux.x_unlock();
+
+    for (auto j= M_ROUNDS; j--; )
+    {
+      sux.s_lock();
+      assert(!critical);
+      sux.s_unlock();
+    }
+
+    for (auto j= M_ROUNDS / 2; j--; )
+    {
+      sux.u_lock();
+      assert(!critical);
+      sux.u_lock();
+      sux.u_x_upgrade();
+      assert(!critical);
+      critical= true;
+      sux.x_unlock();
+      critical= false;
+      sux.x_u_downgrade();
+      sux.u_unlock();
+    }
+  }
+}
+
+int main(int argc __attribute__((unused)), char **argv)
+{
+  std::thread t[N_THREADS];
+
+  MY_INIT(argv[0]);
+
+  plan(4);
+
+  m.init();
+  for (auto i= N_THREADS; i--; )
+    t[i]= std::thread(test_srw_mutex);
+
+  for (auto i= N_THREADS; i--; )
+    t[i].join();
+
+  m.destroy();
+  ok(true, "srw_mutex");
+
+  l.init();
+
+  for (auto i= N_THREADS; i--; )
+    t[i]= std::thread(test_srw_lock);
+
+  for (auto i= N_THREADS; i--; )
+    t[i].join();
+
+  ok(true, "srw_lock");
+
+  l.destroy();
+
+  ssux.init();
+  for (auto i= N_THREADS; i--; )
+    t[i]= std::thread(test_ssux_lock);
+
+  for (auto i= N_THREADS; i--; )
+    t[i].join();
+
+  ok(true, "ssux_lock");
+  ssux.destroy();
+
+  sux.init();
+  for (auto i= N_THREADS; i--; )
+    t[i]= std::thread(test_sux_lock);
+
+  for (auto i= N_THREADS; i--; )
+    t[i].join();
+
+  ok(true, "sux_lock");
+  sux.free();
+}