Blame SOURCES/0016-Ticket-50542-Entry-cache-contention-during-base-sear.patch

e52775
From 03453b7cb4d03691d47ccf5d82d92fe0572ec244 Mon Sep 17 00:00:00 2001
e52775
From: Thierry Bordaz <tbordaz@redhat.com>
e52775
Date: Thu, 8 Aug 2019 12:05:00 +0200
e52775
Subject: [PATCH] Ticket 50542 - Entry cache contention during base search
e52775
e52775
Bug Description:
e52775
	During a base search the entry cache lock is acquired to retrieve the target entry.
e52775
	Later when the candidate list is built, the entry cache lock is also acquired
e52775
	to retrieve the candidate that is actually the target entry itself
e52775
e52775
	So for a base search the entry cache lock is accessed 4 times (2 acquires + 2 releases)
e52775
e52775
	It is very easy to create a huge contention (e.g. dereferencing large group) increasing
e52775
	etime
e52775
e52775
Fix Description:
e52775
	The idea is to acquire the entry, from the entry cache (with refcnt++) when searching the base
e52775
	search. Then instead of returning the entry (refcnt--) the entry is kept in the operation until
e52775
	the operation completes. If later we need the entry (to send it back to the client), the entry is
e52775
	picked up from the operation not from the entry cache lookup
e52775
e52775
https://pagure.io/389-ds-base/issue/50542
e52775
e52775
Reviewed by: Ludwig Krispenz, William Brown
e52775
e52775
Platforms tested: F29
e52775
e52775
Flag Day: no
e52775
e52775
Doc impact: no
e52775
---
e52775
 ldap/servers/slapd/back-ldbm/ldbm_search.c | 45 +++++++++++++++++++---
e52775
 ldap/servers/slapd/operation.c             | 32 +++++++++++++++
e52775
 ldap/servers/slapd/opshared.c              | 36 ++++++++++++++++-
e52775
 ldap/servers/slapd/proto-slap.h            |  4 ++
e52775
 ldap/servers/slapd/slap.h                  |  9 +++++
e52775
 5 files changed, 118 insertions(+), 8 deletions(-)
e52775
e52775
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_search.c b/ldap/servers/slapd/back-ldbm/ldbm_search.c
e52775
index 8f3111813..c8f5719e1 100644
e52775
--- a/ldap/servers/slapd/back-ldbm/ldbm_search.c
e52775
+++ b/ldap/servers/slapd/back-ldbm/ldbm_search.c
e52775
@@ -551,6 +551,13 @@ ldbm_back_search(Slapi_PBlock *pb)
e52775
                                             LDBM_SRCH_DEFAULT_RESULT, NULL, 1, &vlv_request_control, NULL, candidates);
e52775
         }
e52775
     }
e52775
+    /* We have the base search entry and a callback to "cache_return" it.
e52775
+     * Keep it into the operation to avoid additional cache fetch/return
e52775
+     */
e52775
+    if (e && be->be_entry_release) {
e52775
+        operation_set_target_entry(operation, (void *) e);
e52775
+        operation_set_target_entry_id(operation, e->ep_id);
e52775
+    }
e52775
 
e52775
     /*
e52775
      * If this is a persistent search then the client is only
e52775
@@ -807,7 +814,6 @@ ldbm_back_search(Slapi_PBlock *pb)
e52775
         }
e52775
     }
e52775
 
e52775
-    CACHE_RETURN(&inst->inst_cache, &e);
e52775
 
e52775
     /*
e52775
      * if the candidate list is an allids list, arrange for access log
e52775
@@ -1345,6 +1351,27 @@ ldbm_back_next_search_entry(Slapi_PBlock *pb)
e52775
     return ldbm_back_next_search_entry_ext(pb, 0);
e52775
 }
e52775
 
e52775
+/* The reference on the target_entry (base search) is stored in the operation
e52775
+ * This is to prevent additional cache find/return that require cache lock.
e52775
+ *
e52775
+ * The target entry is acquired during be->be_search (when building the candidate list).
e52775
+ * and is returned once the operation completes (or fail).
e52775
+ *
e52775
+ * The others entries sent back to the client have been acquired/returned during send_results_ext.
e52775
+ * If the target entry is sent back to the client it is not returned (refcnt--) during the send_results_ext.
e52775
+ *
e52775
+ * This function returns(refcnt-- in the entry cache) the entry unless it is
e52775
+ * the target_entry (base search). target_entry will be return once the operation
e52775
+ * completes
e52775
+ */
e52775
+static void
e52775
+non_target_cache_return(Slapi_Operation *op, struct cache *cache, struct backentry **e)
e52775
+{
e52775
+    if (e && (*e != operation_get_target_entry(op))) {
e52775
+        CACHE_RETURN(cache, e);
e52775
+    }
e52775
+}
e52775
+
e52775
 int
