Re: git update-ref --stdin : too many open files

[Michael Haggerty <mhagger@alum.mit.edu>]

On 12/24/2014 03:11 AM, Stefan Beller wrote:
> On 22.12.2014 13:22, Junio C Hamano wrote:
>> Loic Dachary <loic@dachary.org> writes:
>>
>>> Hi,
>>>
>>> Steps to reproduce:
>>>
>>> $ git --version
>>> git version 1.9.1
>>> $ wc -l /tmp/1
>>> 9090 /tmp/1
>>> $ head /tmp/1
>>> delete refs/pull/1/head
>>> create refs/heads/pull/1 86b715f346e52920ca7c9dfe65424eb9946ebd61
>>> delete refs/pull/1/merge
>>> create refs/merges/1 c0633abdc5311354c9729374e0ba25c97a89f69e
>>> ...
>>> $ ulimit -n
>>> 1024
>>> $ git update-ref --stdin < /tmp/1
>>> fatal: Unable to create
>>> /home/gitmirror/repositories/Ceph/ceph/refs/heads/pull/1917.lock': Too
>>> many open files
>>> $ head -20 /tmp/1 | git update-ref --stdin
>>> $ echo $?
>>> 0
>>>
>>> The workaround is to increase ulimit -n
>>>
>>> git update-ref --stdin should probably close some files.
>>>
>>> Cheers
>>
>> Sounds like the recent "ref update in a transaction" issue to me.
>>
>> Stefan, want to take a look?  I think we do need to keep the .lock
>> files without renaming while in transaction, but we do not have to
>> keep them open, so I suspect that a fix may be to split the commit
>> function into two (one to close but not rename, the other to
>> finalize by renaming) or something.
>>
>> Also the version of transaction series we have queued seem to lock
>> these refs very late in the process, but as we discussed privately
>> a few weeks ago, we would want to move the lock much earlier, when
>> the first update is attempted.
> 
> So I decided the first thing to do was to put this case into the test
> suite. so I typed in good faith:
> 
> test_expect_success 'but does it scale?' '
> 	for i in $(seq 1 1234567) ; do
> 		git branch branch_${i}
> 		echo "delete refs/heads/branch_${i}" >>large_input
> 	done
> 	git update-ref --stdin <large_input
> '
> 
> And there I have problems with my hard disk having more than a million
> files in one directory. So once I get rid of that I'll come up with a
> better way to test and fix this problem.

I suggest something like the following to demonstrate the failure. Note
the attempt to set "ulimit -l" to a lower value to make the test more
tractable. (Given that change, sharding the references, which is also
demonstrated here, is perhaps superfluous.)

test_expect_failure 'big transaction does not burst open file limit' '
(
	# Temporarily consider it a failure if we cannot reduce
	# the allowed number of open files:
	test "$(ulimit -n)" -le 1024 || ulimit -n 1024 || exit 1
	for i in $(seq 33)
	do
		for j in $(seq 32)
		do
			echo "create refs/heads/$i/$j HEAD"
		done
	done >large_input &&
	git update-ref --stdin <large_input &&
	git rev-parse --verify -q refs/heads/33/32
)
'

After the bug is fixed, this can be changed to

test_expect_success 'big transaction does not burst open file limit' '
(
	# Try to reduce the allowed number of open files, but even if
	# that is not possible, run the test anyway:
	test "$(ulimit -n)" -le 1024 || ulimit -n 1024
	for i in $(seq 33)
	do
		for j in $(seq 32)
		do
			echo "create refs/heads/$i/$j HEAD"
		done
	done >large_input &&
	git update-ref --stdin <large_input &&
	git rev-parse --verify -q refs/heads/33/32
)
'

It might make sense to test both the "create" and the "delete" code
paths, as their locking sequence differs.

I'm doing some work in this area, so I should be able to work on the
bugfix in the not-too-distant future. My feeling is that the bug is
unlikely to affect many current users, though it definitely should be
fixed before sb/atomic-push is merged.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu