From: Garming Sam Date: Thu, 7 Mar 2019 03:45:46 +0000 (+1300) Subject: ldb_kv: Avoid memdup of database records in the case of base searches X-Git-Url: http://git.samba.org/samba.git/?a=commitdiff_plain;h=a76d2865372988c29baef42ecc4257e861692e7b;p=mdw%2Fsamba-autobuild%2F.git ldb_kv: Avoid memdup of database records in the case of base searches This makes LDAP bind significantly faster in the case of having many members, due to large size of these records (with tens of thousands of member links). During the nested group calculation, you are only interested in memberOf not the member links. (We add a bit-field to determine whether or not the backend actually supports pointing into database memory. For some reason TDB pointers aren't stable, so for now we set this option just on LMDB backends.) Signed-off-by: Garming Sam Reviewed-by: Andrew Bartlett --- diff --git a/lib/ldb/include/ldb_module.h b/lib/ldb/include/ldb_module.h index 6ba2a49300a..b45142abe5c 100644 --- a/lib/ldb/include/ldb_module.h +++ b/lib/ldb/include/ldb_module.h @@ -560,6 +560,7 @@ int ldb_unpack_data_only_attr_list_flags(struct ldb_context *ldb, #define LDB_UNPACK_DATA_FLAG_NO_DN 0x0002 #define LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC 0x0004 #define LDB_UNPACK_DATA_FLAG_NO_ATTRS 0x0008 +#define LDB_UNPACK_DATA_FLAG_READ_LOCKED 0x0010 /** Forces a specific ldb handle to use the global event context. diff --git a/lib/ldb/ldb_key_value/ldb_kv.h b/lib/ldb/ldb_key_value/ldb_kv.h index 92106caae85..778a8991bb8 100644 --- a/lib/ldb/ldb_key_value/ldb_kv.h +++ b/lib/ldb/ldb_key_value/ldb_kv.h @@ -13,6 +13,8 @@ typedef int (*ldb_kv_traverse_fn)(struct ldb_kv_private *ldb_kv, void *ctx); struct kv_db_ops { + uint32_t options; + int (*store)(struct ldb_kv_private *ldb_kv, struct ldb_val key, struct ldb_val data, @@ -175,6 +177,13 @@ struct ldb_kv_reindex_context { #define LDB_KV_GUID_SIZE 16 #define LDB_KV_GUID_KEY_SIZE (LDB_KV_GUID_SIZE + sizeof(LDB_KV_GUID_KEY_PREFIX) - 1) +/* LDB KV options */ +/* + * This allows pointers to be referenced after the callback to any variant of + * iterate or fetch_and_parse -- as long as an overall read lock is held. + */ +#define LDB_KV_OPTION_STABLE_READ_LOCK 0x00000001 + /* * The following definitions come from lib/ldb/ldb_key_value/ldb_kv_cache.c */ diff --git a/lib/ldb/ldb_key_value/ldb_kv_index.c b/lib/ldb/ldb_key_value/ldb_kv_index.c index 350289a78e3..9f0de7d260e 100644 --- a/lib/ldb/ldb_key_value/ldb_kv_index.c +++ b/lib/ldb/ldb_key_value/ldb_kv_index.c @@ -406,7 +406,13 @@ normal_index: dn, msg, LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC | - LDB_UNPACK_DATA_FLAG_NO_DN); + LDB_UNPACK_DATA_FLAG_NO_DN | + /* + * The entry point ldb_kv_search_indexed is + * only called from the read-locked + * ldb_kv_search. + */ + LDB_UNPACK_DATA_FLAG_READ_LOCKED); if (ret != LDB_SUCCESS) { talloc_free(msg); return ret; @@ -2222,7 +2228,13 @@ static int ldb_kv_index_filter(struct ldb_kv_private *ldb_kv, keys[i], msg, LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC | - LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC); + LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC | + /* + * The entry point ldb_kv_search_indexed is + * only called from the read-locked + * ldb_kv_search. + */ + LDB_UNPACK_DATA_FLAG_READ_LOCKED); if (ret == LDB_ERR_NO_SUCH_OBJECT) { /* * the record has disappeared? yes, this can diff --git a/lib/ldb/ldb_key_value/ldb_kv_search.c b/lib/ldb/ldb_key_value/ldb_kv_search.c index aa086a88d9a..a2946d6506b 100644 --- a/lib/ldb/ldb_key_value/ldb_kv_search.c +++ b/lib/ldb/ldb_key_value/ldb_kv_search.c @@ -190,22 +190,45 @@ static int ldb_kv_parse_data_unpack(struct ldb_val key, struct ldb_context *ldb = ldb_module_get_ctx(ctx->module); struct ldb_val data_parse = data; - if (ctx->unpack_flags & LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC) { - /* - * If we got LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC - * we need at least do a memdup on the whole - * data buffer as that may change later - * and the caller needs a stable result. - */ - data_parse.data = talloc_memdup(ctx->msg, - data.data, - data.length); - if (data_parse.data == NULL) { - ldb_debug(ldb, LDB_DEBUG_ERROR, - "Unable to allocate data(%d) for %*.*s\n", - (int)data.length, - (int)key.length, (int)key.length, key.data); - return LDB_ERR_OPERATIONS_ERROR; + struct ldb_kv_private *ldb_kv = + talloc_get_type(ldb_module_get_private(ctx->module), struct ldb_kv_private); + + if ((ctx->unpack_flags & LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC)) { + if ((ldb_kv->kv_ops->options & LDB_KV_OPTION_STABLE_READ_LOCK) && + (ctx->unpack_flags & LDB_UNPACK_DATA_FLAG_READ_LOCKED) && + !ldb_kv->kv_ops->transaction_active(ldb_kv)) { + /* + * In the case where no transactions are active and + * we're in a read-lock, we can point directly into + * database memory. + * + * The database can't be changed underneath us and we + * will duplicate this data in the call to filter. + * + * This is seen in: + * - ldb_kv_index_filter + * - ldb_kv_search_and_return_base + */ + } else { + /* + * In every other case, if we got + * LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC we need at least + * do a memdup on the whole data buffer as that may + * change later and the caller needs a stable result. + * + * During transactions, pointers could change and in + * TDB, there just aren't the same guarantees. + */ + data_parse.data = talloc_memdup(ctx->msg, + data.data, + data.length); + if (data_parse.data == NULL) { + ldb_debug(ldb, LDB_DEBUG_ERROR, + "Unable to allocate data(%d) for %*.*s\n", + (int)data.length, + (int)key.length, (int)key.length, key.data); + return LDB_ERR_OPERATIONS_ERROR; + } } } @@ -635,7 +658,8 @@ static int ldb_kv_search_and_return_base(struct ldb_kv_private *ldb_kv, ctx->base, msg, LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC | - LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC); + LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC | + LDB_UNPACK_DATA_FLAG_READ_LOCKED); if (ret == LDB_ERR_NO_SUCH_OBJECT) { if (ldb_kv->check_base == false) { diff --git a/lib/ldb/ldb_mdb/ldb_mdb.c b/lib/ldb/ldb_mdb/ldb_mdb.c index 01fa024e711..68ee97acb64 100644 --- a/lib/ldb/ldb_mdb/ldb_mdb.c +++ b/lib/ldb/ldb_mdb/ldb_mdb.c @@ -739,9 +739,9 @@ static size_t lmdb_get_size(struct ldb_kv_private *ldb_kv) return stats.ms_entries; } - - static struct kv_db_ops lmdb_key_value_ops = { + .options = LDB_KV_OPTION_STABLE_READ_LOCK, + .store = lmdb_store, .delete = lmdb_delete, .iterate = lmdb_traverse_fn, diff --git a/lib/ldb/ldb_tdb/ldb_tdb.c b/lib/ldb/ldb_tdb/ldb_tdb.c index 3dcc158729a..ae0001e8084 100644 --- a/lib/ldb/ldb_tdb/ldb_tdb.c +++ b/lib/ldb/ldb_tdb/ldb_tdb.c @@ -434,24 +434,27 @@ static size_t ltdb_get_size(struct ldb_kv_private *ldb_kv) } static const struct kv_db_ops key_value_ops = { - .store = ltdb_store, - .delete = ltdb_delete, - .iterate = ltdb_traverse_fn, - .update_in_iterate = ltdb_update_in_iterate, - .fetch_and_parse = ltdb_parse_record, - .iterate_range = ltdb_iterate_range, - .lock_read = ltdb_lock_read, - .unlock_read = ltdb_unlock_read, - .begin_write = ltdb_transaction_start, - .prepare_write = ltdb_transaction_prepare_commit, - .finish_write = ltdb_transaction_commit, - .abort_write = ltdb_transaction_cancel, - .error = ltdb_error, - .errorstr = ltdb_errorstr, - .name = ltdb_name, - .has_changed = ltdb_changed, - .transaction_active = ltdb_transaction_active, - .get_size = ltdb_get_size, + /* No support for any additional features */ + .options = 0, + + .store = ltdb_store, + .delete = ltdb_delete, + .iterate = ltdb_traverse_fn, + .update_in_iterate = ltdb_update_in_iterate, + .fetch_and_parse = ltdb_parse_record, + .iterate_range = ltdb_iterate_range, + .lock_read = ltdb_lock_read, + .unlock_read = ltdb_unlock_read, + .begin_write = ltdb_transaction_start, + .prepare_write = ltdb_transaction_prepare_commit, + .finish_write = ltdb_transaction_commit, + .abort_write = ltdb_transaction_cancel, + .error = ltdb_error, + .errorstr = ltdb_errorstr, + .name = ltdb_name, + .has_changed = ltdb_changed, + .transaction_active = ltdb_transaction_active, + .get_size = ltdb_get_size, }; /*