From 03453b7cb4d03691d47ccf5d82d92fe0572ec244 Mon Sep 17 00:00:00 2001

From: Thierry Bordaz <tbordaz@redhat.com>

Date: Thu, 8 Aug 2019 12:05:00 +0200

Subject: [PATCH] Ticket 50542 - Entry cache contention during base search

Bug Description:

	During a base search the entry cache lock is acquired to retrieve the target entry.

	Later when the candidate list is built, the entry cache lock is also acquired

	to retrieve the candidate that is actually the target entry itself

	So for a base search the entry cache lock is accessed 4 times (2 acquires + 2 releases)

	It is very easy to create a huge contention (e.g. dereferencing large group) increasing

	etime

Fix Description:

	The idea is to acquire the entry, from the entry cache (with refcnt++) when searching the base

	search. Then instead of returning the entry (refcnt--) the entry is kept in the operation until

	the operation completes. If later we need the entry (to send it back to the client), the entry is

	picked up from the operation not from the entry cache lookup

https://pagure.io/389-ds-base/issue/50542

Reviewed by: Ludwig Krispenz, William Brown

Platforms tested: F29

Flag Day: no

Doc impact: no

---

 ldap/servers/slapd/back-ldbm/ldbm_search.c | 45 +++++++++++++++++++---

 ldap/servers/slapd/operation.c             | 32 +++++++++++++++

 ldap/servers/slapd/opshared.c              | 36 ++++++++++++++++-

 ldap/servers/slapd/proto-slap.h            |  4 ++

 ldap/servers/slapd/slap.h                  |  9 +++++

 5 files changed, 118 insertions(+), 8 deletions(-)

diff --git a/ldap/servers/slapd/back-ldbm/ldbm_search.c b/ldap/servers/slapd/back-ldbm/ldbm_search.c

index 8f3111813..c8f5719e1 100644

--- a/ldap/servers/slapd/back-ldbm/ldbm_search.c

+++ b/ldap/servers/slapd/back-ldbm/ldbm_search.c

@@ -551,6 +551,13 @@ ldbm_back_search(Slapi_PBlock *pb)

                                             LDBM_SRCH_DEFAULT_RESULT, NULL, 1, &vlv_request_control, NULL, candidates);

         }

     }

+    /* We have the base search entry and a callback to "cache_return" it.

+     * Keep it into the operation to avoid additional cache fetch/return

+     */

+    if (e && be->be_entry_release) {

+        operation_set_target_entry(operation, (void *) e);

+        operation_set_target_entry_id(operation, e->ep_id);

+    }

     /*

      * If this is a persistent search then the client is only

@@ -807,7 +814,6 @@ ldbm_back_search(Slapi_PBlock *pb)

         }

     }

-    CACHE_RETURN(&inst->inst_cache, &e);

     /*

      * if the candidate list is an allids list, arrange for access log

@@ -1345,6 +1351,27 @@ ldbm_back_next_search_entry(Slapi_PBlock *pb)

     return ldbm_back_next_search_entry_ext(pb, 0);

 }

+/* The reference on the target_entry (base search) is stored in the operation

+ * This is to prevent additional cache find/return that require cache lock.

+ *

+ * The target entry is acquired during be->be_search (when building the candidate list).

+ * and is returned once the operation completes (or fail).

+ *

+ * The others entries sent back to the client have been acquired/returned during send_results_ext.

+ * If the target entry is sent back to the client it is not returned (refcnt--) during the send_results_ext.

+ *

+ * This function returns(refcnt-- in the entry cache) the entry unless it is

+ * the target_entry (base search). target_entry will be return once the operation

+ * completes

+ */

+static void

+non_target_cache_return(Slapi_Operation *op, struct cache *cache, struct backentry **e)

+{

+    if (e && (*e != operation_get_target_entry(op))) {

+        CACHE_RETURN(cache, e);

+    }

+}

+

 int

 ldbm_back_next_search_entry_ext(Slapi_PBlock *pb, int use_extension)

 {

@@ -1447,7 +1474,7 @@ ldbm_back_next_search_entry_ext(Slapi_PBlock *pb, int use_extension)

     /* If we are using the extension, the front end will tell

      * us when to do this so we don't do it now */

     if (sr->sr_entry && !use_extension) {

-        CACHE_RETURN(&inst->inst_cache, &(sr->sr_entry));

+        non_target_cache_return(op, &inst->inst_cache, &(sr->sr_entry));

         sr->sr_entry = NULL;

     }

@@ -1559,7 +1586,13 @@ ldbm_back_next_search_entry_ext(Slapi_PBlock *pb, int use_extension)

         }

         /* get the entry */

