bcachefs: two tests for what 1.38 filesystems hit on first read-write open - #105
Open
matthiasgoergens wants to merge 2 commits into
Open
bcachefs: two tests for what 1.38 filesystems hit on first read-write open#105matthiasgoergens wants to merge 2 commits into
matthiasgoergens wants to merge 2 commits into
Conversation
…rade? The 1.39 upgrade entry (per_dev_fragmentation_lru) schedules check_inodes and check_xattrs, which rewrite every inode whose has_inode_opts / has_access_acl / has_default_acl flag is wrong. Those flags do not exist below 1.39, so on a filesystem written by a 1.38 kernel that is *every* inode carrying a per-inode option -- and filesystems in the field carry hundreds of thousands of explicit data_replicas overrides. A rewrite that dropped bi_data_replicas would destroy them silently: the upgrade entry lists those errors as tolerated, which feeds them to errors_silent, so the mount log says only "Fixed errors, running fsck a second time" and names nothing. Two stages, two kernels, one image. Stage 1 formats at 1.38 on a 1.38 kernel, populates 425 files with per-inode and per-directory data_replicas overrides plus POSIX ACLs, records a manifest inside the filesystem and exports the raw device. Stage 2 mounts that image on a 1.39 kernel and checks that the version moved to 1.39, that every recorded option and content checksum is unchanged, and that the number of inodes carrying bi_data_replicas is unchanged. Stage 1 genuinely needs a 1.38 kernel, not merely a 1.38 format. bch2_inode_pack() recomputes has_inode_opts on every pack regardless of the on-disk version, so a 1.39 kernel writing into a 1.38-format filesystem already sets the flag, check_inodes finds nothing to repair, and no inode is ever rewritten. Stage 2 therefore fails, rather than passes, when zero flags were repaired: the rewrite path is the thing being tested, and a run that never entered it has proved nothing. Counting the repairs needs care precisely because the upgrade silences them. The test reads the flag bits straight out of the inodes btree with `bcachefs list` before and after, and cross-checks against the "errors this recovery" summary of a 1.39 fsck run with -o version_upgrade=none. That run doubles as the control for the post-upgrade fsck, holding the checker fixed at 1.39 and varying only whether the upgrade happened, so a dirty result can be attributed to the upgrade rather than to the newer checker simply checking more. data_replicas=1 is deliberately the bulk of the overrides: an explicit override that happens to equal the filesystem default is both the common case in the field and the easy one to overlook, since it looks like the default while still being a nonzero bi_data_replicas that the upgrade could drop. A few inodes get 2 because real filesystems have a few. The mount whose log matters is captured by writing a marker to /dev/kmsg and slicing dmesg at it, not by clearing the ring buffer first. `dmesg -C` would also remove the "========= TEST" line that prelude's check_dmesg anchors on, and with that anchor gone its awk emits nothing at all -- so the grep for "kernel BUG at", "WARNING:", "Oops:" and friends would run over an empty stream and always succeed, switching off kernel-splat detection for exactly the mount this test most wants watched. Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
… past
BCH_DISK_ACCOUNTING_snapshot changed from {id} -> [sectors] to {id, btree} ->
[nr_keys, key_bytes, external_sectors]. The new key aliases exactly onto the
old one: btree reads back as 0 (BTREE_ID_extents) out of the old key's zero
padding, so the bpos is unchanged and an existing key is silently
reinterpreted. The old sectors value is inherited as nr_keys, while key_bytes
and external_sectors start from zero rather than from the truth. Correct
deltas are then applied to a base that was never established, the counters go
negative, and the negative values are persisted. Same bpos, across one
read-write open:
u64s 6 ... 432345564210790400:0:0 : snapshot id=... btree=extents 13528
u64s 8 ... 432345564210790400:0:0 : snapshot id=... btree=extents 13528 -160 0
Reported as koverstreet/bcachefs-tools#803, where
the same thing is reproduced in pure userspace on plain image files with two
tools builds at pinned commits.
No user IO is needed to reach that state. Draining reconcile work queued by
the old code is enough -- each extent that has its background compression
applied drops an 8-byte reconcile: entry, and that correct -8 per key lands on
a key_bytes base of 0.
This test rides the two-stage image from version-upgrade-138-to-139.ktest,
because the kernel that writes stage 1 is also the kernel that predates the
accounting change. The on-disk version is not what discriminates, though: a
filesystem formatted at 1.38 by current code never shows this, and one written
by old code shows it even mounted with -o version_upgrade=none. What matters
is the code that last wrote the key. The assertions are therefore on the key
itself, not on the version, and not on the log -- note that nothing in the
mount's dmesg mentions accounting at all. The corruption is silent at mount
time and only surfaces later, when something runs check_allocations.
Four assertions: no per-snapshot counter is negative after the first
read-write open; a read-only fsck of that device reports no
accounting_key_underflow; the in-kernel and userspace fsck implementations
agree about it, since a divergence would be its own bug; and a repairing fsck
-y makes the repair stick. The first two fail today -- deliberately, the bug
is unfixed and this test is the reproducer.
Since a current kernel cannot produce old-layout input by construction, the
test cannot synthesise its own image. It asserts the precondition instead: the
supplied image must carry a per-snapshot accounting key with a single counter.
An image already in the new layout has nothing to alias onto and would pass
for the wrong reason, so that is reported as a failure rather than quietly
accepted.
Forcing userspace fsck inside ktest needs env --unset=BCACHEFS_KERNEL_ONLY:
bcachefs-test-libs.sh exports that variable, and src/commands/fsck.rs consults
it before cli.no_kernel, so -K alone cannot win. The test prints each run's
implementation banner rather than assuming which one ran.
As in version-upgrade-138-to-139.ktest, the mount under test is captured by
writing a marker to /dev/kmsg and slicing dmesg at it rather than by clearing
the ring buffer, so that prelude's check_dmesg keeps its "========= TEST"
anchor and its kernel-splat grep keeps running over a non-empty stream.
Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
Contributor
Author
|
A correction to the second paragraph, in the reviewer's favour: building the bcachefs-tools carries Verified by building three worktrees of stock
Each kernel's So the recipe for stage 2 is: check out |
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
1 participant
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.
Two tests covering what happens to a filesystem written by a 1.38-era kernel the first time a current kernel opens it read-write. They share one image: the first test's
populate_138stage builds it, and both consumer stages read it.Before anything else, the awkward part, because it decides whether you can run these at all today. Both tests turn on on-disk version 1.39,
per_dev_fragmentation_lru. That version exists in koverstreet/bcachefs-tools master —a02127c7f862, 25 Jul 2026, infs/bcachefs_format.h— but it does not exist in the kernel repo. koverstreet/bcachefs master isca944a61e079(13 May 2026) and tops out at 1.38. So stage 1 runs against koverstreet/bcachefs master unmodified, which is exactly the 1.38 kernel it wants, but stage 2 has no public kernel ref to point-kat. To run it today you need a kernel tree whosefs/bcachefsis the libbcachefs from bcachefs-tools master; I built mine by checking outca944a61e079and replacingfs/bcachefswith the tools repo'sfs/tree, and any equivalent port will do. Nothing in either test file names a fork, a branch or a commit, and neither asserts on a version name — version names differ between forks, the numbers do not, so both files read1.38and1.39out of the superblock and compare those. The moment a kernel commit or tag carryingper_dev_fragmentation_lrulands upstream, pointing stage 2's-kat it is the only thing that changes; the tests run against public refs unmodified.The first test asks whether per-inode IO options survive the 1.38 to 1.39 upgrade. The 1.39 upgrade entry schedules
check_inodesandcheck_xattrs, and those rewrite every inode whosehas_inode_opts,has_access_aclorhas_default_aclflag is wrong. Below 1.39 the flags do not exist, so on a filesystem a 1.38 kernel wrote that is every inode carrying a per-inode option — and filesystems in the field carry hundreds of thousands of explicitdata_replicasoverrides. If that rewrite droppedbi_data_replicasit would do so in silence, because the upgrade entry lists the relevant errors as tolerated and they go toerrors_silent: the mount says only "Fixed errors, running fsck a second time" and names nothing.Getting that test to mean anything turns out to require care in two places. Stage 1 genuinely has to run on a 1.38 kernel rather than merely format at 1.38, because
bch2_inode_pack()recomputeshas_inode_optson every pack regardless of the on-disk version — a 1.39 kernel writing into a 1.38-format filesystem already sets the flag,check_inodesfinds nothing to repair, no inode is ever rewritten, and the run proves nothing while looking green. Stage 2 therefore fails, rather than passes, when zero flags were repaired. And counting the repairs is awkward precisely because the upgrade silences them, so the test reads the flag bits straight out of the inodes btree withbcachefs listbefore and after and cross-checks that against a 1.39 fsck run with-o version_upgrade=none. That run doubles as the control for the post-upgrade fsck: it holds the checker fixed at 1.39 and varies only whether the upgrade happened, so a dirty result can be attributed to the upgrade rather than to a newer checker simply checking more.The second test is a reproducer for koverstreet/bcachefs-tools#803, and it is expected to fail — the bug is unfixed, and the test exists to say so out loud until it is.
BCH_DISK_ACCOUNTING_snapshotchanged from{id} -> [sectors]to{id, btree} -> [nr_keys, key_bytes, external_sectors], and the new key aliases exactly onto the old one —btreereads back as 0 out of the old key's zero padding, so the bpos is unchanged and an existing key is silently reinterpreted. The oldsectorsvalue is inherited asnr_keyswhilekey_bytesandexternal_sectorsstart from zero instead of from the truth, correct deltas then land on a base that was never established, and the counters go negative and stay that way:No user IO is needed to get there; draining reconcile work queued by the old code is enough. The on-disk version is not the discriminating variable either — a filesystem formatted at 1.38 by current code never shows this, and one written by old code shows it even mounted with
-o version_upgrade=none. What matters is the code that last wrote the key. The test rides the 1.38 image only because the kernel that writes it is also the kernel that predates the accounting change, and it asserts on the key itself rather than on the version. It also does not assert on the log, because there is nothing there to assert on: the mount's dmesg does not mention accounting at all, andaccounting_key_underflowsurfaces only later when something runscheck_allocations.Four assertions, of which the first two fail today: no per-snapshot accounting counter is negative after the first read-write open; a read-only fsck reports no
accounting_key_underflow; the in-kernel and userspace fsck implementations agree about it, since a divergence between them on an unmodified device would be its own bug; and a repairingfsck -ymakes the repair stick. Forcing the userspace checker inside ktest needsenv --unset=BCACHEFS_KERNEL_ONLY, sincebcachefs-test-libs.shexports that andsrc/commands/fsck.rsconsults it beforecli.no_kernel— the test prints each run's implementation banner rather than assuming which one ran. A red result from this file is the expected result, the waysnapshot-inject.ktestalready carries subtests marked expected-to-fail until the thing they describe is fixed; it turns green when the aliasing is fixed.Because a current kernel cannot produce old-layout input by construction, that test cannot synthesise its own image, so it asserts the precondition instead: the supplied image must carry a per-snapshot accounting key with a single counter. An image already in the new layout has nothing to alias onto and would pass for the wrong reason, so it is reported as a failure rather than quietly accepted. The same suspicion runs through both files. A test that goes green because its input could not exercise the code under test is worse than no test at all — that was the original defect in the first draft of these — so every input assumption is an assertion rather than a comment: the image must be supplied and must be at 1.38, its per-snapshot accounting key must be in the old layout, the manifest must list files that actually carry an override, the external tools the results are read out of must all be present, and stage 2 must observe a nonzero number of
has_inode_optsrepairs or it reports the option-preservation result as weak and fails. Four of those were checked by booting a VM with the assumption deliberately broken — no image supplied, a new-layout image, a manifest with every override stripped out, and each ofgawk,getfattr,setfacl,sha256sumandbcachefsmoved aside in turn — and each aborts with a named reason instead of reporting a pass.Both tests need two invocations, since ktest boots one kernel per run:
Neither will run unattended in CI, for the same reason
version_upgrade.ktestandcompat.ktestdo not: an old kernel has to write the input first. Both files parse and list their subtests withoutktest_138_imageset, so enumerating tests is unaffected — they fail with an explanation when actually run without one, rather than passing on a blank device.