Skip to content

osd/PeeringState: rename "cancel_backfill" to "suspend_backfill"#61232

Merged
yuriw merged 1 commit intoceph:mainfrom
xxhdx1985126:wip-67888-followup
Feb 3, 2025
Merged

osd/PeeringState: rename "cancel_backfill" to "suspend_backfill"#61232
yuriw merged 1 commit intoceph:mainfrom
xxhdx1985126:wip-67888-followup

Conversation

@xxhdx1985126
Copy link
Contributor

Signed-off-by: Xuehan Xu xuxuehan@qianxin.com

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

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.

This requires a commit message, what do you think about?

PerringState events the are leading to `cancel_backfill()` are:
* DeferBackfill - Called if local recovery reservation is revoked before it completes (See AsyncResever::request_reservation on_preempt)
* UnfoundBackfill
* RemoteReservationRevokedTooFull
* RemoteReservationRevoked

In each event, we merely suspend the the backfill.
The primary will *continue* to keep trying to start this backfill as long as the up set for the current interval includes the osds that needs to be backfilled.
Eventually, the backfill will either succeed and complete or will be made irrelevant due to an interval change (and essentially truly "cancelled")

nit:
In PrimaryLogPG::do_scan we should update the logs to mention suspend instead of cancel:

	dout(1) << __func__ << ": Canceling backfill: Full." << dendl;
...
	// we canceled backfill for a while due to a too full, and this
	// is an extra response from a non-too-full peer
	dout(20) << __func__ << " canceled backfill (too full?)" << dendl;

}

void PG::on_backfill_canceled()
void PG::on_backfill_suspended()
Copy link
Contributor

@Matan-B Matan-B Jan 6, 2025

Choose a reason for hiding this comment

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

Perhaps we should add a comment as to why we are clearing waiting_on_backfill on suspend?

// Scan replies asked before suspending this backfill should be ignored.
// See PrimaryLogPG::do_scan -  case MOSDPGScan::OP_SCAN_DIGEST.
// `waiting_on_backfill` will be re-refilled after the suspended backfill is resumed/restarted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done:-)

@Matan-B Matan-B changed the title osd, crimson/osd: rename "cancel_backfill" to "suspend_backfill" osd/PeeringState: rename "cancel_backfill" to "suspend_backfill" Jan 6, 2025
PerringState events the are leading to `cancel_backfill()` are:
* DeferBackfill - Called if local recovery reservation is revoked
  before it completes (See AsyncResever::request_reservation on_preempt)
* UnfoundBackfill
* RemoteReservationRevokedTooFull
* RemoteReservationRevoked

In each event, we merely suspend the the backfill.
The primary will *continue* to keep trying to start this backfill
as long as the up set for the current interval includes the osds
that needs to be backfilled.

Eventually, the backfill will either succeed and complete or will
be made irrelevant due to an interval change (and essentially truly
"cancelled")

Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>
@ronen-fr
Copy link
Contributor

ronen-fr commented Feb 2, 2025

@yuriw yuriw merged commit 2fe3f07 into ceph:main Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Merged (Pre Tentacle Freeze)

Development

Successfully merging this pull request may close these issues.

6 participants