From: Stefan Sperling <stsp@stsp.name>
Subject: Re: gotwebd: cache repos in struct server
To: gameoftrees@openbsd.org
Date: Wed, 31 Aug 2022 11:20:45 +0200

On Wed, Aug 31, 2022 at 10:54:15AM +0200, Stefan Sperling wrote:
> gotwebd currently opens repositories per-request (and usually closes
> them once the request is done, but fails to close them under certain
> error conditions).
> 
> With this patch we keep a list of cached repositories in the server
> context. Only one cached repo can be used when serving a request
> because we only have one set of pack_fd temp files available.
> 
> However, the cache reduces per-request overhead and also ensures
> that repositories get closed properly as they cycle out of cache
> and when the server terminates.
> 
> ok?

Update version which fixes an off-by-one found by op@, and undoes
the effects of memmove() if we fail to add a new cache element
after eviting an existing one.
 
diff e9d3ad59c92a3ce340f8ac1bd0b0dff75dc3d951 9194501c89f98bcd0491e69332b467b9ab02d406
commit - e9d3ad59c92a3ce340f8ac1bd0b0dff75dc3d951
commit + 9194501c89f98bcd0491e69332b467b9ab02d406
blob - 72a93ffe88a0fd5d23ee1850893733ac1c37c39c
blob + 7a4b84adb7b1259a6f359f3575dea871e09cf0ca
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -292,8 +292,6 @@ err:
 	if (html && fcgi_printf(c, "</div>\n") == -1)
 		return;
 done:
-	if (c->t->repo != NULL && qs && qs->action != INDEX)
-		got_repo_close(c->t->repo);
 	if (html && srv != NULL)
 		gotweb_render_footer(c);
 }
@@ -1096,9 +1094,6 @@ render:
 
 		gotweb_free_repo_dir(repo_dir);
 		repo_dir = NULL;
-		error = got_repo_close(t->repo);
-		if (error)
-			goto done;
 		t->next_disp++;
 		if (d_disp == srv->max_repos_display)
 			break;
@@ -1911,13 +1906,75 @@ done:
 	return error;
 }
 
+static struct got_repository *
+find_cached_repo(struct server *srv, const char *path)
+{
+	int i;
+
+	for (i = 0; i < srv->ncached_repos; i++) {
+		if (strcmp(srv->cached_repos[i].path, path) == 0)
+			return srv->cached_repos[i].repo;
+	}
+
+	return NULL;
+}
+
 static const struct got_error *
+cache_repo(struct got_repository **new, struct server *srv,
+    struct repo_dir *repo_dir, struct socket *sock)
+{
+	const struct got_error *error = NULL;
+	struct got_repository *repo;
+	struct cached_repo *cr;
+	int evicted = 0;
+
+	if (srv->ncached_repos >= nitems(srv->cached_repos)) {
+		cr = &srv->cached_repos[srv->ncached_repos - 1];
+		error = got_repo_close(cr->repo);
+		memset(cr, 0, sizeof(*cr));
+		srv->ncached_repos--;
+		if (error)
+			return error;
+		memmove(&srv->cached_repos[1], &srv->cached_repos[0],
+		    srv->ncached_repos * sizeof(srv->cached_repos[0]));
+		cr = &srv->cached_repos[0];
+		evicted = 1;
+	} else {
+		cr = &srv->cached_repos[srv->ncached_repos];
+	}
+
+	error = got_repo_open(&repo, repo_dir->path, NULL, sock->pack_fds);
+	if (error) {
+		if (evicted) {
+			memmove(&srv->cached_repos[0], &srv->cached_repos[1],
+			    srv->ncached_repos * sizeof(srv->cached_repos[0]));
+		}
+		return error;
+	}
+
+	if (strlcpy(cr->path, repo_dir->path, sizeof(cr->path))
+	    >= sizeof(cr->path)) {
+		if (evicted) {
+			memmove(&srv->cached_repos[0], &srv->cached_repos[1],
+			    srv->ncached_repos * sizeof(srv->cached_repos[0]));
+		}
+		return got_error(GOT_ERR_NO_SPACE);
+	}
+
+	cr->repo = repo;
+	srv->ncached_repos++;
+	*new = repo;
+	return NULL;
+}
+
+static const struct got_error *
 gotweb_load_got_path(struct request *c, struct repo_dir *repo_dir)
 {
 	const struct got_error *error = NULL;
 	struct socket *sock = c->sock;
 	struct server *srv = c->srv;
 	struct transport *t = c->t;
+	struct got_repository *repo = NULL;
 	DIR *dt;
 	char *dir_test;
 	int opened = 0;
@@ -1977,9 +2034,13 @@ gotweb_load_got_path(struct request *c, struct repo_di
 	} else
 		opened = 1;
 done:
-	error = got_repo_open(&t->repo, repo_dir->path, NULL, sock->pack_fds);
-	if (error)
-		goto err;
+	repo = find_cached_repo(srv, repo_dir->path);
+	if (repo == NULL) {
+		error = cache_repo(&repo, srv, repo_dir, sock);
+		if (error)
+			goto err;
+	}
+	t->repo = repo;
 	error = gotweb_get_repo_description(&repo_dir->description, srv,
 	    repo_dir->path);
 	if (error)
blob - ae08f3df1ae9fb58a5fac19a6fcfd02a1aad9247
blob + 74ee67ae6734f6af0af3ab58f3276ceeccc8863a
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -46,6 +46,7 @@
 #define GOTWEBD_MAXPORT		 6
 #define GOTWEBD_NUMPROC		 3
 #define GOTWEBD_MAXIFACE	 16
+#define GOTWEBD_REPO_CACHESIZE	 4
 
 /* GOTWEB DEFAULTS */
 #define MAX_QUERYSTRING		 2048
@@ -250,10 +251,18 @@ struct address {
 };
 TAILQ_HEAD(addresslist, address);
 
+struct cached_repo {
+	char path[PATH_MAX];
+	struct got_repository *repo;
+};
+
 struct server {
 	TAILQ_ENTRY(server)	 entry;
 	struct addresslist	al;
 
+	struct cached_repo	cached_repos[GOTWEBD_REPO_CACHESIZE];
+	int		 ncached_repos;
+
 	char		 name[GOTWEBD_MAXTEXT];
 
 	char		 repos_path[PATH_MAX];
blob - 7e510af9c4ae6f2609874519758443d006c1afc3
blob + c0691c5575dca8c931af0e8e30d9e086b5a5bccc
--- gotwebd/sockets.c
+++ gotwebd/sockets.c
@@ -51,6 +51,7 @@
 
 #include "got_error.h"
 #include "got_opentemp.h"
+#include "got_repository.h"
 
 #include "proc.h"
 #include "gotwebd.h"
@@ -351,6 +352,7 @@ sockets_shutdown(void)
 {
 	struct server *srv, *tsrv;
 	struct socket *sock, *tsock;
+	int i;
 
 	sockets_purge(gotwebd_env);
 
@@ -362,8 +364,11 @@ sockets_shutdown(void)
 	}
 
 	/* clean servers */
-	TAILQ_FOREACH_SAFE(srv, &gotwebd_env->servers, entry, tsrv)
+	TAILQ_FOREACH_SAFE(srv, &gotwebd_env->servers, entry, tsrv) {
+		for (i = 0; i < srv->ncached_repos; i++)
+			got_repo_close(srv->cached_repos[i].repo);
 		free(srv);
+	}
 
 	free(gotwebd_env);
 }