Issue 7462 - Fix SERIAL LOCK and entry cache lock ordering on BDB dea…

[vashirov]

…dlock retry

Bug Description:
In IPA context, server can freeze under heavy write load (add/mod/del users and hosts, deferred memberof enabled).

I found 2 issues:

1. In deadlock retry path, a thread grabs SERIAL LOCK, during deadlock sleeps still holding the SERIAL LOCK. Every other writer thread waits for that sleep to finish.
2. After fixing the first issue, I encountered lock ordering issues with SERIAL LOCK and entry locks. On retry Thread A holds entry lock from the previous attempt, tries to grab SERIAL LOCK. Thread B holds SERIAL LOCK, wants the same entry lock. Neither can proceed, resulting in a deadlock.

Fix Description:

1. Release SERIAL LOCK before backoff sleep on BDB deadlock retry and cache lock retry paths.
2. Fix AB-BA deadlock between SERIAL LOCK and entry cache locks by releasing/re-acquiring entry locks around retry.

Fixes: #7462

Summary by Sourcery

Adjust BDB transaction retry handling to avoid deadlocks involving the global SERIAL LOCK and entry cache locks during high-write workloads.

Bug Fixes:

• Release the SERIAL LOCK before backoff sleeps on BDB deadlock and cache lock retry paths so other writers are not blocked behind a sleeping thread.
• Resolve AB-BA deadlocks between the SERIAL LOCK and entry cache locks by unlocking entries before aborting on retry and re-locking them after re-establishing the transaction.
• Prevent double-unlock and inconsistent cache state on error paths by tracking whether entry and parent cache locks are held before unlocking or returning entries to the cache.

Enhancements:

• Standardize transaction retry behavior in add, delete, modify, and modrdn operations to always acquire the SERIAL LOCK consistently before (re)locking entry cache objects.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The retry paths now contain a lot of duplicated logic for unlocking entry-cache locks, aborting the txn, sleeping, then re-locking (with similar error handling); consider factoring this pattern into small helper functions to reduce complexity and lower the risk of divergence between add/modify/delete/modrdn implementations.
• You’ve introduced several *_locked flags that are updated in many branches; it might be worth double-checking and consolidating the ownership/lifecycle handling (e.g., via small helpers that pair cache_lock_entry with flag updates) to make it harder to accidentally get the flag and the actual lock state out of sync in future changes.
• In ldbm_back_modify, the new error log on failed re-lock ("Failed to re-lock entry after deadlock retry") omits the conn_id/op_id context that the other new log messages include; consider adding those identifiers for consistency and easier debugging.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The retry paths now contain a lot of duplicated logic for unlocking entry-cache locks, aborting the txn, sleeping, then re-locking (with similar error handling); consider factoring this pattern into small helper functions to reduce complexity and lower the risk of divergence between add/modify/delete/modrdn implementations.
- You’ve introduced several `*_locked` flags that are updated in many branches; it might be worth double-checking and consolidating the ownership/lifecycle handling (e.g., via small helpers that pair `cache_lock_entry` with flag updates) to make it harder to accidentally get the flag and the actual lock state out of sync in future changes.
- In `ldbm_back_modify`, the new error log on failed re-lock (`"Failed to re-lock entry after deadlock retry"`) omits the `conn_id`/`op_id` context that the other new log messages include; consider adding those identifiers for consistency and easier debugging.

## Individual Comments