-        e = id2entry(be, id, &txn, &err;;

+        e = operation_get_target_entry(op);

+        if ((e == NULL) || (id != operation_get_target_entry_id(op))) {

+            /* if the entry is not the target_entry (base search)

+             * we need to fetch it from the entry cache (it was not

+             * referenced in the operation) */

+            e = id2entry(be, id, &txn, &err;;

+        }

         if (e == NULL) {

             if (err != 0 && err != DB_NOTFOUND) {

                 slapi_log_err(SLAPI_LOG_ERR, "ldbm_back_next_search_entry_ext", "next_search_entry db err %d\n",

@@ -1679,7 +1712,7 @@ ldbm_back_next_search_entry_ext(Slapi_PBlock *pb, int use_extension)

                     /* check size limit */

                     if (slimit >= 0) {

                         if (--slimit < 0) {

-                            CACHE_RETURN(&inst->inst_cache, &e);

+                            non_target_cache_return(op, &inst->inst_cache, &e);

                             slapi_pblock_set(pb, SLAPI_SEARCH_RESULT_SET_SIZE_ESTIMATE, &estimate);

                             delete_search_result_set(pb, &sr);

                             slapi_send_ldap_result(pb, LDAP_SIZELIMIT_EXCEEDED, NULL, NULL, nentries, urls);

@@ -1717,12 +1750,12 @@ ldbm_back_next_search_entry_ext(Slapi_PBlock *pb, int use_extension)

                     rc = 0;

                     goto bail;

                 } else {

-                    CACHE_RETURN(&inst->inst_cache, &(sr->sr_entry));

+                    non_target_cache_return(op, &inst->inst_cache, &(sr->sr_entry));

                     sr->sr_entry = NULL;

                 }

             } else {

                 /* Failed the filter test, and this isn't a VLV Search */

-                CACHE_RETURN(&inst->inst_cache, &(sr->sr_entry));

+                non_target_cache_return(op, &inst->inst_cache, &(sr->sr_entry));

                 sr->sr_entry = NULL;

                 if (LDAP_UNWILLING_TO_PERFORM == filter_test) {

                     /* Need to catch this error to detect the vattr loop */

diff --git a/ldap/servers/slapd/operation.c b/ldap/servers/slapd/operation.c

index 4a05e0a49..8186fd33b 100644

--- a/ldap/servers/slapd/operation.c

+++ b/ldap/servers/slapd/operation.c

@@ -354,6 +354,38 @@ operation_is_flag_set(Slapi_Operation *op, int flag)

     return op->o_flags & flag;

 }

+void *

+operation_get_target_entry(Slapi_Operation *op)

+{

+    PR_ASSERT(op);

+

+    return op->o_target_entry;

+}

+

+void

+operation_set_target_entry(Slapi_Operation *op, void *target_entry)

+{

+    PR_ASSERT(op);

+

+    op->o_target_entry = target_entry;

+}

+

+u_int32_t

+operation_get_target_entry_id(Slapi_Operation *op)

+{

+    PR_ASSERT(op);

+

+    return op->o_target_entry_id;

+}

+

+void

+operation_set_target_entry_id(Slapi_Operation *op, u_int32_t target_entry_id)

+{

+    PR_ASSERT(op);

+

+    op->o_target_entry_id = target_entry_id;

+}

+

 Slapi_DN *

 operation_get_target_spec(Slapi_Operation *op)

 {

diff --git a/ldap/servers/slapd/opshared.c b/ldap/servers/slapd/opshared.c

index cf6cdff01..b9fc83516 100644

--- a/ldap/servers/slapd/opshared.c

+++ b/ldap/servers/slapd/opshared.c

@@ -193,6 +193,28 @@ slapi_attr_is_last_mod(char *attr)

     return 0;

 }

+/* The reference on the target_entry (base search) is stored in the operation

+ * This is to prevent additional cache find/return that require cache lock.

+ *

+ * The target entry is acquired during be->be_search (when building the candidate list).

+ * and is returned once the operation completes (or fail).

+ *

+ * The others entries sent back to the client have been acquired/returned during send_results_ext.

+ * If the target entry is sent back to the client it is not returned (refcnt--) during the send_results_ext.

+ *

+ * This function only returns (refcnt-- in the entry cache) the target_entry (base search).

+ * It is called at the operation level (op_shared_search)

+ *

+ */

+static void

+cache_return_target_entry(Slapi_PBlock *pb, Slapi_Backend *be, Slapi_Operation *operation)

+{

+    if (operation_get_target_entry(operation) && be->be_entry_release) {

+        (*be->be_entry_release)(pb, operation_get_target_entry(operation));

+        operation_set_target_entry(operation, NULL);

+        operation_set_target_entry_id(operation, 0);

+    }

+}

 /*

  * Returns: 0    - if the operation is successful

  *        < 0    - if operation fails.

@@ -252,6 +274,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)

     /* get search parameters */

     slapi_pblock_get(pb, SLAPI_ORIGINAL_TARGET_DN, &base);

     slapi_pblock_get(pb, SLAPI_SEARCH_TARGET_SDN, &sdn;;

+    slapi_pblock_get(pb, SLAPI_OPERATION, &operation);

     if (NULL == sdn) {

         sdn = slapi_sdn_new_dn_byval(base);

@@ -276,7 +299,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)

     slapi_pblock_get(pb, SLAPI_SEARCH_SCOPE, &scope);

     slapi_pblock_get(pb, SLAPI_SEARCH_STRFILTER, &fstr);

     slapi_pblock_get(pb, SLAPI_SEARCH_ATTRS, &attrs);

-    slapi_pblock_get(pb, SLAPI_OPERATION, &operation);

+

     if (operation == NULL) {

         op_shared_log_error_access(pb, "SRCH", base, "NULL operation");

         send_ldap_result(pb, LDAP_OPERATIONS_ERROR, NULL, "NULL operation", 0, NULL);

@@ -808,6 +831,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)

              * the error has already been sent

              * stop the search here

              */

+                    cache_return_target_entry(pb, be, operation);

                     goto free_and_return;

                 }

@@ -815,6 +839,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)

             case SLAPI_FAIL_DISKFULL:

                 operation_out_of_disk_space();

+                cache_return_target_entry(pb, be, operation);

                 goto free_and_return;

             case 0: /* search was successful and we need to send the result */

@@ -840,6 +865,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)

                             /* no more entries && no more backends */

                             curr_search_count = -1;

                         } else if (rc < 0) {

+                            cache_return_target_entry(pb, be, operation);

                             goto free_and_return;

                         }

                     } else {

@@ -852,6 +878,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)

                             (pagedresults_set_search_result_set_size_estimate(pb_conn, operation, estimate, pr_idx) < 0) ||

                             (pagedresults_set_with_sort(pb_conn, operation, with_sort, pr_idx) < 0)) {

                             pagedresults_unlock(pb_conn, pr_idx);

+                            cache_return_target_entry(pb, be, operation);

                             goto free_and_return;

                         }

                         pagedresults_unlock(pb_conn, pr_idx);

@@ -867,6 +894,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)

                         pagedresults_set_response_control(pb, 0, estimate, -1, pr_idx);

                         send_ldap_result(pb, 0, NULL, "Simple Paged Results Search abandoned", 0, NULL);

                         rc = LDAP_SUCCESS;

