[389-commits] [389-ds-base] branch 389-ds-base-1.4.0 updated: Ticket 50542 - Entry cache contention during base search

This is an automated email from the git hooks/post-receive script. tbordaz pushed a commit to branch 389-ds-base-1.4.0 in repository 389-ds-base. The following commit(s) were added to refs/heads/389-ds-base-1.4.0 by this push: new 4f98f3b Ticket 50542 - Entry cache contention during base search 4f98f3b is described below commit 4f98f3ba595534e25808397e6452d5aa69bdf4ea Author: Thierry Bordaz <tbordaz(a)redhat.com&gt; AuthorDate: Thu Aug 8 12:05:00 2019 +0200 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 7f3600e..7e1c52d 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_search.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_search.c @@ -564,6 +564,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 @@ -820,7 +827,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 @@ -1358,6 +1364,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) { @@ -1460,7 +1487,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; } @@ -1572,7 +1599,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", @@ -1692,7 +1725,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); @@ -1730,12 +1763,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 4a05e0a..8186fd3 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 dd69173..5c5acb3 100644 --- a/ldap/servers/slapd/opshared.c +++ b/ldap/servers/slapd/opshared.c @@ -192,6 +192,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. @@ -251,6 +273,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); @@ -275,7 +298,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); @@ -822,6 +845,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; } @@ -829,6 +853,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 */ @@ -854,6 +879,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 { @@ -866,6 +892,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); @@ -881,6 +908,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); @@ -894,10 +922,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 932828d..4bf5fbf 100644 --- a/ldap/servers/slapd/proto-slap.h +++ b/ldap/servers/slapd/proto-slap.h @@ -883,6 +883,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 2d2de11..2ae395c 100644 --- a/ldap/servers/slapd/slap.h +++ b/ldap/servers/slapd/slap.h @@ -1553,6 +1553,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; -- To stop receiving notification emails like this one, please contact the administrator of this repository.