Re: git-diff opens too many files?

I got this error in a quite big (in files) repository: error: open("vnexpress.net/Suc-khoe/2001/04/3B9AF976"): Too many open files in system fatal: cannot hash vnexpress.net/Suc-khoe/2001/04/3B9AF976

The repository contained about 67.000 files and probably all were modified. git version 1.4.4.rc1.g2bba

Hi,

On Mon, 20 Nov 2006, Nguyen Thai Ngoc Duy wrote:

What was the command line? "git diff" really is a wrapper around different programs...

Ciao, Dscho

On 11/20/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

"git diff" from working directory with no argument. The real command is

git diff |LANG=C grep '^-'|LANG=C grep -v '^--'|LANG=C sort|LANG=C uniq -c

(I squeezed blank lines, so I wanted to check that it just did that, nothing else)

Hi,

On Mon, 20 Nov 2006, Nguyen Thai Ngoc Duy wrote:

I looked in the code, but nothing jumps into my eyes. There is a place where files are mmap()ed, but AFAICT they are unmmap()ed just after diffing. Could you run it with strace and find unclosed open calls?

Ciao, Dscho

On Mon, 20 Nov 2006, Nguyen Thai Ngoc Duy wrote:

Ok, "too many open files in system" is ENFILE - you haven't run out of file descriptors in _one_ process, but you've exceeded the total number of file descriptors in the whole system.

Which is not because we forget to close() something, but because we're keeping file descriptors busy another way.

Hmm. We keep files mmap'ed in "git diff" for possibly too long. What happens is that we mmap a file that we want to diff when we start the whole thing, and keep it mapped over the whole diff session, because we're potentially going to need to compare it against other files (ie rename detection etc). And then we unmap it only at the end (in "diff_flush()" -> "diff_free_filepair()" -> "diff_free_filespec_data()").

And that's normally great, and means that we don't need to worry about the file data (we map it once, and can keep it in memory), but yeah, if you have thousands of files changed, you'll have thousands of mappings. And each one will have a pointer to a "struct file" inside the kernel.

What OS/distro is this? Normally, you shouldn't have that low a limit on number of files open, but we do end up potentially opening thousands.

For example, under Linux, you can do this:

	# in one terminal window, do:
	while : ; do cat /proc/sys/fs/file-nr ; sleep 1; done

	# in another one:
	cd linux-repo
	git ls-files '*.c' | xargs touch
	git diff

and if it looks anything like mine, it could be:

	2464    0       349662
	2464    0       349662
	2464    0       349662
*	5920    0       349662
**	7616    0       349662
***	9024    0       349662
****	10944   0       349662
	2464    0       349662
	2464    0       349662

(see how the numnber of active files grows by thousands).

Anyway, there's two possible solutions:

 - simply make sure that you can have that many open files. 

   If it's a Linux system, just increase the value of the file
   /proc/sys/fs/file-max, and you're done. Of course, if you're not the 
   admin of the box, you may need to ask somebody else to do it for you..

 - we could try to make git not keep them mmap'ed for the whole time. 

Junio? This is your speciality, I'm not sure how painful it would be to unmap and remap on demand.. (or switch it to some kind of "keep the last <n> mmaps active" kind of thing to avoid having thousands and thousands of mmaps active).

One simple thing that might be worth it is to simply _not_ use mmap() at all for small files. If a file is less than 1kB, it might be better to do a malloc() and a read() - partly because it avoids having tons of file descriptors, but partly because it's also more efficient from a virtual memory usage perspective (not that you're probably very likely to ever really hit that problem in practice).

Nguyen - that "use malloc+read" thing might be a quick workaround, but only if you have tons of _small_ files (and if you can't easily just increase file-max).

Linus Torvalds <torvalds@osdl.org> writes:

60,000 files 1kB each is 60MB which is a peanuts these days, but 10kB each would be already nontrivial burden on 32-bit (20% under 3+1 split), so even if we do the "read in small files instead of mapping" we would need diff_unpopulate_filespec() calls.

I think after diffcore_rename() runs, the data in filespec is used only once during final textual diff generation. We would use once more before diff generation if diffcore_pickaxe() is in use. These codepaths begin with diff_populate_filespec(), so if we unpopulate them after diffcore_rename() runs nobody would notice (other than performance degradation and strace showing us reading the same thing twice).

The diffcore_rename() matrix code expects all filespecs involved can be populated at the same time, but it should not be too hard to change it to keep one dst and all src candidates populated but others dropped if space gets tight. I need to look at the code for the details.

But Nguyen's command line did not have -M; I think the filespecs are populated only during the text generation in that case, so the above would not help him while it would be a worthwhile change.

Because there is _no_ processing that comes after textual diff generation that looks at the data, I think diff_flush_patch() after calling run_diff() can unpopulate the data from the filepair *p before returning without harming performance.

I think diff_flush_stat() and diff_flush_checkdiff() would have the same issue, though. Ideally these should be able to do their processing while the main textual diff holds the data in memory for its processing but that is currently not the way the code is structured.

Linus Torvalds <torvalds@osdl.org> writes:

So here is a lunch-time hack to get Nguyen unstuck.

-- >8 -- [PATCH] diff.c: avoid mmap() of small files in populate_filespec()

This would hopefully behave better in VM usage. It is not a real fix if you are dealing with truly huge diff, in which case we would have to LRU out the data in memory for filespecs that are not used in immediate future.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---