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.

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: proper override rocksdb::WritableFile::Allocate #50469

Merged
merged 1 commit into from May 19, 2023

Conversation

ifed01
Copy link
Contributor

@ifed01 ifed01 commented Mar 10, 2023

Failing to do that makes BlueFS file frgmentation higher.

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

@aclamk
Copy link
Contributor

aclamk commented Mar 13, 2023

Do now we have to do the same for:
rocksdb::Status RangeSync(off_t offset, off_t nbytes) ?

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.

Nice!

@aclamk
Copy link
Contributor

aclamk commented Mar 13, 2023

@ifed01
I created https://tracker.ceph.com/issues/58966 to track this back.
Can you please update commit message?

@ifed01 ifed01 force-pushed the wip-ifed-fix-bluefs-prealloc branch from 3ec5f46 to bdc8fd2 Compare March 13, 2023 11:17
@github-actions github-actions bot added the core label Mar 13, 2023
@ifed01
Copy link
Contributor Author

ifed01 commented Mar 13, 2023

Do now we have to do the same for: rocksdb::Status RangeSync(off_t offset, off_t nbytes) ?

yep, done.

@ifed01
Copy link
Contributor Author

ifed01 commented Mar 13, 2023

@ifed01 I created https://tracker.ceph.com/issues/58966 to track this back. Can you please update commit message?

done

@baergj
Copy link

baergj commented Apr 25, 2023

FWIW, I played with this a bit in a staging environment and didn't see any measurable differences to latency or write amp.

@ifed01 ifed01 force-pushed the wip-ifed-fix-bluefs-prealloc branch from bdc8fd2 to a0f6b2b Compare April 25, 2023 16:56
Failing to do that makes BlueFS file frgmentation higher.

Fixes: https://tracker.ceph.com/issues/58966
Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
@ifed01 ifed01 force-pushed the wip-ifed-fix-bluefs-prealloc branch from a0f6b2b to 2d8b9e9 Compare April 25, 2023 16:58
@ifed01
Copy link
Contributor Author

ifed01 commented Apr 25, 2023

FWIW, I played with this a bit in a staging environment and didn't see any measurable differences to latency or write amp.

First of all this is definitely not a major performance improvement... It just potentially decreases sst files fragmentation. Which should be visible through ceph-bluestore-tool's bluefs-log-dump command output. And the following considerations might apply as well:

  1. Aged volumes under high load are more susceptible to cause sst fragmentation/expose negative effect.
  2. The patch slows down volumes' aging/fragmentation.
  3. DB on top of spinning drive is apparently the major winner

1 similar comment
@ifed01
Copy link
Contributor Author

ifed01 commented Apr 25, 2023

FWIW, I played with this a bit in a staging environment and didn't see any measurable differences to latency or write amp.

First of all this is definitely not a major performance improvement... It just potentially decreases sst files fragmentation. Which should be visible through ceph-bluestore-tool's bluefs-log-dump command output. And the following considerations might apply as well:

  1. Aged volumes under high load are more susceptible to cause sst fragmentation/expose negative effect.
  2. The patch slows down volumes' aging/fragmentation.
  3. DB on top of spinning drive is apparently the major winner

@baergj
Copy link

baergj commented Apr 25, 2023

Yeah, understood - I just wanted to confirm that it had effectively zero effect on an nvme test system. I figured that HDDs would be the big winners here.

@ifed01
Copy link
Contributor Author

ifed01 commented Apr 25, 2023

jenkins test make check

@ljflores
Copy link
Contributor

@yuriw yuriw merged commit 7859709 into ceph:main May 19, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants