From: Casey Bodley Date: Wed, 25 Sep 2024 22:38:08 +0000 (-0400) Subject: rgw/rados: rgw_rados_operate() takes version_t* X-Git-Tag: v20.0.0~934^2 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=7769bdb9438d48afc8500c16167f82c8f549a914;p=ceph.git rgw/rados: rgw_rados_operate() takes version_t* instead of calling ioctx.get_last_version() after a rados operation, callers now pass version_t* as an output parameter. in the null_yield case, that version is assigned to ioctx.get_last_version() as normal. in the async case, we get the version out of librados::async_operate()'s return value Fixes: https://tracker.ceph.com/issues/63935 Signed-off-by: Casey Bodley --- diff --git a/src/rgw/driver/rados/rgw_rados.cc b/src/rgw/driver/rados/rgw_rados.cc index e618c40cb90b9..45d74a6ec24b4 100644 --- a/src/rgw/driver/rados/rgw_rados.cc +++ b/src/rgw/driver/rados/rgw_rados.cc @@ -3247,7 +3247,7 @@ int RGWRados::Object::Write::_do_write_meta(uint64_t size, uint64_t accounted_si auto& ioctx = ref.ioctx; tracepoint(rgw_rados, operate_enter, req_id.c_str()); - r = rgw_rados_operate(rctx.dpp, ref.ioctx, ref.obj.oid, &op, rctx.y, 0, &trace); + r = rgw_rados_operate(rctx.dpp, ref.ioctx, ref.obj.oid, &op, rctx.y, 0, &trace, &epoch); tracepoint(rgw_rados, operate_exit, req_id.c_str()); if (r < 0) { /* we can expect to get -ECANCELED if object was replaced under, or -ENOENT if was removed, or -EEXIST if it did not exist @@ -3259,7 +3259,6 @@ int RGWRados::Object::Write::_do_write_meta(uint64_t size, uint64_t accounted_si goto done_cancel; } - epoch = ioctx.get_last_version(); poolid = ioctx.get_id(); r = target->complete_atomic_modification(rctx.dpp, rctx.y); @@ -5870,7 +5869,8 @@ int RGWRados::Object::Delete::delete_obj(optional_yield y, const DoutPrefixProvi } auto& ioctx = ref.ioctx; - r = rgw_rados_operate(dpp, ioctx, ref.obj.oid, &op, y); + version_t epoch = 0; + r = rgw_rados_operate(dpp, ioctx, ref.obj.oid, &op, y, 0, nullptr, &epoch); /* raced with another operation, object state is indeterminate */ const bool need_invalidate = (r == -ECANCELED); @@ -5882,7 +5882,7 @@ int RGWRados::Object::Delete::delete_obj(optional_yield y, const DoutPrefixProvi tombstone_entry entry{*state}; obj_tombstone_cache->add(obj, entry); } - r = index_op.complete_del(dpp, poolid, ioctx.get_last_version(), state->mtime, params.remove_objs, y, log_op); + r = index_op.complete_del(dpp, poolid, epoch, state->mtime, params.remove_objs, y, log_op); int ret = target->complete_atomic_modification(dpp, y); if (ret < 0) { @@ -6603,7 +6603,8 @@ int RGWRados::set_attrs(const DoutPrefixProvider *dpp, RGWObjectCtx* octx, RGWBu struct timespec mtime_ts = real_clock::to_timespec(mtime); op.mtime2(&mtime_ts); auto& ioctx = ref.ioctx; - r = rgw_rados_operate(dpp, ioctx, ref.obj.oid, &op, y); + version_t epoch = 0; + r = rgw_rados_operate(dpp, ioctx, ref.obj.oid, &op, y, 0, nullptr, &epoch); if (state) { if (r >= 0) { ACLOwner owner; @@ -6634,7 +6635,6 @@ int RGWRados::set_attrs(const DoutPrefixProvider *dpp, RGWObjectCtx* octx, RGWBu iter != state->attrset.end()) { storage_class = rgw_bl_str(iter->second); } - uint64_t epoch = ioctx.get_last_version(); int64_t poolid = ioctx.get_id(); r = index_op.complete(dpp, poolid, epoch, state->size, state->accounted_size, mtime, etag, content_type, storage_class, owner, @@ -8799,12 +8799,7 @@ int RGWRados::raw_obj_stat(const DoutPrefixProvider *dpp, } bufferlist outbl; - r = rgw_rados_operate(dpp, ref.ioctx, ref.obj.oid, &op, &outbl, y); - - if (epoch) { - *epoch = ref.ioctx.get_last_version(); - } - + r = rgw_rados_operate(dpp, ref.ioctx, ref.obj.oid, &op, &outbl, y, 0, nullptr, epoch); if (r < 0) return r; @@ -9655,6 +9650,12 @@ int RGWRados::cls_bucket_list_ordered(const DoutPrefixProvider *dpp, num_entries << " total entries" << dendl; auto& ioctx = index_pool; + + // XXX: check_disk_state() relies on ioctx.get_last_version() but that + // returns 0 because CLSRGWIssueBucketList doesn't make any synchonous calls + rgw_bucket_entry_ver index_ver; + index_ver.pool = ioctx.get_id(); + std::map shard_list_results; cls_rgw_obj_key start_after_key(start_after.name, start_after.instance); r = CLSRGWIssueBucketList(ioctx, start_after_key, prefix, delimiter, @@ -9778,12 +9779,10 @@ int RGWRados::cls_bucket_list_ordered(const DoutPrefixProvider *dpp, /* there are uncommitted ops. We need to check the current * state, and if the tags are old we need to do clean-up as * well. */ - librados::IoCtx sub_ctx; - sub_ctx.dup(ioctx); ldout_bitx(bitx, dpp, 20) << "INFO: " << __func__ << " calling check_disk_state bucket=" << bucket_info.bucket << " entry=" << dirent.key << dendl_bitx; - r = check_disk_state(dpp, sub_ctx, bucket_info, dirent, dirent, + r = check_disk_state(dpp, bucket_info, index_ver, dirent, dirent, updates[tracker.oid_name], y); if (r < 0 && r != -ENOENT) { ldpp_dout(dpp, 0) << __func__ << @@ -10005,6 +10004,9 @@ int RGWRados::cls_bucket_list_unordered(const DoutPrefixProvider *dpp, } } + rgw_bucket_entry_ver index_ver; + index_ver.pool = ioctx.get_id(); + uint32_t count = 0u; std::map updates; rgw_obj_index_key last_added_entry; @@ -10019,7 +10021,7 @@ int RGWRados::cls_bucket_list_unordered(const DoutPrefixProvider *dpp, cls_rgw_bucket_list_op(op, marker, prefix, empty_delimiter, num_entries, list_versions, &result); - r = rgw_rados_operate(dpp, ioctx, oid, &op, nullptr, y); + r = rgw_rados_operate(dpp, ioctx, oid, &op, nullptr, y, 0, nullptr, &index_ver.epoch); if (r < 0) { ldpp_dout(dpp, 0) << "ERROR: " << __func__ << ": error in rgw_rados_operate (bucket list op), r=" << r << dendl; @@ -10036,12 +10038,10 @@ int RGWRados::cls_bucket_list_unordered(const DoutPrefixProvider *dpp, force_check) { /* there are uncommitted ops. We need to check the current state, * and if the tags are old we need to do cleanup as well. */ - librados::IoCtx sub_ctx; - sub_ctx.dup(ioctx); ldout_bitx(bitx, dpp, 20) << "INFO: " << __func__ << ": calling check_disk_state bucket=" << bucket_info.bucket << " entry=" << dirent.key << dendl_bitx; - r = check_disk_state(dpp, sub_ctx, bucket_info, dirent, dirent, updates[oid], y); + r = check_disk_state(dpp, bucket_info, index_ver, dirent, dirent, updates[oid], y); if (r < 0 && r != -ENOENT) { ldpp_dout(dpp, 0) << "ERROR: " << __func__ << ": error in check_disk_state, r=" << r << dendl; @@ -10273,8 +10273,8 @@ int RGWRados::remove_objs_from_index(const DoutPrefixProvider *dpp, } int RGWRados::check_disk_state(const DoutPrefixProvider *dpp, - librados::IoCtx io_ctx, RGWBucketInfo& bucket_info, + const rgw_bucket_entry_ver& index_ver, rgw_bucket_dir_entry& list_state, rgw_bucket_dir_entry& object, bufferlist& suggested_updates, @@ -10302,8 +10302,6 @@ int RGWRados::check_disk_state(const DoutPrefixProvider *dpp, ldpp_dout(dpp, 0) << "WARNING: generated locator (" << loc << ") is different from listed locator (" << list_state.locator << ")" << dendl; } - io_ctx.locator_set_key(list_state.locator); - RGWObjState *astate = NULL; RGWObjManifest *manifest = nullptr; RGWObjectCtx octx(this->driver); @@ -10324,8 +10322,7 @@ int RGWRados::check_disk_state(const DoutPrefixProvider *dpp, } // encode a suggested removal of that key - list_state.ver.epoch = io_ctx.get_last_version(); - list_state.ver.pool = io_ctx.get_id(); + list_state.ver = index_ver; ldout_bitx(bitx, dpp, 10) << "INFO: " << __func__ << ": encoding remove of " << list_state.key << " on suggested_updates" << dendl_bitx; cls_rgw_encode_suggestion(CEPH_RGW_REMOVE | suggest_flag, list_state, suggested_updates); return -ENOENT; diff --git a/src/rgw/driver/rados/rgw_rados.h b/src/rgw/driver/rados/rgw_rados.h index f95b6654a93df..e019de612386d 100644 --- a/src/rgw/driver/rados/rgw_rados.h +++ b/src/rgw/driver/rados/rgw_rados.h @@ -1642,8 +1642,8 @@ public: * will encode that info as a suggested update.) */ int check_disk_state(const DoutPrefixProvider *dpp, - librados::IoCtx io_ctx, RGWBucketInfo& bucket_info, + const rgw_bucket_entry_ver& index_ver, rgw_bucket_dir_entry& list_state, rgw_bucket_dir_entry& object, bufferlist& suggested_updates, diff --git a/src/rgw/driver/rados/rgw_tools.cc b/src/rgw/driver/rados/rgw_tools.cc index 0af353b866f83..f5cd193d815e5 100644 --- a/src/rgw/driver/rados/rgw_tools.cc +++ b/src/rgw/driver/rados/rgw_tools.cc @@ -198,36 +198,52 @@ int rgw_delete_system_obj(const DoutPrefixProvider *dpp, int rgw_rados_operate(const DoutPrefixProvider *dpp, librados::IoCtx& ioctx, const std::string& oid, librados::ObjectReadOperation *op, bufferlist* pbl, - optional_yield y, int flags, const jspan_context* trace_info) + optional_yield y, int flags, const jspan_context* trace_info, + version_t* pver) { // given a yield_context, call async_operate() to yield the coroutine instead // of blocking if (y) { auto& yield = y.get_yield_context(); boost::system::error_code ec; - auto bl = librados::async_operate( + auto [ver, bl] = librados::async_operate( yield, ioctx, oid, op, flags, trace_info, yield[ec]); if (pbl) { *pbl = std::move(bl); } + if (pver) { + *pver = ver; + } return -ec.value(); } maybe_warn_about_blocking(dpp); - return ioctx.operate(oid, op, nullptr, flags); + int r = ioctx.operate(oid, op, nullptr, flags); + if (pver) { + *pver = ioctx.get_last_version(); + } + return r; } int rgw_rados_operate(const DoutPrefixProvider *dpp, librados::IoCtx& ioctx, const std::string& oid, librados::ObjectWriteOperation *op, optional_yield y, - int flags, const jspan_context* trace_info) + int flags, const jspan_context* trace_info, version_t* pver) { if (y) { auto& yield = y.get_yield_context(); boost::system::error_code ec; - librados::async_operate(yield, ioctx, oid, op, flags, trace_info, yield[ec]); + version_t ver = librados::async_operate(yield, ioctx, oid, op, flags, + trace_info, yield[ec]); + if (pver) { + *pver = ver; + } return -ec.value(); } maybe_warn_about_blocking(dpp); - return ioctx.operate(oid, op, flags, trace_info); + int r = ioctx.operate(oid, op, flags, trace_info); + if (pver) { + *pver = ioctx.get_last_version(); + } + return r; } int rgw_rados_notify(const DoutPrefixProvider *dpp, librados::IoCtx& ioctx, const std::string& oid, @@ -237,8 +253,8 @@ int rgw_rados_notify(const DoutPrefixProvider *dpp, librados::IoCtx& ioctx, cons if (y) { auto& yield = y.get_yield_context(); boost::system::error_code ec; - auto reply = librados::async_notify(yield, ioctx, oid, - bl, timeout_ms, yield[ec]); + auto [ver, reply] = librados::async_notify(yield, ioctx, oid, + bl, timeout_ms, yield[ec]); if (pbl) { *pbl = std::move(reply); } diff --git a/src/rgw/driver/rados/rgw_tools.h b/src/rgw/driver/rados/rgw_tools.h index 257e513a9f7c1..016da256263f0 100644 --- a/src/rgw/driver/rados/rgw_tools.h +++ b/src/rgw/driver/rados/rgw_tools.h @@ -93,10 +93,12 @@ void rgw_filter_attrset(std::map& unfiltered_attrset, c /// perform the rados operation, using the yield context when given int rgw_rados_operate(const DoutPrefixProvider *dpp, librados::IoCtx& ioctx, const std::string& oid, librados::ObjectReadOperation *op, bufferlist* pbl, - optional_yield y, int flags = 0, const jspan_context* trace_info = nullptr); + optional_yield y, int flags = 0, const jspan_context* trace_info = nullptr, + version_t* pver = nullptr); int rgw_rados_operate(const DoutPrefixProvider *dpp, librados::IoCtx& ioctx, const std::string& oid, librados::ObjectWriteOperation *op, optional_yield y, - int flags = 0, const jspan_context* trace_info = nullptr); + int flags = 0, const jspan_context* trace_info = nullptr, + version_t* pver = nullptr); int rgw_rados_notify(const DoutPrefixProvider *dpp, librados::IoCtx& ioctx, const std::string& oid, bufferlist& bl, uint64_t timeout_ms, bufferlist* pbl, optional_yield y); diff --git a/src/rgw/rgw_aio.cc b/src/rgw/rgw_aio.cc index 7fba58ad63ff7..d2e56c5729843 100644 --- a/src/rgw/rgw_aio.cc +++ b/src/rgw/rgw_aio.cc @@ -76,12 +76,12 @@ struct Handler { librados::IoCtx ctx; AioResult& r; // write callback - void operator()(boost::system::error_code ec) const { + void operator()(boost::system::error_code ec, version_t) const { r.result = -ec.value(); throttle->put(r); } // read callback - void operator()(boost::system::error_code ec, bufferlist bl) const { + void operator()(boost::system::error_code ec, version_t, bufferlist bl) const { r.result = -ec.value(); r.data = std::move(bl); throttle->put(r); diff --git a/src/rgw/services/svc_sys_obj_core.cc b/src/rgw/services/svc_sys_obj_core.cc index 397709c5d9997..cdbbf353832e6 100644 --- a/src/rgw/services/svc_sys_obj_core.cc +++ b/src/rgw/services/svc_sys_obj_core.cc @@ -169,21 +169,21 @@ int RGWSI_SysObj_Core::read(const DoutPrefixProvider *dpp, } } - rgw_rados_ref rados_obj; - int r = get_rados_obj(dpp, zone_svc, obj, &rados_obj); + rgw_rados_ref ref; + int r = get_rados_obj(dpp, zone_svc, obj, &ref); if (r < 0) { ldpp_dout(dpp, 20) << "get_rados_obj() on obj=" << obj << " returned " << r << dendl; return r; } - r = rados_obj.operate(dpp, &op, nullptr, y); + + version_t op_ver = 0; + r = rgw_rados_operate(dpp, ref.ioctx, obj.oid, &op, nullptr, y, 0, nullptr, &op_ver); if (r < 0) { ldpp_dout(dpp, 20) << "rados_obj.operate() r=" << r << " bl.length=" << bl->length() << dendl; return r; } ldpp_dout(dpp, 20) << "rados_obj.operate() r=" << r << " bl.length=" << bl->length() << dendl; - uint64_t op_ver = rados_obj.ioctx.get_last_version(); - if (read_state.last_ver > 0 && read_state.last_ver != op_ver) { ldpp_dout(dpp, 5) << "raced with an object write, abort" << dendl;