The implementation review of the lock fixes found test flaws, which are
corrected here; the production behavior is unchanged.
- The alias and merge tests replaced globalOperationTimeout while
background healing reads it, a data race under -race. They now bound
each request with its own context instead.
- The alias test now holds the cleaned name's write lock while the copy
runs, so it proves the names are checked before anything is locked;
it checks both objects before any later write, and covers a/./x as a
destination.
- The merge test picks aliases that hash to the clean name's set and,
where sets have their own lockers, ones that do not, and requires the
alias's own table to hold a read lock: readers admitted, writers
excluded.
- Release polling creates a fresh lock instance for every probe.
- The getNSLocks event hook and the bulk delete group limit are atomic,
so tests can set them while other goroutines read them.
Without the name check and the resource merge, both tests now fail in
every lock layout.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>
The single-pool lock-domain change made a bulk delete lock each set's
names in that set and ran the groups one after another: a batch over S
sets cost about S lock round trips plus S set deletes, where the release
took one lock and then let erasureSets.DeleteObjects work on all sets at
once. Every distributed single pool with several sets paid for it,
including layouts where every set uses every host.
Run the groups concurrently, up to 16 at a time. Each group takes only
its own set's lock and waits for no other while it holds it, so the
groups cannot hold each other. A group checks for cancellation once it
is admitted, so a queued group of a canceled request does no storage
work. Each result is written only by the group of its object.
Tests: the first drive calls of two sets must overlap, which fails when
the groups run one after the other; with one worker, a group in flight
at a cancel keeps its own result, an earlier group keeps its success,
and a queued group makes no drive call.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>
getNSLocks takes the locks of a copy in one order and waited for a later
lock, such as the source, for up to two seconds while it held the earlier
ones, such as the destination; then it released them, paused up to 250 ms
and tried again until the operation timeout. When the source was write
locked for long (a multi-pool PUT for its whole upload, a heal, a
transition, another copy onto it), the copy held its destination write
lock most of the time: reads and writes of the destination waited for
seconds, their own backoff rarely hitting the gaps. In the release a copy
that waited for its source held nothing.
After a bounded wait for an atomic lock times out while earlier locks are
held, release them, wait for that lock alone with a fresh instance until
the deadline, release it once it is free, and start again from the first
lock in the same order. Holding nothing while waiting forms no cycle, and
the next attempt still takes every lock in one order with fresh
instances. A non-atomic request may hold some of its own names while it
waits, so it keeps the bounded retries.
A test hook reports the bounded timeout, the start of the idle wait and
its end, so the tests coordinate on events rather than timing: a lock
before the contended one is free for others for the whole wait, a cancel
or the deadline ends the idle wait and leaves nothing held, a non-atomic
request never waits alone, and HEAD of a copy's destination succeeds
while the copy waits for its source. All but the non-atomic control fail
without the idle wait.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>
CopyObject locks its source and destination before it reads the source,
and lockObjects merged two requests only when their raw names were equal.
A lock is taken on the cleaned name, so an invalid source such as a//x or
a/./x and the destination a/x asked for a read lock and a write lock on
one resource: the copy waited on itself until the operation timeout and
answered 503, holding the key's lock in short windows meanwhile. Before
the lock-domain change the invalid source was refused at once, when the
source was read. This reached single nodes, multi-pool deployments and
single pools whose sets share hosts.
- Check the source with checkGetObjArgs before any lock, the first check
and the first error the release reported. Lock the destination only
when its name is valid. An invalid destination fails at the copy's own
destination check before it writes, after a missing source (404) or a
failed source precondition (412), as before.
- lockObjects merges requests by the resource the lockers see, the
cleaned bucket/object, within one lock table: the object's set where
the sets of a pool have their own lockers, else one table. This keeps
aliases on a shared local table from conflicting and never merges
requests of different distributed domains.
Tests: aliased sources and destinations are refused with 400 at once in
every lock layout and two pools; an invalid destination keeps the 404
and 412 precedence; lockObjects merges an alias with its clean name. The
alias and merge tests fail without the change.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>
In a single pool, DELETE in every form, bulk delete, the CopyObject
destination, SelectObjectContent, batch key rotation and the upload-ID
guards of complete, abort, get-info and list-parts locked through the
first erasure set, while PUT, multipart commit, PutObjectPart, tag,
retention, legal-hold and metadata updates, restore, transition and heal
lock in the set the object hashes to. When the sets of a pool use
different hosts, as every automatic layout of 18 or more hosts does, the
two locks ask different lockers and do not exclude each other, and
concurrent read-modify-writes of xl.meta can lose acknowledged versions
and delete markers or leave an object on a minority of drives.
Route these locks through NewObjectNSLock, which returns the lock of
the object's own set in a single pool and the pools lock otherwise.
Upload guards choose the set by their object and keep the name
object/uploadID. True prefix deletion and the locks of names that are
not objects keep their domain.
Bulk delete groups its names by set and takes one set's lock at a time,
in ascending set index; a failed group fails alone. A single node, whose
sets share one local lock table, keeps its one bulk lock.
The COPY handler takes its source and destination locks before it opens
the source, sorted, and transactionally: bounded waits while holding a
lock, release in reverse order and retry with new lock instances, one
deadline. It reads the source under them, passes NoLock to every nested
call, and runs that work in the locks' context. This also removes a
deadlock of copies in opposite directions wherever both locks were in
one domain, such as a single node or several pools.
Lock names and RPCs are unchanged, but a pool is protected only when
every node runs this change: affected pools must be upgraded and rolled
back with a drained restart.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>
Feng Ruohang