Reviewing patches, bisection, testing, and responsible fixes

Before you start#

Review ext4 fsync, fault injection and cleanup, KUnit and kselftest, and kernel history. Linux v6.12 is a historical source baseline. This lesson's proposed defect and patch are hypothetical; they are not presented as an upstream fix or a measured regression.

Learning goals#

Build a source-grounded review around an invariant and ownership, distinguish a failed prerequisite from a tested regression, design an honest bisection classifier, and specify before/after evidence and independent review.

Source walkthrough#

A patch review begins with a concrete failure and the invariant the change is supposed to restore. For a filesystem synchronization operation, one invariant is that a writeback error must not be silently transformed into a successful result merely because a later cleanup or notification succeeded. The invariant is conditional on the actual API and documented error-reporting rules; it is not a claim that fsync can guarantee that every device honestly persists data. Review the caller, callback, object lifetime, locking context, error return and userspace contract together.

In fs/ext4/fsync.c, the journaled ext4_sync_file path contains this exact v6.12 SPDX GPL-2.0 excerpt; surrounding state checks and the journal operation are omitted:

	ret = file_write_and_wait_range(file, start, end);
	if (ret)
		goto out;

The first line starts or waits for the relevant file data writeback and stores its result. A nonzero result jumps to the common exit rather than proceeding as if data writeback succeeded. The exit path also checks and advances the file's writeback error state, preserving the earlier error if already nonzero. The inode, mapping and open struct file must remain valid during the call; the caller's fd lookup and reference management occur in fs/sync.c. Journal state, flush requirements and forced shutdown add different branches.

The v6.12 code shown already has the check. For a hypothetical review exercise, imagine a local faulty change removed the conditional and continued to the journal path after file_write_and_wait_range returned -EIO, later overwriting ret with zero. This is an invented mutation, not an upstream defect claim. A minimal candidate fix restores error preservation and branches to the correct cleanup. The counterexample is a writeback failure followed by a successful journal step: returning zero would mislead userspace about the sync result. A normal-path test alone, where writeback returns zero, would not distinguish the faulty and corrected versions.

Follow callers, callbacks and resources#

The source walkthrough must include the VFS dispatch to the filesystem's file_operations.fsync, the lifetime of the file and inode, and the ext4 callback's normal and error exits. A patch changing a callback's return behavior can affect every caller, including fsync, fdatasync and internal paths where applicable. Find actual callers and operation tables in the selected tree; do not infer a call chain from names or from a later release. Examine whether the operation can sleep, which locks are held, and whether writeback or block completion occurs asynchronously on another task or CPU.

For a concurrency change, a pointer must remain valid for every reader, including callbacks and deferred work after the initiating function returns. An RCU read-side section, a reference count and a mutex protect different invariants. Error unwind must release only resources actually acquired, in an order safe for callbacks and hardware. A patch that fixes a leak by freeing an object before pending work is cancelled can create a use-after-free. Review both success and failure state transitions, not just a new goto label.

The user-visible API is another boundary. Changing errno, partial-result behavior, ordering or a stable UAPI structure can break callers even if an internal test passes. Configuration guards can remove code, change helper behavior or expose a build failure on another architecture. Security checks, namespace context and credentials can alter which branch is reached. Performance is also a regression dimension: a correct fix can add contention or excessive synchronous work. A reviewer should ask for evidence proportionate to the risk, not assume a style checker or a single architecture build proves compatibility.

A test that can fail for the right reason#

A valid regression test must establish its preconditions. For the hypothetical fsync mutation, a controlled test needs a supported way to make the relevant writeback operation fail for the test file in a disposable guest, verify that the intended error actually reached the path, and assert the returned error. It should also cover a successful fsync and a failure in a neighboring stage so that a blanket “always fail” implementation does not pass. Fault-injection controls vary by configuration; do not claim that an arbitrary failslab setting reliably causes this exact writeback error.

KUnit's own runner demonstrates why test setup is evidence. In lib/kunit/test.c, this exact v6.12 SPDX GPL-2.0 excerpt from kunit_run_case_internal is part of fixture initialization; surrounding code is omitted:

		if (ret) {
			kunit_err(test, "failed to initialize: %d\n", ret);
			kunit_set_failure(test);
			return;
		}

