This is an automated email from the git hooks/post-receive script.
tbordaz pushed a commit to branch 389-ds-base-1.4.0
in repository 389-ds-base.
The following commit(s) were added to refs/heads/389-ds-base-1.4.0 by this push:
new b998fed Ticket 49873: (cont) Contention on virtual attribute lookup
b998fed is described below
commit b998fed9cb1123849486ff897c9b53fe9979cfdb
Author: Thierry Bordaz <tbordaz(a)redhat.com>
AuthorDate: Wed Mar 6 16:07:53 2019 +0100
Ticket 49873: (cont) Contention on virtual attribute lookup
Bug Description:
The previous fix was incomplete.
It created the thread private counter before the fork.
The deamon process was not inheriting it.
There is a possiblity that an callback of an internal search
tries to update the map. (cos thread monitoring cos definition)
In such case the RW lock was first acquired in read at the top level
of the internal search, then later the callback try to acquire it in write.
this created a deadlock
It stored in in private counter a value (int) rather than the address of
of the value (int*).
Fix Description:
The fix consists to create the thread private counter after the deamon creation.
In adding, when acquiring the lock in write, if the lock was already acquired
at the top level (in read), it release the lock and reset the counter. Then acquires
the lock in write.
In the opposite when releasing the lock in read, if the lock was not already
acquired
it assumes it was acquired in write and do nothing
https://pagure.io/389-ds-base/issue/49873
Reviewed by: Mark Reynolds, William Brown (thanks !!)
Platforms tested: F30
Flag Day: no
Doc impact: no
---
ldap/servers/slapd/connection.c | 1 -
ldap/servers/slapd/main.c | 4 ++
ldap/servers/slapd/opshared.c | 2 +-
ldap/servers/slapd/proto-slap.h | 6 +-
ldap/servers/slapd/psearch.c | 1 -
ldap/servers/slapd/vattr.c | 126 +++++++++++++++++++++++++++++-----------
6 files changed, 102 insertions(+), 38 deletions(-)
diff --git a/ldap/servers/slapd/connection.c b/ldap/servers/slapd/connection.c
index fcc46cd..8b88568 100644
--- a/ldap/servers/slapd/connection.c
+++ b/ldap/servers/slapd/connection.c
@@ -1509,7 +1509,6 @@ connection_threadmain()
long bypasspollcnt = 0;
enable_nunc_stans = config_get_enable_nunc_stans();
- vattr_global_lock_init();
#if defined(hpux)
/* Arrange to ignore SIGPIPE signals. */
SIGNAL(SIGPIPE, SIG_IGN);
diff --git a/ldap/servers/slapd/main.c b/ldap/servers/slapd/main.c
index 185ba90..5a86e2e 100644
--- a/ldap/servers/slapd/main.c
+++ b/ldap/servers/slapd/main.c
@@ -950,6 +950,10 @@ main(int argc, char **argv)
return_value = 1;
goto cleanup;
}
+ /* The thread private counter needs to be allocated after the fork
+ * it is not inherited from parent process
+ */
+ vattr_global_lock_create();
/*
* Create our thread pool here for tasks to utilise.
diff --git a/ldap/servers/slapd/opshared.c b/ldap/servers/slapd/opshared.c
index 8b895a1..dd69173 100644
--- a/ldap/servers/slapd/opshared.c
+++ b/ldap/servers/slapd/opshared.c
@@ -987,7 +987,7 @@ free_and_return:
slapi_be_Unlock(be_single);
}
if (vattr_lock_acquired) {
- vattr_unlock();
+ vattr_rd_unlock();
}
free_and_return_nolock:
diff --git a/ldap/servers/slapd/proto-slap.h b/ldap/servers/slapd/proto-slap.h
index 2029f41..f87c747 100644
--- a/ldap/servers/slapd/proto-slap.h
+++ b/ldap/servers/slapd/proto-slap.h
@@ -1419,9 +1419,11 @@ void subentry_create_filter(Slapi_Filter **filter);
* vattr.c
*/
void vattr_init(void);
-void vattr_global_lock_init(void);
+void vattr_global_lock_create(void);
void vattr_rdlock();
-void vattr_unlock();
+void vattr_rd_unlock();
+void vattr_wrlock();
+void vattr_wr_unlock();
void vattr_cleanup(void);
/*
diff --git a/ldap/servers/slapd/psearch.c b/ldap/servers/slapd/psearch.c
index e7b97a7..8ad268a 100644
--- a/ldap/servers/slapd/psearch.c
+++ b/ldap/servers/slapd/psearch.c
@@ -267,7 +267,6 @@ ps_send_results(void *arg)
Operation *pb_op = NULL;
g_incr_active_threadcnt();
- vattr_global_lock_init();
slapi_pblock_get(ps->ps_pblock, SLAPI_CONNECTION, &pb_conn);
slapi_pblock_get(ps->ps_pblock, SLAPI_OPERATION, &pb_op);
diff --git a/ldap/servers/slapd/vattr.c b/ldap/servers/slapd/vattr.c
index 155afca..ce63f50 100644
--- a/ldap/servers/slapd/vattr.c
+++ b/ldap/servers/slapd/vattr.c
@@ -119,20 +119,45 @@ void
vattr_init()
{
statechange_api = 0;
- PR_NewThreadPrivateIndex(&thread_private_global_vattr_lock, NULL);
vattr_map_create();
#ifdef VATTR_TEST_CODE
vattr_basic_sp_init();
#endif
}
-
+/* Create a private variable for each individual thread of the current process */
void
-vattr_global_lock_init()
+vattr_global_lock_create()
+{
+ if (PR_NewThreadPrivateIndex(&thread_private_global_vattr_lock, NULL) !=
PR_SUCCESS) {
+ slapi_log_err(SLAPI_LOG_ALERT,
+ "vattr_global_lock_create", "Failure to create global lock
for virtual attribute !\n");
+ PR_ASSERT(0);
+ }
+}
+static int
+global_vattr_lock_get_acquired_count()
{
- if (thread_private_global_vattr_lock) {
- PR_SetThreadPrivate(thread_private_global_vattr_lock, (void *) 0);
+ int *nb_acquired;
+ nb_acquired = (int *) PR_GetThreadPrivate(thread_private_global_vattr_lock);
+ if (nb_acquired == NULL) {
+ /* if it was not initialized set it to zero */
+ nb_acquired = (int *) slapi_ch_calloc(1, sizeof(int));
+ PR_SetThreadPrivate(thread_private_global_vattr_lock, (void *) nb_acquired);
}
+ return *nb_acquired;
+}
+static void
+global_vattr_lock_set_acquired_count(int nb_acquired)
+{
+ int *val;
+ val = (int *) PR_GetThreadPrivate(thread_private_global_vattr_lock);
+ if (val == NULL) {
+ /* if it was not initialized set it to zero */
+ val = (int *) slapi_ch_calloc(1, sizeof(int));
+ }
+ *val = nb_acquired;
+ PR_SetThreadPrivate(thread_private_global_vattr_lock, (void *) val);
}
/* The map lock can be acquired recursively. So only the first rdlock
* will acquire the lock.
@@ -142,18 +167,15 @@ vattr_global_lock_init()
void
vattr_rdlock()
{
- if (thread_private_global_vattr_lock) {
- int nb_acquire = (int) PR_GetThreadPrivate(thread_private_global_vattr_lock);
+ int nb_acquire = global_vattr_lock_get_acquired_count();
- if (nb_acquire == 0) {
- /* The lock was not held just acquire it */
- slapi_rwlock_rdlock(the_map->lock);
- }
- nb_acquire++;
- PR_SetThreadPrivate(thread_private_global_vattr_lock, (void *) nb_acquire);
- } else {
+ if (nb_acquire == 0) {
+ /* The lock was not held just acquire it */
slapi_rwlock_rdlock(the_map->lock);
}
+ nb_acquire++;
+ global_vattr_lock_set_acquired_count(nb_acquire);
+
}
/* The map lock can be acquired recursively. So only the last unlock
* will release the lock.
@@ -161,25 +183,63 @@ vattr_rdlock()
* later calls during the operation processing will just increase/decrease a counter.
*/
void
-vattr_unlock()
+vattr_rd_unlock()
{
- if (thread_private_global_vattr_lock) {
- int nb_acquire = (int) PR_GetThreadPrivate(thread_private_global_vattr_lock);
+ int nb_acquire = global_vattr_lock_get_acquired_count();
- if (nb_acquire >= 1) {
- nb_acquire--;
- if (nb_acquire == 0) {
- slapi_rwlock_unlock(the_map->lock);
- }
- PR_SetThreadPrivate(thread_private_global_vattr_lock, (void *) nb_acquire);
- } else {
- slapi_log_err(SLAPI_LOG_CRIT,
- "vattr_unlock", "The lock was not acquire. We should not be
here\n");
- PR_ASSERT(nb_acquire >= 1);
+ if (nb_acquire >= 1) {
+ nb_acquire--;
+ if (nb_acquire == 0) {
+ slapi_rwlock_unlock(the_map->lock);
}
+ global_vattr_lock_set_acquired_count(nb_acquire);
} else {
+ /* this is likely the consequence of lock acquire in read during an internal
search
+ * but the search callback updated the map and release the readlock and acquired
+ * it in write.
+ * So after the update the lock was no longer held but when completing the
internal
+ * search we release the global read lock, that now has nothing to do
+ */
+ slapi_log_err(SLAPI_LOG_INFO,
+ "vattr_rd_unlock", "vattr lock no longer acquired in
read.\n");
+ }
+}
+
+/* The map lock is acquired in write (updating the map)
+ * It exists a possibility that lock is acquired in write while it is already
+ * hold in read by this thread (internal search with updating callback)
+ * In such situation, the we must abandon the read global lock and acquire in write
+ */
+void
+vattr_wrlock()
+{
+ int nb_read_acquire = global_vattr_lock_get_acquired_count();
+
+ if (nb_read_acquire) {
+ /* The lock was acquired in read but we need it in write
+ * release it and set the global vattr_lock counter to 0
+ */
slapi_rwlock_unlock(the_map->lock);
+ global_vattr_lock_set_acquired_count(0);
}
+ slapi_rwlock_wrlock(the_map->lock);
+}
+/* The map lock is release from a write write (updating the map)
+ */
+void
+vattr_wr_unlock()
+{
+ int nb_read_acquire = global_vattr_lock_get_acquired_count();
+
+ if (nb_read_acquire) {
+ /* The lock being acquired in write, the private thread counter
+ * (that count the number of time it was acquired in read) should be 0
+ */
+ slapi_log_err(SLAPI_LOG_INFO,
+ "vattr_unlock", "The lock was acquired in write. We should not
be here\n");
+ PR_ASSERT(nb_read_acquire == 0);
+ }
+ slapi_rwlock_unlock(the_map->lock);
}
/* Called on server shutdown, free all structures, inform service providers that
we're going down etc */
void
@@ -1999,7 +2059,7 @@ vattr_map_lookup(const char *type_to_find, vattr_map_entry
**result)
*result = (vattr_map_entry *)PL_HashTableLookupConst(the_map->hashtable,
(void *)basetype);
/* Release ze lock */
- vattr_unlock();
+ vattr_rd_unlock();
if (tmp) {
slapi_ch_free_string(&tmp);
@@ -2018,13 +2078,13 @@ vattr_map_insert(vattr_map_entry *vae)
{
PR_ASSERT(the_map);
/* Get the writer lock */
- slapi_rwlock_wrlock(the_map->lock);
+ vattr_wrlock();
/* Insert the thing */
/* It's illegal to call this function if the entry is already there */
PR_ASSERT(NULL == PL_HashTableLookupConst(the_map->hashtable, (void
*)vae->type_name));
PL_HashTableAdd(the_map->hashtable, (void *)vae->type_name, (void *)vae);
/* Unlock and we're done */
- slapi_rwlock_unlock(the_map->lock);
+ vattr_wr_unlock();
return 0;
}
@@ -2161,13 +2221,13 @@ schema_changed_callback(Slapi_Entry *e __attribute__((unused)),
void *caller_data __attribute__((unused)))
{
/* Get the writer lock */
- slapi_rwlock_wrlock(the_map->lock);
+ vattr_wrlock();
/* go through the list */
PL_HashTableEnumerateEntries(the_map->hashtable, vattr_map_entry_rebuild_schema,
0);
/* Unlock and we're done */
- slapi_rwlock_unlock(the_map->lock);
+ vattr_wr_unlock();
}
@@ -2204,7 +2264,7 @@ slapi_vattr_schema_check_type(Slapi_Entry *e, char *type)
obj = obj->pNext;
}
- vattr_unlock();
+ vattr_rd_unlock();
}
slapi_valueset_free(vs);
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.