Blame SOURCES/0007-Ticket-48235-Remove-memberOf-global-lock.patch

96373c
From cbe71d7e4901232eaa423b9dc55dba9401c05bec Mon Sep 17 00:00:00 2001
96373c
From: Mark Reynolds <mreynolds@redhat.com>
96373c
Date: Fri, 13 Oct 2017 07:09:08 -0400
96373c
Subject: [PATCH] Ticket 48235 - Remove memberOf global lock
96373c
96373c
Bug Description:  The memberOf global lock no longer servers a purpose since
96373c
                  the plugin is BETXN.  This was causing potential deadlocks
96373c
                  when multiple backends are used.
96373c
96373c
Fix Description:  Remove the lock, and rework the fixup/ancestors caches/hashtables.
96373c
                  Instead of reusing a single cache, we create a fresh cache
96373c
                  when we copy the plugin config (which only happens at the start
96373c
                  of an operation).  Then we destroy the caches when we free
96373c
                  the config.
96373c
96373c
https://pagure.io/389-ds-base/issue/48235
96373c
96373c
Reviewed by: firstyear & tbordaz(Thanks!!)
96373c
96373c
(cherry picked from commit 184b8a164f4ed456c72d58038aa9a0d512be61fa)
96373c
---
96373c
 ldap/servers/plugins/memberof/memberof.c        | 326 +++---------------------
96373c
 ldap/servers/plugins/memberof/memberof.h        |  17 ++
96373c
 ldap/servers/plugins/memberof/memberof_config.c | 166 +++++++++++-
96373c
 3 files changed, 210 insertions(+), 299 deletions(-)
96373c
96373c
diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c
96373c
index a0f997ddf..a23c52abe 100644
96373c
--- a/ldap/servers/plugins/memberof/memberof.c
96373c
+++ b/ldap/servers/plugins/memberof/memberof.c
96373c
@@ -48,14 +48,11 @@ static Slapi_PluginDesc pdesc = {"memberof", VENDOR,
96373c
 static void *_PluginID = NULL;
96373c
 static Slapi_DN *_ConfigAreaDN = NULL;
96373c
 static Slapi_RWLock *config_rwlock = NULL;
96373c
-static Slapi_DN *_pluginDN = NULL;
96373c
-static PRMonitor *memberof_operation_lock = 0;
96373c
+static Slapi_DN* _pluginDN = NULL;
96373c
 MemberOfConfig *qsortConfig = 0;
96373c
 static int usetxn = 0;
96373c
 static int premodfn = 0;
96373c
-#define MEMBEROF_HASHTABLE_SIZE 1000
96373c
-static PLHashTable *fixup_entry_hashtable = NULL;     /* global hash table protected by memberof_lock (memberof_operation_lock) */
96373c
-static PLHashTable *group_ancestors_hashtable = NULL; /* global hash table protected by memberof_lock (memberof_operation_lock) */
96373c
+
96373c
 
96373c
 typedef struct _memberofstringll
96373c
 {
96373c
@@ -73,18 +70,6 @@ typedef struct _memberof_get_groups_data
96373c
     PRBool use_cache;
96373c
 } memberof_get_groups_data;
96373c
 
96373c
-/* The key to access the hash table is the normalized DN
96373c
- * The normalized DN is stored in the value because:
96373c
- *  - It is used in slapi_valueset_find
96373c
- *  - It is used to fill the memberof_get_groups_data.group_norm_vals
96373c
- */
96373c
-typedef struct _memberof_cached_value
96373c
-{
96373c
-    char *key;
96373c
-    char *group_dn_val;
96373c
-    char *group_ndn_val;
96373c
-    int valid;
96373c
-} memberof_cached_value;
96373c
 struct cache_stat
96373c
 {
96373c
     int total_lookup;
96373c
@@ -164,14 +149,9 @@ static int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data);
96373c
 static int memberof_entry_in_scope(MemberOfConfig *config, Slapi_DN *sdn);
96373c
 static int memberof_add_objectclass(char *auto_add_oc, const char *dn);
96373c
 static int memberof_add_memberof_attr(LDAPMod **mods, const char *dn, char *add_oc);
96373c
-static PLHashTable *hashtable_new();
96373c
-static void fixup_hashtable_empty(char *msg);
96373c
-static PLHashTable *hashtable_new();
96373c
-static void ancestor_hashtable_empty(char *msg);
96373c
-static void ancestor_hashtable_entry_free(memberof_cached_value *entry);
96373c
-static memberof_cached_value *ancestors_cache_lookup(const char *ndn);
96373c
-static PRBool ancestors_cache_remove(const char *ndn);
96373c
-static PLHashEntry *ancestors_cache_add(const void *key, void *value);
96373c
+static memberof_cached_value *ancestors_cache_lookup(MemberOfConfig *config, const char *ndn);
96373c
+static PRBool ancestors_cache_remove(MemberOfConfig *config, const char *ndn);
96373c
+static PLHashEntry *ancestors_cache_add(MemberOfConfig *config, const void *key, void *value);
96373c
 
96373c
 /*** implementation ***/
96373c
 
96373c
@@ -344,11 +324,6 @@ memberof_postop_start(Slapi_PBlock *pb)
96373c
     slapi_log_err(SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM,
96373c
                   "--> memberof_postop_start\n");
96373c
 
96373c
-    memberof_operation_lock = PR_NewMonitor();
96373c
-    if (0 == memberof_operation_lock) {
96373c
-        rc = -1;
96373c
-        goto bail;
96373c
-    }
96373c
     if (config_rwlock == NULL) {
96373c
         if ((config_rwlock = slapi_new_rwlock()) == NULL) {
96373c
             rc = -1;
96373c
@@ -356,9 +331,6 @@ memberof_postop_start(Slapi_PBlock *pb)
96373c
         }
96373c
     }
96373c
 
96373c
-    fixup_entry_hashtable = hashtable_new();
96373c
-    group_ancestors_hashtable = hashtable_new();
96373c
-
96373c
     /* Set the alternate config area if one is defined. */
96373c
     slapi_pblock_get(pb, SLAPI_PLUGIN_CONFIG_AREA, &config_area);
96373c
     if (config_area) {
96373c
@@ -413,13 +385,13 @@ memberof_postop_start(Slapi_PBlock *pb)
96373c
         goto bail;
96373c
     }
96373c
 
96373c
-/*
96373c
+    /*
96373c
      * TODO: start up operation actor thread
96373c
      * need to get to a point where server failure
96373c
-         * or shutdown doesn't hose our operations
96373c
-         * so we should create a task entry that contains
96373c
+     * or shutdown doesn't hose our operations
96373c
+     * so we should create a task entry that contains
96373c
      * all required information to complete the operation
96373c
-         * then the tasks can be restarted safely if
96373c
+     * then the tasks can be restarted safely if
96373c
      * interrupted
96373c
      */
96373c
 
96373c
@@ -451,18 +423,7 @@ memberof_postop_close(Slapi_PBlock *pb __attribute__((unused)))
96373c
     slapi_sdn_free(&_pluginDN);
96373c
     slapi_destroy_rwlock(config_rwlock);
96373c
     config_rwlock = NULL;
96373c
-    PR_DestroyMonitor(memberof_operation_lock);
96373c
-    memberof_operation_lock = NULL;
96373c
-
96373c
-    if (fixup_entry_hashtable) {
96373c
-        fixup_hashtable_empty("memberof_postop_close empty fixup_entry_hastable");
96373c
-        PL_HashTableDestroy(fixup_entry_hashtable);
96373c
-    }
96373c
 
96373c
-    if (group_ancestors_hashtable) {
96373c
-        ancestor_hashtable_empty("memberof_postop_close empty group_ancestors_hashtable");
96373c
-        PL_HashTableDestroy(group_ancestors_hashtable);
96373c
-    }
96373c
     slapi_log_err(SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM,
96373c
                   "<-- memberof_postop_close\n");
96373c
     return 0;
96373c
@@ -524,7 +485,7 @@ memberof_postop_del(Slapi_PBlock *pb)
96373c
 {
96373c
     int ret = SLAPI_PLUGIN_SUCCESS;
96373c
     MemberOfConfig *mainConfig = NULL;
96373c
-    MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
96373c
+    MemberOfConfig configCopy = {0};
96373c
     Slapi_DN *sdn;
96373c
     void *caller_id = NULL;
96373c
 
96373c
@@ -553,9 +514,6 @@ memberof_postop_del(Slapi_PBlock *pb)
96373c
         memberof_copy_config(&configCopy, memberof_get_config());
96373c
         memberof_unlock_config();
96373c
 
96373c
-        /* get the memberOf operation lock */
96373c
-        memberof_lock();
96373c
-
96373c
         /* remove this DN from the
96373c
          * membership lists of groups
96373c
          */
96373c
@@ -563,7 +521,6 @@ memberof_postop_del(Slapi_PBlock *pb)
96373c
             slapi_log_err(SLAPI_LOG_ERR, MEMBEROF_PLUGIN_SUBSYSTEM,
96373c
                           "memberof_postop_del - Error deleting dn (%s) from group. Error (%d)\n",
96373c
                           slapi_sdn_get_dn(sdn), ret);
96373c
-            memberof_unlock();
96373c
             goto bail;
96373c
         }
96373c
 
96373c
@@ -583,7 +540,6 @@ memberof_postop_del(Slapi_PBlock *pb)
96373c
                 }
