Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure
On Tue, Jul 30, 2013 at 12:52 PM, Jeff King wrote: > On Tue, Jul 30, 2013 at 08:39:48AM -0700, Junio C Hamano wrote: > >> Brandon Casey writes: >> >> > From: Brandon Casey >> > >> > When the number of open packs exceeds pack_max_fds, unuse_one_window() >> > is called repeatedly to attempt to release the least-recently-used >> > pack windows, which, as a side-effect, will also close a pack file >> > after closing its last open window. If a pack file has been opened, >> > but no windows have been allocated into it, it will never be selected >> > by unuse_one_window() and hence its file descriptor will not be >> > closed. When this happens, git may exceed the number of file >> > descriptors permitted by the system. >> >> An interesting find. The patch from a cursory look reads OK. > > Yeah. I wonder if unuse_one_window() should actually leave the pack fd > open now in general. > > If you close packfile descriptors, you can run into racy situations > where somebody else is repacking and deleting packs, and they go away > while you are trying to access them. If you keep a descriptor open, > you're fine; they last to the end of the process. If you don't, then > they disappear from under you. > > For normal object access, this isn't that big a deal; we just rescan the > packs and retry. But if you are packing yourself (e.g., because you are > a pack-objects started by upload-pack for a clone or fetch), it's much > harder to recover (and we print some warnings). > > We had our core.packedGitWindowSize lowered on GitHub for a while, and > we ran into this warning on busy repositories when we were running "git > gc" on the server. We solved it by bumping the window size so we never > release memory. > > But just not closing the descriptor wouldn't work until Brandon's patch, > because we used the same function to release memory and descriptor > pressure. Now we could do them separately (and progressively if we need > to). I had thought about whether to stop closing the pack file in unuse_one_window(), but didn't have a reason to do so. I think the scenario you described provides a justification. If we're not under file descriptor pressure and we can possibly avoid rescanning the pack directory, it sounds like a net win. >> > This is not likely to occur during upload-pack since upload-pack >> > reads each object from the pack so that it can peel tags and >> > advertise the exposed object. >> >> Another interesting find. Perhaps there is a room for improvements, >> as packed-refs file knows what objects the tags peel to? I vaguely >> recall Peff was actively reducing the object access during ref >> enumeration in not so distant past... > > Yeah, we should be reading almost no objects these days due to the > packed-refs peel lines. I just did a double-check on what "git > upload-pack . /dev/null" reads on my git.git repo, and it is > only three objects: the v1.8.3.3, v1.8.3.4, and v1.8.4-rc0 tag objects. > In other words, the tags I got since the last time I ran "git gc". So I > think all is working as designed. Ok, looks like this has been the case since your 435c8332 which taught upload-pack to use peel_ref(). So looks like we do avoid reaching into the pack for any ref that was read from a (modern) packed-refs file. The repository I was testing with had mostly loose refs. Indeed, after packing refs, upload-pack encounters the same problem as receive-pack and runs out of file descriptors. So my comment about upload-pack is not completely accurate. Upload-pack _can_ run into this problem, but the refs must be packed, as well as there being enough of them that exist in enough different pack files to exceed the processes fd limit. > We could give receive-pack the same treatment; I've spent less time > micro-optimizing it because because we (and most sites, I would think) > get an order of magnitude more fetches than pushes. I don't think it would need the 435c8332 treatment since receive-pack doesn't peel refs when it advertises them to the client and hence does not need to load the ref object from the pack file during ref advertisement, but possibly some of the other stuff you did would be applicable. But like you said, the number of fetches far exceed the number of pushes. -Brandon -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure
On Tue, Jul 30, 2013 at 08:39:48AM -0700, Junio C Hamano wrote: > Brandon Casey writes: > > > From: Brandon Casey > > > > When the number of open packs exceeds pack_max_fds, unuse_one_window() > > is called repeatedly to attempt to release the least-recently-used > > pack windows, which, as a side-effect, will also close a pack file > > after closing its last open window. If a pack file has been opened, > > but no windows have been allocated into it, it will never be selected > > by unuse_one_window() and hence its file descriptor will not be > > closed. When this happens, git may exceed the number of file > > descriptors permitted by the system. > > An interesting find. The patch from a cursory look reads OK. Yeah. I wonder if unuse_one_window() should actually leave the pack fd open now in general. If you close packfile descriptors, you can run into racy situations where somebody else is repacking and deleting packs, and they go away while you are trying to access them. If you keep a descriptor open, you're fine; they last to the end of the process. If you don't, then they disappear from under you. For normal object access, this isn't that big a deal; we just rescan the packs and retry. But if you are packing yourself (e.g., because you are a pack-objects started by upload-pack for a clone or fetch), it's much harder to recover (and we print some warnings). We had our core.packedGitWindowSize lowered on GitHub for a while, and we ran into this warning on busy repositories when we were running "git gc" on the server. We solved it by bumping the window size so we never release memory. But just not closing the descriptor wouldn't work until Brandon's patch, because we used the same function to release memory and descriptor pressure. Now we could do them separately (and progressively if we need to). > > This is not likely to occur during upload-pack since upload-pack > > reads each object from the pack so that it can peel tags and > > advertise the exposed object. > > Another interesting find. Perhaps there is a room for improvements, > as packed-refs file knows what objects the tags peel to? I vaguely > recall Peff was actively reducing the object access during ref > enumeration in not so distant past... Yeah, we should be reading almost no objects these days due to the packed-refs peel lines. I just did a double-check on what "git upload-pack . /dev/null" reads on my git.git repo, and it is only three objects: the v1.8.3.3, v1.8.3.4, and v1.8.4-rc0 tag objects. In other words, the tags I got since the last time I ran "git gc". So I think all is working as designed. We could give receive-pack the same treatment; I've spent less time micro-optimizing it because because we (and most sites, I would think) get an order of magnitude more fetches than pushes. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure
Brandon Casey writes: > From: Brandon Casey > > When the number of open packs exceeds pack_max_fds, unuse_one_window() > is called repeatedly to attempt to release the least-recently-used > pack windows, which, as a side-effect, will also close a pack file > after closing its last open window. If a pack file has been opened, > but no windows have been allocated into it, it will never be selected > by unuse_one_window() and hence its file descriptor will not be > closed. When this happens, git may exceed the number of file > descriptors permitted by the system. An interesting find. The patch from a cursory look reads OK. Thanks. > This is not likely to occur during upload-pack since upload-pack > reads each object from the pack so that it can peel tags and > advertise the exposed object. Another interesting find. Perhaps there is a room for improvements, as packed-refs file knows what objects the tags peel to? I vaguely recall Peff was actively reducing the object access during ref enumeration in not so distant past... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure
On Tue, Jul 30, 2013 at 12:05 AM, Brandon Casey wrote: > When the number of open packs exceeds pack_max_fds, unuse_one_window() > is called repeatedly to attempt to release the least-recently-used > pack windows, which, as a side-effect, will also close a pack file > after closing its last open window. If a pack file has been opened, > but no windows have been allocated into it, it will never be selected > by unuse_one_window() and hence its file descriptor will not be > closed. When this happens, git may exceed the number of file > descriptors permitted by the system. > > This latter situation can occur in show-ref or receive-pack during ref > advertisement. During ref advertisement, receive-pack will iterate > over every ref in the repository and advertise it to the client after > ensuring that the ref exists in the local repository. If the ref is > located inside a pack, then the pack is opened to ensure that it > exists, but since the object is not actually read from the pack, no > mmap windows are allocated. When the number of open packs exceeds > pack_max_fds, unuse_one_window() will not able to find any windows to s/not able/not be able/ ...or... s/not able to find/not find/ > free and will not be able to close any packs. Once the per-process > file descriptor limit is exceeded, receive-pack will produce a warning, > not an error, for each pack it cannot open, and will then most likely > fail with an error to spawn rev-list or index-pack like: > >error: cannot create standard input pipe for rev-list: Too many open files >error: Could not run 'git rev-list' > > This is not likely to occur during upload-pack since upload-pack > reads each object from the pack so that it can peel tags and > advertise the exposed object. So during upload-pack, mmap windows > will be allocated for each pack that is opened and unuse_one_window() > will eventually be able to close unused packs after freeing all of > their windows. > > When we have file descriptor pressure, in contrast to memory pressure, > we need to free all windows and close the pack file descriptor so that > a new pack can be opened. Let's introduce a new function > close_one_pack() designed specifically for this purpose to search > for and close the least-recently-used pack, where LRU is defined as > >* pack with oldest mtime and no allocated mmap windows or >* pack with the least-recently-used windows, i.e. the pack > with the oldest most-recently-used window > > Signed-off-by: Brandon Casey -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
