From: Stefan Sperling Subject: Re: gotwebd: cache repos in struct server To: gameoftrees@openbsd.org Date: Wed, 31 Aug 2022 11:40:44 +0200 On Wed, Aug 31, 2022 at 11:20:45AM +0200, Stefan Sperling wrote: > 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. And another update to address a problem I found during testing. If repositories are updated with new commits while gotwebd is running then gotwebd can run into errors while accessing the repository. For example, I saw it run into this: gotwebd experienced an error: open: \ /got/public/got.git/objects/69/02644eb5884f064faf1f771aaef6bf956e485d: \ No such file or directory This error kept repeating while I reloaded the page in the browser. This new version of the patch invalidates the repo cache on error, causing everything to be refreshed when the next request comes in. ok? diff e9d3ad59c92a3ce340f8ac1bd0b0dff75dc3d951 072a608f2aa900bf9f5a2dc39c593f020580f35d commit - e9d3ad59c92a3ce340f8ac1bd0b0dff75dc3d951 commit + 072a608f2aa900bf9f5a2dc39c593f020580f35d blob - 72a93ffe88a0fd5d23ee1850893733ac1c37c39c blob + 095c430b0281d6659d2f08e87158bb71d41a4285 --- gotwebd/gotweb.c +++ gotwebd/gotweb.c @@ -110,6 +110,78 @@ static void gotweb_free_repo_dir(struct repo_dir *); struct server *gotweb_get_server(uint8_t *, uint8_t *); +static void +invalidate_repo_cache(struct server *srv) +{ + int i; + + for (i = 0; i < srv->ncached_repos; i++) + got_repo_close(srv->cached_repos[i].repo); + memset(srv->cached_repos, 0, sizeof(srv->cached_repos)); + srv->ncached_repos = 0; +} + +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; +} + void gotweb_process_request(struct request *c) { @@ -278,6 +350,13 @@ render: goto done; err: + if (srv != NULL) { + /* + * We might have run into problems due to stale repository + * caches. Close all repositories to try again later. + */ + invalidate_repo_cache(srv); + } if (html && fcgi_printf(c, "
") == -1) return; if (fcgi_printf(c, "%s", err) == -1) @@ -292,8 +371,6 @@ err: if (html && fcgi_printf(c, "
\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 +1173,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; @@ -1918,6 +1992,7 @@ gotweb_load_got_path(struct request *c, struct repo_di 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 +2052,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); }