os/bluestore/BlueStore: fix rename

Install a negative onode entry at the old name position.
Otherwise, a simple transaction like

 rename a -> b
 touch b

will re-read the old b onode key on the second op, and chaos will
ensue (e.g., because it'll reference the same extents from a
different object).

Signed-off-by: Sage Weil <sage@redhat.com>

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 1a17a2859f934d6c7540cfe9bf54a986af23189c..e3844e0044ef019c5a8cedbf59f05e6fb941dbf9 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -508,16 +508,27 @@ void BlueStore::OnodeHashLRU::rename(const ghobject_t& old_oid,
   ceph::unordered_map<ghobject_t,OnodeRef>::iterator po, pn;
   po = onode_map.find(old_oid);
   pn = onode_map.find(new_oid);
+  assert(po != pn);
 
   assert(po != onode_map.end());
   if (pn != onode_map.end()) {
+    dout(30) << __func__ << "  removing target " << pn->second << dendl;
     lru_list_t::iterator p = lru.iterator_to(*pn->second);
     lru.erase(p);
     onode_map.erase(pn);
   }
-  onode_map.insert(make_pair(new_oid, po->second));
-  _touch(po->second);
-  onode_map.erase(po);
+  OnodeRef o = po->second;
+
+  // install a non-existent onode at old location
+  po->second.reset(new Onode(old_oid, o->key));
+  po->second->exists = false;
+  lru.push_back(*po->second);
+
+  // add at new position and fix oid, key
+  onode_map.insert(make_pair(new_oid, o));
+  _touch(o);
+  o->oid = new_oid;
+  get_object_key(new_oid, &o->key);
 }
 
 bool BlueStore::OnodeHashLRU::get_next(
@@ -5422,15 +5433,9 @@ int BlueStore::_rename(TransContext *txc,
       return r;
   }
 
-  get_object_key(old_oid, &old_key);
-  get_object_key(new_oid, &new_key);
-
-  c->onode_map.rename(old_oid, new_oid);
-  oldo->oid = new_oid;
-  oldo->key = new_key;
-
-  txc->t->rmkey(PREFIX_OBJ, old_key);
+  txc->t->rmkey(PREFIX_OBJ, oldo->key);
   txc->write_onode(oldo);
+  c->onode_map.rename(old_oid, new_oid);  // this adjusts oldo->{oid,key}
   r = 0;
 
  out:

diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc
index aa387bd61bf5d850783000dc3f09fbf47973f3b6..738265d5130db10ab409d2ffe27ca6a620b9aa78 100644 (file)
--- a/src/test/objectstore/store_test.cc
+++ b/src/test/objectstore/store_test.cc
@@ -2623,6 +2623,42 @@ TEST_P(StoreTest, TwoHash) {
   ASSERT_EQ(r, 0);
 }
 
+TEST_P(StoreTest, Rename) {
+  ObjectStore::Sequencer osr("test");
+  coll_t cid(spg_t(pg_t(0, 2122),shard_id_t::NO_SHARD));
+  ghobject_t srcoid(hobject_t("src_oid", "", CEPH_NOSNAP, 0, 0, ""));
+  ghobject_t dstoid(hobject_t("dest_oid", "", CEPH_NOSNAP, 0, 0, ""));
+  bufferlist data;
+  data.append("foo");
+  int r;
+  {
+    ObjectStore::Transaction t;
+    t.create_collection(cid, 0);
+    t.write(cid, srcoid, 0, data.length(), data);
+    r = store->apply_transaction(&osr, t);
+    ASSERT_EQ(r, 0);
+  }
+  ASSERT_TRUE(store->exists(cid, srcoid));
+  {
+    ObjectStore::Transaction t;
+    t.collection_move_rename(cid, srcoid, cid, dstoid);
+    t.write(cid, srcoid, 0, data.length(), data);
+    t.setattr(cid, srcoid, "attr", data);
+    r = store->apply_transaction(&osr, t);
+    ASSERT_EQ(r, 0);
+  }
+  ASSERT_TRUE(store->exists(cid, srcoid));
+  ASSERT_TRUE(store->exists(cid, dstoid));
+  {
+    ObjectStore::Transaction t;
+    t.remove(cid, dstoid);
+    t.remove(cid, srcoid);
+    t.remove_collection(cid);
+    r = store->apply_transaction(&osr, t);
+    ASSERT_EQ(r, 0);
+  }
+}
+
 TEST_P(StoreTest, MoveRename) {
   ObjectStore::Sequencer osr("test");
   coll_t cid(spg_t(pg_t(0, 212),shard_id_t::NO_SHARD));