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

rgw: limit rgw_gc_max_objs to RGW_SHARDS_PRIME_1 #39515

Merged
merged 1 commit into from Mar 4, 2021

Conversation

xelexin
Copy link
Contributor

@xelexin xelexin commented Feb 17, 2021

Don't allow GC to process more gc ojects than RGW_SHARDS_PRIME_1

Fixes: https://tracker.ceph.com/issues/49321
Signed-off-by: Rafał Wądołowski rwadolowski@cloudferro.com

@github-actions github-actions bot added the rgw label Feb 17, 2021
@mattbenjamin mattbenjamin self-requested a review February 17, 2021 12:07
gc(_gc),
remove_tags(cct->_conf->rgw_gc_max_objs),
tag_io_size(cct->_conf->rgw_gc_max_objs) {
gc(_gc) {
max_aio = cct->_conf->rgw_gc_max_concurrent_io;
Copy link
Contributor

Choose a reason for hiding this comment

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

this slightly scrambles indentation, maybe try to align all parts of the ctor-initializer list uniformly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, done

max_aio = cct->_conf->rgw_gc_max_concurrent_io;
remove_tags.reserve(min(static_cast<int>(cct->_conf->rgw_gc_max_objs), rgw_shards_max()));
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this enforce limit on rgw_gc_max_objs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will not allow to process over rgw_shards_max() value. Without this change, if we set rgw_gc_max_objs for 70000, the RGWGC::remove tries to gc_aio_operate operate on not existent objects

Copy link
Contributor

Choose a reason for hiding this comment

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

this reserves space for thus many elements, right? is that needed because we're using emplace? do we really want to reserve the max, though?

Copy link
Contributor

Choose a reason for hiding this comment

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

i.e., would it make sense to enforce a max -size-, and reject an attempt to reshard to a larger size? separately, maybe we could avoid the pre-sizing optimization, or else adjust things around it to avoid expecting the current max size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the code, we're currently allocating "unlimited" (according to configuration option) remove_tags and tag_io_size. This reserve won't allow to allocate more than RGW_SHARDS_PRIME_1. We have a lot of static references to these vectors, so yes I'm agree that RGWGCIOManager should be refactored, but this require more work. As for now I think this fix will block, deletion of objects over v RGW_SHARDS_PRIME_1.

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't understand this change to use reserve() instead of constructing the vector with a given size. this leaves the vector's size() at 0, meaning that:

  • all attempts to index it will be out of bounds:
auto& rt = remove_tags[index];
  • for-loops will iterate over an empty vector:
  void flush_remove_tags() {
    int index = 0;
    for (auto& rt : remove_tags) {

instead, i would just default-construct the empty vectors, and wait to resize them until initialize():

 void RGWGC::initialize(CephContext *_cct, RGWRados *_store) {
   cct = _cct;
   store = _store;
 
   max_objs = min(static_cast<int>(cct->_conf->rgw_gc_max_objs), rgw_shards_max());
+  remove_tags.resize(max_objs);
+  tag_io_size.resize(max_objs);

Copy link
Contributor

Choose a reason for hiding this comment

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

@cbodley thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbodley sure, resize is better, I placed it under constructor of IOManager, because the RGWGC::initialize doesn't know about it

Don't allow GC to process more gc ojects than RGW_SHARDS_PRIME_1

Fixes: https://tracker.ceph.com/issues/49321
Signed-off-by: Rafał Wądołowski <rwadolowski@cloudferro.com>
Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

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

looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants