Skip to content

Commit

Permalink
Check the mtime again immediately before garbage collecting a disk ca…
Browse files Browse the repository at this point in the history 
…che 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
  • Loading branch information
@tjgq @copybara-github
tjgq authored and copybara-github committed Oct 1, 2024
1 parent 0f14eff commit caa2c41
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 33 deletions.
51 changes: 43 additions & 8 deletions src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

/**
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -169,7 +187,7 @@ private CollectionStats runUnderLock() throws IOException, InterruptedException
List<Entry> entriesToDelete = policy.getEntriesToDelete(allEntries);

for (Entry entry : entriesToDelete) {
deleter.delete(root.getRelative(entry.path()), entry.size());
deleter.delete(entry);
}

DeletionStats deletionStats = deleter.await();
Expand All @@ -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. */
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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(
Expand All @@ -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);
Expand All @@ -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());
}
}
}
9 changes: 1 addition & 8 deletions ...ain/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorIdleTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
18 changes: 9 additions & 9 deletions src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}

Expand All @@ -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");
}
Expand All @@ -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");
}
Expand All @@ -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");
}

Expand All @@ -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");
}
Expand All @@ -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");
}

Expand All @@ -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");
}
Expand All @@ -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");
}
Expand All @@ -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");
}

Expand Down
9 changes: 1 addition & 8 deletions src/tools/diskcache/Gc.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand Down

0 comments on commit caa2c41

Please sign in to comment.