From 7117aa4ecaf0b619b527b2783fa1d09b11f3dd55 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Wed, 8 Nov 2017 12:31:28 -0500 Subject: [PATCH] librbd: simplify interface between object IO and copyup state machines The initial copyup was not receiving a write hint and the code for hints was duplicated multiple times. Additionally, the object map state should match the last executed write op. Signed-off-by: Jason Dillaman --- src/librbd/io/CopyupRequest.cc | 55 ++++++------ src/librbd/io/CopyupRequest.h | 6 +- src/librbd/io/ObjectRequest.cc | 87 ++++++++++--------- src/librbd/io/ObjectRequest.h | 150 ++++++++++++++------------------- 4 files changed, 145 insertions(+), 153 deletions(-) diff --git a/src/librbd/io/CopyupRequest.cc b/src/librbd/io/CopyupRequest.cc index c2d36abf37e..cb7cbfb93bf 100644 --- a/src/librbd/io/CopyupRequest.cc +++ b/src/librbd/io/CopyupRequest.cc @@ -99,7 +99,7 @@ CopyupRequest::~CopyupRequest() { } template -void CopyupRequest::append_request(ObjectRequest *req) { +void CopyupRequest::append_request(AbstractObjectWriteRequest *req) { ldout(m_ictx->cct, 20) << req << dendl; m_pending_requests.push_back(req); } @@ -107,10 +107,10 @@ void CopyupRequest::append_request(ObjectRequest *req) { template void CopyupRequest::complete_requests(int r) { while (!m_pending_requests.empty()) { - vector *>::iterator it = m_pending_requests.begin(); - ObjectRequest<> *req = *it; + auto it = m_pending_requests.begin(); + auto req = *it; ldout(m_ictx->cct, 20) << "completing request " << req << dendl; - req->complete(r); + req->handle_copyup(r); m_pending_requests.erase(it); } } @@ -140,6 +140,8 @@ bool CopyupRequest::send_copyup() { copyup_op.exec("rbd", "copyup", m_copyup_data); m_copyup_data.clear(); + ObjectRequest::add_write_hint(*m_ictx, ©up_op); + // send only the copyup request with a blank snapshot context so that // all snapshots are detected from the parent for this object. If // this is a CoW request, a second request will be created for the @@ -165,11 +167,10 @@ bool CopyupRequest::send_copyup() { write_op.exec("rbd", "copyup", m_copyup_data); // merge all pending write ops into this single RADOS op - for (size_t i=0; i *req = m_pending_requests[i]; + ObjectRequest::add_write_hint(*m_ictx, &write_op); + for (auto req : m_pending_requests) { ldout(m_ictx->cct, 20) << "add_copyup_ops " << req << dendl; - bool set_hints = (i == 0); - req->add_copyup_ops(&write_op, set_hints); + req->add_copyup_ops(&write_op); } m_pending_copyups++; @@ -187,15 +188,22 @@ bool CopyupRequest::send_copyup() { template bool CopyupRequest::is_copyup_required() { - bool noop = true; - for (const ObjectRequest<> *req : m_pending_requests) { - if (!req->is_op_payload_empty()) { - noop = false; - break; - } + bool copy_on_read = m_pending_requests.empty(); + if (copy_on_read) { + // always force a copyup if CoR enabled + return true; + } + + if (!m_copyup_data.is_zero()) { + return true; } - return (m_copyup_data.is_zero() && noop); + for (auto req : m_pending_requests) { + if (!req->is_empty_write_op()) { + return true; + } + } + return false; } template @@ -235,7 +243,7 @@ bool CopyupRequest::should_complete(int r) ldout(cct, 20) << "READ_FROM_PARENT" << dendl; remove_from_list(); if (r >= 0 || r == -ENOENT) { - if (is_copyup_required()) { + if (!is_copyup_required()) { ldout(cct, 20) << "nop, skipping" << dendl; return true; } @@ -319,23 +327,20 @@ bool CopyupRequest::send_object_map_head() { } bool may_update = false; - uint8_t new_state, current_state; + uint8_t new_state; + uint8_t current_state = (*m_ictx->object_map)[m_object_no]; - vector *>::reverse_iterator r_it = m_pending_requests.rbegin(); - for (; r_it != m_pending_requests.rend(); ++r_it) { - ObjectRequest<> *req = *r_it; - if (!req->pre_object_map_update(&new_state)) { - continue; - } + auto r_it = m_pending_requests.rbegin(); + if (r_it != m_pending_requests.rend()) { + auto req = *r_it; + new_state = req->get_pre_write_object_map_state(); - current_state = (*m_ictx->object_map)[m_object_no]; ldout(cct, 20) << req->get_op_type() << " object no " << m_object_no << " current state " << stringify(static_cast(current_state)) << " new state " << stringify(static_cast(new_state)) << dendl; may_update = true; - break; } if (may_update && (new_state != current_state) && diff --git a/src/librbd/io/CopyupRequest.h b/src/librbd/io/CopyupRequest.h index 25f6241dd3d..a45a48cd61b 100644 --- a/src/librbd/io/CopyupRequest.h +++ b/src/librbd/io/CopyupRequest.h @@ -25,7 +25,7 @@ struct ImageCtx; namespace io { struct AioCompletion; -template class ObjectRequest; +template class AbstractObjectWriteRequest; template class CopyupRequest { @@ -41,7 +41,7 @@ public: Extents &&image_extents, const ZTracer::Trace &parent_trace); ~CopyupRequest(); - void append_request(ObjectRequest *req); + void append_request(AbstractObjectWriteRequest *req); void send(); @@ -93,7 +93,7 @@ private: State m_state; ceph::bufferlist m_copyup_data; - std::vector *> m_pending_requests; + std::vector *> m_pending_requests; std::atomic m_pending_copyups { 0 }; AsyncOperation m_async_op; diff --git a/src/librbd/io/ObjectRequest.cc b/src/librbd/io/ObjectRequest.cc index 12c7afd85b0..b74b73d02f0 100644 --- a/src/librbd/io/ObjectRequest.cc +++ b/src/librbd/io/ObjectRequest.cc @@ -149,6 +149,15 @@ ObjectRequest::ObjectRequest(I *ictx, const std::string &oid, } } +template +void ObjectRequest::add_write_hint(I& image_ctx, + librados::ObjectWriteOperation *wr) { + if (image_ctx.enable_alloc_hint) { + wr->set_alloc_hint(image_ctx.get_object_size(), + image_ctx.get_object_size()); + } +} + template void ObjectRequest::complete(int r) { @@ -449,6 +458,26 @@ AbstractObjectWriteRequest::AbstractObjectWriteRequest( this->compute_parent_extents(&this->m_parent_extents); } +template +void AbstractObjectWriteRequest::add_write_hint( + librados::ObjectWriteOperation *wr) { + I *image_ctx = this->m_ictx; + RWLock::RLocker snap_locker(image_ctx->snap_lock); + if (image_ctx->object_map == nullptr || !this->m_object_may_exist) { + ObjectRequest::add_write_hint(*image_ctx, wr); + } +} + +template +void AbstractObjectWriteRequest::handle_copyup(int r) { + I *image_ctx = this->m_ictx; + ldout(image_ctx->cct, 20) << "r=" << r << dendl; + + // TODO + assert(m_state == LIBRBD_AIO_WRITE_COPYUP); + this->complete(r); +} + template void AbstractObjectWriteRequest::guard_write() { @@ -542,11 +571,11 @@ void AbstractObjectWriteRequest::send() { { RWLock::RLocker snap_lock(image_ctx->snap_lock); if (image_ctx->object_map == nullptr) { - m_object_exist = true; + m_object_may_exist = true; } else { // should have been flushed prior to releasing lock assert(image_ctx->exclusive_lock->is_lock_owner()); - m_object_exist = image_ctx->object_map->object_may_exist( + m_object_may_exist = image_ctx->object_map->object_may_exist( this->m_object_no); } } @@ -562,8 +591,7 @@ void AbstractObjectWriteRequest::send_pre_object_map_update() { { RWLock::RLocker snap_lock(image_ctx->snap_lock); if (image_ctx->object_map != nullptr) { - uint8_t new_state; - this->pre_object_map_update(&new_state); + uint8_t new_state = this->get_pre_write_object_map_state(); RWLock::WLocker object_map_locker(image_ctx->object_map_lock); ldout(image_ctx->cct, 20) << this->m_oid << " " << this->m_object_off << "~" << this->m_object_len << dendl; @@ -612,9 +640,9 @@ void AbstractObjectWriteRequest::send_write() { I *image_ctx = this->m_ictx; ldout(image_ctx->cct, 20) << this->m_oid << " " << this->m_object_off << "~" << this->m_object_len << " object exist " - << m_object_exist << dendl; + << m_object_may_exist << dendl; - if (!m_object_exist && this->has_parent()) { + if (!m_object_may_exist && this->has_parent()) { m_state = LIBRBD_AIO_WRITE_GUARD; handle_write_guard(); } else { @@ -659,7 +687,8 @@ void AbstractObjectWriteRequest::send_write_op() guard_write(); } - add_write_ops(&m_write, true); + add_write_hint(&m_write); + add_write_ops(&m_write); assert(m_write.size() != 0); librados::AioCompletion *rados_completion = @@ -693,15 +722,8 @@ void AbstractObjectWriteRequest::handle_write_guard() } template -void ObjectWriteRequest::add_write_ops(librados::ObjectWriteOperation *wr, - bool set_hints) { +void ObjectWriteRequest::add_write_ops(librados::ObjectWriteOperation *wr) { I *image_ctx = this->m_ictx; - RWLock::RLocker snap_locker(image_ctx->snap_lock); - if (set_hints && image_ctx->enable_alloc_hint && - (image_ctx->object_map == nullptr || !this->m_object_exist)) { - wr->set_alloc_hint(image_ctx->get_object_size(), - image_ctx->get_object_size()); - } if (this->m_object_off == 0 && this->m_object_len == image_ctx->get_object_size()) { @@ -719,7 +741,7 @@ void ObjectWriteRequest::send_write() { this->m_object_len == image_ctx->get_object_size()); ldout(image_ctx->cct, 20) << this->m_oid << " " << this->m_object_off << "~" << this->m_object_len - << " object exist " << this->m_object_exist + << " object exist " << this->m_object_may_exist << " write_full " << write_full << dendl; if (write_full && !this->has_parent()) { this->m_guard = false; @@ -742,8 +764,8 @@ template void ObjectRemoveRequest::send_write() { I *image_ctx = this->m_ictx; ldout(image_ctx->cct, 20) << this->m_oid << " remove " << " object exist " - << this->m_object_exist << dendl; - if (!this->m_object_exist && !this->has_parent()) { + << this->m_object_may_exist << dendl; + if (!this->m_object_may_exist && !this->has_parent()) { this->m_state = AbstractObjectWriteRequest::LIBRBD_AIO_WRITE_FLAT; Context *ctx = util::create_context_callback>(this); image_ctx->op_work_queue->queue(ctx, 0); @@ -756,9 +778,9 @@ template void ObjectTruncateRequest::send_write() { I *image_ctx = this->m_ictx; ldout(image_ctx->cct, 20) << this->m_oid << " truncate " << this->m_object_off - << " object exist " << this->m_object_exist + << " object exist " << this->m_object_may_exist << dendl; - if (!this->m_object_exist && !this->has_parent()) { + if (!this->m_object_may_exist && !this->has_parent()) { this->m_state = AbstractObjectWriteRequest::LIBRBD_AIO_WRITE_FLAT; Context *ctx = util::create_context_callback>(this); image_ctx->op_work_queue->queue(ctx, 0); @@ -772,9 +794,9 @@ void ObjectZeroRequest::send_write() { I *image_ctx = this->m_ictx; ldout(image_ctx->cct, 20) << this->m_oid << " zero " << this->m_object_off << "~" << this->m_object_len - << " object exist " << this->m_object_exist + << " object exist " << this->m_object_may_exist << dendl; - if (!this->m_object_exist && !this->has_parent()) { + if (!this->m_object_may_exist && !this->has_parent()) { this->m_state = AbstractObjectWriteRequest::LIBRBD_AIO_WRITE_FLAT; Context *ctx = util::create_context_callback>(this); image_ctx->op_work_queue->queue(ctx, 0); @@ -785,15 +807,7 @@ void ObjectZeroRequest::send_write() { template void ObjectWriteSameRequest::add_write_ops( - librados::ObjectWriteOperation *wr, bool set_hints) { - I *image_ctx = this->m_ictx; - RWLock::RLocker snap_locker(image_ctx->snap_lock); - if (set_hints && image_ctx->enable_alloc_hint && - (image_ctx->object_map == nullptr || !this->m_object_exist)) { - wr->set_alloc_hint(image_ctx->get_object_size(), - image_ctx->get_object_size()); - } - + librados::ObjectWriteOperation *wr) { wr->writesame(this->m_object_off, this->m_object_len, m_write_data); wr->set_op_flags2(m_op_flags); } @@ -815,15 +829,8 @@ void ObjectWriteSameRequest::send_write() { template void ObjectCompareAndWriteRequest::add_write_ops( - librados::ObjectWriteOperation *wr, bool set_hints) { + librados::ObjectWriteOperation *wr) { I *image_ctx = this->m_ictx; - RWLock::RLocker snap_locker(image_ctx->snap_lock); - - if (set_hints && image_ctx->enable_alloc_hint && - (image_ctx->object_map == nullptr || !this->m_object_exist)) { - wr->set_alloc_hint(image_ctx->get_object_size(), - image_ctx->get_object_size()); - } // add cmpext ops wr->cmpext(this->m_object_off, m_cmp_bl, nullptr); @@ -844,7 +851,7 @@ void ObjectCompareAndWriteRequest::send_write() { this->m_object_len == image_ctx->get_object_size()); ldout(image_ctx->cct, 20) << this->m_oid << " " << this->m_object_off << "~" << this->m_object_len - << " object exist " << this->m_object_exist + << " object exist " << this->m_object_may_exist << " write_full " << write_full << dendl; if (write_full && !this->has_parent()) { this->m_guard = false; diff --git a/src/librbd/io/ObjectRequest.h b/src/librbd/io/ObjectRequest.h index 93a373c40df..b36ac0b0e52 100644 --- a/src/librbd/io/ObjectRequest.h +++ b/src/librbd/io/ObjectRequest.h @@ -105,9 +105,8 @@ public: m_trace.event("finish"); } - virtual void add_copyup_ops(librados::ObjectWriteOperation *wr, - bool set_hints) { - }; + static void add_write_hint(ImageCtxT& image_ctx, + librados::ObjectWriteOperation *wr); virtual void complete(int r); @@ -118,12 +117,7 @@ public: return m_has_parent; } - virtual bool is_op_payload_empty() const { - return false; - } - virtual const char *get_op_type() const = 0; - virtual bool pre_object_map_update(uint8_t *new_state) = 0; protected: bool compute_parent_extents(Extents *parent_extents); @@ -186,10 +180,6 @@ public: return "read"; } - bool pre_object_map_update(uint8_t *new_state) override { - return false; - } - private: /** * @verbatim @@ -245,12 +235,20 @@ public: const ZTracer::Trace &parent_trace, Context *completion); - void add_copyup_ops(librados::ObjectWriteOperation *wr, - bool set_hints) override - { - add_write_ops(wr, set_hints); + virtual bool is_empty_write_op() const { + return false; + } + + virtual uint8_t get_pre_write_object_map_state() const { + return OBJECT_EXISTS; + } + + virtual void add_copyup_ops(librados::ObjectWriteOperation *wr) { + add_write_ops(wr); } + void handle_copyup(int r); + bool should_complete(int r) override; void send() override; @@ -304,11 +302,12 @@ protected: librados::ObjectWriteOperation m_write; uint64_t m_snap_seq; std::vector m_snaps; - bool m_object_exist = false; + bool m_object_may_exist = false; bool m_guard = true; - virtual void add_write_ops(librados::ObjectWriteOperation *wr, - bool set_hints) = 0; + virtual void add_write_hint(librados::ObjectWriteOperation *wr); + virtual void add_write_ops(librados::ObjectWriteOperation *wr) = 0; + virtual void guard_write(); virtual bool post_object_map_update() { return false; @@ -338,7 +337,7 @@ public: m_write_data(data), m_op_flags(op_flags) { } - bool is_op_payload_empty() const override { + bool is_empty_write_op() const override { return (m_write_data.length() == 0); } @@ -346,14 +345,8 @@ public: return "write"; } - bool pre_object_map_update(uint8_t *new_state) override { - *new_state = OBJECT_EXISTS; - return true; - } - protected: - void add_write_ops(librados::ObjectWriteOperation *wr, - bool set_hints) override; + void add_write_ops(librados::ObjectWriteOperation *wr) override; void send_write() override; @@ -370,8 +363,12 @@ public: const ZTracer::Trace &parent_trace, Context *completion) : AbstractObjectWriteRequest(ictx, oid, object_no, 0, 0, snapc, true, "remove", parent_trace, - completion), - m_object_state(OBJECT_NONEXISTENT) { + completion) { + if (this->has_parent()) { + m_object_state = OBJECT_EXISTS; + } else { + m_object_state = OBJECT_PENDING; + } } const char* get_op_type() const override { @@ -381,14 +378,21 @@ public: return "remove"; } - bool pre_object_map_update(uint8_t *new_state) override { + uint8_t get_pre_write_object_map_state() const override { + return m_object_state; + } + +protected: + void add_write_hint(librados::ObjectWriteOperation *wr) override { + // no hint for remove + } + + void add_write_ops(librados::ObjectWriteOperation *wr) override { if (this->has_parent()) { - m_object_state = OBJECT_EXISTS; + wr->truncate(0); } else { - m_object_state = OBJECT_PENDING; + wr->remove(); } - *new_state = m_object_state; - return true; } bool post_object_map_update() override { @@ -401,16 +405,6 @@ public: void guard_write() override; void send_write() override; -protected: - void add_write_ops(librados::ObjectWriteOperation *wr, - bool set_hints) override { - if (this->has_parent()) { - wr->truncate(0); - } else { - wr->remove(); - } - } - private: uint8_t m_object_state; }; @@ -433,21 +427,23 @@ public: return "remove (trim)"; } - bool pre_object_map_update(uint8_t *new_state) override { - *new_state = OBJECT_PENDING; - return true; + uint8_t get_pre_write_object_map_state() const override { + return OBJECT_PENDING; } - bool post_object_map_update() override { - return m_post_object_map_update; +protected: + void add_write_hint(librados::ObjectWriteOperation *wr) override { + // no hint for remove } -protected: - void add_write_ops(librados::ObjectWriteOperation *wr, - bool set_hints) override { + void add_write_ops(librados::ObjectWriteOperation *wr) override { wr->remove(); } + bool post_object_map_update() override { + return m_post_object_map_update; + } + private: bool m_post_object_map_update; }; @@ -468,21 +464,22 @@ public: return "truncate"; } - bool pre_object_map_update(uint8_t *new_state) override { - if (!this->m_object_exist && !this->has_parent()) - *new_state = OBJECT_NONEXISTENT; - else - *new_state = OBJECT_EXISTS; - return true; + uint8_t get_pre_write_object_map_state() const override { + if (!this->m_object_may_exist && !this->has_parent()) + return OBJECT_NONEXISTENT; + return OBJECT_EXISTS; } - void send_write() override; - protected: - void add_write_ops(librados::ObjectWriteOperation *wr, - bool set_hints) override { + void add_write_hint(librados::ObjectWriteOperation *wr) override { + // no hint for truncate + } + + void add_write_ops(librados::ObjectWriteOperation *wr) override { wr->truncate(this->m_object_off); } + + void send_write() override; }; template @@ -501,18 +498,12 @@ public: return "zero"; } - bool pre_object_map_update(uint8_t *new_state) override { - *new_state = OBJECT_EXISTS; - return true; - } - - void send_write() override; - protected: - void add_write_ops(librados::ObjectWriteOperation *wr, - bool set_hints) override { + void add_write_ops(librados::ObjectWriteOperation *wr) override { wr->zero(this->m_object_off, this->m_object_len); } + + void send_write() override; }; template @@ -535,14 +526,8 @@ public: return "writesame"; } - bool pre_object_map_update(uint8_t *new_state) override { - *new_state = OBJECT_EXISTS; - return true; - } - protected: - void add_write_ops(librados::ObjectWriteOperation *wr, - bool set_hints) override; + void add_write_ops(librados::ObjectWriteOperation *wr) override; void send_write() override; @@ -574,15 +559,10 @@ public: return "compare_and_write"; } - bool pre_object_map_update(uint8_t *new_state) override { - *new_state = OBJECT_EXISTS; - return true; - } - void complete(int r) override; + protected: - void add_write_ops(librados::ObjectWriteOperation *wr, - bool set_hints) override; + void add_write_ops(librados::ObjectWriteOperation *wr) override; void send_write() override; -- 2.39.5