+                        cache_return_target_entry(pb, be, operation);

                         goto free_and_return;

                     }

                     pagedresults_set_response_control(pb, 0, estimate, curr_search_count, pr_idx);

@@ -880,10 +908,14 @@ op_shared_search(Slapi_PBlock *pb, int send_result)

          * LDAP error should already have been sent to the client

          * stop the search, free and return

          */

-                if (rc != 0)

+                if (rc != 0) {

+                    cache_return_target_entry(pb, be, operation);

                     goto free_and_return;

+                }

                 break;

             }

+            /* cache return the target_entry */

+            cache_return_target_entry(pb, be, operation);

         }

         nentries += pnentries;

diff --git a/ldap/servers/slapd/proto-slap.h b/ldap/servers/slapd/proto-slap.h

index e37f702ea..d9fb8fd08 100644

--- a/ldap/servers/slapd/proto-slap.h

+++ b/ldap/servers/slapd/proto-slap.h

@@ -873,6 +873,10 @@ void operation_free(Slapi_Operation **op, Connection *conn);

 int slapi_op_abandoned(Slapi_PBlock *pb);

 void operation_out_of_disk_space(void);

 int get_operation_object_type(void);

+void *operation_get_target_entry(Slapi_Operation *op);

+void operation_set_target_entry(Slapi_Operation *op, void *target_void);

+u_int32_t operation_get_target_entry_id(Slapi_Operation *op);

+void operation_set_target_entry_id(Slapi_Operation *op, u_int32_t target_entry_id);

 Slapi_DN *operation_get_target_spec(Slapi_Operation *op);

 void operation_set_target_spec(Slapi_Operation *op, const Slapi_DN *target_spec);

 void operation_set_target_spec_str(Slapi_Operation *op, const char *target_spec);

diff --git a/ldap/servers/slapd/slap.h b/ldap/servers/slapd/slap.h

index bce720974..a8908d94c 100644

--- a/ldap/servers/slapd/slap.h

+++ b/ldap/servers/slapd/slap.h

@@ -1545,6 +1545,15 @@ typedef struct op

     unsigned long o_flags;                                     /* flags for this operation      */

     void *o_extension;                                         /* plugins are able to extend the Operation object */

     Slapi_DN *o_target_spec;                                   /* used to decide which plugins should be called for the operation */

+    void *o_target_entry;                                      /* Only used for SEARCH operation

+                                                                * reference of search target entry (base search) in the entry cache

+                                                                * When it is set the refcnt (of the entry in the entry cache) as been increased

+                                                                */

+    u_int32_t o_target_entry_id;                               /* Only used for SEARCH operation

+                                                                * contains the ID of the o_target_entry. In send_result we have ID of the candidates, this

+                                                                * accelerates the tests as we have not to retrieve for each candidate the

+                                                                * ep_id inside the o_target_entry.

+                                                                */

     unsigned long o_abandoned_op;                              /* operation abandoned by this operation - used to decide which plugins to invoke */

     struct slapi_operation_parameters o_params;

     struct slapi_operation_results o_results;

-- 

2.24.1