From 6167641834e28f8ad322f8fde60866b339bfb7fe Mon Sep 17 00:00:00 2001 From: Martin Fick Date: Tue, 15 Dec 2020 14:20:44 -0700 Subject: [PATCH] Restore preserved packs during missing object seeks Provide a recovery path for objects being referenced during the pack pruning race. Due to the pack pruning race, it is possible for objects to become referenced after a pack has been deemed safe to prune, but before it actually gets pruned. If this happened previously, the newly referenced objects would be missing and potentially result in a corrupted ref. Add the ability to recover from this situation when an object is missing but happens to still be available in a pack in the "preserved" directory. This is likely only useful when used in conjunction with the --preserve-old-packs GC option, which prunes packs by hard-linking to the preserved directory. If an object is missing and found in a pack in the preserved directory, immediately recover that pack and its associated files (idx, bitmaps...) by moving them back to the original pack directory, and then retry the operation that would have failed due to the missing object. This retry can now succeed and the repository may avoid corruption. This approach should drastically reduce the chance of a corrupt repository during pack pruning at very little extra cost. This extra cost should only be incurred when objects are missing and a failure would normally occur. Change-Id: I2a704e3276b88cc892159d9bfe2455c6eec64252 Signed-off-by: Martin Fick Signed-off-by: Nasser Grainawi --- .../storage/file/GcBasicPackingTest.java | 51 ++++++++++++ .../storage/file/ObjectDirectory.java | 83 +++++++++++++++++-- .../internal/storage/file/PackDirectory.java | 28 +++++-- 3 files changed, 150 insertions(+), 12 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcBasicPackingTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcBasicPackingTest.java index 61538c9c2..8dc1ddb9f 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcBasicPackingTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcBasicPackingTest.java @@ -23,6 +23,7 @@ import org.eclipse.jgit.junit.TestRepository.BranchBuilder; import org.eclipse.jgit.lib.ConfigConstants; +import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.storage.file.FileBasedConfig; @@ -314,6 +315,56 @@ private void testPreserveOldPacks() throws Exception { assertTrue(preservedPackFile.exists()); } + @Test + public void testPruneAndRestoreOldPacks() throws Exception { + String tempRef = "refs/heads/soon-to-be-unreferenced"; + BranchBuilder bb = tr.branch(tempRef); + bb.commit().add("A", "A").add("B", "B").create(); + + // Verify setup conditions + stats = gc.getStatistics(); + assertEquals(4, stats.numberOfLooseObjects); + assertEquals(0, stats.numberOfPackedObjects); + + // Force all referenced objects into packs (to avoid having loose objects) + configureGc(gc, false); + gc.setExpireAgeMillis(0); + gc.setPackExpireAgeMillis(0); + gc.gc(); + stats = gc.getStatistics(); + assertEquals(0, stats.numberOfLooseObjects); + assertEquals(4, stats.numberOfPackedObjects); + assertEquals(1, stats.numberOfPackFiles); + + // Delete the temp ref, orphaning its commit + RefUpdate update = tr.getRepository().getRefDatabase().newUpdate(tempRef, false); + update.setForceUpdate(true); + ObjectId objectId = update.getOldObjectId(); // remember it so we can restore it! + RefUpdate.Result result = update.delete(); + assertEquals(RefUpdate.Result.FORCED, result); + + fsTick(); + + // Repack with only orphaned commit, so packfile will be pruned + configureGc(gc, false).setPreserveOldPacks(true); + gc.gc(); + stats = gc.getStatistics(); + assertEquals(0, stats.numberOfLooseObjects); + assertEquals(0, stats.numberOfPackedObjects); + assertEquals(0, stats.numberOfPackFiles); + + // Restore the temp ref to the deleted commit, should restore old-packs! + update = tr.getRepository().getRefDatabase().newUpdate(tempRef, false); + update.setNewObjectId(objectId); + update.setExpectedOldObjectId(null); + result = update.update(); + assertEquals(RefUpdate.Result.NEW, result); + + stats = gc.getStatistics(); + assertEquals(4, stats.numberOfPackedObjects); + assertEquals(1, stats.numberOfPackFiles); + } + private PackConfig configureGc(GC myGc, boolean aggressive) { PackConfig pconfig = new PackConfig(repo); if (aggressive) { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java index 170df5780..627facca0 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java @@ -13,6 +13,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.eclipse.jgit.internal.storage.pack.PackExt.PACK; import static org.eclipse.jgit.internal.storage.pack.PackExt.BITMAP_INDEX; +import static org.eclipse.jgit.internal.storage.pack.PackExt.INDEX; import java.io.BufferedReader; import java.io.File; @@ -31,6 +32,7 @@ import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.internal.storage.pack.ObjectToPack; +import org.eclipse.jgit.internal.storage.pack.PackExt; import org.eclipse.jgit.internal.storage.pack.PackWriter; import org.eclipse.jgit.lib.AbbreviatedObjectId; import org.eclipse.jgit.lib.AnyObjectId; @@ -78,7 +80,7 @@ public class ObjectDirectory extends FileObjectDatabase { private final PackDirectory packed; - private final File preservedDirectory; + private final PackDirectory preserved; private final File alternatesFile; @@ -116,10 +118,11 @@ public ObjectDirectory(final Config cfg, final File dir, objects = dir; infoDirectory = new File(objects, "info"); //$NON-NLS-1$ File packDirectory = new File(objects, "pack"); //$NON-NLS-1$ - preservedDirectory = new File(packDirectory, "preserved"); //$NON-NLS-1$ + File preservedDirectory = new File(packDirectory, "preserved"); //$NON-NLS-1$ alternatesFile = new File(objects, Constants.INFO_ALTERNATES); loose = new LooseObjects(objects); packed = new PackDirectory(config, packDirectory); + preserved = new PackDirectory(config, preservedDirectory); this.fs = fs; this.shallowFile = shallowFile; @@ -155,7 +158,7 @@ public final File getPackDirectory() { * @return the location of the preserved directory. */ public final File getPreservedDirectory() { - return preservedDirectory; + return preserved.getDirectory(); } /** {@inheritDoc} */ @@ -249,7 +252,13 @@ public String toString() { @Override public boolean has(AnyObjectId objectId) { return loose.hasCached(objectId) - || hasPackedInSelfOrAlternate(objectId, null) + || hasPackedOrLooseInSelfOrAlternate(objectId) + || (restoreFromSelfOrAlternate(objectId, null) + && hasPackedOrLooseInSelfOrAlternate(objectId)); + } + + private boolean hasPackedOrLooseInSelfOrAlternate(AnyObjectId objectId) { + return hasPackedInSelfOrAlternate(objectId, null) || hasLooseInSelfOrAlternate(objectId, null); } @@ -318,6 +327,15 @@ private void resolve(Set matches, AbbreviatedObjectId id, @Override ObjectLoader openObject(WindowCursor curs, AnyObjectId objectId) throws IOException { + ObjectLoader ldr = openObjectWithoutRestoring(curs, objectId); + if (ldr == null && restoreFromSelfOrAlternate(objectId, null)) { + ldr = openObjectWithoutRestoring(curs, objectId); + } + return ldr; + } + + private ObjectLoader openObjectWithoutRestoring(WindowCursor curs, AnyObjectId objectId) + throws IOException { if (loose.hasCached(objectId)) { ObjectLoader ldr = openLooseObject(curs, objectId); if (ldr != null) { @@ -379,8 +397,16 @@ ObjectLoader openLooseObject(WindowCursor curs, AnyObjectId id) } @Override - long getObjectSize(WindowCursor curs, AnyObjectId id) - throws IOException { + long getObjectSize(WindowCursor curs, AnyObjectId id) throws IOException { + long sz = getObjectSizeWithoutRestoring(curs, id); + if (0 > sz && restoreFromSelfOrAlternate(id, null)) { + sz = getObjectSizeWithoutRestoring(curs, id); + } + return sz; + } + + private long getObjectSizeWithoutRestoring(WindowCursor curs, + AnyObjectId id) throws IOException { if (loose.hasCached(id)) { long len = loose.getSize(curs, id); if (0 <= len) { @@ -448,6 +474,51 @@ private void selectObjectRepresentation(PackWriter packer, ObjectToPack otp, } } + private boolean restoreFromSelfOrAlternate(AnyObjectId objectId, + Set skips) { + if (restoreFromSelf(objectId)) { + return true; + } + + skips = addMe(skips); + for (AlternateHandle alt : myAlternates()) { + if (!skips.contains(alt.getId())) { + if (alt.db.restoreFromSelfOrAlternate(objectId, skips)) { + return true; + } + } + } + return false; + } + + private boolean restoreFromSelf(AnyObjectId objectId) { + Pack preservedPack = preserved.getPack(objectId); + if (preservedPack == null) { + return false; + } + PackFile preservedFile = new PackFile(preservedPack.getPackFile()); + // Restore the index last since the set will be considered for use once + // the index appears. + for (PackExt ext : PackExt.values()) { + if (!INDEX.equals(ext)) { + restore(preservedFile.create(ext)); + } + } + restore(preservedFile.create(INDEX)); + return true; + } + + private boolean restore(PackFile preservedPack) { + PackFile restored = preservedPack + .createForDirectory(packed.getDirectory()); + try { + Files.createLink(restored.toPath(), preservedPack.toPath()); + } catch (IOException e) { + return false; + } + return true; + } + @Override InsertLooseObjectResult insertUnpackedObject(File tmp, ObjectId id, boolean createDuplicate) throws IOException { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java index 007205e55..5f3097023 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java @@ -28,6 +28,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicReference; +import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.errors.CorruptObjectException; import org.eclipse.jgit.errors.PackInvalidException; import org.eclipse.jgit.errors.PackMismatchException; @@ -122,21 +123,36 @@ public String toString() { * * @param objectId * identity of the object to test for existence of. - * @return true if the specified object is stored in this PackDirectory. + * @return {@code true} if the specified object is stored in this PackDirectory. */ boolean has(AnyObjectId objectId) { + return getPack(objectId) != null; + } + + /** + * Get the {@link org.eclipse.jgit.internal.storage.file.Pack} for the + * specified object if it is stored in this PackDirectory. + * + * @param objectId + * identity of the object to find the Pack for. + * @return {@link org.eclipse.jgit.internal.storage.file.Pack} which + * contains the specified object or {@code null} if it is not stored + * in this PackDirectory. + */ + @Nullable + Pack getPack(AnyObjectId objectId) { PackList pList; do { pList = packList.get(); for (Pack p : pList.packs) { try { if (p.hasObject(objectId)) { - return true; + return p; } } catch (IOException e) { - // The hasObject call should have only touched the index, - // so any failure here indicates the index is unreadable - // by this process, and the pack is likewise not readable. + // The hasObject call should have only touched the index, so + // any failure here indicates the index is unreadable by + // this process, and the pack is likewise not readable. LOG.warn(MessageFormat.format( JGitText.get().unableToReadPackfile, p.getPackFile().getAbsolutePath()), e); @@ -144,7 +160,7 @@ boolean has(AnyObjectId objectId) { } } } while (searchPacksAgain(pList)); - return false; + return null; } /**