Funny error with git gc...

> if you need a chuckle, like me, you might appreciate this story: in one of 
> my repositories, "git gc" dies with
>
> 	unable to open object pack directory: ...: Too many open files
>
> turns out that there are a whopping 1088 packs in that repository...

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> if you need a chuckle, like me, you might appreciate this story: in one of
>> my repositories, "git gc" dies with
>>
>>       unable to open object pack directory: ...: Too many open files
>>
>> turns out that there are a whopping 1088 packs in that repository...
>
> Isn't it a more serious problem than a mere chuckle?  How would one
> recover from such a situation (other than "mv .git/objects/pack-*;
> for p in pack-*.pack; do git unpack-objects <$p; done")?

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > if you need a chuckle, like me, you might appreciate this story: in one of 
> > my repositories, "git gc" dies with
> >
> > 	unable to open object pack directory: ...: Too many open files
> >
> > turns out that there are a whopping 1088 packs in that repository...
> 
> Isn't it a more serious problem than a mere chuckle?  How would one
> recover from such a situation (other than "mv .git/objects/pack-*;
> for p in pack-*.pack; do git unpack-objects <$p; done")?

> On Fri, 15 May 2009, Junio C Hamano wrote:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > > if you need a chuckle, like me, you might appreciate this story: in 
> > > one of my repositories, "git gc" dies with
> > >
> > > 	unable to open object pack directory: ...: Too many open files
> > >
> > > turns out that there are a whopping 1088 packs in that repository...
> > 
> > Isn't it a more serious problem than a mere chuckle?  How would one 
> > recover from such a situation (other than "mv .git/objects/pack-*; for 
> > p in pack-*.pack; do git unpack-objects <$p; done")?
> 
> Well, you can probably just increase the file limits and try again. 
> Depending on setup, you may need root to do so, though.
> 
> I also think you _should_ be able to avoid this by just limiting the 
> pack size usage. IOW, with some packed_git_limit, something like
> 
> 	[core]
> 		packedGitWindowSize = 16k
> 		packedGitLimit = 1M
> 
> you should hopefully be able to repack (slowly) even with a low file 
> descriptor limit, because of the total limit on the size.

> That said, I do agree that ulimit doesn't always work on all systems 
> (whether due to hard system limits or due to not having permission to 
> raise the limits), and playing games with pack limits is non-obvious. We 
> should really try to avoid getting into such a situation. But I think git 
> by default avoids it by the auto-gc, no? So you have to disable that 
> explicitly to get into this bad situation.

> One solution - which I think may be the right one regardless - is to not 
> use "mmap()" for small packs or small SHA1 files.
> 
> mmap is great for random-access multi-use scenarios (and to avoid some 
> memory pressure by allowing sharing of pages), but for anything that is 
> just a couple of pages in size, mmap() just adds big overhead with 
> little upside.
> 
> So if we use malloc+read for small things, we'd probably avoid this. Now, 
> if you have a few thousand _large_ packs, you'd still be screwed, but the 
> most likely reason for having a thousand packfiles is that you did daily 
> "git pull"s, and have lots and lots of packs that are pretty small.
> 
> Dscho? What are your pack-file statistics in this case?

> > 	[core]
> > 		packedGitWindowSize = 16k
> > 		packedGitLimit = 1M
> > 
> > you should hopefully be able to repack (slowly) even with a low file 
> > descriptor limit, because of the total limit on the size.
> 
> I don't think so, because the window size has nothing to do with the 
> amount of open windows, right?

> On Fri, 15 May 2009, Johannes Schindelin wrote:
> 
> 
> > > 	[core]
> > > 		packedGitWindowSize = 16k
> > > 		packedGitLimit = 1M
> > > 
> > > you should hopefully be able to repack (slowly) even with a low file 
> > > descriptor limit, because of the total limit on the size.
> > 
> > I don't think so, because the window size has nothing to do with the 
> > amount of open windows, right?
> 
> Obviously I overlooked the "packedGitLimit" setting.  Still, just a 
> work-around, not a solution that tackles the problem heads-on.

>  
>  	fd = mkstemp(template);
> +	if (fd < 0) {
> +		release_pack_memory((size_t)-1ll, -1);
> +		fd = mkstemp(template);

> On Fri, 15 May 2009, Johannes Schindelin wrote:
> >  
> >  	fd = mkstemp(template);
> > +	if (fd < 0) {
> > +		release_pack_memory((size_t)-1ll, -1);
> > +		fd = mkstemp(template);
> 
> This is wrong. You can't use "template" twice. You need to re-initialize 
> it. The first one will blow away the XXXX pattern.

> On Fri, 15 May 2009, Linus Torvalds wrote:
> 
> > On Fri, 15 May 2009, Johannes Schindelin wrote:
> > >  
> > >  	fd = mkstemp(template);
> > > +	if (fd < 0) {
> > > +		release_pack_memory((size_t)-1ll, -1);
> > > +		fd = mkstemp(template);
> > 
> > This is wrong. You can't use "template" twice. You need to re-initialize 
> > it. The first one will blow away the XXXX pattern.
> 
> Turns out that would be awkward, as the xmkstemp() function would have to 
> copy the template just in case mkstemp() fails due to too many open files, 
> and of course it would need to release the copy afterwards.
> 
> OTOH we cannot just use the initialized filename, because there 
> might be a race condition with another process, right?

> On Sat, 23 May 2009, Johannes Schindelin wrote:
> > On Fri, 15 May 2009, Linus Torvalds wrote:
> > 
> > > On Fri, 15 May 2009, Johannes Schindelin wrote:
> > > >  
> > > >  	fd = mkstemp(template);
> > > > +	if (fd < 0) {
> > > > +		release_pack_memory((size_t)-1ll, -1);
> > > > +		fd = mkstemp(template);
> > > 
> > > This is wrong. You can't use "template" twice. You need to re-initialize 
> > > it. The first one will blow away the XXXX pattern.
> > 
> > Turns out that would be awkward, as the xmkstemp() function would have to 
> > copy the template just in case mkstemp() fails due to too many open files, 
> > and of course it would need to release the copy afterwards.
> > 
> > OTOH we cannot just use the initialized filename, because there 
> > might be a race condition with another process, right?
> 
> Correct. You basically need to have the caller re-create the template. We 
> already do that in some other cases: see odb_mkstemp(), or 
> create_tempfile(). Both of them do that template re-creation on failure, 
> and try again.
> 
> You could save off the template into a local temporary array, of course.

> > 
> > 	[core]
> > 		packedGitWindowSize = 16k
> > 		packedGitLimit = 1M
> > 
> > you should hopefully be able to repack (slowly) even with a low file 
> > descriptor limit, because of the total limit on the size.
> 
> I don't think so, because the window size has nothing to do with the 
> amount of open windows, right?

> On Fri, 15 May 2009, Johannes Schindelin wrote:
>
> > Linus said:
> >
> > > Dscho? What are your pack-file statistics in this case?
> > 
> > Mostly around 50kB.
> 
> Can you give us an approximation for how many are under 16kB or so?

> On Fri, 15 May 2009, Johannes Schindelin wrote:
> > > 
> > > 	[core]
> > > 		packedGitWindowSize = 16k
> > > 		packedGitLimit = 1M
> > > 
> > > you should hopefully be able to repack (slowly) even with a low file 
> > > descriptor limit, because of the total limit on the size.
> > 
> > I don't think so, because the window size has nothing to do with the 
> > amount of open windows, right?
> 
> No, but the packedGitLimit does.
> 
> So the argument is that you can't fit all that many open windows in 1M. 
> 
> > No, in this case, nothing was disabled.  auto-gc did not kick in, probably 
> > due to funny Git usage in hg2git.
> 
> Ahh. Scripting.
> 
> > > Dscho? What are your pack-file statistics in this case?
> > 
> > Mostly around 50kB.
> 
> Can you give us an approximation for how many are under 16kB or so?
> 
> The 16kB cutoff is where it's usually just better to malloc+read (because 
> even if you don't end up using all the 16kB, just a page-fault or two is 
> already more expensive than just doing the memcpy implied in a 16k read() 
> system call).
> 
> > But using malloc()+read() to avoid my use case sounds not 
> > straight-forward; it is rather a work-around than a proper solution.
> 
> Well, it's a workaround that is correct for other reasons too. So it's 
> likely worth doing.
> 
> The "proper solution" is likely to not avoid repacking. Scripted stuff 
> that does that is buggy.

> I also think you _should_ be able to avoid this by just limiting the pack
> size usage. IOW, with some packed_git_limit, something like
>
> 	[core]
> 		packedGitWindowSize = 16k
> 		packedGitLimit = 1M

> 
> You might end up with a situation where you have tons of pack files, e.g.
> when using hg2git.  In this situation, all kinds of operations may 
> end up with a "too many files open" error.  Let's recover gracefully from 
> that.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

> diff --git a/sha1_file.c b/sha1_file.c
> index 28bd908..bd5edd8 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -720,6 +720,8 @@ static int open_packed_git_1(struct packed_git *p)
>  		return error("packfile %s index unavailable", p->pack_name);
>  
>  	p->pack_fd = open(p->pack_name, O_RDONLY);
> +	while (p->pack_fd < 0 && errno == EMFILE && unuse_one_window(p, -1))
> +		p->pack_fd = open(p->pack_name, O_RDONLY);
>  	if (p->pack_fd < 0 || fstat(p->pack_fd, &st))
>  		return -1;
>  
> @@ -937,6 +939,8 @@ static void prepare_packed_git_one(char *objdir, int local)
>  	sprintf(path, "%s/pack", objdir);
>  	len = strlen(path);
>  	dir = opendir(path);
> +	while (!dir && errno == EMFILE && unuse_one_window(packed_git, -1))
> +		dir = opendir(path);
>  	if (!dir) {
>  		if (errno != ENOENT)
>  			error("unable to open object pack directory: %s: %s",
> @@ -2339,6 +2343,8 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
>  
>  	filename = sha1_file_name(sha1);
>  	fd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);
> +	while (fd < 0 && errno == EMFILE && unuse_one_window(packed_git, -1))
> +		fd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);
>  	if (fd < 0) {
>  		if (errno == EACCES)
>  			return error("insufficient permission for adding an object to repository database %s\n", get_object_directory());
> -- 

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> 
>> You might end up with a situation where you have tons of pack files, e.g.
>> when using hg2git.  In this situation, all kinds of operations may 
>> end up with a "too many files open" error.  Let's recover gracefully from 
>> that.
>> 
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Yea, this looks right to me.  JGit had a similar problem and now
> maintains a core.packedGitOpenFiles parameter, set to 128 by default,
> as one of the rules it uses to limit the size of its pack cache area.
>
>> diff --git a/sha1_file.c b/sha1_file.c
>> index 28bd908..bd5edd8 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -720,6 +720,8 @@ static int open_packed_git_1(struct packed_git *p)
>>  		return error("packfile %s index unavailable", p->pack_name);
>>  
>>  	p->pack_fd = open(p->pack_name, O_RDONLY);
>> +	while (p->pack_fd < 0 && errno == EMFILE && unuse_one_window(p, -1))
>> +		p->pack_fd = open(p->pack_name, O_RDONLY);
>>  	if (p->pack_fd < 0 || fstat(p->pack_fd, &st))
>>  		return -1;