Skip to content

os/bluestore: fix btree allocator#52485

Merged
ljflores merged 1 commit intoceph:mainfrom
ifed01:wip-ifed-fix-btree-allocator
Sep 8, 2023
Merged

os/bluestore: fix btree allocator#52485
ljflores merged 1 commit intoceph:mainfrom
ifed01:wip-ifed-fix-btree-allocator

Conversation

@ifed01
Copy link
Contributor

@ifed01 ifed01 commented Jul 17, 2023

Fixes: https://tracker.ceph.com/issues/61949

Signed-off-by: Igor Fedotov igor.fedotov@croit.io

Contribution Guidelines

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

@ifed01 ifed01 requested a review from a team as a code owner July 17, 2023 13:21
@ifed01 ifed01 requested a review from aclamk July 17, 2023 13:21
@ifed01 ifed01 requested a review from tchaikov July 17, 2023 13:22
aclamk
aclamk approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@aclamk aclamk left a comment

Choose a reason for hiding this comment

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

Good fix.
We should schedule iterator invalidation review one day.

src/os/bluestore/BtreeAllocator.cc
range_tree.emplace_hint(rs, seg_after.start, seg_after.end);
range_size_tree.emplace(seg_after);
// shink the left seg in offset tree
rs->second = start;
Copy link
Contributor

@aclamk aclamk Jul 17, 2023

Choose a reason for hiding this comment

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

I think we should create a comment, something like:
// btree_map.emplace_hint invalidates rs.
otherwise there is a cryptic code move.

Copy link
Contributor

@aclamk aclamk Jul 17, 2023

Choose a reason for hiding this comment

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

I just tested that the same happens with btree_map.emplace() = iterator is invalidated.
Maybe we should review the code again, to check for it?

Copy link
Contributor Author

@ifed01 ifed01 Jul 18, 2023

Choose a reason for hiding this comment

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

I think we should create a comment, something like: // btree_map.emplace_hint invalidates rs. otherwise there is a cryptic code move.

done

Copy link
Contributor Author

@ifed01 ifed01 Jul 18, 2023

Choose a reason for hiding this comment

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

I just tested that the same happens with btree_map.emplace() = iterator is invalidated. Maybe we should review the code again, to check for it?

I did a brief check - looks like we mostly safe, the only suspicious location is _try_remove_from_tree() function but it's not used for now since it's intended for HybridAllocator wrapping.

@ifed01 ifed01 force-pushed the wip-ifed-fix-btree-allocator branch from 3447e4a to 1978bfd Compare July 18, 2023 12:43
@ifed01
os/bluestore: fix btree allocator
c4c3e34 
Fixes: https://tracker.ceph.com/issues/61949

Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
@ifed01 ifed01 force-pushed the wip-ifed-fix-btree-allocator branch from 1978bfd to c4c3e34 Compare July 18, 2023 12:54
tchaikov
tchaikov approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

thank you for fixing this issue! didn't get a chance to look into it.

src/os/bluestore/BtreeAllocator.cc
range_size_tree.emplace(seg_after);
// shink the left seg in offset tree
// this should be done before calling any emplace/emplace_hint
// on range_tree since they invalidate rs iterator.
Copy link
Contributor

@tchaikov tchaikov Jul 18, 2023

Choose a reason for hiding this comment

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

ahh, thank you for fixing this!

@tchaikov
Copy link
Contributor

tchaikov commented Jul 19, 2023 

INFO: pip is looking at multiple versions of behave to determine which version is compatible with other requirements. This could take a while.
ERROR: Could not find a version that satisfies the requirement parse>=1.8.2 (from behave) (from versions: none)
ERROR: No matching distribution found for parse>=1.8.2

        Start 272: run-tox-jsonnet-lint
Failed test dependencies: setup-venv-for-jsonnet-lint

retriggering.

@tchaikov
Copy link
Contributor

tchaikov commented Jul 19, 2023

jenkins test make check

3 similar comments
@ifed01
Copy link
Contributor Author

ifed01 commented Jul 19, 2023

jenkins test make check

@ifed01
Copy link
Contributor Author

ifed01 commented Jul 20, 2023

jenkins test make check

@ifed01
Copy link
Contributor Author

ifed01 commented Jul 21, 2023

jenkins test make check

@ljflores
Copy link
Member

ljflores commented Sep 8, 2023

Rados approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrellocomcJxeRJYse1822-wip-yuri4-testing-2023-08-10-1739

@ljflores ljflores merged commit 9c671cd into ceph:main Sep 8, 2023
@ifed01 ifed01 deleted the wip-ifed-fix-btree-allocator branch September 9, 2023 02:57
@k0ste k0ste mentioned this pull request Aug 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tchaikov tchaikov tchaikov approved these changes

@aclamk aclamk aclamk approved these changes

Assignees

No one assigned

Labels

bluestore bug-fix core needs-qa tests wip-yuri4-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ifed01 @tchaikov @ljflores @aclamk