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