389-ds-base

#50181 pull request to review changes for backend refactoring
Closed by spichugi. Opened by lkrispen.
lkrispen/389-ds-base t49476  into  master

Download 50181.patch

see: http://www.port389.org/docs/389ds/design/backend-redesign.html

+ slapi_atomic_incr_64(&(a->ai_dblayer_count), __ATOMIC_ACQREL);
If you are incrementing, you need ATOMIC_ACQUIRE (I think) to establish the correct ordering for a ref count.

Edited by firstyear

I think this all looks pretty reasonable, it's mainly moving code. I think reading it all though, perhaps after this is merged it would be worth a backend-cleanup to promote 64bits, correct size_t use in arrays, and some legacy cleanup?
Have you run this with ASAN?

I think this all looks pretty reasonable, it's mainly moving code. I think reading it all though, perhaps after this is merged it would be worth a backend-cleanup to promote 64bits, correct size_t use in arrays, and some legacy cleanup?
Have you run this with ASAN?

thank you for looking into it, I just waited until the 1.4.0 branch was created and made this PR for people o be able to review and test it.
We will go thru a full test and cleanup before merging, but if you notice anything, like the ATOMIC stuff please add a cooment, I will check

Okay, I will do a build and test as well then soon. If in doubt with the atomics, use SEQCST as the ordering, but generally ref count you want ACQREL.

rebased onto 94ef4ffd12d367cfde9bbad480fa3aadd38b331a

added an additional commit

also noticed that in some tests dbcache had been set to 0, interaction of cache tuning, default caches and conf settings need to be investigated further

I think there are some memory leaks in this change. Nothing major, but probably good to have cleaned.

I'm seeing an error during basic test, but I think this is due to the memory leaks (ASAN + lib389 are really sensitive to this). The cli leak may be due to NSPR not be closed properly at the end of program execution, but I have not investigated deeply this issue.

Hope this helps,

Cli leaks:

> /opt/dirsrv/sbin/ns-slapd ldif2db -D /opt/dirsrv/etc/dirsrv/slapd-standalone1 -i /opt/dirsrv/var/lib/dirsrv/slapd-standalone1/ldif/basic_import.ldif -n userRoot
[31/Jan/2019:13:04:57.110836300 +1000] - ERR - slapd_system_isFIPS - Can not access /proc/sys/crypto/fips_enabled - assuming FIPS is OFF
[31/Jan/2019:13:04:57.113079700 +1000] - ERR - slapd_system_isFIPS - Can not access /proc/sys/crypto/fips_enabled - assuming FIPS is OFF
=================================================================
==98814==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7f90ebd30ed0 in malloc (/usr/lib64/libasan.so.5+0xebed0)
    #1 0x7f90eb221767  (/usr/lib64/libnspr4.so+0x12767)
SUMMARY: AddressSanitizer: 32 byte(s) leaked in 1 allocation(s).

From server leaks:

Direct leak of 200 byte(s) in 1 object(s) allocated from:
    #0 0x7fd2787560b8 in calloc (/usr/lib64/libasan.so.5+0xec0b8)
    #1 0x7fd278379af6 in slapi_ch_calloc /home/william/development/389ds/ds/ldap/servers/slapd/ch_malloc.c:175
    #2 0x7fd27255548a in bdb_init /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/back-bdb/bdb_config.c:41
    #3 0x7fd27246c122 in dblayer_setup /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/dblayer.c:273
    #4 0x7fd272540c1a in ldbm_back_start /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/start.c:300
    #5 0x7fd278482dea in plugin_call_func /home/william/development/389ds/ds/ldap/servers/slapd/plugin.c:2029
    #6 0x7fd278482a46 in plugin_call_one /home/william/development/389ds/ds/ldap/servers/slapd/plugin.c:1978
    #7 0x7fd2784818ea in plugin_dependency_startall /home/william/development/389ds/ds/ldap/servers/slapd/plugin.c:1737
    #8 0x7fd2784829d5 in plugin_startall /home/william/development/389ds/ds/ldap/servers/slapd/plugin.c:1950
    #9 0x44afa4 in main /home/william/development/389ds/ds/ldap/servers/slapd/main.c:1150
    #10 0x7fd277400fea in __libc_start_main (/lib64/libc.so.6+0x22fea)
