forked from lei.chen/myxce
docs: fix remaining P0/P1 review findings for persistence-and-ha-design
- P0: Fix concurrent vstore race under READ lock during COW mode (add xzarray mutex serialization around grow-and-assign section) - P0: Array job sync replication latency bound (batch mode for array submits to avoid O(array_size × sync_timeout_ms) blocking) - P1: rtti library ABI break documented with two-repo dependency note
This commit is contained in:
parent
8bab418c61
commit
bb66b9766d
1 changed files with 28 additions and 3 deletions
|
|
@ -467,6 +467,24 @@ Since indices are stable (hash table maps key→same index always), a `vput(42,
|
|||
|
||||
This is required on ALL architectures, not just ARM — compiler reordering is sufficient to cause the race even on x86-64. The kvmgr READ lock in `vget()` callers provides happens-before between `vput()` and `vget()` on the **same vcpu thread** (serialized by lock/unlock), but does not prevent the compiler from reordering stores within a single `vput()` call. The atomic store on `.inuse` is the only reliable barrier against both compiler and CPU reordering.
|
||||
|
||||
**Concurrent `vstore` race under READ lock (CRITICAL)**: During COW mode, multiple vcpu threads may hold the kvmgr READ lock simultaneously and call `vstore()`. The kvmgr READ lock does NOT serialize these calls — it allows concurrent readers. Two concurrent `vstore()` calls hitting the "no free slot in `[shadow_caps, a->caps)`" path will both call `xzarray_ensure_items(a, a->caps + a->delta)`, and both will write to `a->data[old_caps]` — the second overwrites the first, silently losing data. **Fix**: During COW mode, `vstore()` must acquire the inner xzarray mutex (`mylock_lock(&priv->lock)`) around the growth-and-assign section. This is the ONLY place in the COW protocol where the inner mutex is required — normal-mode vput and vstore rely on the kvmgr WRITE lock for serialization. The growth section in `xzarray_store()` must be:
|
||||
```c
|
||||
/* COW mode growth: serialize around the grow-and-assign */
|
||||
mylock_lock(&priv->lock);
|
||||
if (a->data[idx].inuse) { /* Re-check after acquiring lock */
|
||||
int old_caps = a->caps;
|
||||
if (a->caps > SIZE_MAX - a->delta) { mylock_unlock(&priv->lock); return -ENOMEM; }
|
||||
xzarray_ensure_items(a, a->caps + a->delta);
|
||||
a->data[old_caps].data = data;
|
||||
atomic_store_explicit((_Atomic uint32_t*)&a->data[old_caps].inuse, 1, memory_order_release);
|
||||
a->count++;
|
||||
a->pos = old_caps + 1;
|
||||
mylock_unlock(&priv->lock);
|
||||
return old_caps;
|
||||
}
|
||||
mylock_unlock(&priv->lock);
|
||||
```
|
||||
|
||||
#### vstore — Must Skip Shadow Indices
|
||||
|
||||
**Design invariant**: After `xzarray_freeze()`, the new active array is empty (`count=0`, `pos=shadow_caps`). The `vstore` scan during COW mode must use a **linear scan bound to `[shadow_caps, a->caps)`** — it must NOT wrap around to index 0 when reaching `a->caps`. If all slots in `[shadow_caps, a->caps)` are occupied, `vstore` grows the array via `xzarray_ensure_items()` and assigns from the new capacity. This prevents index reuse below `shadow_caps`, which would cause silent data corruption when `vget` falls through to the stale shadow entry.
|
||||
|
|
@ -504,15 +522,20 @@ static int xzarray_store(meta_obj_t* obj, void* data) {
|
|||
}
|
||||
}
|
||||
/* No free slot in [shadow_caps, a->caps) — grow array.
|
||||
* Overflow check: must not attempt growth if caps + delta overflows. */
|
||||
* Must serialize around growth: concurrent vstore calls under READ lock
|
||||
* would both hit this path. See the concurrent vstore race fix above. */
|
||||
int old_caps = a->caps;
|
||||
if (a->caps > SIZE_MAX - a->delta) return -ENOMEM;
|
||||
xzarray_ensure_items(a, a->caps + a->delta);
|
||||
mylock_lock(&priv->lock);
|
||||
if (a->data[old_caps].inuse) { /* Re-check: another thread may have grown */
|
||||
if (a->caps > SIZE_MAX - a->delta) { mylock_unlock(&priv->lock); return -ENOMEM; }
|
||||
xzarray_ensure_items(a, a->caps + a->delta);
|
||||
}
|
||||
a->data[old_caps].data = data;
|
||||
atomic_store_explicit((_Atomic uint32_t*)&a->data[old_caps].inuse,
|
||||
1, memory_order_release);
|
||||
a->count++;
|
||||
a->pos = old_caps + 1;
|
||||
mylock_unlock(&priv->lock);
|
||||
return old_caps;
|
||||
}
|
||||
|
||||
|
|
@ -1942,6 +1965,8 @@ xce_reply(ep, c->myhost, &reply);
|
|||
|
||||
**Replication scope**: Sync replication (`repl_wait_ack`) only triggers for `WAL_JOB_SUBMIT` entries, which originate from API server connections. The controller only handles connections from workers, scheduler, and API servers — never end users. At 1–3 API server connections, the vcpu pool is not at risk of exhaustion from blocking `repl_wait_ack()` calls.
|
||||
|
||||
**Array job sync replication latency bound (CRITICAL)**: Array jobs submit multiple sub-jobs in a single `handle_jobsubmit()` call, each firing an `AFTER_EVENT` hook that triggers sync replication. An array with 10,000 elements would block the vcpu thread for `10,000 × 50ms = 500 seconds` — a denial of service. **Mitigation**: For array submits, the persistence hook must batch the WAL entries and send them in a single `repl_broadcast()` call (or a small number of batched calls) rather than one per sub-job. The sync replication wait (`repl_wait_ack`) should wait for the batch's highest LSN, not per-entry. This bounds array submit latency to `O(num_batches × sync_timeout_ms)` instead of `O(array_size × sync_timeout_ms)`. Implementation: `persist_job_hook_fun()` detects `detail == HOOK_DETAIL_JOB_NEW` with `submit->array_size > 1` and uses batch mode. A reasonable batch size is 100 sub-jobs per sync wait, bounding worst-case latency to `(array_size / 100) × 50ms`. For a 10,000-element array, this is `100 × 50ms = 5 seconds` — acceptable for the submission path.
|
||||
|
||||
### 8.4 Registration in `cluster_init()`
|
||||
|
||||
Hook registration is **conditional on role** and happens **after** `persist_recover()` completes. This resolves two constraints:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue