refactor(mds): unify inode/partition/dirshard version tracking - #1116
Merged
Merged
Conversation
…ev-deploy dev-regression-test duplicated the test-loop guidance already in automate-test while also hand-holding each tool's CLI. Fold the script-uncovered tools (xfstests, vdbench) into automate-test with explicit pass/fail criteria and drop the redundant skill. dev-deploy is rewritten to document only the knobs the scripts actually consume (mds_deploy_parameters.local, which is gitignored), replace the unreliable `ps -ef | grep` checks with pgrep/mountpoint, and add the missing create_fs step and smoke test.
rock-git
force-pushed
the
fix/main_091701
branch
from
September 20, 2026 05:49
5533a59 to
9d483a2
Compare
rock-git
enabled auto-merge
September 20, 2026 05:49
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate findings affect version correctness, cache rendering, retry safety, storage conflicts, and test reliability.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 4
Open (7)
What changed in this PR
This PR unifies MDS version tracking with per-slot AttrVersionVec, adds optimistic conflict handling and retry-safe mutations, and updates S3 configuration, diagnostics, tests, and tooling.
Changes:
- Replaces scalar inode, partition, shard, and dentry-operation versions with version vectors.
- Adds DummyStorage conflict detection and expanded filesystem/version tests.
- Updates cache rendering, S3 flag wiring, scripts, and development guidance.
| File | Summary | Review findings |
|---|---|---|
test/unit/mds/storage/test_dummy_storage.cc |
Tests transaction conflict behavior. | None. |
test/unit/mds/filesystem/test_partition.cc |
Tests partition and shard version vectors. | None. |
test/unit/mds/filesystem/test_inode.cc |
Tests inode version merging. | None. |
test/unit/mds/filesystem/test_filesystem_state.cc |
Adds filesystem state and concurrency coverage. | Moderate (2 votes, line 847): conflict retries can make the concurrent test flaky. Moderate (1 vote, line 122): SimpleWorkerSet teardown does not stop the worker before destruction. |
test/unit/mds/common/test_type.cc |
Tests version-vector behavior. | None. |
test/unit/common/test_config_mapper.cc |
Tests S3 SDK configuration mapping. | None. |
src/tools/mds-cli/br.cc |
Applies AWS SDK flag configuration. | None. |
src/mds/storage/dummy_storage.h |
Adds transaction version bookkeeping APIs. | None. |
src/mds/storage/dummy_storage.cc |
Implements versioned reads and conflict checks. | Critical (3 votes, line 240): direct Put/Delete paths do not update version bookkeeping. Moderate (1 vote, line 313): unconditional random delays make tests nondeterministic and slower. |
src/mds/storage/dingodb_storage.cc |
Handles failed transaction creation. | None. |
src/mds/service/fsstat_service.cc |
Updates cache version rendering. | Moderate (2 votes, line 745): string versions are converted with asUInt64(), rendering loaded shard versions as zero. |
src/mds/filesystem/store_operation.h |
Documents operation mutation behavior. | None. |
src/mds/filesystem/store_operation.cc |
Resets retryable operation state. | Critical (1 vote, line 3861): retry reset leaves stale attr_with_mutation.attr data. |
src/mds/filesystem/partition.h |
Defines vector-based partition and shard versions. | None. |
src/mds/filesystem/partition.cc |
Implements version-vector merging and replay. | Moderate (2 votes, line 392): shard-page rendering still reads removed base_version/delta_version keys. Moderate (1 vote, line 471): pending operation versions are strings but rendered with asUInt64(). |
src/mds/filesystem/inode.h |
Stores inode version vectors. | None. |
src/mds/filesystem/inode.cc |
Applies monotonic version updates. | None. |
src/mds/filesystem/filesystem.h |
Updates partition version APIs. | None. |
src/mds/filesystem/filesystem.cc |
Propagates operation versions and rename cache updates. | Critical (1 vote, lines 3889/4094): scalar CompleteVersion() comparisons can collide across distinct version slots and reuse stale partition caches. |
src/mds/common/type.h |
Adds unified version types and mutation conversion. | Critical (3 votes, lines 181/191): sparse mutation entries are incorrectly accessed as a slot-indexed vector, producing wrong values or out-of-bounds access. |
src/common/config_mapper.h |
Fills AWS SDK options from flags. | None. |
src/client/vfs/metasystem/mds/file_session.cc |
Removes an obsolete comment. | None. |
scripts/dev-mds/run_all_test.sh |
Adds repeated test rounds and result checks. | None. |
.agents/skills/dev-regression-test/SKILL.md |
Removes superseded regression guidance. | None. |
.agents/skills/dev-deploy/SKILL.md |
Updates deployment guidance. | None. |
.agents/skills/automate-test/SKILL.md |
Consolidates testing guidance. | None. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+181
to
+185
| uint64_t DeltaVersion(uint32_t index) const { | ||
| CHECK(index < mutations.size()) << "invalid mutation index(" << index << "), should be less than " | ||
| << mutations.size(); | ||
| return mutations[index].delta_version(); | ||
| } |
| } | ||
|
|
||
| uint64_t cache_version = use_base_version ? partition->BaseVersion() : partition->DeltaVersion(); | ||
| uint64_t cache_version = use_base_version ? partition->BaseVersion() : partition->CompleteVersion(); |
| Status ScanDirShardOperation::Run(TxnUPtr& txn) { | ||
| // RunAlone retries Run on conflict; clear appended mutations so a retry does | ||
| // not accumulate duplicate slots. | ||
| result_.attr_with_mutation.mutations.clear(); |
Comment on lines
+240
to
+247
| uint64_t new_version = ++commit_version_; | ||
| for (const auto& [key, kv] : writes) { | ||
| if (kv.opt_type == KeyValue::OpType::kPut) { | ||
| data_[key] = kv.value; | ||
| } else if (kv.opt_type == KeyValue::OpType::kDelete) { | ||
| data_.erase(key); | ||
| } | ||
| key_versions_[key] = new_version; |
| value["ino"] = ino_; | ||
| value["base_version"] = base_version_; | ||
| value["delta_version"] = delta_version_; | ||
| value["version"] = version_vec_.ToString(); |
| RenderCachePageStart("Partition Cache", fs_id, os); | ||
| os << fmt::format("<h3>Partition ino {}: base version {}, delta version {}</h3>", ino, | ||
| value["base_version"].asUInt64(), value["delta_version"].asUInt64()); | ||
| os << fmt::format("<h3>Partition ino {}: version {}</h3>", ino, value["version"].asString()); |
| param.name = fmt::format("cc_{}_{}", t, i); | ||
| param.mode = 0777; | ||
| EntryWithPaOut out; | ||
| if (fs_->MkNod(ctx, param, out).ok()) ok_count.fetch_add(1); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A directory's version was tracked in 3 shapes at once:
Inodeheldbase_version_plus a per-slotdelta_versions_vector plus atotal_delta_version_sum,ShardPartitionheld its ownbase_version_/delta_version_pair, andDirShardheld a bareuint64_t. Partition dentry ops carried a scalar version too, so an op produced by a shard mutation was recorded against a complete version rather than its own slot, and the op replay could not distinguish "this shard is already past op N" from "this shard is past op N in a different slot".Version is now one type,
AttrVersionVec(base version + one delta per mutation slot + cached total), merged monotonically per slot byPutIf. It is the only version representation inInode,ShardPartition,DirShard, and partition dentry ops, and dentry ops carry anAttrVersionthat is either a base version or an indexed delta.Changes
src/mds/common/type.haddsAttrVersionandAttrVersionVec, plusAttrWithMutation::ToAttrVersion().TotalDeltaVersion()now dedups mutations by slot (max per slot) instead of summing, so a mutation list that accumulated a duplicate slot no longer inflates the version.AttrVersionVec::PutIfhas overloads forAttrMutationEntry,AttrVersion,AttrEntry,AttrWithMutation, andAttrVersionVec, andLessThanOrEqual/GreaterThanOrEqualcompare a single version against the right slot.Inodecollapsesbase_version_/delta_versions_/total_delta_version_intoversion_vec_.Version()becomesCompleteVersion(), andVersionVec()exposes a copy of the whole vector so a caller can compare an inode against a partition slot by slot.ShardPartition/DirShardcarry anAttrVersionVec.Put/Delete/RefreshVersiontake anAttrVersion,DeltaVersion()becomesCompleteVersion()withVersionVec()alongside, andApplyDeltaOpNoLockskips an op withshard->VersionVec().GreaterThanOrEqual(op.version)instead of a scalar compare, which makes the insertion sort ofdelta_dentry_ops_unnecessary (AddDeltaOpNoLockdrops it).ShardPartition::Newgains anAttrWithMutationoverload soDoFetchPartitionno longer builds aToCompleteAttr()copy just to construct the partition, andDoFetchDirShardbuilds the shard version from the rawAttrWithMutationfor the same reason.FileSystempasses the parent'sAttrVersion(fromAttrWithMutation::ToAttrVersion()) intoAddDentryToPartition/DeleteDentryFromPartition/RefreshPartitionVersioninstead of the in-memory inode's current version, so a partition dentry op is stamped with the exact version component the operation produced.FileSystem::Renamenow invalidates the old parent's partition cache only when the parent actually changes. A same-parent rename no longer tears down the entry it just wrote a DELETE op into.RenameOperation,ScanDirShardOperation,GetInodeAttrOperation) clear accumulatedmutationsat the start ofRun. These operations run underRunAlone, which retriesRunon conflict; without the clear, a retry appended every mutation a second time andToCompleteAttr()doubled the version.DummyStoragegains versioned reads and write-conflict detection at commit (ESTORE_MAYBE_RETRYfor a key that changed since the txn read it). The in-memory backend used by the MDS unit tests now exercises the same optimistic concurrency as DingoStore and TiKV rather than silently doing last-write-wins, which is what let the retry bug above hide.src/common/config_mapper.handmds-cli/br.ccfillaws_sdk_configfrom the--s3_*gflags for S3 filesystems. Without it the AWS SDK fell back to an empty log prefix and wrote<cwd>/<YYYY-MM-DD-HH>.log.DirShard::Dump/ShardPartition::Dumpemit a singleversionstring ("<base> <total>") instead ofbase_version/delta_versionnumbers, and the FsStat partition page renders it as one value. The inode cache page and partition list page keep the previous base/complete split under the new accessor names.Two unrelated commits are folded in:
automate-testabsorbs the xfstests/vdbench guidance from the deleteddev-regression-testskill, anddev-deploydocuments only the knobs the deploy scripts actually read.Test plan
New and extended coverage:
test/unit/mds/common/test_type.cc(new) covers everyAttrVersionVecconstructor andPutIfoverload, per-slot dedup of repeated mutations, and the range comparisons.test/unit/mds/filesystem/test_filesystem_state.cc(new, largest addition) drives the realFileSystemagainstDummyStorageand asserts version arithmetic per operation: create/mkdir/batch variants advance the parent exactly once, nlink moves only where it should, symlink bumps the version but not nlink, read paths change nothing, setattr/setxattr/removexattr refresh the partition, rename advances one or both parents, and long-running concurrent mixes keep inode and partition versions consistent.test/unit/mds/filesystem/test_inode.ccandtest_partition.cccover monotonic base/complete/delta versions, shard split preserving the version vector, partition cache merge of a newer vector, and delta op replay leaving covered ops behind.test/unit/mds/storage/test_dummy_storage.cccovers the new txn conflict semantics: stale read-modify-write conflicts, blind writes do not, recreate after delete does not.test/unit/common/test_config_mapper.cccovers the new S3 SDK config fill.