From 1b7b12e2f7a8f3004348dd1a51788e81d02dfeed Mon Sep 17 00:00:00 2001 From: Thierry Bordaz Date: May 07 2020 14:50:21 +0000 Subject: Ticket 51068 - deadlock when updating the schema Bug Description: It exists a 3 threads deadlock scenario. It involves state change plugins when it calls schema_changed_callback. So the trigger is a change of schema (direct or via replication). The scenario is MOD(cn=schema) hold StateChange lock wait for vattr lock SRCH hold vattr lock wait for DB page MOD hold DB page wait for StateChange lock Fix Description: Statechange lock protects the list of registered callbacks. lock is a mutex where actually registration of callback is only done at startup. Later the list is only lookup. Making statechange lock a rwlock suppresses the deadlock scenario as MODs will only acquire in read StateChange lock. It should also improve performance as at the moment all MODs are serialized on that lock In order to prevent writer starvation a new slapi_new_rwlock_prio create rwlock with priority to writers. https://pagure.io/389-ds-base/issue/51068 Reviewed by: Mark Reynolds, William Brown Platforms tested: 30 Flag Day: no Doc impact: no --- diff --git a/ldap/servers/plugins/statechange/statechange.c b/ldap/servers/plugins/statechange/statechange.c index c6a611d..9026382 100644 --- a/ldap/servers/plugins/statechange/statechange.c +++ b/ldap/servers/plugins/statechange/statechange.c @@ -40,7 +40,7 @@ static SCNotify *head; /* a place to start in the list */ #define SCN_PLUGIN_SUBSYSTEM "statechange-plugin" /* used for logging */ static void *api[5]; -static Slapi_Mutex *buffer_lock = 0; +static Slapi_RWLock *buffer_lock = 0; static PRUint64 g_plugin_started = 0; /* @@ -139,7 +139,7 @@ statechange_start(Slapi_PBlock *pb __attribute__((unused))) api[3] = (void *)_statechange_unregister_all; api[4] = (void *)_statechange_vattr_cache_invalidator_callback; - if (0 == (buffer_lock = slapi_new_mutex())) /* we never free this mutex */ + if (0 == (buffer_lock = slapi_new_rwlock())) { /* badness */ slapi_log_err(SLAPI_LOG_ERR, SCN_PLUGIN_SUBSYSTEM, "statechange_start - Failed to create lock\n"); @@ -179,7 +179,9 @@ statechange_close(Slapi_PBlock *pb __attribute__((unused))) slapi_counter_destroy(&op_counter); slapi_apib_unregister(StateChange_v1_0_GUID); - slapi_destroy_mutex(buffer_lock); + if (buffer_lock) { + slapi_destroy_rwlock(buffer_lock); + } buffer_lock = NULL; slapi_log_err(SLAPI_LOG_TRACE, SCN_PLUGIN_SUBSYSTEM, "<-- statechange_close\n"); @@ -239,7 +241,7 @@ statechange_post_op(Slapi_PBlock *pb, int modtype) slapi_log_err(SLAPI_LOG_TRACE, SCN_PLUGIN_SUBSYSTEM, "--> statechange_post_op\n"); /* evaluate this operation against the notification entries */ - slapi_lock_mutex(buffer_lock); + slapi_rwlock_rdlock(buffer_lock); if (head) { slapi_pblock_get(pb, SLAPI_TARGET_SDN, &sdn); if (NULL == sdn) { @@ -289,7 +291,7 @@ statechange_post_op(Slapi_PBlock *pb, int modtype) } while (notify && notify != head); } bail: - slapi_unlock_mutex(buffer_lock); + slapi_rwlock_unlock(buffer_lock); slapi_log_err(SLAPI_LOG_TRACE, SCN_PLUGIN_SUBSYSTEM, "<-- statechange_post_op\n"); return SLAPI_PLUGIN_SUCCESS; /* always succeed */ @@ -337,7 +339,7 @@ _statechange_register(char *caller_id, char *dn, char *filter, void *caller_data } item->func = func; - slapi_lock_mutex(buffer_lock); + slapi_rwlock_wrlock(buffer_lock); if (head == NULL) { head = item; head->next = head; @@ -348,7 +350,7 @@ _statechange_register(char *caller_id, char *dn, char *filter, void *caller_data head->prev = item; item->prev->next = item; } - slapi_unlock_mutex(buffer_lock); + slapi_rwlock_unlock(buffer_lock); slapi_ch_free_string(&writable_filter); ret = SLAPI_PLUGIN_SUCCESS; @@ -370,7 +372,7 @@ _statechange_unregister(char *dn, char *filter, notify_callback thefunc) return ret; } - slapi_lock_mutex(buffer_lock); + slapi_rwlock_wrlock(buffer_lock); if ((func = statechange_find_notify(dn, filter, thefunc))) { func->prev->next = func->next; @@ -391,7 +393,7 @@ _statechange_unregister(char *dn, char *filter, notify_callback thefunc) slapi_ch_free((void **)&func); } - slapi_unlock_mutex(buffer_lock); + slapi_rwlock_unlock(buffer_lock); slapi_counter_decrement(op_counter); return ret; @@ -409,7 +411,7 @@ _statechange_unregister_all(char *caller_id, caller_data_free_callback callback) return; } - slapi_lock_mutex(buffer_lock); + slapi_rwlock_wrlock(buffer_lock); if (notify) { do { @@ -440,7 +442,7 @@ _statechange_unregister_all(char *caller_id, caller_data_free_callback callback) } while (notify != start_notify && notify != NULL); } - slapi_unlock_mutex(buffer_lock); + slapi_rwlock_unlock(buffer_lock); slapi_counter_decrement(op_counter); } diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h index e7ec581..0e38570 100644 --- a/ldap/servers/slapd/slapi-plugin.h +++ b/ldap/servers/slapd/slapi-plugin.h @@ -6087,6 +6087,22 @@ int slapi_wait_condvar(Slapi_CondVar *cvar, struct timeval *timeout); int slapi_notify_condvar(Slapi_CondVar *cvar, int notify_all); /** + * Creates a new read/write lock + * If prio_writer the rwlock gives priority on writers + * else it give priority on readers (default) + * + * \return A pointer to a \c Slapi_RWLock + * + * \note Free the returned lock by calling slapi_destroy_rwlock() when finished + * + * \see slapi_destroy_rwlock() + * \see slapi_rwlock_rdlock() + * \see slapi_rwlock_wrlock() + * \see slapi_rwlock_unlock() + */ +Slapi_RWLock *slapi_new_rwlock_prio(int32_t prio_writer); + +/** * Creates a new read/write lock. * * \return A pointer to a \c Slapi_RWLock diff --git a/ldap/servers/slapd/slapi2nspr.c b/ldap/servers/slapd/slapi2nspr.c index b3e6d94..232d159 100644 --- a/ldap/servers/slapd/slapi2nspr.c +++ b/ldap/servers/slapd/slapi2nspr.c @@ -182,6 +182,31 @@ slapi_notify_condvar(Slapi_CondVar *cvar, int notify_all) } Slapi_RWLock * +slapi_new_rwlock_prio(int32_t prio_writer) +{ +#ifdef USE_POSIX_RWLOCKS + pthread_rwlock_t *rwlock = NULL; + pthread_rwlockattr_t attr; + + pthread_rwlockattr_init(&attr); + if (prio_writer) { + pthread_rwlockattr_setkind_np(&attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP); + } else { + pthread_rwlockattr_setkind_np(&attr, PTHREAD_RWLOCK_PREFER_READER_NP); + } + + rwlock = (pthread_rwlock_t *)slapi_ch_malloc(sizeof(pthread_rwlock_t)); + if (rwlock) { + pthread_rwlock_init(rwlock, &attr); + } + + return ((Slapi_RWLock *)rwlock); +#else + return ((Slapi_RWLock *)PR_NewRWLock(PR_RWLOCK_RANK_NONE, "slapi_rwlock")); +#endif +} + +Slapi_RWLock * slapi_new_rwlock(void) { #ifdef USE_POSIX_RWLOCKS diff --git a/ldap/servers/slapd/vattr.c b/ldap/servers/slapd/vattr.c index d8b2c83..7c70f41 100644 --- a/ldap/servers/slapd/vattr.c +++ b/ldap/servers/slapd/vattr.c @@ -1996,7 +1996,7 @@ vattr_map_create(void) return ENOMEM; } - the_map->lock = slapi_new_rwlock(); + the_map->lock = slapi_new_rwlock_prio(1 /* priority on writers */); if (NULL == the_map) { slapd_nasty(sourcefile, 3, 0); return ENOMEM;