Skip to content

Add monad-mpt --rescan-devices - #2532

Open
maxkozlovsky wants to merge 1 commit into
mainfrom
max/device-resize-v2
Open

maxkozlovsky wants to merge 1 commit into
mainfrom
max/device-resize-v2

Conversation

@maxkozlovsky

Copy link
Copy Markdown
Contributor

Growing a node's database meant --archive then --restore into a freshly created larger pool: scratch space, plus a full dump and restore. Extending a device the pool already owned was worse than unsupported -- the metadata saying a device belongs to a pool lives at its very end, so lvextend left it unopenable, with no resize2fs step to follow.

--rescan-devices reconciles a database with whatever storage it is now given: --storage lists every device it should contain, and one offline run joins any blank suffix and takes up the space of an extended last device, under one confirmation and one metadata growth.

Taking up an extend needs the previous size of the device, which the extend itself strands mid-device, so db_metadata now records every device's size on each writable open, in padding it already reserves -- no magic bump, no migration. That recording is the only source, and is acted on only where the stranded footer's config_hash confirms it, so an extend must be preceded by a writable open under a release carrying this code.

Refusing two sources that name one device is needed here, but the hole is not new: creating a pool with a device listed twice was silently accepted, and no later open could detect it. That check goes in the constructor, for every mode.

Comment thread category/mpt/cli_tool_impl.cpp
@maxkozlovsky
maxkozlovsky force-pushed the max/test-pool-disk-usage branch from e0d4bc1 to d13ac80 Compare September 1, 2026 22:26

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed per REVIEW.md: traits (n/a — no category/execution or category/vm changes), FFI boundary (untouched — no extern "C" header, RLP, or monad-bft-facing changes), C++ correctness, crash safety, testing, style, PR hygiene, and security.

What I verified in depth:

  • Hash compatibility. compute_unique_hash_ and compute_config_hash_ reproduce the pre-existing FNV1a fold order and truncations exactly (including the low-32-bits-of-size fold and the two-pass unique-hash/chunks+capacity ordering in fill_chunks_), so existing pools keep their identities. The golden-value test (config_hash_formula_is_pinned) pins this.
  • Crash-ordering discipline. The commit protocol is sound at every interruption point I traced: members are stamped with the target hash before the grown device's relocated footer commits; the relocated bytes-used array is written and synced before the footer that validates it; fill_chunks_ flushes device footers before DbMetadataContext grows chunk_info[]; and the metadata growth itself is guarded by PENDING_OP_ADD_DEVICES with idempotent replay. The stranded footer is never rewritten, so re-running an interrupted operation re-validates against a fixed point. Each of these has a dedicated interruption test.
  • extend_chunk_info_ initializes all seven chunk_info_t fields, continues insertion counts from the free-list tail exactly as the existing append_ does, and runs under one dirty scope. The total_seq_chunks <= chunk_offset_t::max_id bound correctly keeps the top id free for the INVALID_CHUNK_ID sentinel.
  • db_metadata layout. device_sizes is carved from future_variables_unused with the total size pinned at 4480 by static_assert, and its placement inside the window the MONAD007 migration zeroes is statically asserted, so the zero sentinel holds on every existing pool.
  • Healing sizing. Both db_copy healing sites that now use db_map_size_of_ are guarded by a current-MAGIC check on the source copy before chunk_info_count is trusted, and the result is bounds-asserted against metadata_mmap_size_.
  • metadata_mapping_() reconstructs the mmap base/length in make_device_ exactly (and incidentally fixes a potential one-page munmap shortfall in the old destructor arithmetic).
  • CLI flag interactions. --rescan-devices sits in the require_option(0, 1) exclusive group, and the out-of-group --restore/--archive combinations have an explicit guard placed before --restore sets truncate_database — both covered by rescan_devices_argument_cross_checks.
  • Coherency. All footer/metadata I/O on this path is buffered (no O_DIRECT) with fdatasync/msync(MS_SYNC) barriers, so the pre-pass reads, relocation writes, and later mappings observe consistent data.

Test coverage is exemplary — every refusal message and crash window has a test, including the death test for a grown pool opened without mode::add_devices.

One P3 posted inline (recorded_device_size reads only metadata copy 0, creating a narrow unrecoverable-without-restore corner when copy 0 is corrupt and a device was already extended). Nothing blocking.

Verdict: CORRECT

🤖 Generated with Claude Code

@maxkozlovsky
maxkozlovsky force-pushed the max/device-resize-v2 branch 2 times, most recently from 3b4b643 to 8f9a6e4 Compare September 1, 2026 23:15
Base automatically changed from max/test-pool-disk-usage to main September 8, 2026 19:43
Growing a node's database meant --archive then --restore into a freshly
created larger pool: scratch space, plus a full dump and restore. Extending a
device the pool already owned was worse than unsupported -- the metadata
saying a device belongs to a pool lives at its very end, so lvextend left it
unopenable, with no resize2fs step to follow.

--rescan-devices reconciles a database with whatever storage it is now given:
--storage lists every device it should contain, and one offline run joins any
blank suffix and takes up the space of an extended last device, under one
confirmation and one metadata growth.

Taking up an extend needs the previous size of the device, which the extend
itself strands mid-device, so db_metadata now records every device's size on
each writable open, in padding it already reserves -- no magic bump, no
migration. That recording is the only source, and is acted on only where the
stranded footer's config_hash confirms it, so an extend must be preceded by a
writable open under a release carrying this code.

Refusing two sources that name one device is needed here, but the hole is not
new: creating a pool with a device listed twice was silently accepted, and no
later open could detect it. That check goes in the constructor, for every mode.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant