i'm concerned about the use of global state here. shouldn't these bids be specific to one rados object? if so, this map should be stored in the object's xattr with the rest of lock_info_t

Yeah, I don't understand this as well. It appears to be specific to one object, but because there is a single map holding data for all objects a mutex is needed? And then, even if bids doesn't need to persist between OSD restarts (why -- because data that helps to ensure fairness is safe to discard?), I think we still need to consider what happens when the primary moves to a different OSD and then returns back after a while? Wouldn't bids go stale?

This...appears to be a per-process bid map and associated mutex. This approach seems entirely untenable. The osd which handles invocation of an object class is the current primary for the pg containing that object and object classes do not get notified when the primary changes since they are categorically not supposed to maintain any state outside of their rados objects.

• What handles removing the bid object map when this specific osd ceases to be the primary for one of these objects?
• How does this ensure that the in-memory state actually reflects the most recent updates to the object if the primary changed briefly and the new primary handled one of these calls?
• How would this state get updated if the OSD's copy of the object is recovered?

I'll try to take a closer look at this tomorrow or Monday, but I strongly discourage you from trying to be clever with stashing state in memory. It seems like you could pretty easily use the existing object class api to actually store each object's state within its own omap entries. If performance is demonstrated to be a problem with that approach and no other simpler solution seems viable, it might be possible to add some object class owned ephemeral state to the ObjectContext, but that would be much more complicated.

inline

This...appears to be a per-process bid map and associated mutex. This approach seems entirely untenable. The osd which handles invocation of an object class is the current primary for the pg containing that object and object classes do not get notified when the primary changes since they are categorically not supposed to maintain any state outside of their rados objects.

* What handles removing the bid object map when this specific osd ceases to be the primary for one of these objects?

* How does this ensure that the in-memory state actually reflects the most recent updates to the object if the primary changed briefly and the new primary handled one of these calls?

* How would this state get updated if the OSD's copy of the object is recovered?

I'll try to take a closer look at this tomorrow or Monday, but I strongly discourage you from trying to be clever with stashing state in memory. It seems like you could pretty easily use the existing object class api to actually store each object's state within its own omap entries. If performance is demonstrated to be a problem with that approach and no other simpler solution seems viable, it might be possible to add some object class owned ephemeral state to the ObjectContext, but that would be much more complicated.

"Completely untenable" seems rather overdone. And the suggestion to transform potentially transient/discardable/reconstructable state into costly, transactional state seems bad advice. (And apparently that would conflict with RADOS semantics some of the time.)

From what I understand, a key point is that the bid state is valuable but not critical--the state is a set of hints, basically. As I observed elsewhere, it's probably indeed intended to be per pg, and not "per process," which would suggest partitioning the map and its mutex by pg, maybe? It seems to me the currently biggest gap is in not bounding the size or lifetime of the state in the map(s), but agree that while the stored state is apparently not critical and thus, apparently ok to allow to be stale (as in your pg primary change/change back and object recovery scenarios). OTOH, it would probably be attractive if something could induce this module to drop state for a pg entirely in that event.

Matt

Iow, no one is (unreasonably) trying to "be clever." Folks are trying to implement an efficient work-sharing mechanism that is, weakly, stateful. I think @cbodley's suggestion to stash some specific data in ObjectContext was made on the assumption that it would be easy, and not intrusive. I buy that that isn't the case, but statefulness of the type intended here is not overreach, it's just ordinary application of standard art.

The librados interface is designed around objects and the osd is set up to maintain durable state around those objects; and to handle their consistency. Nothing in any interface is set up to handle in-memory state or for the object_class api to be stateful separate from what it writes out to objects. A "weakly stateful" Ceph interface doesn't exist.

Now, if this is actually a critical capability to develop, we can talk about that. But trying to shoehorn it in through an accident of the class implementation is indeed not tenable. There needs to be either a fundamentally different solution or a fundamentally different approach to getting it into the OSD, with a new system that has defined semantics and a proper interface for dealing with...all the things the osd does.

That said, there is a mechanism for users of the osd to communicate between each other and send around state. It's watch-notify. Can this bid-and-lock algorithm be constructed that way, instead?

thanks for the feedback!

It's watch-notify. Can this bid-and-lock algorithm be constructed that way, instead?

good point, let's try to build a design around that. multisite already uses watch-notify to coordinate bilog trimming, so that could serve as an example

I had a quick discussion with @mattbenjamin and it sounds like the crux of the issue is that you can't update a hypothetical persistent version of this state in the event that the lock operation fails. Is that a reasonable characterization of the issue?

IIRC, object class writes can return payloads now. We could get around this by modifying the cls_lock behavior to always succeed if the client sent a bid (or add a differently named method with that behavior) and instead indicate success via the returned payload.

IIRC, object class writes can return payloads now.

Correct. It looks like on the client side you have to set the flag OPERATION_RETURNVEC, and then just fill "outdata" in the class (same as for a read method), and the OSD checks it's less than osd_max_write_op_reply_len and passes it back as part of the result. This looks to be used by rgw/cls_fifo_legacy.cc and rgw/rgw_notify.cc, plus examples in cls_hello.

nit: this is the same as default-constructing bid_duration, so we can remove it from the constructor's member initializer list entirely

please log and return these errors, instead of asserting on them. same with the cls_get_obj_info() call below. unexpected errors here should never crash the osd

