Re: git update-ref --stdin : too many open files
On 12/24/2014 03:11 AM, Stefan Beller wrote:
Show 60 quoted lines
> 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
