Bug 11032 - Enable mutexes in gencache_notrans.tdb
Enable mutexes in gencache_notrans.tdb
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
4.2.0rc3
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-01-05 23:21 UTC by Christof Schmitt
Modified: 2015-01-26 20:16 UTC (History)
3 users (show)

See Also:


Attachments
Patches for 4.2 (24.07 KB, patch)
2015-01-05 23:26 UTC, Christof Schmitt
cs: review+
vl: review+
Details
Patches for 4.2, version 2 (24.40 KB, patch)
2015-01-16 16:38 UTC, Christof Schmitt
cs: review+
cs: review? (vl)
obnox: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Christof Schmitt 2015-01-05 23:21:10 UTC
Enabling mutexes for the gencache_notrans.tdb provides performance improvements for large servers that have to cache many id mappings. This bug is to
backport the required patches to 4.2.
Comment 1 Christof Schmitt 2015-01-05 23:26:37 UTC
Created attachment 10578 [details]
Patches for 4.2
Comment 2 Karolin Seeger 2015-01-06 20:27:14 UTC
Pushed to autobuild-v4-2-test.
Comment 3 Michael Adam 2015-01-11 21:54:29 UTC
The autobuild failed:

https://git.samba.org/kseeger/samba-autobuild-v4-2-test/samba.stderr

... 
Unable to create directory /m/kseeger/a42/b19509/prefix/samba/var/cache for file gencache.tdb. Error was No such file or directory
Opening cache file at (null)
Segmentation fault (core dumped)


https://git.samba.org/kseeger/samba-autobuild-v4-2-test/samba.stdout

...
[17/1686 in 4m3s] samba.tests.samba3
command: PYTHONPATH=$PYTHONPATH:/memdisk/kseeger/a42/b19509/samba/lib/subunit/python:/memdisk/kseeger/a42/b19509/samba/lib/testtools python -m subunit.run $LISTOPT samba.tests.samba3
expanded command: PYTHONPATH=/memdisk/kseeger/a42/b19509/samba/bin/python:/memdisk/kseeger/a42/b19509/samba/lib/subunit/python:/memdisk/kseeger/a42/b19509/samba/lib/testtools python -m subunit.run $LISTOPT samba.tests.samba3
ERROR: Testsuite[samba.tests.samba3]
REASON: Exit code was 139


command: PYTHONPATH=$PYTHONPATH:/memdisk/kseeger/a42/b19509/samba/lib/subunit/python:/memdisk/kseeger/a42/b19509/samba/lib/testtools python -m subunit.run $LISTOPT samba.tests.samba3
expanded command: PYTHONPATH=/memdisk/kseeger/a42/b19509/samba/bin/python:/memdisk/kseeger/a42/b19509/samba/lib/subunit/python:/memdisk/kseeger/a42/b19509/samba/lib/testtools python -m subunit.run $LISTOPT samba.tests.samba3
ERROR: Testsuite[samba.tests.samba3]
REASON: Exit code was 139


UNEXPECTED(error): samba.tests.samba3.PassdbTestCase.test_getuser (subunit.RemotedTestCase)
REASON: was started but never finished!

FAILED (0 failures, 1 errors and 0 unexpected successes in 1 testsuites)

A summary with detailed information can be found in:
  ./bin/ab/summary
ERROR: test failed with exit code 1
Comment 4 Michael Adam 2015-01-11 22:04:58 UTC
This was introduced by the conversion of genache to tdb_wrap_open.

https://git.samba.org/?p=samba.git;a=commitdiff;h=f80bbba2870a80ed421a1a222e4

tdb_open_log()  returns NULL upon name==NULL.
tdb_wrap_open() segfault     upon name==NULL.

And here null is passed according to the stderr output
because /m/kseeger/a42/b19509/prefix/samba/var/cache does not exist.
It does not exist because we run make test before make install.
Make test should be able to run w/o make install.

Several comments:
- tdb_wrap_open should check for NULL name
- the change for tdb_wrap_open was made to support mutexes on gencache:
  https://git.samba.org/?p=samba.git;a=shortlog;h=068f9e26486fbcd36c109df9ada
- this is actually not necessary any more because mutexes have been
  changed to support transactions

Question:
- why does master not have this problem??

Looking further...
Comment 5 Karolin Seeger 2015-01-14 20:21:54 UTC
(In reply to Michael Adam from comment #3)
confirmed
Comment 6 Karolin Seeger 2015-01-14 20:25:50 UTC
(In reply to Karolin Seeger from comment #2)
Removing for now as patchset breaks the autobuild.
Re-assigning to Christof.
Comment 7 Christof Schmitt 2015-01-14 23:14:54 UTC
In master, commit e8ee9bb66e765433e94f03d46ccb66459bb5fc3f introduces
an additional check if the code fails to create the cache directory:
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -65,6 +65,9 @@ static bool gencache_init(void)
        if (cache) return True;
 
        cache_fname = cache_path("gencache.tdb");
+       if (cache_fname == NULL) {
+               return false;
+       }
 
        DEBUG(5, ("Opening cache file at %s\n", cache_fname));
 

That could explain why the test in master does not segfault. The actual
fix for the issue here would be properly setting the cache directory
for the test:

--- a/python/samba/tests/samba3.py
+++ b/python/samba/tests/samba3.py
@@ -72,6 +72,7 @@ class PassdbTestCase(TestCaseInTempDir):
         self.lp.set("private dir", datadir)
         self.lp.set("state directory", datadir)
         self.lp.set("lock directory", datadir)
+        self.lp.set("cache directory", datadir)
         passdb.set_secrets_dir(datadir)
         self.pdb = passdb.PDB("tdbsam")

With this additional change, 'make test TESTS=tests.samba3' no  longer
segfaults for me on the v4-2-test branch with the proposed patch series
applied. Should we get the change in samba3.py in master, and then
include it in the patch series for this bugzilla?
Comment 8 Jeremy Allison 2015-01-15 01:04:16 UTC
Sounds good to me - prepare a patch and post to samba-technical and I'll get it into master !

Jeremy.
Comment 9 Christof Schmitt 2015-01-16 16:38:21 UTC
Created attachment 10627 [details]
Patches for 4.2, version 2

Here is the updated patch set. The last patch is the fix for th
problematic testcase.
Comment 10 Karolin Seeger 2015-01-16 20:09:54 UTC
Waiting for another review flag.
Comment 11 Michael Adam 2015-01-16 21:43:06 UTC
(In reply to Christof Schmitt from comment #7)

very good explanation and proper fix, thanks!

(we can still now thing about reverting gencache back to using transactions... :-)
Comment 12 Michael Adam 2015-01-16 21:52:38 UTC
Comment on attachment 10627 [details]
Patches for 4.2, version 2

ACK
Comment 13 Michael Adam 2015-01-16 21:53:00 UTC
karolin please pick! Thanks
Comment 14 Karolin Seeger 2015-01-24 20:43:06 UTC
Pushed to autobuild-v4-2-test.
Comment 15 Karolin Seeger 2015-01-26 20:16:34 UTC
Pushed to v4-2-test.
Closing out bug report.

Thanks!