Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kv: fix some bugs in memdb #10550

Merged
merged 4 commits into from Aug 18, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 28 additions & 5 deletions src/kv/MemDB.cc
Expand Up @@ -270,7 +270,7 @@ int MemDB::_setkey(ms_op_t &op)
}

m_btree[key] = bufferptr((char *) bl.c_str(), bl.length());

iterator_seq_no++;
return 0;
}

Expand All @@ -284,6 +284,7 @@ int MemDB::_rmkey(ms_op_t &op)
assert(m_total_bytes >= bl_old.length());
m_total_bytes -= bl_old.length();
}
iterator_seq_no++;
/*
* Erase will call the destructor for bufferptr.
*/
Expand Down Expand Up @@ -341,6 +342,7 @@ int MemDB::_merge(ms_op_t &op)

assert((int64_t)m_total_bytes + bytes_adjusted >= 0);
m_total_bytes += bytes_adjusted;
iterator_seq_no++;
return 0;
}

Expand Down Expand Up @@ -403,6 +405,18 @@ bool MemDB::MDBWholeSpaceIteratorImpl::valid()
return true;
}

bool MemDB::MDBWholeSpaceIteratorImpl::iterator_validate() {
if (this_seq_no != *global_seq_no) {
auto key = m_key_value.first;
assert(!key.empty());
m_iter = m_btree_p->lower_bound(key);
if (m_iter == m_btree_p->end()) {
return false;
}
}
return true;
}

void
MemDB::MDBWholeSpaceIteratorImpl::free_last()
{
Expand Down Expand Up @@ -442,6 +456,10 @@ bufferlist MemDB::MDBWholeSpaceIteratorImpl::value()
int MemDB::MDBWholeSpaceIteratorImpl::next()
{
std::lock_guard<std::mutex> l(*m_btree_lock_p);
if (!iterator_validate()) {
free_last();
return -1;
}
free_last();
m_iter++;
if (m_iter != m_btree_p->end()) {
Expand All @@ -455,9 +473,13 @@ int MemDB::MDBWholeSpaceIteratorImpl::next()
int MemDB::MDBWholeSpaceIteratorImpl:: prev()
{
std::lock_guard<std::mutex> l(*m_btree_lock_p);
if (!iterator_validate()) {
free_last();
return -1;
}
free_last();
m_iter--;
if (m_iter != m_btree_p->end()) {
if (m_iter != m_btree_p->begin()) {
m_iter--;
fill_current();
return 0;
} else {
Expand All @@ -477,17 +499,17 @@ int MemDB::MDBWholeSpaceIteratorImpl::seek_to_first(const std::string &k)
} else {
m_iter = m_btree_p->lower_bound(k);
}

if (m_iter == m_btree_p->end()) {
return -1;
}
fill_current();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return 0;
}

int MemDB::MDBWholeSpaceIteratorImpl::seek_to_last(const std::string &k)
{
std::lock_guard<std::mutex> l(*m_btree_lock_p);

free_last();
if (k.empty()) {
m_iter = m_btree_p->end();
Expand All @@ -499,6 +521,7 @@ int MemDB::MDBWholeSpaceIteratorImpl::seek_to_last(const std::string &k)
if (m_iter == m_btree_p->end()) {
return -1;
}
fill_current();
return 0;
}

Expand Down
17 changes: 12 additions & 5 deletions src/kv/MemDB.h
Expand Up @@ -48,10 +48,11 @@ class MemDB : public KeyValueDB
void _encode(btree::btree_map<string, bufferptr>:: iterator iter, bufferlist &bl);
void _save();
int _load();
uint64_t iterator_seq_no;

public:
MemDB(CephContext *c, const string &path, void *p) :
m_cct(c), m_priv(p), m_db_path(path)
m_cct(c), m_priv(p), m_db_path(path), iterator_seq_no(1)
{
//Nothing as of now
}
Expand Down Expand Up @@ -132,12 +133,17 @@ class MemDB : public KeyValueDB
std::pair<string, bufferlist> m_key_value;
btree::btree_map<std::string, bufferptr> *m_btree_p;
std::mutex *m_btree_lock_p;
uint64_t *global_seq_no;
uint64_t this_seq_no;

public:
MDBWholeSpaceIteratorImpl(btree::btree_map<std::string, bufferptr> *btree_p,
std::mutex *btree_lock_p) {
std::mutex *btree_lock_p, uint64_t *iterator_seq_no) {
m_btree_p = btree_p;
m_btree_lock_p = btree_lock_p;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

std::lock_guard<std::mutex> l(*m_btree_lock_p);
global_seq_no = iterator_seq_no;
this_seq_no = *iterator_seq_no;
}

void fill_current();
Expand All @@ -147,12 +153,13 @@ class MemDB : public KeyValueDB
int seek_to_first(const std::string &k);
int seek_to_last(const std::string &k);

int seek_to_first() { return seek_to_first(NULL); };
int seek_to_last() { return seek_to_last(NULL); };
int seek_to_first() { return seek_to_first(std::string()); };
int seek_to_last() { return seek_to_last(std::string()); };

int upper_bound(const std::string &prefix, const std::string &after);
int lower_bound(const std::string &prefix, const std::string &to);
bool valid();
bool iterator_validate();

int next();
int prev();
Expand Down Expand Up @@ -183,7 +190,7 @@ class MemDB : public KeyValueDB

WholeSpaceIterator _get_iterator() {
return std::shared_ptr<KeyValueDB::WholeSpaceIteratorImpl>(
new MDBWholeSpaceIteratorImpl(&m_btree, &m_lock));
new MDBWholeSpaceIteratorImpl(&m_btree, &m_lock, &iterator_seq_no));
}

WholeSpaceIterator _get_snapshot_iterator();
Expand Down
13 changes: 7 additions & 6 deletions src/osd/OSD.cc
Expand Up @@ -2716,12 +2716,6 @@ int OSD::shutdown()
<< cpp_strerror(r) << dendl;
}

dout(10) << "syncing store" << dendl;
enable_disable_fuse(true);
store->umount();
delete store;
store = 0;
dout(10) << "Store synced" << dendl;

{
Mutex::Locker l(pg_stat_queue_lock);
Expand Down Expand Up @@ -2757,6 +2751,13 @@ int OSD::shutdown()
#endif
cct->_conf->remove_observer(this);

dout(10) << "syncing store" << dendl;
enable_disable_fuse(true);
store->umount();
delete store;
store = 0;
dout(10) << "Store synced" << dendl;

monc->shutdown();
osd_lock.Unlock();

Expand Down