From c235774864dbcf2e4fa0532e804aa9a2e1bd25c3 Mon Sep 17 00:00:00 2001 From: dimitr Date: Sun, 22 Sep 2013 16:10:06 +0000 Subject: [PATCH] Attempted to fix CORE-4235: Deadlock is possible while accessing the monitoring tables under concurrent load. Refactored the list of reference counted attachments to be useful in other cases. Some minor adjustments in the monitoring code. --- src/jrd/Attachment.cpp | 5 ++ src/jrd/Attachment.h | 114 ++++++++++++++++++++++++++ src/jrd/DatabaseSnapshot.cpp | 151 +++++++++++++++-------------------- src/jrd/DatabaseSnapshot.h | 6 +- src/jrd/jrd.cpp | 60 +++++--------- 5 files changed, 204 insertions(+), 132 deletions(-) diff --git a/src/jrd/Attachment.cpp b/src/jrd/Attachment.cpp index d736f4bac9..7298c3ba06 100644 --- a/src/jrd/Attachment.cpp +++ b/src/jrd/Attachment.cpp @@ -581,3 +581,8 @@ void Jrd::Attachment::SyncGuard::init(const char* f, bool optional) } } } + +void AttachmentsRefHolder::debugHelper(const char* from) +{ + RefDeb(DEB_RLS_JATT, from); +} diff --git a/src/jrd/Attachment.h b/src/jrd/Attachment.h index 4c57db7bbf..ff77d98fd1 100644 --- a/src/jrd/Attachment.h +++ b/src/jrd/Attachment.h @@ -35,6 +35,8 @@ #include "../common/classes/ByteChunk.h" #include "../common/classes/GenericMap.h" +#include "../common/classes/SyncObject.h" +#include "../common/classes/array.h" #include "../common/classes/stack.h" #include "../common/classes/timestamp.h" @@ -216,6 +218,27 @@ public: Firebird::Mutex& m_mutex; }; + class CheckoutSyncGuard + { + public: + CheckoutSyncGuard(Attachment* att, Firebird::SyncObject& sync, Firebird::SyncType type, const char* f) + : m_sync(&sync, f) + { + if (!m_sync.lockConditional(type, f)) + { + Checkout attCout(att, f); + m_sync.lock(type); + } + } + + private: + // copying is prohibited + CheckoutSyncGuard(const CheckoutSyncGuard&); + CheckoutSyncGuard& operator=(const CheckoutSyncGuard&); + + Firebird::Sync m_sync; + }; + public: static Attachment* create(Database* dbb); static void destroy(Attachment* const attachment); @@ -387,6 +410,97 @@ inline void Attachment::setSysTransaction(jrd_tra* trans) att_sys_transaction = trans; } +// This class holds references to all attachments it contains + +class AttachmentsRefHolder +{ + friend class Iterator; + +public: + class Iterator + { + public: + explicit Iterator(AttachmentsRefHolder& list) + : m_list(list), m_index(0) + {} + + JAttachment* operator*() + { + if (m_index < m_list.m_attachments.getCount()) + return m_list.m_attachments[m_index]; + + return NULL; + } + + void operator++() + { + m_index++; + } + + void remove() + { + if (m_index < m_list.m_attachments.getCount()) + { + AttachmentsRefHolder::debugHelper(FB_FUNCTION); + m_list.m_attachments[m_index]->release(); + m_list.m_attachments.remove(m_index); + } + } + + private: + // copying is prohibited + Iterator(const Iterator&); + Iterator& operator=(const Iterator&); + + AttachmentsRefHolder& m_list; + size_t m_index; + }; + + explicit AttachmentsRefHolder(MemoryPool& p) + : m_attachments(p) + {} + + AttachmentsRefHolder& operator=(const AttachmentsRefHolder& other) + { + this->~AttachmentsRefHolder(); + + for (size_t i = 0; i < other.m_attachments.getCount(); i++) + add(other.m_attachments[i]); + + return *this; + } + + ~AttachmentsRefHolder() + { + while (m_attachments.hasData()) + { + debugHelper(FB_FUNCTION); + m_attachments.pop()->release(); + } + } + + void add(JAttachment* jAtt) + { + if (jAtt) + { + jAtt->addRef(); + m_attachments.add(jAtt); + } + } + + void remove(Iterator& iter) + { + iter.remove(); + } + +private: + AttachmentsRefHolder(const AttachmentsRefHolder&); + + static void debugHelper(const char* from); + + Firebird::HalfStaticArray m_attachments; +}; + } // namespace Jrd #endif // JRD_ATTACHMENT_H diff --git a/src/jrd/DatabaseSnapshot.cpp b/src/jrd/DatabaseSnapshot.cpp index a99994500f..171de01798 100644 --- a/src/jrd/DatabaseSnapshot.cpp +++ b/src/jrd/DatabaseSnapshot.cpp @@ -355,7 +355,7 @@ int DatabaseSnapshot::blockingAst(void* ast_object) if (!(dbb->dbb_ast_flags & DBB_monitor_off)) { - SyncLockGuard monGuard(&dbb->dbb_mon_sync, SYNC_EXCLUSIVE, "DatabaseSnapshot::blockingAst"); + SyncLockGuard monGuard(&dbb->dbb_mon_sync, SYNC_EXCLUSIVE, FB_FUNCTION); if (!(dbb->dbb_ast_flags & DBB_monitor_off)) { @@ -364,7 +364,7 @@ int DatabaseSnapshot::blockingAst(void* ast_object) // Write the data to the shared memory try { - dumpData(tdbb); + dumpData(dbb, backup_state_unknown); } catch (const Exception& ex) { @@ -394,6 +394,9 @@ DatabaseSnapshot::DatabaseSnapshot(thread_db* tdbb, MemoryPool& pool) Database* const dbb = tdbb->getDatabase(); fb_assert(dbb); + Attachment* const attachment = tdbb->getAttachment(); + fb_assert(attachment); + // Initialize record buffers RecordBuffer* const dbb_buffer = allocBuffer(tdbb, pool, rel_mon_database); RecordBuffer* const att_buffer = allocBuffer(tdbb, pool, rel_mon_attachments); @@ -405,21 +408,38 @@ DatabaseSnapshot::DatabaseSnapshot(thread_db* tdbb, MemoryPool& pool) RecordBuffer* const ctx_var_buffer = allocBuffer(tdbb, pool, rel_mon_ctx_vars); RecordBuffer* const mem_usage_buffer = allocBuffer(tdbb, pool, rel_mon_mem_usage); - { - SyncLockGuard monGuard(&dbb->dbb_mon_sync, SYNC_EXCLUSIVE, "DatabaseSnapshot::DatabaseSnapshot"); + // Determine the backup state + int backup_state = backup_state_unknown; - // Release our own lock. + BackupManager* const bm = dbb->dbb_backup_manager; + if (bm && !bm->isShutDown()) + { + BackupManager::StateReadGuard holder(tdbb); + + switch (bm->getState()) + { + case nbak_state_normal: + backup_state = backup_state_normal; + break; + case nbak_state_stalled: + backup_state = backup_state_stalled; + break; + case nbak_state_merge: + backup_state = backup_state_merge; + break; + } + } + + { // scope + Attachment::Checkout cout(attachment, FB_FUNCTION); + SyncLockGuard monGuard(&dbb->dbb_mon_sync, SYNC_EXCLUSIVE, FB_FUNCTION); + + // Release our own lock LCK_release(tdbb, dbb->dbb_monitor_lock); dbb->dbb_ast_flags &= ~DBB_monitor_off; - { // scope for the RAII object - - // Ensure we'll be dealing with a valid backup state inside the call below. - BackupManager::StateReadGuard holder(tdbb); - - // Dump our own data - dumpData(tdbb); - } + // Dump our own data + dumpData(dbb, backup_state); } // Signal other processes to dump their data @@ -446,9 +466,6 @@ DatabaseSnapshot::DatabaseSnapshot(thread_db* tdbb, MemoryPool& pool) Reader reader(dataSize, data); - const Attachment* const attachment = tdbb->getAttachment(); - fb_assert(attachment); - string databaseName(dbb->dbb_database_name.c_str()); ISC_systemToUtf8(databaseName); @@ -713,13 +730,8 @@ void DataDump::putField(thread_db* tdbb, Record* record, const DumpField& field, } -void DatabaseSnapshot::dumpData(thread_db* tdbb) +void DatabaseSnapshot::dumpData(Database* dbb, int backup_state) { - fb_assert(tdbb); - - Database* const dbb = tdbb->getDatabase(); - fb_assert(dbb); - MemoryPool& pool = *dbb->dbb_permanent; if (!dbb->dbb_monitoring_data) @@ -733,54 +745,47 @@ void DatabaseSnapshot::dumpData(thread_db* tdbb) // Database information - putDatabase(record, dbb, writer, fb_utils::genUniqueId()); + putDatabase(record, dbb, writer, fb_utils::genUniqueId(), backup_state); // Attachment information - Attachment* const self_attachment = tdbb->getAttachment(); + AttachmentsRefHolder attachments(pool); - if (self_attachment) - dumpAttachment(record, self_attachment, writer); - - try - { - Attachment::Checkout attCout(self_attachment, FB_FUNCTION, true); + { // scope + SyncLockGuard attGuard(&dbb->dbb_sync, SYNC_SHARED, FB_FUNCTION); for (Attachment* attachment = dbb->dbb_attachments; attachment; attachment = attachment->att_next) { - if (attachment != self_attachment) - { - Attachment::SyncGuard attGuard(attachment, FB_FUNCTION); - dumpAttachment(record, attachment, writer); - } + attachments.add(attachment->att_interface); } - - { // scope - SyncLockGuard guard(&dbb->dbb_sys_attach, SYNC_SHARED, FB_FUNCTION); - - for (Attachment* attachment = dbb->dbb_sys_attachments; attachment; - attachment = attachment->att_next) - { - Attachment::SyncGuard attGuard(attachment, FB_FUNCTION); - dumpAttachment(record, attachment, writer); - } - } - - tdbb->setAttachment(self_attachment); } - catch(const Exception&) + + { // scope + SyncLockGuard sysAttGuard(&dbb->dbb_sys_attach, SYNC_SHARED, FB_FUNCTION); + + for (Attachment* attachment = dbb->dbb_sys_attachments; attachment; + attachment = attachment->att_next) + { + attachments.add(attachment->att_interface); + } + } + + for (AttachmentsRefHolder::Iterator iter(attachments); *iter; ++iter) { - tdbb->setAttachment(self_attachment); - throw; + JAttachment* const jAtt = *iter; + Attachment::SyncGuard attGuard(jAtt, FB_FUNCTION); + Attachment* const attachment = jAtt->getHandle(); + + if (attachment && attachment->att_user) + dumpAttachment(record, attachment, writer); } } void DatabaseSnapshot::dumpAttachment(DumpRecord& record, const Attachment* attachment, Writer& writer) { - if (!putAttachment(record, attachment, writer, fb_utils::genUniqueId())) - return; + putAttachment(record, attachment, writer, fb_utils::genUniqueId()); jrd_tra* transaction = NULL; jrd_req* request = NULL; @@ -837,7 +842,7 @@ SINT64 DatabaseSnapshot::getGlobalId(int value) void DatabaseSnapshot::putDatabase(DumpRecord& record, const Database* database, - Writer& writer, int stat_id) + Writer& writer, int stat_id, int backup_state) { fb_assert(database); @@ -873,21 +878,13 @@ void DatabaseSnapshot::putDatabase(DumpRecord& record, const Database* database, // shutdown mode if (database->dbb_ast_flags & DBB_shutdown_full) - { temp = shut_mode_full; - } else if (database->dbb_ast_flags & DBB_shutdown_single) - { temp = shut_mode_single; - } else if (database->dbb_ast_flags & DBB_shutdown) - { temp = shut_mode_multi; - } else - { temp = shut_mode_online; - } record.storeInteger(f_mon_db_shut_mode, temp); // sweep interval @@ -905,25 +902,8 @@ void DatabaseSnapshot::putDatabase(DumpRecord& record, const Database* database, record.storeTimestamp(f_mon_db_created, database->dbb_creation_date); // database size record.storeInteger(f_mon_db_pages, PageSpace::actAlloc(database)); - - // database state - temp = backup_state_unknown; - if (database->dbb_backup_manager) - { - switch (database->dbb_backup_manager->getState()) - { - case nbak_state_normal: - temp = backup_state_normal; - break; - case nbak_state_stalled: - temp = backup_state_stalled; - break; - case nbak_state_merge: - temp = backup_state_merge; - break; - } - } - record.storeInteger(f_mon_db_backup_state, temp); + // database backup state + record.storeInteger(f_mon_db_backup_state, backup_state); // crypt thread status if (database->dbb_crypto_manager) @@ -951,13 +931,10 @@ void DatabaseSnapshot::putDatabase(DumpRecord& record, const Database* database, } -bool DatabaseSnapshot::putAttachment(DumpRecord& record, const Jrd::Attachment* attachment, +void DatabaseSnapshot::putAttachment(DumpRecord& record, const Jrd::Attachment* attachment, Writer& writer, int stat_id) { - fb_assert(attachment); - - if (!attachment->att_user) - return false; + fb_assert(attachment && attachment->att_user); record.reset(rel_mon_attachments); @@ -1034,8 +1011,6 @@ bool DatabaseSnapshot::putAttachment(DumpRecord& record, const Jrd::Attachment* // context vars putContextVars(record, attachment->att_context_vars, writer, attachment->att_attachment_id, true); - - return true; } diff --git a/src/jrd/DatabaseSnapshot.h b/src/jrd/DatabaseSnapshot.h index 3b30e1bbbd..32f1c2d6ce 100644 --- a/src/jrd/DatabaseSnapshot.h +++ b/src/jrd/DatabaseSnapshot.h @@ -345,13 +345,13 @@ protected: private: RecordBuffer* allocBuffer(thread_db*, MemoryPool&, int); - static void dumpData(thread_db*); + static void dumpData(Database*, int); static void dumpAttachment(DumpRecord&, const Attachment*, Writer&); static SINT64 getGlobalId(int); - static void putDatabase(DumpRecord&, const Database*, Writer&, int); - static bool putAttachment(DumpRecord&, const Attachment*, Writer&, int); + static void putDatabase(DumpRecord&, const Database*, Writer&, int, int); + static void putAttachment(DumpRecord&, const Attachment*, Writer&, int); static void putTransaction(DumpRecord&, const jrd_tra*, Writer&, int); static void putRequest(DumpRecord&, const jrd_req*, Writer&, int); static void putCall(DumpRecord&, const jrd_req*, Writer&, int); diff --git a/src/jrd/jrd.cpp b/src/jrd/jrd.cpp index 796c5d723e..d48a473741 100644 --- a/src/jrd/jrd.cpp +++ b/src/jrd/jrd.cpp @@ -5130,11 +5130,11 @@ bool JRD_reschedule(thread_db* tdbb, SLONG quantum, bool punt) * **************************************/ Database* const dbb = tdbb->getDatabase(); - Jrd::Attachment* const att = tdbb->getAttachment(); + Jrd::Attachment* const attachment = tdbb->getAttachment(); if (!tdbb->checkCancelState(false)) { - Jrd::Attachment::Checkout cout(att, FB_FUNCTION); + Jrd::Attachment::Checkout cout(attachment, FB_FUNCTION); THREAD_YIELD(); } @@ -5163,7 +5163,9 @@ bool JRD_reschedule(thread_db* tdbb, SLONG quantum, bool punt) // Enable signal handler for the monitoring stuff if (dbb->dbb_ast_flags & DBB_monitor_off) { - SyncLockGuard monGuard(&dbb->dbb_mon_sync, SYNC_EXCLUSIVE, "JRD_reschedule"); + Attachment::CheckoutSyncGuard monGuard(attachment, dbb->dbb_mon_sync, + SYNC_EXCLUSIVE, FB_FUNCTION); + if (dbb->dbb_ast_flags & DBB_monitor_off) { dbb->dbb_ast_flags &= ~DBB_monitor_off; @@ -5280,7 +5282,8 @@ static void check_database(thread_db* tdbb, bool async) if (dbb->dbb_ast_flags & DBB_monitor_off) { - SyncLockGuard monGuard(&dbb->dbb_mon_sync, SYNC_EXCLUSIVE, "check_database"); + Attachment::CheckoutSyncGuard monGuard(attachment, dbb->dbb_mon_sync, + SYNC_EXCLUSIVE, FB_FUNCTION); if (dbb->dbb_ast_flags & DBB_monitor_off) { @@ -7091,35 +7094,16 @@ static void unwindAttach(thread_db* tdbb, const Exception& ex, IStatus* userStat namespace { - class AttQueue : public HalfStaticArray + bool shutdownAttachments(AttachmentsRefHolder* arg) { - public: - explicit AttQueue(MemoryPool& p) - : HalfStaticArray(p) - { } - - ~AttQueue() - { - RefDeb(DEB_RLS_JATT, FB_FUNCTION); - // Release interfaces - while (hasData()) - { - pop()->release(); - } - } - }; - - bool shutdownAttachments(AttQueue* arg) - { - AutoPtr queue(arg); - AttQueue& attachments = *arg; + AutoPtr queue(arg); + AttachmentsRefHolder& attachments = *arg; bool success = true; // Set terminate flag for all attachments - unsigned i; - for (i = 0; i < attachments.getCount(); ++i) + for (AttachmentsRefHolder::Iterator iter(attachments); *iter; ++iter) { - JAttachment* jAtt = attachments[i]; + JAttachment* const jAtt = *iter; MutexLockGuard guard(*(jAtt->getMutex(true)), FB_FUNCTION); Attachment* attachment = jAtt->getHandle(); @@ -7129,9 +7113,9 @@ namespace } // Purge all attachments - for (i = 0; i < attachments.getCount(); ++i) + for (AttachmentsRefHolder::Iterator iter(attachments); *iter; ++iter) { - JAttachment* jAtt = attachments[i]; + JAttachment* const jAtt = *iter; MutexLockGuard guard(*(jAtt->getMutex()), FB_FUNCTION); Attachment* attachment = jAtt->getHandle(); @@ -7175,7 +7159,7 @@ namespace return 0; } - shutdownAttachments(static_cast(arg)); + shutdownAttachments(static_cast(arg)); } catch(const Exception& ex) { @@ -7205,7 +7189,7 @@ static THREAD_ENTRY_DECLARE shutdown_thread(THREAD_ENTRY_PARAM arg) bool success = true; MemoryPool& pool = *getDefaultMemoryPool(); - AttQueue* attachments = FB_NEW(pool) AttQueue(pool); + AttachmentsRefHolder* const attachments = FB_NEW(pool) AttachmentsRefHolder(pool); try { @@ -7222,14 +7206,8 @@ static THREAD_ENTRY_DECLARE shutdown_thread(THREAD_ENTRY_PARAM arg) Sync dbbGuard(&dbb->dbb_sync, FB_FUNCTION); dbbGuard.lock(SYNC_EXCLUSIVE); - for (Attachment* att = dbb->dbb_attachments; att; att = att->att_next) - { - if (att->att_interface) - { - att->att_interface->addRef(); - attachments->push(att->att_interface); - } - } + for (const Attachment* att = dbb->dbb_attachments; att; att = att->att_next) + attachments->add(att->att_interface); } } // No need in databases_mutex any more @@ -7778,7 +7756,7 @@ void JRD_shutdown_attachments(Database* dbb) try { MemoryPool& pool = *getDefaultMemoryPool(); - AttQueue* queue = FB_NEW(pool) AttQueue(pool); + AttachmentsRefHolder* queue = FB_NEW(pool) AttachmentsRefHolder(pool); { // scope Sync guard(&dbb->dbb_sync, "JRD_shutdown_attachments");