Volker's tdb mutex patches at: https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-tdb-mutex Need integration before 4.2 can be release. Jeremy.
When we get a 4.2.x release bug this should be marked as a blocker. Jeremy.
Found the 4.2 blocker bug and added it there :-). Jeremy.
Private until IBM bug # removed..
Created attachment 9839 [details] Updated without IBM bug number.
Thanks Lars - marking public again ! Jeremy.
tdb locking is just too complex to see this major change
To justify my judgement: We are sitting on this code for over a year now, and thorough review still finds fundamental flaws. This is only possible specialized use cases like Samba, but general tdb users will see too many subtle API changes that we will not be able to document and control.
One more comment: For Samba OEMs we will certainly make patches available, so that the speed improvements can be used in controlled environments.
Not letting you off this easy :-). Fundamental flaws can (and will) be fixed. This code needs to go into Samba 4.2.0 :-). Jeremy.
Metze, can you post your summary of the issues with the code so we have a central place to track the fixes ? Thanks ! Jeremy.
Created attachment 9858 [details] Current work in progress on top of master I still need to squash some commits and recheck the following: - the error checking for invalid tdb_flag combinations in tdb_open_ex() - fix the standalone make test without mutex support. - recheck the error code pathes, we may need to set errno= in some places.
Created attachment 9939 [details] Cleaned patch on top of master The "ORIG-COMMIT ..." commits are just for reference to make it easier to do a local git diff between the early customer patch set "ORIG-COMMIT vCustomer ...: tdb: Add mutex support" and a port to master of it "ORIG-COMMIT v2 ....=> SPLIT: tdb: Add mutex support". These patches will be removed later... The code has changed a bit so that we only allow TDB_MUTEX_LOCKING if TDB_CLEAR_IF_FIRST is also used, that makes sure that users of TDB_MUTEX_LOCKING hopefully think about what they're doing. And make sure they understand the locking limitation e.g. no shared read chainlocks from 2 processes on the same hashchain. The configure checks have changed a bit, there's no runtime check at configure stage anymore, there's only the tdb_runtime_check_for_robust_mutexes() function which must be checked before using TDB_MUTEX_LOCKING. That allows builds in chroot environments with older kernel versions running. The code allows that a tdb version which wasn't build with mutex support (or with tdb_runtime_check_for_robust_mutexes() returning false) to open a tdb with the mutex feature using the TDB_NOLOCK flag. This means tdbdump and tdbtool always work at least readonly with TDB_NOLOCK!
Are you happy for this to start being merged into master ?
(In reply to comment #14) > Are you happy for this to start being merged into master ? Yes, as it passed autobuilds with and without mutex support beeing available in the system. It also passed my manual testing with copying mutex tdb from one system to another and run tdbdump there. So the missing parts are: - review from you (Jeremy) - the "TODO: Signed-off-by:" lines in the commit messages need to be renewed (Volker and Michael) - remove the "ORIG-COMMIT ..." commits Then we could post this to samba-technical with an explanation of the improved performance and push it to master.
OK, I'll start the review asap ! Jeremy.
(In reply to comment #15) > (In reply to comment #14) > > Are you happy for this to start being merged into master ? > > Yes, as it passed autobuilds with and without mutex support beeing > available in the system. It also passed my manual testing > with copying mutex tdb from one system to another and run tdbdump > there. > > So the missing parts are: > - review from you (Jeremy) > - the "TODO: Signed-off-by:" lines in the commit messages > need to be renewed (Volker and Michael) > - remove the "ORIG-COMMIT ..." commits > > Then we could post this to samba-technical with an explanation > of the improved performance and push it to master. I have reviewed the patches and: - added review to a few patches - converted "TODO: Signed-off-by: /me" to "Signed-off-by: /me" - fixed a (very) few typos in the excellent mutex.txt document - pushed the resulting branch to git://git.samba.org/obnox/samba/samba-obnox.git branch master-tdb-mutex - successfully ran autobuild on sn-devel (doesn't support robust mutexes) - I am currently running an autobuild on my system (supports robust mutexes) Michael
(In reply to comment #17) > (In reply to comment #15) > > (In reply to comment #14) > > > Are you happy for this to start being merged into master ? > > > > Yes, as it passed autobuilds with and without mutex support beeing > > available in the system. It also passed my manual testing > > with copying mutex tdb from one system to another and run tdbdump > > there. > > > > So the missing parts are: > > - review from you (Jeremy) > > - the "TODO: Signed-off-by:" lines in the commit messages > > need to be renewed (Volker and Michael) > > - remove the "ORIG-COMMIT ..." commits > > > > Then we could post this to samba-technical with an explanation > > of the improved performance and push it to master. > > I have reviewed the patches and: > - added review to a few patches > - converted "TODO: Signed-off-by: /me" to "Signed-off-by: /me" > - fixed a (very) few typos in the excellent mutex.txt document > - pushed the resulting branch to > git://git.samba.org/obnox/samba/samba-obnox.git > branch master-tdb-mutex > - successfully ran autobuild on sn-devel > (doesn't support robust mutexes) > - I am currently running an autobuild on my system > (supports robust mutexes) "autobuild tdb" succeeds on my system. "autobuild samba" fails on my system at "samba.tests.gensec". But it fails the same way without the mutex patches. So the mutex patches are not the cause.
Sorry, been offline with family (my brother) until today (20th). I'm planning to review this as soon and propose to push to master (if I'm happy :-) as soon I get back into the office tomorrow morning (21st). Thanks, Jeremy.
(In reply to comment #19) > Sorry, been offline with family (my brother) until today (20th). I'm planning > to review this as soon and propose to push to master (if I'm happy :-) as soon > I get back into the office tomorrow morning (21st). > > Thanks, > > Jeremy. Jeremy, since I know you don't like to fetch git branches, I'll attach my rebased, updated and reviewed patchset for further processing. Michael
Created attachment 9959 [details] updated patchset for master (w/ my reviews) as announced..
(In reply to comment #21) > Created attachment 9959 [details] > updated patchset for master (w/ my reviews) > > as announced.. OK, I looked over it carefully and the result after application seems good to me - but it still has some reverts in the middle of the patchset. Can we converge on a *final* patchset we can just push to master please ? One without intermediate reverts would be good as it looks like all the functionality we need is here. I don't understand why the patches: ------------------------------------------------ Subject: [PATCH 17/29] Revert "ORIG-COMMIT vCustomer ...: tdb: Add mutex support" This reverts commit a91830621e5d12b59f27ff3bbf813e523094fbaf. ------------------------------------------------ Subject: [PATCH 19/29] Revert "ORIG-COMMIT v2 ....=> SPLIT: tdb: Add mutex support" This reverts commit ab2c574632f43c51cff8ce1964e630a9d658a545. ------------------------------------------------ are in this patchset, as the following patches just re-introduce the code :-). Can we sort this out please ? Cheers, Jeremy.
(In reply to comment #23) > (In reply to comment #21) > > Created attachment 9959 [details] [details] > > updated patchset for master (w/ my reviews) > > > > as announced.. > > OK, I looked over it carefully and the result after application seems good to > me - but it still has some reverts in the middle of the patchset. > > Can we converge on a *final* patchset we can just push to master please ? One > without intermediate reverts would be good as it looks like all the > functionality we need is here. I don't understand why the patches: > > ------------------------------------------------ > Subject: [PATCH 17/29] Revert "ORIG-COMMIT vCustomer ...: tdb: Add mutex > support" > > This reverts commit a91830621e5d12b59f27ff3bbf813e523094fbaf. > ------------------------------------------------ > Subject: [PATCH 19/29] Revert "ORIG-COMMIT v2 ....=> SPLIT: tdb: Add mutex > support" > > This reverts commit ab2c574632f43c51cff8ce1964e630a9d658a545. > ------------------------------------------------ > > are in this patchset, as the following patches just re-introduce the code :-). > > Can we sort this out please ? See https://bugzilla.samba.org/show_bug.cgi?id=10525#c13 and https://bugzilla.samba.org/show_bug.cgi?id=10525#c15 They should just make it possible to see the difference compared to the older versions using git diff -p --stat between the patchsets. These commits should be removed once the real patches are reviewed and signed off.
Created attachment 9964 [details] Patch on top of master to be reviewed and signed off Jeremy, this is the patchset without the "ORIG-COMMIT ..." patches, please add your review tags. Volker, are you fine if we remove the 'TODO' from TODO: Signed-off-by: Volker Lendecke <vl@samba.org> in the commit messages?
(In reply to comment #25) > Volker, are you fine if we remove the 'TODO' from > TODO: Signed-off-by: Volker Lendecke <vl@samba.org> > in the commit messages? Sure! Thanks for doing the work on this! Volker
Created attachment 9965 [details] Patches for master Here's the patchset for master. Please add your review tags if you want.
(In reply to comment #27) > Created attachment 9965 [details] > Patches for master > > Here's the patchset for master. > > Please add your review tags if you want. Yay finally ! I'll review today. Jeremy.
tdb-1.3.0 contains this...