patchRe: [PATCH] Fix random fast-import errors when compiled with NO_MMAP

On Thu, 17 Jan 2008, Shawn O. Pearce wrote:

In fact, even with mmap(), it's not guaranteed. There are really crappy mmap implementations out there, partly due to bad CPU design (virtual CPU caches without coherency), but more often due to total crap OS.

(Yeah, Linux did count in that area at some point. Long ago. Early 90's. Maybe)

I think HP-UX used to have non-coherent mmap for the longest time, due to carrying around some totally crap memory management based on some ancient BSD version that everybody else (including the BSD's) had long since jettisoned.

That said, I suspect any unix you can run today (without calling it a retro setup) probably has coherent-enough mmap. The possible virtual cache coherency issue is unlikely to be able to trigger this (and not relevant on any sane hardware anyway).

				Linus

On Thu, Jan 17, 2008 at 08:27:08PM -0800, Linus Torvalds wrote:

I've just checked the Mac OS X build and it looks like there is a mmap and git is indeed using it, so this is obviously an example of a "really crappy" mmap implementation.

This adds more ammunition to the fight against the whole Mac OS X is powered/built/based on UNIX myth.

Charles.

On Fri, 18 Jan 2008, Charles Bailey wrote:

Looking closer, this is not necessarily the case here.

Git uses MAP_PRIVATE, because that whole pack-file mapping was really *meant* to map an existing read-only pack-file, and fast-import seems to really be screwing with it.

It so happens that Linux has a particularly clean and streamlined VM, and if you do only reads to a MAP_PRIVATE mapping on a normal filesystem, you'll always be as coherent as with MAP_SHARED because Linux will simply map in the page cache pages directly.

But this is definitely not portable, and the git fast-import mmap window usage before Shawn's patch it was simply wrong.

So in this case, it really was git that was crap.

It just happened to work because the Linux mmap handling is just generally pretty sane. It probably also worked fine on pretty much any other modern UNIX (ie Solaris).

I'm not quite sure what OS X does to MAP_PRIVATE mappings, but if OS X is still based on Mach (with FreeBSD just as a single-server on top), I suspect that may be why it broke on OS X. The Mach VM is insanely complex and does really odd things.

But the fact is, without MAP_SHARED, you shouldn't expect things to be coherent, even if they often will be (especially for PROT_READ).

Btw, even with Shawn's patch, I wonder if the index_data usage is correct.

			Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

Hmph.

gfi uses data in a "pack" in two quite different ways.

A new object is written to an unfinalized pack. Such a pack already has "struct packed_git" allocated for it and a pointer to it is held in pack_data. As far as the core part of git (that is, sha1_file.c) is concerned, however, this pack does not even exist. It is still not part of packed_git list in sha1_file.c, and read_sha1_file() will not see objects in it, as no idx into the packfile exists yet. gfi has a table of objects in this pack and uses gfi_unpack_entry() function to retrieve data from it.

A packfile is finalized in end_packfile(). The pack header and footer is recomputed, an idx file is written, and the pack is finally registered. Before that time p->index_data is not even used for that pack (it is initialized with NULL).

So I do not think "index_data usage" can be incorrect, as there won't be any index_data usage with unfinalized pack, and the core part of git would not even have any mmap(2) (nor open fd) into its idx file before it is finalized.

By the way, I was quite puzzled how the gfi_unpack_entry() function manages to work correctly when it has to read an object it deltified based on another object it wrote into the same unfinalized pack earlier. It knows where in the unfinalized pack it wrote the object, so it can find from its own "struct object_entry" the offset for the object, and calls unpack_entry() defined in the core to do the rest.

However, most of the core does not really know about the other objects in this half-built pack. If the object is a delta, unpack_delta_entry() needs to find the delta base. And it needs to do that without having the idx.

The trick (the code really needs a bit more documentation) is that gfi never writes anything but OFS_DELTA. So the core, even though it does not have the corresponding idx file, does not have to look up the object (in fact it does not even know what object to look up for the base, it only knows the offset).

On Fri, 18 Jan 2008, Junio C Hamano wrote:

Oh, ok. I did try to grep for index_data, and didn't find anything that looked bad, but the incestuous things that fast-import.c does just made me worry - but I was too lazy to really follow it all. It's one of those files I don't think I've ever had anything what-so-ever to do with.

In that case, I think Shawn fixed it all, and we're all good, and it's not just hidden well enough that it "just happens" to work.

				Linus

Junio C Hamano <gitster@pobox.com> wrote:

...

Correct.
 

Yup.

Older fast-imports (pre OFS_DELTA) had to replicate a good chunk of the unpack_entry() logic directly inside of fast-import. But it later occurred to me that OFS_DELTA simplifies the code and lets me reuse more of the existing sha1_file.c implementation.

So yea, fast-import only emits OFS_DELTA, and does so only so that it can pull this delta-base-unpack trick on the core, without actually giving the core the corresponding idx file first.

"Shawn O. Pearce" <spearce@spearce.org> writes:

Nobody seems to pass anything but true in retain_fd parameter. Is it worth it?

I am not sure about this inuse_cnt business.

The only caller we know that needs this function is fast-import that wants to drop all windows into a pack while keeping the file descriptor and it should not have an in-use windows.

It is unclear what semantics is the right one for callers that do want to retain some windows but still want to call this function. If somebody is in the middle of chasing a delta chain and still calls this function, *why* is the call being made, and what is the right behaviour if not all the windows can be closed because of these open windows?

What about the case the value passed in ratain_fd is 0, which presumably means the caller is asking us to close the file descriptor? If we close the packfile, later accesses through the unclosed windows will obviously barf and I understand that is why you are ignoring retain_fd in that case, but maybe for the caller it was of higher priority that the packfile to get closed than the remaining windows to be still usable. Or maybe the caller wants to be notified of such an error, in which case it probably is not enough to just call warning().

IOW, I think the patch is trying to be too flexible without having a clear definition of what that flexibility is trying to achieve.

Maybe we would need more flexible version later, but I am not convinced if the semantics the above patch implements is the right one.

So I'd prefer something much simpler like this one instead...

void close_pack_windows(struct packed_git *p)
{
	while (p->windows) {
		struct pack_window *w = p->windows;

		if (w->inuse_cnt)
			die("pack '%s' still has outstanding windows",
				p->pack_name)
		munmap(w->base, w->len);
		pack_mapped -= w->len;
		pack_open_windows--;
		p->windows = w->next;
		free(w);
	}
}

Junio C Hamano <gitster@pobox.com> wrote:

Hmm, well, I originally wrote something like what you fixed it to be, then thought that at some point in the future someone may try to invoke the method and expect different semantics than it implemented. :-)

In short I made the implementation I gave you way too complex.
 

Me too. Thank you for the sanity check, and for fixing the code to actually be somewhat sane.

Shawn O. Pearce schrieb:

Thanks a lot. It works here on Windows, too.

-- Hannes

I can ACK this as fixing the test suite failures on Mac OS X 10.4.11 both with and without NO_MMAP=Yes on top of 1.5.4.rc3.24.gb53139 and also on my oldish Fedora install with NO_MMAP=Yes.

Acked-by: Charles Bailey <charles@hashpling.org>

Charles Bailey <charles@hashpling.org> writes:

Well, I do not want to be picky with these things, but you do not own that area, so that would be "Tested-by:"

I've already committed the simplified version I suggested to Shawn.

On Fri, Jan 18, 2008 at 01:36:55AM -0800, Junio C Hamano wrote:

Sorry, I thought that Acked-by was an acceptable tag for people not involved in the development path of a patch. I don't want to add a "Tested-by" as I am not a fast-import user and I am *only* reporting the success of the test suite, not any real-world testing.

Charles.

Charles Bailey <charles@hashpling.org> writes:

It still counts as "Tested-by:". Earlier you saw breakage and now you see it fixed in your environment, to which neither I nor Shawn have access to. That's a good verification that the patch fixed the issue for you and your feedback was very much appreciated (your timely initial breakage report even more so).

Acked-by: is usually given by people who are the most familiar
with the code being affected (or at least by the ones more
familiar than the patch's author), in order to let me and the
world know that he thinks the contents of the patch makes sense.
IOW, the patch was checked by somebody who knows the existing
code well enough to catch any subtleties in the existing code
that might have been broken if the patch's author was not
careful.  It just felt slightly funny that anybody is giving an
Acked-by: to a patch Shawn made to fast-import.c, which is his
brainchild ;-).

Signed-off-by is about the kosherness from software license point of view, while Acked-by and Tested-by are about the correctness.