[WIP][PATCH] Safe LDB locking for better replication
Stefan Metzmacher
metze at samba.org
Fri Jun 16 13:13:37 UTC 2017
Am 16.06.2017 um 06:55 schrieb Andrew Bartlett:
> On Thu, 2017-06-15 at 16:58 +1200, Andrew Bartlett via samba-technical
> wrote:
>> On Wed, 2017-06-14 at 22:02 +1200, Andrew Bartlett via samba-
>> technical
>> wrote:
>>>> Further comments most welcome, and I plan to re-write it to use
>>>> read_lock() and read_unlock() operations tomorrow.
>>
>> Attached is the patch set against master
>>
>> As mentioned previously, my TODO list is:
>> - fix tests not to hang when failing
>
> They don't seem to hang, at least when reverting the whole-db lock.
>
>> - add test for the talloc_free(req) unlock case
>
> I've added such a test.
>
>> - add some kind of test for the partitions locking
>
> I've added a test for this.
>
>> - a dbcheck rule for out of date @ATTRIBUTES and @INDEXLIST (rather
>> than a runtime check)
>
> This is still TODO.
>
> With the proviso that I need that dbcheck rule (or drop this aspect of
> the patches), can I please get your review on this series. I'll run
> some more builds, but I'm pretty confident on the patch series at this
> point and would like to get this into master as soon as you are
> comfortable.
>
> It would be good to have at least a week of this in master before rc1
> freezes, so we can deal with any unexpected fallout, despite our
> thorough testing.
I've pushed the first bunch of patches.
You can also push the attaches two patches (which I split from one
single commit that looked strange).
Are there any other pending ldb/tdb changes?
I'll look at the rest next week.
metze
-------------- next part --------------
From 4dd1e9119ec7fdf19530460f039feabadbdf3dc6 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Thu, 8 Jun 2017 23:05:26 +1200
Subject: [PATCH 1/2] dsdb: Rework schema_init module to avoid database write
and use valid memory
The schema can go away unless the second argument (the memory context) is supplied
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
---
source4/dsdb/samdb/ldb_modules/schema_load.c | 62 +++++++++++++++++++---------
1 file changed, 42 insertions(+), 20 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/schema_load.c b/source4/dsdb/samdb/ldb_modules/schema_load.c
index a2f8e57..b3313b4 100644
--- a/source4/dsdb/samdb/ldb_modules/schema_load.c
+++ b/source4/dsdb/samdb/ldb_modules/schema_load.c
@@ -358,29 +358,15 @@ failed:
return ret;
}
-
-static int schema_load_init(struct ldb_module *module)
+static int schema_load(struct ldb_context *ldb,
+ struct ldb_module *module)
{
- struct schema_load_private_data *private_data;
- struct ldb_context *ldb = ldb_module_get_ctx(module);
struct dsdb_schema *schema;
void *readOnlySchema;
int ret, metadata_ret;
-
- private_data = talloc_zero(module, struct schema_load_private_data);
- if (private_data == NULL) {
- return ldb_oom(ldb);
- }
- private_data->module = module;
+ TALLOC_CTX *frame = talloc_stackframe();
- ldb_module_set_private(module, private_data);
-
- ret = ldb_next_init(module);
- if (ret != LDB_SUCCESS) {
- return ret;
- }
-
- schema = dsdb_get_schema(ldb, NULL);
+ schema = dsdb_get_schema(ldb, frame);
metadata_ret = schema_metadata_open(module);
@@ -394,10 +380,12 @@ static int schema_load_init(struct ldb_module *module)
ldb_debug_set(ldb, LDB_DEBUG_FATAL,
"schema_load_init: dsdb_set_schema_refresh_fns() failed: %d:%s: %s",
ret, ldb_strerror(ret), ldb_errstring(ldb));
+ TALLOC_FREE(frame);
return ret;
}
}
+ TALLOC_FREE(frame);
return LDB_SUCCESS;
}
@@ -408,11 +396,12 @@ static int schema_load_init(struct ldb_module *module)
* have to update the backend server schema too */
if (readOnlySchema != NULL) {
struct dsdb_schema *new_schema;
- ret = dsdb_schema_from_db(module, private_data, 0, &new_schema);
+ ret = dsdb_schema_from_db(module, frame, 0, &new_schema);
if (ret != LDB_SUCCESS) {
ldb_debug_set(ldb, LDB_DEBUG_FATAL,
"schema_load_init: dsdb_schema_from_db() failed: %d:%s: %s",
ret, ldb_strerror(ret), ldb_errstring(ldb));
+ TALLOC_FREE(frame);
return ret;
}
@@ -422,6 +411,7 @@ static int schema_load_init(struct ldb_module *module)
ldb_debug_set(ldb, LDB_DEBUG_FATAL,
"schema_load_init: dsdb_set_schema() failed: %d:%s: %s",
ret, ldb_strerror(ret), ldb_errstring(ldb));
+ TALLOC_FREE(frame);
return ret;
}
@@ -432,20 +422,23 @@ static int schema_load_init(struct ldb_module *module)
ldb_debug_set(ldb, LDB_DEBUG_FATAL,
"schema_load_init: dsdb_set_schema_refresh_fns() failed: %d:%s: %s",
ret, ldb_strerror(ret), ldb_errstring(ldb));
+ TALLOC_FREE(frame);
return ret;
}
} else {
ldb_debug_set(ldb, LDB_DEBUG_FATAL,
"schema_load_init: failed to open metadata.tdb");
+ TALLOC_FREE(frame);
return metadata_ret;
}
- schema = dsdb_get_schema(ldb, NULL);
+ schema = dsdb_get_schema(ldb, frame);
/* We do this, invoking the refresh handler, so we know that it works */
if (schema == NULL) {
ldb_debug_set(ldb, LDB_DEBUG_FATAL,
"schema_load_init: dsdb_get_schema failed");
+ TALLOC_FREE(frame);
return LDB_ERR_OPERATIONS_ERROR;
}
@@ -457,6 +450,35 @@ static int schema_load_init(struct ldb_module *module)
"@INDEXLIST and @ATTRIBUTES "
"records to match database schema: %s",
ldb_errstring(ldb));
+ TALLOC_FREE(frame);
+ return ret;
+ }
+
+ TALLOC_FREE(frame);
+ return LDB_SUCCESS;
+}
+
+static int schema_load_init(struct ldb_module *module)
+{
+ struct ldb_context *ldb = ldb_module_get_ctx(module);
+ struct schema_load_private_data *private_data;
+ int ret;
+
+ private_data = talloc_zero(module, struct schema_load_private_data);
+ if (private_data == NULL) {
+ return ldb_oom(ldb);
+ }
+ private_data->module = module;
+
+ ldb_module_set_private(module, private_data);
+
+ ret = ldb_next_init(module);
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
+
+ ret = schema_load(ldb, module);
+ if (ret != LDB_SUCCESS) {
return ret;
}
--
1.9.1
From 6dce3cde2c05b164c4e7a3daafa85d71544dda5d Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Thu, 8 Jun 2017 23:05:26 +1200
Subject: [PATCH 2/2] dsdb: remove dsdb_schema_set_indices_and_attributes()
from schema_load()
There is no need to write the @ATTRIBUTES and @INDEXLIST on every DB load
we only need to write it if the schema is changed, and the repl_meta_data module
will notice if that happens and trigger the DSDB_EXTENDED_SCHEMA_UPDATE_NOW_OID
extended operation.
Note that dsdb_schema_set_indices_and_attributes(write_indices_and_attributes=false)
is already called deep inside dsdb_get_schema().
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
---
source4/dsdb/samdb/ldb_modules/schema_load.c | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/schema_load.c b/source4/dsdb/samdb/ldb_modules/schema_load.c
index b3313b4..e9ffdf0 100644
--- a/source4/dsdb/samdb/ldb_modules/schema_load.c
+++ b/source4/dsdb/samdb/ldb_modules/schema_load.c
@@ -442,18 +442,6 @@ static int schema_load(struct ldb_context *ldb,
return LDB_ERR_OPERATIONS_ERROR;
}
- /* Now check the @INDEXLIST is correct, or fix it up */
- ret = dsdb_schema_set_indices_and_attributes(ldb, schema,
- true);
- if (ret != LDB_SUCCESS) {
- ldb_asprintf_errstring(ldb, "Failed to update "
- "@INDEXLIST and @ATTRIBUTES "
- "records to match database schema: %s",
- ldb_errstring(ldb));
- TALLOC_FREE(frame);
- return ret;
- }
-
TALLOC_FREE(frame);
return LDB_SUCCESS;
}
--
1.9.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20170616/34623489/signature.sig>
More information about the samba-technical
mailing list