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: OpSequencer: reduce kv_submitted_waiters if _is_all_kv_submitted() return true. #18622

Merged
merged 2 commits into from Nov 15, 2017
Merged

os/bluestore: OpSequencer: reduce kv_submitted_waiters if _is_all_kv_submitted() return true. #18622

merged 2 commits into from Nov 15, 2017

Conversation

majianpeng
Copy link
Member

No description provided.

@majianpeng
os/bluestore: reduce kv_submitted_waiters if _is_all_kv_submitted() r…
a251d67 
…eturn true.

Signed-off-by: Jianpeng Ma <jianpeng.ma@intel.com>
ifed01
ifed01 reviewed Oct 30, 2017
View reviewed changes
src/os/bluestore/BlueStore.h Outdated
@@ -1741,6 +1741,7 @@ class BlueStore : public ObjectStore,
// sure those threads see waiters and signal qcond.
++kv_submitted_waiters;
if (_is_all_kv_submitted()) {
--kv_submitted_waiters;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we can just do ++kv_submitted_waiters in else clause...

Copy link
Member Author

Choose a reason for hiding this comment

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

But the comments: // set flag before the check because the condition
// may become true outside qlock, and we need to make
// sure those threads see waiters and signal qcond.
++kv_submitted_waiters;
I think move in else condition is wrong.

ifed01
ifed01 reviewed Oct 30, 2017
View reviewed changes
src/os/bluestore/BlueStore.h Outdated
@@ -1740,7 +1738,7 @@ class BlueStore : public ObjectStore,
// may become true outside qlock, and we need to make
// sure those threads see waiters and signal qcond.
++kv_submitted_waiters;
if (_is_all_kv_submitted()) {
if (!q.empty() && _is_all_kv_submitted()) {
Copy link
Contributor

@ifed01 ifed01 Oct 30, 2017 •
edited

Choose a reason for hiding this comment

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

what's about q.empty() == true case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Move q.empty() from _is_all_kv_submitted to this. Most call _is_all_kv_submitted is the situation q.empyt() == false. So i move code . Because flush() should first check q whether empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

Before your changes q.empty() == true caused return here. Now it doesn't. Hence the new behavior isn't identical. Is that expected? I suppose proper condition should be (q.empty() || is_all_kv_submitted())

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. you are right. Thanks!

@majianpeng
Copy link
Member Author

@ifed01, update. please review.

@majianpeng
os/bluestore: remove "q.empyt()" check in _is_all_kv_submitted.
aedb6a1 
Most call _is_all_kv_submitted under the situation: q.empty() is false.

Signed-off-by: Jianpeng Ma <jianpeng.ma@intel.com>
@liewegas liewegas changed the title Bluestore OpSequencer : reduce kv_submitted_waiters if _is_all_kv_submitted() return true. os/bluestore: OpSequencer: reduce kv_submitted_waiters if _is_all_kv_submitted() return true. Nov 1, 2017
@tchaikov
Copy link
Contributor

tchaikov commented Nov 9, 2017

http://pulpito.ceph.com/kchai-2017-11-09_02:23:27-rados-wip-kefu-testing-2017-11-08-1620-distro-basic-smithi/

while master is green.

dropping from my test batch for now.

@majianpeng
Copy link
Member Author

@tchaikov . Sorry i don't understand what's mean" while master is green". It mean my PR has problem. Or?

@tchaikov
Copy link
Contributor

tchaikov commented Nov 9, 2017

sorry, i forgot to post the test result on master: http://pulpito.ceph.com/kchai-2017-11-09_02:25:40-rados-master-distro-basic-smithi/

i suspect that your PR is causing this issue.

@majianpeng
Copy link
Member Author

From the stack info, it's about filestore rather than bluestore.

@tchaikov
Copy link
Contributor

tchaikov commented Nov 9, 2017

@majianpeng mind taking a look at /a/kchai-2017-11-09_02:58:01-rados-wip-kefu-testing-2017-11-08-1620-distro-basic-mira/1827920?

@majianpeng
Copy link
Member Author

@tchaikov . From http://qa-proxy.ceph.com/teuthology/kchai-2017-11-09_02:58:01-rados-wip-kefu-testing-2017-11-08-1620-distro-basic-mira/1827920/teuthology.log:
2017-11-09T03:06:50.875 INFO:teuthology.orchestra.run.mira084.stderr:/build/ceph-13.0.0-2969-gb890ff9/src/os/filestore/FileStore.h: In function 'virtual FileStore::OpSequencer::~OpSequencer()' thread 7f2e48fbc080 time 2017-11-09 03:06:50.871191
2017-11-09T03:06:50.875 INFO:teuthology.orchestra.run.mira084.stderr:/build/ceph-13.0.0-2969-gb890ff9/src/os/filestore/FileStore.h: 358: FAILED assert(q.empty())
2017-11-09T03:06:50.877 INFO:teuthology.orchestra.run.mira084.stderr: ceph version 13.0.0-2969-gb890ff9 (b890ff9471dcd7e8b467e8afa8a522e593ec1c9d) mimic (dev)
2017-11-09T03:06:50.877 INFO:teuthology.orchestra.run.mira084.stderr: 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x102) [0x7f2e3f8be752]
2017-11-09T03:06:50.877 INFO:teuthology.orchestra.run.mira084.stderr: 2: (FileStore::OpSequencer::~OpSequencer()+0x11a) [0x5591be888c5a]
2017-11-09T03:06:50.877 INFO:teuthology.orchestra.run.mira084.stderr: 3: (RefCountedObject::put() const+0xfc) [0x5591be80897c]

Or am i miss something?

@tchaikov
Copy link
Contributor

tchaikov commented Nov 9, 2017

@majianpeng /a//kchai-2017-11-09_02:58:01-rados-wip-kefu-testing-2017-11-08-1620-distro-basic-mira/1827917/remote/mira037/log/valgrind/osd.0.log.gz

@majianpeng
Copy link
Member Author

@tchaikov . I didn't find /a//kchai-2017-11-09_02:58:01-rados-wip-kefu-testing-2017-11-08-1620-distro-basic-mira/1827917/remote/mira037/log/valgrind/osd.0.log.gz. Could you give me a link? Thanks

@tchaikov
Copy link
Contributor

tchaikov commented Nov 9, 2017

@majianpeng http://qa-proxy.ceph.com/teuthology/kchai-2017-11-09_02:58:01-rados-wip-kefu-testing-2017-11-08-1620-distro-basic-mira/1827917/remote/mira037/log/valgrind/

@tchaikov
Copy link
Contributor

http://pulpito.ceph.com/kchai-2017-11-12_17:42:28-rados-wip-kefu-testing-2017-11-10-1154-distro-basic-mira/1841840/

@tchaikov
Copy link
Contributor

http://pulpito.ceph.com/kchai-2017-11-14_08:08:02-rados-wip-kefu-testing-2017-11-14-1048-distro-basic-mira/

@tchaikov tchaikov merged commit 267b97e into ceph:master Nov 15, 2017
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-kefu-testing
Projects
None yet
Milestone
No milestone
3 participants
@majianpeng @tchaikov @ifed01