e52775
 ldbm_back_next_search_entry_ext(Slapi_PBlock *pb, int use_extension)
e52775
 {
e52775
@@ -1447,7 +1474,7 @@ ldbm_back_next_search_entry_ext(Slapi_PBlock *pb, int use_extension)
e52775
     /* If we are using the extension, the front end will tell
e52775
      * us when to do this so we don't do it now */
e52775
     if (sr->sr_entry && !use_extension) {
e52775
-        CACHE_RETURN(&inst->inst_cache, &(sr->sr_entry));
e52775
+        non_target_cache_return(op, &inst->inst_cache, &(sr->sr_entry));
e52775
         sr->sr_entry = NULL;
e52775
     }
e52775
 
e52775
@@ -1559,7 +1586,13 @@ ldbm_back_next_search_entry_ext(Slapi_PBlock *pb, int use_extension)
e52775
         }
e52775
 
e52775
         /* get the entry */
e52775
-        e = id2entry(be, id, &txn, &err);
e52775
+        e = operation_get_target_entry(op);
e52775
+        if ((e == NULL) || (id != operation_get_target_entry_id(op))) {
e52775
+            /* if the entry is not the target_entry (base search)
e52775
+             * we need to fetch it from the entry cache (it was not
e52775
+             * referenced in the operation) */
e52775
+            e = id2entry(be, id, &txn, &err);
e52775
+        }
e52775
         if (e == NULL) {
e52775
             if (err != 0 && err != DB_NOTFOUND) {
e52775
                 slapi_log_err(SLAPI_LOG_ERR, "ldbm_back_next_search_entry_ext", "next_search_entry db err %d\n",
e52775
@@ -1679,7 +1712,7 @@ ldbm_back_next_search_entry_ext(Slapi_PBlock *pb, int use_extension)
e52775
                     /* check size limit */
e52775
                     if (slimit >= 0) {
e52775
                         if (--slimit < 0) {
e52775
-                            CACHE_RETURN(&inst->inst_cache, &e);
e52775
+                            non_target_cache_return(op, &inst->inst_cache, &e);
e52775
                             slapi_pblock_set(pb, SLAPI_SEARCH_RESULT_SET_SIZE_ESTIMATE, &estimate);
e52775
                             delete_search_result_set(pb, &sr);
e52775
                             slapi_send_ldap_result(pb, LDAP_SIZELIMIT_EXCEEDED, NULL, NULL, nentries, urls);
e52775
@@ -1717,12 +1750,12 @@ ldbm_back_next_search_entry_ext(Slapi_PBlock *pb, int use_extension)
e52775
                     rc = 0;
e52775
                     goto bail;
e52775
                 } else {
e52775
-                    CACHE_RETURN(&inst->inst_cache, &(sr->sr_entry));
e52775
+                    non_target_cache_return(op, &inst->inst_cache, &(sr->sr_entry));
e52775
                     sr->sr_entry = NULL;
e52775
                 }
e52775
             } else {
e52775
                 /* Failed the filter test, and this isn't a VLV Search */
e52775
-                CACHE_RETURN(&inst->inst_cache, &(sr->sr_entry));
e52775
+                non_target_cache_return(op, &inst->inst_cache, &(sr->sr_entry));
e52775
                 sr->sr_entry = NULL;
e52775
                 if (LDAP_UNWILLING_TO_PERFORM == filter_test) {
e52775
                     /* Need to catch this error to detect the vattr loop */
e52775
diff --git a/ldap/servers/slapd/operation.c b/ldap/servers/slapd/operation.c
e52775
index 4a05e0a49..8186fd33b 100644
e52775
--- a/ldap/servers/slapd/operation.c
e52775
+++ b/ldap/servers/slapd/operation.c
e52775
@@ -354,6 +354,38 @@ operation_is_flag_set(Slapi_Operation *op, int flag)
e52775
     return op->o_flags & flag;
e52775
 }
e52775
 
e52775
+void *
e52775
+operation_get_target_entry(Slapi_Operation *op)
e52775
+{
e52775
+    PR_ASSERT(op);
e52775
+
e52775
+    return op->o_target_entry;
e52775
+}
e52775
+
e52775
+void
e52775
+operation_set_target_entry(Slapi_Operation *op, void *target_entry)
e52775
+{
e52775
+    PR_ASSERT(op);
e52775
+
e52775
+    op->o_target_entry = target_entry;
e52775
+}
e52775
+
e52775
+u_int32_t
e52775
+operation_get_target_entry_id(Slapi_Operation *op)
e52775
+{
e52775
+    PR_ASSERT(op);
e52775
+
e52775
+    return op->o_target_entry_id;
e52775
+}
e52775
+
e52775
+void
e52775
+operation_set_target_entry_id(Slapi_Operation *op, u_int32_t target_entry_id)
e52775
+{
e52775
+    PR_ASSERT(op);
e52775
+
e52775
+    op->o_target_entry_id = target_entry_id;
e52775
+}
e52775
+
e52775
 Slapi_DN *
e52775
 operation_get_target_spec(Slapi_Operation *op)
e52775
 {
e52775
diff --git a/ldap/servers/slapd/opshared.c b/ldap/servers/slapd/opshared.c
e52775
index cf6cdff01..b9fc83516 100644
e52775
--- a/ldap/servers/slapd/opshared.c
e52775
+++ b/ldap/servers/slapd/opshared.c
e52775
@@ -193,6 +193,28 @@ slapi_attr_is_last_mod(char *attr)
e52775
     return 0;
e52775
 }
e52775
 
e52775
+/* The reference on the target_entry (base search) is stored in the operation
e52775
+ * This is to prevent additional cache find/return that require cache lock.
e52775
+ *
e52775
+ * The target entry is acquired during be->be_search (when building the candidate list).
e52775
+ * and is returned once the operation completes (or fail).
e52775
+ *
e52775
+ * The others entries sent back to the client have been acquired/returned during send_results_ext.
e52775
+ * If the target entry is sent back to the client it is not returned (refcnt--) during the send_results_ext.
e52775
+ *
e52775
+ * This function only returns (refcnt-- in the entry cache) the target_entry (base search).
e52775
+ * It is called at the operation level (op_shared_search)
e52775
+ *
e52775
+ */
e52775
+static void
e52775
+cache_return_target_entry(Slapi_PBlock *pb, Slapi_Backend *be, Slapi_Operation *operation)
e52775
+{
e52775
+    if (operation_get_target_entry(operation) && be->be_entry_release) {
e52775
+        (*be->be_entry_release)(pb, operation_get_target_entry(operation));
e52775
+        operation_set_target_entry(operation, NULL);
e52775
+        operation_set_target_entry_id(operation, 0);
e52775
+    }
e52775
+}
e52775
 /*
e52775
  * Returns: 0    - if the operation is successful
e52775
  *        < 0    - if operation fails.
e52775
@@ -252,6 +274,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)
e52775
     /* get search parameters */
e52775
     slapi_pblock_get(pb, SLAPI_ORIGINAL_TARGET_DN, &base);
e52775
     slapi_pblock_get(pb, SLAPI_SEARCH_TARGET_SDN, &sdn);
e52775
+    slapi_pblock_get(pb, SLAPI_OPERATION, &operation);
e52775
 
e52775
     if (NULL == sdn) {
e52775
         sdn = slapi_sdn_new_dn_byval(base);
e52775
@@ -276,7 +299,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)
e52775
     slapi_pblock_get(pb, SLAPI_SEARCH_SCOPE, &scope);
e52775
     slapi_pblock_get(pb, SLAPI_SEARCH_STRFILTER, &fstr);
e52775
     slapi_pblock_get(pb, SLAPI_SEARCH_ATTRS, &attrs);
e52775
-    slapi_pblock_get(pb, SLAPI_OPERATION, &operation);
e52775
+
e52775
     if (operation == NULL) {
e52775
         op_shared_log_error_access(pb, "SRCH", base, "NULL operation");
e52775
         send_ldap_result(pb, LDAP_OPERATIONS_ERROR, NULL, "NULL operation", 0, NULL);
e52775
@@ -808,6 +831,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)
e52775
              * the error has already been sent
e52775
              * stop the search here
e52775
              */
e52775
+                    cache_return_target_entry(pb, be, operation);
e52775
                     goto free_and_return;
e52775
                 }
e52775
 
e52775
@@ -815,6 +839,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)
e52775
 
e52775
             case SLAPI_FAIL_DISKFULL:
e52775
                 operation_out_of_disk_space();
e52775
+                cache_return_target_entry(pb, be, operation);
e52775
                 goto free_and_return;
e52775
 
e52775
             case 0: /* search was successful and we need to send the result */
e52775
@@ -840,6 +865,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)
e52775
                             /* no more entries && no more backends */
e52775
                             curr_search_count = -1;
e52775
                         } else if (rc < 0) {
e52775
+                            cache_return_target_entry(pb, be, operation);
e52775
                             goto free_and_return;
e52775
                         }
