Conversation
|
jenkins test api |
1 similar comment
|
jenkins test api |
ifed01
left a comment
There was a problem hiding this comment.
I like the patch. It's nice to have it as a code cleanup even if it doesn't bring much improvement to DB lookup performance.
The patch breaks the following test cases from ceph_test_objectstore though:
[ FAILED ] ObjectStore/StoreTest.List_0xfffffff_Hash_Test_in_meta/2, where GetParam() = "bluestore"
[ FAILED ] ObjectStore/StoreTest.List_0xfffffff_Hash_Test_in_PG/2, where GetParam() = "bluestore"
[ FAILED ] ObjectStore/StoreTest.HashCollisionSorting/2, where GetParam() = "bluestore"
src/os/bluestore/BlueStore.cc
Outdated
| dout(30) << __func__ << " pend " << pend << dendl; | ||
| continue; | ||
| while (true) { | ||
| if (!it->valid()) { |
There was a problem hiding this comment.
may be replace with
while(it->valid()) {
?
There was a problem hiding this comment.
thanks, will update that!
cd20779 to
7b0404b
Compare
7b0404b to
1338a27
Compare
|
@ifed01 I updated to fix those tests. Do they not run as part of the GH checks? |
src/os/bluestore/BlueStore.cc
Outdated
| it->lower_bound(start); | ||
| } else { | ||
| pend = temp ? coll_range_temp_end : end; | ||
| it->upper_bound(cur_range_start); |
There was a problem hiding this comment.
Shouldn't this be it->lower_bound(cur_range_start) instead?
Overwise we miss the first entry when start <= cur_range_start, don't we?
Looks like the original implementation has got a bug here as well,,,
There was a problem hiding this comment.
It doesn't look to me like an actual onode key could == cur_range_start because get_coll_range is only returning a key prefix and an actual key would necessarily have additional bytes for the namespace, oid, etc. That said, the same argument also supports the fact that it isn't meaningful to use upper_bound vs. lower_bound in this case, so I think I should simplify by removing the if/else and always doing:
it->lower_bound(low);
There was a problem hiding this comment.
Let's have this change and tag the PR with "needs_qa" afterwards.
src/os/bluestore/BlueStore.cc
Outdated
| it->lower_bound(start); | ||
| } else { | ||
| pend = temp ? coll_range_temp_end : end; | ||
| it->upper_bound(cur_range_start); |
1338a27 to
54a8cae
Compare
aclamk
left a comment
There was a problem hiding this comment.
This is good improvement, but needs fixes.
54a8cae to
80cde1a
Compare
|
jenkins test make check |
|
Hey @cfsnyder, we found a failure related to this PR in teuthology: /a/yuriw-2023-02-13_22:35:30-rados-wip-yuri11-testing-2023-02-06-1424-distro-default-smithi/7173572 The failure can be reproduced locally with: |
|
Thanks @ljflores , I'll look into this |
|
@cfsnyder ping? |
|
Thanks for the ping @aclamk. I do intend to get back to this and wrap it up, I've just been preoccupied with more pressing matters lately. I'll do my best to fit it in in the next week or so. |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
jenkins test make check |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution! |
|
We still want this likely! |
|
@cfsnyder looks like the blocker was that it failed a teuthology test. Can you take another look and/or let us know if you need another QA run? |
80cde1a to
7054fb9
Compare
|
jenkins test make check |
| dout(20) << __func__ << " reached max " << max << dendl; | ||
| *pnext = it->oid(); | ||
| return 0; | ||
| ls->push_back(it->oid()); |
There was a problem hiding this comment.
It is good that most of dout were removed.
But I think the one like
dout(20) << __func__ << " oid " << it->oid() << dendl;
would be useful.
There was a problem hiding this comment.
agreed, just added that log line. thanks for the review!
aclamk
left a comment
There was a problem hiding this comment.
Great!
But please add oid dout.
7054fb9 to
b6cb356
Compare
…list() When Bluestore::_collection_list() is called during PG removal, rocksdb often has to iterate over many rocksdb deletion tombstones from recenently deleted onode keys. This change bounds the rocksdb iteration when possible, to avoid high latencies caused by iteration over many contiguous deletion tombstones. Fixes: https://tracker.ceph.com/issues/58274 Signed-off-by: Cory Snyder <csnyder@iland.com>
b6cb356 to
a6bcb57
Compare
|
This PR is under test in https://tracker.ceph.com/issues/65560. |
When Bluestore::_collection_list() is called during PG removal, rocksdb often has to iterate over many deletion tombstones from recently deleted onode keys. This change bounds the rocksdb iteration when possible, to avoid high latencies caused by iteration over many contiguous deletion tombstones.
Fixes: https://tracker.ceph.com/issues/58274
Signed-off-by: Cory Snyder csnyder@iland.com
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windows