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

rgw: use vector for librados handles #9295

Merged
merged 4 commits into from
Jun 2, 2016
Merged

rgw: use vector for librados handles #9295

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
02fb39d
rgw: use vector for librados handles
cbodley May 3, 2016
72d5a48
rgw: remove unneeded RGWRados::num_rados_handles
cbodley May 24, 2016
1adff94
rgw: dont need atomic RGWRados::next_rados_handle
cbodley May 24, 2016
53a7f1a
rgw: don't leak RGWRados::cr_registry on error
cbodley May 24, 2016
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
72 changes: 22 additions & 50 deletions src/rgw/rgw_rados.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2549,7 +2549,7 @@ int RGWRados::unwatch(uint64_t watch_handle)
ldout(cct, 0) << "ERROR: rados->unwatch2() returned r=" << r << dendl;
return r;
}
r = rados[0]->watch_flush();
r = rados[0].watch_flush();
if (r < 0) {
ldout(cct, 0) << "ERROR: rados->watch_flush() returned r=" << r << dendl;
return r;
Expand Down Expand Up @@ -3199,58 +3199,32 @@ void RGWRados::finalize()
int RGWRados::init_rados()
{
int ret = 0;
auto handles = std::vector<librados::Rados>{cct->_conf->rgw_num_rados_handles};
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this idea very much!


num_rados_handles = cct->_conf->rgw_num_rados_handles;

rados = new librados::Rados *[num_rados_handles];
if (!rados) {
ret = -ENOMEM;
return ret;
}

for (uint32_t i=0; i < num_rados_handles; i++) {

rados[i] = new Rados();
if (!rados[i]) {
ret = -ENOMEM;
goto fail;
}

ret = rados[i]->init_with_context(cct);
for (auto& r : handles) {
ret = r.init_with_context(cct);
if (ret < 0) {
goto fail;
return ret;
}

ret = rados[i]->connect();
ret = r.connect();
if (ret < 0) {
goto fail;
return ret;
}
}

cr_registry = new RGWCoroutinesManagerRegistry(cct);
ret = cr_registry->hook_to_admin_command("cr dump");
auto crs = std::unique_ptr<RGWCoroutinesManagerRegistry>{
new RGWCoroutinesManagerRegistry(cct)};
ret = crs->hook_to_admin_command("cr dump");
if (ret < 0) {
goto fail;
return ret;
}

meta_mgr = new RGWMetadataManager(cct, this);
data_log = new RGWDataChangesLog(cct, this);
cr_registry = crs.release();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not now but is there a chance to convert cr_registry into a C++11's smart pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed with all members of RGWRados, althought it has very specific requirements for the order of construction and destruction. so we wouldn't want to rely on ~RGWRados to free them automatically


return ret;

fail:
for (uint32_t i=0; i < num_rados_handles; i++) {
if (rados[i]) {
delete rados[i];
rados[i] = NULL;
}
}
num_rados_handles = 0;
if (rados) {
delete[] rados;
rados = NULL;
}

std::swap(handles, rados);
return ret;
}

Expand Down Expand Up @@ -3993,7 +3967,7 @@ int RGWRados::init_watch()
{
const char *control_pool = get_zone_params().control_pool.name.c_str();

librados::Rados *rad = rados[0];
librados::Rados *rad = &rados[0];
int r = rad->ioctx_create(control_pool, control_pool_ctx);

if (r == -ENOENT) {
Expand Down Expand Up @@ -12109,28 +12083,26 @@ void RGWStoreManager::close_storage(RGWRados *store)

librados::Rados* RGWRados::get_rados_handle()
{
if (num_rados_handles == 1) {
return rados[0];
if (rados.size() == 1) {
return &rados[0];
} else {
handle_lock.get_read();
pthread_t id = pthread_self();
std::map<pthread_t, int>:: iterator it = rados_map.find(id);

if (it != rados_map.end()) {
handle_lock.put_read();
return rados[it->second];
return &rados[it->second];
} else {
handle_lock.put_read();
handle_lock.get_write();
uint32_t handle = next_rados_handle.read();
if (handle == num_rados_handles) {
next_rados_handle.set(0);
handle = 0;
}
const uint32_t handle = next_rados_handle;
rados_map[id] = handle;
next_rados_handle.inc();
if (++next_rados_handle == rados.size()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is holding the read-write on handle_lock enough to protect next_rados_handle? In the previous version it was additionally atomic_t. Are we sure that it isn't touched by someone else who doesn't actually take the lock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function is the only place that next_rados_handle is referenced, it doesn't need to be atomic

Copy link
Contributor

Choose a reason for hiding this comment

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

Great. Yet another one optimization here. :-)

Copy link
Member

Choose a reason for hiding this comment

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

@cbodley can't this function be called concurrently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

next_rados_handle is only accessed under an exclusive lock on handle_lock

next_rados_handle = 0;
}
handle_lock.put_write();
return rados[handle];
return &rados[handle];
}
}
}
Expand Down
21 changes: 5 additions & 16 deletions src/rgw/rgw_rados.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1800,9 +1800,8 @@ class RGWRados
protected:
CephContext *cct;

librados::Rados **rados;
atomic_t next_rados_handle;
uint32_t num_rados_handles;
std::vector<librados::Rados> rados;
uint32_t next_rados_handle;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is rather stylistic but, as next_rados_handle is used as an index, it would be nice to see size_t here.

RWLock handle_lock;
std::map<pthread_t, int> rados_map;

Expand Down Expand Up @@ -1841,8 +1840,8 @@ class RGWRados
bucket_id_lock("rados_bucket_id"),
bucket_index_max_shards(0),
max_bucket_id(0), cct(NULL),
rados(NULL), next_rados_handle(0),
num_rados_handles(0), handle_lock("rados_handle_lock"),
next_rados_handle(0),
handle_lock("rados_handle_lock"),
binfo_cache(NULL),
pools_initialized(false),
quota_handler(NULL),
Expand Down Expand Up @@ -1956,17 +1955,7 @@ class RGWRados

RGWDataChangesLog *data_log;

virtual ~RGWRados() {
for (uint32_t i=0; i < num_rados_handles; i++) {
if (rados[i]) {
rados[i]->shutdown();
delete rados[i];
}
}
if (rados) {
delete[] rados;
}
}
virtual ~RGWRados() = default;
Copy link
Contributor

Choose a reason for hiding this comment

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

We aren't calling librados::Rados::shutdown() anymore. Is that OK? Does librados::Rados::~Rados() do that for us?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks we're leaking cr_registry but this isn't new (no regression here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does librados::Rados::~Rados() do that for us?

librados::Rados::~Rados()
{
  shutdown();
}

It looks we're leaking cr_registry but this isn't new (no regression here).

correct, cr_registry is only freed in RGWRados::finalize(). before this patch, it was leaked if the call to cr_registry->hook_to_admin_command("cr dump"); failed in RGWRados::init_rados(). but if init_rados() succeeds, it's fair to expect finalize() to be called for cleanup

Copy link
Member

Choose a reason for hiding this comment

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

@cbodley this sounds familiar, I might have fixed it somewhere else


int get_required_alignment(rgw_bucket& bucket, uint64_t *alignment);
int get_max_chunk_size(rgw_bucket& bucket, uint64_t *max_chunk_size);
Expand Down