[SCM] Samba Shared Repository - branch master updated
Tim Beale
timbeale at samba.org
Wed Nov 21 04:32:02 UTC 2018
The branch, master has been updated
via a370f217bb9 replmd: Make replmd_process_linked_attribute() mem dependencies clearer
via 05147d25e7b replmd: Avoid redundant dsdb_get_deleted_objects_dn() checks
via 698cf271f43 replmd: Minimize get_parsed_dns_trusted() calls during replication
via 90f5e49a879 replmd: Pass old_el into replmd_process_linked_attribute()
via 19a36b367f1 replmd: Remove some redundant code
via f53954d0fd4 replmd: Move where we update the usnChanged/whenChanged
via c371fef5863 replmd: Only modify the object if it actually changed
via cb3520fbaf9 replmd: replmd_process_link_attribute() returns type of change made
from ad57cac7db0 source4 samr: Tidy DBG_WARNING calls
https://git.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit a370f217bb94601345ad5700ea546259e1d04bfd
Author: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue Nov 20 10:59:40 2018 +1300
replmd: Make replmd_process_linked_attribute() mem dependencies clearer
This patch should not alter functionality - it is just making memory
assumptions used in replmd_process_linked_attribute() clearer.
When adding/removing msg->elements we have to take care, as this will
invalidate things like the parsed-DN array or old ldb_message_element
pointers. This has always been the case (i.e. f6bc4c08b19f5615a49),
however, now we need to take even more care, as the msg being modified
is re-used and split across 2 different functions.
Add more code comments to highlight this. We can also free
pdn_list/old_el to prevent them being incorrectly used after realloc.
It seems appropriate to also add a sanity-check that the tmp_ctx alloc
succeeds (which all the other memory hangs off).
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
Autobuild-User(master): Tim Beale <timbeale at samba.org>
Autobuild-Date(master): Wed Nov 21 05:31:10 CET 2018 on sn-devel-144
commit 05147d25e7b4a9343378c59927f443b723606960
Author: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon Nov 12 15:49:28 2018 +1300
replmd: Avoid redundant dsdb_get_deleted_objects_dn() checks
Quite a bit of time was spent in dsdb_get_deleted_objects_dn()
processing during either a join (~9%) or a full-sync (~28%).
The problem is we're *always* doing the dsdb_get_deleted_objects_dn()
call for each object, regardless of whether it's actually deleted or
not. i.e. we were doing an expensive query and a lot of the time just
ignoring the query result.
If it's not a deleted object we're dealing with, we can just return
early and skip the unnecessary processing.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
commit 698cf271f439c252c77f67fb88b09c9dcc84139d
Author: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon Nov 12 12:11:38 2018 +1300
replmd: Minimize get_parsed_dns_trusted() calls during replication
When a group has 10,000+ links, get_parsed_dns_trusted() can be costly
(simply the talloc calls alone are expensive). Instead of re-generating
the pdn_list for every single link attribute, we can change to only
re-generate it when we really need to.
When we add a new link, it reallocates old_el->values, and so we need to
recreate the pdn_list because all the memory pointers will have changed.
However, in the other cases, where we're simply updating the existing
link value (or ignoring the update, if it's already applied), we can
continue using the same pdn_list (rather than re-parsing it again).
This would generally only save time with a full-sync - it won't really
help with the join case (because every link processed results in a
realloc).
On a DB with 5000 users, this makes a full-sync about ~13% faster.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
commit 90f5e49a8797f821b07ae761056ec0c5235f8978
Author: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon Nov 12 12:00:47 2018 +1300
replmd: Pass old_el into replmd_process_linked_attribute()
We should only need to lookup the msg attribute once per source object.
The old_el->values may change due to link-processing, but old_el itself
should not.
This is not aimed at improving performance, but we need to change how
old_el is used before we can change pdn_list (which is more costly
processing-wise).
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
commit 19a36b367f1a33f1eb65e0c5164a3209fcef16e6
Author: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon Nov 12 11:21:36 2018 +1300
replmd: Remove some redundant code
At first glance, this code seemed completely unnecessary. However, it
was added (by commit f6bc4c08b19f5615) for a valid reason: adding the
whenChanged/uSNChanged attributes to the message can cause msg->elements
to be reallocated, which means the old_el pointer (which points to
msg->elements memory) can be out of date.
whenChanged/uSNChanged now get added to the msg last, just before the DB
modify operation. So old_el can no longer become out of date within
replmd_process_link_attribute(), so re-fetching it is now redundant.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
commit f53954d0fd4d67c73336e9fda81429df9c55d77a
Author: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon Nov 12 11:13:28 2018 +1300
replmd: Move where we update the usnChanged/whenChanged
Move this closer to where the source object actually gets modified.
The main reason to do this is that adding fields can cause the
msg->elements to be reallocated, which will invalidate all the
old_el and pdn_list pointers which are derived from the msg.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
commit c371fef58638a7570becf29dd0df651004219d0a
Author: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon Nov 12 11:00:52 2018 +1300
replmd: Only modify the object if it actually changed
Commit 775054afbe1512 reworked replmd_process_link_attribute() so that
we batch together DB operations for the same source object. However, it
was possible that the object had not actually changed at all, e.g.
- link was already processed by critical-objects-only during join, or
- we were doing a full-sync and processing info that was already
up-to-date in our DB.
In these cases we modified the object anyway, even though nothing had
changed. This patch fixes it up, so we check that the object has
actually changed before modifying the DB.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
commit cb3520fbaf9413da71645e653f9501d1645405b4
Author: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon Nov 12 10:43:39 2018 +1300
replmd: replmd_process_link_attribute() returns type of change made
In order to share work across related link attribute updates, we need
replmd_process_link_attribute() to let the caller know what actually
changed.
This patch adds an extra return type that'll be used in the next patch.
What we're interested in is: the update was ignored (i.e. it's old news),
a new link attribute was added (because this affects the overall
msg/element memory), and an existing link attribute was modified (due to
how links are actually stored, this includes deleting the link, as in
reality it simply involves setting the existing link to 'inactive').
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
-----------------------------------------------------------------------
Summary of changes:
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 197 +++++++++++++++++-------
1 file changed, 144 insertions(+), 53 deletions(-)
Changeset truncated at 500 lines:
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 58f9df9c98c..d89776e4bc6 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -134,6 +134,17 @@ struct replmd_replicated_request {
bool fix_link_sid;
};
+/*
+ * the result of replmd_process_linked_attribute(): either there was no change
+ * (update was ignored), a new link was added (either inactive or active), or
+ * an existing link was modified (active/inactive status may have changed).
+ */
+typedef enum {
+ LINK_CHANGE_NONE,
+ LINK_CHANGE_ADDED,
+ LINK_CHANGE_MODIFIED,
+} replmd_link_changed;
+
static int replmd_replicated_apply_merge(struct replmd_replicated_request *ar);
static int replmd_delete_internals(struct ldb_module *module, struct ldb_request *req, bool re_delete);
static int replmd_check_upgrade_links(struct ldb_context *ldb,
@@ -6796,9 +6807,18 @@ static int replmd_replicated_apply_isDeleted(struct replmd_replicated_request *a
{
struct ldb_dn *deleted_objects_dn;
struct ldb_message *msg = ar->objs->objects[ar->index_current].msg;
- int ret = dsdb_get_deleted_objects_dn(ldb_module_get_ctx(ar->module), msg, msg->dn,
- &deleted_objects_dn);
- if (ar->isDeleted && (ret != LDB_SUCCESS || ldb_dn_compare(msg->dn, deleted_objects_dn) != 0)) {
+ int ret;
+
+ if (!ar->isDeleted) {
+
+ /* not a deleted object, so nothing to do */
+ ar->index_current++;
+ return replmd_replicated_apply_next(ar);
+ }
+
+ ret = dsdb_get_deleted_objects_dn(ldb_module_get_ctx(ar->module), msg,
+ msg->dn, &deleted_objects_dn);
+ if (ret != LDB_SUCCESS || ldb_dn_compare(msg->dn, deleted_objects_dn) != 0) {
/*
* Do a delete here again, so that if there is
* anything local that conflicts with this
@@ -7917,8 +7937,21 @@ static int replmd_check_singleval_la_conflict(struct ldb_module *module,
return LDB_SUCCESS;
}
-/*
- process one linked attribute structure
+/**
+ * Processes one linked attribute received via replication.
+ * @param msg the source object for the link. For optimization, the same msg
+ * can be used across multiple calls to replmd_process_linked_attribute().
+ * @note this function should not add or remove any msg attributes (it should
+ * only add/modify values for the linked attribute being processed). Otherwise
+ * msg->elements is realloc'd and old_el/pdn_list pointers will be invalidated
+ * @param attr schema info for the linked attribute
+ * @param la_entry the linked attribute info received via DRS
+ * @param old_el the corresponding msg->element[] for the linked attribute
+ * @param pdn_list a (binary-searchable) parsed DN array for the existing link
+ * values in the msg. E.g. for a group, this is the existing members.
+ * @param change what got modified: either nothing, an existing link value was
+ * modified, or a new link value was added.
+ * @returns LDB_SUCCESS if OK, an error otherwise
*/
static int replmd_process_linked_attribute(struct ldb_module *module,
TALLOC_CTX *mem_ctx,
@@ -7926,7 +7959,10 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
struct ldb_message *msg,
const struct dsdb_attribute *attr,
struct la_entry *la_entry,
- struct ldb_request *parent)
+ struct ldb_request *parent,
+ struct ldb_message_element *old_el,
+ struct parsed_dn *pdn_list,
+ replmd_link_changed *change)
{
struct drsuapi_DsReplicaLinkedAttribute *la = la_entry->la;
struct ldb_context *ldb = ldb_module_get_ctx(module);
@@ -7934,9 +7970,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
int ret;
struct dsdb_dn *dsdb_dn = NULL;
uint64_t seq_num = 0;
- struct ldb_message_element *old_el;
- time_t t = time(NULL);
- struct parsed_dn *pdn_list, *pdn, *next;
+ struct parsed_dn *pdn, *next;
struct GUID guid = GUID_zero();
bool active = (la->flags & DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE)?true:false;
bool ignore_link;
@@ -7945,6 +7979,8 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
bool add_as_inactive = false;
WERROR status;
+ *change = LINK_CHANGE_NONE;
+
/* the value blob for the attribute holds the target object DN */
status = dsdb_dn_la_from_blob(ldb, attr, schema, mem_ctx,
la->value.blob, &dsdb_dn);
@@ -7956,25 +7992,6 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
return LDB_ERR_OPERATIONS_ERROR;
}
- old_el = ldb_msg_find_element(msg, attr->lDAPDisplayName);
- if (old_el == NULL) {
- ret = ldb_msg_add_empty(msg, attr->lDAPDisplayName, LDB_FLAG_MOD_REPLACE, &old_el);
- if (ret != LDB_SUCCESS) {
- ldb_module_oom(module);
- return LDB_ERR_OPERATIONS_ERROR;
- }
- } else {
- old_el->flags = LDB_FLAG_MOD_REPLACE;
- }
-
- /* parse the existing links */
- ret = get_parsed_dns_trusted(module, replmd_private, mem_ctx, old_el, &pdn_list,
- attr->syntax->ldap_oid, parent);
-
- if (ret != LDB_SUCCESS) {
- return ret;
- }
-
ret = replmd_check_target_exists(module, dsdb_dn, la_entry, msg->dn,
true, &guid, &ignore_link);
@@ -8048,6 +8065,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
val_to_update = pdn->v;
old_dsdb_dn = pdn->dsdb_dn;
+ *change = LINK_CHANGE_MODIFIED;
} else {
unsigned offset;
@@ -8088,6 +8106,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
val_to_update = &old_el->values[offset];
old_dsdb_dn = NULL;
+ *change = LINK_CHANGE_ADDED;
}
/* set the link attribute's value to the info that was received */
@@ -8126,27 +8145,6 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
}
}
- /* we only change whenChanged and uSNChanged if the seq_num
- has changed */
- ldb_msg_remove_attr(msg, "whenChanged");
- ldb_msg_remove_attr(msg, "uSNChanged");
- ret = add_time_element(msg, "whenChanged", t);
- if (ret != LDB_SUCCESS) {
- ldb_operr(ldb);
- return ret;
- }
-
- ret = add_uint64_element(ldb, msg, "uSNChanged", seq_num);
- if (ret != LDB_SUCCESS) {
- ldb_operr(ldb);
- return ret;
- }
-
- old_el = ldb_msg_find_element(msg, attr->lDAPDisplayName);
- if (old_el == NULL) {
- return ldb_operr(ldb);
- }
-
ret = dsdb_check_single_valued_link(attr, old_el);
if (ret != LDB_SUCCESS) {
return ret;
@@ -8196,7 +8194,7 @@ static int replmd_start_transaction(struct ldb_module *module)
/**
* Processes a group of linked attributes that apply to the same source-object
- * and attribute-ID
+ * and attribute-ID (and were received in the same replication chunk).
*/
static int replmd_process_la_group(struct ldb_module *module,
struct replmd_private *replmd_private,
@@ -8205,12 +8203,23 @@ static int replmd_process_la_group(struct ldb_module *module,
struct la_entry *la = NULL;
struct la_entry *prev = NULL;
int ret;
- TALLOC_CTX *tmp_ctx = talloc_new(la_group);
+ TALLOC_CTX *tmp_ctx = NULL;
struct la_entry *first_la = DLIST_TAIL(la_group->la_entries);
struct ldb_message *msg = NULL;
enum deletion_state deletion_state = OBJECT_NOT_DELETED;
struct ldb_context *ldb = ldb_module_get_ctx(module);
const struct dsdb_attribute *attr = NULL;
+ struct ldb_message_element *old_el = NULL;
+ struct parsed_dn *pdn_list = NULL;
+ replmd_link_changed change_type;
+ uint32_t num_changes = 0;
+ time_t t;
+ uint64_t seq_num = 0;
+
+ tmp_ctx = talloc_new(la_group);
+ if (tmp_ctx == NULL) {
+ return ldb_oom(ldb);
+ }
/*
* get the attribute being modified and the search result for the
@@ -8257,18 +8266,64 @@ static int replmd_process_la_group(struct ldb_module *module,
ldb_msg_remove_attr(msg, "isDeleted");
ldb_msg_remove_attr(msg, "isRecycled");
- /* go through and process the link targets for this source object */
+ /* get the msg->element[] for the link attribute being processed */
+ old_el = ldb_msg_find_element(msg, attr->lDAPDisplayName);
+ if (old_el == NULL) {
+ ret = ldb_msg_add_empty(msg, attr->lDAPDisplayName,
+ LDB_FLAG_MOD_REPLACE, &old_el);
+ if (ret != LDB_SUCCESS) {
+ ldb_module_oom(module);
+ return LDB_ERR_OPERATIONS_ERROR;
+ }
+ } else {
+ old_el->flags = LDB_FLAG_MOD_REPLACE;
+ }
+
+ /*
+ * go through and process the link target value(s) for this particular
+ * source object and attribute
+ */
for (la = DLIST_TAIL(la_group->la_entries); la; la=prev) {
prev = DLIST_PREV(la);
DLIST_REMOVE(la_group->la_entries, la);
+
+ /*
+ * parse the existing links (this can be costly for a large
+ * group, so we try to minimize the times we do it)
+ */
+ if (pdn_list == NULL) {
+ ret = get_parsed_dns_trusted(module, replmd_private,
+ tmp_ctx, old_el,
+ &pdn_list,
+ attr->syntax->ldap_oid,
+ NULL);
+
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
+ }
ret = replmd_process_linked_attribute(module, tmp_ctx,
replmd_private,
- msg, attr, la, NULL);
+ msg, attr, la, NULL,
+ old_el, pdn_list,
+ &change_type);
if (ret != LDB_SUCCESS) {
replmd_txn_cleanup(replmd_private);
return ret;
}
+ /*
+ * Adding a link reallocs memory, and so invalidates all the
+ * pointers in pdn_list. Reparse the PDNs on the next loop
+ */
+ if (change_type == LINK_CHANGE_ADDED) {
+ TALLOC_FREE(pdn_list);
+ }
+
+ if (change_type != LINK_CHANGE_NONE) {
+ num_changes++;
+ }
+
if ((++replmd_private->num_processed % 8192) == 0) {
DBG_NOTICE("Processed %u/%u linked attributes\n",
replmd_private->num_processed,
@@ -8276,6 +8331,42 @@ static int replmd_process_la_group(struct ldb_module *module,
}
}
+ /*
+ * it's possible we're already up-to-date and so don't need to modify
+ * the object at all (e.g. doing a 'drs replicate --full-sync')
+ */
+ if (num_changes == 0) {
+ TALLOC_FREE(tmp_ctx);
+ return LDB_SUCCESS;
+ }
+
+ /*
+ * Note that adding the whenChanged/etc attributes below will realloc
+ * msg->elements, invalidating the existing element/parsed-DN pointers
+ */
+ old_el = NULL;
+ TALLOC_FREE(pdn_list);
+
+ /* update whenChanged/uSNChanged as the object has changed */
+ t = time(NULL);
+ ret = ldb_sequence_number(ldb, LDB_SEQ_HIGHEST_SEQ,
+ &seq_num);
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
+
+ ret = add_time_element(msg, "whenChanged", t);
+ if (ret != LDB_SUCCESS) {
+ ldb_operr(ldb);
+ return ret;
+ }
+
+ ret = add_uint64_element(ldb, msg, "uSNChanged", seq_num);
+ if (ret != LDB_SUCCESS) {
+ ldb_operr(ldb);
+ return ret;
+ }
+
/* apply the link changes to the source object */
ret = linked_attr_modify(module, msg, NULL);
if (ret != LDB_SUCCESS) {
--
Samba Shared Repository
More information about the samba-cvs
mailing list