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.

Sign up for GitHub

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

Jump to bottom

crimson/osd/osd_operations/snaptrim_event: update PG's stats #56378

Merged
merged 1 commit into from Mar 27, 2024
Merged

crimson/osd/osd_operations/snaptrim_event: update PG's stats #56378

merged 1 commit into from Mar 27, 2024

Conversation

guojidan
Copy link
Contributor

@guojidan guojidan commented Mar 22, 2024 •
edited

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

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

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
  • jenkins test rook e2e

@guojidan guojidan requested a review from a team as a code owner March 22, 2024 02:46
@guojidan
Copy link
Contributor Author

I imitate the corresponding logic in classic 😕

@guojidan
Copy link
Contributor Author

hi @athanatos , please take a look at my PR when you have free time, thank you 😄

Matan-B
Matan-B requested changes Mar 24, 2024
View reviewed changes
Copy link
Contributor

@Matan-B Matan-B left a comment

Choose a reason for hiding this comment

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

Hey @guojidan, thank you for the contribution.

Commit title should be:
crimson/osd/osd_operations/snaptrim_event: update PG's stats

Also, the commit message should include:
Fixes: https://tracker.ceph.com/issues/63307

Please see: https://github.com/ceph/ceph/blob/main/SubmittingPatches.rst

src/crimson/osd/osd_operations/snaptrim_event.cc Outdated
}
pg->get_peering_state().apply_op_stats(coid, delta_stats);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move OpsExecuter::apply_stats() logic into PG::apply_stats(). That way both OpsExecuter and SnapTrimObjSubEvent will be able to call pg->apply_stats().

src/crimson/osd/osd_operations/snaptrim_event.cc Outdated
Comment on lines 411 to 413
int64_t num_objects_trimmed =
num_objects_before_trim - delta_stats.num_objects;
pg->get_peering_state().update_stats_wo_resched([num_objects_trimmed](auto &history, auto &stats) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Styling: indention and line length

src/crimson/osd/osd_operations/snaptrim_event.cc Outdated
// num_objects_before_trim - delta_stats.num_objects;
//add_objects_trimmed_count(num_objects_trimmed);
int64_t num_objects_trimmed =
num_objects_before_trim - delta_stats.num_objects;
Copy link
Contributor

Choose a reason for hiding this comment

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

num_objects_before_trim is not an entirely correct name since delta_stats is local to each SnapTrimObjSubEvent and is zeroed. I think that we can drop it and replace with:

if (delta_stats.num_objects < 0) {
  int64_t num_objects_trimmed = std::abs(delta_stats.num_objects);
}

@guojidan guojidan changed the title Crimson: scrub will update the PG's stats crimson/osd/osd_operations/snaptrim_event: update PG's stats Mar 25, 2024
@guojidan
Copy link
Contributor Author

@Matan-B thank you so much for your review, I have made the changes according to your suggestion. 😄

Matan-B
Matan-B approved these changes Mar 25, 2024
View reviewed changes
Copy link
Contributor

@Matan-B Matan-B left a comment

Choose a reason for hiding this comment

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

LGTM

src/crimson/osd/osd_operations/snaptrim_event.cc Outdated Show resolved Hide resolved
@Matan-B
Copy link
Contributor

Matan-B commented Mar 25, 2024

Note: SnapTrimObjSubEvent calculation of num_bytes seems to be off. I suspect this is caused due to wrong clone_overlap which are used in SnapSet::get_clone_bytes. This is out of the scope of this PR and I'll open a tracker once this PR is merged.

CC: @guojidan

@guojidan
Copy link
Contributor Author

Note: SnapTrimObjSubEvent calculation of num_bytes seems to be off. I suspect this is caused due to wrong clone_overlap which are used in SnapSet::get_clone_bytes. This is out of the scope of this PR and I'll open a tracker once this PR is merged.

CC: @guojidan

please let me try implement this, I am very willing. 😄

@Matan-B
Copy link
Contributor

Matan-B commented Mar 25, 2024

please let me try implement this, I am very willing. 😄

@guojidan, feel free to assign the ticket to yourself.
https://tracker.ceph.com/issues/65113

@Matan-B Matan-B requested a review from athanatos March 26, 2024 14:18
athanatos
athanatos approved these changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@athanatos athanatos left a comment

Choose a reason for hiding this comment

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

This needs to go through testing, but it should probably wait until the scrub errors shake out. I'll add it to my test branch.

@athanatos
Copy link
Contributor

jenkins test make check arm64

@athanatos
Copy link
Contributor

Nicely done, clean and to the point.

@athanatos athanatos added the wip-sjust-crimson-testing Test run pending by Sam label Mar 27, 2024
@athanatos
Copy link
Contributor

No failures (!) https://pulpito.ceph.com/sjust-2024-03-27_18:08:01-crimson-rados-wip-sjust-crimson-testing-2024-03-26-distro-default-smithi/

@athanatos athanatos merged commit ca414b0 into ceph:main Mar 27, 2024
10 of 11 checks passed
@guojidan guojidan deleted the snap-stats branch March 28, 2024 01:23
@guojidan
Copy link
Contributor Author

thanks you! @Matan-B @athanatos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@athanatos athanatos athanatos approved these changes

@Matan-B Matan-B Matan-B approved these changes

Assignees
No one assigned
Labels
crimson needs-crimson-qa wip-sjust-crimson-testing Test run pending by Sam
Projects
None yet
Milestone
No milestone
3 participants
@guojidan @Matan-B @athanatos