jenkings test make check

jenkins test make check

IIUC, this PR is treating the whole backfill of a PG as a single backfill operation, which can contain tens of thousands of pushes and objectstore transactions. I think it is too large to be treated as a single operation. The whole backfill is splitted into multiple batches of pushes, so we should probably treat a batch of those pushes as a single operation instead.

IIUC, this PR is treating the whole backfill of a PG as a single backfill operation, which can contain tens of thousands of pushes and objectstore transactions. I think it is too large to be treated as a single operation. The whole backfill is splitted into multiple batches of pushes, so we should probably treat a batch of those pushes as a single operation instead.

I am not sure it would be a good idea to use throttle future in multiple batches. The "BackfillState::Enqueuing::Enqueuing" state is doing other time consuming activity also like should_rescan_primary . If we do breakup function(Enqueuing) in multiple task and spawn under throttle it will be a complex and difficult to maintain. If we do check classic osd code also in recover_backfill it is backfilled all objects in one shot specific to a PG so i believe current behavior is similar to classic OSD. We can check later if we will find some issue.

@Matan-B Can you please share your view on the same ?

IIUC, this PR is treating the whole backfill of a PG as a single backfill operation, which can contain tens of thousands of pushes and objectstore transactions. I think it is too large to be treated as a single operation. The whole backfill is splitted into multiple batches of pushes, so we should probably treat a batch of those pushes as a single operation instead.

I am not sure it would be a good idea to use throttle future in multiple batches. The "BackfillState::Enqueuing::Enqueuing" state is doing other time consuming activity also like should_rescan_primary . If we do breakup function(Enqueuing) in multiple task and spawn under throttle it will be a complex and difficult to maintain. If we do check classic osd code also in recover_backfill it is backfilled all objects in one shot specific to a PG so i believe current behavior is similar to classic OSD. We can check later if we will find some issue.

A single run of Enqueuing is a single batch of object pushes, the backfill is splitted into multiple batches by the multiple Enqueuing run. The classic OSD also split the backfill into multiple batches, which is controlled by osd_recovery_max_single_start and osd_recovery_max_active.

A single run of Enqueuing is a single batch of object pushes, the backfill is splitted into multiple batches by the multiple Enqueuing run. The classic OSD also split the backfill into multiple batches, which is controlled by osd_recovery_max_single_start and osd_recovery_max_active.

• osd_recovery_max_active could be used here crimson/osd/pg_recovery: use OperationThrottler to throttle object pushes/pulls #62080 to have a per reactor limit.

• osd_recovery_max_single_start should probably used by the throttler introduced here.

@mohit84, Looking at Classic:
We call start_recovery_ops with the passed reserved_pushes.
reserved_pushes is set via:

    uint64_t to_start = std::min(
      available_pushes,
      cct->_conf->osd_recovery_max_single_start);

Later on, when we actually backfill see PrimaryLogPG::start_recovery_op we use the reserved_pushes as max.
In recover_replicas or recover_primary and most notably in `PrimaryLogPG::recover_backfill the backfill interval iteration is limited with:

while (ops < max) {

Going back to the comment above:

If we do check classic osd code also in recover_backfill it is backfilled all objects in one shot specific to a PG so i believe current behavior is similar to classic OSD.

Classic does limit the object backfilled in _maybe_queue_recovery by using both the osd_recovery_max_active and the osd_recovery_max_single_start.
Note: Crimson's throttle is defined in PerShardState::throttler meaning we should be able to throttle the backfill interval per shard.

Once we would support both configurables we could pick to_start, similarly to classic, "std::min" above.

(CC: @sseshasa)

The issue is fixed by the pull request #62080 so i am closing this.