Skip to content

os/bluestore, rgw: Get rid of superfluous iterator seeking#60056

Closed
aclamk wants to merge 2 commits intoceph:mainfrom
aclamk:wip-aclamk-rgw-better-omap-iterator
Closed

os/bluestore, rgw: Get rid of superfluous iterator seeking#60056
aclamk wants to merge 2 commits intoceph:mainfrom
aclamk:wip-aclamk-rgw-better-omap-iterator

Conversation

@aclamk
Copy link
Contributor

@aclamk aclamk commented Sep 30, 2024 •
edited
Loading

Eliminates inefficiencies in using of omap iterators.

  1. BlueStore omap iterator implementation used to seek to first element
  2. CEPH_OSD_OP_OMAPGETVALS used to use both upper_bound and lower_bound in short succession.

Added assert guard rails if iterator obtained by ObjectStore::get_omap_iterator is used without initial seek_to_first/upper_bound/lower_bound.

This PR is related to #60000

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows
  • jenkins test rook e2e

@aclamk
os/osd: Get rid of superfluous upper_bound
c6f5fd6 
In PrimaryLogPG::do_osd_ops operation CEPH_OSD_OP_OMAPGETVALS
used to do both upper_bound and lower_bound in some cases.

Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>
@aclamk
os/bluestore: Cleanup on OmapIteratorImpl
68c3f4e 
Remove lower_bound that is applied when iterator is created.
Added assertions in case someone would create iterator but not
initialize position before using it.

Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>
@aclamk aclamk requested a review from a team as a code owner September 30, 2024 11:47
@aclamk aclamk marked this pull request as draft September 30, 2024 11:55
@athanatos
Copy link
Contributor

athanatos commented Oct 8, 2024

Do we actually need seeked? Looks like most of those check it->valid() -- would that suffice?

@athanatos athanatos mentioned this pull request Oct 8, 2024
Closed
14 tasks
@aclamk
Copy link
Contributor Author

aclamk commented Oct 8, 2024 •
edited
Loading

Do we actually need seeked? Looks like most of those check it->valid() -- would that suffice?

@athanatos
Using it->valid() instead of seeked will work except for OmapIteratorImpl::valid().
In fact, seeked is here because our interface was defined by implementation.
I haven't seen in our code base usage of OMAP iterator that is not doing one of seek_to_first / upper_bound / lower_bound before calling accessors: next / value / key / valid.

So maybe entire PR should be without seeked?

This was referenced Oct 15, 2024
Closed
Merged
@aclamk aclamk added the aclamk-testing-ceres bluestore testing label Oct 30, 2024
@aclamk
Copy link
Contributor Author

aclamk commented Oct 31, 2024

Testing results of this PR done using #60315.
Short: minimal improvement.

Run using command:
./bin/omap_bench --bluestore_block_path=/ceph-devices/block.adam.0 --gtest_filter=\*/0 --bluestore_throttle_trace_rate=1000000 --osd_memory_target=16G --log-to-stderr=false
Tested on sub tests 0 1 3 4. 2 and 5 were too large to complete.

Left column images are without the PR, right with new iterator modification (with the PR).
cre = iterator creation time
fi = seek to first (1st time after iterator creation)
fi_2 seek to first (2nd+ after iterator creation)
up = upper_bound (1st time after iterator creation)
up_2 upper_bound (2nd+ after iterator creation)
lo = lower_bound (1st time after iterator creation)
lo_2 lower_bound (2nd+ after iterator creation)

In reference code iterator creation (blue line) dominates execution time.
In new code, when there is no seek_to_first, iterator creation is fast.
Operations seek_to_first / upper_bound / lower_bound take time, but only first execution.
Any subsequent calls to them are relatively cheap.

1M1K50c+NI
1M1K500c+NI
10M100o 50c+NI
10M100o 500c+NI

@aclamk aclamk marked this pull request as ready for review October 31, 2024 11:07
@aclamk aclamk removed the aclamk-testing-ceres bluestore testing label Dec 17, 2024
@github-actions
Copy link

github-actions bot commented Jan 13, 2025

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@rzarzynski
Copy link
Contributor

rzarzynski commented Jan 13, 2025 •
edited
Loading

Closing as the far broader #60278 has been merged.

@rzarzynski rzarzynski closed this Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bluestore core needs-rebase performance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@aclamk @athanatos @rzarzynski @markhpc