Quantcast

Proposed ldb 1.1.30 and tdb 1.3.13 (improve AD DC search performance)

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Proposed ldb 1.1.30 and tdb 1.3.13 (improve AD DC search performance)

Samba - samba-technical mailing list
G'Day Metze,

Attached is a patch series to bump up the ldb and tdb version numbers
with important fixes for performance, resulting from the locking work
discussed in the other thread, and the performance improvements found
by using Samba's binary search over the list of attributes with an
index.

You will no doubt want to carefully consider the tdb patches along with
the rest of the team, and asked to be the one to push the ldb changes
so you could tag the release.

This passes a private autobuild.

Please let me know if there is anything else I can do to assist.

Thanks,

Andrew Bartlett

ldb-1.1.30-for-metze.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Proposed ldb 1.1.30 and tdb 1.3.13 (improve AD DC search performance)

Samba - samba-technical mailing list
Hi Andrew,

> Attached is a patch series to bump up the ldb and tdb version numbers
> with important fixes for performance, resulting from the locking work
> discussed in the other thread, and the performance improvements found
> by using Samba's binary search over the list of attributes with an
> index.
>
> You will no doubt want to carefully consider the tdb patches along with
> the rest of the team, and asked to be the one to push the ldb changes
> so you could tag the release.
>
> This passes a private autobuild.
>
> Please let me know if there is anything else I can do to assist.
Please retry again with --stdout :-)

metze


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Proposed ldb 1.1.30 and tdb 1.3.13 (improve AD DC search performance)

Samba - samba-technical mailing list
On Mon, 2017-04-03 at 13:04 +0200, Stefan Metzmacher wrote:

> Hi Andrew,
>
> > Attached is a patch series to bump up the ldb and tdb version
> > numbers
> > with important fixes for performance, resulting from the locking
> > work
> > discussed in the other thread, and the performance improvements
> > found
> > by using Samba's binary search over the list of attributes with an
> > index.
> >
> > You will no doubt want to carefully consider the tdb patches along
> > with
> > the rest of the team, and asked to be the one to push the ldb
> > changes
> > so you could tag the release.
> >
> > This passes a private autobuild.
> >
> > Please let me know if there is anything else I can do to assist.
>
> Please retry again with --stdout :-)
:-)

http://git.catalyst.net.nz/gw?p=samba.git;a=shortlog;h=refs/heads/ldb-1
.1.30-for-metze

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba

ldb-1.1.30-for-metze.patch (81K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] Re: Proposed ldb 1.1.30 and tdb 1.3.13 (improve AD DC search performance, make multi-process)

Samba - samba-technical mailing list
On Mon, 2017-04-03 at 23:48 +1200, Andrew Bartlett via samba-technical
wrote:

> On Mon, 2017-04-03 at 13:04 +0200, Stefan Metzmacher wrote:
> > Hi Andrew,
> >
> > > Attached is a patch series to bump up the ldb and tdb version
> > > numbers
> > > with important fixes for performance, resulting from the locking
> > > work
> > > discussed in the other thread, and the performance improvements
> > > found
> > > by using Samba's binary search over the list of attributes with
> > > an
> > > index.
> > >
> > > You will no doubt want to carefully consider the tdb patches
> > > along
> > > with
> > > the rest of the team, and asked to be the one to push the ldb
> > > changes
> > > so you could tag the release.
> > >
> > > This passes a private autobuild.
> > >
> > > Please let me know if there is anything else I can do to assist.
> >
> > Please retry again with --stdout :-)
>
> :-)
>
> http://git.catalyst.net.nz/gw?p=samba.git;a=shortlog;h=refs/heads/ldb
> -1
> .1.30-for-metze
Attached is the whole set of patches I'm currently trying to land, as
well as just the set discussed.  I've improved the whitespace.

The end purpose I'm trying to end up with is the AD DC set to use
multiple processes for LDAP, and fixing up the locking issues, so that
is more practical.  I've written tests to show where becoming multi-
process costs, and where it helps, and I'm happy with the trade-off.  

The attached graph illustrates it well.  (Run on my workstation, so
fairly noisy.  I may be able to get better graphs later).

For the multi-process, the worst case is when one client connects,
binds, searches for one thing and drops the socket, but this is a
pretty un-realistic workload, and once we get multiple clients this
change helps.

Thanks,

Andrew Bartlett
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba




