This is validating an invariant that should always hold, right? I would think that derr would be more appropriate.

It was (that's why I used dout(1) - and should have probably used derr).
I was less sure now (that's why changed to 10), and as the logic changes in that
othe scrub-sched PR - didn't invest time in making sure.

For now - left this change out of the PR

It seems to me that we probably want FullReset{} to be a valid event regardless of the state of the state machine -- why not add noop handlers so that it's universally valid?

the full_reset event is always effective, regardless of the FSM state (as it was meant
as a way to guarantee reset).

For the replica case - I like the suggestion of adding a new event to the state machine. Performing
this change now.

For the primary case - that would not be enough to replace the 'role' data I have added here (for the
same reason m_active does not cover the whole timespan covered by is_queued_or_active). Adding an
event for this case is only reducing clarity (I have tried that now).

The commit message for these removals is:

    osd/scrub: remove some calls to publish_stats_to_osd()
    
    The calls were removed as publish_stats..() expects a
    stable PG state, and that state is not synchronized
    with the scrubber operation.

I don't think this conveys to a future reader of this commit why it's safe to remove them. Either
a) the publish_stats_to_osd() calls are necessary or
b) they aren't.

In the case of a), the fact that they also require a stable PG state merely means that removing them introduces another bug. In the case of b), the fact that they require a stable PG state isn't really relevant.

Presumably, you've audited the code and concluded that they are safe to remove. Please explain that in the commit message.

The message for this

    osd/scrub: fix replica-reservation timeout cancellation
    
    resetting m_no_reply from within causes a crush, and prevents
    scrub code testing.

doesn't explain how this results in a crash. At first glance, this modification would appear to be a noop as send_reject() is called immediately.

Further inspection shows that ReplicaReservations::handle_no_reply_timeout() also invokes ReplicaReservations::send_reject(). ReplicaReservations::handle_no_reply_timeout() is in turn invoked from ReplicaReservations::no_reply_t::m_abort_callback as populated in no_reply_t(). I suspect that the crash you are seeing is due to m_abort_callback being invoked from the timer thread under the sleep_lock. Invoking m_no_reply.reset() in that context would take the sleep_lock again resulting in an assert failure.

If the above is indeed the cause of the crash, this doesn't appear to be a fix as it fails to clean up m_no_reply in the timeout path.

@athanatos : yes - that was the failed scenario. Checking what you said in the last sentence.

I'm now less sure about this diagnosis. SafeTimer can optionally either hold the lock while calling callbacks or not. There's a boolean param passed to the constructor which governs this behavior -- safe_callbacks. sleep_timer specifically seems to be constructed with safe_callbacks set to false, so locking OSDService::sleep_lock shouldn't be a problem.

Can you link to the tracker id?

This diff doesn't happen to contain PgScrubber::scrub_clear_state(), so I comment here.

// uses process_event(), so must be invoked externally
void PgScrubber::scrub_clear_state()
{
  dout(10) << __func__ << dendl;

  clear_pgscrub_state();
  m_fsm->process_event(FullReset{});
}

scrub_clear_state() copied above still uses a FullReset{}. ActiveReplica does happen to have a FullReset{} handler, but its behavior differs from IntervalEnded handler in that it lacks the call to replica_handling_done(). scrub_clear_state() has a few callers:

1. PgScrubber::verify_against_abort() -- I find this mechanism confusing, tbh, but it does seem to be callable at any time, including when there are outstanding replica scrubs.
2. PgScrubber::replica_scrub_op -- This is an error case and seems to also be callable while there is outstanding replica state.
3. PrimaryLogPG::on_change -- I don't really see a reason for this one now that we call into scrubber in PG::on_new_interval(). This also seems like it means that replicas will get IntervalEnded and then FullReset and primaries will get FullReset twice.
4. PrimaryLogPG::on_shutdown -- Same observations as above, uses FullReset though the scrubber might be a replica.

Introducing IntervalEnded seems to have created a bunch of opportunities for pg state resets to be inconsistent. Instead, I propose the following changes:

1. Rename PgScrubber::stop_active_scrubs to PgScrubber::on_new_interval, simplify it to just submit a FullReset
2. Modify the FullReset handlers for every state to call back into cleanup functions specific to their states (or do it in their destructors?)
3. Drop the call to PgScrubber::clear_scrub_state() from PrimaryLogPG::on_change()
4. Change PrimaryLogPG::on_shutdown to invoke a PgScrubber::on_shutdown method which submits a FullReset event.
5. Update other reset pathways to use FullReset as well.

This way, the state machine states handle any state-specific behavior, and reset becomes simply an event which can be done identically from anywhere.

I think this should remove the need for queued_for_role() as well?

Rename PgScrubber::stop_active_scrubs to PgScrubber::on_new_interval, simplify it to just submit a FullReset

@athanatos the problem with that is that the states of "being reserved by a Primary" (and the longer "a message carrying the request to be reserved by a primary was queued" are not handled within the scrubber state machine (it could have been done differently, at least regarding the first, but it's not something I wish to change now)

By "a message carrying the request to be reserved by a primary was queued", do you mean the time the message spends between being queued via the messenger Dispatcher interface and when it gets processed by the PG and and scrubber? Why is that important?

Or, do you mean the time between when replica_scrub_op is called and when the event queued there is processed?

@ronen-fr I think it's worth mapping these resources onto the state machine. I've started working on a branch https://github.com/athanatos/ceph/tree/sjust/wip-scrub-interval-pr-49959-2 which does that. I'm still working on it, so it's likely got errors.