* [JGIT PATCH 0/5] Make ObjectLoader safer during repack
@ 2009-04-23 3:36 Shawn O. Pearce
2009-04-23 3:36 ` [JGIT PATCH 1/5] Remove throws IOException from UnpackedObjectLoader.getCachedBytes Shawn O. Pearce
0 siblings, 1 reply; 6+ messages in thread
From: Shawn O. Pearce @ 2009-04-23 3:36 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git
This is a start to fixing the ObjectLoader save during repack
problems I talked about yesterday. It applies on top of the
WindowedFile+PackFile patch I sent earlier today.
Patch 1 and 2 are trivial.
Patch 3 fixes the broken 10/10 test case I raised yesterday.
Patch 4 introduces that test case.
Patch 5 starts to fix PackWriter to be safe during repack.
Its still a bit iffy. The basic logic here is sound, but I'm more
worried about someone else calling PackFile.close() and closing the
underlying RandomFile while we are otherwise accessing the file
during the copyRaw in PackWriter. I *think* its impossible for
that to occur, but I haven't proven it to myself yet, and its late,
so this one is RFC for now.
Shawn O. Pearce (5):
Remove throws IOException from UnpackedObjectLoader.getCachedBytes
Add missing @Override annotations to UnpackedObjectLoader
Fully materialize an ObjectLoader before returning it from
ObjectDatabase
Test that ObjectLoader stays valid across repacks
Teach PackWriter to recover from removed/replaced packs
.../org/spearce/jgit/lib/ConcurrentRepackTest.java | 40 +++++++
.../jgit/lib/DeltaOfsPackedObjectLoader.java | 7 +-
.../spearce/jgit/lib/DeltaPackedObjectLoader.java | 41 +++----
.../jgit/lib/DeltaRefPackedObjectLoader.java | 9 +-
.../src/org/spearce/jgit/lib/ObjectDirectory.java | 3 +-
.../src/org/spearce/jgit/lib/ObjectLoader.java | 23 +---
.../src/org/spearce/jgit/lib/PackFile.java | 14 +-
.../src/org/spearce/jgit/lib/PackWriter.java | 118 +++++++++++++-------
.../org/spearce/jgit/lib/PackedObjectLoader.java | 78 +++++++++++--
.../org/spearce/jgit/lib/UnpackedObjectLoader.java | 4 +-
.../spearce/jgit/lib/WholePackedObjectLoader.java | 21 ++--
.../src/org/spearce/jgit/lib/WindowCache.java | 59 +++++++----
12 files changed, 280 insertions(+), 137 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread* [JGIT PATCH 1/5] Remove throws IOException from UnpackedObjectLoader.getCachedBytes 2009-04-23 3:36 [JGIT PATCH 0/5] Make ObjectLoader safer during repack Shawn O. Pearce @ 2009-04-23 3:36 ` Shawn O. Pearce 2009-04-23 3:36 ` [JGIT PATCH 2/5] Add missing @Override annotations to UnpackedObjectLoader Shawn O. Pearce 0 siblings, 1 reply; 6+ messages in thread From: Shawn O. Pearce @ 2009-04-23 3:36 UTC (permalink / raw) To: Robin Rosenberg; +Cc: git The UnpackedObjectLoader always fully materializes itself during the object's constructor. Thus the cached bytes are always ready on any invocation of getCachedBytes(), and this method will never fail. Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- .../org/spearce/jgit/lib/UnpackedObjectLoader.java | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java b/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java index 7552b42..1352b72 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java @@ -201,7 +201,7 @@ public long getSize() { } @Override - public byte[] getCachedBytes() throws IOException { + public byte[] getCachedBytes() { return bytes; } -- 1.6.3.rc1.205.g37f8 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [JGIT PATCH 2/5] Add missing @Override annotations to UnpackedObjectLoader 2009-04-23 3:36 ` [JGIT PATCH 1/5] Remove throws IOException from UnpackedObjectLoader.getCachedBytes Shawn O. Pearce @ 2009-04-23 3:36 ` Shawn O. Pearce 2009-04-23 3:36 ` [JGIT PATCH 3/5] Fully materialize an ObjectLoader before returning it from ObjectDatabase Shawn O. Pearce 0 siblings, 1 reply; 6+ messages in thread From: Shawn O. Pearce @ 2009-04-23 3:36 UTC (permalink / raw) To: Robin Rosenberg; +Cc: git These are declared by the base class, ObjectLoader. Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- .../org/spearce/jgit/lib/UnpackedObjectLoader.java | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java b/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java index 1352b72..e2de41c 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java @@ -192,10 +192,12 @@ private void decompress(final AnyObjectId id, final Inflater inf, int p) throw new CorruptObjectException(id, "incorrect length"); } + @Override public int getType() { return objectType; } + @Override public long getSize() { return objectSize; } -- 1.6.3.rc1.205.g37f8 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [JGIT PATCH 3/5] Fully materialize an ObjectLoader before returning it from ObjectDatabase 2009-04-23 3:36 ` [JGIT PATCH 2/5] Add missing @Override annotations to UnpackedObjectLoader Shawn O. Pearce @ 2009-04-23 3:36 ` Shawn O. Pearce 2009-04-23 3:36 ` [JGIT PATCH 4/5] Test that ObjectLoader stays valid across repacks Shawn O. Pearce 0 siblings, 1 reply; 6+ messages in thread From: Shawn O. Pearce @ 2009-04-23 3:36 UTC (permalink / raw) To: Robin Rosenberg; +Cc: git By forcing the ObjectLoader to fully reconstruct the requested object and store it under getCachedBytes() before we return the ObjectLoader to the caller we can ensure that the underlying PackFile remains valid throughout the entire load. If memory pressure causes the PackFile's windows to be evicted, and the pack closes, and is replaced underneath of us, we can correctly catch and handle the PackMismatchException or IOException by closing the pack, rescanning the ObjectDirectory, and starting our search all over again. Doing so is expensive, as we may have already unpacked a large part of the object's delta chain before finding the pack was modified, but its better than failing outright due to a race condition. Note that this does not resolve the concurrent access problems in PackWriter, as the cached PackedObjectLoaders are referenced very late, and go back to the original PackFile, not the cached store. Because we fully access the pack data up front during materialize() we no longer need to hold the WindowCursor inside of the individual loader instance. Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- .../jgit/lib/DeltaOfsPackedObjectLoader.java | 7 ++- .../spearce/jgit/lib/DeltaPackedObjectLoader.java | 41 +++++++--------- .../jgit/lib/DeltaRefPackedObjectLoader.java | 9 ++-- .../src/org/spearce/jgit/lib/ObjectDirectory.java | 3 +- .../src/org/spearce/jgit/lib/ObjectLoader.java | 23 +++------- .../src/org/spearce/jgit/lib/PackFile.java | 14 +++--- .../src/org/spearce/jgit/lib/PackWriter.java | 4 +- .../org/spearce/jgit/lib/PackedObjectLoader.java | 48 +++++++++++++++---- .../spearce/jgit/lib/WholePackedObjectLoader.java | 21 +++++---- 9 files changed, 94 insertions(+), 76 deletions(-) diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/DeltaOfsPackedObjectLoader.java b/org.spearce.jgit/src/org/spearce/jgit/lib/DeltaOfsPackedObjectLoader.java index 5c9fb00..ee2a0de 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/DeltaOfsPackedObjectLoader.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/DeltaOfsPackedObjectLoader.java @@ -46,14 +46,15 @@ class DeltaOfsPackedObjectLoader extends DeltaPackedObjectLoader { private final long deltaBase; - DeltaOfsPackedObjectLoader(final WindowCursor curs, final PackFile pr, + DeltaOfsPackedObjectLoader(final PackFile pr, final long dataOffset, final long objectOffset, final int deltaSz, final long base) { - super(curs, pr, dataOffset, objectOffset, deltaSz); + super(pr, dataOffset, objectOffset, deltaSz); deltaBase = base; } - protected PackedObjectLoader getBaseLoader() throws IOException { + protected PackedObjectLoader getBaseLoader(final WindowCursor curs) + throws IOException { return pack.resolveBase(curs, deltaBase); } diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/DeltaPackedObjectLoader.java b/org.spearce.jgit/src/org/spearce/jgit/lib/DeltaPackedObjectLoader.java index 85f2bda..8194d94 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/DeltaPackedObjectLoader.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/DeltaPackedObjectLoader.java @@ -49,47 +49,39 @@ private final int deltaSize; - DeltaPackedObjectLoader(final WindowCursor curs, final PackFile pr, - final long dataOffset, final long objectOffset, final int deltaSz) { - super(curs, pr, dataOffset, objectOffset); + DeltaPackedObjectLoader(final PackFile pr, final long dataOffset, + final long objectOffset, final int deltaSz) { + super(pr, dataOffset, objectOffset); objectType = -1; deltaSize = deltaSz; } - public int getType() throws IOException { - if (objectType < 0) - getCachedBytes(); - return objectType; - } - - public long getSize() throws IOException { - if (objectType < 0) - getCachedBytes(); - return objectSize; - } - @Override - public byte[] getCachedBytes() throws IOException { + public void materialize(final WindowCursor curs) throws IOException { + if (cachedBytes != null) { + return; + } + if (objectType != OBJ_COMMIT) { final UnpackedObjectCache.Entry cache = pack.readCache(dataOffset); if (cache != null) { curs.release(); objectType = cache.type; objectSize = cache.data.length; - return cache.data; + cachedBytes = cache.data; } } try { - final PackedObjectLoader baseLoader = getBaseLoader(); - final byte[] data = BinaryDelta.apply(baseLoader.getCachedBytes(), + final PackedObjectLoader baseLoader = getBaseLoader(curs); + baseLoader.materialize(curs); + cachedBytes = BinaryDelta.apply(baseLoader.getCachedBytes(), pack.decompress(dataOffset, deltaSize, curs)); curs.release(); objectType = baseLoader.getType(); - objectSize = data.length; + objectSize = cachedBytes.length; if (objectType != OBJ_COMMIT) - pack.saveCache(dataOffset, data, objectType); - return data; + pack.saveCache(dataOffset, cachedBytes, objectType); } catch (DataFormatException dfe) { final CorruptObjectException coe; coe = new CorruptObjectException("Object at " + dataOffset + " in " @@ -105,8 +97,11 @@ public long getRawSize() { } /** + * @param curs + * temporary thread storage during data access. * @return the object loader for the base object * @throws IOException */ - protected abstract PackedObjectLoader getBaseLoader() throws IOException; + protected abstract PackedObjectLoader getBaseLoader(WindowCursor curs) + throws IOException; } diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/DeltaRefPackedObjectLoader.java b/org.spearce.jgit/src/org/spearce/jgit/lib/DeltaRefPackedObjectLoader.java index b126bbd..82a3a7f 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/DeltaRefPackedObjectLoader.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/DeltaRefPackedObjectLoader.java @@ -46,14 +46,15 @@ class DeltaRefPackedObjectLoader extends DeltaPackedObjectLoader { private final ObjectId deltaBase; - DeltaRefPackedObjectLoader(final WindowCursor curs, final PackFile pr, + DeltaRefPackedObjectLoader(final PackFile pr, final long dataOffset, final long objectOffset, final int deltaSz, final ObjectId base) { - super(curs, pr, dataOffset, objectOffset, deltaSz); + super(pr, dataOffset, objectOffset, deltaSz); deltaBase = base; } - protected PackedObjectLoader getBaseLoader() throws IOException { + protected PackedObjectLoader getBaseLoader(final WindowCursor curs) + throws IOException { final PackedObjectLoader or = pack.get(curs, deltaBase); if (or == null) throw new MissingObjectException(deltaBase, "delta base"); @@ -61,7 +62,7 @@ protected PackedObjectLoader getBaseLoader() throws IOException { } @Override - public int getRawType() throws IOException { + public int getRawType() { return Constants.OBJ_REF_DELTA; } diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java index 6ba0180..3e9ed1c 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java @@ -197,8 +197,9 @@ protected ObjectLoader openObject1(final WindowCursor curs, SEARCH: for (;;) { for (final PackFile p : pList) { try { - final ObjectLoader ldr = p.get(curs, objectId); + final PackedObjectLoader ldr = p.get(curs, objectId); if (ldr != null) { + ldr.materialize(curs); return ldr; } } catch (PackMismatchException e) { diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectLoader.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectLoader.java index 5485d8d..6b92694 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectLoader.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectLoader.java @@ -38,7 +38,6 @@ package org.spearce.jgit.lib; -import java.io.IOException; /** * Base class for a set of loaders for different representations of Git objects. @@ -47,15 +46,13 @@ public abstract class ObjectLoader { /** * @return Git in pack object type, see {@link Constants}. - * @throws IOException */ - public abstract int getType() throws IOException; + public abstract int getType(); /** * @return size of object in bytes - * @throws IOException */ - public abstract long getSize() throws IOException; + public abstract long getSize(); /** * Obtain a copy of the bytes of this object. @@ -64,10 +61,8 @@ * be modified by the caller. * * @return the bytes of this object. - * @throws IOException - * the object cannot be read. */ - public final byte[] getBytes() throws IOException { + public final byte[] getBytes() { final byte[] data = getCachedBytes(); final byte[] copy = new byte[data.length]; System.arraycopy(data, 0, copy, 0, data.length); @@ -83,25 +78,19 @@ * Changes (if made) will affect the cache but not the repository itself. * * @return the cached bytes of this object. Do not modify it. - * @throws IOException - * the object cannot be read. */ - public abstract byte[] getCachedBytes() throws IOException; + public abstract byte[] getCachedBytes(); /** * @return raw object type from object header, as stored in storage (pack, * loose file). This may be different from {@link #getType()} result * for packs (see {@link Constants}). - * @throws IOException - * when type cannot be read from the object header. */ - public abstract int getRawType() throws IOException; + public abstract int getRawType(); /** * @return raw size of object from object header (pack, loose file). * Interpretation of this value depends on {@link #getRawType()}. - * @throws IOException - * when raw size cannot be read from the object header. */ - public abstract long getRawSize() throws IOException; + public abstract long getRawSize(); } diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java index dccc067..813ebc7 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java @@ -244,11 +244,11 @@ final void saveCache(final long position, final byte[] data, final int type) { } final void copyRawData(final PackedObjectLoader loader, - final OutputStream out, final byte buf[]) throws IOException { + final OutputStream out, final byte buf[], final WindowCursor curs) + throws IOException { final long objectOffset = loader.objectOffset; final long dataOffset = loader.dataOffset; final int cnt = (int) (findEndOffset(objectOffset) - dataOffset); - final WindowCursor curs = loader.curs; final PackIndex idx = idx(); if (idx.hasCRC32Support()) { @@ -436,8 +436,8 @@ private PackedObjectLoader reader(final WindowCursor curs, case Constants.OBJ_TREE: case Constants.OBJ_BLOB: case Constants.OBJ_TAG: - return new WholePackedObjectLoader(curs, this, pos, objOffset, - typeCode, (int) dataSize); + return new WholePackedObjectLoader(this, pos, objOffset, typeCode, + (int) dataSize); case Constants.OBJ_OFS_DELTA: { readFully(pos, ib, 0, 20, curs); @@ -450,12 +450,12 @@ private PackedObjectLoader reader(final WindowCursor curs, ofs <<= 7; ofs += (c & 127); } - return new DeltaOfsPackedObjectLoader(curs, this, pos + p, - objOffset, (int) dataSize, objOffset - ofs); + return new DeltaOfsPackedObjectLoader(this, pos + p, objOffset, + (int) dataSize, objOffset - ofs); } case Constants.OBJ_REF_DELTA: { readFully(pos, ib, 0, 20, curs); - return new DeltaRefPackedObjectLoader(curs, this, pos + ib.length, + return new DeltaRefPackedObjectLoader(this, pos + ib.length, objOffset, (int) dataSize, ObjectId.fromRaw(ib)); } default: diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java index c2e6c4b..e1397fd 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java @@ -720,7 +720,7 @@ private void writeWholeObject(final ObjectToPack otp) throws IOException { if (otp.hasReuseLoader()) { final PackedObjectLoader loader = otp.getReuseLoader(); writeObjectHeader(otp.getType(), loader.getSize()); - loader.copyRawData(out, buf); + loader.copyRawData(out, buf, windowCursor); otp.disposeLoader(); } else { final ObjectLoader loader = db.openObject(windowCursor, otp); @@ -756,7 +756,7 @@ private void writeDeltaObject(final ObjectToPack otp) throws IOException { otp.getDeltaBaseId().copyRawTo(buf, 0); out.write(buf, 0, Constants.OBJECT_ID_LENGTH); } - loader.copyRawData(out, buf); + loader.copyRawData(out, buf, windowCursor); otp.disposeLoader(); } diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackedObjectLoader.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackedObjectLoader.java index 56985c1..60333e7 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackedObjectLoader.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackedObjectLoader.java @@ -47,8 +47,6 @@ abstract class PackedObjectLoader extends ObjectLoader { protected final PackFile pack; - protected final WindowCursor curs; - protected final long dataOffset; protected final long objectOffset; @@ -57,26 +55,53 @@ protected int objectSize; - PackedObjectLoader(final WindowCursor c, final PackFile pr, - final long dataOffset, final long objectOffset) { - curs = c; + protected byte[] cachedBytes; + + PackedObjectLoader(final PackFile pr, final long dataOffset, + final long objectOffset) { pack = pr; this.dataOffset = dataOffset; this.objectOffset = objectOffset; } - public int getType() throws IOException { + /** + * Force this object to be loaded into memory and pinned in this loader. + * <p> + * Once materialized, subsequent get operations for the following methods + * will always succeed without raising an exception, as all information is + * pinned in memory by this loader instance. + * <ul> + * <li>{@link #getType()}</li> + * <li>{@link #getSize()}</li> + * <li>{@link #getBytes()}, {@link #getCachedBytes}</li> + * <li>{@link #getRawSize()}</li> + * <li>{@link #getRawType()}</li> + * </ul> + * + * @param curs + * temporary thread storage during data access. + * @throws IOException + * the object cannot be read. + */ + public abstract void materialize(WindowCursor curs) throws IOException; + + public final int getType() { return objectType; } - public long getSize() throws IOException { + public final long getSize() { return objectSize; } + @Override + public final byte[] getCachedBytes() { + return cachedBytes; + } + /** * @return offset of object data within pack file */ - public long getDataOffset() { + public final long getDataOffset() { return dataOffset; } @@ -92,11 +117,14 @@ public long getDataOffset() { * @param buf * temporary buffer used during copying. Recommended size is at * least few kB. + * @param curs + * temporary thread storage during data access. * @throws IOException * when the object cannot be read. */ - public void copyRawData(OutputStream out, byte buf[]) throws IOException { - pack.copyRawData(this, out, buf); + public void copyRawData(OutputStream out, byte buf[], WindowCursor curs) + throws IOException { + pack.copyRawData(this, out, buf, curs); } /** diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WholePackedObjectLoader.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WholePackedObjectLoader.java index 3b4f90d..a1df3d2 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/WholePackedObjectLoader.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WholePackedObjectLoader.java @@ -46,30 +46,33 @@ class WholePackedObjectLoader extends PackedObjectLoader { private static final int OBJ_COMMIT = Constants.OBJ_COMMIT; - WholePackedObjectLoader(final WindowCursor curs, final PackFile pr, - final long dataOffset, final long objectOffset, final int type, - final int size) { - super(curs, pr, dataOffset, objectOffset); + WholePackedObjectLoader(final PackFile pr, final long dataOffset, + final long objectOffset, final int type, final int size) { + super(pr, dataOffset, objectOffset); objectType = type; objectSize = size; } @Override - public byte[] getCachedBytes() throws IOException { + public void materialize(final WindowCursor curs) throws IOException { + if (cachedBytes != null) { + return; + } + if (objectType != OBJ_COMMIT) { final UnpackedObjectCache.Entry cache = pack.readCache(dataOffset); if (cache != null) { curs.release(); - return cache.data; + cachedBytes = cache.data; + return; } } try { - final byte[] data = pack.decompress(dataOffset, objectSize, curs); + cachedBytes = pack.decompress(dataOffset, objectSize, curs); curs.release(); if (objectType != OBJ_COMMIT) - pack.saveCache(dataOffset, data, objectType); - return data; + pack.saveCache(dataOffset, cachedBytes, objectType); } catch (DataFormatException dfe) { final CorruptObjectException coe; coe = new CorruptObjectException("Object at " + dataOffset + " in " -- 1.6.3.rc1.205.g37f8 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [JGIT PATCH 4/5] Test that ObjectLoader stays valid across repacks 2009-04-23 3:36 ` [JGIT PATCH 3/5] Fully materialize an ObjectLoader before returning it from ObjectDatabase Shawn O. Pearce @ 2009-04-23 3:36 ` Shawn O. Pearce 2009-04-23 3:36 ` [JGIT RFC PATCH 5/5] Teach PackWriter to recover from removed/replaced packs Shawn O. Pearce 0 siblings, 1 reply; 6+ messages in thread From: Shawn O. Pearce @ 2009-04-23 3:36 UTC (permalink / raw) To: Robin Rosenberg; +Cc: git What we are trying to verify is that an ObjectLoader remains valid if the underlying storage for the object has moved, such as when a repository is repacked, the old pack was deleted, and the object is now in the new pack. Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- .../org/spearce/jgit/lib/ConcurrentRepackTest.java | 40 ++++++++++++++++++++ 1 files changed, 40 insertions(+), 0 deletions(-) diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java index 825fbb8..b56e0f4 100644 --- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java +++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java @@ -41,6 +41,7 @@ import java.io.File; import java.io.FileOutputStream; import java.io.IOException; +import java.util.Arrays; import org.spearce.jgit.errors.IncorrectObjectTypeException; import org.spearce.jgit.errors.MissingObjectException; @@ -128,6 +129,45 @@ public void testObjectMovedWithinPack() assertEquals(o2.name(), parse(o2).name()); } + public void testObjectMovedToNewPack2() + throws IncorrectObjectTypeException, IOException { + // Create an object and pack it. Then remove that pack and put the + // object into a different pack file, with some other object. We + // still should be able to access the objects. + // + final Repository eden = createNewEmptyRepo(); + final RevObject o1 = writeBlob(eden, "o1"); + final File[] out1 = pack(eden, o1); + assertEquals(o1.name(), parse(o1).name()); + + final ObjectLoader load1 = db.openBlob(o1); + assertNotNull(load1); + + final RevObject o2 = writeBlob(eden, "o2"); + pack(eden, o2, o1); + + // Force close, and then delete, the old pack. + // + whackCache(); + delete(out1); + + // Now here is the interesting thing... can the loader we made + // earlier still resolve the object, even though its underlying + // pack is gone, but the object still exists. + // + final ObjectLoader load2 = db.openBlob(o1); + assertNotNull(load2); + assertNotSame(load1, load2); + + final byte[] data2 = load2.getCachedBytes(); + final byte[] data1 = load1.getCachedBytes(); + assertNotNull(data2); + assertNotNull(data1); + assertNotSame(data1, data2); // cache should be per-pack, not per object + assertTrue(Arrays.equals(data1, data2)); + assertEquals(load2.getType(), load1.getType()); + } + private static void whackCache() { final WindowCacheConfig config = new WindowCacheConfig(); -- 1.6.3.rc1.205.g37f8 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [JGIT RFC PATCH 5/5] Teach PackWriter to recover from removed/replaced packs 2009-04-23 3:36 ` [JGIT PATCH 4/5] Test that ObjectLoader stays valid across repacks Shawn O. Pearce @ 2009-04-23 3:36 ` Shawn O. Pearce 0 siblings, 0 replies; 6+ messages in thread From: Shawn O. Pearce @ 2009-04-23 3:36 UTC (permalink / raw) To: Robin Rosenberg; +Cc: git A concurrently running "git gc" in the same repository could cause a PackFile that was previously identified for object reuse to disappear (or be rewritten) between the time that a segment was selected to be reused, and when we actually need to copy the raw data from the pack to the output stream. We now peg the pack file open during the reuse period, ensuring that the underlying file descriptor cannot be closed while we are copying data, even if memory pressure gets high and windows are evicted from the WindowCache. If the pack file is gone (or has been rewritten) since we originally picked it for reuse we throw away that reuse decision and make it again. This is a relative waste of CPU and disk IO, as we have to do work twice, but it should be fairly infrequent as repositories are not repacked that often. If we do have to recompute one object, it is likely that we may need to recompute all reuse decisions for the remainder of this pack output stream, but doing a bit more work and succeeding is better than failing outright with an obtuse error. The way we handle the recovery is subject to livelock. We could pick a new reuse location, see it disappear before we can get a pin, and need to select another one. Livelock however is not likely here, as the situation can only happen when the selected pack file has been deleted or overwritten. Repacking takes some time on any repository, typically longer than a single PackWriter may need to stream to a network client. It is highly improbable that the repository administator is running "while true; git gc; done", as that would suck up all system resources and generally make the host unresponsive anyway. So, long story short, we should be OK against livelock if the repository administrator isn't hellbent on otherwise sucking up CPU usage through repeat git gc attempts. And if they are, we'll just help them out by falling for the obvious livelock case. Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- As I said in the cover letter of this series; this one is still iffy. I think its sound as-is, but I'm not convinced that this patch is sufficient to cover everything that can happen. .../src/org/spearce/jgit/lib/PackWriter.java | 118 +++++++++++++------- .../org/spearce/jgit/lib/PackedObjectLoader.java | 30 +++++ .../src/org/spearce/jgit/lib/WindowCache.java | 59 +++++++---- 3 files changed, 145 insertions(+), 62 deletions(-) diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java index e1397fd..2da8bbc 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java @@ -606,14 +606,7 @@ private void searchForReuse() throws IOException { throw new IOException( "Packing cancelled during objects writing"); reuseLoaders.clear(); - db.openObjectInAllPacks(otp, reuseLoaders, windowCursor); - if (reuseDeltas) { - selectDeltaReuseForObject(otp, reuseLoaders); - } - // delta reuse is preferred over object reuse - if (reuseObjects && !otp.hasReuseLoader()) { - selectObjectReuseForObject(otp, reuseLoaders); - } + searchForReuse(reuseLoaders, otp); initMonitor.update(1); } } @@ -621,6 +614,19 @@ private void searchForReuse() throws IOException { initMonitor.endTask(); } + private void searchForReuse( + final Collection<PackedObjectLoader> reuseLoaders, + final ObjectToPack otp) throws IOException { + db.openObjectInAllPacks(otp, reuseLoaders, windowCursor); + if (reuseDeltas) { + selectDeltaReuseForObject(otp, reuseLoaders); + } + // delta reuse is preferred over object reuse + if (reuseObjects && !otp.hasReuseLoader()) { + selectObjectReuseForObject(otp, reuseLoaders); + } + } + private void selectDeltaReuseForObject(final ObjectToPack otp, final Collection<PackedObjectLoader> loaders) throws IOException { PackedObjectLoader bestLoader = null; @@ -707,40 +713,69 @@ private void writeObject(final ObjectToPack otp) throws IOException { out.resetCRC32(); otp.setOffset(out.length()); - if (otp.isDeltaRepresentation()) - writeDeltaObject(otp); - else - writeWholeObject(otp); + + final PackedObjectLoader reuse = open(otp); + if (reuse != null) { + try { + if (otp.isDeltaRepresentation()) { + writeDeltaObjectReuse(otp, reuse); + } else { + writeObjectHeader(otp.getType(), reuse.getSize()); + reuse.copyRawData(out, buf, windowCursor); + } + } finally { + reuse.endCopyRawData(); + } + } else if (otp.isDeltaRepresentation()) { + throw new IOException("creating deltas is not implemented"); + } else { + writeWholeObjectDeflate(otp); + } otp.setCRC(out.getCRC32()); writeMonitor.update(1); } - private void writeWholeObject(final ObjectToPack otp) throws IOException { - if (otp.hasReuseLoader()) { - final PackedObjectLoader loader = otp.getReuseLoader(); - writeObjectHeader(otp.getType(), loader.getSize()); - loader.copyRawData(out, buf, windowCursor); - otp.disposeLoader(); - } else { - final ObjectLoader loader = db.openObject(windowCursor, otp); - final byte[] data = loader.getCachedBytes(); - writeObjectHeader(otp.getType(), data.length); - deflater.reset(); - deflater.setInput(data, 0, data.length); - deflater.finish(); - do { - final int n = deflater.deflate(buf, 0, buf.length); - if (n > 0) - out.write(buf, 0, n); - } while (!deflater.finished()); - } - } - - private void writeDeltaObject(final ObjectToPack otp) throws IOException { - final PackedObjectLoader loader = otp.getReuseLoader(); + private PackedObjectLoader open(final ObjectToPack otp) throws IOException { + for (;;) { + PackedObjectLoader reuse = otp.useLoader(); + if (reuse == null) { + return null; + } + + try { + reuse.beginCopyRawData(); + return reuse; + } catch (IOException err) { + // The pack we found the object in originally is gone, or + // it has been overwritten with a different layout. + // + otp.clearDeltaBase(); + searchForReuse(new ArrayList<PackedObjectLoader>(), otp); + continue; + } + } + } + + private void writeWholeObjectDeflate(final ObjectToPack otp) + throws IOException { + final ObjectLoader loader = db.openObject(windowCursor, otp); + final byte[] data = loader.getCachedBytes(); + writeObjectHeader(otp.getType(), data.length); + deflater.reset(); + deflater.setInput(data, 0, data.length); + deflater.finish(); + do { + final int n = deflater.deflate(buf, 0, buf.length); + if (n > 0) + out.write(buf, 0, n); + } while (!deflater.finished()); + } + + private void writeDeltaObjectReuse(final ObjectToPack otp, + final PackedObjectLoader reuse) throws IOException { if (deltaBaseAsOffset && otp.getDeltaBase() != null) { - writeObjectHeader(Constants.OBJ_OFS_DELTA, loader.getRawSize()); + writeObjectHeader(Constants.OBJ_OFS_DELTA, reuse.getRawSize()); final ObjectToPack deltaBase = otp.getDeltaBase(); long offsetDiff = otp.getOffset() - deltaBase.getOffset(); @@ -752,12 +787,11 @@ private void writeDeltaObject(final ObjectToPack otp) throws IOException { out.write(buf, pos, buf.length - pos); } else { - writeObjectHeader(Constants.OBJ_REF_DELTA, loader.getRawSize()); + writeObjectHeader(Constants.OBJ_REF_DELTA, reuse.getRawSize()); otp.getDeltaBaseId().copyRawTo(buf, 0); out.write(buf, 0, Constants.OBJECT_ID_LENGTH); } - loader.copyRawData(out, buf, windowCursor); - otp.disposeLoader(); + reuse.copyRawData(out, buf, windowCursor); } private void writeObjectHeader(final int objectType, long dataLength) @@ -955,8 +989,10 @@ boolean isWritten() { return getOffset() != 0; } - PackedObjectLoader getReuseLoader() { - return reuseLoader; + PackedObjectLoader useLoader() { + final PackedObjectLoader r = reuseLoader; + reuseLoader = null; + return r; } boolean hasReuseLoader() { diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackedObjectLoader.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackedObjectLoader.java index 60333e7..6066ba1 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackedObjectLoader.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackedObjectLoader.java @@ -106,6 +106,30 @@ public final long getDataOffset() { } /** + * Peg the pack file open to support data copying. + * <p> + * Applications trying to copy raw pack data should ensure the pack stays + * open and available throughout the entire copy. To do that use: + * + * <pre> + * loader.beginCopyRawData(); + * try { + * loader.copyRawData(out, tmpbuf, curs); + * } finally { + * loader.endCopyRawData(); + * } + * </pre> + * + * @throws IOException + * this loader contains stale information and cannot be used. + * The most likely cause is the underlying pack file has been + * deleted, and the object has moved to another pack file. + */ + public void beginCopyRawData() throws IOException { + WindowCache.pin(pack); + } + + /** * Copy raw object representation from storage to provided output stream. * <p> * Copied data doesn't include object header. User must provide temporary @@ -121,12 +145,18 @@ public final long getDataOffset() { * temporary thread storage during data access. * @throws IOException * when the object cannot be read. + * @see #beginCopyRawData() */ public void copyRawData(OutputStream out, byte buf[], WindowCursor curs) throws IOException { pack.copyRawData(this, out, buf, curs); } + /** Release resources after {@link #beginCopyRawData()}. */ + public void endCopyRawData() { + WindowCache.unpin(pack); + } + /** * @return true if this loader is capable of fast raw-data copying basing on * compressed data checksum; false if raw-data copying needs diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java index 03e531a..51d149c 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java @@ -223,6 +223,19 @@ public static final void get(final WindowCursor curs, final PackFile wp, curs.window.ensureLoaded(curs.handle); } + static synchronized final void pin(final PackFile wp) throws IOException { + if (++wp.openCount == 1) { + openFile(wp); + } + } + + static synchronized final void unpin(final PackFile wp) { + if (--wp.openCount == 0) { + openFileCount--; + wp.cacheClose(); + } + } + private static synchronized final void getImpl(final WindowCursor curs, final PackFile wp, final long position) throws IOException { final int id = (int) (position >> windowSizeShift); @@ -241,27 +254,7 @@ private static synchronized final void getImpl(final WindowCursor curs, } if (wp.openCount == 0) { - try { - openFileCount++; - releaseMemory(); - runClearedWindowQueue(); - wp.openCount = 1; - wp.cacheOpen(); - } catch (IOException ioe) { - openFileCount--; - wp.openCount = 0; - throw ioe; - } catch (RuntimeException ioe) { - openFileCount--; - wp.openCount = 0; - throw ioe; - } catch (Error ioe) { - openFileCount--; - wp.openCount = 0; - throw ioe; - } finally { - wp.openCount--; - } + openFile(wp); // The cacheOpen may have mapped the window we are trying to // map ourselves. Retrying the search ensures that does not @@ -294,6 +287,30 @@ private static synchronized final void getImpl(final WindowCursor curs, insertLRU(e); } + private static void openFile(final PackFile wp) throws IOException { + try { + openFileCount++; + releaseMemory(); + runClearedWindowQueue(); + wp.openCount = 1; + wp.cacheOpen(); + } catch (IOException ioe) { + openFileCount--; + wp.openCount = 0; + throw ioe; + } catch (RuntimeException ioe) { + openFileCount--; + wp.openCount = 0; + throw ioe; + } catch (Error ioe) { + openFileCount--; + wp.openCount = 0; + throw ioe; + } finally { + wp.openCount--; + } + } + static synchronized void markLoaded(final ByteWindow w) { if (--w.provider.openCount == 0) { openFileCount--; -- 1.6.3.rc1.205.g37f8 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-04-23 3:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-04-23 3:36 [JGIT PATCH 0/5] Make ObjectLoader safer during repack Shawn O. Pearce 2009-04-23 3:36 ` [JGIT PATCH 1/5] Remove throws IOException from UnpackedObjectLoader.getCachedBytes Shawn O. Pearce 2009-04-23 3:36 ` [JGIT PATCH 2/5] Add missing @Override annotations to UnpackedObjectLoader Shawn O. Pearce 2009-04-23 3:36 ` [JGIT PATCH 3/5] Fully materialize an ObjectLoader before returning it from ObjectDatabase Shawn O. Pearce 2009-04-23 3:36 ` [JGIT PATCH 4/5] Test that ObjectLoader stays valid across repacks Shawn O. Pearce 2009-04-23 3:36 ` [JGIT RFC PATCH 5/5] Teach PackWriter to recover from removed/replaced packs Shawn O. Pearce
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.