ldb-1.1.30-for-metze.patch (79K) Download Attachment
multi-process-ldap-server.patch (99K) Download Attachment
multi-process-ldap-server.pdf (90K) Download Attachment
multi-process-ldap-server-abs.pdf (88K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Re: Proposed ldb 1.1.30 and tdb 1.3.13 (improve AD DC search performance, make multi-process)

Samba - samba-technical mailing list
On Wed, 2017-04-05 at 12:19 +1200, Andrew Bartlett wrote:
>
> Attached is the whole set of patches I'm currently trying to land, as
> well as just the set discussed.  I've improved the whitespace.

Attached is an updated patch of the whole series, to avoid confusion.

Thanks,

Andrew Bartlett
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba




ldap-multi-process.patch (107K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Re: Proposed ldb 1.1.30 and tdb 1.3.13 (improve AD DC search performance, make multi-process)

Samba - samba-technical mailing list
Am 05.04.2017 um 07:06 schrieb Andrew Bartlett via samba-technical:
> On Wed, 2017-04-05 at 12:19 +1200, Andrew Bartlett wrote:
>>
>> Attached is the whole set of patches I'm currently trying to land, as
>> well as just the set discussed.  I've improved the whitespace.
>
> Attached is an updated patch of the whole series, to avoid confusion.

Here's an update on current master.

I've added the run-fcntl-deadlock as test.

I still want to run the standalone make test of tdb and ldb on solaris,
before we push this and have a closer look at the ldb changes.

metze

tmp.diff.txt (98K) Download Attachment
signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Re: Proposed ldb 1.1.30 and tdb 1.3.13 (improve AD DC search performance, make multi-process)

Samba - samba-technical mailing list
On Tue, 2017-04-11 at 18:08 +0200, Stefan Metzmacher wrote:
>
> Here's an update on current master.
>
> I've added the run-fcntl-deadlock as test.
>
> I still want to run the standalone make test of tdb and ldb on
> solaris,
> before we push this and have a closer look at the ldb changes.

Thanks!

Finally, this is may also be a 'data corruption' issue:  

My theory is that because there is no read lock held, the index might
refer to different records compared with the data.  Sometimes that will
fall back to a traverse (we do that on errors), but sometimes that will
mean we don't return all the records, as we essentially check twice,
both in the index and in the final search filter.  

If a search took a long time, a modify could happen between those two
points, and while the modify would be atomic, the search would not be
atomic.

On the flip side, with this patch reads will block all writes for
longer.

BTW, A test for the LDB changes is in the cmocka thread.  Because LDB
is buggy it passes right now, but fails if we do have the ldb fix but
don't have the tdb fix.

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Re: Proposed ldb 1.1.30 and tdb 1.3.13 (improve AD DC search performance, make multi-process)

Samba - samba-technical mailing list
Hi Andrew,

>> Here's an update on current master.
>>
>> I've added the run-fcntl-deadlock as test.
>>
>> I still want to run the standalone make test of tdb and ldb on
>> solaris,
>> before we push this and have a closer look at the ldb changes.
>
> Thanks!
>
> Finally, this is may also be a 'data corruption' issue:  
>
> My theory is that because there is no read lock held, the index might
> refer to different records compared with the data.  Sometimes that will
> fall back to a traverse (we do that on errors), but sometimes that will
> mean we don't return all the records, as we essentially check twice,
> both in the index and in the final search filter.  
>
> If a search took a long time, a modify could happen between those two
> points, and while the modify would be atomic, the search would not be
> atomic.
I don't understand what you're trying to say...

Do you mean we need to call ltdb_lock_read() in a wider window?
E.g. when starting the ldb_search() from the top level module stack
until end of that search? Currently only searches are only atomic
within ltdb_search(), but not ldb_search().

> On the flip side, with this patch reads will block all writes for
> longer.

And here...?

> BTW, A test for the LDB changes is in the cmocka thread.  Because LDB
> is buggy it passes right now, but fails if we do have the ldb fix but
> don't have the tdb fix.

And here...

Be explicit please and refer to specific commits and tests.

Thanks!
metze


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Re: Proposed ldb 1.1.30 and tdb 1.3.13 (improve AD DC search performance, make multi-process)

Samba - samba-technical mailing list
On Tue, 2017-04-11 at 21:40 +0200, Stefan Metzmacher wrote:

> Hi Andrew,
>
> > > Here's an update on current master.
> > >
> > > I've added the run-fcntl-deadlock as test.
> > >
> > > I still want to run the standalone make test of tdb and ldb on
> > > solaris,
> > > before we push this and have a closer look at the ldb changes.
> >
> > Thanks!
> >
> > Finally, this is may also be a 'data corruption' issue:  
> >
> > My theory is that because there is no read lock held, the index
> > might
> > refer to different records compared with the data.  Sometimes that
> > will
> > fall back to a traverse (we do that on errors), but sometimes that
> > will
> > mean we don't return all the records, as we essentially check
> > twice,
> > both in the index and in the final search filter.  
> >
> > If a search took a long time, a modify could happen between those
> > two
> > points, and while the modify would be atomic, the search would not
> > be
> > atomic.
>
> I don't understand what you're trying to say...
>
> Do you mean we need to call ltdb_lock_read() in a wider window?
> E.g. when starting the ldb_search() from the top level module stack
> until end of that search? Currently only searches are only atomic
> within ltdb_search(), but not ldb_search().

They are not even that atomic!  The bug that started all this was that
ltdb_search() does not actually hold a lock (after the first read).  

I've not considered the implications further up the stack at this
point...

> > On the flip side, with this patch reads will block all writes for
> > longer.
>
> And here...?

If we actually hold a lock in ltdb_search(), then we will block writes
more often.  That is, what we fix in '[PATCH 07/17] ldb_tdb: Ensure we
correctly decrement ltdb->read_lock_count'.

> > BTW, A test for the LDB changes is in the cmocka thread.  Because
> > LDB
> > is buggy it passes right now, but fails if we do have the ldb fix
> > but
> > don't have the tdb fix. 
>
> And here...
>
> Be explicit please and refer to specific commits and tests.

https://lists.samba.org/archive/samba-technical/2017-April/119857.html
has
http://lists.samba.org/pipermail/samba-technical/attachments/20170407/4
a60faea/0002-ldb-Add-test-for-transaction-deadlock-detected-when-.bin

I'll merge the other cmocka tests shortly, then hope to work with
Andreas to get this on merged.  

When we Garming tried to fix the refcounting ([PATCH 07/17] ldb_tdb:
Ensure we correctly decrement ltdb->read_lock_count) and so hold the
lock in ltdb_search(), make test failed with deadlock detected.  This
test re-creates this situation manually, for easy validation.

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Loading...