If the suite initializer fails, the framework records failure and returns before the case body. That result is not a successful test of the intended filesystem error. The enclosing runner separately invokes cleanup under its framework contract. A real test must prove that its injected error, target file, configuration and assertion were reached. Keep three outcomes distinct: pass (the intended assertion ran and held), fail (the intended assertion ran and failed), and not executed/inconclusive (setup failure, skip, timeout or missing prerequisite).

A complete protocol for the hypothetical bug is: establish the guest's exact known-bad source and config; run the targeted failure case and save its error and trace proving the injection point; run a normal fsync control and a neighboring error control; apply the candidate fix in a separate test checkout; rebuild and repeat the same cases; inspect resource cleanup and file error propagation; then obtain independent review. If the bad baseline does not reproduce the faulty success, stop and investigate the test rather than claiming the fix worked. This is a proposed test plan, not a test executed in this workspace.

Bisection and classification#

git bisect uses a known-good and known-bad revision to narrow a transition. The classification must measure the same behavior under a sufficiently stable environment. A build failure, missing dependency, timeout with uncertain cause or unavailable test is untestable, not automatically good or bad. For git bisect run, exit 0 marks good, exit 125 skips an untestable revision, and every exit from 1 through 127 except 125 marks bad; other statuses abort. In particular, shell statuses 126 (found but not executable) and 127 (command not found) are treated as bad by Git, so a wrapper must explicitly translate those infrastructure failures to 125. Signals or other abnormal exits must also be handled according to Git's documented rules.

A classifier should first verify prerequisites, build the selected revision in a disposable output directory, and check that the exact expected test ran. Map the test's explicit results to good, bad or skip. For example, a wrapper can use a dedicated harness whose documented exit codes are 0 for the known-good behavior, 1 for the reproduced bug and 125 for inconclusive setup. It should translate a build failure or missing harness to 125 explicitly, and stop the investigation if the classifier itself is not trustworthy. The following is a pseudocode policy, not a runnable script or a claim that such a harness exists:

if build or prerequisite fails: return 125
run trusted isolated test
if expected good result: return 0
if expected bad result: return 1
otherwise: return 125

Run bisection in a separate disposable checkout, never in the user's shared working tree. A separate build output directory prevents stale objects from contaminating a revision, but the source revision, compiler, config and userspace must also be recorded. When the first bad commit is found, inspect its parent, the change and relevant follow-up history; it may expose an older defect or interact with another change. A merge commit or a flaky test can complicate the result. The v6.12 bisection guide is a starting point, not a guarantee that an arbitrary classifier is valid.

Flakiness, configuration and backports#

A race reproducer may fail one in ten runs. A single good run cannot reliably classify it. Decide in advance how many trials, what threshold and what timeout make a revision classifiable; retain each result and use skip when confidence is inadequate. Changes in compiler, virtual hardware, timing, cgroups, cache state or instrumentation can produce a different failure rate. Sanitizers can expose an error and also perturb the timing. A bisection that changes the test harness between revisions may find an environmental transition rather than a kernel regression.

A regression matrix should identify relevant architectures, kernel configs, compiler variants, workload shapes and test types. For the fsync example, journaled and no-journal ext4 paths, appropriate mount options, success and error cases, supported storage flush behavior, and a filesystem-independent control can matter. KUnit, kselftest and an end-to-end guest test cover different boundaries. KASAN or KCSAN may be useful for a lifetime or race hypothesis but do not replace an fsync error assertion. The matrix should state what was not run and why, including the known GCC 16/C23 obstacle to the chapter 08 guest.

A stable backport can need a different implementation because APIs and surrounding fixes differ. Preserve the original commit provenance, explain dependencies and test the actual target tree. A change intended only for a newer kernel might not apply or might have a different security effect on an older one. Check UAPI behavior and error paths again; a patch applying without conflicts is not evidence of equivalent semantics. Do not weaken tests, suppress warnings or remove failure cases merely to make a backport appear green.

