Bug 13952 - tdb_transaction_commit() contains a hidden, rarely triggered second transaction for tdb_repack()
Summary: tdb_transaction_commit() contains a hidden, rarely triggered second transacti...
Status: ASSIGNED
Alias: None
Product: TDB
Classification: Unclassified
Component: libtdb (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Andrew Bartlett
QA Contact: Samba QA Contact
URL: https://gitlab.com/samba-team/samba/m...
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-16 04:31 UTC by Andrew Bartlett
Modified: 2019-05-17 02:28 UTC (History)
2 users (show)

See Also:
abartlet: review? (jra)


Attachments
First iteration patch under test in the merge request (15.31 KB, patch)
2019-05-16 04:35 UTC, Andrew Bartlett
abartlet: review? (jra)
Details
Slightly modified version (15.02 KB, patch)
2019-05-16 18:21 UTC, Jeremy Allison
jra: review? (abartlet)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2019-05-16 04:31:18 UTC
In tdb_transaction_commit() the DB will occasionally repack.

To do this, after the transaction locks are released to other callers, a new tdb_transaction is opened, used and attempted to be closed.

This fails if the lock order is violated, eg:

 ltdb: tdb(../private/sam.ldb.d/DC=SAMBA,DC=EXAMPLE,DC=COM.ldb): tdb_transaction_prepare_commit: failed to upgrade hash locks: Locking error

ldb: ltdb: tdb(../private/sam.ldb.d/DC=SAMBA,DC=EXAMPLE,DC=COM.ldb): ../../lib/tdb/common/tdb.c:1120 Failed to commit
Comment 1 Andrew Bartlett 2019-05-16 04:35:56 UTC
Created attachment 15160 [details]
First iteration patch under test in the merge request

Jeremy,

Can you take a very careful look at this.

I've not dared trying to rework TDB to do this without giving up the transaction locks, but that would also be a reasonable approach.
Comment 2 Jeremy Allison 2019-05-16 17:17:23 UTC
This original code was added by tridge as:

commit a386173fa1c7c5bcc11ea9260d84b6c52c154b3d
Author: Andrew Tridgell <tridge@samba.org>
Date:   Mon Jun 1 13:11:39 2009 +1000

    auto-repack in transactions that expand the tdb
    
    The idea behind this is to recover from badly fragmented free
    lists. Choosing the point where the file expands is fairly arbitrary,
    but seems to work well.

so it's been there for a long time. I see why he placed it where it is, but there's no reason it *has* to be in the exit path of 
tdb_transaction_commit(), after the transaction has been committed successfully.

Initially, your patch certainly looks like it will do the right thing, but I'd like to discuss more first, to see if there are other options we might look at.
Comment 3 Jeremy Allison 2019-05-16 18:21:28 UTC
Created attachment 15162 [details]
Slightly modified version

So I'd be slightly happier with the following, as it only fails the transaction if tdb_repack() returns -1 and tdb->ecode != TDB_ERR_LOCK, which I think might be safer. Otherwise we might miss an error in write inside the repack code.

Also adds your patch #2 to make a complete replacement.

What do you think ? Happy to discuss on the phone.
Comment 4 Jeremy Allison 2019-05-16 18:37:02 UTC
Another thing we might think about is adding in a tdb_repack() on the ldb tdb databases as a periodic (60 seconds?) timer in the main samba ldap server code, outside of any transactions (possibly from the main daemon). Again, ignore any errors except for lock errors.

ctdb_vacuum.c already does this (not on a timer).
Comment 5 Jeremy Allison 2019-05-16 18:37:35 UTC
Sorry, meant to add:

ctdb_vacuum.c already does this (not on a timer) but on child connection after fork.
Comment 6 Jeremy Allison 2019-05-16 19:00:44 UTC
FYI, I also looked at moving the tdb_repack() call to just before the call to _tdb_transaction_cancel() - which might also be a solution without having to check for lock order violations.

However we'd then also need to check that the flag TDB_DISALLOW_NESTING isn't set for the tdb, otherwise the nested transaction request inside tdb_repack() would always fail.

That would mean that any tdb opened with TDB_DISALLOW_NESTING doesn't get implicit repack in tdb_transaction_commit() calls. This seems to be heavily used inside ctdb, so we'd need to consult with the ctdb folks before landing that alternate change.

Your patch (or my variation thereof) looks safer IMHO.
Comment 7 Andrew Bartlett 2019-05-16 19:22:00 UTC
Yeah, LDB sets TDB_DISALLOW_NESTING because it does it's own (even dodgier) nesting.  It would have to be a special modification of the code that rebuilds the memory/disk side of the transaction but not the locking side.

I'm also quite concerned however that we should NEVER return an error from tdb_transaction_commit() after successfully writing data, as that would mislead the application and client, which needs to know what is in the DB.

I'll give you a call.
Comment 8 Andrew Bartlett 2019-05-17 01:43:16 UTC
(In reply to Jeremy Allison from comment #6)
Thanks for taking the phone call.  There we agreed that TDB should never return tdb_repack() errors on the basis that the transaction that was successfully written must return success to the callers, even if the repack went very badly.

You asked that I try to make repack compulsory to determine if Samba can ever repack.  The answer is yes, we can perform some operations, so repack is happening except when we loose the race.

I'll therefore propose my patch.  It may still cause lock order issues elsewhere but it should be rarer. 

We can later determine if it is possible to do the repack while still holding the lock, after locking away the user's original work.