tdb: Do not return errors from tdb_repack() in the tail of tdb_transaction_commit()

The call to tdb_repack() inside tdb_transaction_commit()
is an optimization, not part of the transaction itself,
so failing due to lock or other errors isn't a fatal error
that should cause the caller to think the transaction was
a failure by returning -1.

The tdb transaction itself has finished and been committed
onto stable storage via fsync and all locks released at the
point tdb_repack() is called.

tdb_repack() is only called here as it's a convenient point
to attempt to reduce tdb fragmentation without having to add
a timer call to repack in all users of tdb.

This causes lock ordering issues in Samba, showing up as:

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

This is because Samba has multiple tdb databases open, and the lock order between them
is important.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13952

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>

1 files changed, 23 insertions, 1 deletions

diff --git a/lib/tdb/common/transaction.c b/lib/tdb/common/transaction.c
index 73d02b684a3..b67f84f215d 100644
--- a/lib/tdb/common/transaction.c
+++ b/lib/tdb/common/transaction.c
@@ -1209,7 +1209,29 @@ _PUBLIC_ int tdb_transaction_commit(struct tdb_context *tdb)
 	_tdb_transaction_cancel(tdb);
 
 	if (need_repack) {
-		return tdb_repack(tdb);
+		int ret = tdb_repack(tdb);
+		if (ret != 0) {
+			TDB_LOG((tdb, TDB_DEBUG_FATAL,
+				 __location__ " Failed to repack database (not fatal)\n"));
+		}
+		/*
+		 * Ignore the error.
+		 *
+		 * Why?
+		 *
+		 * We just committed to the DB above, so anything
+		 * written during the transaction is committed, the
+		 * caller needs to know that the long-term state was
+		 * successfully modified.
+		 *
+		 * tdb_repack is an optimization that can fail for
+		 * reasons like lock ordering and we cannot recover
+		 * the transaction lock at this point, having released
+		 * it above.
+		 *
+		 * If we return a failure the caller thinks the
+		 * transaction was rolled back.
+		 */
 	}
 
 	return 0;