From f931066cf5eb44d6490fee08e8ad3be00161a1df Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Wed, 6 Apr 2016 17:18:33 -0400 Subject: [PATCH] journal: Future does not require metadata shared pointer Avoid keeping the metadata object alive through the lifespan of the future. Callers can expect to receive a re-entrant callback if the future is already safe and flush/wait is invoked. Fixes: http://tracker.ceph.com/issues/15364 Signed-off-by: Jason Dillaman --- src/journal/FutureImpl.cc | 14 +++---- src/journal/FutureImpl.h | 6 +-- src/journal/JournalRecorder.cc | 3 +- src/test/journal/test_FutureImpl.cc | 43 ++++++++++--------- src/test/journal/test_ObjectRecorder.cc | 55 +++++++++---------------- 5 files changed, 49 insertions(+), 72 deletions(-) diff --git a/src/journal/FutureImpl.cc b/src/journal/FutureImpl.cc index 11eda4456616c..aebfe12e31002 100644 --- a/src/journal/FutureImpl.cc +++ b/src/journal/FutureImpl.cc @@ -2,15 +2,14 @@ // vim: ts=8 sw=2 smarttab #include "journal/FutureImpl.h" -#include "journal/JournalMetadata.h" #include "journal/Utils.h" namespace journal { -FutureImpl::FutureImpl(JournalMetadataPtr journal_metadata, uint64_t tag_tid, - uint64_t entry_tid, uint64_t commit_tid) - : RefCountedObject(NULL, 0), m_journal_metadata(journal_metadata), - m_tag_tid(tag_tid), m_entry_tid(entry_tid), m_commit_tid(commit_tid), +FutureImpl::FutureImpl(uint64_t tag_tid, uint64_t entry_tid, + uint64_t commit_tid) + : RefCountedObject(NULL, 0), m_tag_tid(tag_tid), m_entry_tid(entry_tid), + m_commit_tid(commit_tid), m_lock(utils::unique_lock_name("FutureImpl::m_lock", this)), m_safe(false), m_consistent(false), m_return_value(0), m_flush_state(FLUSH_STATE_NONE), m_consistent_ack(this) { @@ -51,7 +50,7 @@ void FutureImpl::flush(Context *on_safe) { } if (complete && on_safe != NULL) { - m_journal_metadata->queue(on_safe, m_return_value); + on_safe->complete(m_return_value); } else if (flush_handler) { // attached to journal object -- instruct it to flush all entries through // this one. possible to become detached while lock is released, so flush @@ -69,7 +68,8 @@ void FutureImpl::wait(Context *on_safe) { return; } } - m_journal_metadata->queue(on_safe, m_return_value); + + on_safe->complete(m_return_value); } bool FutureImpl::is_complete() const { diff --git a/src/journal/FutureImpl.h b/src/journal/FutureImpl.h index 0a9eba58d56d1..5c11c4bb10500 100644 --- a/src/journal/FutureImpl.h +++ b/src/journal/FutureImpl.h @@ -18,7 +18,6 @@ class Context; namespace journal { class FutureImpl; -class JournalMetadata; typedef boost::intrusive_ptr FutureImplPtr; class FutureImpl : public RefCountedObject, boost::noncopyable { @@ -29,11 +28,9 @@ public: virtual void get() = 0; virtual void put() = 0; }; - typedef boost::intrusive_ptr JournalMetadataPtr; typedef boost::intrusive_ptr FlushHandlerPtr; - FutureImpl(JournalMetadataPtr journal_metadata, uint64_t tag_tid, - uint64_t entry_tid, uint64_t commit_tid); + FutureImpl(uint64_t tag_tid, uint64_t entry_tid, uint64_t commit_tid); void init(const FutureImplPtr &prev_future); @@ -96,7 +93,6 @@ private: virtual void finish(int r) {} }; - JournalMetadataPtr m_journal_metadata; uint64_t m_tag_tid; uint64_t m_entry_tid; uint64_t m_commit_tid; diff --git a/src/journal/JournalRecorder.cc b/src/journal/JournalRecorder.cc index 065f692821771..b730b267a5ccd 100644 --- a/src/journal/JournalRecorder.cc +++ b/src/journal/JournalRecorder.cc @@ -80,8 +80,7 @@ Future JournalRecorder::append(uint64_t tag_tid, ObjectRecorderPtr object_ptr = get_object(splay_offset); uint64_t commit_tid = m_journal_metadata->allocate_commit_tid( object_ptr->get_object_number(), tag_tid, entry_tid); - FutureImplPtr future(new FutureImpl(m_journal_metadata, tag_tid, entry_tid, - commit_tid)); + FutureImplPtr future(new FutureImpl(tag_tid, entry_tid, commit_tid)); future->init(m_prev_future); m_prev_future = future; diff --git a/src/test/journal/test_FutureImpl.cc b/src/test/journal/test_FutureImpl.cc index 51e19cf35e913..eb5f80696bd62 100644 --- a/src/test/journal/test_FutureImpl.cc +++ b/src/test/journal/test_FutureImpl.cc @@ -25,12 +25,11 @@ public: } }; - journal::FutureImplPtr create_future(journal::JournalMetadataPtr metadata, - uint64_t tag_tid, uint64_t entry_tid, + journal::FutureImplPtr create_future(uint64_t tag_tid, uint64_t entry_tid, uint64_t commit_tid, const journal::FutureImplPtr &prev = journal::FutureImplPtr()) { - journal::FutureImplPtr future(new journal::FutureImpl(metadata, tag_tid, + journal::FutureImplPtr future(new journal::FutureImpl(tag_tid, entry_tid, commit_tid)); future->init(prev); @@ -50,7 +49,7 @@ TEST_F(TestFutureImpl, Getters) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future = create_future(metadata, 234, 123, 456); + journal::FutureImplPtr future = create_future(234, 123, 456); ASSERT_EQ(234U, future->get_tag_tid()); ASSERT_EQ(123U, future->get_entry_tid()); ASSERT_EQ(456U, future->get_commit_tid()); @@ -63,7 +62,7 @@ TEST_F(TestFutureImpl, Attach) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future = create_future(metadata, 234, 123, 456); + journal::FutureImplPtr future = create_future(234, 123, 456); ASSERT_FALSE(future->attach(&m_flush_handler)); ASSERT_EQ(1U, m_flush_handler.refs); } @@ -75,7 +74,7 @@ TEST_F(TestFutureImpl, AttachWithPendingFlush) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future = create_future(metadata, 234, 123, 456); + journal::FutureImplPtr future = create_future(234, 123, 456); future->flush(NULL); ASSERT_TRUE(future->attach(&m_flush_handler)); @@ -89,7 +88,7 @@ TEST_F(TestFutureImpl, Detach) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future = create_future(metadata, 234, 123, 456); + journal::FutureImplPtr future = create_future(234, 123, 456); ASSERT_FALSE(future->attach(&m_flush_handler)); future->detach(); ASSERT_EQ(0U, m_flush_handler.refs); @@ -102,7 +101,7 @@ TEST_F(TestFutureImpl, DetachImplicit) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future = create_future(metadata, 234, 123, 456); + journal::FutureImplPtr future = create_future(234, 123, 456); ASSERT_FALSE(future->attach(&m_flush_handler)); future.reset(); ASSERT_EQ(0U, m_flush_handler.refs); @@ -115,7 +114,7 @@ TEST_F(TestFutureImpl, Flush) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future = create_future(metadata, 234, 123, 456); + journal::FutureImplPtr future = create_future(234, 123, 456); ASSERT_FALSE(future->attach(&m_flush_handler)); C_SaferCond cond; @@ -133,7 +132,7 @@ TEST_F(TestFutureImpl, FlushWithoutContext) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future = create_future(metadata, 234, 123, 456); + journal::FutureImplPtr future = create_future(234, 123, 456); ASSERT_FALSE(future->attach(&m_flush_handler)); future->flush(NULL); @@ -150,10 +149,10 @@ TEST_F(TestFutureImpl, FlushChain) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future1 = create_future(metadata, 234, 123, 456); - journal::FutureImplPtr future2 = create_future(metadata, 234, 124, 457, + journal::FutureImplPtr future1 = create_future(234, 123, 456); + journal::FutureImplPtr future2 = create_future(234, 124, 457, future1); - journal::FutureImplPtr future3 = create_future(metadata, 235, 1, 458, + journal::FutureImplPtr future3 = create_future(235, 1, 458, future2); ASSERT_FALSE(future1->attach(&m_flush_handler)); ASSERT_FALSE(future2->attach(&m_flush_handler)); @@ -184,8 +183,8 @@ TEST_F(TestFutureImpl, FlushInProgress) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future1 = create_future(metadata, 234, 123, 456); - journal::FutureImplPtr future2 = create_future(metadata, 234, 124, 457, + journal::FutureImplPtr future1 = create_future(234, 123, 456); + journal::FutureImplPtr future2 = create_future(234, 124, 457, future1); ASSERT_FALSE(future1->attach(&m_flush_handler)); ASSERT_FALSE(future2->attach(&m_flush_handler)); @@ -206,7 +205,7 @@ TEST_F(TestFutureImpl, FlushAlreadyComplete) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future = create_future(metadata, 234, 123, 456); + journal::FutureImplPtr future = create_future(234, 123, 456); future->safe(-EIO); C_SaferCond cond; @@ -221,7 +220,7 @@ TEST_F(TestFutureImpl, Wait) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future = create_future(metadata, 234, 1, 456); + journal::FutureImplPtr future = create_future(234, 1, 456); C_SaferCond cond; future->wait(&cond); @@ -236,7 +235,7 @@ TEST_F(TestFutureImpl, WaitAlreadyComplete) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future = create_future(metadata, 234, 1, 456); + journal::FutureImplPtr future = create_future(234, 1, 456); future->safe(-EEXIST); C_SaferCond cond; @@ -251,8 +250,8 @@ TEST_F(TestFutureImpl, SafePreservesError) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future1 = create_future(metadata, 234, 123, 456); - journal::FutureImplPtr future2 = create_future(metadata, 234, 124, 457, + journal::FutureImplPtr future1 = create_future(234, 123, 456); + journal::FutureImplPtr future2 = create_future(234, 124, 457, future1); future1->safe(-EIO); @@ -268,8 +267,8 @@ TEST_F(TestFutureImpl, ConsistentPreservesError) { journal::JournalMetadataPtr metadata = create_metadata(oid); ASSERT_EQ(0, init_metadata(metadata)); - journal::FutureImplPtr future1 = create_future(metadata, 234, 123, 456); - journal::FutureImplPtr future2 = create_future(metadata, 234, 124, 457, + journal::FutureImplPtr future1 = create_future(234, 123, 456); + journal::FutureImplPtr future2 = create_future(234, 124, 457, future1); future2->safe(-EEXIST); diff --git a/src/test/journal/test_ObjectRecorder.cc b/src/test/journal/test_ObjectRecorder.cc index 65d74b6e58afb..f26e5261688b7 100644 --- a/src/test/journal/test_ObjectRecorder.cc +++ b/src/test/journal/test_ObjectRecorder.cc @@ -67,11 +67,10 @@ public: m_flush_age = i; } - journal::AppendBuffer create_append_buffer(journal::JournalMetadataPtr metadata, - uint64_t tag_tid, uint64_t entry_tid, + journal::AppendBuffer create_append_buffer(uint64_t tag_tid, uint64_t entry_tid, const std::string &payload) { - journal::FutureImplPtr future(new journal::FutureImpl(metadata, tag_tid, - entry_tid, 456)); + journal::FutureImplPtr future(new journal::FutureImpl(tag_tid, entry_tid, + 456)); future->init(journal::FutureImplPtr()); bufferlist bl; @@ -98,16 +97,14 @@ TEST_F(TestObjectRecorder, Append) { journal::ObjectRecorderPtr object = create_object(oid, 24); - journal::AppendBuffer append_buffer1 = create_append_buffer(metadata, - 234, 123, + journal::AppendBuffer append_buffer1 = create_append_buffer(234, 123, "payload"); journal::AppendBuffers append_buffers; append_buffers = {append_buffer1}; ASSERT_FALSE(object->append(append_buffers)); ASSERT_EQ(1U, object->get_pending_appends()); - journal::AppendBuffer append_buffer2 = create_append_buffer(metadata, - 234, 124, + journal::AppendBuffer append_buffer2 = create_append_buffer(234, 124, "payload"); append_buffers = {append_buffer2}; ASSERT_FALSE(object->append(append_buffers)); @@ -129,16 +126,14 @@ TEST_F(TestObjectRecorder, AppendFlushByCount) { set_flush_interval(2); journal::ObjectRecorderPtr object = create_object(oid, 24); - journal::AppendBuffer append_buffer1 = create_append_buffer(metadata, - 234, 123, + journal::AppendBuffer append_buffer1 = create_append_buffer(234, 123, "payload"); journal::AppendBuffers append_buffers; append_buffers = {append_buffer1}; ASSERT_FALSE(object->append(append_buffers)); ASSERT_EQ(1U, object->get_pending_appends()); - journal::AppendBuffer append_buffer2 = create_append_buffer(metadata, - 234, 124, + journal::AppendBuffer append_buffer2 = create_append_buffer(234, 124, "payload"); append_buffers = {append_buffer2}; ASSERT_FALSE(object->append(append_buffers)); @@ -159,16 +154,14 @@ TEST_F(TestObjectRecorder, AppendFlushByBytes) { set_flush_bytes(10); journal::ObjectRecorderPtr object = create_object(oid, 24); - journal::AppendBuffer append_buffer1 = create_append_buffer(metadata, - 234, 123, + journal::AppendBuffer append_buffer1 = create_append_buffer(234, 123, "payload"); journal::AppendBuffers append_buffers; append_buffers = {append_buffer1}; ASSERT_FALSE(object->append(append_buffers)); ASSERT_EQ(1U, object->get_pending_appends()); - journal::AppendBuffer append_buffer2 = create_append_buffer(metadata, - 234, 124, + journal::AppendBuffer append_buffer2 = create_append_buffer(234, 124, "payload"); append_buffers = {append_buffer2}; ASSERT_FALSE(object->append(append_buffers)); @@ -189,15 +182,13 @@ TEST_F(TestObjectRecorder, AppendFlushByAge) { set_flush_age(0.1); journal::ObjectRecorderPtr object = create_object(oid, 24); - journal::AppendBuffer append_buffer1 = create_append_buffer(metadata, - 234, 123, + journal::AppendBuffer append_buffer1 = create_append_buffer(234, 123, "payload"); journal::AppendBuffers append_buffers; append_buffers = {append_buffer1}; ASSERT_FALSE(object->append(append_buffers)); - journal::AppendBuffer append_buffer2 = create_append_buffer(metadata, - 234, 124, + journal::AppendBuffer append_buffer2 = create_append_buffer(234, 124, "payload"); append_buffers = {append_buffer2}; ASSERT_FALSE(object->append(append_buffers)); @@ -218,15 +209,13 @@ TEST_F(TestObjectRecorder, AppendFilledObject) { journal::ObjectRecorderPtr object = create_object(oid, 12); std::string payload(2048, '1'); - journal::AppendBuffer append_buffer1 = create_append_buffer(metadata, - 234, 123, + journal::AppendBuffer append_buffer1 = create_append_buffer(234, 123, payload); journal::AppendBuffers append_buffers; append_buffers = {append_buffer1}; ASSERT_FALSE(object->append(append_buffers)); - journal::AppendBuffer append_buffer2 = create_append_buffer(metadata, - 234, 124, + journal::AppendBuffer append_buffer2 = create_append_buffer(234, 124, payload); append_buffers = {append_buffer2}; ASSERT_TRUE(object->append(append_buffers)); @@ -246,8 +235,7 @@ TEST_F(TestObjectRecorder, Flush) { journal::ObjectRecorderPtr object = create_object(oid, 24); - journal::AppendBuffer append_buffer1 = create_append_buffer(metadata, - 234, 123, + journal::AppendBuffer append_buffer1 = create_append_buffer(234, 123, "payload"); journal::AppendBuffers append_buffers; append_buffers = {append_buffer1}; @@ -273,8 +261,7 @@ TEST_F(TestObjectRecorder, FlushFuture) { journal::ObjectRecorderPtr object = create_object(oid, 24); - journal::AppendBuffer append_buffer = create_append_buffer(metadata, - 234, 123, + journal::AppendBuffer append_buffer = create_append_buffer(234, 123, "payload"); journal::AppendBuffers append_buffers; append_buffers = {append_buffer}; @@ -298,8 +285,7 @@ TEST_F(TestObjectRecorder, FlushDetachedFuture) { journal::ObjectRecorderPtr object = create_object(oid, 24); - journal::AppendBuffer append_buffer = create_append_buffer(metadata, - 234, 123, + journal::AppendBuffer append_buffer = create_append_buffer(234, 123, "payload"); journal::AppendBuffers append_buffers; @@ -326,11 +312,9 @@ TEST_F(TestObjectRecorder, Overflow) { journal::ObjectRecorderPtr object2 = create_object(oid, 12); std::string payload(2048, '1'); - journal::AppendBuffer append_buffer1 = create_append_buffer(metadata, - 234, 123, + journal::AppendBuffer append_buffer1 = create_append_buffer(234, 123, payload); - journal::AppendBuffer append_buffer2 = create_append_buffer(metadata, - 234, 124, + journal::AppendBuffer append_buffer2 = create_append_buffer(234, 124, payload); journal::AppendBuffers append_buffers; append_buffers = {append_buffer1, append_buffer2}; @@ -341,8 +325,7 @@ TEST_F(TestObjectRecorder, Overflow) { ASSERT_EQ(0, cond.wait()); ASSERT_EQ(0U, object1->get_pending_appends()); - journal::AppendBuffer append_buffer3 = create_append_buffer(metadata, - 456, 123, + journal::AppendBuffer append_buffer3 = create_append_buffer(456, 123, payload); append_buffers = {append_buffer3}; -- 2.39.5