Re: [PATCH 4/4] commit-graph: close descriptors after mmap
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Apr 24, 2020, 13:17 UTC
- Message-ID
- <2232c379-d0ec-0b52-96b4-379438642785@gmail.com>
- In-Reply-To
- <xmqq368tg8po.fsf@gitster.c.googlers.com>
On 4/23/2020 6:04 PM, Junio C Hamano wrote:
Show 14 quoted lines
> Taylor Blau <me@ttaylorr.com> writes: > >> From: Jeff King <peff@peff.net> >> >> We don't ever refer to the descriptor after mmap-ing it. And keeping it >> open means we can run out of descriptors in degenerate cases (e.g., >> thousands of split chain files). Let's close it as soon as possible. > > Yikes. > > Sorry, I should have looked at the use of mmap in this topioc more > carefully when we queued the series. It is an easy mistake to make > by anybody new to the API to leave it open while the region is in > use.
You are right. I was new when first contributing the commit-graph. It was also easier to miss because we only had one commit-graph open at the time. Adding in the incremental file format led to multiple file descriptors being open.
However, this idea of closing a descriptor after an mmap is new to me. So I thought about other situations where I made the same mistake. Please see the patch below.
> With this fix, with or without the other topics still in flight, I > do not think no code touches graph_fd. Should we remove the > graph_fd field from the structure as well?
I agree that this should be done.
Thanks, -Stolee
-->8--
From: Derrick Stolee <dstolee@microsoft.com> Date: Fri, 24 Apr 2020 13:11:13 +0000 Subject: [PATCH] multi-pack-index: close file descriptor after mmap
We recently discovered that the commit-graph was not closing its file descriptor after memory-mapping the file contents. After this mmap() succeeds, there is no need to keep the file descriptor open. In fact, there is signficant reason to close it so we do not run out of descriptors.
This is entirely my fault for not knowing that we can have an open mmap while also closing the descriptor. Some could blame this on being a new contributor when the series was first submitted, but even years later this is still new information to me.
That made me realize that I used the same pattern when opening a multi-pack-index. Since this file is not (yet) incremental, there will be at most one descriptor open for this reason. It is still worth fixing, especially if we extend the format to be incremental like the commit-graph.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- midx.c | 4 +--- midx.h | 2 -- 2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/midx.c b/midx.c index 1527e464a7..60d30e873b 100644 --- a/midx.c +++ b/midx.c @@ -72,9 +72,9 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local FREE_AND_NULL(midx_name); midx_map = xmmap(NULL, midx_size, PROT_READ, MAP_PRIVATE, fd, 0); + close(fd); FLEX_ALLOC_STR(m, object_dir, object_dir); - m->fd = fd; m->data = midx_map; m->data_len = midx_size; m->local = local; @@ -190,8 +190,6 @@ void close_midx(struct multi_pack_index *m) return; munmap((unsigned char *)m->data, m->data_len); - close(m->fd); - m->fd = -1; for (i = 0; i < m->num_packs; i++) { if (m->packs[i]) diff --git a/midx.h b/midx.h index e6fa356b5c..b18cf53bc4 100644 --- a/midx.h +++ b/midx.h @@ -12,8 +12,6 @@ struct repository; struct multi_pack_index { struct multi_pack_index *next; - int fd; - const unsigned char *data; size_t data_len;
-- 2.26.2