e52775
                     } else {
e52775
@@ -852,6 +878,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)
e52775
                             (pagedresults_set_search_result_set_size_estimate(pb_conn, operation, estimate, pr_idx) < 0) ||
e52775
                             (pagedresults_set_with_sort(pb_conn, operation, with_sort, pr_idx) < 0)) {
e52775
                             pagedresults_unlock(pb_conn, pr_idx);
e52775
+                            cache_return_target_entry(pb, be, operation);
e52775
                             goto free_and_return;
e52775
                         }
e52775
                         pagedresults_unlock(pb_conn, pr_idx);
e52775
@@ -867,6 +894,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)
e52775
                         pagedresults_set_response_control(pb, 0, estimate, -1, pr_idx);
e52775
                         send_ldap_result(pb, 0, NULL, "Simple Paged Results Search abandoned", 0, NULL);
e52775
                         rc = LDAP_SUCCESS;
e52775
+                        cache_return_target_entry(pb, be, operation);
e52775
                         goto free_and_return;
e52775
                     }
e52775
                     pagedresults_set_response_control(pb, 0, estimate, curr_search_count, pr_idx);
e52775
@@ -880,10 +908,14 @@ op_shared_search(Slapi_PBlock *pb, int send_result)
e52775
          * LDAP error should already have been sent to the client
