LMDB key value backend for ldb_tdb (to be renamed ldb_key_val)

[Garming Sam via samba-technical]

Hi,

This is our current set of patches for implementing an LMDB based
backend for LDB. The work is based on a prototype I wrote around this
time last year inspired by Jakub's efforts. In saying that, the approach
I took was completely different. The idea was to refactor ldb_tdb to be
agnostic about which database backend was being used. The advantage has
been quite minimal amount of code required to implement a functional
64-bit database backend. Many of the performance optimizations made for
ldb_tdb can simply be reused, but conversely, for now we have deferred
re-thinking the overall architecture e.g. consolidating the partitions
into a single file using LMDB sub databases.

Currently this backend must have keys restricted to less than 511 bytes,
which is fine for our new GUID indexing scheme, but can run into issues
with our indexes. Gary is currently working on using truncated keys to
bypass this limit.

This current set of patches passes autobuild while still running with
the TDB backend. We have patches to pass the testsuite using the LMDB
backend, but a few of them still need tidying up and have been omitted
here for now.

Performance numbers at this point seem a bit tricky to obtain. Our
existing perf testing infrastructure relies on the test environment,
however, LMDB appears to run noticeably slower under cwrap due to some
calls being intercepted. Basic testing indicates better baseline figures
and better concurrency (reads no longer block writes, and writes
effectively only block reads during commit - not prepare commit), but
there needs to be a lot more testing to properly understand the
performance characteristics.

Somewhat related, I am currently investigating running our tests without
socket wrapper by using network namespaces. There are still areas where
being able to run socket wrapper is definitely useful, but performance
testing is definitely not one of them.

Noteworthy fixes required or bugs found:

- metadata.tdb needs to be committed last during a transaction
(getncchanges tests were causing replication errors as reads occurred
between the commit of metadata.tdb and the rest of the partitions).

- schema loading during a read lock needs a cached value (as writes can
happen during reads, long running read-locked operations could read new
metadata.tdb values).

Any thoughts or comments would be well appreciated. There is definitely
more to come in this space and using LMDB allows us to effectively
implement a number of further improvements like indexing for >= which
would make replication much faster.


Cheers,

Garming

[jim via samba-technical]

diff --git a/source4/dsdb/samdb/ldb_modules/partition_metadata.c b/source4/dsdb/samdb/ldb_modules/partition_metadata.c
index 3c5180b..3ba7905 100644
--- a/source4/dsdb/samdb/ldb_modules/partition_metadata.c
+++ b/source4/dsdb/samdb/ldb_modules/partition_metadata.c
@@ -319,13 +319,33 @@ int partition_metadata_init(struct ldb_module *module)
goto end;
}
+ ret = partition_start_trans(module);
+ if (ret != LDB_SUCCESS) {
+ talloc_free(data->metadata);
+ data->metadata = NULL;
+ }
+
ret = partition_metadata_set_sequence_number(module);
if (ret != LDB_SUCCESS) {
talloc_free(data->metadata);
data->metadata = NULL;
+ partition_del_trans(module);
+ } else {
+
+ ret = partition_prepare_commit(module);
+ if (ret != LDB_SUCCESS) {
+ talloc_free(data->metadata);
+ data->metadata = NULL;
+ } else {
+ ret = partition_end_trans(module);
+ if (ret != LDB_SUCCESS) {
+ talloc_free(data->metadata);
+ data->metadata = NULL;
+ }
+ }
}
return ret;
}

You should 'goto end;' in each failure case just like the original code.
This will eliminate the 'else' groups and all of the extra indentation.
It will read easier too.

Do you need to call partition_del_trans(module) when
partition_prepare_commit and partition_prepare_commit fail?

[Andreas Schneider via samba-technical]

On Wednesday, 21 February 2018 23:45:08 CET Garming Sam via samba-technical

Post by Garming Sam via samba-technical
Hi,

Hi Garming,

Post by Garming Sam via samba-technical
Any thoughts or comments would be well appreciated. There is definitely
more to come in this space and using LMDB allows us to effectively
implement a number of further improvements like indexing for >= which
would make replication much faster.

when Jakub started to look into lmdb for ldb as a backend, we didn't have any
test to verify that the API is working the same way with tdb and lmdb. For
that he started to write tests for the API. The tests also revealed some bugs.

If I look at the patchset I would expect more unit tests. The unit tests are
fast to run and we do that when we package ldb to verify it is working
correctly. Especially when we apply patches.

And if you think some things can't be tested, the mocking support of cmocka
might be the solution. Let me know if you need help.



Cheers,


Andreas

[Andrew Bartlett via samba-technical]

On Thu, 2018-02-22 at 08:14 +0100, Andreas Schneider via samba-

Post by Andreas Schneider via samba-technical
On Wednesday, 21 February 2018 23:45:08 CET Garming Sam via samba-technical

Post by Garming Sam via samba-technical
Hi,

Hi Garming,

Post by Garming Sam via samba-technical
Any thoughts or comments would be well appreciated. There is definitely
more to come in this space and using LMDB allows us to effectively
implement a number of further improvements like indexing for >= which
would make replication much faster.

when Jakub started to look into lmdb for ldb as a backend, we didn't have any
test to verify that the API is working the same way with tdb and lmdb. For
that he started to write tests for the API. The tests also revealed some bugs.
If I look at the patchset I would expect more unit tests. The unit tests are
fast to run and we do that when we package ldb to verify it is working
correctly. Especially when we apply patches.

I totally agree, and I've got a proposed README.Coding that says much
the same. I'll send that for comment tomorrow.

Thankfully Jakub's tests did land, as did a whole lot more around the
GUID index work, and as Garming mentioned there are a few more already
written that are not ready yet.

Is there something specific you are looking for or that I should be
looking out for when I start a review pass over this?

Post by Andreas Schneider via samba-technical
And if you think some things can't be tested, the mocking support of cmocka
might be the solution. Let me know if you need help.

Thanks!

Andrew Bartlett

[Andrew Bartlett via samba-technical]

On Thu, 2018-02-22 at 11:45 +1300, Garming Sam via samba-technical

Post by Garming Sam via samba-technical
Hi,
This is our current set of patches for implementing an LMDB based
backend for LDB. The work is based on a prototype I wrote around this
time last year inspired by Jakub's efforts. In saying that, the approach
I took was completely different. The idea was to refactor ldb_tdb to be
agnostic about which database backend was being used. The advantage has
been quite minimal amount of code required to implement a functional
64-bit database backend. Many of the performance optimizations made for
ldb_tdb can simply be reused, but conversely, for now we have deferred
re-thinking the overall architecture e.g. consolidating the partitions
into a single file using LMDB sub databases.

G'Day Garming,

Attached is a few of your patches that I've reviewed, to prepare the
way for this in Samba master. Thanks for the cross-review in the
office today.

Post by Garming Sam via samba-technical
Currently this backend must have keys restricted to less than 511 bytes,
which is fine for our new GUID indexing scheme, but can run into issues
with our indexes. Gary is currently working on using truncated keys to
bypass this limit.

I'm very glad to say that this has been very well addressed by Gary's
patches. When the rest lands we will need to ensure the integration of
the two works (mostly a matter of running the new tests on both).

Post by Garming Sam via samba-technical
This current set of patches passes autobuild while still running with
the TDB backend. We have patches to pass the testsuite using the LMDB
backend, but a few of them still need tidying up and have been omitted
here for now.

These, and the actual LMDB backend, will be in the next round.

Post by Garming Sam via samba-technical
Performance numbers at this point seem a bit tricky to obtain. Our
existing perf testing infrastructure relies on the test environment,
however, LMDB appears to run noticeably slower under cwrap due to some
calls being intercepted. Basic testing indicates better baseline figures
and better concurrency (reads no longer block writes, and writes
effectively only block reads during commit - not prepare commit), but
there needs to be a lot more testing to properly understand the
performance characteristics.
Somewhat related, I am currently investigating running our tests without
socket wrapper by using network namespaces. There are still areas where
being able to run socket wrapper is definitely useful, but performance
testing is definitely not one of them.
- metadata.tdb needs to be committed last during a transaction
(getncchanges tests were causing replication errors as reads occurred
between the commit of metadata.tdb and the rest of the partitions).
- schema loading during a read lock needs a cached value (as writes can
happen during reads, long running read-locked operations could read new
metadata.tdb values).

These fixes are included in the reviewed set.

I would like to push the attached tomorrow, once I get confirmation
they pass a private autobuild.

Thanks!

Andrew Bartlett

[Andrew Bartlett via samba-technical]

On Mon, 2018-03-05 at 18:31 +1300, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
On Thu, 2018-02-22 at 11:45 +1300, Garming Sam via samba-technical

Post by Garming Sam via samba-technical
Hi,
This is our current set of patches for implementing an LMDB based
backend for LDB. The work is based on a prototype I wrote around this
time last year inspired by Jakub's efforts. In saying that, the approach
I took was completely different. The idea was to refactor ldb_tdb to be
agnostic about which database backend was being used. The advantage has
been quite minimal amount of code required to implement a functional
64-bit database backend. Many of the performance optimizations made for
ldb_tdb can simply be reused, but conversely, for now we have deferred
re-thinking the overall architecture e.g. consolidating the partitions
into a single file using LMDB sub databases.

G'Day Garming,
Attached is a few of your patches that I've reviewed, to prepare the
way for this in Samba master. Thanks for the cross-review in the
office today.

G'Day Team,

Attached are the current patches to add an lmdb backend, controlled
with an --with-lmdb configure option (currently off) to Samba and ldb.

We are on the last leg of this journey, with just some tests of the
key-value operations to write and a little work on the rest of the code
(fork() handling primarily). We are re-working the existing ldb cmokca
and python tests to operate against LMDB. The patch set is attached
here for comment.

Andrew Bartlett

[Stefan Metzmacher via samba-technical]

Hi Andrew,

Post by Andrew Bartlett via samba-technical
Attached are the current patches to add an lmdb backend, controlled
with an --with-lmdb configure option (currently off) to Samba and ldb.
We are on the last leg of this journey, with just some tests of the
key-value operations to write and a little work on the rest of the code
(fork() handling primarily). We are re-working the existing ldb cmokca
and python tests to operate against LMDB. The patch set is attached
here for comment.

The first patches seem to have some leading whitespaces in some places.
Often there's no empty line after the variable declaration section
and also new empty line after a closing '}' just before the final
'return' line of a function.
Can you please fix this? It would make it much easier to read/review the
code (at least for me).

0644 seems to be used hardcoded, shouldn't this be ldb_get_create_perms()?

metze

[Garming Sam via samba-technical]

Fixed a number of whitespace issues and removed the hardcoded 0644.

Post by Stefan Metzmacher via samba-technical
Hi Andrew,

Post by Andrew Bartlett via samba-technical
Attached are the current patches to add an lmdb backend, controlled
with an --with-lmdb configure option (currently off) to Samba and ldb.
We are on the last leg of this journey, with just some tests of the
key-value operations to write and a little work on the rest of the code
(fork() handling primarily). We are re-working the existing ldb cmokca
and python tests to operate against LMDB. The patch set is attached
here for comment.

The first patches seem to have some leading whitespaces in some places.
Often there's no empty line after the variable declaration section
and also new empty line after a closing '}' just before the final
'return' line of a function.
Can you please fix this? It would make it much easier to read/review the
code (at least for me).
0644 seems to be used hardcoded, shouldn't this be ldb_get_create_perms()?
metze

[Garming Sam via samba-technical]

Post by Garming Sam via samba-technical
Performance numbers at this point seem a bit tricky to obtain. Our
existing perf testing infrastructure relies on the test environment,
however, LMDB appears to run noticeably slower under cwrap due to some
calls being intercepted. Basic testing indicates better baseline figures
and better concurrency (reads no longer block writes, and writes
effectively only block reads during commit - not prepare commit), but
there needs to be a lot more testing to properly understand the
performance characteristics.
Somewhat related, I am currently investigating running our tests without
socket wrapper by using network namespaces. There are still areas where
being able to run socket wrapper is definitely useful, but performance
testing is definitely not one of them.

Findings so far:

Socket wrapper has a roughly 20% overhead with pure adds when using LMDB
(compared to using a network namespace) with some NOSYNC options set.
Although I find TDB incurs a similar overhead for running under
namespaces strangely enough, which I don't have an adequate explanation for.

Some further testing seems to indicate that LMDB performance is on
parity with TDB performance with writes, and has improved performance
for searches as the database grows larger (5-10% slower with no added
users, 10-20% faster on an unindexed search with around 5000 users). The
other refactorings required for the key value store also appear to have
little to no performance impact on TDB.

Cheers,

Garming

[Gary Lockyer via samba-technical]

Updated patch set adding an lmdb backend to ldb, incorporating the
feedback received to date. The patch set adds extra testing of the API
and the new key value store layers.


A lot of the names are still tdb specific. Once these patches have
landed on master, I'll put up patches that rename things sensibly.

Please note that these patches can't be pushed until the lib-lmdb and
lmdb-utils have been installed on sndevel.


Comments and reviews greatly appreciated.
Gary.

[Gary Lockyer via samba-technical]

Please add my reviewed by to Andrews commits in this patch set, when it
gets pushed.

Cheers
Gary.

Post by Gary Lockyer via samba-technical
Updated patch set adding an lmdb backend to ldb, incorporating the
feedback received to date. The patch set adds extra testing of the API
and the new key value store layers.
A lot of the names are still tdb specific. Once these patches have
landed on master, I'll put up patches that rename things sensibly.
Please note that these patches can't be pushed until the lib-lmdb and
lmdb-utils have been installed on sndevel.
Comments and reviews greatly appreciated.
Gary.

[Stefan Metzmacher via samba-technical]

Hi Gary,

This seems to be still some kind of work in progress, correct?
There're things like this:

+ else:
+ # TODO this should default to false in the final release
+ # REJECT if in final patch set
+ conf.env['ENABLE_LMDB'] = False

+ ... "Specify the database backend to be used TODO better words"

I'd also like to have this patchset rearranged, so that all generic
cleanups, like :
- introducing the 'ldb://' prefix
- adding new tests
- ldb: Change some prototypes to using ldb_val instead of TDB_DATA
- ldb: Change remaining fetch prototypes to remove TDB_DATA
come first.
- and more..
I don't understand the change from 'test.ldb' to 'sam.ldb'

Why is initialize_password_db() not within reinit_after_fork()?

metze

Post by Gary Lockyer via samba-technical
Updated patch set adding an lmdb backend to ldb, incorporating the
feedback received to date. The patch set adds extra testing of the API
and the new key value store layers.
A lot of the names are still tdb specific. Once these patches have
landed on master, I'll put up patches that rename things sensibly.
Please note that these patches can't be pushed until the lib-lmdb and
lmdb-utils have been installed on sndevel.
Comments and reviews greatly appreciated.
Gary.

[Andrew Bartlett via samba-technical]

On Thu, 2018-03-29 at 10:31 +0200, Stefan Metzmacher via samba-

Post by Stefan Metzmacher via samba-technical
Hi Gary,
This seems to be still some kind of work in progress, correct?
+ # TODO this should default to false in the final release
+ # REJECT if in final patch set
+ conf.env['ENABLE_LMDB'] = False
+ ... "Specify the database backend to be used TODO better words"

I can't find that in the patch set (attached) that Gary sent. Are you
sure you reviewed the right patch set?

Post by Stefan Metzmacher via samba-technical
I'd also like to have this patchset rearranged, so that all generic
- introducing the 'ldb://' prefix
- adding new tests
- ldb: Change some prototypes to using ldb_val instead of TDB_DATA
- ldb: Change remaining fetch prototypes to remove TDB_DATA
come first.
- and more..

At this stage in the process this will be quite difficult. Sadly this
effort has been ongoing for a number of months now, and re-ordering the
patches at this point is not practical.

The ldb:// patch set you ask to separate from the lmdb work depends on
lmdb and was posted in substantially this form by Garming in February.

In general the big difference here is that in ldb tests cannot be added
earlier in the series than the code they depend on because the ldb
'make test' has no concept of knownfail.

Sadly this means the lmdb tests cannot be included before lmdb is
implemented, nor can tests be included before the bug they demonstrate.

However I will work today to see if anything can be pulled forward
without upending the entire patch set. It may be possible to bring
some tests forward without the build hook for the lmdb mode, for
example.

(Gary has already started on other work).

Post by Stefan Metzmacher via samba-technical
I don't understand the change from 'test.ldb' to 'sam.ldb'

The code to create the sam.ldb.d directory changed (to remove the last
component via ldb_relative_name()) and for a test of a feature
(samba3sam and the ldb_map stuff) it wasn't worth contortions not to
just use the sam.ldb name.

Post by Stefan Metzmacher via samba-technical
Why is initialize_password_db() not within reinit_after_fork()?

Because it causes a dependency loop. I personally tried really hard to
untangle this one, but it just wasn't possible.

I hope this clarifies things and you can allow us to merge the patch
set without a substantial re-work at this stage.

Thanks,

Andrew Bartlett

[Andrew Bartlett via samba-technical]

Post by Andrew Bartlett via samba-technical
However I will work today to see if anything can be pulled forward
without upending the entire patch set. It may be possible to bring
some tests forward without the build hook for the lmdb mode, for
example.

I've spend my entire work day today re-ordering the patch set. I hope
this is now satisfactory, as I can't put much more time into this.

Attached are two patches, one on top of the other patches sent today,
and one with all the LDB patches I'm trying to get merged.

Each patch compiles on its own and passes the ldb tests on its own.

If you could let me know that you are happy that I've addressed your
concerns as far as is practical that would be most appreciated.
(Review comments also most welcome).

The full CI results will be here:
https://gitlab.com/catalyst-samba/samba/pipelines/19933665

I've got one last patch ordering issue to work out with the
upgradeprovision.alpha13 test, but otherwise I do think this is good to
go.

Thanks,

Andrew Bartlett

[Stefan Metzmacher via samba-technical]

Hi Andrew,

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
However I will work today to see if anything can be pulled forward
without upending the entire patch set. It may be possible to bring
some tests forward without the build hook for the lmdb mode, for
example.

I've spend my entire work day today re-ordering the patch set. I hope
this is now satisfactory, as I can't put much more time into this.
Attached are two patches, one on top of the other patches sent today,
and one with all the LDB patches I'm trying to get merged.
Each patch compiles on its own and passes the ldb tests on its own.
If you could let me know that you are happy that I've addressed your
concerns as far as is practical that would be most appreciated.
(Review comments also most welcome).
https://gitlab.com/catalyst-samba/samba/pipelines/19933665
I've got one last patch ordering issue to work out with the
upgradeprovision.alpha13 test, but otherwise I do think this is good to
go.

Thanks! I'll have a closer look tomorrow.

metze

[Andrew Bartlett via samba-technical]

Post by Stefan Metzmacher via samba-technical
Hi Andrew,

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
However I will work today to see if anything can be pulled forward
without upending the entire patch set. It may be possible to bring
some tests forward without the build hook for the lmdb mode, for
example.

I've spend my entire work day today re-ordering the patch set. I hope
this is now satisfactory, as I can't put much more time into this.
Attached are two patches, one on top of the other patches sent today,
and one with all the LDB patches I'm trying to get merged.
Each patch compiles on its own and passes the ldb tests on its own.
If you could let me know that you are happy that I've addressed your
concerns as far as is practical that would be most appreciated.
(Review comments also most welcome).
https://gitlab.com/catalyst-samba/samba/pipelines/19933665
I've got one last patch ordering issue to work out with the
upgradeprovision.alpha13 test, but otherwise I do think this is good to
go.

Thanks! I'll have a closer look tomorrow.

I'll avoid sending a blow-by-blow update series but I've rebased this
on master (Andreas added a new test for ldb_qsort) and the whole set is
in this branch:

https://gitlab.com/catalyst-samba/samba/commits/abartlet-lmdb-reordered

Andrew Bartlett