96373c
             }
96373c
         }
96373c
-        memberof_unlock();
96373c
     bail:
96373c
         memberof_free_config(&configCopy);
96373c
     }
96373c
@@ -776,7 +732,7 @@ memberof_call_foreach_dn(Slapi_PBlock *pb __attribute__((unused)), Slapi_DN *sdn
96373c
         memberof_cached_value *ht_grp = NULL;
96373c
         const char *ndn = slapi_sdn_get_ndn(sdn);
96373c
 
96373c
-        ht_grp = ancestors_cache_lookup((const void *)ndn);
96373c
+        ht_grp = ancestors_cache_lookup(config, (const void *)ndn);
96373c
         if (ht_grp) {
96373c
 #if MEMBEROF_CACHE_DEBUG
96373c
             slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_call_foreach_dn: Ancestors of %s already cached (%x)\n", ndn, ht_grp);
96373c
@@ -918,7 +874,7 @@ memberof_postop_modrdn(Slapi_PBlock *pb)
96373c
 
96373c
     if (memberof_oktodo(pb)) {
96373c
         MemberOfConfig *mainConfig = 0;
96373c
-        MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
96373c
+        MemberOfConfig configCopy = {0};
96373c
         struct slapi_entry *pre_e = NULL;
96373c
         struct slapi_entry *post_e = NULL;
96373c
         Slapi_DN *pre_sdn = 0;
96373c
@@ -944,8 +900,6 @@ memberof_postop_modrdn(Slapi_PBlock *pb)
96373c
             goto bail;
96373c
         }
96373c
 
96373c
-        memberof_lock();
96373c
-
96373c
         /*  update any downstream members */
96373c
         if (pre_sdn && post_sdn && configCopy.group_filter &&
96373c
             0 == slapi_filter_test_simple(post_e, configCopy.group_filter)) {
96373c
@@ -1010,7 +964,6 @@ memberof_postop_modrdn(Slapi_PBlock *pb)
96373c
                 }
96373c
             }
96373c
         }
96373c
-        memberof_unlock();
96373c
     bail:
96373c
         memberof_free_config(&configCopy);
96373c
     }
96373c
@@ -1166,7 +1119,7 @@ memberof_postop_modify(Slapi_PBlock *pb)
96373c
     if (memberof_oktodo(pb)) {
96373c
         int config_copied = 0;
96373c
         MemberOfConfig *mainConfig = 0;
96373c
-        MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
96373c
+        MemberOfConfig configCopy = {0};
96373c
 
96373c
         /* get the mod set */
96373c
         slapi_pblock_get(pb, SLAPI_MODIFY_MODS, &mods);
96373c
@@ -1209,8 +1162,6 @@ memberof_postop_modify(Slapi_PBlock *pb)
96373c
             if (interested) {
96373c
                 int op = slapi_mod_get_operation(smod);
96373c
 
96373c
-                memberof_lock();
96373c
-
96373c
                 /* the modify op decides the function */
96373c
                 switch (op & ~LDAP_MOD_BVALUES) {
96373c
                 case LDAP_MOD_ADD: {
96373c
@@ -1221,7 +1172,6 @@ memberof_postop_modify(Slapi_PBlock *pb)
96373c
                                       "Error (%d)\n",
96373c
                                       slapi_sdn_get_dn(sdn), ret);
96373c
                         slapi_mod_done(next_mod);
96373c
-                        memberof_unlock();
96373c
                         goto bail;
96373c
                     }
96373c
                     break;
96373c
@@ -1239,7 +1189,6 @@ memberof_postop_modify(Slapi_PBlock *pb)
96373c
                                           "Error (%d)\n",
96373c
                                           slapi_sdn_get_dn(sdn), ret);
96373c
                             slapi_mod_done(next_mod);
96373c
-                            memberof_unlock();
96373c
                             goto bail;
96373c
                         }
96373c
                     } else {
96373c
@@ -1250,7 +1199,6 @@ memberof_postop_modify(Slapi_PBlock *pb)
96373c
                                           "Error (%d)\n",
96373c
                                           slapi_sdn_get_dn(sdn), ret);
96373c
                             slapi_mod_done(next_mod);
96373c
-                            memberof_unlock();
96373c
                             goto bail;
96373c
                         }
96373c
                     }
96373c
@@ -1265,7 +1213,6 @@ memberof_postop_modify(Slapi_PBlock *pb)
96373c
                                       "Error (%d)\n",
96373c
                                       slapi_sdn_get_dn(sdn), ret);
96373c
                         slapi_mod_done(next_mod);
96373c
-                        memberof_unlock();
96373c
                         goto bail;
96373c
                     }
96373c
                     break;
96373c
@@ -1280,8 +1227,6 @@ memberof_postop_modify(Slapi_PBlock *pb)
96373c
                     break;
96373c
                 }
96373c
                 }
96373c
-
96373c
-                memberof_unlock();
96373c
             }
96373c
 
96373c
             slapi_mod_done(next_mod);
96373c
@@ -1336,7 +1281,7 @@ memberof_postop_add(Slapi_PBlock *pb)
96373c
 
96373c
     if (memberof_oktodo(pb) && (sdn = memberof_getsdn(pb))) {
96373c
         struct slapi_entry *e = NULL;
96373c
-        MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
96373c
+        MemberOfConfig configCopy = {0};
96373c
         MemberOfConfig *mainConfig;
96373c
         slapi_pblock_get(pb, SLAPI_ENTRY_POST_OP, &e);
96373c
 
96373c
@@ -1361,8 +1306,6 @@ memberof_postop_add(Slapi_PBlock *pb)
96373c
             int i = 0;
96373c
             Slapi_Attr *attr = 0;
96373c
 
96373c
-            memberof_lock();
96373c
-
96373c
             for (i = 0; configCopy.groupattrs && configCopy.groupattrs[i]; i++) {
96373c
                 if (0 == slapi_entry_attr_find(e, configCopy.groupattrs[i], &attr)) {
96373c
                     if ((ret = memberof_add_attr_list(pb, &configCopy, sdn, attr))) {
96373c
@@ -1373,8 +1316,6 @@ memberof_postop_add(Slapi_PBlock *pb)
96373c
                     }
96373c
                 }
96373c
             }
96373c
-
96373c
-            memberof_unlock();
96373c
             memberof_free_config(&configCopy);
96373c
         }
96373c
     }
96373c
@@ -2094,7 +2035,7 @@ dump_cache_entry(memberof_cached_value *double_check, const char *msg)
96373c
  * the firsts elements of the array has 'valid=1' and the dn/ndn of group it belong to
96373c
  */
96373c
 static void
