osd/backfill: give deletion ops a cost when performing backfill

[liuchang0812]

Fixes: http://tracker.ceph.com/issues/18875

Signed-off-by: liuchang0812 liuchang0812@gmail.com

[liewegas]

Have you tested this? If we don't get replies for the deletes are you sure we'll get woken up again?

[liuchang0812]

thanks Sages, I'm not sure about that, I will try to test it

[liuchang0812]

Hi, @liewegas . I read code carefully today to test this PR. Here is my understand. I'm not sure whether it's correct. please take a look.

The modified function called PrimaryLog::recover_backfill is used in PrimaryLogPG::start_recovery_ops[2]. In PrimaryLogPG::start_recovery_ops, started is a ref of 3rd parameter ops_started. We reset started to 0 at first[3], then plus value returned by function recover_backfill. We print started only :

    } else {
      started += recover_backfill(max - started, handle, &work_in_progress);
    }
  }
  dout(10) << " started " << started << dendl;
  osd->logger->inc(l_osd_rop, started);

Now, I want to know who use start_recovery_ops and what ops_started means. We could know that start_recovery_ops is used only in OSD::do_recovery[4] via searching source code as:

# grep -nr start_recovery_ops ../src/osd
../src/osd/PrimaryLogPG.cc:10540:bool PrimaryLogPG::start_recovery_ops(
../src/osd/PG.h:1087:  virtual bool start_recovery_ops(
../src/osd/OSD.cc:8723:    bool more = pg->start_recovery_ops(reserved_pushes, handle, &started);
../src/osd/PrimaryLogPG.h:1173:  bool start_recovery_ops(

The simplified code do_recovery as following [5]:

void OSD::do_recovery(  PG *pg, epoch_t queued, uint64_t reserved_pushes, ThreadPool::TPHandle &handle)
{
  uint64_t started = 0;
 /* some works about missing item, maybe inc started */
  bool more = pg->start_recovery_ops(reserved_pushes, handle, &started);
  service.release_reserved_pushes(reserved_pushes);
}

and function OSDService::release_reserved_pushes(uint64_t pushes) subtract pushes from recovery_ops_reserved, as follwing:

  void release_reserved_pushes(uint64_t pushes) {                               
    Mutex::Locker l(recovery_lock);                                             
    assert(recovery_ops_reserved >= pushes);                                     
    recovery_ops_reserved -= pushes;                                             
    _maybe_queue_recovery();                                                     
  }

There are recovery_ops_active and recovery_ops_reserved in class OSDService[1], which are used for limitting recovery QPS. In all of those functions( OSD::do_recovery, PrimaryLogPG::start_recovery_ops, PrimaryLogPG::recover_backfill), reserved_pushes/max used for QPS limitation. Giving delete ops a cost will not broken current code. The import point is that we should be careful when we do ++ops. We must make sure ops <= max.

uint64_t PrimaryLogPG::recover_backfill(uint64_t max, ThreadPool::TPHandle &handle, bool *work_started);
bool PrimaryLogPG::start_recovery_ops(uint64_t max, ThreadPool::TPHandle &handle, uint64_t *ops_started)
void OSD::do_recovery(PG *pg, epoch_t queued, uint64_t reserved_pushes, ThreadPool::TPHandle &handle)

I also notice that recovery_ops_active/recovery_ops_reserved are used to judge whether to do recovery now (OSDService::_recover_now). We must desc recovery_ops_ active when we finish a recovery op. recovery_ops_active was inc by start_recovery_op, dec by finish_recovery_op. We only invoke start_recovery_op for to_push array, not to_remove array, so it's safe too.

[1]: recovery_ops_* variables

ceph/src/osd/OSD.h

Line 978 in fcd64d7

uint64_t recovery_ops_reserved;

[2]: PrimaryLogPG::start_recovery_ops https://github.com/ceph/ceph/blob/master/src/osd/PrimaryLogPG.cc#L10654

[3]: reset started in start_recovery_ops https://github.com/ceph/ceph/blob/master/src/osd/PrimaryLogPG.cc#L10589

[4]: OSD::do_recovery https://github.com/ceph/ceph/blob/master/src/osd/OSD.cc#L8799

[liuchang0812]

well, I've known this is a incomplete PR. I need to add more code that ++recovery_ops_active when invoke a delete ops and --recovery_ops_active when delete ops is successful/failed. Maybe I need to add one interface on_remove_failed to PGListener. this question is not as simple as I thought. I will read more code and try to solve it.

[liuchang0812]

jenkins test this please

[liewegas]

What worries me is if we don't get acks for removes we don't know when it is "done"... I think that would require adding a reply message, which it would be nice to avoid.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If you are a maintainer or core committer, please follow-up on this issue to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

[gregsfortytwo]

While I'd still love to see deletions get a cost, it looks like everybody agrees this PR is incomplete, so I'm closing it.