Re: Be more careful about updating refs

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

"Shawn O. Pearce" <spearce@spearce.org> wrote:
> "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.

Junio pointed out my first attempt at this didn't update the
memory pressure values, so we could "run out of memory" even
if we had plenty free.

Try #2...
 
--8>--
diff --git a/cache.h b/cache.h
index 24735bd..15388aa 100644
--- a/cache.h
+++ b/cache.h
@@ -561,6 +561,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
 extern void pack_report(void);
 extern int open_pack_index(struct packed_git *);
 extern unsigned char* use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);
+extern void close_pack_windows(struct packed_git *, int);
 extern void unuse_pack(struct pack_window **);
 extern struct packed_git *add_packed_git(const char *, int, int);
 extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t);
diff --git a/fast-import.c b/fast-import.c
index 3609c24..82c82ce 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -917,6 +917,7 @@ static void end_packfile(void)
 		struct branch *b;
 		struct tag *t;
 
+		close_pack_windows(pack_data, 0);
 		fixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,
 				    pack_data->pack_name, object_count);
 		close(pack_data->pack_fd);
@@ -926,7 +927,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 +1129,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_pack_windows(p, 0);
 		p->pack_size = pack_size + 20;
+	}
 	return unpack_entry(p, oe->offset, &type, sizep);
 }
 
diff --git a/sha1_file.c b/sha1_file.c
index 6583797..50d1dea 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -611,6 +611,34 @@ void release_pack_memory(size_t need, int fd)
 		; /* nothing */
 }
 
+void close_pack_windows(struct packed_git *p, int retain_fd)
+{
+	struct pack_window **tail = NULL, *n = p->windows;
+	while (n) {
+		struct pack_window *w = p->windows;
+
+		if (w->inuse_cnt) {
+			*tail = w;
+			tail = &w->next;
+			continue;
+		}
+
+		munmap(w->base, w->len);
+		pack_mapped -= w->len;
+		pack_open_windows--;
+		n = w->next;
+		free(w);
+	}
+
+	p->windows = *tail;
+	if (p->windows)
+		warning("pack windows still in-use during close attempt");
+	else if (!retain_fd && p->pack_fd != -1) {
+		close(p->pack_fd);
+		p->pack_fd = -1;
+	}
+}
+
 void unuse_pack(struct pack_window **w_cursor)
 {
 	struct pack_window *w = *w_cursor;

-- 
Shawn.