Re: Be more careful about updating refs

["Shawn O. Pearce" <spearce@spearce.org>]

"Shawn O. Pearce" <spearce@spearce.org> wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
> > 
> > Especially, I am not sure if the issue only exists at the
> > end_packfile() boundary.  Don't we have the same issue reading
> > from the packfile being built, and isn't the only reason my hack
> > works it around is because access patterns of the testsuite
> > happens to not trigger it?
> 
> Yes, that's my take on it as well (see my other email).  The
> testsuite must just be really lucky that its not hitting the
> boundary condition.
> 
> I almost said gfi_unpack_entry() was immune from this bug, but
> I went back and read the code again and determined that it does
> in fact suffer from this under NO_MMAP, and we're just really
> damn lucky nobody has caused it.

I think this solves the problem.  Its based on your first patch, but
would replace it.  The trick here is we close the cached windows if
we are accessing data from the packfile we are appending into and we
have increased the file length.  This way we don't blow away windows
during high read/low write periods, like during branch cache reloads.

--8>--
diff --git a/fast-import.c b/fast-import.c
index 3609c24..8e7747c 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -907,6 +907,16 @@ static void unkeep_all_packs(void)
 	}
 }
 
+static void close_all_windows(struct packed_git *p)
+{
+	while (p->windows) {
+		struct pack_window *w = p->windows;
+		munmap(w->base, w->len);
+		p->windows = w->next;
+		free(w);
+	}
+}
+
 static void end_packfile(void)
 {
 	struct packed_git *old_p = pack_data, *new_p;
@@ -917,6 +927,7 @@ static void end_packfile(void)
 		struct branch *b;
 		struct tag *t;
 
+		close_all_windows(pack_data);
 		fixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,
 				    pack_data->pack_name, object_count);
 		close(pack_data->pack_fd);
@@ -926,7 +937,6 @@ static void end_packfile(void)
 		new_p = add_packed_git(idx_name, strlen(idx_name), 1);
 		if (!new_p)
 			die("core git rejected index %s", idx_name);
-		new_p->windows = old_p->windows;
 		all_packs[pack_id] = new_p;
 		install_packed_git(new_p);
 
@@ -1129,8 +1139,10 @@ static void *gfi_unpack_entry(
 {
 	enum object_type type;
 	struct packed_git *p = all_packs[oe->pack_id];
-	if (p == pack_data)
+	if (p == pack_data && p->pack_size < (pack_size + 20)) {
+		close_all_windows(p);
 		p->pack_size = pack_size + 20;
+	}
 	return unpack_entry(p, oe->offset, &type, sizep);
 }
 

-- 
Shawn.