* [BUG] t9801 and t9803 broken on next @ 2016-05-13 10:02 Lars Schneider 2016-05-13 10:36 ` Eric Wong 0 siblings, 1 reply; 19+ messages in thread From: Lars Schneider @ 2016-05-13 10:02 UTC (permalink / raw) To: Git Users; +Cc: normalperson, Luke Diamand Hi, t9801 and t9803 seem to be broken on next. A quick bisect indicates that d9545c7 "fast-import: implement unpack limit" might be the reason. (@gmane/292562). Did anyone look into that already? Thanks, Lars ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] t9801 and t9803 broken on next 2016-05-13 10:02 [BUG] t9801 and t9803 broken on next Lars Schneider @ 2016-05-13 10:36 ` Eric Wong 2016-05-13 16:37 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Eric Wong @ 2016-05-13 10:36 UTC (permalink / raw) To: Lars Schneider; +Cc: Git Users, Luke Diamand Lars Schneider <larsxschneider@gmail.com> wrote: > Hi, > > t9801 and t9803 seem to be broken on next. A quick bisect indicates that d9545c7 > "fast-import: implement unpack limit" might be the reason. (@gmane/292562). > > Did anyone look into that already? Just took a look (no p4, so couldn't run tests) and I guess it's because the default changed and it doesn't generate tiny packs. The easiest change is probably to call: git config fastimport.unpackLimit 0 during setup like t9300 now does. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] t9801 and t9803 broken on next 2016-05-13 10:36 ` Eric Wong @ 2016-05-13 16:37 ` Junio C Hamano 2016-05-14 8:04 ` Lars Schneider 0 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2016-05-13 16:37 UTC (permalink / raw) To: Eric Wong; +Cc: Lars Schneider, Git Users, Luke Diamand Eric Wong <e@80x24.org> writes: > Lars Schneider <larsxschneider@gmail.com> wrote: >> Hi, >> >> t9801 and t9803 seem to be broken on next. A quick bisect indicates that d9545c7 >> "fast-import: implement unpack limit" might be the reason. (@gmane/292562). >> >> Did anyone look into that already? > > Just took a look (no p4, so couldn't run tests) and I guess it's > because the default changed and it doesn't generate tiny packs. I looked at t9801 but I couldn't spot where it cares if the objects are in a (tiny) pack or stored as individual loose objects. All the counting I saw was about rev-list/log output and they shouldn't care. Are you saying that "git p4" itself breaks unless fast-import always writes a new (tiny) packfile? That sounds quite broken, and setting unpacklimit to 0 does not sound like a sensible "fix". Of course, if the test script is somehow looking at the number of packs or loose objects and declaring a failure, even when the resulting history in p4 and git are correct, then that is a different issue, and forcing to explode a tiny pack is a reasonable workaround. I couldn't quite tell which the case is. Puzzled. Please help. > The easiest change is probably to call: > > git config fastimport.unpackLimit 0 > > during setup like t9300 now does. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] t9801 and t9803 broken on next 2016-05-13 16:37 ` Junio C Hamano @ 2016-05-14 8:04 ` Lars Schneider 2016-05-14 18:15 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Lars Schneider @ 2016-05-14 8:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: Eric Wong, Git Users, Luke Diamand > On 13 May 2016, at 18:37, Junio C Hamano <gitster@pobox.com> wrote: > > Eric Wong <e@80x24.org> writes: > >> Lars Schneider <larsxschneider@gmail.com> wrote: >>> Hi, >>> >>> t9801 and t9803 seem to be broken on next. A quick bisect indicates that d9545c7 >>> "fast-import: implement unpack limit" might be the reason. (@gmane/292562). >>> >>> Did anyone look into that already? >> >> Just took a look (no p4, so couldn't run tests) and I guess it's >> because the default changed and it doesn't generate tiny packs. > > I looked at t9801 but I couldn't spot where it cares if the objects > are in a (tiny) pack or stored as individual loose objects. All the > counting I saw was about rev-list/log output and they shouldn't care. > > Are you saying that "git p4" itself breaks unless fast-import always > writes a new (tiny) packfile? That sounds quite broken, and setting > unpacklimit to 0 does not sound like a sensible "fix". Of course, > if the test script is somehow looking at the number of packs or > loose objects and declaring a failure, even when the resulting > history in p4 and git are correct, then that is a different issue, > and forcing to explode a tiny pack is a reasonable workaround. I > couldn't quite tell which the case is. > > Puzzled. Please help. t9801 "import depot, branch detection" is the first test that fails with a fast import error: https://github.com/git/git/blob/78b384c29366e199741393e56030a8384110760d/t/t9801-git-p4-branch.sh#L110 fast-import crash report: fast-import process: 77079 parent process : 77077 at 2016-05-14 07:48:40 +0000 fatal: offset beyond end of packfile (truncated pack?) Most Recent Commands Before Crash --------------------------------- commit refs/remotes/p4/master mark :1 committer Dr. author <author@example.com> 1463212118 +0000 data <<EOT M 100644 inline f1 data 3 commit refs/remotes/p4/master mark :2 committer Dr. author <author@example.com> 1463212118 +0000 data <<EOT M 100644 inline f2 data 3 commit refs/remotes/p4/master mark :4 committer Dr. author <author@example.com> 1463212118 +0000 data <<EOT M 100644 inline f4 data 3 checkpoint commit git-p4-tmp/6 mark :6 committer Dr. author <author@example.com> 1463212118 +0000 data <<EOT M 100644 inline f1 data 3 M 100644 inline f2 data 3 M 100644 inline f4 data 3 checkpoint progress checkpoint commit refs/remotes/p4/depot/branch2 mark :6 committer Dr. author <author@example.com> 1463212118 +0000 data <<EOT from 1ab0d5b43b6070682f8bd6f3fd674d4fcddd9345 * M 100644 inline f1 Active Branch LRU ----------------- active_branches = 2 cur, 5 max pos clock name ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1) 4 git-p4-tmp/6 2) 3 refs/remotes/p4/master Inactive Branches ----------------- git-p4-tmp/6: status : active loaded tip commit : 3aef1b3e7d821051780a89262f2e2fae33ed8af3 old tree : 30a8495449245cdb9626ef48902ba672cf76576f cur tree : 30a8495449245cdb9626ef48902ba672cf76576f commit clock: 4 last pack : 0 refs/remotes/p4/master: status : active loaded tip commit : 1ab0d5b43b6070682f8bd6f3fd674d4fcddd9345 old tree : 30a8495449245cdb9626ef48902ba672cf76576f cur tree : 30a8495449245cdb9626ef48902ba672cf76576f commit clock: 3 last pack : 0 refs/remotes/p4/depot/branch2: status : loaded tip commit : 1ab0d5b43b6070682f8bd6f3fd674d4fcddd9345 old tree : 30a8495449245cdb9626ef48902ba672cf76576f cur tree : 30a8495449245cdb9626ef48902ba672cf76576f commit clock: 0 last pack : Marks ----- :1 47474c2e749d579c63b375231c018289aa00cd0f :2 9c6b957c0b4d4991b8bbd17858aa2a271d2c76c3 :4 1ab0d5b43b6070682f8bd6f3fd674d4fcddd9345 :6 3aef1b3e7d821051780a89262f2e2fae33ed8af3 ------------------- END OF CRASH REPORT > >> The easiest change is probably to call: >> >> git config fastimport.unpackLimit 0 >> >> during setup like t9300 now does. Unfortunately that seems not to help. The problem seems to be related to the git-p4 branch import. I don't have time this weekend to look further into it, but I will try next week. Cheers, Lars ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] t9801 and t9803 broken on next 2016-05-14 8:04 ` Lars Schneider @ 2016-05-14 18:15 ` Junio C Hamano 2016-05-17 8:07 ` Lars Schneider 2016-05-17 8:35 ` Luke Diamand 0 siblings, 2 replies; 19+ messages in thread From: Junio C Hamano @ 2016-05-14 18:15 UTC (permalink / raw) To: Lars Schneider; +Cc: Eric Wong, Git Users, Luke Diamand Lars Schneider <larsxschneider@gmail.com> writes: >> On 13 May 2016, at 18:37, Junio C Hamano <gitster@pobox.com> wrote: >> >> Are you saying that "git p4" itself breaks unless fast-import always >> writes a new (tiny) packfile? That sounds quite broken, and setting >> unpacklimit to 0 does not sound like a sensible "fix". Of course, >> if the test script is somehow looking at the number of packs or >> loose objects and declaring a failure, even when the resulting >> history in p4 and git are correct, then that is a different issue, >> and forcing to explode a tiny pack is a reasonable workaround. I >> couldn't quite tell which the case is. >> >> Puzzled. Please help. > > t9801 "import depot, branch detection" is the first test that fails > with a fast import error: > https://github.com/git/git/blob/78b384c29366e199741393e56030a8384110760d/t/t9801-git-p4-branch.sh#L110 > > fast-import crash report: > fast-import process: 77079 > parent process : 77077 > at 2016-05-14 07:48:40 +0000 > > fatal: offset beyond end of packfile (truncated pack?) Hmm, does that suggest Eric's "explode them into loose instead of keeping a small pack" insufficient? It sounds like that somebody wanted to read some data back from its packfile without knowing that the updated code may make it available in a loose object form, which would mean that somebody needs to be told about what is going on, namely, these objects are not in a half-written pack but are found as loose objects. > The problem seems to be related to the git-p4 branch import. > I don't have time this weekend to look further into it, but > I will try next week. Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] t9801 and t9803 broken on next 2016-05-14 18:15 ` Junio C Hamano @ 2016-05-17 8:07 ` Lars Schneider 2016-05-17 9:13 ` Eric Wong 2016-05-17 12:13 ` Jeff King 2016-05-17 8:35 ` Luke Diamand 1 sibling, 2 replies; 19+ messages in thread From: Lars Schneider @ 2016-05-17 8:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: Eric Wong, Git Users, Luke Diamand > On 14 May 2016, at 20:15, Junio C Hamano <gitster@pobox.com> wrote: > > Lars Schneider <larsxschneider@gmail.com> writes: > >>> On 13 May 2016, at 18:37, Junio C Hamano <gitster@pobox.com> wrote: >>> >>> Are you saying that "git p4" itself breaks unless fast-import always >>> writes a new (tiny) packfile? That sounds quite broken, and setting >>> unpacklimit to 0 does not sound like a sensible "fix". Of course, >>> if the test script is somehow looking at the number of packs or >>> loose objects and declaring a failure, even when the resulting >>> history in p4 and git are correct, then that is a different issue, >>> and forcing to explode a tiny pack is a reasonable workaround. I >>> couldn't quite tell which the case is. >>> >>> Puzzled. Please help. >> >> t9801 "import depot, branch detection" is the first test that fails >> with a fast import error: >> https://github.com/git/git/blob/78b384c29366e199741393e56030a8384110760d/t/t9801-git-p4-branch.sh#L110 >> >> fast-import crash report: >> fast-import process: 77079 >> parent process : 77077 >> at 2016-05-14 07:48:40 +0000 >> >> fatal: offset beyond end of packfile (truncated pack?) > > Hmm, does that suggest Eric's "explode them into loose instead of > keeping a small pack" insufficient? It sounds like that somebody > wanted to read some data back from its packfile without knowing that > the updated code may make it available in a loose object form, which > would mean that somebody needs to be told about what is going on, > namely, these objects are not in a half-written pack but are found > as loose objects. I think that is pretty much the problem. Here is what is happening: 1. git-p4 imports all changelists for the "main" branch 2. git-p4 starts to import a second branch and creates a fast-import "progress checkpoint". This triggers: --> fast-import.c: cycle_packfile --> fast-import.c: end_packfile --> fast-import.c: loosen_small_pack At this point we have no packfile anymore. 3. git-p4 sends the command "commit refs/remotes/p4/depot/branch2" to fast-import in order to create a new branch. This triggers: --> fast-import.c: parse_new_commit --> fast-import.c: load_branch --> fast-import.c: load_tree "load_tree" calls "find_object" and the result has a "pack_id" of 0. I think this indicates that the object is in the packfile. Shouldn't the "pack_id" be "MAX_PACK_ID" instead? myoe = find_object(sha1); if (myoe && myoe->pack_id != MAX_PACK_ID) { --> fast-import.c: gfi_unpack_entry In "gfi_unpack_entry" this condition evaluates to "true": struct packed_git *p = all_packs[oe->pack_id]; if (p == pack_data && p->pack_size < (pack_size + 20)) In the comment below Git thinks that the object is stored in the packfile. This seems wrong but the flow continues: --> sha1_filec: unpack_entry --> sha1_filec: unpack_object_header --> sha1_filec: use_pack At this point fast-import dies with "offset beyond end of packfile (truncated pack?)". I am no expert in "fast-import". Does anyone have a few hints how to proceed? Thanks, Lars ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] t9801 and t9803 broken on next 2016-05-17 8:07 ` Lars Schneider @ 2016-05-17 9:13 ` Eric Wong 2016-05-17 12:13 ` Jeff King 1 sibling, 0 replies; 19+ messages in thread From: Eric Wong @ 2016-05-17 9:13 UTC (permalink / raw) To: Lars Schneider; +Cc: Junio C Hamano, Git Users, Luke Diamand Lars Schneider <larsxschneider@gmail.com> wrote: > I am no expert in "fast-import". Does anyone have a few hints how to > proceed? I don't know fast-import or the C internals of git well at all, either. But capturing the exact input to fast-import (using tee) would be useful for non-p4 users to debug the problem and would go a long way in reproducing a standalone test case. I'm Python-illiterate , but hopefully this works to capture the stdin fed to fast-import (totally untested): --- a/git-p4.py +++ b/git-p4.py @@ -3366,7 +3366,8 @@ class P4Sync(Command, P4UserMap): self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 3600) / 60)) - self.importProcess = subprocess.Popen(["git", "fast-import"], + cmd = [ "sh", "-c", "tee /tmp/gfi-in.$$ | git fast-import" ] + self.importProcess = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE); ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] t9801 and t9803 broken on next 2016-05-17 8:07 ` Lars Schneider 2016-05-17 9:13 ` Eric Wong @ 2016-05-17 12:13 ` Jeff King 2016-05-19 8:19 ` Lars Schneider 2016-05-25 22:49 ` Eric Wong 1 sibling, 2 replies; 19+ messages in thread From: Jeff King @ 2016-05-17 12:13 UTC (permalink / raw) To: Lars Schneider; +Cc: Junio C Hamano, Eric Wong, Git Users, Luke Diamand On Tue, May 17, 2016 at 10:07:16AM +0200, Lars Schneider wrote: > I think that is pretty much the problem. Here is what is happening: > > 1. git-p4 imports all changelists for the "main" branch > > 2. git-p4 starts to import a second branch and creates a fast-import > "progress checkpoint". This triggers: > > --> fast-import.c: cycle_packfile > --> fast-import.c: end_packfile > --> fast-import.c: loosen_small_pack > > At this point we have no packfile anymore. > > 3. git-p4 sends the command "commit refs/remotes/p4/depot/branch2" > to fast-import in order to create a new branch. This triggers: > > --> fast-import.c: parse_new_commit > --> fast-import.c: load_branch > --> fast-import.c: load_tree > > "load_tree" calls "find_object" and the result has a "pack_id" of 0. > I think this indicates that the object is in the packfile. Shouldn't > the "pack_id" be "MAX_PACK_ID" instead? > > myoe = find_object(sha1); > if (myoe && myoe->pack_id != MAX_PACK_ID) { Thanks for the analysis. I think this is definitely the problem. After fast-import finalizes a packfile (either at the end of the program or due to a checkpoint), it never discards its internal mapping of objects to pack locations. It _has_ to keep such a mapping before the pack is finalized, because git's regular object database doesn't know about it. But afterwards, it should be able to rely on the object database. Retaining the mapping at the end of the program is obviously OK because we won't make any more lookups in it. Retaining it at a checkpoint when we keep the packfile is OK, because we can (usually) still access that packfile (the exception would be if somebody runs "git repack" while fast-import is running). But obviously a checkpoint where we throw away the pack needs to clear that mapping. The patch below probably makes your case work, but there are a lot of open questions: 1. Should we always discard the mapping, even when not loosening objects? I can't think of a real downside to always using git's object lookups. 2. Can we free memory associated with object_entry structs at this point? They won't be accessible via the hash, but might other bits of the code have kept pointers to them? I suspect it may screw up the statistics that fast-import prints at the end, but that's a minor point. 3. I notice that a few other structs (branch and tag) hold onto the pack_id, which will now point to a pack we can't access. Does this matter? I don't think so, because checkpoint() seems to dump the branches and tags. 4. In general, should we be loosening objects at checkpoints at all? I guess it is probably more efficient than creating a bunch of little packs. And it should probably not happen much at all outside of tests (i.e., callers should generally checkpoint after an appreciable amount of work is done). diff --git a/fast-import.c b/fast-import.c index 0e8bc6a..9bfbfb0 100644 --- a/fast-import.c +++ b/fast-import.c @@ -597,6 +597,22 @@ static struct object_entry *insert_object(unsigned char *sha1) return e; } +static void clear_object_table(void) +{ + unsigned int h; + for (h = 0; h < ARRAY_SIZE(object_table); h++) { + /* + * We can't individually free objects here + * because they are allocated from a pool. + */ + object_table[h] = NULL; + } + /* + * XXX maybe free object_entry_pool here, + * or might something still be referencing them? + */ +} + static unsigned int hc_str(const char *s, size_t len) { unsigned int r = 0; @@ -1035,6 +1051,9 @@ discard_pack: pack_data = NULL; running = 0; + /* The objects are now available via git's regular lookups. */ + clear_object_table(); + /* We can't carry a delta across packfiles. */ strbuf_release(&last_blob.data); last_blob.offset = 0; ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [BUG] t9801 and t9803 broken on next 2016-05-17 12:13 ` Jeff King @ 2016-05-19 8:19 ` Lars Schneider 2016-05-19 17:03 ` Junio C Hamano 2016-05-25 22:49 ` Eric Wong 1 sibling, 1 reply; 19+ messages in thread From: Lars Schneider @ 2016-05-19 8:19 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Eric Wong, Git Users, Luke Diamand, spearce On 17 May 2016, at 14:13, Jeff King <peff@peff.net> wrote: > On Tue, May 17, 2016 at 10:07:16AM +0200, Lars Schneider wrote: > >> I think that is pretty much the problem. Here is what is happening: >> >> 1. git-p4 imports all changelists for the "main" branch >> >> 2. git-p4 starts to import a second branch and creates a fast-import >> "progress checkpoint". This triggers: >> >> --> fast-import.c: cycle_packfile >> --> fast-import.c: end_packfile >> --> fast-import.c: loosen_small_pack >> >> At this point we have no packfile anymore. >> >> 3. git-p4 sends the command "commit refs/remotes/p4/depot/branch2" >> to fast-import in order to create a new branch. This triggers: >> >> --> fast-import.c: parse_new_commit >> --> fast-import.c: load_branch >> --> fast-import.c: load_tree >> >> "load_tree" calls "find_object" and the result has a "pack_id" of 0. >> I think this indicates that the object is in the packfile. Shouldn't >> the "pack_id" be "MAX_PACK_ID" instead? >> >> myoe = find_object(sha1); >> if (myoe && myoe->pack_id != MAX_PACK_ID) { > > Thanks for the analysis. I think this is definitely the problem. After > fast-import finalizes a packfile (either at the end of the program or > due to a checkpoint), it never discards its internal mapping of objects > to pack locations. It _has_ to keep such a mapping before the pack is > finalized, because git's regular object database doesn't know about it. > But afterwards, it should be able to rely on the object database. > > Retaining the mapping at the end of the program is obviously OK because > we won't make any more lookups in it. > > Retaining it at a checkpoint when we keep the packfile is OK, because we > can (usually) still access that packfile (the exception would be if > somebody runs "git repack" while fast-import is running). > > But obviously a checkpoint where we throw away the pack needs to clear > that mapping. > > The patch below probably makes your case work, but there are a lot of > open questions: Confirmed. The offending tests pass with your patch. > 1. Should we always discard the mapping, even when not loosening > objects? I can't think of a real downside to always using git's > object lookups. > > 2. Can we free memory associated with object_entry structs at this > point? They won't be accessible via the hash, but might other bits > of the code have kept pointers to them? > > I suspect it may screw up the statistics that fast-import prints at > the end, but that's a minor point. > > 3. I notice that a few other structs (branch and tag) hold onto the > pack_id, which will now point to a pack we can't access. Does this > matter? I don't think so, because checkpoint() seems to dump the > branches and tags. > 4. In general, should we be loosening objects at checkpoints at all? > > I guess it is probably more efficient than creating a bunch of > little packs. And it should probably not happen much at all outside > of tests (i.e., callers should generally checkpoint after an > appreciable amount of work is done). I am not too familiar with the code and therefore I can't give a good answer to your questions. However, I noticed that Shawn (CC'ed) wrote a lot of fast-import code. Maybe he can help? >From my point of view little packs are no problem. I run fast-import on a dedicated migration machine. After fast-import completion I run repack [1] before I upload the repo to its final location. Thanks, Lars [1] https://gcc.gnu.org/ml/gcc/2007-12/msg00165.html > > diff --git a/fast-import.c b/fast-import.c > index 0e8bc6a..9bfbfb0 100644 > --- a/fast-import.c > +++ b/fast-import.c > @@ -597,6 +597,22 @@ static struct object_entry *insert_object(unsigned char *sha1) > return e; > } > > +static void clear_object_table(void) > +{ > + unsigned int h; > + for (h = 0; h < ARRAY_SIZE(object_table); h++) { > + /* > + * We can't individually free objects here > + * because they are allocated from a pool. > + */ > + object_table[h] = NULL; > + } > + /* > + * XXX maybe free object_entry_pool here, > + * or might something still be referencing them? > + */ > +} > + > static unsigned int hc_str(const char *s, size_t len) > { > unsigned int r = 0; > @@ -1035,6 +1051,9 @@ discard_pack: > pack_data = NULL; > running = 0; > > + /* The objects are now available via git's regular lookups. */ > + clear_object_table(); > + > /* We can't carry a delta across packfiles. */ > strbuf_release(&last_blob.data); > last_blob.offset = 0; ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] t9801 and t9803 broken on next 2016-05-19 8:19 ` Lars Schneider @ 2016-05-19 17:03 ` Junio C Hamano 2016-05-19 17:32 ` Lars Schneider 0 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2016-05-19 17:03 UTC (permalink / raw) To: Lars Schneider; +Cc: Jeff King, Eric Wong, Git Users, Luke Diamand, spearce Lars Schneider <larsxschneider@gmail.com> writes: > From my point of view little packs are no problem. I run fast-import on > a dedicated migration machine. After fast-import completion I run repack [1] > before I upload the repo to its final location. How do you determine that many little packs are not a problem to you, though? Until they get repacked, they are where the fast-import will get existing objects from. The more little packs you accumulate as you go, the more slow fast-import's access to them has to become. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] t9801 and t9803 broken on next 2016-05-19 17:03 ` Junio C Hamano @ 2016-05-19 17:32 ` Lars Schneider 0 siblings, 0 replies; 19+ messages in thread From: Lars Schneider @ 2016-05-19 17:32 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Eric Wong, Git Users, Luke Diamand, spearce > On 19 May 2016, at 19:03, Junio C Hamano <gitster@pobox.com> wrote: > > Lars Schneider <larsxschneider@gmail.com> writes: > >> From my point of view little packs are no problem. I run fast-import on >> a dedicated migration machine. After fast-import completion I run repack [1] >> before I upload the repo to its final location. > > How do you determine that many little packs are not a problem to > you, though? Until they get repacked, they are where the > fast-import will get existing objects from. The more little packs > you accumulate as you go, the more slow fast-import's access to them > has to become. True. But my particular use case is a one time operation for a given repository. Therefore I don't care how long it takes. The only important aspect is that the result is correct. That being said, Peff's proposed fix looks correct to me but since I am no fast-import expert my opinion doesn't count too much. - Lars ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] t9801 and t9803 broken on next 2016-05-17 12:13 ` Jeff King 2016-05-19 8:19 ` Lars Schneider @ 2016-05-25 22:49 ` Eric Wong 2016-05-25 22:54 ` [RFC] fast-import: invalidate pack_id references after loosening Eric Wong 2016-05-25 22:56 ` [BUG] t9801 and t9803 broken on next Jeff King 1 sibling, 2 replies; 19+ messages in thread From: Eric Wong @ 2016-05-25 22:49 UTC (permalink / raw) To: Jeff King; +Cc: Lars Schneider, Junio C Hamano, git, Luke Diamand Jeff King <peff@peff.net> wrote: > On Tue, May 17, 2016 at 10:07:16AM +0200, Lars Schneider wrote: > > > I think that is pretty much the problem. Here is what is happening: > > > > 1. git-p4 imports all changelists for the "main" branch > > > > 2. git-p4 starts to import a second branch and creates a fast-import > > "progress checkpoint". This triggers: > > > > --> fast-import.c: cycle_packfile > > --> fast-import.c: end_packfile > > --> fast-import.c: loosen_small_pack > > > > At this point we have no packfile anymore. > > > > 3. git-p4 sends the command "commit refs/remotes/p4/depot/branch2" > > to fast-import in order to create a new branch. This triggers: > > > > --> fast-import.c: parse_new_commit > > --> fast-import.c: load_branch > > --> fast-import.c: load_tree > > > > "load_tree" calls "find_object" and the result has a "pack_id" of 0. > > I think this indicates that the object is in the packfile. Shouldn't > > the "pack_id" be "MAX_PACK_ID" instead? Yes; I think that is correct. Alternative patch to Jeff's coming in reply to this message. > > myoe = find_object(sha1); > > if (myoe && myoe->pack_id != MAX_PACK_ID) { > > Thanks for the analysis. I think this is definitely the problem. After > fast-import finalizes a packfile (either at the end of the program or > due to a checkpoint), it never discards its internal mapping of objects > to pack locations. It _has_ to keep such a mapping before the pack is > finalized, because git's regular object database doesn't know about it. > But afterwards, it should be able to rely on the object database. Almost; but relying on marks is a problem since that set can contain mark => object_entry mappings which the normal object database won't know about. > The patch below probably makes your case work, but there are a lot of > open questions: > > 1. Should we always discard the mapping, even when not loosening > objects? I can't think of a real downside to always using git's > object lookups. I'm not sure. It's safe to clear the top-level table, but it might speedup some lookups for just oe->type if we keep it around. I decided to keep it, anyways, because the mark set references them. > 2. Can we free memory associated with object_entry structs at this > point? They won't be accessible via the hash, but might other bits > of the code have kept pointers to them? Yes, invalid entries are also held in "struct mark_set marks"; this is a major problem with merely clearing the top-level object table. > I suspect it may screw up the statistics that fast-import prints at > the end, but that's a minor point. I still need to check, on that; but yeah, minor. > 3. I notice that a few other structs (branch and tag) hold onto the > pack_id, which will now point to a pack we can't access. Does this > matter? I don't think so, because checkpoint() seems to dump the > branches and tags. I don't think it matters unless a crash log or core dump is created; then it becomes confusing to the person tracking down a problem, so I've invalidated pack_id. This doesn't affect dump_branches or dump_tags from what I can tell. > 4. In general, should we be loosening objects at checkpoints at all? I think so. It should be useful to checkpoint to make objects available to other read-only processes while leaving a fast-import running indefinitely. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC] fast-import: invalidate pack_id references after loosening 2016-05-25 22:49 ` Eric Wong @ 2016-05-25 22:54 ` Eric Wong 2016-05-25 23:09 ` Jeff King 2016-05-25 22:56 ` [BUG] t9801 and t9803 broken on next Jeff King 1 sibling, 1 reply; 19+ messages in thread From: Eric Wong @ 2016-05-25 22:54 UTC (permalink / raw) To: Jeff King, Lars Schneider, Luke Diamand; +Cc: Junio C Hamano, git When loosening a pack, the current pack_id gets reused when checkpointing and the import does not terminate. This causes problems after checkpointing as the object table, branch, and tag lists still contains pre-checkpoint references to the recycled pack_id. Merely clearing the object_table as suggested by Jeff King in http://mid.gmane.org/20160517121330.GA7346@sigill.intra.peff.net is insufficient as the marks set still contains references to object entries. Wrong pack_id references branch and tags lists do not cause errors, but can lead to misleading crash reports and core dumps, so they are also invalidated. Signed-off-by: Eric Wong <e@80x24.org> --- I started writing a standalone test case; but testing with original failing cases would be greatly appreciated. Still learning my way around the fast-import code... Thanks. fast-import.c | 31 +++++++++++++++++++- t/t9302-fast-import-unpack-limit.sh | 57 +++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/fast-import.c b/fast-import.c index 0e8bc6a..b9db4b6 100644 --- a/fast-import.c +++ b/fast-import.c @@ -597,6 +597,33 @@ static struct object_entry *insert_object(unsigned char *sha1) return e; } +static void invalidate_pack_id(unsigned int id) +{ + unsigned int h; + unsigned long lu; + struct tag *t; + + for (h = 0; h < ARRAY_SIZE(object_table); h++) { + struct object_entry *e; + + for (e = object_table[h]; e; e = e->next) + if (e->pack_id == id) + e->pack_id = MAX_PACK_ID; + } + + for (lu = 0; lu < branch_table_sz; lu++) { + struct branch *b; + + for (b = branch_table[lu]; b; b = b->table_next_branch) + if (b->pack_id == id) + b->pack_id = MAX_PACK_ID; + } + + for (t = first_tag; t; t = t->next_tag) + if (t->pack_id == id) + t->pack_id = MAX_PACK_ID; +} + static unsigned int hc_str(const char *s, size_t len) { unsigned int r = 0; @@ -993,8 +1020,10 @@ static void end_packfile(void) cur_pack_sha1, pack_size); if (object_count <= unpack_limit) { - if (!loosen_small_pack(pack_data)) + if (!loosen_small_pack(pack_data)) { + invalidate_pack_id(pack_id); goto discard_pack; + } } close(pack_data->pack_fd); diff --git a/t/t9302-fast-import-unpack-limit.sh b/t/t9302-fast-import-unpack-limit.sh index 0f686d2..a04de14 100755 --- a/t/t9302-fast-import-unpack-limit.sh +++ b/t/t9302-fast-import-unpack-limit.sh @@ -45,4 +45,61 @@ test_expect_success 'bigger packs are preserved' ' test $(find .git/objects/pack -type f | wc -l) -eq 2 ' +test_expect_success 'lookups after checkpoint works' ' + hello_id=$(echo hello | git hash-object --stdin -t blob) && + id="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE" && + before=$(git rev-parse refs/heads/master^0) && + ( + cat <<-INPUT_END && + blob + mark :1 + data 6 + hello + + commit refs/heads/master + mark :2 + committer $id + data <<COMMIT + checkpoint after this + COMMIT + from refs/heads/master^0 + M 100644 :1 hello + + # pre-checkpoint + cat-blob :1 + cat-blob $hello_id + checkpoint + # post-checkpoint + cat-blob :1 + cat-blob $hello_id + INPUT_END + + n=0 && + from=$before && + while test x"$from" = x"$before" + do + if test $n -gt 30 + then + echo >&2 "checkpoint did not update branch" + exit 1 + else + n=$(($n + 1)) + fi && + sleep 1 && + from=$(git rev-parse refs/heads/master^0) + done && + cat <<-INPUT_END && + commit refs/heads/master + committer $id + data <<COMMIT + make sure from "unpacked sha1 reference" works, too + COMMIT + from $from + INPUT_END + echo done + ) | git -c fastimport.unpackLimit=100 fast-import --done && + test $(find .git/objects/?? -type f | wc -l) -eq 6 && + test $(find .git/objects/pack -type f | wc -l) -eq 2 +' + test_done ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [RFC] fast-import: invalidate pack_id references after loosening 2016-05-25 22:54 ` [RFC] fast-import: invalidate pack_id references after loosening Eric Wong @ 2016-05-25 23:09 ` Jeff King 2016-05-26 8:02 ` Eric Wong 0 siblings, 1 reply; 19+ messages in thread From: Jeff King @ 2016-05-25 23:09 UTC (permalink / raw) To: Eric Wong; +Cc: Lars Schneider, Luke Diamand, Junio C Hamano, git On Wed, May 25, 2016 at 10:54:02PM +0000, Eric Wong wrote: > When loosening a pack, the current pack_id gets reused when > checkpointing and the import does not terminate. This causes > problems after checkpointing as the object table, branch, and > tag lists still contains pre-checkpoint references to the > recycled pack_id. > > Merely clearing the object_table as suggested by Jeff King in > http://mid.gmane.org/20160517121330.GA7346@sigill.intra.peff.net > is insufficient as the marks set still contains references > to object entries. > > Wrong pack_id references branch and tags lists do not cause > errors, but can lead to misleading crash reports and core dumps, > so they are also invalidated. Nicely explained. > +static void invalidate_pack_id(unsigned int id) > +{ > + unsigned int h; > + unsigned long lu; > + struct tag *t; > + > + for (h = 0; h < ARRAY_SIZE(object_table); h++) { > + struct object_entry *e; > + > + for (e = object_table[h]; e; e = e->next) > + if (e->pack_id == id) > + e->pack_id = MAX_PACK_ID; > + } > + > + for (lu = 0; lu < branch_table_sz; lu++) { > + struct branch *b; > + > + for (b = branch_table[lu]; b; b = b->table_next_branch) > + if (b->pack_id == id) > + b->pack_id = MAX_PACK_ID; > + } > + > + for (t = first_tag; t; t = t->next_tag) > + if (t->pack_id == id) > + t->pack_id = MAX_PACK_ID; > +} This looks pretty straightforward. I do notice that we never shrink the number of items in the object table when checkpointing, and so our linear walk will grow ever larger. So if one were to checkpoint every k-th object, it makes the whole operation quadratic in the number of objects (actually, O(n^2/k) but k is a constant). That's probably OK in practice, as I think the actual pack-write already does a linear walk of the object table to generate the pack index. So presumably nobody checkpoints often enough for it to be a problem. And the solution would be to keep a separate list of pointers to objects for the current pack-id, which would trivially fix both this case and the one in create_index(). So we can punt on it until somebody actually runs into it, I think. -Peff ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] fast-import: invalidate pack_id references after loosening 2016-05-25 23:09 ` Jeff King @ 2016-05-26 8:02 ` Eric Wong 2016-05-26 14:16 ` Jeff King 0 siblings, 1 reply; 19+ messages in thread From: Eric Wong @ 2016-05-26 8:02 UTC (permalink / raw) To: Jeff King; +Cc: Lars Schneider, Luke Diamand, Junio C Hamano, git Jeff King <peff@peff.net> wrote: > On Wed, May 25, 2016 at 10:54:02PM +0000, Eric Wong wrote: > > + for (h = 0; h < ARRAY_SIZE(object_table); h++) { > > + struct object_entry *e; > > + > > + for (e = object_table[h]; e; e = e->next) > > + if (e->pack_id == id) > > + e->pack_id = MAX_PACK_ID; > > + } <snip> > This looks pretty straightforward. I do notice that we never shrink the > number of items in the object table when checkpointing, and so our > linear walk will grow ever larger. So if one were to checkpoint every > k-th object, it makes the whole operation quadratic in the number of > objects (actually, O(n^2/k) but k is a constant). Good point, I'll work on a separate patch to fix it. > That's probably OK in practice, as I think the actual pack-write already > does a linear walk of the object table to generate the pack index. So > presumably nobody checkpoints often enough for it to be a problem. And > the solution would be to keep a separate list of pointers to objects for > the current pack-id, which would trivially fix both this case and the > one in create_index(). So we can punt on it until somebody actually > runs into it, I think. I might checkpoint that much and run into the problem soon :) Too tired now; maybe in a day or two I'll be able to make sense of C again :x ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] fast-import: invalidate pack_id references after loosening 2016-05-26 8:02 ` Eric Wong @ 2016-05-26 14:16 ` Jeff King 2016-05-28 0:20 ` Eric Wong 0 siblings, 1 reply; 19+ messages in thread From: Jeff King @ 2016-05-26 14:16 UTC (permalink / raw) To: Eric Wong; +Cc: Lars Schneider, Luke Diamand, Junio C Hamano, git On Thu, May 26, 2016 at 08:02:36AM +0000, Eric Wong wrote: > > That's probably OK in practice, as I think the actual pack-write already > > does a linear walk of the object table to generate the pack index. So > > presumably nobody checkpoints often enough for it to be a problem. And > > the solution would be to keep a separate list of pointers to objects for > > the current pack-id, which would trivially fix both this case and the > > one in create_index(). So we can punt on it until somebody actually > > runs into it, I think. > > I might checkpoint that much and run into the problem soon :) > Too tired now; maybe in a day or two I'll be able to make sense > of C again :x In theory the list of objects in the allocation pool are contiguous for a particular pack. So you could break out of the double-loop at the first non-matching object you see: diff --git a/fast-import.c b/fast-import.c index 83558dc..f214bd3 100644 --- a/fast-import.c +++ b/fast-import.c @@ -900,10 +900,13 @@ static const char *create_index(void) c = idx; for (o = blocks; o; o = o->next_pool) for (e = o->next_free; e-- != o->entries;) if (pack_id == e->pack_id) *c++ = &e->idx; + else + goto done; +done: last = idx + object_count; if (c != last) die("internal consistency error creating the index"); tmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts, pack_data->sha1); But that seems to break some of the tests, so I think there is some case which does not match my theory. :) Another strategy is to note that we cross-check against object_count here. So you could simply break out of the loop when we have found object_count matches. We don't keep similar counters for branches/tags (which have a similar quadratic problem, though presumably much smaller "n"), but it would be easy to keep an offset in start_packfile() and just start the iteration there. -Peff ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [RFC] fast-import: invalidate pack_id references after loosening 2016-05-26 14:16 ` Jeff King @ 2016-05-28 0:20 ` Eric Wong 0 siblings, 0 replies; 19+ messages in thread From: Eric Wong @ 2016-05-28 0:20 UTC (permalink / raw) To: Jeff King, Junio C Hamano; +Cc: Lars Schneider, Luke Diamand, git Jeff King <peff@peff.net> wrote: > On Thu, May 26, 2016 at 08:02:36AM +0000, Eric Wong wrote: > > > > That's probably OK in practice, as I think the actual pack-write already > > > does a linear walk of the object table to generate the pack index. So > > > presumably nobody checkpoints often enough for it to be a problem. And > > > the solution would be to keep a separate list of pointers to objects for > > > the current pack-id, which would trivially fix both this case and the > > > one in create_index(). So we can punt on it until somebody actually > > > runs into it, I think. > > > > I might checkpoint that much and run into the problem soon :) > > Too tired now; maybe in a day or two I'll be able to make sense > > of C again :x OK, I guess I was too tired and not thinking clearly. I now figure it's not worth it to checkpoint frequently and have a very-long-lived fast-import process. The object table and marks set will grow indefinitely, so forking off another gfi with a new set of marks is better for my case[1], anyways. Junio: I think my RFC can go into pu as-is and replace Jeff's object_table discarding patch. [1] inotify watching a Maildir + fast-import > In theory the list of objects in the allocation pool are contiguous for > a particular pack. So you could break out of the double-loop at the > first non-matching object you see: > > diff --git a/fast-import.c b/fast-import.c > index 83558dc..f214bd3 100644 > --- a/fast-import.c > +++ b/fast-import.c > @@ -900,10 +900,13 @@ static const char *create_index(void) > c = idx; > for (o = blocks; o; o = o->next_pool) > for (e = o->next_free; e-- != o->entries;) > if (pack_id == e->pack_id) > *c++ = &e->idx; > + else > + goto done; > +done: > last = idx + object_count; > if (c != last) > die("internal consistency error creating the index"); > > tmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts, pack_data->sha1); > > But that seems to break some of the tests, so I think there is some case > which does not match my theory. :) Yes, it's probably because duplicate objects from the object store will create entries with e->pack_id == MAX_PACK_ID > Another strategy is to note that we cross-check against object_count > here. So you could simply break out of the loop when we have found > object_count matches. I'm worried that could fail to detect bugs which should've led us to die of the internal consistency error. What we could probably do more safely is limit the scan to only recent object pools (new ones since the previous create_index call and the last one scanned). This passes tests, but I could be missing something: --- a/fast-import.c +++ b/fast-import.c @@ -926,14 +926,16 @@ static const char *create_index(void) struct pack_idx_entry **idx, **c, **last; struct object_entry *e; struct object_entry_pool *o; + static struct object_entry_pool *last_seen; /* Build the table of object IDs. */ ALLOC_ARRAY(idx, object_count); c = idx; - for (o = blocks; o; o = o->next_pool) + for (o = blocks; o; o = (o == last_seen) ? NULL : o->next_pool) for (e = o->next_free; e-- != o->entries;) if (pack_id == e->pack_id) *c++ = &e->idx; + last_seen = blocks; last = idx + object_count; if (c != last) die("internal consistency error creating the index"); ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] t9801 and t9803 broken on next 2016-05-25 22:49 ` Eric Wong 2016-05-25 22:54 ` [RFC] fast-import: invalidate pack_id references after loosening Eric Wong @ 2016-05-25 22:56 ` Jeff King 1 sibling, 0 replies; 19+ messages in thread From: Jeff King @ 2016-05-25 22:56 UTC (permalink / raw) To: Eric Wong; +Cc: Lars Schneider, Junio C Hamano, git, Luke Diamand On Wed, May 25, 2016 at 10:49:07PM +0000, Eric Wong wrote: > > Thanks for the analysis. I think this is definitely the problem. After > > fast-import finalizes a packfile (either at the end of the program or > > due to a checkpoint), it never discards its internal mapping of objects > > to pack locations. It _has_ to keep such a mapping before the pack is > > finalized, because git's regular object database doesn't know about it. > > But afterwards, it should be able to rely on the object database. > > Almost; but relying on marks is a problem since that set can contain > mark => object_entry mappings which the normal object database won't > know about. Ah, thanks for finding that. I had a feeling there might be other users of the object_entry structs, but I didn't dig. Given that and your other responses here, I agree that just invalidating the pack_id is probably the most sensible route. -Peff ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] t9801 and t9803 broken on next 2016-05-14 18:15 ` Junio C Hamano 2016-05-17 8:07 ` Lars Schneider @ 2016-05-17 8:35 ` Luke Diamand 1 sibling, 0 replies; 19+ messages in thread From: Luke Diamand @ 2016-05-17 8:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: Lars Schneider, Eric Wong, Git Users On 14 May 2016 at 19:15, Junio C Hamano <gitster@pobox.com> wrote: > Lars Schneider <larsxschneider@gmail.com> writes: > >>> On 13 May 2016, at 18:37, Junio C Hamano <gitster@pobox.com> wrote: >>> >>> Are you saying that "git p4" itself breaks unless fast-import always >>> writes a new (tiny) packfile? That sounds quite broken, and setting >>> unpacklimit to 0 does not sound like a sensible "fix". Of course, >>> if the test script is somehow looking at the number of packs or >>> loose objects and declaring a failure, even when the resulting >>> history in p4 and git are correct, then that is a different issue, >>> and forcing to explode a tiny pack is a reasonable workaround. I >>> couldn't quite tell which the case is. >>> >>> Puzzled. Please help. >> >> t9801 "import depot, branch detection" is the first test that fails >> with a fast import error: >> https://github.com/git/git/blob/78b384c29366e199741393e56030a8384110760d/t/t9801-git-p4-branch.sh#L110 >> >> fast-import crash report: >> fast-import process: 77079 >> parent process : 77077 >> at 2016-05-14 07:48:40 +0000 >> >> fatal: offset beyond end of packfile (truncated pack?) > > Hmm, does that suggest Eric's "explode them into loose instead of > keeping a small pack" insufficient? It sounds like that somebody > wanted to read some data back from its packfile without knowing that > the updated code may make it available in a loose object form, which > would mean that somebody needs to be told about what is going on, > namely, these objects are not in a half-written pack but are found > as loose objects. > >> The problem seems to be related to the git-p4 branch import. >> I don't have time this weekend to look further into it, but >> I will try next week. The last thing that fast-import is sent before it crashes is this: checkpoint progress checkpoint commit refs/remotes/p4/depot/branch2 mark :6 committer Dr. author <author@example.com> 1463212118 +0000 data <<EOT from 1ab0d5b43b6070682f8bd6f3fd674d4fcddd9345 i.e. creating a new branch, referring to an existing object. Is it possible that fast-import can't find 1ab0d... because of the reasons given by Junio above? I don't know enough about fast-import tbh. I will see if I can get fast-import to tell me a bit more about why it's failing. Luke ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2016-05-28 0:20 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-13 10:02 [BUG] t9801 and t9803 broken on next Lars Schneider 2016-05-13 10:36 ` Eric Wong 2016-05-13 16:37 ` Junio C Hamano 2016-05-14 8:04 ` Lars Schneider 2016-05-14 18:15 ` Junio C Hamano 2016-05-17 8:07 ` Lars Schneider 2016-05-17 9:13 ` Eric Wong 2016-05-17 12:13 ` Jeff King 2016-05-19 8:19 ` Lars Schneider 2016-05-19 17:03 ` Junio C Hamano 2016-05-19 17:32 ` Lars Schneider 2016-05-25 22:49 ` Eric Wong 2016-05-25 22:54 ` [RFC] fast-import: invalidate pack_id references after loosening Eric Wong 2016-05-25 23:09 ` Jeff King 2016-05-26 8:02 ` Eric Wong 2016-05-26 14:16 ` Jeff King 2016-05-28 0:20 ` Eric Wong 2016-05-25 22:56 ` [BUG] t9801 and t9803 broken on next Jeff King 2016-05-17 8:35 ` Luke Diamand
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.