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

os/bluestore: eliminate some excessive stuff #14675

Merged
merged 3 commits into from Apr 28, 2017

Conversation

Projects
None yet
2 participants
@ifed01
Contributor

ifed01 commented Apr 20, 2017

Signed-off-by: Igor Fedotov ifedotov@mirantis.com

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
{
std::lock_guard<std::mutex> l(osr->qlock);
txc->state = TransContext::STATE_DONE;
empty = _osr_reap_done(osr.get(), &c);

This comment has been minimized.

@liewegas

liewegas Apr 20, 2017

Member

Since tehre is only one osr_reap_done caller we can just move it inline too and avoid the awkward * argument. (I had to do somethign similar in wip-bluestore-rtc so i think we're headed in this direction anyway?)

@liewegas

liewegas Apr 20, 2017

Member

Since tehre is only one osr_reap_done caller we can just move it inline too and avoid the awkward * argument. (I had to do somethign similar in wip-bluestore-rtc so i think we're headed in this direction anyway?)

This comment has been minimized.

@ifed01

ifed01 Apr 20, 2017

Contributor

Fixed

@ifed01

ifed01 Apr 20, 2017

Contributor

Fixed

Igor Fedotov
os/bluestore: get rid off duplicate lock acquisition at txc_finish()
Signed-off-by: Igor Fedotov <ifedotov@mirantis.com>

@ifed01 ifed01 changed the title from os/bluestore: get rid off duplicate lock acquisition at txc_finish() to os/bluestore: eliminate some excessive stuff Apr 20, 2017

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
std::unique_lock<std::mutex> l(flush_lock);
ldout(c->store->cct, 20) << flush_txns << dendl;
while (!flush_txns.empty())
while (!flushing_count.load()) {

This comment has been minimized.

@liewegas

liewegas Apr 20, 2017

Member

shoudl be while (flushing_count.load()) right?

@liewegas

liewegas Apr 20, 2017

Member

shoudl be while (flushing_count.load()) right?

This comment has been minimized.

@ifed01

ifed01 Apr 20, 2017

Contributor

yep, thanks!

@ifed01

ifed01 Apr 20, 2017

Contributor

yep, thanks!

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
o->flush_txns.erase(txc);
o->flushing_count--;
if (o->flush_txns.empty()) {
if (o->flushing_count.fetch_sub(1) == 1) {

This comment has been minimized.

@liewegas

liewegas Apr 20, 2017

Member

how about

if (--o->flushing_count == 0) {

?

@liewegas

liewegas Apr 20, 2017

Member

how about

if (--o->flushing_count == 0) {

?

@ifed01

This comment has been minimized.

Show comment
Hide comment
@ifed01

ifed01 Apr 20, 2017

Contributor

@liewegas - fixed

Contributor

ifed01 commented Apr 20, 2017

@liewegas - fixed

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Apr 21, 2017

Member
2017-04-21T21:44:29.571 INFO:tasks.ceph.osd.1.smithi053.stderr: ceph version 12.0.1-1346-g4302f127 (4302f127d23bbba458c8480cbad25622cab39fef)
2017-04-21T21:44:29.571 INFO:tasks.ceph.osd.1.smithi053.stderr: 1: (()+0xbc7e47) [0x7fcac98b2e47]
2017-04-21T21:44:29.571 INFO:tasks.ceph.osd.1.smithi053.stderr: 2: (()+0x10330) [0x7fcac71f2330]
2017-04-21T21:44:29.571 INFO:tasks.ceph.osd.1.smithi053.stderr: 3: (pthread_cond_wait()+0xc4) [0x7fcac71ee404]
2017-04-21T21:44:29.571 INFO:tasks.ceph.osd.1.smithi053.stderr: 4: (std::condition_variable::wait(std::unique_lock&)+0xc) [0x7fcac6b6f4dc]
2017-04-21T21:44:29.571 INFO:tasks.ceph.osd.1.smithi053.stderr: 5: (IOContext::aio_wait()+0xa0) [0x7fcac9861c50]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 6: (BlueStore::_do_read(BlueStore::Collection*, boost::intrusive_ptr, unsigned long, unsigned long, ceph::buffer::list&, unsigned int)+0xd20) [0x7fcac97bf6f0]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 7: (BlueStore::read(boost::intrusive_ptr&, ghobject_t const&, unsigned long, unsigned long, ceph::buffer::list&, unsigned int, bool)+0x5ca) [0x7fcac97c18fa]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 8: (ReplicatedBackend::build_push_op(ObjectRecoveryInfo const&, ObjectRecoveryProgress const&, ObjectRecoveryProgress*, PushOp*, object_stat_sum_t*, bool)+0x24c) [0x7fcac96596bc]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 9: (ReplicatedBackend::handle_push_reply(pg_shard_t, PushReplyOp const&, PushOp*)+0x275) [0x7fcac9660e65]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 10: (ReplicatedBackend::do_push_reply(boost::intrusive_ptr)+0xe1) [0x7fcac9662451]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 11: (ReplicatedBackend::handle_message(boost::intrusive_ptr)+0x244) [0x7fcac9662834]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 12: (PrimaryLogPG::do_request(boost::intrusive_ptr&, ThreadPool::TPHandle&)+0x445) [0x7fcac9528975]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 13: (OSD::dequeue_op(boost::intrusive_ptr, boost::intrusive_ptr, ThreadPool::TPHandle&)+0x212) [0x7fcac93d8ee2]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: 14: (PGQueueable::RunVis::operator()(boost::intrusive_ptr const&)+0x47) [0x7fcac93d9317]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: 15: (OSD::ShardedOpWQ::_process(unsigned int, ceph::heartbeat_handle_d*)+0xff5) [0x7fcac9402485]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: 16: (ShardedThreadPool::shardedthreadpool_worker(unsigned int)+0x955) [0x7fcac99137f5]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: 17: (ShardedThreadPool::WorkThreadSharded::entry()+0x10) [0x7fcac9915950]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: 18: (()+0x8184) [0x7fcac71ea184]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: 19: (clone()+0x6d) [0x7fcac62da37d]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: NOTE: a copy of the executable, or `objdump -rdS ` is needed to interpret this.

/a/sage-2017-04-21_20:22:31-rados-wip-sage-testing2---basic-smithi/1053796

not certain this pr is to blame but i haven't seen this one before

Member

liewegas commented Apr 21, 2017

2017-04-21T21:44:29.571 INFO:tasks.ceph.osd.1.smithi053.stderr: ceph version 12.0.1-1346-g4302f127 (4302f127d23bbba458c8480cbad25622cab39fef)
2017-04-21T21:44:29.571 INFO:tasks.ceph.osd.1.smithi053.stderr: 1: (()+0xbc7e47) [0x7fcac98b2e47]
2017-04-21T21:44:29.571 INFO:tasks.ceph.osd.1.smithi053.stderr: 2: (()+0x10330) [0x7fcac71f2330]
2017-04-21T21:44:29.571 INFO:tasks.ceph.osd.1.smithi053.stderr: 3: (pthread_cond_wait()+0xc4) [0x7fcac71ee404]
2017-04-21T21:44:29.571 INFO:tasks.ceph.osd.1.smithi053.stderr: 4: (std::condition_variable::wait(std::unique_lock&)+0xc) [0x7fcac6b6f4dc]
2017-04-21T21:44:29.571 INFO:tasks.ceph.osd.1.smithi053.stderr: 5: (IOContext::aio_wait()+0xa0) [0x7fcac9861c50]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 6: (BlueStore::_do_read(BlueStore::Collection*, boost::intrusive_ptr, unsigned long, unsigned long, ceph::buffer::list&, unsigned int)+0xd20) [0x7fcac97bf6f0]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 7: (BlueStore::read(boost::intrusive_ptr&, ghobject_t const&, unsigned long, unsigned long, ceph::buffer::list&, unsigned int, bool)+0x5ca) [0x7fcac97c18fa]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 8: (ReplicatedBackend::build_push_op(ObjectRecoveryInfo const&, ObjectRecoveryProgress const&, ObjectRecoveryProgress*, PushOp*, object_stat_sum_t*, bool)+0x24c) [0x7fcac96596bc]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 9: (ReplicatedBackend::handle_push_reply(pg_shard_t, PushReplyOp const&, PushOp*)+0x275) [0x7fcac9660e65]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 10: (ReplicatedBackend::do_push_reply(boost::intrusive_ptr)+0xe1) [0x7fcac9662451]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 11: (ReplicatedBackend::handle_message(boost::intrusive_ptr)+0x244) [0x7fcac9662834]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 12: (PrimaryLogPG::do_request(boost::intrusive_ptr&, ThreadPool::TPHandle&)+0x445) [0x7fcac9528975]
2017-04-21T21:44:29.572 INFO:tasks.ceph.osd.1.smithi053.stderr: 13: (OSD::dequeue_op(boost::intrusive_ptr, boost::intrusive_ptr, ThreadPool::TPHandle&)+0x212) [0x7fcac93d8ee2]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: 14: (PGQueueable::RunVis::operator()(boost::intrusive_ptr const&)+0x47) [0x7fcac93d9317]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: 15: (OSD::ShardedOpWQ::_process(unsigned int, ceph::heartbeat_handle_d*)+0xff5) [0x7fcac9402485]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: 16: (ShardedThreadPool::shardedthreadpool_worker(unsigned int)+0x955) [0x7fcac99137f5]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: 17: (ShardedThreadPool::WorkThreadSharded::entry()+0x10) [0x7fcac9915950]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: 18: (()+0x8184) [0x7fcac71ea184]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: 19: (clone()+0x6d) [0x7fcac62da37d]
2017-04-21T21:44:29.573 INFO:tasks.ceph.osd.1.smithi053.stderr: NOTE: a copy of the executable, or `objdump -rdS ` is needed to interpret this.

/a/sage-2017-04-21_20:22:31-rados-wip-sage-testing2---basic-smithi/1053796

not certain this pr is to blame but i haven't seen this one before

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Apr 25, 2017

Member

that was a trusty box; maybe that's it.

Member

liewegas commented Apr 25, 2017

that was a trusty box; maybe that's it.

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
@@ -2853,11 +2853,12 @@ BlueStore::BlobRef BlueStore::ExtentMap::split_blob(
void BlueStore::Onode::flush()
{
if (flushing_count) {
if (flushing_count.load()) {
ldout(c->store->cct, 20) << flushing_count << dendl;

This comment has been minimized.

@liewegas

liewegas Apr 27, 2017

Member

func << " flushing_count " << flushing_cout << dendl;

@liewegas

liewegas Apr 27, 2017

Member

func << " flushing_count " << flushing_cout << dendl;

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
osr->qcond.notify_all();
}
if (osr->q.empty()) {
dout(20) << __func__ << " osr " << osr << " q now empty" << dendl;
empty = true;
}
}
while(!releasing_txc.empty()) {

This comment has been minimized.

@liewegas

liewegas Apr 27, 2017

Member

while (

@liewegas

liewegas Apr 27, 2017

Member

while (

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Apr 27, 2017

Member

two nits, but otherwise yes, looks good! running through qa now.

Member

liewegas commented Apr 27, 2017

two nits, but otherwise yes, looks good! running through qa now.

Igor Fedotov added some commits Apr 20, 2017

Igor Fedotov
os/bluestore: get rid off Onode::flush_txn set.
It looks like flushing_count counter is enough.

Signed-off-by: Igor Fedotov <ifedotov@mirantis.com>
Igor Fedotov
os/bluestore: move TransContext finalization out of osr_lock to avoid…
… potential contention.

Signed-off-by: Igor Fedotov <ifedotov@mirantis.com>
@ifed01

This comment has been minimized.

Show comment
Hide comment
@ifed01

ifed01 Apr 28, 2017

Contributor

@liewegas - fixed

Contributor

ifed01 commented Apr 28, 2017

@liewegas - fixed

@liewegas liewegas merged commit 2944c48 into ceph:master Apr 28, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@ifed01 ifed01 deleted the ifed01:wip-bluestore-nolock branch May 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment