mbox series

[0/4] commit-graph: handle file descriptor exhaustion

Message ID cover.1587677671.git.me@ttaylorr.com (mailing list archive)
Headers show
Series commit-graph: handle file descriptor exhaustion | expand

Message

Taylor Blau April 23, 2020, 9:40 p.m. UTC
Hi,

This is another series to handle an issue we started seeing at GitHub on
repositories that have too many commit-graph layers.

This issue evaded me for several days, but the gist is that having too
many descriptors open caused us to fail 'git_mkstemp_mode', which in
turn left some values in 'ctx->commit_graph_filenames_after'
uninitialized. This in turn caused a general protection fault when
trying to free these uninitialized entries. Frustratingly, it causes
stray 'commit-graph-chain.lock' files to stick around even after the
process has gone away, causing all subsequent commit-graph write
operations to fail.

The first two patches are semi-related cleanups and other preparation.
The third patch fixes the main issue described here. Peff contributed
the fourth patch which teaches the commit-graph machinery to be more
willing to close file descriptors.

These patches are very much owed to the help that Stolee and Peff
provided while writing them and for their willingness to let me bounce
some ideas off of them.

Thanks in advance for your review.

Jeff King (1):
  commit-graph: close descriptors after mmap

Taylor Blau (3):
  commit-graph.c: don't use discarded graph_name in error
  t/test-lib.sh: make ULIMIT_FILE_DESCRIPTORS available to tests
  commit-graph.c: gracefully handle file descriptor exhaustion

 commit-graph.c                | 21 ++++++++-------------
 commit-graph.h                |  3 +--
 fuzz-commit-graph.c           |  5 ++---
 t/t1400-update-ref.sh         |  9 ---------
 t/t5324-split-commit-graph.sh | 13 +++++++++++++
 t/test-lib.sh                 |  9 +++++++++
 6 files changed, 33 insertions(+), 27 deletions(-)

--
2.26.0.113.ge9739cdccc

Comments

Taylor Blau April 23, 2020, 9:43 p.m. UTC | #1
...I should mention that this is based on my series in [1], since it's
much easier to express the new test in t5324 with '--split=no-merge'.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/cover.1586836700.git.me@ttaylorr.com/