this inheritance structure doesn't make a lot of sense. BiddedRadosLock inherits SimpleRadosLock for its member variables, but overrides all of its logic. and the only difference is this RGWGetBidFunc piece

instead of creating this new Bidded subclass, maybe we just add an optional<RGWLockBid> argument to SimpleRadosLock? then ContinuousLeaseCR would pass std::nullopt, and RebiddableLeaseCR would pass the result of its bid func

std::function can be a very useful abstraction, but it comes with a cost. whenever the lambda capture is bigger than ~24 bytes, it has to allocate memory. full_bid_func = [this, bid_func, bid_duration_secs]() -> RGWLockBid { is always going to allocate because it captures another function. passing that function around will allocate again every time it's copied

in this case, we don't really need the abstraction - we're defining RGWBidManagerCR just below. we should be able to take a pointer to that and call into it directly

there's also the fu2 (more efficient std::function alternative) by @adamemerson ?

It's not more efficient, the main reason for pulling it in is for unique_function. (For functions that aren't copyable.)

We will not use the std::function here at all, the solution that @cbodley should work just fine and maybe we can remove the shuffle mechanism, from my tests, it doesn't improve the locks distribution (I mean the rgw will need to shuffle only to initial the bids for one time only) over time and it makes the solution a little bit more complex.

full_sync already switched, right?

it doesn't look like we're checking this until we've already finished all the work. i feel like the best place for this check is at the very top of the work loop above:

for (log_iter = log_entries.begin(); log_iter != log_entries.end() && !done_with_period; ++log_iter) {
  if (ceph::coarse_mono_clock::now() >= work_period_end) {

if we see that we're passed the work period, we should spawning more work. but we can't drop the lock immediately - first we need to wait for any pending work to finish, then write the updated sync status. it doesn't look like metadata sync ever flushes the sync status, but here's an example from data sync: https://github.com/ceph/ceph/blob/e9d361f6/src/rgw/rgw_data_sync.cc#L4080-L4084

this approach should minimize how far we overshoot the work period. does that make sense? do you think that's consistent with your design?

can you help me understand what we're trying to do differently between these two cases? if i understand correctly, there's very little difference between return 0 and continue here

return set_cr_done() would complete the coroutine successfully, and i agree that's not what we want to do here. RGWMetaSyncShardControlCR spawned us, and it doesn't retry on success, so nothing would ever run sync on this shard again

return 0 just acts like a yield that leaves the coroutine scheduled. that would let RGWCoroutinesManager::run() go and process any other scheduled coroutines, then call us right back

what do you think should happen here? should R_WORK_COMPLETE wait for a bit before trying to grab the lock again? or should both cases retry right away?

this bid manager coroutine is simple and straightforward, but it complicates RGWRemoteMetaLog::run_sync() with an extra background coroutine. that's passing a raw RGWBidManagerCR pointer into RGWMetaSyncCR, but i don't think we can guarantee that the former coroutine will outlive the latter. to make that safe, RGWMetaSyncCR would need to hold a reference with intrusive_ptr<RGWBidManagerCR>, as well as the intrusive_ptr<RGWCoroutinesStack> it was spawned on

instead of this coroutine, what if the shuffle was just part of the 'bid function'?

class BidFunc {
  ceph::timespan interval;
  ceph::timespan bid_duration;
  ceph::coarse_mono_time last_shuffle;
  rados::cls::lock::BidSet<int32_t> bids;
 public:
  BidFunc(...);

  RGWLockBid operator()(int shard_id, ceph::coarse_mono_time now) {
    if (last_shuffle + interval < now) {
      bids.shuffle();
      last_shuffle = now;
    }
    return {bids.get_bid(shard_id), bid_duration};
  }
};

this could just live on the stack in RGWRemoteMetaLog::run_sync(), so wouldn't need any special memory management

do we really need a template here if the functions all require int32_t bid_amount?

here's a nice 3-line example of a random shuffle from rgw_lc.cc:

  std::random_device rd;
  std::default_random_engine rng{rd()};
  std::shuffle(v.begin(), v.end(), rng);

we can avoid reopening the random device on every call by doing it once in the constructor to seed a std::default_random_engine member variable. don't forget to #include <random> and <algorithm>

RGWContinuousLeaseCR doesn't have a polling loop like this, it just fails and returns EBUSY up through RGWMetaSyncShardCR, and RGWMetaSyncShardControlCR keeps retrying with backoff. it shouldn't be necessary to have a retry loop here in RGWRebiddableLeaseCR

otherwise, the rest of this coroutine looks essentially the same as RGWContinuousLeaseCR except that it calls RGWBiddedRadosLockCR instead of RGWSimpleRadosLockCR

in another comment, i suggested combining RGWBiddedRadosLockCR into RGWSimpleRadosLockCR by using an optional<RGWLockBid> argument. could we do the same with RGWRebiddableLeaseCR and RGWContinuousLeaseCR, except with an optional<BidFunc>?

didn't full_sync() already wait on this lock acquisition above on line 1637?

maybe this part inside the do-while loop should just check if (!rebiddable_lease_cr->is_locked()) { for an early exit like you do below in incremental_sync()?

May is_highest_bidder(shard_id) may be invoked before spawn RGWMetaSyncShardControlCR ? i mean if rgw could not get the highest bidder, it's not necessary spawn RGWMetaSyncShardControlCR. @smanjara @cbodley

RGWMetaSyncShardControlCR is where the retry loop lives. we want to run even if we aren't the highest bidder yet