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: do not select absent device in volume selector #43818

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

ifed01
Copy link
Contributor

@ifed01 ifed01 commented Nov 5, 2021

Fixes: https://tracker.ceph.com/issues/53139
Signed-off-by: Igor Fedotov igor.fedotov@croit.io

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

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

Fixes: ttps://tracker.ceph.com/issues/53139
Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
if (slow_size == 0) {
slow_size = db_size;
}
res.emplace_back(base + ".slow", slow_size);
Copy link
Contributor

Choose a reason for hiding this comment

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

@ifed01 Could you please remind me, why we must provide base + ".slow" to RocksDB ?
I mean - cant we just provide base ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally we provide db.slow vs. db for RocksDB to be able to indicate less frequently used data. Hence we're able to put them to larger but slower main device.
This works fine for 2+ device setups.
For single device setup we had that "dual-folder" provisioning in RocksDBBlueFSVolumeSelector from its introduction but it wasn't used since that selector isn't created for single device setup at all.
For OriginalVolumeSelector in #42992 I introduced dual folder provisioning for single device setup as well recently - to fix the case when files originally put into db.slow are still referred via this subdir after DB volume migration/removal.
So now we have unconditional dual-folder setup for both selectors.
And it's still a no-op for RocksDBBlueFSVolumeSelector as the latter is not used for single-device deployment for now.
Perhaps we can change this some day though, primarily for the sake of getting rid of the original selector and having a bit better bluefs stats reporting..
So while preparing this patch (which is primarily for original selector) I additionally tried RocksDBBlueFSVolumeSelector in a single device setup and discovered that it should adjust indicated capacity for slow folder for RocksDB to parse that properly. The latter was complaining about size set to 0...
So despite this specific modification in RocksDBBlueFSVolumeSelector::get_path() is generally unrelated to the proposed fix I kept it as-is for the sake of future selector unification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aclamk - ping

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.

I do not like this solution.
But I have no idea how to make better one.
So it is best solution we have!

@yuriw yuriw merged commit 274f7bf into ceph:master Nov 16, 2021
@ifed01 ifed01 deleted the wip-ifed-fix-vol-select branch November 16, 2021 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants