Blame SOURCES/0010-Ticket-50260-Invalid-cache-flushing-improvements.patch

26521d
From e07605531f978f6767c9b1cc947c0012ff6c83e3 Mon Sep 17 00:00:00 2001
26521d
From: Mark Reynolds <mreynolds@redhat.com>
26521d
Date: Sun, 17 Mar 2019 13:09:07 -0400
26521d
Subject: [PATCH 3/4] Ticket 50260 - Invalid cache flushing improvements
26521d
26521d
Description:  The original version of the fix only checked if backend
26521d
              transaction "post" operation plugins failed, but it did
26521d
              not check for errors from the backend transaction "pre"
26521d
              operation plugin.  To address this we flush invalid
26521d
              entries whenever any error occurs.
26521d
26521d
              We were also not flushing invalid cache entries when
26521d
              modrdn errors occurred.  Modrdns only make changes to
26521d
              the DN hashtable inside the entry cache, but we were only
26521d
              checking the ID hashtable.  So we also need to check the
26521d
              DN hashtable in the entry cache for invalid entries.
26521d
26521d
https://pagure.io/389-ds-base/issue/50260
26521d
26521d
Reviewed by: firstyear & tbordaz(Thanks!!)
26521d
26521d
(cherry picked from commit 33fbced25277b88695bfba7262e606380e9d891f)
26521d
---
26521d
 dirsrvtests/tests/suites/betxns/betxn_test.py | 23 ++++++----
26521d
 ldap/servers/slapd/back-ldbm/cache.c          | 42 ++++++++++++++++++-
26521d
 ldap/servers/slapd/back-ldbm/ldbm_add.c       |  9 ++--
26521d
 ldap/servers/slapd/back-ldbm/ldbm_delete.c    | 11 ++---
26521d
 ldap/servers/slapd/back-ldbm/ldbm_modify.c    | 10 ++---
26521d
 ldap/servers/slapd/back-ldbm/ldbm_modrdn.c    | 10 ++---
26521d
 6 files changed, 74 insertions(+), 31 deletions(-)
26521d
26521d
diff --git a/dirsrvtests/tests/suites/betxns/betxn_test.py b/dirsrvtests/tests/suites/betxns/betxn_test.py
26521d
index f03fb93cc..84f5e2087 100644
26521d
--- a/dirsrvtests/tests/suites/betxns/betxn_test.py
26521d
+++ b/dirsrvtests/tests/suites/betxns/betxn_test.py
26521d
@@ -86,9 +86,7 @@ def test_betxt_7bit(topology_st, dynamic_plugins):
26521d
         log.fatal('Error while searching for test entry: ' + e.message['desc'])
26521d
         assert False
26521d
 
26521d
-    #
26521d
     # Cleanup - remove the user
26521d
-    #
26521d
     try:
26521d
         topology_st.standalone.delete_s(USER_DN)
26521d
     except ldap.LDAPError as e:
26521d
@@ -241,14 +239,15 @@ def test_betxn_memberof(topology_st, dynamic_plugins):
26521d
     except ldap.LDAPError as e:
26521d
         log.info('test_betxn_memberof: Group2 was correctly rejected (mod add): error ' + e.message['desc'])
26521d
 
26521d
-    #
26521d
+    # verify entry cache reflects the current/correct state of group1
26521d
+    assert not group1.is_member(group2.dn)
26521d
+
26521d
     # Done
26521d
-    #
26521d
     log.info('test_betxn_memberof: PASSED')
26521d
 
26521d
 
26521d
 def test_betxn_modrdn_memberof_cache_corruption(topology_st):
26521d
-    """Test modrdn operations and memberOf
26521d
+    """Test modrdn operations and memberOf be txn post op failures
26521d
 
26521d
     :id: 70d0b96e-b693-4bf7-bbf5-102a66ac5994
26521d
 
26521d
@@ -297,9 +296,7 @@ def test_betxn_modrdn_memberof_cache_corruption(topology_st):
26521d
     with pytest.raises(ldap.OBJECT_CLASS_VIOLATION):
26521d
         group.rename('cn=group_to_people', newsuperior=peoplebase)
26521d
 
26521d
-    #
26521d
     # Done
26521d
-    #
26521d
     log.info('test_betxn_modrdn_memberof: PASSED')
26521d
 
26521d
 
26521d
@@ -374,15 +371,23 @@ def test_ri_and_mep_cache_corruption(topology_st):
26521d
         log.fatal("MEP group was not created for the user")
26521d
         assert False
26521d
 
26521d
+    # Test MEP be txn pre op failure does not corrupt entry cache
26521d
+    # Should get the same exception for both rename attempts
26521d
+    with pytest.raises(ldap.UNWILLING_TO_PERFORM):
26521d
+        mep_group.rename("cn=modrdn group")
26521d
+
26521d
+    with pytest.raises(ldap.UNWILLING_TO_PERFORM):
26521d
+        mep_group.rename("cn=modrdn group")
26521d
+
26521d
     # Mess with MEP so it fails
