# Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP 13 messages from 2008-01-18 to 2008-01-21. Participants: Linus Torvalds, Junio C Hamano, Johannes Sixt, Charles Bailey, Shawn O. Pearce. Thread: https://gitlist.dev/t/11663 ## Linus Torvalds, 2008-01-18 04:27 Subject: Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP Message-ID: URL: https://gitlist.dev/e/alpine.LFD.1.00.0801172013270.2957%40woody.linux-foundation.org In-Reply-To: <20080118035700.GA3458@spearce.org> ``` On Thu, 17 Jan 2008, Shawn O. Pearce wrote: > > fast-import was relying on the fact that on most systems mmap() and > write() are synchronized by the filesystem's buffer cache. We were > relying on the ability to mmap() 20 bytes beyond the current end > of the file, then later fill in those bytes with a future write() > call, then read them through the previously obtained mmap() address. > > This isn't always true with some implementations of NFS, but it is > especially not true with our NO_MMAP=YesPlease build time option used > on some platforms. 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 ``` ## Junio C Hamano, 2008-01-18 06:10 Subject: Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP Message-ID: <7vtzlbirz0.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtzlbirz0.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080118035700.GA3458@spearce.org> ``` "Shawn O. Pearce" writes: > +extern void close_pack_windows(struct packed_git *, int); Nobody seems to pass anything but true in retain_fd parameter. Is it worth it? > +void close_pack_windows(struct packed_git *p, int retain_fd) > +{ > + struct pack_window *tail_var = NULL, *n = p->windows; > + struct pack_window **tail = &tail_var; > + 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_var; > + 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; > + } > +} 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); } } ``` ## Johannes Sixt, 2008-01-18 07:53 Subject: Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP Message-ID: <47905B08.4040302@viscovery.net> URL: https://gitlist.dev/e/47905B08.4040302%40viscovery.net In-Reply-To: <20080118035700.GA3458@spearce.org> ``` Shawn O. Pearce schrieb: > This should do the trick. I SOB'd the patch as I did test it with > NO_MMAP and I think its valid. :-) Thanks a lot. It works here on Windows, too. -- Hannes ``` ## Charles Bailey, 2008-01-18 08:42 Subject: Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP Message-ID: <20080118084201.GA14763@hashpling.org> URL: https://gitlist.dev/e/20080118084201.GA14763%40hashpling.org In-Reply-To: ``` On Thu, Jan 17, 2008 at 08:27:08PM -0800, Linus Torvalds wrote: > > > On Thu, 17 Jan 2008, Shawn O. Pearce wrote: > > > > fast-import was relying on the fact that on most systems mmap() and > > write() are synchronized by the filesystem's buffer cache. We were > > relying on the ability to mmap() 20 bytes beyond the current end > > of the file, then later fill in those bytes with a future write() > > call, then read them through the previously obtained mmap() address. > > > > This isn't always true with some implementations of NFS, but it is > > especially not true with our NO_MMAP=YesPlease build time option used > > on some platforms. > > 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 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. ``` ## Charles Bailey, 2008-01-18 09:26 Subject: Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP Message-ID: <20080118092652.GA22052@hashpling.org> URL: https://gitlist.dev/e/20080118092652.GA22052%40hashpling.org In-Reply-To: <20080118035700.GA3458@spearce.org> ``` 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 ``` ## Junio C Hamano, 2008-01-18 09:36 Subject: Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP Message-ID: <7v8x2nh3uw.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8x2nh3uw.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080118092652.GA22052@hashpling.org> ``` Charles Bailey writes: > 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 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. ``` ## Charles Bailey, 2008-01-18 09:45 Subject: Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP Message-ID: <20080118094553.GB22052@hashpling.org> URL: https://gitlist.dev/e/20080118094553.GB22052%40hashpling.org In-Reply-To: <7v8x2nh3uw.fsf@gitster.siamese.dyndns.org> ``` On Fri, Jan 18, 2008 at 01:36:55AM -0800, Junio C Hamano wrote: > > 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. > 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. ``` ## Junio C Hamano, 2008-01-18 10:57 Subject: Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP Message-ID: <7v8x2nflkj.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8x2nflkj.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080118094553.GB22052@hashpling.org> ``` Charles Bailey writes: > 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. 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. ``` ## Linus Torvalds, 2008-01-18 17:08 Subject: Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP Message-ID: URL: https://gitlist.dev/e/alpine.LFD.1.00.0801180842350.2957%40woody.linux-foundation.org In-Reply-To: <20080118084201.GA14763@hashpling.org> ``` On Fri, 18 Jan 2008, Charles Bailey 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. 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 ``` ## Junio C Hamano, 2008-01-19 03:25 Subject: Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP Message-ID: <7vwsq6a44w.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwsq6a44w.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Linus Torvalds writes: > Btw, even with Shawn's patch, I wonder if the index_data usage is correct. 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). ``` ## Linus Torvalds, 2008-01-19 03:55 Subject: Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP Message-ID: URL: https://gitlist.dev/e/alpine.LFD.1.00.0801181953130.2957%40woody.linux-foundation.org In-Reply-To: <7vwsq6a44w.fsf@gitster.siamese.dyndns.org> ``` On Fri, 18 Jan 2008, Junio C Hamano wrote: > > 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). 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. > 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. 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 ``` ## Shawn O. Pearce, 2008-01-21 03:57 Subject: Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP Message-ID: <20080121035755.GC24004@spearce.org> URL: https://gitlist.dev/e/20080121035755.GC24004%40spearce.org In-Reply-To: <7vwsq6a44w.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > Linus Torvalds writes: > > > Btw, even with Shawn's patch, I wonder if the index_data usage is correct. ... > 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. Correct. > 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). 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. ``` ## Shawn O. Pearce, 2008-01-21 04:10 Subject: Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP Message-ID: <20080121041031.GF24004@spearce.org> URL: https://gitlist.dev/e/20080121041031.GF24004%40spearce.org In-Reply-To: <7vtzlbirz0.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > "Shawn O. Pearce" writes: > > > +extern void close_pack_windows(struct packed_git *, int); > > Nobody seems to pass anything but true in retain_fd parameter. > Is it worth it? 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. > 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); > } > } Me too. Thank you for the sanity check, and for fixing the code to actually be somewhat sane. -- Shawn. ```