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.

Sign up for GitHub

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

Jump to bottom

os/bluestore: put strings in mempool #12651

Merged
merged 7 commits into from Jan 15, 2017
Merged

os/bluestore: put strings in mempool #12651

merged 7 commits into from Jan 15, 2017

Conversation

liewegas
Copy link
Member

This adds string support to mempool, and then moves a few bluestore strings
into the blustore_meta_other pool. I don't think there are any remaining
strings of interest.

@liewegas
Copy link
Member Author

subsumes #12290

@liewegas liewegas mentioned this pull request Dec 23, 2016
Closed
@liewegas liewegas mentioned this pull request Dec 27, 2016
Merged
@liewegas liewegas force-pushed the wip-mempool-string branch 3 times, most recently from 95bebc9 to bea7c6d Compare December 27, 2016 14:52
@xiexingguo
Copy link
Member

Need rebase

ifed01
ifed01 reviewed Jan 9, 2017
View reviewed changes
src/os/kstore/KStore.cc
@@ -21,7 +21,7 @@

#include "KStore.h"
#include "osd/osd_types.h"
#include "kv.h"
#include "os/bluestore/kv.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to have kv.h at 'os' subfolder level to avoid cross-referencing between store implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

src/os/bluestore/kv.h
#else
# error wtf
#endif
key->append((char*)&bu, 4);
Copy link
Contributor

Choose a reason for hiding this comment

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

replace 4 with sizeof(bu) here and below?

src/kv/KeyValueDB.h
@@ -56,7 +56,13 @@ class KeyValueDB {
const std::string &k, ///< [in] Key to set
const bufferlist &bl ///< [in] Value to set
) = 0;

virtual void set(
const std::string &prefix,
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it would be better to get rid of prefix+key split in the KeyValueDB::TransactionImpl interface and use single full key parameter for all functions. And force the client to create that full key from prefix+subkey via specific 'make_key' member function(s)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. Let's do that with a separate changeset, though?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure.

@liewegas liewegas mentioned this pull request Jan 10, 2017
Closed
@liewegas liewegas requested a review from ifed01 January 11, 2017 03:28
@xiexingguo
Copy link
Member

This fails the mempool UT

The following tests FAILED:
	 35 - unittest_mempool (Failed)
	116 - osd-crush.sh (Failed)

@ifed01
Copy link
Contributor

ifed01 commented Jan 11, 2017

@liewegas - any feedback on my previous comments in this PR?

@liewegas
Copy link
Member Author

Hmm, unit test passes for me. :/

ifed01
ifed01 reviewed Jan 11, 2017
View reviewed changes
src/os/kv.h
@@ -1,13 +1,15 @@
// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
// vim: ts=8 sw=2 smarttab

#include "kv.h"
#ifndef CEPH_OS_BLUESTORE_KV_H
Copy link
Contributor

@ifed01 ifed01 Jan 11, 2017 •
edited

Choose a reason for hiding this comment

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

CEPH_OS_BLUESTORE_KV_H -> CEPH_OS_KV_H

ifed01
ifed01 approved these changes Jan 11, 2017
View reviewed changes
Copy link
Contributor

@ifed01 ifed01 left a comment

Choose a reason for hiding this comment

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

LGTM except a nit in kv.,h

allensamuels and others added 7 commits January 13, 2017 11:48
@allensamuels @liewegas
mempool: Add string to mempool data types.
78125f7 
Signed-off-by: Allen Samuels <allen.samuels@sandisk.com>
@liewegas
include/denc: parameterize string by allocator
f9e3a78 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
os/bluestore: use mempool string for onode_t attr map
22d186e 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
os/bluestore,kstore: combine kv helpers in header
685002b 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
kv: add char* versions of get, set, rmkey
399b633 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
os/bluestore: avoid substr
ff81d54 
This creates an intermediate string, which is a waste.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
os/bluestore: put Onode::key in mempool
c0e12bf 
This is a pain in the butt because std::string and
std::basic_string<...,custom allocator> are incompatible.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas liewegas merged commit 03236ed into ceph:master Jan 15, 2017
@liewegas liewegas deleted the wip-mempool-string branch January 15, 2017 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ifed01 ifed01 ifed01 approved these changes

Assignees
No one assigned
Labels
bluestore needs-qa wip-sage-testing
Projects
None yet
Milestone
No milestone
4 participants
@liewegas @xiexingguo @ifed01 @allensamuels