From 229f61f5f54aeb9e1a1756f731dfe7bcedbf148c Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Fri, 13 Oct 2017 07:09:08 -0400 Subject: [PATCH 06/10] Ticket 48235 - Remove memberOf global lock Bug Description: The memberOf global lock no longer servers a purpose since the plugin is BETXN. This was causing potential deadlocks when multiple backends are used. Fix Description: Remove the lock, and rework the fixup/ancestors caches/hashtables. Instead of reusing a single cache, we create a fresh cache when we copy the plugin config (which only happens at the start of an operation). Then we destroy the caches when we free the config. https://pagure.io/389-ds-base/issue/48235 Reviewed by: tbordaz & firstyear(Thanks!!) --- ldap/servers/plugins/memberof/memberof.c | 312 +++--------------------- ldap/servers/plugins/memberof/memberof.h | 17 ++ ldap/servers/plugins/memberof/memberof_config.c | 152 +++++++++++- 3 files changed, 200 insertions(+), 281 deletions(-) diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c index 9bbe13c9c..bbf47dd49 100644 --- a/ldap/servers/plugins/memberof/memberof.c +++ b/ldap/servers/plugins/memberof/memberof.c @@ -49,13 +49,10 @@ static void* _PluginID = NULL; static Slapi_DN* _ConfigAreaDN = NULL; static Slapi_RWLock *config_rwlock = NULL; static Slapi_DN* _pluginDN = NULL; -static PRMonitor *memberof_operation_lock = 0; MemberOfConfig *qsortConfig = 0; static int usetxn = 0; static int premodfn = 0; -#define MEMBEROF_HASHTABLE_SIZE 1000 -static PLHashTable *fixup_entry_hashtable = NULL; /* global hash table protected by memberof_lock (memberof_operation_lock) */ -static PLHashTable *group_ancestors_hashtable = NULL; /* global hash table protected by memberof_lock (memberof_operation_lock) */ + typedef struct _memberofstringll { @@ -73,18 +70,7 @@ typedef struct _memberof_get_groups_data PRBool use_cache; } memberof_get_groups_data; -/* The key to access the hash table is the normalized DN - * The normalized DN is stored in the value because: - * - It is used in slapi_valueset_find - * - It is used to fill the memberof_get_groups_data.group_norm_vals - */ -typedef struct _memberof_cached_value -{ - char *key; - char *group_dn_val; - char *group_ndn_val; - int valid; -} memberof_cached_value; + struct cache_stat { int total_lookup; @@ -189,14 +175,9 @@ static int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data); static int memberof_entry_in_scope(MemberOfConfig *config, Slapi_DN *sdn); static int memberof_add_objectclass(char *auto_add_oc, const char *dn); static int memberof_add_memberof_attr(LDAPMod **mods, const char *dn, char *add_oc); -static PLHashTable *hashtable_new(); -static void fixup_hashtable_empty(char *msg); -static PLHashTable *hashtable_new(); -static void ancestor_hashtable_empty(char *msg); -static void ancestor_hashtable_entry_free(memberof_cached_value *entry); -static memberof_cached_value *ancestors_cache_lookup(const char *ndn); -static PRBool ancestors_cache_remove(const char *ndn); -static PLHashEntry *ancestors_cache_add(const void *key, void *value); +static memberof_cached_value *ancestors_cache_lookup(MemberOfConfig *config, const char *ndn); +static PRBool ancestors_cache_remove(MemberOfConfig *config, const char *ndn); +static PLHashEntry *ancestors_cache_add(MemberOfConfig *config, const void *key, void *value); /*** implementation ***/ @@ -375,12 +356,6 @@ int memberof_postop_start(Slapi_PBlock *pb) slapi_log_err(SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM, "--> memberof_postop_start\n" ); - memberof_operation_lock = PR_NewMonitor(); - if(0 == memberof_operation_lock) - { - rc = -1; - goto bail; - } if(config_rwlock == NULL){ if((config_rwlock = slapi_new_rwlock()) == NULL){ rc = -1; @@ -388,9 +363,6 @@ int memberof_postop_start(Slapi_PBlock *pb) } } - fixup_entry_hashtable = hashtable_new(); - group_ancestors_hashtable = hashtable_new(); - /* Set the alternate config area if one is defined. */ slapi_pblock_get(pb, SLAPI_PLUGIN_CONFIG_AREA, &config_area); if (config_area) @@ -482,18 +454,7 @@ int memberof_postop_close(Slapi_PBlock *pb) slapi_sdn_free(&_pluginDN); slapi_destroy_rwlock(config_rwlock); config_rwlock = NULL; - PR_DestroyMonitor(memberof_operation_lock); - memberof_operation_lock = NULL; - - if (fixup_entry_hashtable) { - fixup_hashtable_empty("memberof_postop_close empty fixup_entry_hastable"); - PL_HashTableDestroy(fixup_entry_hashtable); - } - if (group_ancestors_hashtable) { - ancestor_hashtable_empty("memberof_postop_close empty group_ancestors_hashtable"); - PL_HashTableDestroy(group_ancestors_hashtable); - } slapi_log_err(SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM, "<-- memberof_postop_close\n" ); return 0; @@ -554,7 +515,7 @@ int memberof_postop_del(Slapi_PBlock *pb) { int ret = SLAPI_PLUGIN_SUCCESS; MemberOfConfig *mainConfig = NULL; - MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}; + MemberOfConfig configCopy = {0}; Slapi_DN *sdn; void *caller_id = NULL; @@ -583,9 +544,6 @@ int memberof_postop_del(Slapi_PBlock *pb) } memberof_copy_config(&configCopy, memberof_get_config()); memberof_unlock_config(); - - /* get the memberOf operation lock */ - memberof_lock(); /* remove this DN from the * membership lists of groups @@ -594,7 +552,6 @@ int memberof_postop_del(Slapi_PBlock *pb) slapi_log_err(SLAPI_LOG_ERR, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_postop_del - Error deleting dn (%s) from group. Error (%d)\n", slapi_sdn_get_dn(sdn),ret); - memberof_unlock(); goto bail; } @@ -618,7 +575,6 @@ int memberof_postop_del(Slapi_PBlock *pb) } } } - memberof_unlock(); bail: memberof_free_config(&configCopy); } @@ -813,7 +769,7 @@ memberof_call_foreach_dn(Slapi_PBlock *pb __attribute__((unused)), Slapi_DN *sdn memberof_cached_value *ht_grp = NULL; const char *ndn = slapi_sdn_get_ndn(sdn); - ht_grp = ancestors_cache_lookup((const void *) ndn); + ht_grp = ancestors_cache_lookup(config, (const void *) ndn); if (ht_grp) { #if MEMBEROF_CACHE_DEBUG slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_call_foreach_dn: Ancestors of %s already cached (%x)\n", ndn, ht_grp); @@ -960,7 +916,7 @@ int memberof_postop_modrdn(Slapi_PBlock *pb) if(memberof_oktodo(pb)) { MemberOfConfig *mainConfig = 0; - MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}; + MemberOfConfig configCopy = {0}; struct slapi_entry *pre_e = NULL; struct slapi_entry *post_e = NULL; Slapi_DN *pre_sdn = 0; @@ -988,8 +944,6 @@ int memberof_postop_modrdn(Slapi_PBlock *pb) goto bail; } - memberof_lock(); - /* update any downstream members */ if(pre_sdn && post_sdn && configCopy.group_filter && 0 == slapi_filter_test_simple(post_e, configCopy.group_filter)) @@ -1060,7 +1014,6 @@ int memberof_postop_modrdn(Slapi_PBlock *pb) } } } - memberof_unlock(); bail: memberof_free_config(&configCopy); } @@ -1220,7 +1173,7 @@ int memberof_postop_modify(Slapi_PBlock *pb) { int config_copied = 0; MemberOfConfig *mainConfig = 0; - MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}; + MemberOfConfig configCopy = {0}; /* get the mod set */ slapi_pblock_get(pb, SLAPI_MODIFY_MODS, &mods); @@ -1267,8 +1220,6 @@ int memberof_postop_modify(Slapi_PBlock *pb) { int op = slapi_mod_get_operation(smod); - memberof_lock(); - /* the modify op decides the function */ switch(op & ~LDAP_MOD_BVALUES) { @@ -1280,7 +1231,6 @@ int memberof_postop_modify(Slapi_PBlock *pb) "memberof_postop_modify - Failed to add dn (%s) to target. " "Error (%d)\n", slapi_sdn_get_dn(sdn), ret ); slapi_mod_done(next_mod); - memberof_unlock(); goto bail; } break; @@ -1299,7 +1249,6 @@ int memberof_postop_modify(Slapi_PBlock *pb) "memberof_postop_modify - Failed to replace list (%s). " "Error (%d)\n", slapi_sdn_get_dn(sdn), ret ); slapi_mod_done(next_mod); - memberof_unlock(); goto bail; } } @@ -1311,7 +1260,6 @@ int memberof_postop_modify(Slapi_PBlock *pb) "memberof_postop_modify: failed to remove dn (%s). " "Error (%d)\n", slapi_sdn_get_dn(sdn), ret ); slapi_mod_done(next_mod); - memberof_unlock(); goto bail; } } @@ -1326,7 +1274,6 @@ int memberof_postop_modify(Slapi_PBlock *pb) "memberof_postop_modify - Failed to replace values in dn (%s). " "Error (%d)\n", slapi_sdn_get_dn(sdn), ret ); slapi_mod_done(next_mod); - memberof_unlock(); goto bail; } break; @@ -1342,8 +1289,6 @@ int memberof_postop_modify(Slapi_PBlock *pb) break; } } - - memberof_unlock(); } slapi_mod_done(next_mod); @@ -1398,7 +1343,7 @@ int memberof_postop_add(Slapi_PBlock *pb) if(memberof_oktodo(pb) && (sdn = memberof_getsdn(pb))) { struct slapi_entry *e = NULL; - MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}; + MemberOfConfig configCopy = {0}; MemberOfConfig *mainConfig; slapi_pblock_get( pb, SLAPI_ENTRY_POST_OP, &e ); @@ -1424,8 +1369,6 @@ int memberof_postop_add(Slapi_PBlock *pb) int i = 0; Slapi_Attr *attr = 0; - memberof_lock(); - for (i = 0; configCopy.groupattrs && configCopy.groupattrs[i]; i++) { if(0 == slapi_entry_attr_find(e, configCopy.groupattrs[i], &attr)) @@ -1438,8 +1381,6 @@ int memberof_postop_add(Slapi_PBlock *pb) } } } - - memberof_unlock(); memberof_free_config(&configCopy); } } @@ -2201,7 +2142,7 @@ dump_cache_entry(memberof_cached_value *double_check, const char *msg) * the firsts elements of the array has 'valid=1' and the dn/ndn of group it belong to */ static void -cache_ancestors(Slapi_Value **member_ndn_val, memberof_get_groups_data *groups) +cache_ancestors(MemberOfConfig *config, Slapi_Value **member_ndn_val, memberof_get_groups_data *groups) { Slapi_ValueSet *groupvals = *((memberof_get_groups_data*)groups)->groupvals; Slapi_Value *sval; @@ -2298,14 +2239,14 @@ cache_ancestors(Slapi_Value **member_ndn_val, memberof_get_groups_data *groups) #if MEMBEROF_CACHE_DEBUG dump_cache_entry(cache_entry, key); #endif - if (ancestors_cache_add((const void*) key_copy, (void *) cache_entry) == NULL) { + if (ancestors_cache_add(config, (const void*) key_copy, (void *) cache_entry) == NULL) { slapi_log_err( SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache_ancestors: Failed to cache ancestor of %s\n", key); ancestor_hashtable_entry_free(cache_entry); slapi_ch_free ((void**)&cache_entry); return; } #if MEMBEROF_CACHE_DEBUG - if (double_check = ancestors_cache_lookup((const void*) key)) { + if (double_check = ancestors_cache_lookup(config, (const void*) key)) { dump_cache_entry(double_check, "read back"); } #endif @@ -2390,9 +2331,9 @@ memberof_get_groups_r(MemberOfConfig *config, Slapi_DN *member_sdn, memberof_get_groups_callback, &member_data, &cached, member_data.use_cache); merge_ancestors(&member_ndn_val, &member_data, data); - if (!cached && member_data.use_cache) - cache_ancestors(&member_ndn_val, &member_data); - + if (!cached && member_data.use_cache) { + cache_ancestors(config, &member_ndn_val, &member_data); + } slapi_value_free(&member_ndn_val); slapi_valueset_free(groupvals); @@ -2969,46 +2910,9 @@ int memberof_qsort_compare(const void *a, const void *b) val1, val2); } -/* betxn: This locking mechanism is necessary to guarantee the memberof - * consistency */ -void memberof_lock() -{ - if (usetxn) { - PR_EnterMonitor(memberof_operation_lock); - } - if (fixup_entry_hashtable) { - fixup_hashtable_empty("memberof_lock"); - } - if (group_ancestors_hashtable) { - ancestor_hashtable_empty("memberof_lock empty group_ancestors_hashtable"); - memset(&cache_stat, 0, sizeof(cache_stat)); - } -} - -void memberof_unlock() -{ - if (group_ancestors_hashtable) { - ancestor_hashtable_empty("memberof_unlock empty group_ancestors_hashtable"); -#if MEMBEROF_CACHE_DEBUG - slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache statistics: total lookup %d (success %d), add %d, remove %d, enum %d\n", - cache_stat.total_lookup, cache_stat.successfull_lookup, - cache_stat.total_add, cache_stat.total_remove, cache_stat.total_enumerate); - slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache statistics duration: lookup %ld, add %ld, remove %ld, enum %ld\n", - cache_stat.cumul_duration_lookup, cache_stat.cumul_duration_add, - cache_stat.cumul_duration_remove, cache_stat.cumul_duration_enumerate); -#endif - } - if (fixup_entry_hashtable) { - fixup_hashtable_empty("memberof_lock"); - } - if (usetxn) { - PR_ExitMonitor(memberof_operation_lock); - } -} - void memberof_fixup_task_thread(void *arg) { - MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}; + MemberOfConfig configCopy = {0}; Slapi_Task *task = (Slapi_Task *)arg; task_data *td = NULL; int rc = 0; @@ -3068,14 +2972,8 @@ void memberof_fixup_task_thread(void *arg) } } - /* get the memberOf operation lock */ - memberof_lock(); - /* do real work */ rc = memberof_fix_memberof(&configCopy, task, td); - - /* release the memberOf operation lock */ - memberof_unlock(); done: if (usetxn && fixup_pb) { @@ -3240,7 +3138,7 @@ int memberof_fix_memberof(MemberOfConfig *config, Slapi_Task *task, task_data *t } static memberof_cached_value * -ancestors_cache_lookup(const char *ndn) +ancestors_cache_lookup(MemberOfConfig *config, const char *ndn) { memberof_cached_value *e; #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME) @@ -3258,7 +3156,7 @@ ancestors_cache_lookup(const char *ndn) } #endif - e = (memberof_cached_value *) PL_HashTableLookupConst(group_ancestors_hashtable, (const void *) ndn); + e = (memberof_cached_value *) PL_HashTableLookupConst(config->ancestors_cache, (const void *) ndn); #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME) if (start) { @@ -3274,7 +3172,7 @@ ancestors_cache_lookup(const char *ndn) } static PRBool -ancestors_cache_remove(const char *ndn) +ancestors_cache_remove(MemberOfConfig *config, const char *ndn) { PRBool rc; #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME) @@ -3292,7 +3190,7 @@ ancestors_cache_remove(const char *ndn) } #endif - rc = PL_HashTableRemove(group_ancestors_hashtable, (const void *) ndn); + rc = PL_HashTableRemove(config->ancestors_cache, (const void *) ndn); #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME) if (start) { @@ -3305,7 +3203,7 @@ ancestors_cache_remove(const char *ndn) } static PLHashEntry * -ancestors_cache_add(const void *key, void *value) +ancestors_cache_add(MemberOfConfig *config, const void *key, void *value) { PLHashEntry *e; #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME) @@ -3322,7 +3220,7 @@ ancestors_cache_add(const void *key, void *value) } #endif - e = PL_HashTableAdd(group_ancestors_hashtable, key, value); + e = PL_HashTableAdd(config->ancestors_cache, key, value); #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME) if (start) { @@ -3360,10 +3258,11 @@ int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data) goto bail; } - /* Check if the entry has not already been fixed */ + /* Check if the entry has not already been fixed */ ndn = slapi_sdn_get_ndn(sdn); - if (ndn && fixup_entry_hashtable && PL_HashTableLookupConst(fixup_entry_hashtable, (void*) ndn)) { - slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: Entry %s already fixed up\n", ndn); + if (ndn && config->fixup_cache && PL_HashTableLookupConst(config->fixup_cache, (void*) ndn)) { + slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, + "memberof_fix_memberof_callback: Entry %s already fixed up\n", ndn); goto bail; } @@ -3383,9 +3282,9 @@ int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data) #if MEMBEROF_CACHE_DEBUG slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: This is NOT a group %s\n", ndn); #endif - ht_grp = ancestors_cache_lookup((const void *) ndn); + ht_grp = ancestors_cache_lookup(config, (const void *) ndn); if (ht_grp) { - if (ancestors_cache_remove((const void *) ndn)) { + if (ancestors_cache_remove(config, (const void *) ndn)) { slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: free cached values for %s\n", ndn); ancestor_hashtable_entry_free(ht_grp); slapi_ch_free((void **) &ht_grp); @@ -3400,6 +3299,7 @@ int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data) } } } + /* If we found some groups, replace the existing memberOf attribute * with the found values. */ if (groups && slapi_valueset_count(groups)) @@ -3439,9 +3339,9 @@ int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data) slapi_valueset_free(groups); /* records that this entry has been fixed up */ - if (fixup_entry_hashtable) { + if (config->fixup_cache) { dn_copy = slapi_ch_strdup(ndn); - if (PL_HashTableAdd(fixup_entry_hashtable, dn_copy, dn_copy) == NULL) { + if (PL_HashTableAdd(config->fixup_cache, dn_copy, dn_copy) == NULL) { slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: " "failed to add dn (%s) in the fixup hashtable; NSPR error - %d\n", dn_copy, PR_GetError()); @@ -3539,150 +3439,8 @@ memberof_add_objectclass(char *auto_add_oc, const char *dn) return rc; } -static PRIntn memberof_hash_compare_keys(const void *v1, const void *v2) -{ - PRIntn rc; - if (0 == strcasecmp((const char *) v1, (const char *) v2)) { - rc = 1; - } else { - rc = 0; - } - return rc; -} - -static PRIntn memberof_hash_compare_values(const void *v1, const void *v2) -{ - PRIntn rc; - if ((char *) v1 == (char *) v2) { - rc = 1; - } else { - rc = 0; - } - return rc; -} - -/* - * Hashing function using Bernstein's method - */ -static PLHashNumber memberof_hash_fn(const void *key) -{ - PLHashNumber hash = 5381; - unsigned char *x = (unsigned char *)key; - int c; - - while ((c = *x++)){ - hash = ((hash << 5) + hash) ^ c; - } - return hash; -} - -/* allocates the plugin hashtable - * This hash table is used by operation and is protected from - * concurrent operations with the memberof_lock (if not usetxn, memberof_lock - * is not implemented and the hash table will be not used. - * - * The hash table contains all the DN of the entries for which the memberof - * attribute has been computed/updated during the current operation - * - * hash table should be empty at the beginning and end of the plugin callback - */ -static PLHashTable *hashtable_new() -{ - if (!usetxn) { - return NULL; - } - - return PL_NewHashTable(MEMBEROF_HASHTABLE_SIZE, - memberof_hash_fn, - memberof_hash_compare_keys, - memberof_hash_compare_values, NULL, NULL); -} -/* this function called for each hash node during hash destruction */ -static PRIntn fixup_hashtable_remove(PLHashEntry *he, PRIntn index, void *arg) -{ - char *dn_copy; - - if (he == NULL) { - return HT_ENUMERATE_NEXT; - } - dn_copy = (char*) he->value; - slapi_ch_free_string(&dn_copy); - - return HT_ENUMERATE_REMOVE; -} - -static void fixup_hashtable_empty(char *msg) -{ - if (fixup_entry_hashtable) { - PL_HashTableEnumerateEntries(fixup_entry_hashtable, fixup_hashtable_remove, msg); - } -} - - -/* allocates the plugin hashtable - * This hash table is used by operation and is protected from - * concurrent operations with the memberof_lock (if not usetxn, memberof_lock - * is not implemented and the hash table will be not used. - * - * The hash table contains all the DN of the entries for which the memberof - * attribute has been computed/updated during the current operation - * - * hash table should be empty at the beginning and end of the plugin callback - */ - -static -void ancestor_hashtable_entry_free(memberof_cached_value *entry) -{ - int i; - for (i = 0; entry[i].valid; i++) { - slapi_ch_free((void **) &entry[i].group_dn_val); - slapi_ch_free((void **) &entry[i].group_ndn_val); - } - /* Here we are at the ending element containing the key */ - slapi_ch_free((void**) &entry[i].key); -} -/* this function called for each hash node during hash destruction */ -static PRIntn ancestor_hashtable_remove(PLHashEntry *he, PRIntn index, void *arg) +int +memberof_use_txn() { - memberof_cached_value *group_ancestor_array; - - if (he == NULL) - return HT_ENUMERATE_NEXT; - - - group_ancestor_array = (memberof_cached_value *) he->value; - ancestor_hashtable_entry_free(group_ancestor_array); - slapi_ch_free((void **)&group_ancestor_array); - - return HT_ENUMERATE_REMOVE; + return usetxn; } - -static void ancestor_hashtable_empty(char *msg) -{ -#if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME) - long int start; - struct timespec tsnow; -#endif - - if (group_ancestors_hashtable) { - cache_stat.total_enumerate++; -#if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME) - if (clock_gettime(CLOCK_REALTIME, &tsnow) != 0) { - start = 0; - } else { - start = tsnow.tv_nsec; - } -#endif - PL_HashTableEnumerateEntries(group_ancestors_hashtable, ancestor_hashtable_remove, msg); - -#if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME) - if (start) { - if (clock_gettime(CLOCK_REALTIME, &tsnow) == 0) { - cache_stat.cumul_duration_enumerate += (tsnow.tv_nsec - start); - } - } -#endif - } - -} - diff --git a/ldap/servers/plugins/memberof/memberof.h b/ldap/servers/plugins/memberof/memberof.h index 9a3a6a25d..a01c4d247 100644 --- a/ldap/servers/plugins/memberof/memberof.h +++ b/ldap/servers/plugins/memberof/memberof.h @@ -62,8 +62,22 @@ typedef struct memberofconfig { int skip_nested; int fixup_task; char *auto_add_oc; + PLHashTable *ancestors_cache; + PLHashTable *fixup_cache; } MemberOfConfig; +/* The key to access the hash table is the normalized DN + * The normalized DN is stored in the value because: + * - It is used in slapi_valueset_find + * - It is used to fill the memberof_get_groups_data.group_norm_vals + */ +typedef struct _memberof_cached_value +{ + char *key; + char *group_dn_val; + char *group_ndn_val; + int valid; +} memberof_cached_value; /* * functions @@ -88,5 +102,8 @@ int memberof_apply_config (Slapi_PBlock *pb, Slapi_Entry* entryBefore, Slapi_Ent void *memberof_get_plugin_id(void); void memberof_release_config(void); PRUint64 get_plugin_started(void); +void ancestor_hashtable_entry_free(memberof_cached_value *entry); +PLHashTable *hashtable_new(); +int memberof_use_txn(); #endif /* _MEMBEROF_H_ */ diff --git a/ldap/servers/plugins/memberof/memberof_config.c b/ldap/servers/plugins/memberof/memberof_config.c index c3474bf2c..3cc7c4d9c 100644 --- a/ldap/servers/plugins/memberof/memberof_config.c +++ b/ldap/servers/plugins/memberof/memberof_config.c @@ -14,12 +14,12 @@ * memberof_config.c - configuration-related code for memberOf plug-in * */ - +#include "plhash.h" #include - #include "memberof.h" #define MEMBEROF_CONFIG_FILTER "(objectclass=*)" +#define MEMBEROF_HASHTABLE_SIZE 1000 /* * The configuration attributes are contained in the plugin entry e.g. @@ -33,7 +33,9 @@ /* * function prototypes - */ + */ +static void fixup_hashtable_empty( MemberOfConfig *config, char *msg); +static void ancestor_hashtable_empty(MemberOfConfig *config, char *msg); static int memberof_validate_config (Slapi_PBlock *pb, Slapi_Entry* entryBefore, Slapi_Entry* e, int *returncode, char *returntext, void *arg); static int memberof_search (Slapi_PBlock *pb, Slapi_Entry* entryBefore, Slapi_Entry* e, @@ -48,7 +50,7 @@ static int memberof_search (Slapi_PBlock *pb, Slapi_Entry* entryBefore, Slapi_En /* This is the main configuration which is updated from dse.ldif. The * config will be copied when it is used by the plug-in to prevent it * being changed out from under a running memberOf operation. */ -static MemberOfConfig theConfig = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}; +static MemberOfConfig theConfig = {0}; static Slapi_RWLock *memberof_config_lock = 0; static int inited = 0; @@ -696,6 +698,12 @@ memberof_copy_config(MemberOfConfig *dest, MemberOfConfig *src) { if (dest && src) { + /* Allocate our caches here since we only copy the config at the start of an op */ + if (memberof_use_txn() == 1){ + dest->ancestors_cache = hashtable_new(); + dest->fixup_cache = hashtable_new(); + } + /* Check if the copy is already up to date */ if (src->groupattrs) { @@ -799,6 +807,14 @@ memberof_free_config(MemberOfConfig *config) slapi_ch_free_string(&config->memberof_attr); memberof_free_scope(config->entryScopes, &config->entryScopeCount); memberof_free_scope(config->entryScopeExcludeSubtrees, &config->entryExcludeScopeCount); + if (config->fixup_cache) { + fixup_hashtable_empty(config, "memberof_free_config empty fixup_entry_hastable"); + PL_HashTableDestroy(config->fixup_cache); + } + if (config->ancestors_cache) { + ancestor_hashtable_empty(config, "memberof_free_config empty group_ancestors_hashtable"); + PL_HashTableDestroy(config->ancestors_cache); + } } } @@ -1001,3 +1017,131 @@ bail: return ret; } + + +static PRIntn memberof_hash_compare_keys(const void *v1, const void *v2) +{ + PRIntn rc; + if (0 == strcasecmp((const char *) v1, (const char *) v2)) { + rc = 1; + } else { + rc = 0; + } + return rc; +} + +static PRIntn memberof_hash_compare_values(const void *v1, const void *v2) +{ + PRIntn rc; + if ((char *) v1 == (char *) v2) { + rc = 1; + } else { + rc = 0; + } + return rc; +} + +/* + * Hashing function using Bernstein's method + */ +static PLHashNumber memberof_hash_fn(const void *key) +{ + PLHashNumber hash = 5381; + unsigned char *x = (unsigned char *)key; + int c; + + while ((c = *x++)){ + hash = ((hash << 5) + hash) ^ c; + } + return hash; +} + +/* allocates the plugin hashtable + * This hash table is used by operation and is protected from + * concurrent operations with the memberof_lock (if not usetxn, memberof_lock + * is not implemented and the hash table will be not used. + * + * The hash table contains all the DN of the entries for which the memberof + * attribute has been computed/updated during the current operation + * + * hash table should be empty at the beginning and end of the plugin callback + */ +PLHashTable *hashtable_new(int usetxn) +{ + if (!usetxn) { + return NULL; + } + + return PL_NewHashTable(MEMBEROF_HASHTABLE_SIZE, + memberof_hash_fn, + memberof_hash_compare_keys, + memberof_hash_compare_values, NULL, NULL); +} + +/* this function called for each hash node during hash destruction */ +static PRIntn fixup_hashtable_remove(PLHashEntry *he, PRIntn index __attribute__((unused)), void *arg __attribute__((unused))) +{ + char *dn_copy; + + if (he == NULL) { + return HT_ENUMERATE_NEXT; + } + dn_copy = (char*) he->value; + slapi_ch_free_string(&dn_copy); + + return HT_ENUMERATE_REMOVE; +} + +static void fixup_hashtable_empty(MemberOfConfig *config, char *msg) +{ + if (config->fixup_cache) { + PL_HashTableEnumerateEntries(config->fixup_cache, fixup_hashtable_remove, msg); + } +} + + +/* allocates the plugin hashtable + * This hash table is used by operation and is protected from + * concurrent operations with the memberof_lock (if not usetxn, memberof_lock + * is not implemented and the hash table will be not used. + * + * The hash table contains all the DN of the entries for which the memberof + * attribute has been computed/updated during the current operation + * + * hash table should be empty at the beginning and end of the plugin callback + */ + +void ancestor_hashtable_entry_free(memberof_cached_value *entry) +{ + int i; + + for (i = 0; entry[i].valid; i++) { + slapi_ch_free((void **) &entry[i].group_dn_val); + slapi_ch_free((void **) &entry[i].group_ndn_val); + } + /* Here we are at the ending element containing the key */ + slapi_ch_free((void**) &entry[i].key); +} + +/* this function called for each hash node during hash destruction */ +static PRIntn ancestor_hashtable_remove(PLHashEntry *he, PRIntn index __attribute__((unused)), void *arg __attribute__((unused))) +{ + memberof_cached_value *group_ancestor_array; + + if (he == NULL) { + return HT_ENUMERATE_NEXT; + } + group_ancestor_array = (memberof_cached_value *) he->value; + ancestor_hashtable_entry_free(group_ancestor_array); + slapi_ch_free((void **)&group_ancestor_array); + + return HT_ENUMERATE_REMOVE; +} + +static void ancestor_hashtable_empty(MemberOfConfig *config, char *msg) +{ + if (config->ancestors_cache) { + PL_HashTableEnumerateEntries(config->ancestors_cache, ancestor_hashtable_remove, msg); + } + +} -- 2.13.6