Direct leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7fd2786a77e0 in strdup (/usr/lib64/libasan.so.5+0x3d7e0)
    #1 0x7fd278379bdc in slapi_ch_strdup /home/william/development/389ds/ds/ldap/servers/slapd/ch_malloc.c:196
    #2 0x7fd278398674 in ndn_cache_lookup /home/william/development/389ds/ds/ldap/servers/slapd/dn.c:3154
    #3 0x7fd27838aa85 in slapi_dn_normalize_ext /home/william/development/389ds/ds/ldap/servers/slapd/dn.c:576
    #4 0x7fd278390c4c in slapi_create_dn_string /home/william/development/389ds/ds/ldap/servers/slapd/dn.c:1193
    #5 0x7fd27255b067 in bdb_config_upgrade_dse_info /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/back-bdb/bdb_config.c:1698
    #6 0x7fd27255bd7b in bdb_config_load_dse_info /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/back-bdb/bdb_config.c:1843
    #7 0x7fd27246c1c4 in dblayer_setup /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/dblayer.c:283
    #8 0x7fd272540c1a in ldbm_back_start /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/start.c:300
    #9 0x7fd278482dea in plugin_call_func /home/william/development/389ds/ds/ldap/servers/slapd/plugin.c:2029
    #10 0x7fd278482a46 in plugin_call_one /home/william/development/389ds/ds/ldap/servers/slapd/plugin.c:1978
    #11 0x7fd2784818ea in plugin_dependency_startall /home/william/development/389ds/ds/ldap/servers/slapd/plugin.c:1737
    #12 0x7fd2784829d5 in plugin_startall /home/william/development/389ds/ds/ldap/servers/slapd/plugin.c:1950
    #13 0x44afa4 in main /home/william/development/389ds/ds/ldap/servers/slapd/main.c:1150
    #14 0x7fd277400fea in __libc_start_main (/lib64/libc.so.6+0x22fea)
Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x7fd2787560b8 in calloc (/usr/lib64/libasan.so.5+0xec0b8)
    #1 0x7fd278379af6 in slapi_ch_calloc /home/william/development/389ds/ds/ldap/servers/slapd/ch_malloc.c:175
    #2 0x7fd2784916d2 in search_internal_pb /home/william/development/389ds/ds/ldap/servers/slapd/plugin_internal_op.c:600
    #3 0x7fd278490ef7 in slapi_search_internal_pb /home/william/development/389ds/ds/ldap/servers/slapd/plugin_internal_op.c:504
    #4 0x7fd27255bb3b in bdb_config_load_dse_info /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/back-bdb/bdb_config.c:1822
    #5 0x7fd27246c1c4 in dblayer_setup /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/dblayer.c:283
    #6 0x7fd272540c1a in ldbm_back_start /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/start.c:300
    #7 0x7fd278482dea in plugin_call_func /home/william/development/389ds/ds/ldap/servers/slapd/plugin.c:2029
    #8 0x7fd278482a46 in plugin_call_one /home/william/development/389ds/ds/ldap/servers/slapd/plugin.c:1978
    #9 0x7fd2784818ea in plugin_dependency_startall /home/william/development/389ds/ds/ldap/servers/slapd/plugin.c:1737
    #10 0x7fd2784829d5 in plugin_startall /home/william/development/389ds/ds/ldap/servers/slapd/plugin.c:1950
    #11 0x44afa4 in main /home/william/development/389ds/ds/ldap/servers/slapd/main.c:1150
    #12 0x7fd277400fea in __libc_start_main (/lib64/libc.so.6+0x22fea)

added an additional commit
also noticed that in some tests dbcache had been set to 0, interaction of cache tuning, default caches and conf settings need to be investigated further

