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

librbd: simplify image open/close semantics #13701

Merged
merged 4 commits into from Mar 2, 2017
Merged

librbd: simplify image open/close semantics #13701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d66a40
librbd: utilize a singleton ContextWQ instead of per-image instance
Feb 28, 2017
bf6d347
librbd: cleanup synchronous open/close memory management
Feb 28, 2017
5fb1277
librbd: eliminate context switch from remove state machine
Feb 28, 2017
b4861a5
rbd-mirror: eliminate context switch from close image state machine
Feb 28, 2017
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
27 changes: 17 additions & 10 deletions src/librbd/ImageCtx.cc
View file
Open in desktop
Expand Up @@ -54,12 +54,20 @@ namespace {

class ThreadPoolSingleton : public ThreadPool {
public:
ContextWQ *op_work_queue;

explicit ThreadPoolSingleton(CephContext *cct)
: ThreadPool(cct, "librbd::thread_pool", "tp_librbd", 1,
"rbd_op_threads") {
"rbd_op_threads"),
op_work_queue(new ContextWQ("librbd::op_work_queue",
cct->_conf->rbd_op_thread_timeout,
this)) {
start();
}
~ThreadPoolSingleton() override {
op_work_queue->drain();
delete op_work_queue;

stop();
}
};
Expand Down Expand Up @@ -199,13 +207,11 @@ struct C_InvalidateCache : public Context {

memset(&header, 0, sizeof(header));

ThreadPool *thread_pool_singleton = get_thread_pool_instance(cct);
ThreadPool *thread_pool;
get_thread_pool_instance(cct, &thread_pool, &op_work_queue);
io_work_queue = new io::ImageRequestWQ(
this, "librbd::io_work_queue", cct->_conf->rbd_op_thread_timeout,
thread_pool_singleton);
op_work_queue = new ContextWQ("librbd::op_work_queue",
cct->_conf->rbd_op_thread_timeout,
thread_pool_singleton);
thread_pool);

if (cct->_conf->rbd_auto_exclusive_lock_until_manual_request) {
exclusive_lock_policy = new exclusive_lock::AutomaticPolicy(this);
Expand Down Expand Up @@ -241,12 +247,10 @@ struct C_InvalidateCache : public Context {

md_ctx.aio_flush();
data_ctx.aio_flush();
op_work_queue->drain();
io_work_queue->drain();

delete journal_policy;
delete exclusive_lock_policy;
delete op_work_queue;
delete io_work_queue;
delete operations;
delete state;
Expand Down Expand Up @@ -1092,11 +1096,14 @@ struct C_InvalidateCache : public Context {
journal_policy = policy;
}

ThreadPool *ImageCtx::get_thread_pool_instance(CephContext *cct) {
void ImageCtx::get_thread_pool_instance(CephContext *cct,
ThreadPool **thread_pool,
ContextWQ **op_work_queue) {
ThreadPoolSingleton *thread_pool_singleton;
cct->lookup_or_create_singleton_object<ThreadPoolSingleton>(
thread_pool_singleton, "librbd::thread_pool");
return thread_pool_singleton;
*thread_pool = thread_pool_singleton;
*op_work_queue = thread_pool_singleton->op_work_queue;
}

void ImageCtx::get_timer_instance(CephContext *cct, SafeTimer **timer,
Expand Down
4 changes: 3 additions & 1 deletion src/librbd/ImageCtx.h
View file
Open in desktop
Expand Up @@ -317,7 +317,9 @@ namespace librbd {
journal::Policy *get_journal_policy() const;
void set_journal_policy(journal::Policy *policy);

static ThreadPool *get_thread_pool_instance(CephContext *cct);
static void get_thread_pool_instance(CephContext *cct,
ThreadPool **thread_pool,
ContextWQ **op_work_queue);
Copy link
Contributor

Choose a reason for hiding this comment

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

@dillaman Looking at get_thread_pool_instance usage, why not crate a function that just returns work queue?

Copy link
Author

Choose a reason for hiding this comment

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

Mainly because I had to combine the ContextWQ into ThreadPoolSingleton to be absolutely sure about construction/destruction ordering; wanted to keep the ThreadPoolSingleton private; and didn't want to have two functions that could potentially initialize the singleton.

static void get_timer_instance(CephContext *cct, SafeTimer **timer,
Mutex **timer_lock);
};
Expand Down
9 changes: 7 additions & 2 deletions src/librbd/ImageState.cc
View file
Open in desktop
Expand Up @@ -248,7 +248,12 @@ template <typename I>
int ImageState<I>::open(bool skip_open_parent) {
C_SaferCond ctx;
open(skip_open_parent, &ctx);
return ctx.wait();

int r = ctx.wait();
if (r < 0) {
delete m_image_ctx;
}
return r;
}

template <typename I>
Expand Down Expand Up @@ -556,7 +561,7 @@ void ImageState<I>::complete_action_unlock(State next_state, int r) {
ctx->complete(r);
}

if (next_state != STATE_CLOSED) {
if (next_state != STATE_UNINITIALIZED && next_state != STATE_CLOSED) {
m_lock.Lock();
if (!is_transition_state() && !m_actions_contexts.empty()) {
execute_next_action_unlock();
Expand Down
28 changes: 12 additions & 16 deletions src/librbd/Journal.cc
View file
Open in desktop
Expand Up @@ -344,39 +344,35 @@ int Journal<I>::create(librados::IoCtx &io_ctx, const std::string &image_id,
CephContext *cct = reinterpret_cast<CephContext *>(io_ctx.cct());
ldout(cct, 5) << __func__ << ": image=" << image_id << dendl;

ThreadPool *thread_pool;
ContextWQ *op_work_queue;
ImageCtx::get_thread_pool_instance(cct, &thread_pool, &op_work_queue);

C_SaferCond cond;
journal::TagData tag_data(LOCAL_MIRROR_UUID);
ContextWQ op_work_queue("librbd::op_work_queue",
cct->_conf->rbd_op_thread_timeout,
ImageCtx::get_thread_pool_instance(cct));
journal::CreateRequest<I> *req = journal::CreateRequest<I>::create(
io_ctx, image_id, order, splay_width, object_pool, cls::journal::Tag::TAG_CLASS_NEW,
tag_data, IMAGE_CLIENT_ID, &op_work_queue, &cond);
tag_data, IMAGE_CLIENT_ID, op_work_queue, &cond);
req->send();

int r = cond.wait();
op_work_queue.drain();

return r;
return cond.wait();
}

template <typename I>
int Journal<I>::remove(librados::IoCtx &io_ctx, const std::string &image_id) {
CephContext *cct = reinterpret_cast<CephContext *>(io_ctx.cct());
ldout(cct, 5) << __func__ << ": image=" << image_id << dendl;

ThreadPool *thread_pool;
ContextWQ *op_work_queue;
ImageCtx::get_thread_pool_instance(cct, &thread_pool, &op_work_queue);

C_SaferCond cond;
ContextWQ op_work_queue("librbd::op_work_queue",
cct->_conf->rbd_op_thread_timeout,
ImageCtx::get_thread_pool_instance(cct));
journal::RemoveRequest<I> *req = journal::RemoveRequest<I>::create(
io_ctx, image_id, IMAGE_CLIENT_ID, &op_work_queue, &cond);
io_ctx, image_id, IMAGE_CLIENT_ID, op_work_queue, &cond);
req->send();

int r = cond.wait();
op_work_queue.drain();

return r;
return cond.wait();
}

template <typename I>
Expand Down