kv: fix some bugs in memdb #10550

tanghaodong25 · 2016-08-03T01:38:28Z

I'm trying to replace rocksdb with memdb to identify the overheads of rocksdb for bluestore, but I hit a few errors.

Can't kill ceph-osd process and then restart it.
if using memdb for bitmap allocator, it will hit assert fault in 'split_key' function.
fix iterator invalidation in memdb.

Signed-off-by: Haodong Tang haodong.tang@intel.com

liewegas · 2016-08-10T21:54:49Z

src/kv/MemDB.cc

+  size_t pos = m_key_value.first.find(KEY_DELIM, 0);
+  if (pos == std::string::npos) {
+    return false;
+  }


hmm, why is there a key that doesn't have KEY_DELIM?

After checking for iterator invalidation, I no longer hit the 'assert_fail'. So I will delete this commit.

liewegas · 2016-08-10T21:56:39Z

rebase please?

tanghaodong25 · 2016-08-11T06:24:03Z

@liewegas have updated and rebased.
Thanks.

liewegas · 2016-08-11T14:08:14Z

Ok, I think in order to fix this properly, we need a more complicated solution. We can't have any update to the kv db anywhere invalidate all iterators. And a sequence like (1) create iterator, (2) insert value and invalidate, (3) create second iterator, (4) use first iterator leads to the same problem (using the invalid iterator created in (1)). Instead,

replace the bool with a uint64_t seq.
store the seq in the iterator class when we create the btree_map iterator.
when we update the btree, increment seq.
make an iterator validate() function (or similar) that checks if the seq doesn't match. if so, create a new btree_map iterator with the current position (m_key_value.first I think).

chhabaramesh · 2016-08-11T14:20:40Z

src/kv/MemDB.cc