96373c
-cache_ancestors(Slapi_Value **member_ndn_val, memberof_get_groups_data *groups)
96373c
+cache_ancestors(MemberOfConfig *config, Slapi_Value **member_ndn_val, memberof_get_groups_data *groups)
96373c
 {
96373c
     Slapi_ValueSet *groupvals = *((memberof_get_groups_data *)groups)->groupvals;
96373c
     Slapi_Value *sval;
96373c
@@ -2191,14 +2132,14 @@ cache_ancestors(Slapi_Value **member_ndn_val, memberof_get_groups_data *groups)
96373c
 #if MEMBEROF_CACHE_DEBUG
96373c
     dump_cache_entry(cache_entry, key);
96373c
 #endif
96373c
-    if (ancestors_cache_add((const void *)key_copy, (void *)cache_entry) == NULL) {
96373c
-        slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache_ancestors: Failed to cache ancestor of %s\n", key);
96373c
+    if (ancestors_cache_add(config, (const void*) key_copy, (void *) cache_entry) == NULL) {
96373c
+        slapi_log_err( SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache_ancestors: Failed to cache ancestor of %s\n", key);
96373c
         ancestor_hashtable_entry_free(cache_entry);
96373c
-        slapi_ch_free((void **)&cache_entry);
96373c
+        slapi_ch_free ((void**)&cache_entry);
96373c
         return;
96373c
     }
96373c
 #if MEMBEROF_CACHE_DEBUG
96373c
-    if (double_check = ancestors_cache_lookup((const void *)key)) {
96373c
+    if (double_check = ancestors_cache_lookup(config, (const void*) key)) {
96373c
         dump_cache_entry(double_check, "read back");
96373c
     }
96373c
 #endif
96373c
@@ -2283,8 +2224,7 @@ memberof_get_groups_r(MemberOfConfig *config, Slapi_DN *member_sdn, memberof_get
96373c
 
96373c
     merge_ancestors(&member_ndn_val, &member_data, data);
96373c
     if (!cached && member_data.use_cache)
96373c
-        cache_ancestors(&member_ndn_val, &member_data);
96373c
-
96373c
+        cache_ancestors(config, &member_ndn_val, &member_data);
96373c
 
96373c
     slapi_value_free(&member_ndn_val);
96373c
     slapi_valueset_free(groupvals);
96373c
@@ -2825,49 +2765,10 @@ memberof_qsort_compare(const void *a, const void *b)
96373c
                                     val1, val2);
96373c
 }
96373c
 
96373c
-/* betxn: This locking mechanism is necessary to guarantee the memberof
96373c
- * consistency */
96373c
-void
96373c
-memberof_lock()
96373c
-{
96373c
-    if (usetxn) {
96373c
-        PR_EnterMonitor(memberof_operation_lock);
96373c
-    }
96373c
-    if (fixup_entry_hashtable) {
96373c
-        fixup_hashtable_empty("memberof_lock");
96373c
-    }
96373c
-    if (group_ancestors_hashtable) {
96373c
-        ancestor_hashtable_empty("memberof_lock empty group_ancestors_hashtable");
96373c
-        memset(&cache_stat, 0, sizeof(cache_stat));
96373c
-    }
96373c
-}
96373c
-
96373c
-void
96373c
-memberof_unlock()
96373c
-{
96373c
-    if (group_ancestors_hashtable) {
96373c
-        ancestor_hashtable_empty("memberof_unlock empty group_ancestors_hashtable");
96373c
-#if MEMBEROF_CACHE_DEBUG
96373c
-        slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache statistics: total lookup %d (success %d), add %d, remove %d, enum %d\n",
96373c
-                      cache_stat.total_lookup, cache_stat.successfull_lookup,
96373c
-                      cache_stat.total_add, cache_stat.total_remove, cache_stat.total_enumerate);
96373c
-        slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache statistics duration: lookup %ld, add %ld, remove %ld, enum %ld\n",
96373c
-                      cache_stat.cumul_duration_lookup, cache_stat.cumul_duration_add,
96373c
-                      cache_stat.cumul_duration_remove, cache_stat.cumul_duration_enumerate);
96373c
-#endif
96373c
-    }
96373c
-    if (fixup_entry_hashtable) {
96373c
-        fixup_hashtable_empty("memberof_lock");
96373c
-    }
96373c
-    if (usetxn) {
96373c
-        PR_ExitMonitor(memberof_operation_lock);
96373c
-    }
96373c
-}
96373c
-
96373c
 void
96373c
 memberof_fixup_task_thread(void *arg)
96373c
 {
96373c
-    MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
96373c
+    MemberOfConfig configCopy = {0};
96373c
     Slapi_Task *task = (Slapi_Task *)arg;
96373c
     task_data *td = NULL;
96373c
     int rc = 0;
96373c
@@ -2933,9 +2834,6 @@ memberof_fixup_task_thread(void *arg)
96373c
     /* do real work */
96373c
     rc = memberof_fix_memberof(&configCopy, task, td);
96373c
 
96373c
-    /* release the memberOf operation lock */
96373c
-    memberof_unlock();
96373c
-
96373c
 done:
96373c
     if (usetxn && fixup_pb) {
96373c
         if (rc) { /* failed */
96373c
@@ -3100,7 +2998,7 @@ memberof_fix_memberof(MemberOfConfig *config, Slapi_Task *task, task_data *td)
96373c
 }
96373c
 
96373c
 static memberof_cached_value *
96373c
-ancestors_cache_lookup(const char *ndn)
96373c
+ancestors_cache_lookup(MemberOfConfig *config, const char *ndn)
96373c
 {
96373c
     memberof_cached_value *e;
96373c
 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
96373c
@@ -3118,7 +3016,7 @@ ancestors_cache_lookup(const char *ndn)
96373c
     }
96373c
 #endif
96373c
 
96373c
-    e = (memberof_cached_value *)PL_HashTableLookupConst(group_ancestors_hashtable, (const void *)ndn);
96373c
+    e = (memberof_cached_value *) PL_HashTableLookupConst(config->ancestors_cache, (const void *) ndn);
96373c
 
96373c
 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
96373c
     if (start) {
96373c
@@ -3133,7 +3031,7 @@ ancestors_cache_lookup(const char *ndn)
96373c
     return e;
96373c
 }
96373c
 static PRBool
96373c
-ancestors_cache_remove(const char *ndn)
96373c
+ancestors_cache_remove(MemberOfConfig *config, const char *ndn)
96373c
 {
96373c
     PRBool rc;
96373c
 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
96373c
@@ -3151,7 +3049,8 @@ ancestors_cache_remove(const char *ndn)
96373c
     }
96373c
 #endif
96373c
 
96373c
-    rc = PL_HashTableRemove(group_ancestors_hashtable, (const void *)ndn);
96373c
+
96373c
+    rc = PL_HashTableRemove(config->ancestors_cache, (const void *)ndn);
96373c
 
96373c
 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
96373c
     if (start) {
96373c
@@ -3164,7 +3063,7 @@ ancestors_cache_remove(const char *ndn)
96373c
 }
96373c
 
96373c
 static PLHashEntry *
96373c
-ancestors_cache_add(const void *key, void *value)
96373c
+ancestors_cache_add(MemberOfConfig *config, const void *key, void *value)
96373c
 {
96373c
     PLHashEntry *e;
96373c
 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
96373c
@@ -3181,7 +3080,7 @@ ancestors_cache_add(const void *key, void *value)
96373c
     }
96373c
 #endif
96373c
 
96373c
-    e = PL_HashTableAdd(group_ancestors_hashtable, key, value);
96373c
+    e = PL_HashTableAdd(config->ancestors_cache, key, value);
96373c
 
96373c
 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
96373c
     if (start) {
96373c
@@ -3211,7 +3110,6 @@ memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data)
96373c
     const char *ndn;
96373c
     char *dn_copy;
96373c
 
96373c
-
96373c
     /*
96373c
      * If the server is ordered to shutdown, stop the fixup and return an error.
96373c
      */
96373c
@@ -3222,7 +3120,7 @@ memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data)
96373c
 
96373c
     /* Check if the entry has not already been fixed */
96373c
     ndn = slapi_sdn_get_ndn(sdn);
96373c
-    if (ndn && fixup_entry_hashtable && PL_HashTableLookupConst(fixup_entry_hashtable, (void *)ndn)) {
96373c
+    if (ndn && config->fixup_cache && PL_HashTableLookupConst(config->fixup_cache, (void *)ndn)) {
96373c
         slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: Entry %s already fixed up\n", ndn);
96373c
         goto bail;
96373c
     }
96373c
@@ -3240,12 +3138,13 @@ memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data)
96373c
              * so free this memory
96373c
              */
96373c
             ndn = slapi_sdn_get_ndn(sdn);
96373c
+
96373c
 #if MEMBEROF_CACHE_DEBUG
96373c
             slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: This is NOT a group %s\n", ndn);
96373c
 #endif
96373c
-            ht_grp = ancestors_cache_lookup((const void *)ndn);
96373c
+            ht_grp = ancestors_cache_lookup(config, (const void *)ndn);
96373c
             if (ht_grp) {
96373c
-                if (ancestors_cache_remove((const void *)ndn)) {
96373c
+                if (ancestors_cache_remove(config, (const void *)ndn)) {
96373c
                     slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: free cached values for %s\n", ndn);
96373c
                     ancestor_hashtable_entry_free(ht_grp);
96373c
                     slapi_ch_free((void **)&ht_grp);
96373c
@@ -3297,11 +3196,11 @@ memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data)
96373c
     slapi_valueset_free(groups);
96373c
 
96373c
     /* records that this entry has been fixed up */
96373c
-    if (fixup_entry_hashtable) {
96373c
+    if (config->fixup_cache) {
96373c
         dn_copy = slapi_ch_strdup(ndn);
96373c
-        if (PL_HashTableAdd(fixup_entry_hashtable, dn_copy, dn_copy) == NULL) {
96373c
+        if (PL_HashTableAdd(config->fixup_cache, dn_copy, dn_copy) == NULL) {
96373c
             slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: "
96373c
-                                                                      "failed to add dn (%s) in the fixup hashtable; NSPR error - %d\n",
96373c
+                          "failed to add dn (%s) in the fixup hashtable; NSPR error - %d\n",
96373c
                           dn_copy, PR_GetError());
96373c
             slapi_ch_free((void **)&dn_copy);
96373c
             /* let consider this as not a fatal error, it just skip an optimization */
96373c
@@ -3397,157 +3296,8 @@ memberof_add_objectclass(char *auto_add_oc, const char *dn)
96373c
     return rc;
96373c
 }
96373c
 
96373c
-static PRIntn
96373c
-memberof_hash_compare_keys(const void *v1, const void *v2)
96373c
-{
96373c
-    PRIntn rc;
96373c
-    if (0 == strcasecmp((const char *)v1, (const char *)v2)) {
96373c
-        rc = 1;
96373c
-    } else {
96373c
-        rc = 0;
96373c
-    }
96373c
-    return rc;
96373c
-}
96373c
-
96373c
-static PRIntn
96373c
-memberof_hash_compare_values(const void *v1, const void *v2)
96373c
-{
96373c
-    PRIntn rc;
96373c
-    if ((char *)v1 == (char *)v2) {
96373c
-        rc = 1;
96373c
-    } else {
96373c
-        rc = 0;
96373c
-    }
96373c
-    return rc;
96373c
-}
96373c
-
96373c
-/*
96373c
- *  Hashing function using Bernstein's method
96373c
- */
96373c
-static PLHashNumber
96373c
-memberof_hash_fn(const void *key)
96373c
-{
96373c
-    PLHashNumber hash = 5381;
96373c
-    unsigned char *x = (unsigned char *)key;
96373c
-    int c;
96373c
-
96373c
-    while ((c = *x++)) {
96373c
-        hash = ((hash << 5) + hash) ^ c;
96373c
-    }
96373c
-    return hash;
96373c
-}
96373c
-
96373c
-/* allocates the plugin hashtable
96373c
- * This hash table is used by operation and is protected from
96373c
- * concurrent operations with the memberof_lock (if not usetxn, memberof_lock
96373c
- * is not implemented and the hash table will be not used.
96373c
- *
96373c
- * The hash table contains all the DN of the entries for which the memberof
96373c
- * attribute has been computed/updated during the current operation
96373c
- *
96373c
- * hash table should be empty at the beginning and end of the plugin callback
96373c
- */
96373c
-static PLHashTable *
96373c
-hashtable_new()
96373c
-{
96373c
-    if (!usetxn) {
96373c
-        return NULL;
96373c
-    }
96373c
-
96373c
-    return PL_NewHashTable(MEMBEROF_HASHTABLE_SIZE,
96373c
-                           memberof_hash_fn,
96373c
-                           memberof_hash_compare_keys,
96373c
-                           memberof_hash_compare_values, NULL, NULL);
96373c
-}
96373c
-/* this function called for each hash node during hash destruction */
96373c
-static PRIntn
96373c
-fixup_hashtable_remove(PLHashEntry *he, PRIntn index __attribute__((unused)), void *arg __attribute__((unused)))
96373c
-{
96373c
-    char *dn_copy;
96373c
-
96373c
-    if (he == NULL) {
96373c
-        return HT_ENUMERATE_NEXT;
96373c
-    }
96373c
-    dn_copy = (char *)he->value;
96373c
-    slapi_ch_free_string(&dn_copy);
96373c
-
96373c
-    return HT_ENUMERATE_REMOVE;
96373c
-}
96373c
-
96373c
-static void
96373c
-fixup_hashtable_empty(char *msg)
96373c
-{
96373c
-    if (fixup_entry_hashtable) {
96373c
-        PL_HashTableEnumerateEntries(fixup_entry_hashtable, fixup_hashtable_remove, msg);
96373c
-    }
96373c
-}
96373c
-
96373c
-
96373c
-/* allocates the plugin hashtable
96373c
- * This hash table is used by operation and is protected from
96373c
- * concurrent operations with the memberof_lock (if not usetxn, memberof_lock
96373c
- * is not implemented and the hash table will be not used.
96373c
- *
96373c
- * The hash table contains all the DN of the entries for which the memberof
96373c
- * attribute has been computed/updated during the current operation
96373c
- *
96373c
- * hash table should be empty at the beginning and end of the plugin callback
96373c
- */
96373c
-
96373c
-static void
96373c
-ancestor_hashtable_entry_free(memberof_cached_value *entry)
96373c
-{
96373c
-    int i;
96373c
-    for (i = 0; entry[i].valid; i++) {
96373c
-        slapi_ch_free((void **)&entry[i].group_dn_val);
96373c
-        slapi_ch_free((void **)&entry[i].group_ndn_val);
96373c
-    }
96373c
-    /* Here we are at the ending element containing the key */
96373c
-    slapi_ch_free((void **)&entry[i].key);
96373c
-}
96373c
-/* this function called for each hash node during hash destruction */
96373c
-static PRIntn
96373c
-ancestor_hashtable_remove(PLHashEntry *he, PRIntn index __attribute__((unused)), void *arg __attribute__((unused)))
96373c
-{
96373c
-    memberof_cached_value *group_ancestor_array;
96373c
-
96373c
-    if (he == NULL) {
96373c
-        return HT_ENUMERATE_NEXT;
96373c
-    }
96373c
-
96373c
-
96373c
-    group_ancestor_array = (memberof_cached_value *)he->value;
96373c
-    ancestor_hashtable_entry_free(group_ancestor_array);
96373c
-    slapi_ch_free((void **)&group_ancestor_array);
96373c
-
96373c
-    return HT_ENUMERATE_REMOVE;
96373c
-}
96373c
-
96373c
-static void
96373c
-ancestor_hashtable_empty(char *msg)
96373c
+int
96373c
+memberof_use_txn()
96373c
 {
96373c
-#if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
96373c
-    long int start;
96373c
-    struct timespec tsnow;
96373c
-#endif
96373c
-
96373c
-    if (group_ancestors_hashtable) {
96373c
-        cache_stat.total_enumerate++;
96373c
-#if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
96373c
-        if (clock_gettime(CLOCK_REALTIME, &tsnow) != 0) {
96373c
-            start = 0;
96373c
-        } else {
96373c
-            start = tsnow.tv_nsec;
96373c
-        }
96373c
-#endif
96373c
-        PL_HashTableEnumerateEntries(group_ancestors_hashtable, ancestor_hashtable_remove, msg);
96373c
-
96373c
-#if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)
96373c
-        if (start) {
96373c
-            if (clock_gettime(CLOCK_REALTIME, &tsnow) == 0) {
96373c
-                cache_stat.cumul_duration_enumerate += (tsnow.tv_nsec - start);
96373c
-            }
96373c
-        }
96373c
-#endif
96373c
-    }
96373c
+    return usetxn;
96373c
 }
96373c
diff --git a/ldap/servers/plugins/memberof/memberof.h b/ldap/servers/plugins/memberof/memberof.h
96373c
index 4833ce221..ba64e9dfa 100644
96373c
--- a/ldap/servers/plugins/memberof/memberof.h
96373c
+++ b/ldap/servers/plugins/memberof/memberof.h
96373c
@@ -64,8 +64,22 @@ typedef struct memberofconfig
96373c
     int skip_nested;
96373c
     int fixup_task;
96373c
     char *auto_add_oc;
96373c
+    PLHashTable *ancestors_cache;
96373c
+    PLHashTable *fixup_cache;
96373c
 } MemberOfConfig;
96373c
 
96373c
+/* The key to access the hash table is the normalized DN
96373c
+ * The normalized DN is stored in the value because:
96373c
+ *  - It is used in slapi_valueset_find
96373c
+ *  - It is used to fill the memberof_get_groups_data.group_norm_vals
96373c
+ */
96373c
+typedef struct _memberof_cached_value
96373c
+{
96373c
+    char *key;
96373c
+    char *group_dn_val;
96373c
+    char *group_ndn_val;
96373c
+    int valid;
96373c
+} memberof_cached_value;
96373c
 
96373c
 /*
96373c
  * functions
96373c
@@ -89,5 +103,8 @@ int memberof_apply_config(Slapi_PBlock *pb, Slapi_Entry *entryBefore, Slapi_Entr
96373c
 void *memberof_get_plugin_id(void);
96373c
 void memberof_release_config(void);
96373c
 PRUint64 get_plugin_started(void);
96373c
+void ancestor_hashtable_entry_free(memberof_cached_value *entry);
96373c
+PLHashTable *hashtable_new();
96373c
+int memberof_use_txn();
96373c
 
96373c
 #endif /* _MEMBEROF_H_ */
96373c
diff --git a/ldap/servers/plugins/memberof/memberof_config.c b/ldap/servers/plugins/memberof/memberof_config.c
96373c
index c5ca4b137..3f22d95d6 100644
96373c
--- a/ldap/servers/plugins/memberof/memberof_config.c
96373c
+++ b/ldap/servers/plugins/memberof/memberof_config.c
96373c
@@ -14,12 +14,12 @@
96373c
  * memberof_config.c - configuration-related code for memberOf plug-in
96373c
  *
96373c
  */
96373c
-
96373c
+#include "plhash.h"
96373c
 #include <plstr.h>
96373c
-
96373c
 #include "memberof.h"
96373c
 
96373c
 #define MEMBEROF_CONFIG_FILTER "(objectclass=*)"
96373c
+#define MEMBEROF_HASHTABLE_SIZE 1000
96373c
 
96373c
 /*
96373c
  * The configuration attributes are contained in the plugin entry e.g.
96373c
@@ -34,14 +34,16 @@
96373c
 /*
96373c
  * function prototypes
96373c
  */
96373c
-static int memberof_validate_config(Slapi_PBlock *pb, Slapi_Entry *entryBefore, Slapi_Entry *e, int *returncode, char *returntext, void *arg);
96373c
-static int
96373c
-memberof_search(Slapi_PBlock *pb __attribute__((unused)),
96373c
-                Slapi_Entry *entryBefore __attribute__((unused)),
96373c
-                Slapi_Entry *e __attribute__((unused)),
96373c
-                int *returncode __attribute__((unused)),
96373c
-                char *returntext __attribute__((unused)),
96373c
-                void *arg __attribute__((unused)))
96373c
+static void fixup_hashtable_empty( MemberOfConfig *config, char *msg);
96373c
+static void ancestor_hashtable_empty(MemberOfConfig *config, char *msg);
96373c
+static int memberof_validate_config (Slapi_PBlock *pb, Slapi_Entry* entryBefore, Slapi_Entry* e, 
96373c
+										 int *returncode, char *returntext, void *arg);
96373c
+static int memberof_search (Slapi_PBlock *pb __attribute__((unused)),
96373c
+                            Slapi_Entry* entryBefore __attribute__((unused)),
96373c
+                            Slapi_Entry* e __attribute__((unused)),
96373c
+                            int *returncode __attribute__((unused)),
96373c
+                            char *returntext __attribute__((unused)),
96373c
+                            void *arg __attribute__((unused)))
96373c
 {
96373c
     return SLAPI_DSE_CALLBACK_OK;
96373c
 }
96373c
@@ -52,7 +54,7 @@ memberof_search(Slapi_PBlock *pb __attribute__((unused)),
96373c
 /* This is the main configuration which is updated from dse.ldif.  The
96373c
  * config will be copied when it is used by the plug-in to prevent it
96373c
  * being changed out from under a running memberOf operation. */
96373c
-static MemberOfConfig theConfig = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
96373c
+static MemberOfConfig theConfig = {0};
96373c
 static Slapi_RWLock *memberof_config_lock = 0;
96373c
 static int inited = 0;
96373c
 
96373c
@@ -693,6 +695,13 @@ void
96373c
 memberof_copy_config(MemberOfConfig *dest, MemberOfConfig *src)
96373c
 {
96373c
     if (dest && src) {
96373c
+
96373c
+        /* Allocate our caches here since we only copy the config at the start of an op */
96373c
+        if (memberof_use_txn() == 1){
96373c
+            dest->ancestors_cache = hashtable_new();
96373c
+            dest->fixup_cache = hashtable_new();
96373c
+        }
96373c
+
96373c
         /* Check if the copy is already up to date */
96373c
         if (src->groupattrs) {
96373c
             int i = 0, j = 0;
96373c
@@ -787,6 +796,14 @@ memberof_free_config(MemberOfConfig *config)
96373c
         slapi_ch_free_string(&config->memberof_attr);
96373c
         memberof_free_scope(&(config->entryScopes), &config->entryScopeCount);
96373c
         memberof_free_scope(&(config->entryScopeExcludeSubtrees), &config->entryExcludeScopeCount);
96373c
+        if (config->fixup_cache) {
96373c
+            fixup_hashtable_empty(config, "memberof_free_config empty fixup_entry_hastable");
96373c
+            PL_HashTableDestroy(config->fixup_cache);
96373c
+        }
96373c
+        if (config->ancestors_cache) {
96373c
+            ancestor_hashtable_empty(config, "memberof_free_config empty group_ancestors_hashtable");
96373c
+            PL_HashTableDestroy(config->ancestors_cache);
96373c
+        }
96373c
     }
96373c
 }
96373c
 
96373c
@@ -982,3 +999,130 @@ bail:
96373c
 
96373c
     return ret;
96373c
 }
96373c
+
96373c
+
96373c
+static PRIntn memberof_hash_compare_keys(const void *v1, const void *v2)
96373c
+{
96373c
+    PRIntn rc;
96373c
+    if (0 == strcasecmp((const char *) v1, (const char *) v2)) {
96373c
+        rc = 1;
96373c
+    } else {
96373c
+        rc = 0;
96373c
+    }
96373c
+    return rc;
96373c
+}
96373c
+
96373c
+static PRIntn memberof_hash_compare_values(const void *v1, const void *v2)
96373c
+{
96373c
+    PRIntn rc;
96373c
+    if ((char *) v1 == (char *) v2) {
96373c
+        rc = 1;
96373c
+    } else {
96373c
+        rc = 0;
96373c
+    }
96373c
+    return rc;
96373c
+}
96373c
+
96373c
+/*
96373c
+ *  Hashing function using Bernstein's method
96373c
+ */
96373c
+static PLHashNumber memberof_hash_fn(const void *key)
96373c
+{
96373c
+    PLHashNumber hash = 5381;
96373c
+    unsigned char *x = (unsigned char *)key;
96373c
+    int c;
96373c
+
96373c
+    while ((c = *x++)){
96373c
+        hash = ((hash << 5) + hash) ^ c;
96373c
+    }
96373c
+    return hash;
96373c
+}
96373c
+
96373c
+/* allocates the plugin hashtable
96373c
+ * This hash table is used by operation and is protected from
96373c
+ * concurrent operations with the memberof_lock (if not usetxn, memberof_lock
96373c
+ * is not implemented and the hash table will be not used.
96373c
+ *
96373c
+ * The hash table contains all the DN of the entries for which the memberof
96373c
+ * attribute has been computed/updated during the current operation
96373c
+ *
96373c
+ * hash table should be empty at the beginning and end of the plugin callback
96373c
+ */
96373c
+PLHashTable *hashtable_new(int usetxn)
96373c
+{
96373c
+    if (!usetxn) {
96373c
+        return NULL;
96373c
+    }
96373c
+
96373c
+    return PL_NewHashTable(MEMBEROF_HASHTABLE_SIZE,
96373c
+        memberof_hash_fn,
96373c
+        memberof_hash_compare_keys,
96373c
+        memberof_hash_compare_values, NULL, NULL);
96373c
+}
96373c
+
96373c
+/* this function called for each hash node during hash destruction */
96373c
+static PRIntn fixup_hashtable_remove(PLHashEntry *he, PRIntn index __attribute__((unused)), void *arg __attribute__((unused)))
96373c
+{
96373c
+	char *dn_copy;
96373c
+
96373c
+	if (he == NULL) {
96373c
+		return HT_ENUMERATE_NEXT;
96373c
+	}
96373c
+	dn_copy = (char*) he->value;
96373c
+	slapi_ch_free_string(&dn_copy);
96373c
+
96373c
+	return HT_ENUMERATE_REMOVE;
96373c
+}
96373c
+
96373c
+static void fixup_hashtable_empty(MemberOfConfig *config, char *msg)
96373c
+{
96373c
+    if (config->fixup_cache) {
96373c
+        PL_HashTableEnumerateEntries(config->fixup_cache, fixup_hashtable_remove, msg);
96373c
+    }
96373c
+}
96373c
+
96373c
+
96373c
+/* allocates the plugin hashtable
96373c
+ * This hash table is used by operation and is protected from
96373c
+ * concurrent operations with the memberof_lock (if not usetxn, memberof_lock
96373c
+ * is not implemented and the hash table will be not used.
96373c
+ *
96373c
+ * The hash table contains all the DN of the entries for which the memberof
96373c
+ * attribute has been computed/updated during the current operation
96373c
+ *
96373c
+ * hash table should be empty at the beginning and end of the plugin callback
96373c
+ */
96373c
+
96373c
+void ancestor_hashtable_entry_free(memberof_cached_value *entry)
96373c
+{
96373c
+    int i;
96373c
+
96373c
+    for (i = 0; entry[i].valid; i++) {
96373c
+        slapi_ch_free((void **) &entry[i].group_dn_val);
96373c
+        slapi_ch_free((void **) &entry[i].group_ndn_val);
96373c
+    }
96373c
+    /* Here we are at the ending element containing the key */
96373c
+    slapi_ch_free((void**) &entry[i].key);
96373c
+}
96373c
+
96373c
+/* this function called for each hash node during hash destruction */
96373c
+static PRIntn ancestor_hashtable_remove(PLHashEntry *he, PRIntn index __attribute__((unused)), void *arg __attribute__((unused)))
96373c
+{
96373c
+    memberof_cached_value *group_ancestor_array;
96373c
+
96373c
+    if (he == NULL) {
96373c
+        return HT_ENUMERATE_NEXT;
96373c
+    }
96373c
+    group_ancestor_array = (memberof_cached_value *) he->value;
96373c
+    ancestor_hashtable_entry_free(group_ancestor_array);
96373c
+    slapi_ch_free((void **)&group_ancestor_array);
96373c
+
96373c
+    return HT_ENUMERATE_REMOVE;
96373c
+}
96373c
+
96373c
+static void ancestor_hashtable_empty(MemberOfConfig *config, char *msg)
96373c
+{
96373c
+    if (config->ancestors_cache) {
96373c
+        PL_HashTableEnumerateEntries(config->ancestors_cache, ancestor_hashtable_remove, msg);
96373c
+    }
96373c
+}
96373c
-- 
96373c
2.13.6
96373c