### Comment 1
<location path="ldap/servers/slapd/back-ldbm/ldbm_modify.c" line_range="708-709" />
<code_context>
+        if (retry_count > 0 && e) {
+            int lock_rc = cache_lock_entry(&inst->inst_cache, e);
+            if (lock_rc != 0) {
+                slapi_log_err(SLAPI_LOG_ERR, "ldbm_back_modify",
+                              "Failed to re-lock entry after deadlock "
+                              "retry (rc=%d)\n", lock_rc);
+                CACHE_RETURN(&inst->inst_cache, &e);
</code_context>
<issue_to_address>
**suggestion:** Log message for re-lock failure in modify lacks connection/operation context, unlike the other new log messages.

To keep logging consistent and aid correlation in production, please include `conn_id` and `op_id` in this re-lock failure message as well, similar to the re-lock logs in `ldbm_back_add`, `ldbm_back_delete`, and `ldbm_back_modrdn`.

Suggested implementation:

```c
            int lock_rc = cache_lock_entry(&inst->inst_cache, e);
            if (lock_rc != 0) {
                slapi_log_err(SLAPI_LOG_ERR, "ldbm_back_modify",
                              "conn=%" PRIu64 " op=%d Failed to re-lock entry after deadlock "
                              "retry (rc=%d)\n",
                              conn_id, op_id, lock_rc);

```

This change assumes that `conn_id` and `op_id` are already available in this scope as in the other logging sites you referenced, and that `<inttypes.h>`/`PRIu64` are already included for the file (as is common elsewhere in this codebase). If the existing re-lock logs in `ldbm_back_add`, `ldbm_back_delete`, or `ldbm_back_modrdn` use a slightly different format string (e.g. including `ldbm_back_modify -` or different spacing), you should align this message to match that exact pattern for consistency.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[packit-as-a-service]

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

• sudo dnf install -y 'dnf*-command(copr)'
• dnf copr enable packit/389ds-389-ds-base-7463
• And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

[progier389]

Looks good except a minor point (use bool for the flags instead of int32_t)

[progier389]

Should better use bool and true/false

[droideck]

Overall, looks great!

Regarding testing, do you think it's possible to reproduce it via stress test?
Something that spawns N writer threads doing add/mod/del under deferred-memberof probably would exercise the retry path. But that's a stretch and it's not a blocker for the PR merge.

[droideck]

Do I understand correctly that for MANAGE_ENTRY_BEFORE_DBLOCK case on iter 1 we acquire E first and then SERIAL LOCK? But then on iter 2 -- it's another way around (SERIAL LOCK first and then E)?

IIUC, even though very rarely (and especially because MANAGE_ENTRY_BEFORE_DBLOCK is not a default mode but also experimental mode) but we still might have the AB-BA deadlock here.

[mreynolds389]

Perhaps when (!MANAGE_ENTRY_BEFORE_DBLOCK) we should still lock the entry if it is not locked already (near line 721). I think that will fix the ordering when using that db opt level.

if  (!MANAGE_ENTRY_BEFORE_DBLOCK) {
   ...
} else if (e && !e_locked) {
    cache_lock_entry(&inst->inst_cache, e);
    e_locked = 1;
}

[tbordaz]

I wonder if there is not a confusion between backend lock and db_env lock.
Initially the backend lock was held during all lmdb_add and db_env release during retry/sleep.
With the move of plugins in betxn (#351) it was changed from dblayer_txn_abort (release the db_env) to dblayer_txn_abort_ext(...FALSE) (to not release the db_env) but the comment was about 'Serial Lock' that in my mind means backend lock.
Of course we do not want to release the backend lock for a retry but it looks fine to me to release db_env.

What is not clear to me is the impact of releasing it. IIUC it allows others txn but on different backend. Is it the expected benefit ?

[aadhikar]

Hey @vashirov, could you address the outstanding review comments and rebase? Thanks!

[vashirov]

@aadhikar, it's ready to be merged, but waiting for slapi-nis PR to be merged https://codeberg.org/freeipa/slapi-nis/pulls/71 at the same time, otherwise this will break FreeIPA nightly CI

[droideck]

IIUC, if a concurrent modify touches the parent during our backoff, modify_switch_entries() goes through cache_replace(), which marks the old backentry ENTRY_STATE_DELETED. Our pointer still references that old backentry, so cache_lock_entry() here returns RETRY_CACHE_LOCK and we turn it into LDAP_OPERATIONS_ERROR.
Previously, the concurrent writer just blocked on the lock and both operations succeeded; now the retrying add fails with err=1. And since a deadlock retry implies a concurrent writer, this fires exactly on the collision path the patch is smoothing, unless I miss something.
Same shape in modify, delete, and modrdn.

Maybe, on a RETRY_CACHE_LOCK we can refetch the entry by DN and restart the attempt?
And fail only if the entry is genuinely gone (RETRY_CACHE_LOCK also covers real deletion, so the refetch distinguishes the two for free).

[droideck]

The preamble releases e, parent, and newparent, but moddn_get_children() (inside the run-once block) takes cache_lock_entry() on every cached child

389-ds-base/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c

Lines 2186 to 2191 in 65a1afc

	e = cache_find_id(&inst->inst_cache, id);
	if (e != NULL) {
	cache_lock_entry(&inst->inst_cache, e);
	(*child_entries)[entrynum] = e;
	entrynum++;
	}

And those are only released in the completion paths

389-ds-base/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c

Lines 1301 to 1305 in 65a1afc

	for (i = 0; child_entries[i] != NULL; i++) {
	CACHE_REMOVE(&inst->inst_cache, child_entries[i]);
	cache_unlock_entry(&inst->inst_cache, child_entries[i]);
	CACHE_RETURN(&inst->inst_cache, &child_entries[i]);
	}

So a subtree rename sleeps holding N child monitors and then waits for the serial lock, while a modify of one of those children holds the serial lock and waits for the child's monitor. IIUC, that is the same AB-BA this patch removes for the top entries, moved to the children.
And cached tombstone children count too, so it is not limited to big subtrees.

Possible, we can release the child locks in the preamble as well, so nothing entry-level is held across the backoff, and re-lock (or refetch) them after the serial lock is re-acquired. What do you think?

[droideck]

This dup (and the SDN read at :231) runs after e was unlocked a few lines above. The refcount keeps the backentry struct alive, but not ep_entry: the URP path at :421 does slapi_entry_free(e->ep_entry) and replaces it in place, under a lock we no longer hold. A replicated delete of the same entry during our backoff makes this read freed memory. Admittedly hard to hit, but the fix is cheap.

[droideck]

Small commit note...
This restart triggers on RETRY_CACHE_LOCK, hence this part is backend-independent, so it is live on LMDB. And it looks like an improvement! (the old code slept holding what on LMDB is the single write transaction). Worth reflecting in the commit message, IMO.:)

[droideck]

After my dive into the DB retry stuff, I decided to dig a bit deeper into this one... (as it might help another case with memberof massive writes) And... Well, you see what came out of it...
Sorry for the "changed my mind" recheck :(

Hopefully, some of the findings are worthy though!