Conversation
The SAVE checkpoint sampled /sys/fs/cgroup/memory.stat once, the instant posix_fadvise returned. That counter is global_node_page_state(NR_FILE_PAGES), fed by per-CPU vmstat deltas the kernel folds in lazily, so it reads above the settled value for up to a second after a large write is evicted. vmtouch reported the RDB at 0/250032 resident pages in both observed failures, so the reclaim under test had worked and only the counter was behind. Replace the three single-shot compares with a bounded retry that polls for up to 10s and breaks as soon as the counter is within budget. Fixes #50 Signed-off-by: Madelyn Olson <matolson@amazon.com>
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.
The SAVE checkpoint at
.github/workflows/daily.yml:801reads the runner's cgroup file page cache counter the instantposix_fadvisereturns, and that counter is not current yet. On cgroup v2's root cgroupmemory.stat'sfilefield isglobal_node_page_state(NR_FILE_PAGES), fed by per-CPU vmstat deltas the kernel folds in lazily, so it can read several MB above the settled value for up to a second after a large write is evicted.vmtouchreported/tmp/master/dump.rdbat0/250032resident pages in both observed failures, so the reclaim under test worked and only the counter was behind. This replaces the three single-shot compares with a bounded retry that polls for up to 10s and breaks as soon as the counter is within budget.Details
Problem
daily.yml:797runssave,:798runsvmtouch,:801readsmemory.stat. Nothing in between. The other two checkpoints sample the same counter aftersleep 1 # wait for the completion of cache reclaim bio(daily.yml:807anddaily.yml:825) and have not failed.The two failures reported in #50:
Deltas 30,953,472 and 16,097,280 against the 8,000,000 budget, with the RDB at zero resident pages both times.
reclaimFilePageCachecannot be the defect.rdb.c:1701-1709doesfflushthenfsyncthen a whole-fileposix_fadvise, so by the timesavereturns every page is clean and dropped, which is whatvmtouchmeasures.rio.c:155-161already documents that the incremental fadvise calls during the write skip pages under writeback and leave them for "other chances to be reclaimed". The final fsync closes that gap for the mapping. It does not make the vmstat counter current.Reproduction
Isolating the counter from valkey: write N files in parallel,
fsync,posix_fadvise(DONTNEED), then read/proc/vmstatnr_file_pages(the same counter, times page size) at t=0, t=1s and t=2s. The forcing knob is the number of parallel writers, which is what determines how much per-CPU vmstat delta is pending when you read.8 of 8. The unsettled read is 0.48 to 3.5 MB high; one second later it is within 120 KB of baseline. Turning the knob up crosses the 8,000,000 budget:
11.2 MB at t=0, which fails the assertion, against 3.8 MB one second later, which passes it.
Running the actual
daily.ymlSAVE checkpoint verbatim (samedebug populate 10000 k 102400, same two servers, same flags) on a 32-CPU box reproduces the direction but not the magnitude, since the box has 246 GB of RAM against the runner's 16 GB:t=0 reads above t=1s in 5 of 6 loops, up to 3.2x. The settled deltas match the 0.5 to 1.1 MB seen in passing CI runs. Absolute values stay under budget, so this arm does not fail before the patch and pass after it; the synthetic loop above is what crosses the budget.
Why a retry and not another
sleep 1sleep 1is what the other two checkpoints do and it has held so far, but it is a duration where the thing being waited on is a condition. A retry waits exactly as long as the counter needs and no longer, and it costs nothing in the common case: the loop breaks on the first sample in every passing run.The two existing
sleep 1 # wait for the completion of cache reclaim biolines stay. They also gate thevmtouch0%checks at:808,:811and:826, which the retry does not cover.Not fixed here
/sys/fs/cgroup/memory.statis the root cgroup, so the assertion counts the Actions runner writing step logs, journald, snapd and theapt-get install vmtouchfromdaily.yml:781alongside valkey. A retry does not fix that, and in the reproduction above the loops that failed at 10 GB scale failed at t=0 and t=10s alike for exactly that reason. Attributing the measurement to a dedicated cgroup would be the real fix and is a larger change than this one.This was generated by AI but verified, with love, by a human.