@@ -397,7 +401,7 @@ void MemDB::MDBWholeSpaceIteratorImpl::fill_current()

 bool MemDB::MDBWholeSpaceIteratorImpl::valid()
 {
-  if (m_key_value.first.empty()) {
+  if (!(*is_valid) || m_key_value.first.empty()) {


Rest looks fine, But I am not sure if current iterator becomes invalid then we should return invalid or restart iterator from current position. And if we return invalid, will bluestore take care of this and restart new iterator. I will confirm soon,

I did not notice @liewegas comment . His comment supersedes mine.

After keeping track of seqno, I think just restarting the iterator with m_key_value.first as key should work. m_iter = m_btree_p->lower_bound(k);

@chhabaramesh @liewegas Thanks for your comments.

I'm not sure if bluestore will retry to get a valid iterator before, so I just add a bool. Then I will fix it with seqno:)

chhabaramesh · 2016-08-13T09:56:02Z

src/kv/MemDB.h


  public:
    MDBWholeSpaceIteratorImpl(btree::btree_map<std::string, bufferptr> *btree_p,
-                             std::mutex *btree_lock_p) {
+                             std::mutex *btree_lock_p, uint64_t *btree_seq_) {
        m_btree_p = btree_p;
        m_btree_lock_p = btree_lock_p;


I think we need to take btree lock here.
Also please fix indentation.

chhabaramesh · 2016-08-13T10:10:10Z

@tanghaodong25 I have some minor comments on latest code. Please fix those and retest. Overall looks fine to me.

tanghaodong25 · 2016-08-13T16:07:57Z

@chhabaramesh Thanks. :)

liewegas · 2016-08-13T20:30:01Z

src/kv/MemDB.cc

@@ -472,24 +494,24 @@ int MemDB::MDBWholeSpaceIteratorImpl::seek_to_first(const std::string &k)
 {
  std::lock_guard<std::mutex> l(*m_btree_lock_p);
  free_last();
-  if (k.empty()) {
+  if (k == "NULL") {


pretty sure k.empty() is the right thing here...

tanghaodong25 · 2016-08-13T23:37:21Z

@liewegas updated. Thanks.

liewegas · 2016-08-17T13:33:16Z

src/kv/MemDB.cc

  if (m_iter == m_btree_p->end()) {
    return -1;
  }
+  fill_current();


these two changes are fixing an unrelated bug in seek_to_*, right? Can you put them in a separate commit?

liewegas · 2016-08-17T13:34:12Z

It would be great to have a test for this in test/objectstore/test_kv.cc... :)

chhabaramesh · 2016-08-18T04:17:20Z

@tanghaodong25, please also run ceph_test_objectstore.

Using memdb for bluestore kvbackend, we will hit segfault when we use 'kill' command to shut down osd process. After destructing pg, some reference to bluestore will be release, but bluestore has been deleted at this time. Signed-off-by: Haodong Tang <haodong.tang@intel.com>

Signed-off-by: Haodong Tang <haodong.tang@intel.com>

tanghaodong25 · 2016-08-18T15:42:27Z

@chhabaramesh @liewegas All test in "ceph_test_objectstore" and "ceph_test_keyvaluedb" passed. :)

tanghaodong25 changed the title ~~kv: fix osd shutdown error if using memdb~~ kv: fix some bugs in memdb Aug 10, 2016

tanghaodong25 force-pushed the in-memory-db branch from db00156 to 71f364f Compare August 10, 2016 10:58

liewegas added bug-fix core bluestore labels Aug 10, 2016

liewegas reviewed Aug 10, 2016
View reviewed changes

tanghaodong25 force-pushed the in-memory-db branch from 71f364f to 8c3365c Compare August 11, 2016 05:28

chhabaramesh reviewed Aug 11, 2016
View reviewed changes

tanghaodong25 force-pushed the in-memory-db branch from 8c3365c to b8551bd Compare August 12, 2016 09:52

chhabaramesh reviewed Aug 13, 2016
View reviewed changes

tanghaodong25 force-pushed the in-memory-db branch from b8551bd to 26aaa7c Compare August 13, 2016 12:49

liewegas reviewed Aug 13, 2016
View reviewed changes

tanghaodong25 force-pushed the in-memory-db branch from 26aaa7c to 17cdc2d Compare August 13, 2016 23:27

liewegas reviewed Aug 17, 2016
View reviewed changes

haodong added 4 commits August 18, 2016 15:17

kv: fix iterator invalidation in memdb.

00289b2

Signed-off-by: Haodong Tang <haodong.tang@intel.com>

kv: fill 'm_key_value' after 'seek_to_first' or 'seek_to_last'

7e72a09

Signed-off-by: Haodong Tang <haodong.tang@intel.com>

kv: fix iterator bug in prev

f5a320c

Signed-off-by: Haodong Tang <haodong.tang@intel.com>

tanghaodong25 force-pushed the in-memory-db branch from 17cdc2d to f5a320c Compare August 18, 2016 15:38

liewegas merged commit 8c39bb0 into ceph:master Aug 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kv: fix some bugs in memdb #10550

kv: fix some bugs in memdb #10550

tanghaodong25 commented Aug 3, 2016 •

edited

liewegas Aug 10, 2016

tanghaodong25 Aug 11, 2016

liewegas commented Aug 10, 2016

tanghaodong25 commented Aug 11, 2016

liewegas commented Aug 11, 2016

chhabaramesh Aug 11, 2016

chhabaramesh Aug 11, 2016

chhabaramesh Aug 11, 2016

tanghaodong25 Aug 12, 2016

chhabaramesh Aug 13, 2016 •

edited

chhabaramesh commented Aug 13, 2016

tanghaodong25 commented Aug 13, 2016

liewegas Aug 13, 2016

tanghaodong25 commented Aug 13, 2016

liewegas Aug 17, 2016

liewegas commented Aug 17, 2016

chhabaramesh commented Aug 18, 2016

tanghaodong25 commented Aug 18, 2016

kv: fix some bugs in memdb #10550

kv: fix some bugs in memdb #10550

Conversation

tanghaodong25 commented Aug 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liewegas commented Aug 10, 2016

tanghaodong25 commented Aug 11, 2016

liewegas commented Aug 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chhabaramesh Aug 13, 2016 • edited

Choose a reason for hiding this comment

chhabaramesh commented Aug 13, 2016

tanghaodong25 commented Aug 13, 2016

Choose a reason for hiding this comment

tanghaodong25 commented Aug 13, 2016

Choose a reason for hiding this comment

liewegas commented Aug 17, 2016

chhabaramesh commented Aug 18, 2016

tanghaodong25 commented Aug 18, 2016

tanghaodong25 commented Aug 3, 2016 •

edited

chhabaramesh Aug 13, 2016 •

edited