Nit: According to the C++ documentation, emplace still constructs a temporary map even if the key already exists in rule_weight_map. For large numbers of iterations, could this behavior lead to noticeable CPU overhead? I’m wondering if using try_emplace might save a meaningful amount of CPU in this case.

This line is executed once per crush root (not once per PG) - i.e. only a handful of times, and inserts an empty weight_map into rule_weight_map. The cost of populating the weights (the get_rule_weight_osd_map call that follows) is orders of magnitude more than copying an empty map once. Also - the key doesn't exist for sure - that's what the if statement above checks.

I would suggest renaming it to rule_entry or something more descriptive, as it was difficult to parse out what exactly it was referencing. But now I understand that this refers to the full rule entry, including the rule id and its associated weight map.

I've rewritten it here in full:

    // below we check against crush-topology changing..
    auto rule_entry = rule_weight_map.find(crush_rule);
    if (rule_entry == rule_weight_map.end()) {
      rule_entry = rule_weight_map.emplace(crush_rule, map<int, float>()).first;
      auto r = crush->get_rule_weight_osd_map(crush_rule, &rule_entry->second);
      if (r < 0) {
        lderr(cct) << __func__ << " unable to get crush weight_map for "
                   << "crush_rule " << crush_rule
                   << dendl;
        rule_weight_map.erase(rule_entry);
        continue;
      }
    }
    const map<int, float> &weight_map = rule_entry->second;
    ldout(cct, 10) << __func__ << " pg " << pg
                   << " weight_map " << weight_map
                   << dendl;

Also, I have another suggestion for readability. It seems redundant to use emplace(), which returns a pair containing:

1. The crush rule entry
2. A boolean indicating if the entry was freshly inserted or not

If the crush entry already exists, my understanding is that rule_weight_map.emplace(crush_rule, map<int, float>()) would return a pair containing 1. the crush entry, and 2. false, indicating that it did not create a fresh entry.

Currently, we only make use of 1, but ignore 2. 2 could be a good indicator of whether or not we need to populate the weight map.

What if we did something like this and got rid of the iterator?

    // below we check against crush-topology changing..
    // rule_check inserts a fresh entry only if one does not already exist.
    auto rule_check = rule_weight_map.emplace(crush_rule, map<int, float>());
    auto rule_entry = rule_check.first;
    bool new_entry_inserted = rule_check.second;
    if (new_entry_inserted) {
      // Populate empty weight map
      auto r = crush->get_rule_weight_osd_map(crush_rule, &rule_entry->second);
      if (r < 0) {
        lderr(cct) << __func__ << " unable to get crush weight_map for "
                   << "crush_rule " << crush_rule
                   << dendl;
        rule_weight_map.erase(rule_entry);
        continue;
      }
    }
    const map<int, float> &weight_map = rule_entry->second;
    ldout(cct, 10) << __func__ << " pg " << pg
                   << " weight_map " << weight_map
                   << dendl;

I don't think this compromises logic or performance. But please disregard if it does. Definitely the first option I listed is the more conservative option for improving readability.

I've made the change for renaming 'it' to 'rule_entry' - it definitely makes the code clearer.
The concern I have about the second change is that an empty map<int, float>() is created and destroyed for every PG iteration, adding lots of memory allocations/deallocations, which isn't ideal.

Got it, yeah we don't want to compromise performance. LGTM otherwise!

Ouch, that was the problem. How has it been found?