Bug 10525 - tdb mutex patches need integration.
tdb mutex patches need integration.
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
unspecified
All Linux
: P1 critical
: ---
Assigned To: Volker Lendecke
Samba QA Contact
:
Depends on:
Blocks: 10077
  Show dependency treegraph
 
Reported: 2014-03-30 00:32 UTC by Jeremy Allison
Modified: 2014-10-13 10:52 UTC (History)
3 users (show)

See Also:


Attachments
Updated without IBM bug number. (145.49 KB, patch)
2014-04-09 19:06 UTC, Jeremy Allison
no flags Details
Current work in progress on top of master (380.92 KB, patch)
2014-04-17 14:52 UTC, Stefan Metzmacher
no flags Details
Cleaned patch on top of master (580.33 KB, patch)
2014-05-13 02:56 UTC, Stefan Metzmacher
no flags Details
updated patchset for master (w/ my reviews) (580.21 KB, patch)
2014-05-20 13:52 UTC, Michael Adam
no flags Details
Patch on top of master to be reviewed and signed off (159.53 KB, patch)
2014-05-22 06:28 UTC, Stefan Metzmacher
no flags Details
Patches for master (159.49 KB, patch)
2014-05-22 07:17 UTC, Stefan Metzmacher
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2014-03-30 00:32:27 UTC
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.
Comment 1 Jeremy Allison 2014-03-30 00:33:18 UTC
When we get a 4.2.x release bug this should be marked as a blocker.

Jeremy.
Comment 2 Jeremy Allison 2014-03-30 00:38:57 UTC
Found the 4.2 blocker bug and added it there :-).
Jeremy.
Comment 4 Jeremy Allison 2014-04-04 20:39:21 UTC
Private until IBM bug # removed..
Comment 5 Jeremy Allison 2014-04-09 19:06:54 UTC
Created attachment 9839 [details]
Updated without IBM bug number.
Comment 6 Jeremy Allison 2014-04-09 20:30:37 UTC
Thanks Lars - marking public again !
Jeremy.
Comment 7 Volker Lendecke 2014-04-16 10:18:47 UTC
tdb locking is just too complex to see this major change
Comment 8 Volker Lendecke 2014-04-16 10:24:30 UTC
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.
Comment 9 Volker Lendecke 2014-04-16 10:28:45 UTC
One more comment: For Samba OEMs we will certainly make patches available, so that the speed improvements can be used in controlled environments.
Comment 10 Jeremy Allison 2014-04-16 15:40:03 UTC
Not letting you off this easy :-). Fundamental flaws can (and will) be fixed. This code needs to go into Samba 4.2.0 :-).

Jeremy.
Comment 11 Jeremy Allison 2014-04-16 15:47:12 UTC
Metze, can you post your summary of the issues with the code so we have a central place to track the fixes ?

Thanks !

Jeremy.
Comment 12 Stefan Metzmacher 2014-04-17 14:52:34 UTC
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.
Comment 13 Stefan Metzmacher 2014-05-13 02:56:32 UTC
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!
Comment 14 Jeremy Allison 2014-05-15 07:10:16 UTC
Are you happy for this to start being merged into master ?
Comment 15 Stefan Metzmacher 2014-05-15 07:53:18 UTC
(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.
Comment 16 Jeremy Allison 2014-05-15 08:50:58 UTC
OK, I'll start the review asap !
Jeremy.
Comment 17 Michael Adam 2014-05-17 12:29:05 UTC
(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
Comment 18 Michael Adam 2014-05-17 21:51:49 UTC
(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.
Comment 19 Jeremy Allison 2014-05-20 12:05:13 UTC
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.
Comment 20 Michael Adam 2014-05-20 13:51:51 UTC
(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
Comment 21 Michael Adam 2014-05-20 13:52:44 UTC
Created attachment 9959 [details]
updated patchset for master (w/ my reviews)

as announced..
Comment 23 Jeremy Allison 2014-05-21 23:20:49 UTC
(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.
Comment 24 Stefan Metzmacher 2014-05-22 06:12:20 UTC
(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.
Comment 25 Stefan Metzmacher 2014-05-22 06:28:56 UTC
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?
Comment 26 Volker Lendecke 2014-05-22 07:07:00 UTC
(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
Comment 27 Stefan Metzmacher 2014-05-22 07:17:54 UTC
Created attachment 9965 [details]
Patches for master

Here's the patchset for master.

Please add your review tags if you want.
Comment 28 Jeremy Allison 2014-05-22 15:56:48 UTC
(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.
Comment 29 Stefan Metzmacher 2014-05-23 13:23:53 UTC
tdb-1.3.0 contains this...