26521d
     mep_plugin.disable()
26521d
     mep_group.delete()
26521d
     mep_plugin.enable()
26521d
 
26521d
-    # Add another group for verify entry cache is not corrupted
26521d
+    # Add another group to verify entry cache is not corrupted
26521d
     test_group = groups.create(properties={'cn': 'test_group'})
26521d
 
26521d
-    # Delete user, should fail, and user should still be a member
26521d
+    # Delete user, should fail in MEP be txn post op, and user should still be a member
26521d
     with pytest.raises(ldap.NO_SUCH_OBJECT):
26521d
         user.delete()
26521d
 
26521d
diff --git a/ldap/servers/slapd/back-ldbm/cache.c b/ldap/servers/slapd/back-ldbm/cache.c
26521d
index 458d7912f..02453abac 100644
26521d
--- a/ldap/servers/slapd/back-ldbm/cache.c
26521d
+++ b/ldap/servers/slapd/back-ldbm/cache.c
26521d
@@ -517,7 +517,8 @@ flush_remove_entry(struct timespec *entry_time, struct timespec *start_time)
26521d
 /*
26521d
  * Flush all the cache entries that were added after the "start time"
26521d
  * This is called when a backend transaction plugin fails, and we need
26521d
- * to remove all the possible invalid entries in the cache.
26521d
+ * to remove all the possible invalid entries in the cache.  We need
26521d
+ * to check both the ID and DN hashtables when checking the entry cache.
26521d
  *
26521d
  * If the ref count is 0, we can straight up remove it from the cache, but
26521d
  * if the ref count is greater than 1, then the entry is currently in use.
26521d
@@ -528,8 +529,8 @@ flush_remove_entry(struct timespec *entry_time, struct timespec *start_time)
26521d
 static void
26521d
 flush_hash(struct cache *cache, struct timespec *start_time, int32_t type)
26521d
 {
26521d
+    Hashtable *ht = cache->c_idtable; /* start with the ID table as it's in both ENTRY and DN caches */
26521d
     void *e, *laste = NULL;
26521d
-    Hashtable *ht = cache->c_idtable;
26521d
 
26521d
     cache_lock(cache);
26521d
 
26521d
@@ -570,6 +571,43 @@ flush_hash(struct cache *cache, struct timespec *start_time, int32_t type)
26521d
         }
26521d
     }
26521d
 
26521d
+    if (type == ENTRY_CACHE) {
26521d
+        /* Also check the DN hashtable */
26521d
+        ht = cache->c_dntable;
26521d
+
26521d
+        for (size_t i = 0; i < ht->size; i++) {
26521d
+            e = ht->slot[i];
26521d
+            while (e) {
26521d
+                struct backcommon *entry = (struct backcommon *)e;
26521d
+                uint64_t remove_it = 0;
26521d
+                if (flush_remove_entry(&entry->ep_create_time, start_time)) {
26521d
+                    /* Mark the entry to be removed */
26521d
+                    slapi_log_err(SLAPI_LOG_CACHE, "flush_hash", "[ENTRY CACHE] Removing entry id (%d)\n",
26521d
+                            entry->ep_id);
26521d
+                    remove_it = 1;
26521d
+                }
26521d
+                laste = e;
26521d
+                e = HASH_NEXT(ht, e);
26521d
+
26521d
+                if (remove_it) {
26521d
+                    /* since we have the cache lock we know we can trust refcnt */
26521d
+                    entry->ep_state |= ENTRY_STATE_INVALID;
26521d
+                    if (entry->ep_refcnt == 0) {
26521d
+                        entry->ep_refcnt++;
26521d
+                        lru_delete(cache, laste);
26521d
+                        entrycache_remove_int(cache, laste);
26521d
+                        entrycache_return(cache, (struct backentry **)&laste);
26521d
+                    } else {
26521d
+                        /* Entry flagged for removal */
26521d
+                        slapi_log_err(SLAPI_LOG_CACHE, "flush_hash",
26521d
+                                "[ENTRY CACHE] Flagging entry to be removed later: id (%d) refcnt: %d\n",
26521d
+                                entry->ep_id, entry->ep_refcnt);
26521d
+                    }
26521d
+                }
26521d
+            }
26521d
+        }
26521d
+    }
26521d
+
26521d
     cache_unlock(cache);
26521d
 }
26521d
 
26521d
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_add.c b/ldap/servers/slapd/back-ldbm/ldbm_add.c
26521d
index aa5b59aea..264f0ceea 100644
26521d
--- a/ldap/servers/slapd/back-ldbm/ldbm_add.c
26521d
+++ b/ldap/servers/slapd/back-ldbm/ldbm_add.c
26521d
@@ -1221,11 +1221,6 @@ ldbm_back_add(Slapi_PBlock *pb)
26521d
             slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval);
26521d
         }
26521d
         slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
26521d
-
26521d
-        /* Revert the caches if this is the parent operation */
26521d
-        if (parent_op) {
26521d
-            revert_cache(inst, &parent_time);
26521d
-        }
26521d
         goto error_return;
