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 c5a5b69c95b670bb1af14e925b736855280bb9ee..ba73dc8ee3af6a81200e8e027f83f14f2502d8df 100644 (file)
--- a/src/os/DBObjectMap.cc
+++ b/src/os/DBObjectMap.cc
@@ -1070,6 +1070,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);
@@ -1111,7 +1113,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);
     MapHeaderLock hl(this, *oid);
@@ -1122,11 +1123,18 @@ int DBObjectMap::sync(const ghobject_t *oid,
       header->spos = *spos;
       set_map_header(hl, *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;