e52775
          * stop the search, free and return
e52775
          */
e52775
-                if (rc != 0)
e52775
+                if (rc != 0) {
e52775
+                    cache_return_target_entry(pb, be, operation);
e52775
                     goto free_and_return;
e52775
+                }
e52775
                 break;
e52775
             }
e52775
+            /* cache return the target_entry */
e52775
+            cache_return_target_entry(pb, be, operation);
e52775
         }
e52775
 
e52775
         nentries += pnentries;
e52775
diff --git a/ldap/servers/slapd/proto-slap.h b/ldap/servers/slapd/proto-slap.h
e52775
index e37f702ea..d9fb8fd08 100644
e52775
--- a/ldap/servers/slapd/proto-slap.h
e52775
+++ b/ldap/servers/slapd/proto-slap.h
e52775
@@ -873,6 +873,10 @@ void operation_free(Slapi_Operation **op, Connection *conn);
e52775
 int slapi_op_abandoned(Slapi_PBlock *pb);
e52775
 void operation_out_of_disk_space(void);
e52775
 int get_operation_object_type(void);
e52775
+void *operation_get_target_entry(Slapi_Operation *op);
e52775
+void operation_set_target_entry(Slapi_Operation *op, void *target_void);
e52775
+u_int32_t operation_get_target_entry_id(Slapi_Operation *op);
e52775
+void operation_set_target_entry_id(Slapi_Operation *op, u_int32_t target_entry_id);
e52775
 Slapi_DN *operation_get_target_spec(Slapi_Operation *op);
e52775
 void operation_set_target_spec(Slapi_Operation *op, const Slapi_DN *target_spec);
e52775
 void operation_set_target_spec_str(Slapi_Operation *op, const char *target_spec);
e52775
diff --git a/ldap/servers/slapd/slap.h b/ldap/servers/slapd/slap.h
e52775
index bce720974..a8908d94c 100644
e52775
--- a/ldap/servers/slapd/slap.h
e52775
+++ b/ldap/servers/slapd/slap.h
e52775
@@ -1545,6 +1545,15 @@ typedef struct op
e52775
     unsigned long o_flags;                                     /* flags for this operation      */
e52775
     void *o_extension;                                         /* plugins are able to extend the Operation object */
e52775
     Slapi_DN *o_target_spec;                                   /* used to decide which plugins should be called for the operation */
e52775
+    void *o_target_entry;                                      /* Only used for SEARCH operation
e52775
+                                                                * reference of search target entry (base search) in the entry cache
e52775
+                                                                * When it is set the refcnt (of the entry in the entry cache) as been increased
e52775
+                                                                */
e52775
+    u_int32_t o_target_entry_id;                               /* Only used for SEARCH operation
e52775
+                                                                * contains the ID of the o_target_entry. In send_result we have ID of the candidates, this
e52775
+                                                                * accelerates the tests as we have not to retrieve for each candidate the
e52775
+                                                                * ep_id inside the o_target_entry.
e52775
+                                                                */
e52775
     unsigned long o_abandoned_op;                              /* operation abandoned by this operation - used to decide which plugins to invoke */
e52775
     struct slapi_operation_parameters o_params;
e52775
     struct slapi_operation_results o_results;
e52775
-- 
e52775
2.24.1
e52775