Re: Be more careful about updating refs

"Shawn O. Pearce" [off-list ref] writes:

I think the problem is nobody has tested fast-import updating an
existing ref while using NO_MMAP.  Or if they did, they didn't report
the problem as they didn't figure they needed fast-import that badly.

Updating an existing ref is not a common operation, but the test
suite does test for it.  So it must be the NO_MMAP configuration
is simply not being tested well enough.

Ok, thanks.

Now a more important question is how we would properly fix this
issue?

I suspect that fast-import is the only one that opens windows
into an unfinalized pack, and if that is the case, it would be
the only program that may be hit by the issue of mmap emulation
getting stale data.

I do not think the patch I posted was correct at all.

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?

Junio C Hamano [off-list ref] wrote:

"Shawn O. Pearce" [off-list ref] writes:

quoted

I think the problem is nobody has tested fast-import updating an
existing ref while using NO_MMAP.  Or if they did, they didn't report
the problem as they didn't figure they needed fast-import that badly.

Updating an existing ref is not a common operation, but the test
suite does test for it.  So it must be the NO_MMAP configuration
is simply not being tested well enough.

Now a more important question is how we would properly fix this
issue?

I suspect that fast-import is the only one that opens windows
into an unfinalized pack, and if that is the case, it would be
the only program that may be hit by the issue of mmap emulation
getting stale data.

Yes, it is the only program that is foolish enough to access a
partial packfile.  index-pack uses pread(), and only after it has
the entire packfile downloaded to the local system.  The only other
reader of a partial packfile is unpack-objects, and obviously that
doesn't care about seeking backwards within the packfile.

So its probably the only user who suffers from the mmap emulation.
 

I do not think the patch I posted was correct at all.

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'll try to work up a patch this evening.

-- 
Shawn.

"Shawn O. Pearce" [off-list ref] wrote:

Junio C Hamano [off-list ref] wrote:

quoted

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.

"Shawn O. Pearce" [off-list ref] wrote:

"Shawn O. Pearce" [off-list ref] wrote:

quoted

Junio C Hamano [off-list ref] wrote:

quoted

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.

"Shawn O. Pearce" [off-list ref] wrote:

"Shawn O. Pearce" [off-list ref] wrote:

quoted

"Shawn O. Pearce" [off-list ref] wrote:

quoted

Junio C Hamano [off-list ref] wrote:

quoted

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...

OK, that was crap.  Don't even try it.

I'm holding off sending anything more until I get the test suite
to actually run without telling me the bus crashed into the wall.

-- 
Shawn.