From e0622586fb6987eabd909f36c2454751f0beed9c Mon Sep 17 00:00:00 2001 From: Pritha Srivastava Date: Wed, 27 Sep 2023 11:39:41 +0530 Subject: [PATCH] rgw/cache: saving dpp in set_client_cb() instead of flush_write_part() so that the correct value is available everywhere in handle_data(). Saving dpp value in flush_last_part() caused a crash in rgw in handle_data(). Signed-off-by: Pritha Srivastava --- src/rgw/driver/d4n/rgw_sal_d4n.cc | 45 +++++++++++++++---------------- src/rgw/driver/d4n/rgw_sal_d4n.h | 9 +++---- 2 files changed, 25 insertions(+), 29 deletions(-) diff --git a/src/rgw/driver/d4n/rgw_sal_d4n.cc b/src/rgw/driver/d4n/rgw_sal_d4n.cc index a9404826d69..3a264eae350 100644 --- a/src/rgw/driver/d4n/rgw_sal_d4n.cc +++ b/src/rgw/driver/d4n/rgw_sal_d4n.cc @@ -482,7 +482,7 @@ int D4NFilterObject::D4NFilterReadOp::iterate(const DoutPrefixProvider* dpp, int ldpp_dout(dpp, 20) << "D4NFilterObject::iterate:: " << "oid: " << oid << " ofs: " << ofs << " end: " << end << dendl; this->client_cb = cb; - this->cb->set_client_cb(cb); // what's this for? -Sam + this->cb->set_client_cb(cb, dpp); // what's this for? -Sam /* This algorithm stores chunks for ranged requests also in the cache, which might be smaller than obj_max_req_size One simplification could be to overwrite the smaller chunks with a bigger chunk of obj_max_req_size, and to serve requests for smaller @@ -620,7 +620,7 @@ int D4NFilterObject::D4NFilterReadOp::iterate(const DoutPrefixProvider* dpp, int return r; } - return this->cb->flush_last_part(dpp); + return this->cb->flush_last_part(); /* / Execute cache replacement policy / @@ -732,9 +732,8 @@ int D4NFilterObject::D4NFilterReadOp::iterate(const DoutPrefixProvider* dpp, int return next->iterate(dpp, ofs, end, cb, y); */ } -int D4NFilterObject::D4NFilterReadOp::D4NFilterGetCB::flush_last_part(const DoutPrefixProvider* dpp) +int D4NFilterObject::D4NFilterReadOp::D4NFilterGetCB::flush_last_part() { - save_dpp = dpp; last_part = true; return handle_data(bl_rem, 0, bl_rem.length()); } @@ -764,47 +763,45 @@ int D4NFilterObject::D4NFilterReadOp::D4NFilterGetCB::handle_data(bufferlist& bl std::string oid = this->oid + "_" + std::to_string(ofs) + "_" + std::to_string(bl_len); block.size = bl.length(); block.blockId = ofs; - uint64_t freeSpace = filter->get_cache_driver()->get_free_space(save_dpp); + uint64_t freeSpace = filter->get_cache_driver()->get_free_space(dpp); while(freeSpace < block.size) { - freeSpace += filter->get_policy_driver()->cachePolicy->eviction(save_dpp, filter->get_cache_driver()); + freeSpace += filter->get_policy_driver()->cachePolicy->eviction(dpp, filter->get_cache_driver()); } - if (filter->get_cache_driver()->put_async(save_dpp, oid, bl, bl.length(), source->get_attrs()) == 0) { - filter->get_policy_driver()->cachePolicy->update(save_dpp, oid, ofs, bl.length(), filter->get_cache_driver()); + if (filter->get_cache_driver()->put_async(dpp, oid, bl, bl.length(), source->get_attrs()) == 0) { + filter->get_policy_driver()->cachePolicy->update(dpp, oid, ofs, bl.length(), filter->get_cache_driver()); /* Store block in directory */ if (!blockDir->exist_key(oid)) { int ret = blockDir->set_value(&block); if (ret < 0) { - ldpp_dout(save_dpp, 0) << "D4N Filter: Block directory set operation failed." << dendl; + ldpp_dout(dpp, 0) << "D4N Filter: Block directory set operation failed." << dendl; return ret; } else { - ldpp_dout(save_dpp, 20) << "D4N Filter: Block directory set operation succeeded." << dendl; + ldpp_dout(dpp, 20) << "D4N Filter: Block directory set operation succeeded." << dendl; } } } - filter->get_policy_driver()->cachePolicy->update(save_dpp, oid, ofs, bl.length(), filter->get_cache_driver()); } else if (bl.length() == rgw_get_obj_max_req_size && bl_rem.length() == 0) { // if bl is the same size as rgw_get_obj_max_req_size, write it to cache std::string oid = this->oid + "_" + std::to_string(ofs) + "_" + std::to_string(bl_len); ofs += bl_len; block.blockId = ofs; block.size = bl.length(); - uint64_t freeSpace = filter->get_cache_driver()->get_free_space(save_dpp); + uint64_t freeSpace = filter->get_cache_driver()->get_free_space(dpp); while(freeSpace < block.size) { - freeSpace += filter->get_policy_driver()->cachePolicy->eviction(save_dpp, filter->get_cache_driver()); + freeSpace += filter->get_policy_driver()->cachePolicy->eviction(dpp, filter->get_cache_driver()); } - if (filter->get_cache_driver()->put_async(save_dpp, oid, bl, bl.length(), source->get_attrs()) == 0) { - filter->get_policy_driver()->cachePolicy->update(save_dpp, oid, ofs, bl.length(), filter->get_cache_driver()); + if (filter->get_cache_driver()->put_async(dpp, oid, bl, bl.length(), source->get_attrs()) == 0) { + filter->get_policy_driver()->cachePolicy->update(dpp, oid, ofs, bl.length(), filter->get_cache_driver()); /* Store block in directory */ if (!blockDir->exist_key(oid)) { int ret = blockDir->set_value(&block); if (ret < 0) { - ldpp_dout(save_dpp, 0) << "D4N Filter: Block directory set operation failed." << dendl; + ldpp_dout(dpp, 0) << "D4N Filter: Block directory set operation failed." << dendl; return ret; } else { - ldpp_dout(save_dpp, 20) << "D4N Filter: Block directory set operation succeeded." << dendl; + ldpp_dout(dpp, 20) << "D4N Filter: Block directory set operation succeeded." << dendl; } } } - filter->get_policy_driver()->cachePolicy->update(save_dpp, oid, ofs, bl.length(), filter->get_cache_driver()); } else { //copy data from incoming bl to bl_rem till it is rgw_get_obj_max_req_size, and then write it to cache uint64_t rem_space = rgw_get_obj_max_req_size - bl_rem.length(); uint64_t len_to_copy = rem_space > bl.length() ? bl.length() : rem_space; @@ -818,20 +815,20 @@ int D4NFilterObject::D4NFilterReadOp::D4NFilterGetCB::handle_data(bufferlist& bl ofs += bl_rem.length(); block.blockId = ofs; block.size = bl_rem.length(); - uint64_t freeSpace = filter->get_cache_driver()->get_free_space(save_dpp); + uint64_t freeSpace = filter->get_cache_driver()->get_free_space(dpp); while(freeSpace < block.size) { - freeSpace += filter->get_policy_driver()->cachePolicy->eviction(save_dpp, filter->get_cache_driver()); + freeSpace += filter->get_policy_driver()->cachePolicy->eviction(dpp, filter->get_cache_driver()); } - if (filter->get_cache_driver()->put_async(save_dpp, oid, bl_rem, bl_rem.length(), source->get_attrs()) == 0) { - filter->get_policy_driver()->cachePolicy->update(save_dpp, oid, ofs, bl_rem.length(), filter->get_cache_driver()); + if (filter->get_cache_driver()->put_async(dpp, oid, bl_rem, bl_rem.length(), source->get_attrs()) == 0) { + filter->get_policy_driver()->cachePolicy->update(dpp, oid, ofs, bl_rem.length(), filter->get_cache_driver()); /* Store block in directory */ if (!blockDir->exist_key(oid)) { int ret = blockDir->set_value(&block); if (ret < 0) { - ldpp_dout(save_dpp, 0) << "D4N Filter: Block directory set operation failed." << dendl; + ldpp_dout(dpp, 0) << "D4N Filter: Block directory set operation failed." << dendl; return ret; } else { - ldpp_dout(save_dpp, 20) << "D4N Filter: Block directory set operation succeeded." << dendl; + ldpp_dout(dpp, 20) << "D4N Filter: Block directory set operation succeeded." << dendl; } } } diff --git a/src/rgw/driver/d4n/rgw_sal_d4n.h b/src/rgw/driver/d4n/rgw_sal_d4n.h index 2114ce3e578..c5961cfb078 100644 --- a/src/rgw/driver/d4n/rgw_sal_d4n.h +++ b/src/rgw/driver/d4n/rgw_sal_d4n.h @@ -107,18 +107,17 @@ class D4NFilterObject : public FilterObject { bool last_part{false}; std::mutex d4n_get_data_lock; bool write_to_cache{true}; + const DoutPrefixProvider* dpp; public: D4NFilterGetCB(D4NFilterDriver* _filter, std::string& _oid, D4NFilterObject* _source) : filter(_filter), oid(_oid), - source(_source) {} - - const DoutPrefixProvider* save_dpp; + source(_source) {} int handle_data(bufferlist& bl, off_t bl_ofs, off_t bl_len) override; - void set_client_cb(RGWGetDataCB* client_cb) { this->client_cb = client_cb;} + void set_client_cb(RGWGetDataCB* client_cb, const DoutPrefixProvider* dpp) { this->client_cb = client_cb; this->dpp = dpp;} void set_ofs(uint64_t ofs) { this->ofs = ofs; } - int flush_last_part(const DoutPrefixProvider* dpp); + int flush_last_part(); void bypass_cache_write() { this->write_to_cache = false; } }; -- 2.39.5