[Andrew Bartlett via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
However I will work today to see if anything can be pulled forward
without upending the entire patch set. It may be possible to bring
some tests forward without the build hook for the lmdb mode, for
example.

I've spend my entire work day today re-ordering the patch set. I hope
this is now satisfactory, as I can't put much more time into this.
Attached are two patches, one on top of the other patches sent today,
and one with all the LDB patches I'm trying to get merged.
Each patch compiles on its own and passes the ldb tests on its own.
If you could let me know that you are happy that I've addressed your
concerns as far as is practical that would be most appreciated.
(Review comments also most welcome).
https://gitlab.com/catalyst-samba/samba/pipelines/19933665

Garming, this showed:
https://gitlab.com/catalyst-samba/samba/-/jobs/60989918

That is, a return to the imfamous 'failed to upgrade hash locks' on
TDB. Is this still something we get from time to time or should I look
into it more specifically?

Andrew Bartlett

[Andrew Bartlett via samba-technical]

On Thu, 2018-04-05 at 07:18 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
However I will work today to see if anything can be pulled forward
without upending the entire patch set. It may be possible to bring
some tests forward without the build hook for the lmdb mode, for
example.

I've spend my entire work day today re-ordering the patch set. I hope
this is now satisfactory, as I can't put much more time into this.
Attached are two patches, one on top of the other patches sent today,
and one with all the LDB patches I'm trying to get merged.
Each patch compiles on its own and passes the ldb tests on its own.
If you could let me know that you are happy that I've addressed your
concerns as far as is practical that would be most appreciated.
(Review comments also most welcome).
https://gitlab.com/catalyst-samba/samba/pipelines/19933665

https://gitlab.com/catalyst-samba/samba/-/jobs/60989918
That is, a return to the imfamous 'failed to upgrade hash locks' on
TDB. Is this still something we get from time to time or should I look
into it more specifically?

I've taken out three suspicious patches and got a pass on this branch:

https://gitlab.com/catalyst-samba/samba/commits/abartlet-ldb-lmdb-pre-no-fork-change

I plan on landing these preparatory patches today, while I wait for
Metze's comments on the rest and debug which of the three suspicious
patches is the issue.

Metze: I trust this won't pre-empt any further comments you have on
the actual LMDB change. On that wider patch set posted in the other
thread, please let me know your thoughts or if you are happy for me to
proceed without your specific review.

Thanks,

Andrew Bartlett

[Andrew Bartlett via samba-technical]

On Fri, 2018-04-06 at 06:38 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
On Thu, 2018-04-05 at 07:18 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
However I will work today to see if anything can be pulled forward
without upending the entire patch set. It may be possible to bring
some tests forward without the build hook for the lmdb mode, for
example.

I've spend my entire work day today re-ordering the patch set. I hope
this is now satisfactory, as I can't put much more time into this.
Attached are two patches, one on top of the other patches sent today,
and one with all the LDB patches I'm trying to get merged.
Each patch compiles on its own and passes the ldb tests on its own.
If you could let me know that you are happy that I've addressed your
concerns as far as is practical that would be most appreciated.
(Review comments also most welcome).
https://gitlab.com/catalyst-samba/samba/pipelines/19933665

https://gitlab.com/catalyst-samba/samba/-/jobs/60989918
That is, a return to the imfamous 'failed to upgrade hash locks' on
TDB. Is this still something we get from time to time or should I look
into it more specifically?

https://gitlab.com/catalyst-samba/samba/commits/abartlet-ldb-lmdb-pre-no-fork-change
I plan on landing these preparatory patches today, while I wait for
Metze's comments on the rest and debug which of the three suspicious
patches is the issue.
Metze: I trust this won't pre-empt any further comments you have on
the actual LMDB change. On that wider patch set posted in the other
thread, please let me know your thoughts or if you are happy for me to
proceed without your specific review.

Sorry, I posted this on the wrong sub-thread. The wider patches are
those I posted here earlier. The patches I'm planning to land today
are some of those that Andreas said "Wow, this is great work and a load
of unit tests." about yesterday.

Thanks,

Andrew Bartlett

Post by Andrew Bartlett via samba-technical
Thanks,
Andrew Bartlett

[Andrew Bartlett via samba-technical]

On Thu, 2018-04-05 at 07:18 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
However I will work today to see if anything can be pulled forward
without upending the entire patch set. It may be possible to bring
some tests forward without the build hook for the lmdb mode, for
example.

I've spend my entire work day today re-ordering the patch set. I hope
this is now satisfactory, as I can't put much more time into this.
Attached are two patches, one on top of the other patches sent today,
and one with all the LDB patches I'm trying to get merged.
Each patch compiles on its own and passes the ldb tests on its own.
If you could let me know that you are happy that I've addressed your
concerns as far as is practical that would be most appreciated.
(Review comments also most welcome).
https://gitlab.com/catalyst-samba/samba/pipelines/19933665

https://gitlab.com/catalyst-samba/samba/-/jobs/60989918
That is, a return to the imfamous 'failed to upgrade hash locks' on
TDB. Is this still something we get from time to time or should I look
into it more specifically?

I've seen this on a master build, so it is just happening without these
patches, but I also have a patch to address the issue by locking the
init() hook.

I have them in this branch:

https://gitlab.com/catalyst-samba/samba/commits/lmdb-pre-7

You can see the CI results for the current iteration by clicking the CI
icon on the top commit, currently
https://gitlab.com/catalyst-samba/samba/pipelines/20234789

Andrew Bartlett

[Andrew Bartlett via samba-technical]

G'Day metze,

As you may have noticed I'm chasing down some existing TDB locking
issues that this patch seems to have made more common.

However the patch set is essentially final and the LMDB parts haven't
changed since those mailed to you this time last week.

Here is the current status:
https://gitlab.com/catalyst-samba/samba/commits/abartlet-lmdb-reordered
-2

I know you are busy, so I'm assuming this has dropped off your radar.
That's OK, but I'm writing this to say that once I get past the CI,
deal with the knownfail Jim requested and obtain a second review on the
amended branch, I'm looking to push tomorrow or Friday.

If you have any comments please get them to me for my work day
tomorrow.

Thanks,

Andrew Bartlett

[Andrew Bartlett via samba-technical]

Post by Andrew Bartlett via samba-technical
G'Day metze,
As you may have noticed I'm chasing down some existing TDB locking
issues that this patch seems to have made more common.
However the patch set is essentially final and the LMDB parts haven't
changed since those mailed to you this time last week.
https://gitlab.com/catalyst-samba/samba/commits/abartlet-lmdb-reordered
-2

I think I've fixed the last bug (or at least the last one for tonight)
and here is the CI for the updated branch above (and attached):

https://gitlab.com/catalyst-samba/samba/pipelines/20322095

Post by Andrew Bartlett via samba-technical
I know you are busy, so I'm assuming this has dropped off your radar.
That's OK, but I'm writing this to say that once I get past the CI,
deal with the knownfail Jim requested and obtain a second review on the
amended branch, I'm looking to push tomorrow or Friday.
If you have any comments please get them to me for my work day
tomorrow.
Thanks,
Andrew Bartlett

[Stefan Metzmacher via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
G'Day metze,
As you may have noticed I'm chasing down some existing TDB locking
issues that this patch seems to have made more common.
However the patch set is essentially final and the LMDB parts haven't
changed since those mailed to you this time last week.
https://gitlab.com/catalyst-samba/samba/commits/abartlet-lmdb-reordered
-2

I think I've fixed the last bug (or at least the last one for tonight)
https://gitlab.com/catalyst-samba/samba/pipelines/20322095

Post by Andrew Bartlett via samba-technical
I know you are busy, so I'm assuming this has dropped off your radar.
That's OK, but I'm writing this to say that once I get past the CI,
deal with the knownfail Jim requested and obtain a second review on the
amended branch, I'm looking to push tomorrow or Friday.
If you have any comments please get them to me for my work day
tomorrow.

I'd like to have a look, but that might take a few days.

And as you seem to still find bugs, I don't understand why this needs
to get to master as soon as possible. The branch point for 4.9.0rc1
won't happen in near future.

Thanks!
metze

[Andrew Bartlett via samba-technical]

On Wed, 2018-04-11 at 14:15 +0200, Stefan Metzmacher via samba-

Post by Stefan Metzmacher via samba-technical
I'd like to have a look, but that might take a few days.

Can you please be specific? Last week you said you would have feedback
for me in a day, and it has been over a week.

Post by Stefan Metzmacher via samba-technical
And as you seem to still find bugs, I don't understand why this needs
to get to master as soon as possible.

I've not found any bugs in the LMDB patch set. It has not changed
since the re-order you requested last week.

What I have done is address yet another serious locking bug. Indeed
rare but existing locking bugs have taken up most of the time for this
project!

Perhaps it happens more often with this patch, or perhaps it is just
like the branch point before Samba 4.8 where flapping tests came out of
nowhere. Either way, what I've done is find and fix it.

(Gary did his due testing the entire time he was testing, and when he
first proposed it, and they didn't fail then).

Post by Stefan Metzmacher via samba-technical
The branch point for 4.9.0rc1
won't happen in near future.

I really, really don't want to leave this any longer. I've got a set
of patches that pass the Gitlab CI. They also show good results on
Catalyst Cloud autobuilds, results being:

lmdb-pre-7:
- 2 success
- nt4_dc notify

abartlet-lmdb-reordered-2:
- 3 success
- samba4.rpc.altercontext on ncalrpc with seal,padcheck.altercontext

dsheuristics-userpassword:
- 2 success

I've got 7 more builds of abartlet-lmdb-reordered-2 running now to be
sure. Once I get those back, I really want to land this.

For a number of reasons, I can't keep ploughing time into this.

That said, I realise you have put this aside into the 'don't need to
look at right now' category, but I've addressed the concerns you raised
and I do think anything else can be done after it lands in master.

All I'm asking is that you allow me to get review from other team
members.

Thanks,

Andrew Bartlett

[Andrew Bartlett via samba-technical]

Post by Andrew Bartlett via samba-technical
On Wed, 2018-04-11 at 14:15 +0200, Stefan Metzmacher via samba-

Post by Stefan Metzmacher via samba-technical
I'd like to have a look, but that might take a few days.

Can you please be specific? Last week you said you would have feedback
for me in a day, and it has been over a week.

Post by Stefan Metzmacher via samba-technical
And as you seem to still find bugs, I don't understand why this needs
to get to master as soon as possible.

I've not found any bugs in the LMDB patch set. It has not changed
since the re-order you requested last week.
What I have done is address yet another serious locking bug. Indeed
rare but existing locking bugs have taken up most of the time for this
project!
Perhaps it happens more often with this patch, or perhaps it is just
like the branch point before Samba 4.8 where flapping tests came out of
nowhere. Either way, what I've done is find and fix it.
(Gary did his due testing the entire time he was testing, and when he
first proposed it, and they didn't fail then).

Post by Stefan Metzmacher via samba-technical
The branch point for 4.9.0rc1
won't happen in near future.

I really, really don't want to leave this any longer. I've got a set
of patches that pass the Gitlab CI. They also show good results on
- 2 success
- nt4_dc notify
- 3 success
- samba4.rpc.altercontext on ncalrpc with seal,padcheck.altercontext
- 2 success
I've got 7 more builds of abartlet-lmdb-reordered-2 running now to be
sure. Once I get those back, I really want to land this.

The status on those were:
- 5 success
- samba3.smb2.lease(nt4_dc)
- samba.tests.blackbox.samba_dnsupdate(chgdcpass:local)

Therefore I think this is ready, as far as testing shows. Frankly I
think I've probably fixed some of our long-standing flapping issues in
the AD DC, but only time will really tell on that.

Post by Andrew Bartlett via samba-technical
For a number of reasons, I can't keep ploughing time into this.
That said, I realise you have put this aside into the 'don't need to
look at right now' category, but I've addressed the concerns you raised
and I do think anything else can be done after it lands in master.
All I'm asking is that you allow me to get review from other team
members.
Thanks,
Andrew Bartlett

[Stefan Metzmacher via samba-technical]

Hi Andrew,

Post by Andrew Bartlett via samba-technical
On Wed, 2018-04-11 at 14:15 +0200, Stefan Metzmacher via samba-

Post by Stefan Metzmacher via samba-technical
I'd like to have a look, but that might take a few days.

Can you please be specific? Last week you said you would have feedback
for me in a day, and it has been over a week.

I looked at it today.

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
And as you seem to still find bugs, I don't understand why this needs
to get to master as soon as possible.

I've not found any bugs in the LMDB patch set. It has not changed
since the re-order you requested last week.
What I have done is address yet another serious locking bug. Indeed
rare but existing locking bugs have taken up most of the time for this
project!

As always:-)

Post by Andrew Bartlett via samba-technical
Perhaps it happens more often with this patch, or perhaps it is just
like the branch point before Samba 4.8 where flapping tests came out of
nowhere. Either way, what I've done is find and fix it.
(Gary did his due testing the entire time he was testing, and when he
first proposed it, and they didn't fail then).

Post by Stefan Metzmacher via samba-technical
The branch point for 4.9.0rc1
won't happen in near future.

I really, really don't want to leave this any longer. I've got a set
of patches that pass the Gitlab CI. They also show good results on

I reordered the patchset a bit further:

The patches in
https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-lmdb-pre1
should be fine for master if they pass autobuild.

https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-lmdb-pre2

Contains the test_transaction_start_across_fork and
test_lock_read_across_fork tests, but it tries to run them
also with tdb.

I think it's very important to have the same external behavior between
tdb and lmdb. We should also protect against a fork between
transaction_start and transaction_[prepare]_commit. Maybe even more
situations.

This should then pass a build without having lmdb.h on the system.

https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-lmdb-full

Contains the rest with some build fixes, whitespace cleanups
and some more TODO markers in commit messages.

I think the ldb_mdb/* related fixes should be squashed all into the
initial commit (maybe with an expection for tests, which can follow)
otherwise it's really confusing to see the work in progress history.

Is it correct that we'll use lmdb as default for the sam.ldb and
sam.ldb.d/* databases? Or will all ldb files use it?
From reading the changes it's not obvious where the switch to "ldb://"
is done for the creation process of databases.

I was also not able to understand if the getpid() based detection for
the fork case correct. What is the correct way for lmdb to cleanup
after fork? Is close(fd) in lmdb_pvt_destructor() really the only
thing? Doesn't lmdb has other state, which we will leak?

Thanks for your patience, but there's still a bit of work required,
sorry! But given that will be the core of our AD database careful review
is required.

metze

[Andrew Bartlett via samba-technical]

Thanks Metze for getting back to me.

I'll look over this more after I get to the office, but some initial

Post by Stefan Metzmacher via samba-technical
I think the ldb_mdb/* related fixes should be squashed all into the
initial commit (maybe with an expection for tests, which can follow)
otherwise it's really confusing to see the work in progress history.

OK. The reason these were left out was that they are significant
points, subtle things that seemed better to expand on than hide into a
'too large to really review/follow' commit. So it wasn't a mistake,
but of course it can be done.

Post by Stefan Metzmacher via samba-technical
Is it correct that we'll use lmdb as default for the sam.ldb and
sam.ldb.d/* databases? Or will all ldb files use it?
From reading the changes it's not obvious where the switch to "ldb://"
is done for the creation process of databases.

Correct, the sam.ldb is still TDB, and will likely stay that way until
this is all combined into one environment. This allows a number of
things, in particular it ensures we don't break LMDB databases
trivially by having the main Samba tool open them as TDB.

The ldb:// thing is needed for the tests that poke into the backend,
and when setting up the backend databases in provision.

Post by Stefan Metzmacher via samba-technical
I was also not able to understand if the getpid() based detection for
the fork case correct. What is the correct way for lmdb to cleanup
after fork? Is close(fd) in lmdb_pvt_destructor() really the only
thing? Doesn't lmdb has other state, which we will leak?

It seems so, and there isn't much we can do. If you think about a
fork() based memory model then cleaning up the internal state will only
dirty memory anyway. They don't provide a 'clean up after fork()'
function, but do strictly require that you not touch an environment
after a fork().

Andrew Bartlett

[Stefan Metzmacher via samba-technical]

Post by Andrew Bartlett via samba-technical
Thanks Metze for getting back to me.
I'll look over this more after I get to the office, but some initial

Post by Stefan Metzmacher via samba-technical
I think the ldb_mdb/* related fixes should be squashed all into the
initial commit (maybe with an expection for tests, which can follow)
otherwise it's really confusing to see the work in progress history.

OK. The reason these were left out was that they are significant
points, subtle things that seemed better to expand on than hide into a
'too large to really review/follow' commit. So it wasn't a mistake,
but of course it can be done.

I think these important points could be better comments in the code.

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
Is it correct that we'll use lmdb as default for the sam.ldb and
sam.ldb.d/* databases? Or will all ldb files use it?
From reading the changes it's not obvious where the switch to "ldb://"
is done for the creation process of databases.

Correct, the sam.ldb is still TDB, and will likely stay that way until
this is all combined into one environment. This allows a number of
things, in particular it ensures we don't break LMDB databases
trivially by having the main Samba tool open them as TDB.
The ldb:// thing is needed for the tests that poke into the backend,
and when setting up the backend databases in provision.

So, there will be provision command line option to specifiy mdb for
the sam.ldb.d/* partition databases?

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
I was also not able to understand if the getpid() based detection for
the fork case correct. What is the correct way for lmdb to cleanup
after fork? Is close(fd) in lmdb_pvt_destructor() really the only
thing? Doesn't lmdb has other state, which we will leak?

It seems so, and there isn't much we can do. If you think about a
fork() based memory model then cleaning up the internal state will only
dirty memory anyway. They don't provide a 'clean up after fork()'
function, but do strictly require that you not touch an environment
after a fork().

From reading the code of mdb_env_close() I don't see why we shouldn't
be able to call it after fork. What we may need is a pthread_at_fork()
handler to make sure we don't work in the middle of a transaction.

metze

[Andrew Bartlett via samba-technical]

Post by Stefan Metzmacher via samba-technical

Post by Andrew Bartlett via samba-technical
Thanks Metze for getting back to me.
I'll look over this more after I get to the office, but some initial

Post by Stefan Metzmacher via samba-technical
I think the ldb_mdb/* related fixes should be squashed all into the
initial commit (maybe with an expection for tests, which can follow)
otherwise it's really confusing to see the work in progress history.

OK. The reason these were left out was that they are significant
points, subtle things that seemed better to expand on than hide into a
'too large to really review/follow' commit. So it wasn't a mistake,
but of course it can be done.

I think these important points could be better comments in the code.

OK

Post by Stefan Metzmacher via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
Is it correct that we'll use lmdb as default for the sam.ldb and
sam.ldb.d/* databases? Or will all ldb files use it?
From reading the changes it's not obvious where the switch to "ldb://"
is done for the creation process of databases.

Correct, the sam.ldb is still TDB, and will likely stay that way until
this is all combined into one environment. This allows a number of
things, in particular it ensures we don't break LMDB databases
trivially by having the main Samba tool open them as TDB.
The ldb:// thing is needed for the tests that poke into the backend,
and when setting up the backend databases in provision.

So, there will be provision command line option to specifiy mdb for
the sam.ldb.d/* partition databases?

There is (it already landed): --backend-store=[tdb|mdb]

Post by Stefan Metzmacher via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
I was also not able to understand if the getpid() based detection for
the fork case correct. What is the correct way for lmdb to cleanup
after fork? Is close(fd) in lmdb_pvt_destructor() really the only
thing? Doesn't lmdb has other state, which we will leak?

It seems so, and there isn't much we can do. If you think about a
fork() based memory model then cleaning up the internal state will only
dirty memory anyway. They don't provide a 'clean up after fork()'
function, but do strictly require that you not touch an environment
after a fork().

From reading the code of mdb_env_close() I don't see why we shouldn't
be able to call it after fork. What we may need is a pthread_at_fork()
handler to make sure we don't work in the middle of a transaction.

Given we are not going to bundle it, we were nervous about operating
outside their advice in this regard:

http://www.lmdb.tech/doc/

* Use an MDB_env* in the process which opened it, without fork()ing.

* Do not have open an LMDB database twice in the same process at the
same time. Not even from a plain open() call - close()ing it breaks
flock() advisory locking.

Indeed, perhaps it might be better not do do anything at all, even
'leaking' the FD: if a new LMDB is open then close() on the inherited
FD could destroy locks.

I hate POSIX locking.

Andrew Bartlett

[Volker Lendecke via samba-technical]

Post by Andrew Bartlett via samba-technical
It seems so, and there isn't much we can do. If you think about a
fork() based memory model then cleaning up the internal state will only
dirty memory anyway. They don't provide a 'clean up after fork()'
function, but do strictly require that you not touch an environment
after a fork().
After discussing this on -devel IRC, I think we could add an
mdb_env_postfork() call that the child can use to make its copy of
the env valid. It would have the same restrictions as
mdb_env_set_mapsize() - there must not be any active txns in the
parent process at the time of fork - they'd just be memleaks in the
child. The child must call this immediately, and cannot call any
other LMDB APIs until this is done. But assuming those conditions
are met, we can continue to use the existing file descriptors and
mmaps without needing to tear down and reopen.

Would that help us?

Volker

[Simo via samba-technical]

On Mon, 2018-04-16 at 14:36 +0200, Volker Lendecke via samba-technical

Post by Volker Lendecke via samba-technical

Post by Andrew Bartlett via samba-technical
It seems so, and there isn't much we can do. If you think about a
fork() based memory model then cleaning up the internal state will only
dirty memory anyway. They don't provide a 'clean up after fork()'
function, but do strictly require that you not touch an environment
after a fork().
After discussing this on -devel IRC, I think we could add an
mdb_env_postfork() call that the child can use to make its copy of
the env valid. It would have the same restrictions as
mdb_env_set_mapsize() - there must not be any active txns in the
parent process at the time of fork - they'd just be memleaks in the
child. The child must call this immediately, and cannot call any
other LMDB APIs until this is done. But assuming those conditions
are met, we can continue to use the existing file descriptors and
mmaps without needing to tear down and reopen.

Would that help us?

It would require to gate forks on the fact no transactions are open.
Can we guarantee no transactions in parents or block forks?

Note: If we need to fork a utility within the transaction we do not
need to gate it as long as we have call semantics that enforce an exec
or a panic I guess.

Simo.

[Howard Chu via samba-technical]

Post by Simo via samba-technical
On Mon, 2018-04-16 at 14:36 +0200, Volker Lendecke via samba-technical

Post by Volker Lendecke via samba-technical

Post by Andrew Bartlett via samba-technical
It seems so, and there isn't much we can do. If you think about a
fork() based memory model then cleaning up the internal state will only
dirty memory anyway. They don't provide a 'clean up after fork()'
function, but do strictly require that you not touch an environment
after a fork().
After discussing this on -devel IRC, I think we could add an
mdb_env_postfork() call that the child can use to make its copy of
the env valid. It would have the same restrictions as
mdb_env_set_mapsize() - there must not be any active txns in the
parent process at the time of fork - they'd just be memleaks in the
child. The child must call this immediately, and cannot call any
other LMDB APIs until this is done. But assuming those conditions
are met, we can continue to use the existing file descriptors and
mmaps without needing to tear down and reopen.

Would that help us?

It would require to gate forks on the fact no transactions are open.
Can we guarantee no transactions in parents or block forks?
Note: If we need to fork a utility within the transaction we do not
need to gate it as long as we have call semantics that enforce an exec
or a panic I guess.

Post by Volker Lendecke via samba-technical
One question that came up: What to do
with an open lmdb after fork? Samba does fork a lot, and with tdb we
just reopen the fd and restart everything from scratch. What's the
advice for lmdb?

Does the parent process continue to use the DB after the fork?
If the child is just going to exec, there's not much needed. You will leak a descriptor unless you close mdb_env_get_fd() before exec'ing.
If both processes are using the DB, you will need to env_close before forking and reopen in both processes. Otherwise you'll mess up the fcntl lock on the lockfile.
If you don't close in the parent, and only open in the child, things will work, but the child will leak the entire copy of the parent's environment.

I wrote the above before we had the discussion about adding a _postfork() API.
If you don't care about leaking the memory from any active transactions, you
could ignore that detail as well.

There's also the case where you fork and then the parent exits, but our
proposed _postfork() would handle that as well.

[Stefan Metzmacher via samba-technical]

Post by Howard Chu via samba-technical

Post by Simo via samba-technical
On Mon, 2018-04-16 at 14:36 +0200, Volker Lendecke via samba-technical

Post by Volker Lendecke via samba-technical

It seems so, and there isn't much we can do.  If you think about a
fork() based memory model then cleaning up the internal state will only
dirty memory anyway.  They don't provide a 'clean up after fork()'
function, but do strictly require that you not touch an environment
after a fork().
After discussing this on -devel IRC, I think we could add an
mdb_env_postfork() call that the child can use to make its copy of
the env valid. It would have the same restrictions as
mdb_env_set_mapsize() - there must not be any active txns in the
parent process at the time of fork - they'd just be memleaks in the
child. The child must call this immediately, and cannot call any
other LMDB APIs until this is done. But assuming those conditions
are met, we can continue to use the existing file descriptors and
mmaps without needing to tear down and reopen.

Would that help us?

It would require to gate forks on the fact no transactions are open.
Can we guarantee no transactions in parents or block forks?
Note: If we need to fork a utility within the transaction we do not
need to gate it as long as we have call semantics that enforce an exec
or a panic I guess.

Post by Volker Lendecke via samba-technical
One question that came up: What to do
with an open lmdb after fork? Samba does fork a lot, and with tdb we
just reopen the fd and restart everything from scratch. What's the
advice for lmdb?

Does the parent process continue to use the DB after the fork?
If the child is just going to exec, there's not much needed. You will
leak a descriptor unless you close mdb_env_get_fd() before exec'ing.

Why isn't *CLOEXEC used for env->me_fd and env->me_mfd ?
For me_fd we could set it ourself using mdb_env_get_fd(), but
me_mfd is completely hidden.

Post by Howard Chu via samba-technical

Post by Simo via samba-technical
If both processes are using the DB, you will need to env_close before
forking and reopen in both processes. Otherwise you'll mess up the
fcntl lock on the lockfile.

With tdb we just need to close and reopen in the child, why is that a
problem with fcntl locking?

Post by Howard Chu via samba-technical

Post by Simo via samba-technical
If you don't close in the parent, and only open in the child, things
will work, but the child will leak the entire copy of the parent's
environment.

Without a pending transaction I'd guess mdb_env_close() would be all we
need in the child. But what would be the problems with that?

Post by Howard Chu via samba-technical
I wrote the above before we had the discussion about adding a
_postfork() API. If you don't care about leaking the memory from any
active transactions, you could ignore that detail as well.
There's also the case where you fork and then the parent exits, but our
proposed _postfork() would handle that as well.

What would the _postfork() do in detail compaired to mdb_env_close()?

metze

[Howard Chu via samba-technical]

Post by Stefan Metzmacher via samba-technical

Post by Howard Chu via samba-technical

Post by Simo via samba-technical
On Mon, 2018-04-16 at 14:36 +0200, Volker Lendecke via samba-technical

Post by Volker Lendecke via samba-technical

Post by Andrew Bartlett via samba-technical
It seems so, and there isn't much we can do. If you think about a
fork() based memory model then cleaning up the internal state will only
dirty memory anyway. They don't provide a 'clean up after fork()'
function, but do strictly require that you not touch an environment
after a fork().
After discussing this on -devel IRC, I think we could add an
mdb_env_postfork() call that the child can use to make its copy of
the env valid. It would have the same restrictions as
mdb_env_set_mapsize() - there must not be any active txns in the
parent process at the time of fork - they'd just be memleaks in the
child. The child must call this immediately, and cannot call any
other LMDB APIs until this is done. But assuming those conditions
are met, we can continue to use the existing file descriptors and
mmaps without needing to tear down and reopen.

Would that help us?

It would require to gate forks on the fact no transactions are open.
Can we guarantee no transactions in parents or block forks?
Note: If we need to fork a utility within the transaction we do not
need to gate it as long as we have call semantics that enforce an exec
or a panic I guess.

Post by Volker Lendecke via samba-technical
One question that came up: What to do
with an open lmdb after fork? Samba does fork a lot, and with tdb we
just reopen the fd and restart everything from scratch. What's the
advice for lmdb?

Does the parent process continue to use the DB after the fork?
If the child is just going to exec, there's not much needed. You will
leak a descriptor unless you close mdb_env_get_fd() before exec'ing.

Why isn't *CLOEXEC used for env->me_fd and env->me_mfd ?
For me_fd we could set it ourself using mdb_env_get_fd(), but
me_mfd is completely hidden.

CLOEXEC is set on me_mfd already. It is deliberately not set on me_fd since
that's been exposed for users to manipulate.

Post by Stefan Metzmacher via samba-technical

Post by Howard Chu via samba-technical

Post by Simo via samba-technical
If both processes are using the DB, you will need to env_close before
forking and reopen in both processes. Otherwise you'll mess up the
fcntl lock on the lockfile.

With tdb we just need to close and reopen in the child, why is that a
problem with fcntl locking?

I misspoke. You will mess up the lockfile, but not because of its fcntl lock.

Post by Stefan Metzmacher via samba-technical

Post by Howard Chu via samba-technical

Post by Simo via samba-technical
If you don't close in the parent, and only open in the child, things
will work, but the child will leak the entire copy of the parent's
environment.

Without a pending transaction I'd guess mdb_env_close() would be all we
need in the child. But what would be the problems with that?

mdb_env_close() will reset any reader slots owned by the process. The PID is
stored in env->me_pid so unfortunately both parent and child will have the
same PID recorded here after a fork. Thus env_close in the child would end up
releasing the parent's reader slots. Fairly disastrous. Obviously we can fix
this trivially by just using getpid() in env_close().

Post by Stefan Metzmacher via samba-technical

Post by Howard Chu via samba-technical
I wrote the above before we had the discussion about adding a
_postfork() API. If you don't care about leaking the memory from any
active transactions, you could ignore that detail as well.
There's also the case where you fork and then the parent exits, but our
proposed _postfork() would handle that as well.

What would the _postfork() do in detail compaired to mdb_env_close()?

Mainly it would reacquire the lockfile fcntl lock. There may be other cleanup
needed, depends on the platform and which flavor of mutexes was used. Just to
be clear, the only reason I suggested adding this _postfork() function is to
allow the child process to keep using the existing env. If you just want to
close the env() then there's really not much else needed.

[Andrew Bartlett via samba-technical]

Post by Howard Chu via samba-technical
mdb_env_close() will reset any reader slots owned by the process. The PID is
stored in env->me_pid so unfortunately both parent and child will have the
same PID recorded here after a fork. Thus env_close in the child would end up
releasing the parent's reader slots. Fairly disastrous. Obviously we can fix
this trivially by just using getpid() in env_close().

So, if after the fork(), we just ignore the old DB, except to close
mdb_env_get_fd() (which is not the lock file, and so has no fcntl
semantics) are we safe?

I know we will leak a little memory, but as we don't create a deep tree
(sometimes a wide one, but never more than about 3 deep), we can cope
with that.

Then, when the re-open behaviour becomes available in a release we can
consider using that.

Would this be safe? (It seems safe from our tests so far).

Thanks,

Andrew Bartlett

[Howard Chu via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Howard Chu via samba-technical
mdb_env_close() will reset any reader slots owned by the process. The PID is
stored in env->me_pid so unfortunately both parent and child will have the
same PID recorded here after a fork. Thus env_close in the child would end up
releasing the parent's reader slots. Fairly disastrous. Obviously we can fix
this trivially by just using getpid() in env_close().

So, if after the fork(), we just ignore the old DB, except to close
mdb_env_get_fd() (which is not the lock file, and so has no fcntl
semantics) are we safe?

Safe, yes. But you could also just merge in this patch now
http://www.openldap.org/devel/gitweb.cgi?p=openldap.git;a=tree;f=libraries/liblmdb;hb=3b01bbbc67389b63e6623da99d397283729e70e7

which makes mdb_env_close() safe, as mentioned above.

Post by Andrew Bartlett via samba-technical
I know we will leak a little memory, but as we don't create a deep tree
(sometimes a wide one, but never more than about 3 deep), we can cope
with that.
Then, when the re-open behaviour becomes available in a release we can
consider using that.
Would this be safe? (It seems safe from our tests so far).
Thanks,
Andrew Bartlett

[Andrew Bartlett via samba-technical]

Post by Howard Chu via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Howard Chu via samba-technical
mdb_env_close() will reset any reader slots owned by the process. The PID is
stored in env->me_pid so unfortunately both parent and child will have the
same PID recorded here after a fork. Thus env_close in the child would end up
releasing the parent's reader slots. Fairly disastrous. Obviously we can fix
this trivially by just using getpid() in env_close().

So, if after the fork(), we just ignore the old DB, except to close
mdb_env_get_fd() (which is not the lock file, and so has no fcntl
semantics) are we safe?

Safe, yes. But you could also just merge in this patch now
http://www.openldap.org/devel/gitweb.cgi?p=openldap.git;a=tree;f=libraries/liblmdb;hb=3b01bbbc67389b63e6623da99d397283729e70e7
which makes mdb_env_close() safe, as mentioned above.

OK. At this stage we were not planning on bundling lmdb into
third_party or vendoring it into Samba so we can't rely on recent
fixes, but once there is a released version with this in we can call
that within an #ifdef guard. (bonus points if some CPP macro declares
it safe).

A reopen would still be nice because then we could match tdb where
would not need a new FD and so can't run out of FDs or have files go
missing under our feet.

Thanks,

Andrew Bartlett

[Howard Chu via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Howard Chu via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Howard Chu via samba-technical
mdb_env_close() will reset any reader slots owned by the process. The PID is
stored in env->me_pid so unfortunately both parent and child will have the
same PID recorded here after a fork. Thus env_close in the child would end up
releasing the parent's reader slots. Fairly disastrous. Obviously we can fix
this trivially by just using getpid() in env_close().

So, if after the fork(), we just ignore the old DB, except to close
mdb_env_get_fd() (which is not the lock file, and so has no fcntl
semantics) are we safe?

Safe, yes. But you could also just merge in this patch now
http://www.openldap.org/devel/gitweb.cgi?p=openldap.git;a=tree;f=libraries/liblmdb;hb=3b01bbbc67389b63e6623da99d397283729e70e7
which makes mdb_env_close() safe, as mentioned above.

OK. At this stage we were not planning on bundling lmdb into
third_party or vendoring it into Samba so we can't rely on recent
fixes, but once there is a released version with this in we can call
that within an #ifdef guard. (bonus points if some CPP macro declares
it safe).

Next release probably in a couple weeks.

But not bundling makes no sense to me, you won't have MDB_VL32 support on
32bit builds. As I recall, 32bit support was the main reason we started this
conversation however-many-years ago.

Post by Andrew Bartlett via samba-technical
A reopen would still be nice because then we could match tdb where
would not need a new FD and so can't run out of FDs or have files go
missing under our feet.

OK, we can implement the mdb_env_postfork() API I described earlier.

Post by Andrew Bartlett via samba-technical
Thanks,
Andrew Bartlett

[Andrew Bartlett via samba-technical]

Post by Howard Chu via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Howard Chu via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Howard Chu via samba-technical
mdb_env_close() will reset any reader slots owned by the process. The PID is
stored in env->me_pid so unfortunately both parent and child will have the
same PID recorded here after a fork. Thus env_close in the child would end up
releasing the parent's reader slots. Fairly disastrous. Obviously we can fix
this trivially by just using getpid() in env_close().

So, if after the fork(), we just ignore the old DB, except to close
mdb_env_get_fd() (which is not the lock file, and so has no fcntl
semantics) are we safe?

Safe, yes. But you could also just merge in this patch now
http://www.openldap.org/devel/gitweb.cgi?p=openldap.git;a=tree;f=libraries/liblmdb;hb=3b01bbbc67389b63e6623da99d397283729e70e7
which makes mdb_env_close() safe, as mentioned above.

OK. At this stage we were not planning on bundling lmdb into
third_party or vendoring it into Samba so we can't rely on recent
fixes, but once there is a released version with this in we can call
that within an #ifdef guard. (bonus points if some CPP macro declares
it safe).

Next release probably in a couple weeks.
But not bundling makes no sense to me, you won't have MDB_VL32 support on
32bit builds. As I recall, 32bit support was the main reason we started this
conversation however-many-years ago.

OK. What happens if you build on 32 bit on the older versions?

In any case this is an AD DC only feature so far so I'm happy to
require that folks either have a modern lmdb or be on 64 bit. It will
be different if the file server side takes it on, but folks running an
AD DC on a 32 bit old enterprise linux are rare I think.

Post by Howard Chu via samba-technical

Post by Andrew Bartlett via samba-technical
A reopen would still be nice because then we could match tdb where
would not need a new FD and so can't run out of FDs or have files go
missing under our feet.

OK, we can implement the mdb_env_postfork() API I described earlier.

Thanks,

Andrew Bartlett

[Andrew Bartlett via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Howard Chu via samba-technical
Next release probably in a couple weeks.
But not bundling makes no sense to me, you won't have MDB_VL32 support on
32bit builds. As I recall, 32bit support was the main reason we started this
conversation however-many-years ago.

OK. What happens if you build on 32 bit on the older versions?

Then you're limited to a 2GB database size.

Thanks. That (oddly) isn't a concern for me, I'm really not worried
about large AD domains on 32 bit platforms.

Post by Andrew Bartlett via samba-technical
In any case this is an AD DC only feature so far so I'm happy to
require that folks either have a modern lmdb or be on 64 bit. It will
be different if the file server side takes it on, but folks running an
AD DC on a 32 bit old enterprise linux are rare I think.

It's not about new vs old version; MDB_VL32 is a non-default build flag. It's
been there for 3-4 years already. It remains non-default because supporting
larger than 32bit DBs on a 32bit machine is necessarily slower than native
32bit support.

OK. I know we asked for it but I'll leave that for someone who needs
it before I make things more complex.

Thanks,

Andrew Bartlett

[Andrew Bartlett via samba-technical]

Post by Stefan Metzmacher via samba-technical
Hi Andrew,

Post by Andrew Bartlett via samba-technical
On Wed, 2018-04-11 at 14:15 +0200, Stefan Metzmacher via samba-

Post by Stefan Metzmacher via samba-technical
I'd like to have a look, but that might take a few days.

Can you please be specific? Last week you said you would have feedback
for me in a day, and it has been over a week.

I looked at it today.

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
And as you seem to still find bugs, I don't understand why this needs
to get to master as soon as possible.

I've not found any bugs in the LMDB patch set. It has not changed
since the re-order you requested last week.
What I have done is address yet another serious locking bug. Indeed
rare but existing locking bugs have taken up most of the time for this
project!

As always:-)

Post by Andrew Bartlett via samba-technical
Perhaps it happens more often with this patch, or perhaps it is just
like the branch point before Samba 4.8 where flapping tests came out of
nowhere. Either way, what I've done is find and fix it.
(Gary did his due testing the entire time he was testing, and when he
first proposed it, and they didn't fail then).

Post by Stefan Metzmacher via samba-technical
The branch point for 4.9.0rc1
won't happen in near future.

I really, really don't want to leave this any longer. I've got a set
of patches that pass the Gitlab CI. They also show good results on

The patches in
https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-lmdb-pre1
should be fine for master if they pass autobuild.
https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-lmdb-pre2
Contains the test_transaction_start_across_fork and
test_lock_read_across_fork tests, but it tries to run them
also with tdb.
I think it's very important to have the same external behavior between
tdb and lmdb. We should also protect against a fork between
transaction_start and transaction_[prepare]_commit. Maybe even more
situations.
This should then pass a build without having lmdb.h on the system.
https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-lmdb-full
Contains the rest with some build fixes, whitespace cleanups
and some more TODO markers in commit messages.

I've pushed the first set.

ON the rest Most of those comments and fixes are fine, but just one of
those before I finish up tonight:

commit 61d58289826576c27e521d6f98e9c1bcea572441
Author: Gary Lockyer <***@catalyst.net.nz>
Date:   Tue Mar 6 15:30:10 2018 +1300

    SQ??? ldb_mdb/tests: Run api and index test also on lmdb WHY not
the same dict for all tests?  

The different index dict is deliberate, we don't want to leave the
traditional TDB without an index untested.

Otherwise, beyond how to handle the destructor after the fork(), I
generally agree. 

and I think leaking it is the safest option so far: we can't get more
than 1 leaked object and fd per actual DB per actual parent anyway.

Finally, on:
# we don't want to run ldb_lmdb_size_test

Correct, we don't want to create 4GB files on the disk-limited test
instances.

Gary can sort the rest out with you next week.

Thanks!

Andrew Bartlett

[Andrew Bartlett via samba-technical]

Post by Stefan Metzmacher via samba-technical
Hi Andrew,
I was also not able to understand if the getpid() based detection for
the fork case correct. What is the correct way for lmdb to cleanup
after fork? Is close(fd) in lmdb_pvt_destructor() really the only
thing? Doesn't lmdb has other state, which we will leak?
Thanks for your patience, but there's still a bit of work required,
sorry! But given that will be the core of our AD database careful review
is required.

Is it correct to say that as long as we have the same behaviour, that
is either allowing transparent (when not locked) use across a fork(),
or denying it for both tdb and mdb, that you are OK with the patches
now?

I'm happy with your squashes in general, except around the tests where
I need to look much more carefully at what is being done there (it was
fairly deliberately constructed, so I need to double-check that).  

This, and the fork() behaviour aside, are you happy with the tree
otherwise?

Thanks,

Andrew Bartlett

Post by Stefan Metzmacher via samba-technical
metze

[Stefan Metzmacher via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
Hi Andrew,
I was also not able to understand if the getpid() based detection for
the fork case correct. What is the correct way for lmdb to cleanup
after fork? Is close(fd) in lmdb_pvt_destructor() really the only
thing? Doesn't lmdb has other state, which we will leak?
Thanks for your patience, but there's still a bit of work required,
sorry! But given that will be the core of our AD database careful review
is required.

Is it correct to say that as long as we have the same behaviour, that
is either allowing transparent (when not locked) use across a fork(),
or denying it for both tdb and mdb, that you are OK with the patches
now?

Likely:-) But I would need to swap it into my brain again...

Post by Andrew Bartlett via samba-technical
I'm happy with your squashes in general, except around the tests where
I need to look much more carefully at what is being done there (it was
fairly deliberately constructed, so I need to double-check that).  

Please check it and maybe already do the squashes.

Post by Andrew Bartlett via samba-technical
This, and the fork() behaviour aside, are you happy with the tree
otherwise?

I've lost track, can you rebase the different stages on top of
master, so that I can see what's actually remaining.

I'll try to have another look when you give me the pointers to
rebased branches.

Thanks!
metze

[Andrew Bartlett via samba-technical]

Post by Stefan Metzmacher via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
Hi Andrew,
I was also not able to understand if the getpid() based detection for
the fork case correct. What is the correct way for lmdb to cleanup
after fork? Is close(fd) in lmdb_pvt_destructor() really the only
thing? Doesn't lmdb has other state, which we will leak?
Thanks for your patience, but there's still a bit of work required,
sorry! But given that will be the core of our AD database careful review
is required.

Is it correct to say that as long as we have the same behaviour, that
is either allowing transparent (when not locked) use across a fork(),
or denying it for both tdb and mdb, that you are OK with the patches
now?

Likely:-) But I would need to swap it into my brain again...

Post by Andrew Bartlett via samba-technical
I'm happy with your squashes in general, except around the tests where
I need to look much more carefully at what is being done there (it was
fairly deliberately constructed, so I need to double-check that).

Please check it and maybe already do the squashes.

Post by Andrew Bartlett via samba-technical
This, and the fork() behaviour aside, are you happy with the tree
otherwise?

I've lost track, can you rebase the different stages on top of
master, so that I can see what's actually remaining.
I'll try to have another look when you give me the pointers to
rebased branches.

Thanks. My plan for today (now that I'm past the flapping test) is to
do exactly that, there just wasn't time once I got the LSA thing done.

Andrew Bartlett

[Andrew Bartlett via samba-technical]

On Fri, 2018-05-04 at 06:52 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
I'll try to have another look when you give me the pointers to
rebased branches.

Thanks. My plan for today (now that I'm past the flapping test) is to
do exactly that, there just wasn't time once I got the LSA thing done.

I've finally got a set of patches I'm happy with here:

https://gitlab.com/catalyst-samba/samba/commits/metze-master4-lmdb-full

Thanks,

Andrew Bartlett

[Andrew Bartlett via samba-technical]

Post by Andrew Bartlett via samba-technical
On Fri, 2018-05-04 at 06:52 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
I'll try to have another look when you give me the pointers to
rebased branches.

Thanks. My plan for today (now that I'm past the flapping test) is to
do exactly that, there just wasn't time once I got the LSA thing done.

https://gitlab.com/catalyst-samba/samba/commits/metze-master4-lmdb-full

These have passed a full autobuild in the gitlab CI here:

https://gitlab.com/catalyst-samba/samba/pipelines/21509189
(for the previous set) and here:

https://gitlab.com/catalyst-samba/samba/pipelines/21600415
(for the current set)

The diff against a (very rough) rebase of the previous patches I posted
is attached, just to highlight the areas of change. (That is, don't
worry about the 'wrong' stuff in the ldb_ldb stuff, it is right in the
actual patches).

Every patch passes a full make test in lib/ldb.


Gary/Garming,

Can you confirm you are happy with the review tags and review my extra
tests?

Metze,

I've addressed your concerns as far as practical[1]. If you could
please allow this to proceed into master I would most appreciate it.

Thanks,

Andrew Bartlett

[1] Moving some of the tests to ldb_lmdb_test (in a TODO commit) didn't
compile and was not practical to fix due to dependencies, so I've left
those in ldb_mod_op_test.

[Stefan Metzmacher via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
On Fri, 2018-05-04 at 06:52 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
I'll try to have another look when you give me the pointers to
rebased branches.

Thanks. My plan for today (now that I'm past the flapping test) is to
do exactly that, there just wasn't time once I got the LSA thing done.

https://gitlab.com/catalyst-samba/samba/commits/metze-master4-lmdb-full

https://gitlab.com/catalyst-samba/samba/pipelines/21509189
https://gitlab.com/catalyst-samba/samba/pipelines/21600415
(for the current set)
The diff against a (very rough) rebase of the previous patches I posted
is attached, just to highlight the areas of change. (That is, don't
worry about the 'wrong' stuff in the ldb_ldb stuff, it is right in the
actual patches).
Every patch passes a full make test in lib/ldb.
Gary/Garming,
Can you confirm you are happy with the review tags and review my extra
tests?
Metze,
I've addressed your concerns as far as practical[1]. If you could
please allow this to proceed into master I would most appreciate it.

Can you move all tdb related patch to the start of the patchset,
they should work on their own. Also the fork protection tests.

I don't see where ldb_tdb_test is used in tests...

From reading the patches it seems that it will no longer possible
to build ldb or an AD DC without lmdb, e.g. 32bit support will be gone.
I don't think that's acceptable right now, maybe in a few years.

metze

[Rowland Penny via samba-technical]

On Mon, 7 May 2018 09:48:22 -0700

On Mon, May 07, 2018 at 05:45:40PM +0100, Rowland Penny via

On Mon, 7 May 2018 09:35:54 -0700

On Mon, May 07, 2018 at 05:57:35PM +0200, Stefan Metzmacher via

Post by Stefan Metzmacher via samba-technical
From reading the patches it seems that it will no longer
possible to build ldb or an AD DC without lmdb, e.g. 32bit
support will be gone. I don't think that's acceptable right
now, maybe in a few years.

Do we really have evidence of users/OEMs using 32-bit AD-DC's ?
I can see needing 32-bit for embedded small file servers, but
AD-DC's ?
Just curious, I don't mind either way, but I thought the 32->64
bit transition happened in the 2000 -> 2010 timeframe. I'd kind
of assumed we were mostly done with it.
Jeremy.

There are quite a few RaspberryPi AD DCs out there ;-)

Ah, that's what I'm missing. I assumed that even RaspberryPi's
were 64-bit these days (don't use 'em myself :-).

The latest ones potentially are, but raspbian is still 32bit, perhaps
Samba going 64bit only might make them think again ;-)

Rowland

[Andrew Bartlett via samba-technical]

If it would be optional for sometime and it would actually work
it would be much easier to agree to push it to master.

I've made it optional. Builds of ldb and Samba have a --without-ldb-
lmdb option. Builds --without-ad-dc do not enable lmdb, and only AD DC
sefltest builds require it.

Otherwise, the only other restriction is that 'make test' on ldb is
incompatible with --without-ldb-lmdb. This pre-empts the 'tests gone
missing' issue. Both the python and the cmocka tests require lmdb, so
it is clearer to just require it.

The ad_dc and vampire_dc environments in Samba's selftest now use 
--backend-store=mdb

autobuild confirms that ldb will compile --without-ldb-lmdb

If an AD DC build is made against a system ldb without lmdb support,
some tests will fail.

https://gitlab.com/catalyst-samba/samba/pipelines/21959419
https://gitlab.com/catalyst-samba/samba/commits/metze-master4-lmdb-full

Finally, an ldb make test passes on all the ldb_mdb commits.

Please review and push ;-)

Thanks,

Andrew Bartlett

[Stefan Metzmacher via samba-technical]

Post by Andrew Bartlett via samba-technical

If it would be optional for sometime and it would actually work
it would be much easier to agree to push it to master.

I've made it optional. Builds of ldb and Samba have a --without-ldb-
lmdb option. Builds --without-ad-dc do not enable lmdb, and only AD DC
sefltest builds require it.
Otherwise, the only other restriction is that 'make test' on ldb is
incompatible with --without-ldb-lmdb. This pre-empts the 'tests gone
missing' issue. Both the python and the cmocka tests require lmdb, so
it is clearer to just require it.
The ad_dc and vampire_dc environments in Samba's selftest now use 
--backend-store=mdb
autobuild confirms that ldb will compile --without-ldb-lmdb
If an AD DC build is made against a system ldb without lmdb support,
some tests will fail.
https://gitlab.com/catalyst-samba/samba/pipelines/21959419
https://gitlab.com/catalyst-samba/samba/commits/metze-master4-lmdb-full
Finally, an ldb make test passes on all the ldb_mdb commits.
Please review and push ;-)

Thanks! I'll have a look later.

In the meantime can you please try a private autobuild on sn-devel-144?

metze

[Andrew Bartlett via samba-technical]

On Mon, 2018-05-14 at 08:02 +0200, Stefan Metzmacher via samba-

Post by Stefan Metzmacher via samba-technical

Post by Andrew Bartlett via samba-technical

If it would be optional for sometime and it would actually work
it would be much easier to agree to push it to master.

I've made it optional. Builds of ldb and Samba have a --without-ldb-
lmdb option. Builds --without-ad-dc do not enable lmdb, and only AD DC
sefltest builds require it.
Otherwise, the only other restriction is that 'make test' on ldb is
incompatible with --without-ldb-lmdb. This pre-empts the 'tests gone
missing' issue. Both the python and the cmocka tests require lmdb, so
it is clearer to just require it.
The ad_dc and vampire_dc environments in Samba's selftest now use 
--backend-store=mdb
autobuild confirms that ldb will compile --without-ldb-lmdb
If an AD DC build is made against a system ldb without lmdb support,
some tests will fail.
https://gitlab.com/catalyst-samba/samba/pipelines/21959419
https://gitlab.com/catalyst-samba/samba/commits/metze-master4-lmdb-full
Finally, an ldb make test passes on all the ldb_mdb commits.
Please review and push ;-)

Thanks! I'll have a look later.

I've reworked the patches to hook this up to the join etc, and the new
pipeline is here:

https://gitlab.com/catalyst-samba/samba/pipelines/22016682

Post by Stefan Metzmacher via samba-technical
In the meantime can you please try a private autobuild on sn-devel-144?

I ran one, it may be due to the number of other autobuilds being run by
the same user as the ldb tests passed previously but failed now.  

(So while they all failed, some failed with a different flapping test,
well past the ldb tests).

Are there unusual limits on mmap() areas on that host?

In the meantime I've updated the error message and dropped the size to
8GB for now. Having this be a relatively fixed number just for sn-
devel is not ideal however.

Thanks,

Andrew Bartlett

[Stefan Metzmacher via samba-technical]

Post by Andrew Bartlett via samba-technical
On Mon, 2018-05-14 at 08:02 +0200, Stefan Metzmacher via samba-

Post by Stefan Metzmacher via samba-technical

Post by Andrew Bartlett via samba-technical

If it would be optional for sometime and it would actually work
it would be much easier to agree to push it to master.

I've made it optional. Builds of ldb and Samba have a --without-ldb-
lmdb option. Builds --without-ad-dc do not enable lmdb, and only AD DC
sefltest builds require it.
Otherwise, the only other restriction is that 'make test' on ldb is
incompatible with --without-ldb-lmdb. This pre-empts the 'tests gone
missing' issue. Both the python and the cmocka tests require lmdb, so
it is clearer to just require it.
The ad_dc and vampire_dc environments in Samba's selftest now use 
--backend-store=mdb
autobuild confirms that ldb will compile --without-ldb-lmdb
If an AD DC build is made against a system ldb without lmdb support,
some tests will fail.
https://gitlab.com/catalyst-samba/samba/pipelines/21959419
https://gitlab.com/catalyst-samba/samba/commits/metze-master4-lmdb-full
Finally, an ldb make test passes on all the ldb_mdb commits.
Please review and push ;-)

Thanks! I'll have a look later.

I've reworked the patches to hook this up to the join etc, and the new
https://gitlab.com/catalyst-samba/samba/pipelines/22016682

Post by Stefan Metzmacher via samba-technical
In the meantime can you please try a private autobuild on sn-devel-144?

I ran one, it may be due to the number of other autobuilds being run by
the same user as the ldb tests passed previously but failed now.  
(So while they all failed, some failed with a different flapping test,
well past the ldb tests).
Are there unusual limits on mmap() areas on that host?
In the meantime I've updated the error message and dropped the size to
8GB for now. Having this be a relatively fixed number just for sn-
devel is not ideal however.

I guess we need to remove the ulimit -v.

On my system top shows this:

29037 metze 39 19 80,238g 109276 38468 S 0,3 0,7 0:22.95 samba

metze

[Stefan Metzmacher via samba-technical]

Post by Stefan Metzmacher via samba-technical

Post by Andrew Bartlett via samba-technical
On Mon, 2018-05-14 at 08:02 +0200, Stefan Metzmacher via samba-

Post by Stefan Metzmacher via samba-technical

Post by Andrew Bartlett via samba-technical

If it would be optional for sometime and it would actually work
it would be much easier to agree to push it to master.

I've made it optional. Builds of ldb and Samba have a --without-ldb-
lmdb option. Builds --without-ad-dc do not enable lmdb, and only AD DC
sefltest builds require it.
Otherwise, the only other restriction is that 'make test' on ldb is
incompatible with --without-ldb-lmdb. This pre-empts the 'tests gone
missing' issue. Both the python and the cmocka tests require lmdb, so
it is clearer to just require it.
The ad_dc and vampire_dc environments in Samba's selftest now use 
--backend-store=mdb
autobuild confirms that ldb will compile --without-ldb-lmdb
If an AD DC build is made against a system ldb without lmdb support,
some tests will fail.
https://gitlab.com/catalyst-samba/samba/pipelines/21959419
https://gitlab.com/catalyst-samba/samba/commits/metze-master4-lmdb-full
Finally, an ldb make test passes on all the ldb_mdb commits.
Please review and push ;-)

Thanks! I'll have a look later.

I've reworked the patches to hook this up to the join etc, and the new
https://gitlab.com/catalyst-samba/samba/pipelines/22016682

Post by Stefan Metzmacher via samba-technical
In the meantime can you please try a private autobuild on sn-devel-144?

I ran one, it may be due to the number of other autobuilds being run by
the same user as the ldb tests passed previously but failed now.  
(So while they all failed, some failed with a different flapping test,
well past the ldb tests).
Are there unusual limits on mmap() areas on that host?
In the meantime I've updated the error message and dropped the size to
8GB for now. Having this be a relatively fixed number just for sn-
devel is not ideal however.

I guess we need to remove the ulimit -v.
29037 metze 39 19 80,238g 109276 38468 S 0,3 0,7 0:22.95 samba

Here's a more complete list:

29052 metze 39 19 80,259g 102180 31780 S 0,0 0,6 0:01.12 smbd


29064 metze 39 19 0,157t 98896 18696 S 0,0 0,6 0:00.21
winbindd

28282 metze 39 19 0,157t 98232 18484 S 0,0 0,6 0:00.06
winbindd

29066 metze 39 19 0,157t 98016 17828 S 0,0 0,6 0:00.04
winbindd

28195 metze 39 19 0,157t 97808 17432 S 0,0 0,6 0:00.33
samba

28283 metze 39 19 0,157t 97740 18004 S 0,0 0,6 0:00.00
winbindd

28190 metze 39 19 0,157t 96156 15784 S 0,0 0,6 0:00.09
samba

29043 metze 39 19 80,218g 95448 25600 S 0,0 0,6 0:00.56
winbindd

29067 metze 39 19 0,157t 95428 14728 S 0,0 0,6 0:00.10 lpqd


28199 metze 39 19 0,157t 94676 14320 S 0,0 0,6 0:00.05
samba

28208 metze 39 19 80,217g 94628 25228 S 0,0 0,6 0:00.36
winbindd

28201 metze 39 19 0,157t 94112 13752 S 0,0 0,6 0:00.00
samba

29065 metze 39 19 0,157t 93752 13576 S 0,0 0,6 0:00.08
winbindd

28186 metze 39 19 80,226g 93744 23700 S 0,0 0,6 0:00.22
samba

28284 metze 39 19 0,157t 93052 13340 S 0,0 0,6 0:00.03
winbindd

28191 metze 39 19 0,157t 91832 11476 S 0,0 0,6 0:00.02
samba

28194 metze 39 19 0,157t 91148 10792 S 0,0 0,6 0:00.00 samba

metze

[Andrew Bartlett via samba-technical]

Post by Stefan Metzmacher via samba-technical

Post by Stefan Metzmacher via samba-technical
I guess we need to remove the ulimit -v.

Thanks, that would be most appreciated. Can you let me know when that
changes so I can re-try the build?

Post by Stefan Metzmacher via samba-technical

Post by Stefan Metzmacher via samba-technical
29037 metze 39 19 80,238g 109276 38468 S 0,3 0,7 0:22.95 samba

29052 metze 39 19 80,259g 102180 31780 S 0,0 0,6 0:01.12 smbd
29064 metze 39 19 0,157t 98896 18696 S 0,0 0,6 0:00.21
winbindd

These are likely larger as after a fork() we have no way to safely
destroy the old mmap()ed region (pending the yet to be written post-
fork close and reopen patches).

I hope this clarifies things,

Andrew Bartlett

[Volker Lendecke via samba-technical]

Post by Andrew Bartlett via samba-technical
These are likely larger as after a fork() we have no way to safely
destroy the old mmap()ed region (pending the yet to be written post-
fork close and reopen patches).

From my point of view we need to work towards a model where the raw
ldb access is done only in the LDAP server. Everything else goes via
the LDAP protocol, possibly via ldapi with PEERCRED checks. If no LDAP
server is around when we need it, we need to fork one on demand and
work through a parent<->child socket. This solves the memory
footprint and the asynchronosity problem. Transactions? Make them an
exop blocking everything else like it's done locally right now. That
makes it even easier to have a watchdog forcibly aborting blocked
processes with an open transaction.

I know this is not a simple task, but I think we need that in the long
run and I'd like to hear comments about it.

Thanks,

Volker

[Andrew Bartlett via samba-technical]

On Tue, 2018-05-15 at 22:17 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
I guess we need to remove the ulimit -v.

Thanks, that would be most appreciated. Can you let me know when that
changes so I can re-try the build?

The limits in the buildnice script seems to be the issue. I'm running
without those now to see how it goes.

Andrew Bartlett

[Andrew Bartlett via samba-technical]

Post by Andrew Bartlett via samba-technical
On Tue, 2018-05-15 at 22:17 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
I guess we need to remove the ulimit -v.

Thanks, that would be most appreciated. Can you let me know when that
changes so I can re-try the build?

The limits in the buildnice script seems to be the issue. I'm running
without those now to see how it goes.

Without the limits from buildnice, a full autobuild on sn-devel passed.

Metze, Can you please indicate what your need next to approve this?

Gary/Garming, can you look over the tweaked branch again and indicate
if you are still happy with it?

Thanks,

Andrew Bartlett

[Stefan Metzmacher via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
On Tue, 2018-05-15 at 22:17 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
I guess we need to remove the ulimit -v.

Thanks, that would be most appreciated. Can you let me know when that
changes so I can re-try the build?

The limits in the buildnice script seems to be the issue. I'm running
without those now to see how it goes.

Without the limits from buildnice, a full autobuild on sn-devel passed.
Metze, Can you please indicate what your need next to approve this?

I'm also running a few private autobuilds now...

The main thing seems to be 32bit builds. We should be just
make this a 64bit only thing at configure time in order to
simplify things?

And I guess we need to set the close on exec bit on the file handle
after open.

metze

[Stefan Metzmacher via samba-technical]

Hi,

Post by Andrew Bartlett via samba-technical
Without the limits from buildnice, a full autobuild on sn-devel passed.
Metze, Can you please indicate what your need next to approve this?

Do you actually have some performance numbers for the new code?

metze

[Andrew Bartlett via samba-technical]

On Wed, 2018-05-16 at 17:09 +0200, Stefan Metzmacher via samba-

Post by Stefan Metzmacher via samba-technical
Hi,

Post by Andrew Bartlett via samba-technical
Without the limits from buildnice, a full autobuild on sn-devel passed.
Metze, Can you please indicate what your need next to approve this?

Do you actually have some performance numbers for the new code?

I've asked Joe to run the perf tool on it, as he as set up an automated
rig and did the same for Tim's PSO patches just yesterday.

Earlier results (during the main development) didn't show a massive

change either way after eliminating a high overhead for socket-wrapper.

While performance is important, the key requirement was from customers
who didn't want their DB to freeze up totally with no practical way out
if they got anywhere near 4GB per TDB.

Andrew Bartlett

[Andrew Bartlett via samba-technical]

On Wed, 2018-05-16 at 17:03 +0200, Stefan Metzmacher via samba-

Post by Stefan Metzmacher via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
On Tue, 2018-05-15 at 22:17 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical
I guess we need to remove the ulimit -v.

Thanks, that would be most appreciated. Can you let me know when that
changes so I can re-try the build?

The limits in the buildnice script seems to be the issue. I'm running
without those now to see how it goes.

Without the limits from buildnice, a full autobuild on sn-devel passed.
Metze, Can you please indicate what your need next to approve this?

I'm also running a few private autobuilds now...
The main thing seems to be 32bit builds. We should be just
make this a 64bit only thing at configure time in order to
simplify things?

OK. Selftest of the AD DC will require a 64-bit system, which is OK.

Later, when we get the reopen functionality we can enable it but with a512MB DB size or some dynamic system.

Post by Stefan Metzmacher via samba-technical
And I guess we need to set the close on exec bit on the file handle
after open.

I'll do that. Thanks for reminding me.

Andrew Bartlett

[Stefan Metzmacher via samba-technical]

But while writing this the first already failed
the ldb standalone tests with similar failures
Could not run the test - check test fixtures
[  ERROR   ] test_transaction_commit_across_fork
Failed to connect to 'mdb://apitest.ldb' with backend 'mdb': Could not
create MDB environment apitest.ldb: Accessing a corrupted shared library
Could not run the test - check test fixtures
[  ERROR   ] test_lock_read_across_fork
Failed to connect to 'mdb://apitest.ldb' with backend 'mdb': Could not
create MDB environment apitest.ldb: Accessing a corrupted shared library

That is very interesting, and I agree a little worrying.
The full autobuild passed on that set, indeed part of why the 'just re-
order it' required so much work is that the full LDB 'make test' is run
and passes against every commit (on my workstation, Debian 9.4 stable).
https://gitlab.com/catalyst-samba/samba/pipelines/21724895
Likewise ldb's make test passes on sn-devel and you have no doubt seen
Denis Cardon's successful report back.
Can you check your system please and work out what is different?

This was on sn-devel-144 that's why I'm so confused.

metze

[Stefan Metzmacher via samba-technical]

Post by Stefan Metzmacher via samba-technical

But while writing this the first already failed
the ldb standalone tests with similar failures
Could not run the test - check test fixtures
[  ERROR   ] test_transaction_commit_across_fork
Failed to connect to 'mdb://apitest.ldb' with backend 'mdb': Could not
create MDB environment apitest.ldb: Accessing a corrupted shared library
Could not run the test - check test fixtures
[  ERROR   ] test_lock_read_across_fork
Failed to connect to 'mdb://apitest.ldb' with backend 'mdb': Could not
create MDB environment apitest.ldb: Accessing a corrupted shared library

That is very interesting, and I agree a little worrying.
The full autobuild passed on that set, indeed part of why the 'just re-
order it' required so much work is that the full LDB 'make test' is run
and passes against every commit (on my workstation, Debian 9.4 stable).
https://gitlab.com/catalyst-samba/samba/pipelines/21724895
Likewise ldb's make test passes on sn-devel and you have no doubt seen
Denis Cardon's successful report back.
Can you check your system please and work out what is different?

This was on sn-devel-144 that's why I'm so confused.

Ok, it's ulimit -v 8000000, e.g. ldb_lmdb_test only starts working
reliable with ulimit -v 40000000.

Otherwise the mmap() with size 17179869184 (16G) fails.

metze

[Douglas Bagnall via samba-technical]

Post by Andrew Bartlett via samba-technical
I've seen this on a master build, so it is just happening without these
patches, but I also have a patch to address the issue by locking the
init() hook.
https://gitlab.com/catalyst-samba/samba/commits/lmdb-pre-7

I have reviewed these ones and they LGTM.

Douglas

[Andrew Bartlett via samba-technical]

Post by Douglas Bagnall via samba-technical

Post by Andrew Bartlett via samba-technical
I've seen this on a master build, so it is just happening without these
patches, but I also have a patch to address the issue by locking the
init() hook.
https://gitlab.com/catalyst-samba/samba/commits/lmdb-pre-7

I have reviewed these ones and they LGTM.

Thanks! It will be good to get these in.  

I'll autobuild these later today.

Andrew Bartlett

[Andrew Bartlett via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Douglas Bagnall via samba-technical

Post by Andrew Bartlett via samba-technical
I've seen this on a master build, so it is just happening without these
patches, but I also have a patch to address the issue by locking the
init() hook.
https://gitlab.com/catalyst-samba/samba/commits/lmdb-pre-7

I have reviewed these ones and they LGTM.

Thanks! It will be good to get these in.
I'll autobuild these later today.
[133(711)/525 at 10m56s] samba3.rpc.lsa.lookupsids(ad_dc)
smbtorture 4.9.0pre1-DEVELOPERBUILD
Using seed 1523610756
UNEXPECTED(failure): samba3.rpc.lsa.lookupsids.lsa.LookupSidsReply(ad_dc)
REASON: Exception: Exception: ../source4/torture/rpc/lsa_lookup.c:400: names.names[0].name.string was , expected S-1-5-21-1111111111-2222222222-3333333333-512: unexpected names[0].string
FAILED (1 failures, 0 errors and 0 unexpected successes in 0 testsuites)

And I mentioned before that I got one LSA failure on the branch up to:

commit cb607346d3c7c662343b0eae69e43eaa6358c188
Author: Gary Lockyer <***@catalyst.net.nz>
Date: Tue Mar 13 16:43:54 2018 +1300

ldb-samba: require pid match for cached ldb

Post by Andrew Bartlett via samba-technical
Testing OpenPolicy2
UNEXPECTED(failure): samba4.rpc.altercontext on ncalrpc with seal,padcheck.altercontext(ad_dc_ntvfs:local)
REASON: Exception: Exception: ../source4/torture/rpc/lsa.c:188: status was NT_STATUS_CONNECTION_RESET, expected NT_STATUS_CONNECTION_DISCONNECTED: OpenPolicy2 failed

(but wrote it off as I also got about 10 successes on branches with
that series in it).

Post by Andrew Bartlett via samba-technical
Testing LookupSids
ndr_pull_error(1): Bad array size - got 0 expected 8
UNEXPECTED(failure): samba3.rpc.lsa.privileges.lsa.Privileges(ad_dc)
REASON: Exception: Exception: ../source4/torture/rpc/lsa.c:774: dcerpc_lsa_LookupSids_r(b, tctx, &r) was NT_STATUS_ARRAY_BOUNDS_EXCEEDED, expected NT_STATUS_OK: LookupSids failed
FAILED (1 failures, 0 errors and 0 unexpected successes in 0 testsuites)

If anybody has any insights or suggestions please don't hesitate to
investigate.

Thanks,

Andrew Bartlett

[Andrew Bartlett via samba-technical]

On Sat, 2018-04-14 at 07:07 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
[133(711)/525 at 10m56s] samba3.rpc.lsa.lookupsids(ad_dc)
smbtorture 4.9.0pre1-DEVELOPERBUILD
Using seed 1523610756
UNEXPECTED(failure): samba3.rpc.lsa.lookupsids.lsa.LookupSidsReply(ad_dc)
REASON: Exception: Exception: ../source4/torture/rpc/lsa_lookup.c:400: names.names[0].name.string was , expected S-1-5-21-1111111111-2222222222-3333333333-512: unexpected names[0].string
FAILED (1 failures, 0 errors and 0 unexpected successes in 0 testsuites)

commit cb607346d3c7c662343b0eae69e43eaa6358c188
Date: Tue Mar 13 16:43:54 2018 +1300
ldb-samba: require pid match for cached ldb

Post by Andrew Bartlett via samba-technical
Testing OpenPolicy2
UNEXPECTED(failure): samba4.rpc.altercontext on ncalrpc with seal,padcheck.altercontext(ad_dc_ntvfs:local)
REASON: Exception: Exception: ../source4/torture/rpc/lsa.c:188: status was NT_STATUS_CONNECTION_RESET, expected NT_STATUS_CONNECTION_DISCONNECTED: OpenPolicy2 failed

(but wrote it off as I also got about 10 successes on branches with
that series in it).

Post by Andrew Bartlett via samba-technical
Testing LookupSids
ndr_pull_error(1): Bad array size - got 0 expected 8
UNEXPECTED(failure): samba3.rpc.lsa.privileges.lsa.Privileges(ad_dc)
REASON: Exception: Exception: ../source4/torture/rpc/lsa.c:774: dcerpc_lsa_LookupSids_r(b, tctx, &r) was NT_STATUS_ARRAY_BOUNDS_EXCEEDED, expected NT_STATUS_OK: LookupSids failed
FAILED (1 failures, 0 errors and 0 unexpected successes in 0 testsuites)

If anybody has any insights or suggestions please don't hesitate to
investigate.

It turned out to be an unrelated use-after-free in the LSA server after
the trusts changes recently.

The issue was found fairly easily with address-sanitizer and is fixed
in the attached. This needs to be in 4.8.2 the regression was shipped
with 4.8.0.

Andrew Bartlett

[Andrew Bartlett via samba-technical]

On Thu, 2018-05-03 at 16:29 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
It turned out to be an unrelated use-after-free in the LSA server after
the trusts changes recently.
The issue was found fairly easily with address-sanitizer and is fixed
in the attached. This needs to be in 4.8.2 the regression was shipped
with 4.8.0.

Douglas has reviewed it and this is in autoubuild.

Thanks,

Andrew Bartlett

[Stefan Metzmacher via samba-technical]

Post by Andrew Bartlett via samba-technical
On Sat, 2018-04-14 at 07:07 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
[133(711)/525 at 10m56s] samba3.rpc.lsa.lookupsids(ad_dc)
smbtorture 4.9.0pre1-DEVELOPERBUILD
Using seed 1523610756
UNEXPECTED(failure): samba3.rpc.lsa.lookupsids.lsa.LookupSidsReply(ad_dc)
REASON: Exception: Exception: ../source4/torture/rpc/lsa_lookup.c:400: names.names[0].name.string was , expected S-1-5-21-1111111111-2222222222-3333333333-512: unexpected names[0].string
FAILED (1 failures, 0 errors and 0 unexpected successes in 0 testsuites)

commit cb607346d3c7c662343b0eae69e43eaa6358c188
Date: Tue Mar 13 16:43:54 2018 +1300
ldb-samba: require pid match for cached ldb

Post by Andrew Bartlett via samba-technical
Testing OpenPolicy2
UNEXPECTED(failure): samba4.rpc.altercontext on ncalrpc with seal,padcheck.altercontext(ad_dc_ntvfs:local)
REASON: Exception: Exception: ../source4/torture/rpc/lsa.c:188: status was NT_STATUS_CONNECTION_RESET, expected NT_STATUS_CONNECTION_DISCONNECTED: OpenPolicy2 failed

(but wrote it off as I also got about 10 successes on branches with
that series in it).

Post by Andrew Bartlett via samba-technical
Testing LookupSids
ndr_pull_error(1): Bad array size - got 0 expected 8
UNEXPECTED(failure): samba3.rpc.lsa.privileges.lsa.Privileges(ad_dc)
REASON: Exception: Exception: ../source4/torture/rpc/lsa.c:774: dcerpc_lsa_LookupSids_r(b, tctx, &r) was NT_STATUS_ARRAY_BOUNDS_EXCEEDED, expected NT_STATUS_OK: LookupSids failed
FAILED (1 failures, 0 errors and 0 unexpected successes in 0 testsuites)

If anybody has any insights or suggestions please don't hesitate to
investigate.

It turned out to be an unrelated use-after-free in the LSA server after
the trusts changes recently.
The issue was found fairly easily with address-sanitizer and is fixed
in the attached. This needs to be in 4.8.2 the regression was shipped
with 4.8.0.

Do you have more detailed information on what memory is used after free?
I'd prefer to do the correct talloc_move() calls in order to get a sane
memory tree instead of being lazy.

metze

[Andrew Bartlett via samba-technical]

Post by Stefan Metzmacher via samba-technical

Post by Andrew Bartlett via samba-technical
On Sat, 2018-04-14 at 07:07 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
[133(711)/525 at 10m56s] samba3.rpc.lsa.lookupsids(ad_dc)
smbtorture 4.9.0pre1-DEVELOPERBUILD
Using seed 1523610756
UNEXPECTED(failure): samba3.rpc.lsa.lookupsids.lsa.LookupSidsReply(ad_dc)
REASON: Exception: Exception: ../source4/torture/rpc/lsa_lookup.c:400: names.names[0].name.string was , expected S-1-5-21-1111111111-2222222222-3333333333-512: unexpected names[0].string
FAILED (1 failures, 0 errors and 0 unexpected successes in 0 testsuites)

commit cb607346d3c7c662343b0eae69e43eaa6358c188
Date: Tue Mar 13 16:43:54 2018 +1300
ldb-samba: require pid match for cached ldb

Post by Andrew Bartlett via samba-technical
Testing OpenPolicy2
UNEXPECTED(failure): samba4.rpc.altercontext on ncalrpc with seal,padcheck.altercontext(ad_dc_ntvfs:local)
REASON: Exception: Exception: ../source4/torture/rpc/lsa.c:188: status was NT_STATUS_CONNECTION_RESET, expected NT_STATUS_CONNECTION_DISCONNECTED: OpenPolicy2 failed

(but wrote it off as I also got about 10 successes on branches with
that series in it).

Post by Andrew Bartlett via samba-technical
Testing LookupSids
ndr_pull_error(1): Bad array size - got 0 expected 8
UNEXPECTED(failure): samba3.rpc.lsa.privileges.lsa.Privileges(ad_dc)
REASON: Exception: Exception: ../source4/torture/rpc/lsa.c:774: dcerpc_lsa_LookupSids_r(b, tctx, &r) was NT_STATUS_ARRAY_BOUNDS_EXCEEDED, expected NT_STATUS_OK: LookupSids failed
FAILED (1 failures, 0 errors and 0 unexpected successes in 0 testsuites)

If anybody has any insights or suggestions please don't hesitate to
investigate.

It turned out to be an unrelated use-after-free in the LSA server after
the trusts changes recently.
The issue was found fairly easily with address-sanitizer and is fixed
in the attached. This needs to be in 4.8.2 the regression was shipped
with 4.8.0.

Do you have more detailed information on what memory is used after free?

It shows up pretty fast under address sanitizer with the other patches
posted today. It seemed so ovbious that I didn't go much further in
making notes, but here is the backtrace.

https://attachments.samba.org/attachment.cgi?id=14174

Post by Stefan Metzmacher via samba-technical
I'd prefer to do the correct talloc_move() calls in order to get a sane
memory tree instead of being lazy.

Have a look at:
dcesrv_lsa_LookupSids()
state->r.out.names = talloc_zero(state, struct
lsa_TransNameArray2);

Thanks!

Andrew Bartlett

[Stefan Metzmacher via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Stefan Metzmacher via samba-technical

Post by Andrew Bartlett via samba-technical
On Sat, 2018-04-14 at 07:07 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical
[133(711)/525 at 10m56s] samba3.rpc.lsa.lookupsids(ad_dc)
smbtorture 4.9.0pre1-DEVELOPERBUILD
Using seed 1523610756
UNEXPECTED(failure): samba3.rpc.lsa.lookupsids.lsa.LookupSidsReply(ad_dc)
REASON: Exception: Exception: ../source4/torture/rpc/lsa_lookup.c:400: names.names[0].name.string was , expected S-1-5-21-1111111111-2222222222-3333333333-512: unexpected names[0].string
FAILED (1 failures, 0 errors and 0 unexpected successes in 0 testsuites)

commit cb607346d3c7c662343b0eae69e43eaa6358c188
Date: Tue Mar 13 16:43:54 2018 +1300
ldb-samba: require pid match for cached ldb

Post by Andrew Bartlett via samba-technical
Testing OpenPolicy2
UNEXPECTED(failure): samba4.rpc.altercontext on ncalrpc with seal,padcheck.altercontext(ad_dc_ntvfs:local)
REASON: Exception: Exception: ../source4/torture/rpc/lsa.c:188: status was NT_STATUS_CONNECTION_RESET, expected NT_STATUS_CONNECTION_DISCONNECTED: OpenPolicy2 failed

(but wrote it off as I also got about 10 successes on branches with
that series in it).

Post by Andrew Bartlett via samba-technical
Testing LookupSids
ndr_pull_error(1): Bad array size - got 0 expected 8
UNEXPECTED(failure): samba3.rpc.lsa.privileges.lsa.Privileges(ad_dc)
REASON: Exception: Exception: ../source4/torture/rpc/lsa.c:774: dcerpc_lsa_LookupSids_r(b, tctx, &r) was NT_STATUS_ARRAY_BOUNDS_EXCEEDED, expected NT_STATUS_OK: LookupSids failed
FAILED (1 failures, 0 errors and 0 unexpected successes in 0 testsuites)

If anybody has any insights or suggestions please don't hesitate to
investigate.

It turned out to be an unrelated use-after-free in the LSA server after
the trusts changes recently.
The issue was found fairly easily with address-sanitizer and is fixed
in the attached. This needs to be in 4.8.2 the regression was shipped
with 4.8.0.

Do you have more detailed information on what memory is used after free?

It shows up pretty fast under address sanitizer with the other patches
posted today. It seemed so ovbious that I didn't go much further in
making notes, but here is the backtrace.
https://attachments.samba.org/attachment.cgi?id=14174

Post by Stefan Metzmacher via samba-technical
I'd prefer to do the correct talloc_move() calls in order to get a sane
memory tree instead of being lazy.

dcesrv_lsa_LookupSids()
state->r.out.names = talloc_zero(state, struct
lsa_TransNameArray2);

I guess this would also fix it?

metze

[Andrew Bartlett via samba-technical]

Post by Stefan Metzmacher via samba-technical
I guess this would also fix it?

I'll let you chase it from here. Given we had to patch both lookupSide
and LookupNames I think there is more than that one line.

Anyway, ASan support is now in the tree and the test command I used
was:

make test TESTS='samba3.rpc.lsa.lookupsids samba4.rpc.altercontext
samba3.rpc.lsa.privileges.lsa.Privileges samba.wbinfo_sids2xids --
include-env=ad_member --include-env=ad_dc_ntvfs --include-env=ad_dc'

Thanks,

Andrew Bartlett

[Andrew Bartlett via samba-technical]

On Thu, 2018-04-12 at 11:47 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Douglas Bagnall via samba-technical

Post by Andrew Bartlett via samba-technical
I've seen this on a master build, so it is just happening without these
patches, but I also have a patch to address the issue by locking the
init() hook.
https://gitlab.com/catalyst-samba/samba/commits/lmdb-pre-7

I have reviewed these ones and they LGTM.

Thanks! It will be good to get these in.  
I'll autobuild these later today.

With the LSA issue found, I've re-send these to autobuild.

Thanks,

Andrew Bartlett

[William Brown via samba-technical]

On Thu, 2018-02-22 at 11:45 +1300, Garming Sam via samba-technical

Post by Garming Sam via samba-technical
Hi,
This is our current set of patches for implementing an LMDB based
backend for LDB. The work is based on a prototype I wrote around this
time last year inspired by Jakub's efforts. In saying that, the approach
I took was completely different. The idea was to refactor ldb_tdb to be
agnostic about which database backend was being used. The advantage has
been quite minimal amount of code required to implement a functional
64-bit database backend. Many of the performance optimizations made for
ldb_tdb can simply be reused, but conversely, for now we have
deferred
re-thinking the overall architecture e.g. consolidating the
partitions
into a single file using LMDB sub databases.

There are some arguments both ways here I think.

Keeping them seperate means that you can easily identify any component
that has a size blow out (specific index, changelog etc), and
potentially resolve it. This is pretty nice for admins to be able to
deal with. It also makes it easier to export/import large files to
resize them (lmdb files can't be resized iirc). If you have a combined
DB and it's say ... 8GB, then you have to have at least 8GB free to
export and import to a new db. If this was spilt to many files (say
0.5GB each) it becomes easier to manage this.

However, joining these would be good for simpler transaction
managament, especially to prevent deadlocking if tables are locked out-
of-order.

Provided you are really careful about transaction managament - for
example, if you take a write txn, you take a write txn on ALL mdb
files, you can "emulate" the behaviour of a single backend and gain
most of it's benefits, but you do pay an overhead to manage all those
transactions - and the cleanup of the reads when you close these.

Hope my vague comments help,

[Howard Chu via samba-technical]

Post by William Brown via samba-technical

Post by Garming Sam via samba-technical
re-thinking the overall architecture e.g. consolidating the
partitions
into a single file using LMDB sub databases.

There are some arguments both ways here I think.
Keeping them seperate means that you can easily identify any component
that has a size blow out (specific index, changelog etc), and
potentially resolve it. This is pretty nice for admins to be able to
deal with. It also makes it easier to export/import large files to
resize them (lmdb files can't be resized iirc). If you have a combined
DB and it's say ... 8GB, then you have to have at least 8GB free to
export and import to a new db. If this was spilt to many files (say
0.5GB each) it becomes easier to manage this.

mdb_stat will tell you the sizes of each subDB. You can use "mdb_dump -s" to
export a specific subDB. And "mdb_drop -s" to delete a specific subDB. Not
sure what you mean about "LMDB files can't be resized" - sure they can, that's
what mdb_env_set_mapsize() does.

Post by William Brown via samba-technical
However, joining these would be good for simpler transaction
managament, especially to prevent deadlocking if tables are locked out-
of-order.

[Garming Sam via samba-technical]

Post by Howard Chu via samba-technical

Post by William Brown via samba-technical

Post by Garming Sam via samba-technical
re-thinking the overall architecture e.g. consolidating the
partitions
into a single file using LMDB sub databases.

There are some arguments both ways here I think.
Keeping them seperate means that you can easily identify any component
that has a size blow out (specific index, changelog etc), and
potentially resolve it. This is pretty nice for admins to be able to
deal with. It also makes it easier to export/import large files to
resize them (lmdb files can't be resized iirc). If you have a combined
DB and it's say ... 8GB, then you have to have at least 8GB free to
export and import to a new db. If this was spilt to many files (say
0.5GB each) it becomes easier to manage this.

mdb_stat will tell you the sizes of each subDB. You can use "mdb_dump
-s" to export a specific subDB. And "mdb_drop -s" to delete a specific
subDB. Not sure what you mean about "LMDB files can't be resized" -
sure they can, that's what mdb_env_set_mapsize() does.

From Gary attempting an automatic resize mechanism, the caveats
associated with mdb_env_set_mapsize() made it not straightforward to
use, at least within the context of LDB. I presume William ran into
similar complications. Right now, we've only used a fixed figure, but
with the next iteration of our work, we'll probably be looking at how to
get this right.


Cheers,

Garming

Post by Howard Chu via samba-technical

Post by William Brown via samba-technical
However, joining these would be good for simpler transaction
managament, especially to prevent deadlocking if tables are locked out-
of-order.

[William Brown via samba-technical]

On Tue, 2018-04-17 at 12:21 +1200, Garming Sam via samba-technical

Post by Garming Sam via samba-technical

Post by Howard Chu via samba-technical

Post by William Brown via samba-technical

Post by Garming Sam via samba-technical
re-thinking the overall architecture e.g. consolidating the
partitions
into a single file using LMDB sub databases.

There are some arguments both ways here I think.
Keeping them seperate means that you can easily identify any component
that has a size blow out (specific index, changelog etc), and
potentially resolve it. This is pretty nice for admins to be able to
deal with. It also makes it easier to export/import large files to
resize them (lmdb files can't be resized iirc). If you have a combined
DB and it's say ... 8GB, then you have to have at least 8GB free to
export and import to a new db. If this was spilt to many files (say
0.5GB each) it becomes easier to manage this.

mdb_stat will tell you the sizes of each subDB. You can use
"mdb_dump
-s" to export a specific subDB. And "mdb_drop -s" to delete a specific
subDB. Not sure what you mean about "LMDB files can't be resized" -
sure they can, that's what mdb_env_set_mapsize() does.

From Gary attempting an automatic resize mechanism, the caveats
associated with mdb_env_set_mapsize() made it not straightforward to
use, at least within the context of LDB. I presume William ran into
similar complications. Right now, we've only used a fixed figure, but
with the next iteration of our work, we'll probably be looking at how to
get this right.

I've seen a number of sites get into a bit of an issue when they hit
the max size, and resizing didn't work without export/imports. I really
really just need to find some time to stress test mdb's edge cases so I
can give a report to Howard.

Post by Garming Sam via samba-technical
Cheers,
Garming

Post by Howard Chu via samba-technical

Post by William Brown via samba-technical
However, joining these would be good for simpler transaction
managament, especially to prevent deadlocking if tables are locked out-
of-order.