DBObjectMap: lock header_lock on sync()

Otherwise, we can race with another thread updating state.seq
resulting in the old, smaller value getting persisted.  If there
is a crash at that time, we will reuse a sequence number, resulting
in an inconsistent node tree and bug #9891.

Fixes: 9891
Backport: giant, firefly, dumpling
Signed-off-by: Samuel Just <sjust@redhat.com>
(cherry picked from commit 2b63dd25fc1c73fa42e52e9ea4ab5a45dd9422a0)

Conflicts:
	src/os/DBObjectMap.cc
        because we have state.v = 1; instead of state.v = 2;

diff --git a/src/os/DBObjectMap.cc b/src/os/DBObjectMap.cc
index 5d2a2e616242c0f67224febae08b87b06c9d9927..0a2580011463df03b6c6b6834a9e6e81536227e0 100644 (file)
--- a/src/os/DBObjectMap.cc
+++ b/src/os/DBObjectMap.cc
@@ -1042,6 +1042,8 @@ int DBObjectMap::upgrade()
     db->submit_transaction(t);
   }
   state.v = 1;
+
+  Mutex::Locker l(header_lock);
   KeyValueDB::Transaction t = db->get_transaction();
   write_state(t);
   db->submit_transaction_sync(t);
@@ -1083,7 +1085,6 @@ int DBObjectMap::init(bool do_upgrade)
 int DBObjectMap::sync(const ghobject_t *oid,
                      const SequencerPosition *spos) {
   KeyValueDB::Transaction t = db->get_transaction();
-  write_state(t);
   if (oid) {
     assert(spos);
     Header header = lookup_map_header(*oid);
@@ -1093,11 +1094,18 @@ int DBObjectMap::sync(const ghobject_t *oid,
       header->spos = *spos;
       set_map_header(*oid, *header, t);
     }
+    Mutex::Locker l(header_lock);
+    write_state(t);
+    return db->submit_transaction_sync(t);
+  } else {
+    Mutex::Locker l(header_lock);
+    write_state(t);
+    return db->submit_transaction_sync(t);
   }
-  return db->submit_transaction_sync(t);
 }
 
 int DBObjectMap::write_state(KeyValueDB::Transaction _t) {
+  assert(header_lock.is_locked_by_me());
   dout(20) << "dbobjectmap: seq is " << state.seq << dendl;
   KeyValueDB::Transaction t = _t ? _t : db->get_transaction();
   bufferlist bl;