Repository navigation
test(shader): skip open-entry cache replacements where SMB has no POSIX rename - #11434
Conversation
|
The tested base in the description is |
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
b26e0e8 to
8259ecb
Compare
|
@MarcZX10 rebased onto current AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the Windows SMB probing/fallback and its test changes on exact head 8259ecb26dac1890717b823dd42f9558eb4fa9a4. I found no implementation defect in the bounded FILE_RENAME_INFO_EX buffer construction or the fallback behavior. The author reports repeated Windows runs on two GPUs and local NTFS plus \\localhost\C$, including the cache test matrix and validation-layer comparison. I did not independently run those Windows tests. Current conventions, git diff --check, and the merge-tree preview pass; hosted Linux/Windows product jobs are path-skipped.
Before merge, please sync this branch to current main (b0f1183ca12b). Its tested base is 9656d366be; the intervening commits do not touch this PR's implementation, tests, or debt entry, and the preview merges cleanly. This is the remaining branch-freshness requirement from CONTRIBUTING.md.
…IX rename On Windows, WriteFileAtomically replaces a cache entry with a POSIX rename so that readers and mappings of the old entry keep working. Through an SMB share that rename fails with ERROR_INVALID_PARAMETER, and the plain rename it falls back to is refused while the destination is open or mapped, so agc_shader_disk_cache and shader_cache_file_read failed with their temporary directory on such a share (boykopovar#10833). The POSIX rename moves into PosixReplaceByHandle, unchanged, and PosixReplaceUnsupported probes it in a directory. Where it reports that error, the tests skip only the replacements of an entry that a reader holds open or the cache has mapped, and the disk cache test's cleanup leaves its directory behind when the POSIX delete fails the same way; any other failure still fails them. shader_cache_file_read also builds its \?\ alias of a UNC path as \?\UNC\server\share. The limitation is recorded in the shader debt file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8259ecb to
b95b22c
Compare
|
@MarcZX10 synced: rebased onto current AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head b95b22cf1622, now based on live main da89f18ea7c1. The rebase preserves the reviewed one-commit implementation and reported Windows SMB/NTFS regression coverage; no source or test files changed between the tested base and this main. The current-main merge preview, conventions and whitespace checks pass. Linux and Windows relinker plus ASan checks pass; macOS relinker jobs are still running/queued, and runtime product tests are skipped by path policy. I did not independently run the Windows/GPU tests. The earlier branch-freshness request is resolved. Approved.
What
Fixes #10833 the way agreed there (option 1): replacement stays atomic, and the tests skip only what an SMB share cannot do.
On Windows
WriteFileAtomicallyreplaces a cache entry with a POSIX-semantics rename (SetFileInformationByHandleclass 22,FileRenameInfoEx), so readers and mappings of the old entry keep working. Through an SMB share that rename fails withERROR_INVALID_PARAMETER, and thestd::filesystem::renamefallback is refused while the destination is open or mapped. The issue's NAS and\\localhost\C$(Windows' own SMB server, over NTFS) behave the same; the error codes are in #10833 (comment).With the first skip in place, the tests reached three more places that need the same operations:
verifyStoreandverifyPersistreplace an entry thatShaderDiskCacheitself has mapped withMapViewOfFileafter loading it. A mapping blocks the plain rename just like an open reader.removeCacheDirectorydeletes withFileDispositionInfoExandFILE_DISPOSITION_FLAG_POSIX_SEMANTICS, which SMB also rejects withERROR_INVALID_PARAMETER, and the entries the cache still maps can't be deleted there.shader_cache_file_readbuilt the\\?\alias of a UNC path as\\?\\\server\share\…; the extended form of a UNC path is\\?\UNC\server\share\….Changes:
ShaderCacheDirectory.hpp: the POSIX rename moves unchanged intoPosixReplaceByHandle, whichWriteFileAtomicallycalls as before, so production behaviour does not change.PosixReplaceUnsupported(directory)probes that rename on two scratch files in a directory and reports onlyERROR_INVALID_PARAMETER.agc_shader_disk_cache: where the probe reports it,verifyAtomicReplacementskips the retained-reader replacement,verifyStoreskips the truncated- and damaged-entry checks, andverifyPersistskips replacing the mapped persisted entry, each printing why. Any other replacement failure still fails. The cleanup falls back to a plain delete only when the POSIX delete returnsERROR_INVALID_PARAMETER, and then leaves the directory behind with a note instead of failing.shader_cache_file_read: the UNC alias fix, and the retained-reader replacement is skipped under the same probe. A plain replacement without a reader is still checked there.docs/dev/debt/shader.md: a "Shader cache directories on SMB shares" entry records that such a cache directory cannot update an open or mapped entry, including one this process's cache has mapped, and why replacement stays atomic.Tested
Rebased onto
main9656d366be(head8259ecb26d); on the RTX 5060 Ti the whole table below and the validation run were repeated on the rebased head and on9656d366be, with the same results. The first runs, on both machines, were on base7b415520b6.Same commands on two machines,
TEMP/TMPset to a local NTFS directory or to\\localhost\C$\aps5-smbtmp, each test 3 times, on this head and on its basemain7b415520b6:TEMPmain(both machines)agc_shader_disk_cacheshader_cache_file_readagc_shader_disk_cachecannot replace cache entry with a retained readershader_cache_file_readverifyStore/verifyPersist/cleanup changes, a local build that ran everyverify*case undertry/catchlisted the failing SMB cases: onlyverifyStore(cannot truncate the entry) andverifyPersist(cannot replace the persisted entry), then the cleanup's POSIX delete. The other 31 cases pass on the share unchanged.ctest -V -R "shader_disk_cache|shader_cache|pipeline_cache|pipeline_journal"withANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(Vulkan SDK 1.4.363.0): 27/27 on this head and onmain, on both GPUs, with noVUID-lines and nowas not found but was requested by env var VK_INSTANCE_LAYERS. WithVK_LOADER_DEBUG=layer, four of these create a device and showInsert instance layer "VK_LAYER_KHRONOS_validation":agc_pipeline_cache_header,agc_pipeline_cache,agc_pipeline_cache_fileandagc_driver_pipeline_journal; the disk cache tests themselves are CPU-only.\\localhost\C$reproduces the issue's failure and error codes.check_conventions.py --base origin/mainpasses.pr_overlap.pyfinds no conflicting comparison; PRs that already conflict withmain(fix(shader): preserve raw fragment exports for integer attachments #6235, fix(agc): keep texture images inside their extent's mip chain #6280, fix(recompiler): defer the TYPE check of a guarded read-only static V# #6412, feat(recompiler): run waterfall loops over per-lane T#s through image tables #6488, perf(recompiler): keep runtime-specialized shader modules in the shader disk cache #6924, perf(recompiler): leave the descriptor-range comparison out of direct buffer loads when the device zeroes past the range #7484, fix(shader): bound image-table snapshots by table-key ranges #7684, perf(shader): let spanned waves run at any host subgroup size where it can be narrower than 32 #8004, feat(agc): emulate mesh-fallback barycentrics through tessellation #9387, feat(agc): blend into 10_11_11 unorm color targets in the pixel shader #9748, fix(agc): retire a multisampled color target a write-only storage image overwrites #10049, fix(agc): read the compressed FMASK of a resident two-sample color target #10233) shareShaderDiskCache.cppor the shader debt file and were not compared.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)