Re: Be more careful about updating refs

From

Shawn O. Pearce <spearce@spearce.org>

Date

Jan 18, 2008, 03:18 UTC

Message-ID

<20080118031831.GW24004@spearce.org>

In-Reply-To

<20080118025837.GU24004@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.

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.