Independent review and responsible reporting#

The author and reviewer should each be able to state the precondition, invariant, ownership and failure path. Independent review is a gate for a proposed change, not an assertion that the reviewer must agree. A reviewer can challenge whether the test actually reaches the branch, whether an asynchronous completion holds a reference, or whether a different configuration changes locking. Resolve objections with evidence or record them as limitations. Do not invent a Reviewed-by or Tested-by tag.

A responsible bug report includes exact source and build identity, architecture, configuration, triggering workload, expected and observed behavior, full relevant error output, reproduction frequency and a minimal trusted test where possible. Sensitive data and security vulnerabilities require appropriate private handling under current project guidance. No email, issue, patch upload or publication is part of this exercise. The report's credibility comes from reproducibility and clear uncertainty, not an assertion that a formatting tool passed.

A compact review matrix#

For the hypothetical fsync defect, write the intended result beside each test condition. With data writeback succeeding and journal synchronization succeeding, the call should return success under the remaining filesystem and device assumptions. With data writeback returning -EIO, the existing source takes the exit path and preserves a failure; the hypothetical faulty mutation could overwrite it. With no journal, the separate no-journal path must be checked rather than assumed to follow the quoted branch. With forced shutdown, the early error takes precedence over the normal sequence. Each row needs a way to establish that the target branch was actually reached.

A reviewer should also ask whether the test leaves a dirty file, mounted image, pending request or guest process behind after failure. Cleanup is part of the test's own correctness. For a race, collect enough repetitions to estimate reproduction frequency and record test timeouts separately. A test that only asserts a log message may miss a wrong return value; a test that only checks errno may miss a leaked object. Use independent evidence where the risk warrants it, and keep unsupported configurations and unrun hardware explicit in the final report.

Practice#

Read-only source and test review

With a clean v6.12 source checkout, inspect the two pinned paths without changing the user's worktree:

git describe --tags --exact-match v6.12
git show v6.12:fs/ext4/fsync.c | sed -n '125,180p'
git show v6.12:lib/kunit/test.c | sed -n '379,403p'

The first source slice shows the ext4 error and success routes; the second shows fixture initialization and case execution. A missing tag, shallow source or unavailable Git is an environment limitation, not a test pass. No patch is applied and no kernel test is run. Record the source lines and classify the hypothetical -EIO before and after the imagined mutation.

Source-only alternative: use the quoted excerpts above. Build a table for writeback success, hypothetical writeback failure with the faulty mutation, and failure with the existing checked branch. Add a second table mapping fixture-init failure to “case not executed” and a bisection build failure to skip. Deliver the tables, a negative control and one independent-review question; mark every proposed patch and test as unrun.

Knowledge check#

1. Why would the hypothetical removed error check be missed by a success-only test?
2. What does the KUnit initializer failure establish about the case body?
3. How should a bisection harness classify a revision that cannot build?
4. Why can a clean cherry-pick still be a bad stable backport?
5. Why is a clean checkpatch result not an independent correctness review?

Answers#

1. When writeback succeeds, both faulty and corrected paths can return success. The distinguishing case is an actual writeback error followed by a later operation that would otherwise overwrite it.
2. The cited branch marks a failure and returns before the case body. The intended assertion was not exercised.
3. As untestable or skipped under the bisection protocol, unless a separately justified classification applies. It is not evidence of the target behavior.
4. Older code, dependencies, ABI and configuration can differ; the patch needs semantic review and testing in the actual target tree.
5. A style tool cannot establish locking, lifetime, security, performance or UAPI correctness, and it does not provide another person's reasoned review.

Checkpoint#

Submit the hypothetical error table, before/after and negative-control protocol, bisection classifications and an independent-review question. Ready means setup failure is not a test pass, the actual v6.12 check is not described as a new upstream fix, and all unrun tests are labeled. Retry the source branches if an error can be overwritten or an acquired resource has no owner. This is not certification.

Sources#

• ext4 fsync v6.12
• VFS sync v6.12
• KUnit case runner v6.12
• Bisecting a regression v6.12
• Submitting patches v6.12