From 4c41c039e014271c51eed22f0fd75f488505088f Mon Sep 17 00:00:00 2001 From: Ladd Van Tol Date: Mon, 17 Aug 2026 17:34:30 -0700 Subject: [PATCH] Clone files into the disk cache where the filesystem supports it Uploading a file to the disk cache streamed its bytes into a temporary file, so the cache entry became a second physical copy of an output that was already on disk. Copy through FileSystemUtils#copyFile instead: it reaches Files#copy, which lets the filesystem serve the copy as a copy-on-write clone (clonefile on macOS, copy_file_range on Linux with a supporting filesystem), and the entry then shares its blocks with the output it came from. Where no clone is possible -- a different filesystem, or one without support -- Files#copy falls back to a byte copy, so this is never slower than the stream it replaces. The materialization direction already clones, since commit ec90e05dee ("Optimize file copies by using NIO methods"). Measured on macOS/APFS with a large iOS app: materializing bazel-out from a warm disk cache costs 200 MiB of real disk for a 2.07 GiB tree, while a locally built app leaves a 13.1 GiB second copy in the cache. This change removes that copy. A clone inherits the source's permissions and mtime, so both are reset: an entry must stay readable for every user of a shared cache, and the mtime is what the garbage collector reads to find the least recently used entries. The cloned temporary file is fsynced before the rename, keeping the durability the streamed write had. Note that a cache entry sharing blocks with an output is no longer freed by deleting either one alone, and that the garbage collector sizes entries by their logical length, so it now over-counts what deletion will reclaim. --- .../lib/remote/disk/DiskCacheClient.java | 68 ++++++++++++++++--- .../lib/remote/disk/DiskCacheClientTest.java | 20 ++++++ 2 files changed, 77 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java index 138bad34eb40e2..5898a9a9895d3a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java @@ -41,6 +41,7 @@ import com.google.protobuf.ByteString; import com.google.protobuf.ExtensionRegistryLite; import java.io.FileNotFoundException; +import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; @@ -270,9 +271,7 @@ public void close() { public ListenableFuture uploadFile(Digest digest, Path file) { return executorService.submit( () -> { - try (InputStream in = file.getInputStream()) { - saveFile(digest, Store.CAS, in); - } + saveFile(digest, Store.CAS, file); return null; }); } @@ -313,6 +312,51 @@ public Path toPath(String hash, Store store) { } public void saveFile(Digest digest, Store store, InputStream in) throws IOException { + save( + digest, + store, + temp -> { + try (OutputStream out = temp.getOutputStream()) { + ByteStreams.copy(in, out); + // Fsync temp before we rename it to avoid data loss in the case of machine + // crashes (the OS may reorder the writes and the rename). + if (out instanceof FileOutputStream fos) { + fos.getFD().sync(); + } + } + }); + } + + /** + * Saves an existing file into the cache. + * + *

The contents are copied through {@link FileSystemUtils#copyFile}, so a filesystem with + * copy-on-write support (clonefile on macOS, copy_file_range on Linux) can serve the copy as a + * clone, leaving the entry sharing its blocks with the file it was saved from. + */ + private void saveFile(Digest digest, Store store, Path file) throws IOException { + save( + digest, + store, + temp -> { + FileSystemUtils.copyFile(file, temp); + // copyFile preserves the source's permissions and mtime, neither of which suits a cache + // entry: an entry must remain readable by every user of a shared cache, and its mtime + // records when it was last stored or retrieved. + temp.chmod(0644); + temp.setLastModifiedTime(Path.NOW_SENTINEL_TIME); + // Fsync temp before we rename it to avoid data loss in the case of machine + // crashes (the OS may reorder the writes and the rename). + syncFile(temp); + }); + } + + /** Writes the contents of a cache entry into a temporary file. */ + private interface TempFileWriter { + void write(Path temp) throws IOException; + } + + private void save(Digest digest, Store store, TempFileWriter writer) throws IOException { Path path = toPath(digest, store); // CAS entries are content-addressed and thus automatically have the correct content if they @@ -325,14 +369,7 @@ public void saveFile(Digest digest, Store store, InputStream in) throws IOExcept Path temp = getTempPath(); try { - try (OutputStream out = temp.getOutputStream()) { - ByteStreams.copy(in, out); - // Fsync temp before we rename it to avoid data loss in the case of machine - // crashes (the OS may reorder the writes and the rename). - if (out instanceof FileOutputStream fos) { - fos.getFD().sync(); - } - } + writer.write(temp); path.getParentDirectory().createDirectoryAndParents(); FileSystemUtils.renameToleratingConcurrentCreation(temp, path); } catch (IOException e) { @@ -344,4 +381,13 @@ public void saveFile(Digest digest, Store store, InputStream in) throws IOExcept throw e; } } + + /** Flushes a file's contents to stable storage, where the filesystem supports it. */ + private static void syncFile(Path path) throws IOException { + try (InputStream in = path.getInputStream()) { + if (in instanceof FileInputStream fileInputStream) { + fileInputStream.getFD().sync(); + } + } + } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java index e4289358278db4..0b9429b5099156 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java @@ -154,6 +154,26 @@ public void uploadFile_whenPresent_updatesMtime() throws Exception { assertThat(path.getLastModifiedTime()).isNotEqualTo(0); } + @Test + public void uploadFile_whenMissing_doesNotInheritSourceMtimeOrPermissions() throws Exception { + Path file = fs.getPath("/file"); + FileSystemUtils.writeContent(file, UTF_8, "contents"); + // A build output is read-only, and may have been written long before it is uploaded. Neither + // property may leak into the cache entry: the mtime records when the entry was last stored or + // retrieved, and the entry must remain readable by every user of a shared cache. + file.chmod(0555); + file.setLastModifiedTime(1000); + Digest digest = getDigest("contents"); + + var unused = getFromFuture(client.uploadFile(digest, file)); + + Path path = getCasPath(digest); + assertThat(FileSystemUtils.readContent(path, UTF_8)).isEqualTo("contents"); + assertThat(path.getLastModifiedTime()).isNotEqualTo(1000); + assertThat(path.isReadable()).isTrue(); + assertThat(path.isWritable()).isTrue(); + } + @Test public void uploadBlob_whenMissing_populatesCas() throws Exception { ByteString blob = ByteString.copyFromUtf8("contents");