From 53839d302f4e109d1e50f98bd401d03951186212 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Oct 2024 04:48:28 -0700 Subject: [PATCH] [7.4.0] Check the mtime again immediately before garbage collecting a disk cache entry. This reduces the window for a race condition with a concurrent build. (We don't need to close it completely, as Bazel has the ability to retry a build on a cache eviction.) PiperOrigin-RevId: 680963998 Change-Id: I474f192e93da5cf98333ec9e211b3abae95f7b69 --- .../disk/DiskCacheGarbageCollector.java | 51 ++++++++++++++++--- .../DiskCacheGarbageCollectorIdleTask.java | 9 +--- .../disk/DiskCacheGarbageCollectorTest.java | 18 +++---- src/tools/diskcache/Gc.java | 9 +--- 4 files changed, 54 insertions(+), 33 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java index 60b4146137e57a..a57c2bf5fff105 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.remote.disk; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.devtools.build.lib.remote.util.Utils.bytesCountToDisplayString; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ComparisonChain; @@ -33,6 +34,7 @@ import java.util.List; import java.util.Optional; import java.util.concurrent.ExecutorService; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.LongAdder; /** @@ -111,11 +113,27 @@ private long getTimeCutoff() { } } - private record DeletionStats(long deletedEntries, long deletedBytes) {} + private record DeletionStats(long deletedEntries, long deletedBytes, boolean concurrentUpdate) {} /** Stats for a garbage collection run. */ public record CollectionStats( - long totalEntries, long totalBytes, long deletedEntries, long deletedBytes) {} + long totalEntries, + long totalBytes, + long deletedEntries, + long deletedBytes, + boolean concurrentUpdate) { + + /** Returns a human-readable summary. */ + public String displayString() { + return "Deleted %d of %d files, reclaimed %s of %s%s" + .formatted( + deletedEntries(), + totalEntries(), + bytesCountToDisplayString(deletedBytes()), + bytesCountToDisplayString(totalBytes()), + concurrentUpdate() ? " (concurrent update detected)" : ""); + } + } private final Path root; private final CollectionPolicy policy; @@ -169,7 +187,7 @@ private CollectionStats runUnderLock() throws IOException, InterruptedException List entriesToDelete = policy.getEntriesToDelete(allEntries); for (Entry entry : entriesToDelete) { - deleter.delete(root.getRelative(entry.path()), entry.size()); + deleter.delete(entry); } DeletionStats deletionStats = deleter.await(); @@ -178,7 +196,8 @@ private CollectionStats runUnderLock() throws IOException, InterruptedException allEntries.size(), allEntries.stream().mapToLong(Entry::size).sum(), deletionStats.deletedEntries(), - deletionStats.deletedBytes()); + deletionStats.deletedBytes(), + deletionStats.concurrentUpdate()); } /** Lists all disk cache entries, performing I/O in parallel. */ @@ -209,7 +228,7 @@ private void visitDirectory(Path path) { for (Dirent dirent : path.readdir(Symlinks.NOFOLLOW)) { Path childPath = path.getChild(dirent.getName()); if (dirent.getType().equals(Dirent.Type.FILE)) { - // The file may be gone by the time we open it. + // The file may be gone by the time we stat it. FileStatus status = childPath.statIfFound(); if (status != null) { Entry entry = @@ -237,6 +256,7 @@ private void visitDirectory(Path path) { private final class EntryDeleter extends AbstractQueueVisitor { private final LongAdder deletedEntries = new LongAdder(); private final LongAdder deletedBytes = new LongAdder(); + private final AtomicBoolean concurrentUpdate = new AtomicBoolean(false); EntryDeleter() { super( @@ -247,13 +267,28 @@ private final class EntryDeleter extends AbstractQueueVisitor { } /** Enqueues an entry to be deleted. */ - void delete(Path path, long size) { + void delete(Entry entry) { execute( () -> { + Path path = root.getRelative(entry.path()); try { + FileStatus status = path.statIfFound(); + if (status == null) { + // The entry is already gone. + concurrentUpdate.set(true); + return; + } + if (status.getLastModifiedTime() != entry.mtime()) { + // The entry was likely accessed by a build since we statted it. + concurrentUpdate.set(true); + return; + } if (path.delete()) { deletedEntries.increment(); - deletedBytes.add(size); + deletedBytes.add(entry.size()); + } else { + // The entry is already gone. + concurrentUpdate.set(true); } } catch (IOException e) { throw new IORuntimeException(e); @@ -268,7 +303,7 @@ DeletionStats await() throws IOException, InterruptedException { } catch (IORuntimeException e) { throw e.getCauseIOException(); } - return new DeletionStats(deletedEntries.sum(), deletedBytes.sum()); + return new DeletionStats(deletedEntries.sum(), deletedBytes.sum(), concurrentUpdate.get()); } } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorIdleTask.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorIdleTask.java index ddccdbacd5f120..7c03e16fbb6b3c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorIdleTask.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorIdleTask.java @@ -13,8 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.remote.disk; -import static com.google.devtools.build.lib.remote.util.Utils.bytesCountToDisplayString; - import com.google.common.annotations.VisibleForTesting; import com.google.common.flogger.GoogleLogger; import com.google.common.util.concurrent.ThreadFactoryBuilder; @@ -94,12 +92,7 @@ public void run() { try { logger.atInfo().log("Disk cache garbage collection started"); CollectionStats stats = gc.run(); - logger.atInfo().log( - "Disk cache garbage collection finished: deleted %d of %d files, reclaimed %s of %s", - stats.deletedEntries(), - stats.totalEntries(), - bytesCountToDisplayString(stats.deletedBytes()), - bytesCountToDisplayString(stats.totalBytes())); + logger.atInfo().log("%s", stats.displayString()); } catch (IOException e) { logger.atInfo().withCause(e).log("Disk cache garbage collection failed"); } catch (InterruptedException e) { diff --git a/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java b/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java index 75dc8a8a3491a3..41b778aacb3d98 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java @@ -61,7 +61,7 @@ public void sizePolicy_noCollection() throws Exception { CollectionStats stats = runGarbageCollector(Optional.of(kbytes(2)), Optional.empty()); - assertThat(stats).isEqualTo(new CollectionStats(2, kbytes(2), 0, 0)); + assertThat(stats).isEqualTo(new CollectionStats(2, kbytes(2), 0, 0, false)); assertFilesExist("ac/123", "cas/456"); } @@ -75,7 +75,7 @@ public void sizePolicy_collectsOldest() throws Exception { CollectionStats stats = runGarbageCollector(Optional.of(kbytes(2)), Optional.empty()); - assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2))); + assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false)); assertFilesExist("ac/123", "cas/456"); assertFilesDoNotExist("ac/abc", "cas/def"); } @@ -90,7 +90,7 @@ public void sizePolicy_tieBreakByPath() throws Exception { CollectionStats stats = runGarbageCollector(Optional.of(kbytes(2)), Optional.empty()); - assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2))); + assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false)); assertFilesExist("cas/456", "cas/def"); assertFilesDoNotExist("ac/123", "ac/abc"); } @@ -103,7 +103,7 @@ public void agePolicy_noCollection() throws Exception { CollectionStats stats = runGarbageCollector(Optional.empty(), Optional.of(days(3))); - assertThat(stats).isEqualTo(new CollectionStats(2, kbytes(2), 0, 0)); + assertThat(stats).isEqualTo(new CollectionStats(2, kbytes(2), 0, 0, false)); assertFilesExist("ac/123", "cas/456"); } @@ -117,7 +117,7 @@ public void agePolicy_collectsOldest() throws Exception { CollectionStats stats = runGarbageCollector(Optional.empty(), Optional.of(Duration.ofDays(3))); - assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2))); + assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false)); assertFilesExist("ac/123", "cas/456"); assertFilesDoNotExist("ac/abc", "cas/def"); } @@ -130,7 +130,7 @@ public void sizeAndAgePolicy_noCollection() throws Exception { CollectionStats stats = runGarbageCollector(Optional.of(kbytes(2)), Optional.of(days(1))); - assertThat(stats).isEqualTo(new CollectionStats(2, kbytes(2), 0, 0)); + assertThat(stats).isEqualTo(new CollectionStats(2, kbytes(2), 0, 0, false)); assertFilesExist("ac/123", "cas/456"); } @@ -144,7 +144,7 @@ public void sizeAndAgePolicy_sizeMoreRestrictiveThanAge_collectsOldest() throws CollectionStats stats = runGarbageCollector(Optional.of(kbytes(2)), Optional.of(days(4))); - assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2))); + assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false)); assertFilesExist("ac/123", "cas/456"); assertFilesDoNotExist("ac/abc", "cas/def"); } @@ -159,7 +159,7 @@ public void sizeAndAgePolicy_ageMoreRestrictiveThanSize_collectsOldest() throws CollectionStats stats = runGarbageCollector(Optional.of(kbytes(3)), Optional.of(days(3))); - assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2))); + assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2), false)); assertFilesExist("ac/123", "cas/456"); assertFilesDoNotExist("ac/abc", "cas/def"); } @@ -171,7 +171,7 @@ public void ignoresTmpAndGcSubdirectories() throws Exception { CollectionStats stats = runGarbageCollector(Optional.of(1L), Optional.of(days(1))); - assertThat(stats).isEqualTo(new CollectionStats(0, 0, 0, 0)); + assertThat(stats).isEqualTo(new CollectionStats(0, 0, 0, 0, false)); assertFilesExist("gc/foo", "tmp/foo"); } diff --git a/src/tools/diskcache/Gc.java b/src/tools/diskcache/Gc.java index f1b5b9dee9b8fb..c678e5bfeb9258 100644 --- a/src/tools/diskcache/Gc.java +++ b/src/tools/diskcache/Gc.java @@ -13,7 +13,6 @@ // limitations under the License. package diskcache; -import static com.google.devtools.build.lib.remote.util.Utils.bytesCountToDisplayString; import static java.lang.Math.min; import com.google.common.util.concurrent.ThreadFactoryBuilder; @@ -111,13 +110,7 @@ public static void main(String[] args) throws Exception { CollectionStats stats = gc.run(); - System.out.printf( - "Deleted %d of %d files, reclaimed %s of %s\n", - stats.deletedEntries(), - stats.totalEntries(), - bytesCountToDisplayString(stats.deletedBytes()), - bytesCountToDisplayString(stats.totalBytes())); - + System.out.println(stats.displayString()); System.exit(0); }