26521d
     }
26521d
 
26521d
@@ -1253,6 +1248,10 @@ ldbm_back_add(Slapi_PBlock *pb)
26521d
     goto common_return;
26521d
 
26521d
 error_return:
26521d
+    /* Revert the caches if this is the parent operation */
26521d
+    if (parent_op) {
26521d
+        revert_cache(inst, &parent_time);
26521d
+    }
26521d
     if (addingentry_id_assigned) {
26521d
         next_id_return(be, addingentry->ep_id);
26521d
     }
26521d
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_delete.c b/ldap/servers/slapd/back-ldbm/ldbm_delete.c
26521d
index 3f687eb91..1ad846447 100644
26521d
--- a/ldap/servers/slapd/back-ldbm/ldbm_delete.c
26521d
+++ b/ldap/servers/slapd/back-ldbm/ldbm_delete.c
26521d
@@ -1279,11 +1279,6 @@ replace_entry:
26521d
             slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, &retval);
26521d
         }
26521d
         slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
26521d
-
26521d
-        /* Revert the caches if this is the parent operation */
26521d
-        if (parent_op) {
26521d
-            revert_cache(inst, &parent_time);
26521d
-        }
26521d
         goto error_return;
26521d
     }
26521d
     if (parent_found) {
26521d
@@ -1370,6 +1365,11 @@ commit_return:
26521d
     goto common_return;
26521d
 
26521d
 error_return:
26521d
+    /* Revert the caches if this is the parent operation */
26521d
+    if (parent_op) {
26521d
+        revert_cache(inst, &parent_time);
26521d
+    }
26521d
+
26521d
     if (tombstone) {
26521d
         if (cache_is_in_cache(&inst->inst_cache, tombstone)) {
26521d
             tomb_ep_id = tombstone->ep_id; /* Otherwise, tombstone might have been freed. */
26521d
@@ -1388,6 +1388,7 @@ error_return:
26521d
         CACHE_RETURN(&inst->inst_cache, &tombstone);
26521d
         tombstone = NULL;
26521d
     }
26521d
+
26521d
     if (retval == DB_RUNRECOVERY) {
26521d
         dblayer_remember_disk_filled(li);
26521d
         ldbm_nasty("ldbm_back_delete", "Delete", 79, retval);
26521d
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_modify.c b/ldap/servers/slapd/back-ldbm/ldbm_modify.c
26521d
index b90b3e0f0..b0c477e3f 100644
26521d
--- a/ldap/servers/slapd/back-ldbm/ldbm_modify.c
26521d
+++ b/ldap/servers/slapd/back-ldbm/ldbm_modify.c
26521d
@@ -873,11 +873,6 @@ ldbm_back_modify(Slapi_PBlock *pb)
26521d
             slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval);
26521d
         }
26521d
         slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
26521d
-
26521d
-        /* Revert the caches if this is the parent operation */
26521d
-        if (parent_op) {
26521d
-            revert_cache(inst, &parent_time);
26521d
-        }
26521d
         goto error_return;
26521d
     }
26521d
     retval = plugin_call_mmr_plugin_postop(pb, NULL,SLAPI_PLUGIN_BE_TXN_POST_MODIFY_FN);
26521d
@@ -901,6 +896,11 @@ ldbm_back_modify(Slapi_PBlock *pb)
26521d
     goto common_return;
26521d
 
26521d
 error_return:
26521d
+    /* Revert the caches if this is the parent operation */
26521d
+    if (parent_op) {
26521d
+        revert_cache(inst, &parent_time);
26521d
+    }
26521d
+
26521d
     if (postentry != NULL) {
26521d
         slapi_entry_free(postentry);
26521d
         postentry = NULL;
26521d
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c
26521d
index 73e50ebcc..65610d613 100644
26521d
--- a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c
26521d
+++ b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c
26521d
@@ -1217,11 +1217,6 @@ ldbm_back_modrdn(Slapi_PBlock *pb)
26521d
             slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval);
26521d
         }
26521d
         slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
26521d
-
26521d
-        /* Revert the caches if this is the parent operation */
26521d
-        if (parent_op) {
26521d
-            revert_cache(inst, &parent_time);
26521d
-        }
26521d
         goto error_return;
26521d
     }
26521d
 	retval = plugin_call_mmr_plugin_postop(pb, NULL,SLAPI_PLUGIN_BE_TXN_POST_MODRDN_FN);
26521d
@@ -1290,6 +1285,11 @@ ldbm_back_modrdn(Slapi_PBlock *pb)
26521d
     goto common_return;
26521d
 
26521d
 error_return:
26521d
+    /* Revert the caches if this is the parent operation */
26521d
+    if (parent_op) {
26521d
+        revert_cache(inst, &parent_time);
26521d
+    }
26521d
+
26521d
     /* result already sent above - just free stuff */
26521d
     if (postentry) {
26521d
         slapi_entry_free(postentry);
26521d
-- 
26521d
2.17.2
26521d