There are some times where you require this value to be 0 for the autotuning to work. I think you are correct that we should investigate to be sure the behaviour is "as expected". When originally writing the feature there were better possible ways to express the configuration, but we were limited by supporting legacy config syntax which made it difficult.

@firstyear thanks for testing, I will look into the memory leaks, Viktor already found another one which I fixed, and he also determined the NSPR leaks: https://hg.mozilla.org/projects/nspr/file/tip/pr/src/io/prprf.c#l1104

Regarding cahces and tuning I need to have a bit more time for a closer look. Do we have a doc for autotune somewhere ?

http://www.port389.org/docs/389ds/design/autotuning.html here is the design document I wrote while developing the feature. Additionally, I remember that I put in extensive comments in the code to help with this. Hope that helps.

CONFIG_DB_TRANSACTION_LOGGING is defined at ldbm and bdb level. Both updates li_dblayer_private. Why keeping it at the ldbm level ?

same here. I would expect all tuning in li_dblayer_private to be on 'bdb' side.
Note that I think this all patch needs to go in master first and after we can open separate tickets for those minor changes.

should it be moved to bdb config (dbcache, dblock), even li_directory ?

An other option would be to create a plugin entry in dse.ldif and with an init function that would register a start/stop function (for example for bdb, register bdb_start (similar to bdb_init).
Backend could call this start/stop according to li_backend_implement.

Just a remark, mapping tree was initialized before the backend. I think it has no impact but to be coherent between archive2db and db2archive it would be nice to have the same order
  • slapi_atomic_incr_64(&(a->ai_dblayer_count), __ATOMIC_ACQREL);
    If you are incrementing, you need ATOMIC_ACQUIRE (I think) to establish the correct ordering for a ref count.

But this part of code was only moved, not changed. And we have:

 git grep slapi_atomic_incr_64
 ldap/servers/slapd/attrsyntax.c:            slapi_atomic_incr_64(&(asi->asi_refcnt), __ATOMIC_RELEASE);
 ldap/servers/slapd/attrsyntax.c:            slapi_atomic_incr_64(&(asi->asi_refcnt), __ATOMIC_RELEASE);
 ldap/servers/slapd/back-ldbm/dblayer.c:    slapi_atomic_incr_64(&(a->ai_dblayer_count), __ATOMIC_RELEASE);
 ldap/servers/slapd/libglobs.c:    slapi_atomic_incr_64(&active_threads, __ATOMIC_RELEASE);
 ldap/servers/slapd/log.c:    slapi_atomic_incr_64(&(lbi->refcount), __ATOMIC_RELEASE);
 ldap/servers/slapd/object.c:    slapi_atomic_incr_64(&(o->refcnt), __ATOMIC_RELEASE);
 ldap/servers/slapd/psearch.c:            slapi_atomic_incr_64(&(ps->ps_complete), __ATOMIC_RELEASE);
 ldap/servers/slapd/slapi-plugin.h:uint64_t slapi_atomic_incr_64(uint64_t *ptr, int memorder);
 ldap/servers/slapd/slapi_counter.c:slapi_atomic_incr_64(uint64_t *ptr, int memorder)
 ldap/servers/slapd/vattr.c:        slapi_atomic_incr_64(&(sp->rc), __ATOMIC_RELAXED);
 ldap/servers/slapd/vattr.c:        slapi_atomic_incr_64(&(sp->rc), __ATOMIC_RELAXED)

so you are saying that it is incorrect everywhere ?

CONFIG_DB_TRANSACTION_LOGGING is defined at ldbm and bdb level. Both updates li_dblayer_private. Why keeping it at the ldbm level ?

good catch

same here. I would expect all tuning in li_dblayer_private to be on 'bdb' side.
Note that I think this all patch needs to go in master first and after we can open separate tickets for those minor changes.

yes, it should not be duplicate, but it belongs to the ldbm layer, the idl divisor controls how we build the key value pairs for indexes, not how it is written to the database.

But it is only used in the OLD IDL format, which is no longer really used. Maybe we can get rid of this at all

this is obsolete, the change was committed in PR 50596

Pull-Request has been closed by lkrispen

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3240

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.