Skip to content

crimson/osd: submit_error_log cleanup#54765

Merged
Matan-B merged 4 commits intoceph:mainfrom
Matan-B:wip-matanb-crimson-submit-error-cleanup
Jan 17, 2024
Merged

crimson/osd: submit_error_log cleanup#54765
Matan-B merged 4 commits intoceph:mainfrom
Matan-B:wip-matanb-crimson-submit-error-cleanup

Conversation

@Matan-B
Copy link
Contributor

@Matan-B Matan-B commented Dec 4, 2023 •
edited
Loading

Following: #54287

https://pulpito.ceph.com/matan-2023-12-04_08:39:13-crimson-rados-wip-matanb-crimson-submit-error-cleanup-testing-distro-crimson-smithi/

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

@Matan-B Matan-B added the cleanup label Dec 4, 2023
@Matan-B Matan-B requested a review from a team as a code owner December 4, 2023 09:36
@Matan-B Matan-B added the TESTED label Dec 4, 2023
cyx1231st
cyx1231st reviewed Dec 6, 2023
View reviewed changes
Copy link
Member

@cyx1231st cyx1231st left a comment

Choose a reason for hiding this comment

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

Make check is failed due to compile issue.

src/crimson/osd/pg.cc
return seastar::now();
});
}
}
Copy link
Member

@cyx1231st cyx1231st Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Looks this logic/effort is moved from the concurrent wait_repop stage into the exclusive process stage, is it intentional?

If it isn't intentional, probably leaving it in the concurrent wait_repop phase is better than in the exclusive phase from performance perspective.

Or probably it doesn't matter because it isn't supposed to be frequent, what do you think?

Copy link
Contributor Author

@Matan-B Matan-B Dec 6, 2023

Choose a reason for hiding this comment

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

I'm inclined to move it into the exclusive stage. This path shouldn't be too frequent and I think that it'll be better to avoid completing the write (by calling PeeringState::complete_write) in the concurrent stage.

Copy link
Member

@cyx1231st cyx1231st Dec 7, 2023

Choose a reason for hiding this comment

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

Worth adding this impact to the commit message.

Copy link
Member

@cyx1231st cyx1231st Dec 7, 2023

Choose a reason for hiding this comment

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

This path shouldn't be too frequent and I think that it'll be better to avoid completing the write (by calling PeeringState::complete_write) in the concurrent stage.

Reasonable to me. Just thinking that probably it is structurally better to keep log_update.all_committed.get_shared_future() in the concurrent wait_repop phase, and keep PeeringState::complete_write in the next exclusive send_reply phase.

Copy link
Member

@cyx1231st cyx1231st Dec 7, 2023

Choose a reason for hiding this comment

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

Hmm, looks like in the normal path, PeeringState::complete_write is also called in the concurrent stage, see https://github.com/ceph/ceph/blob/ea54fcc13ebadbd23b2071701f9788e7d3a4ba8c/src/crimson/osd/pg.cc#L790C4-L790C34

Copy link
Contributor Author

@Matan-B Matan-B Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Reasonable to me. Just thinking that probably it is structurally better to keep log_update.all_committed.get_shared_future() in the concurrent wait_repop phase, and keep PeeringState::complete_write in the next exclusive send_reply phase.

I moved the helper method to be executed in the concurrent stage.

@Matan-B Matan-B removed the TESTED label Dec 7, 2023
@Matan-B Matan-B force-pushed the wip-matanb-crimson-submit-error-cleanup branch 4 times, most recently from 2d74327 to fbb1b6d Compare December 7, 2023 14:23
@Matan-B Matan-B mentioned this pull request Dec 7, 2023
Merged
14 tasks
@Matan-B Matan-B requested review from a team, athanatos and cyx1231st December 7, 2023 14:26
cyx1231st
cyx1231st approved these changes Dec 8, 2023
View reviewed changes
Copy link
Member

@cyx1231st cyx1231st left a comment

Choose a reason for hiding this comment

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

make check is failed by /home/jenkins-build/build/workspace/ceph-pull-requests/src/crimson/osd/pg.cc:914:10: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]

Others seem good to me.

@github-actions
Copy link

github-actions bot commented Dec 19, 2023

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@Matan-B Matan-B force-pushed the wip-matanb-crimson-submit-error-cleanup branch from fbb1b6d to 645c0d9 Compare December 19, 2023 08:54
@Matan-B
Copy link
Contributor Author

Matan-B commented Dec 19, 2023

https://shaman.ceph.com/builds/ceph/wip-matanb-crimson-testing-submit-error-cleanup/

athanatos
athanatos requested changes Dec 19, 2023
View reviewed changes
@Matan-B Matan-B force-pushed the wip-matanb-crimson-submit-error-cleanup branch from 645c0d9 to 2d98d31 Compare December 27, 2023 09:36
@Matan-B Matan-B requested a review from athanatos December 27, 2023 13:26
@Matan-B Matan-B force-pushed the wip-matanb-crimson-submit-error-cleanup branch from 2d98d31 to 6dd1c1b Compare December 27, 2023 13:28
@Matan-B Matan-B force-pushed the wip-matanb-crimson-submit-error-cleanup branch from 6dd1c1b to 1c5db6b Compare January 14, 2024 08:23
@Matan-B
Copy link
Contributor Author

Matan-B commented Jan 14, 2024

Merging based on:
https://pulpito.ceph.com/matan-2024-01-11_16:11:41-crimson-rados-wip-matanb-crimson-submit-error-cleanup-v2-distro-crimson-smithi/

@Matan-B
Copy link
Contributor Author

Matan-B commented Jan 14, 2024

jenkins test docs

@Matan-B Matan-B force-pushed the wip-matanb-crimson-submit-error-cleanup branch from 1c5db6b to 4e7913c Compare January 14, 2024 10:11
@Matan-B
crimson/osd/pg: do_osd_ops_execute drop error_func_ptr
841bc65 
std::move to the only user instead

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/osd/pg: submit_error_log cleanup
b44c41f 
* error log completion logic is moved into maybe_submit_error_log
* renamed it and it2
* maybe_submit_error_log is moved outside of failure_func
* failure_func no longer gets rep_tid and record_error params
* log_entry_version is removed, submit_error_log returns the version instead

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/osd/pg: on_removal to assert empty log_entry map
0f99cdc 
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/osd/pg: move maybe_submit_error_log
4997ead 
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B Matan-B force-pushed the wip-matanb-crimson-submit-error-cleanup branch from 4e7913c to 4997ead Compare January 16, 2024 12:44
@Matan-B
Copy link
Contributor Author

Matan-B commented Jan 16, 2024

jenkins test api

@Matan-B Matan-B merged commit 73d7cb6 into ceph:main Jan 17, 2024
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

@cyx1231st cyx1231st cyx1231st approved these changes

Assignees

No one assigned

Labels

cleanup crimson ready-to-merge TESTED

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Matan-B @athanatos @cyx1231st