Siyao Meng created HDDS-16733:
---------------------------------
Summary: Concurrent open of an absent key on a versioned bucket
drops the first committed version and leaks its blocks
Key: HDDS-16733
URL: https://issues.apache.org/jira/browse/HDDS-16733
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: MC-2-versioned-overwrite-orphans-previous-blocks.patch
h3. Mechanism
Bucket versioning preserves an overwritten version only through block
inheritance done at OPEN time ({{OMKeyRequest.prepareFileInfo}}, the
{{dbKeyInfo != null}} branch at {{OMKeyRequest.java:1013}}), not through
anything commit does. When two writers open the same not-yet-existing key,
neither open key inherits anything.
The first commit publishes its record. The second commit finds {{keyToDelete !=
null}} but, versioning being enabled, skips the overwrite cleanup branch at
{{OMKeyCommitRequest.java:348}} and falls into the {{else}} at {{:391}}, which
only does quota accounting: no old version retention, no deletion enqueue, no
reference tracking. The committed record is built from the second writer's open
key, which never contained the first writer's blocks.
The first version is therefore silently discarded despite versioning being on,
and its blocks are referenced by nothing and queued for deletion by nothing.
The same branch also calls {{incrUsedNamespace(1L)}} for what is really an
overwrite.
h3. Trigger
Bucket {{v/b}} with versioning enabled (settable through the public native
client: {{OzoneBucket.setVersioning(true)}} to {{RpcClient.java:1224}} to
{{OMBucketSetPropertyRequest.java:175-181}}, no gate), key {{k}} absent:
# Writer A sends {{CreateKey v/b/k}}. {{dbKeyInfo == null}}, so A's open key
holds only A's blocks.
# Writer B sends {{CreateKey v/b/k}} before A commits. Still {{dbKeyInfo ==
null}}, so B's open key holds only B's blocks.
# A sends {{CommitKey}}. {{keyToDelete == null}}; key table gets A's record,
version 0, blocks {{{bA}}}.
# B sends {{CommitKey}}. {{keyToDelete}} is A's record, versioning on, so
control goes to the {{else}} at {{:391}}. Key table gets B's record, version 0,
blocks {{{bB}}}. A's version is gone and {{{bA}}} was never enqueued.
Both are plain unconditional PUTs, so {{validateAtomicRewrite}}
({{OMKeyRequest.java:1367}}) is a no-op on this path. Four ordinary requests,
no injected fault.
h3. Impact
The first committed version is unrecoverable and its blocks are leaked
permanently: nothing in the tree scans containers for unreferenced blocks, and
a block absent from {{deletedTable}} is never reclaimed. The bucket's
{{usedNamespace}} and {{usedBytes}} served to users are also inflated, which
{{ozone repair om quota}} can recompute later but which does not reclaim the
blocks.
h3. Developer intent
{{TestOmBlockVersioning}} enables versioning ({{:78}}) and asserts versions
accumulate 0, 1, 2 through the public client ({{:99-135}}), so version
retention on the native path is a promised property and losing a version is
incorrect. Blame on the {{else}} block shows only quota work; nothing discusses
old block ownership on the versioned path. The "versioning is not supported"
notes in the tree are about the S3 API and the MPU path, which is awareness
that versioning is incomplete rather than a report of this defect, and they do
not disable the native flag the commit path honours.
h3. Reproduction
PASS at escalation level 1.
{{repro/test_bugMC-2_versioned_concurrent_open_orphans_blocks.java}}, run as
{{TestSpeculaMC2VersionedConcurrentOpen extends OMKeyRequestTests}} in the
existing {{request/key}} suite, no new cluster.
Versioning ON, the bug lines are the last four:
{noformat}
[MC-2] bucket versioning enabled = true
[MC-2] after A commit, keyTable versions=[version:0 conID 1001 locID 101 || ]
[MC-2] after B commit, keyTable versions=[version:0 conID 1002 locID 102 || ]
[MC-2] blocks referenced by keyTable: [{blockID={conID: 1002 locID: 102}}]
[MC-2] blocks enqueued in deletedTable: []
[MC-2] bucket usedNamespace=2 usedBytes=2000
[MC-2] writer A block conID: 1001 locID: 101 referenced=false
queuedForDelete=false
{noformat}
Negative control, identical interleaving with versioning OFF, which passes:
{noformat}
[MC-2] bucket versioning enabled = false
[MC-2] blocks enqueued in deletedTable: [{blockID={conID: 1001 locID: 101}}]
[MC-2] bucket usedNamespace=1 usedBytes=1000
[MC-2] writer A block conID: 1001 locID: 101 referenced=false
queuedForDelete=true
{noformat}
Expected correct behaviour is the control's: A's blocks either retained as
version 0 of the versioned key, or enqueued in {{deletedTable}}. Flipping only
{{isVersionEnabled}} moves control from {{:348}} (enqueues, quota correct at 1
key and 1000 bytes) to {{:391}} (enqueues nothing, quota inflated to 2 and
2000), so the cause is isolated and a harness artifact is ruled out.
The OM apply path is single threaded by construction, so the interleaving is
chosen rather than raced. The only harness adjustment of note is that the
shared base class's {{allocateBlock}} stub returns one fixed block ID for every
call, which real SCM never does and which would have hidden the leak for a mock
only reason.
h3. Suggested fix
On the versioned commit path, treat {{keyToDelete != null}} as an overwrite
that must preserve or release the previous version's blocks, rather than
falling through to quota accounting only. Either retain the previous record's
version group in the committed record (which is what versioning promises) or
enqueue its blocks in {{deletedTable}} as the non versioned branch does. The
quota call on that branch should also stop counting an overwrite as a new
namespace entry.
Add coverage to the existing {{request/key}} suite: two writers opening the
same absent key on a versioned bucket, asserting after both commits that every
block from the first commit is either referenced by {{keyTable}} or present in
{{deletedTable}}, and that {{usedNamespace}} is 1.
h3. Patch
[^MC-2-versioned-overwrite-orphans-previous-blocks.patch], against
20e6a1c0f6039248373efd3eb1d7f2afcf7f1535.
Widens the overwrite cleanup branch in {{OMKeyCommitRequest}} and
{{OMKeyCommitRequestWithFSO}} from {{keyToDelete != null &&
!getIsVersionEnabled()}} to also cover the versioned case where the committed
record carries over none of the previous record's blocks, which is exactly the
concurrent open shape. A new shared {{hasBlocksNotCarriedOver}} in
{{OMKeyRequest}} is the predicate. An ordinary versioned overwrite, where the
open key did inherit the previous version, still takes the untouched {{else}}
branch, so version retention and its quota arithmetic are unchanged. On the
orphaning path, {{filterOutBlocksStillInUse}} keeps any block the new record
does share, and the branch's existing {{decrUsedNamespace}} plus
{{incrUsedNamespace}} arithmetic nets the inflated namespace count back to 1.
Scope note, for reviewers to decide: this makes the first version's blocks
reclaimable and fixes the namespace count. It does not add true version
retention, which would mean renumbering the committing writer's version group
and prepending the previous one, a change to version numbering semantics that
belongs in its own issue. After this patch the first version is still lost, as
it is today, but its blocks are no longer leaked.
True version retention is also in scope for the active S3 object versioning
work (HDDS-15728 and its T1 to T7 subtasks), so that half may belong there
rather than in a separate fix.
Covered by {{testVersionedConcurrentOpenDoesNotOrphanTheFirstVersionsBlocks}}
in the existing {{TestOMKeyCommitRequest}}, inherited by the FSO subclass. Two
writers commit the same absent key on a versioned bucket; the test asserts the
first writer's block is enqueued for deletion and that {{usedNamespace}} is 1.
Verified to fail without the change and pass with it on OBS and FSO. Checkstyle
clean. The 23 tests in each of those two classes pass.
Found by TLA+ model checking and code review of the OM conditional create,
overwrite and delete paths under HDDS-15926, on commit
20e6a1c0f6039248373efd3eb1d7f2afcf7f1535. Checked against open HDDS issues for
duplicates before filing, in addition to the repository's merged git history.
Generated with Specula (Claude Opus 5).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
