List for cgit developers and users
 help / color / mirror / Atom feed
From: Steffen Nurpmeso <steffen@sdaoden.eu>
To: gs-cgit-lists.zx2c4.com@gluelogic.com
Cc: cgit@lists.zx2c4.com
Subject: Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
Date: Tue, 22 Dec 2020 22:22:07 +0100	[thread overview]
Message-ID: <20201222212207.3qb-M%steffen@sdaoden.eu> (raw)
In-Reply-To: <20201222150957._aAW5%steffen@sdaoden.eu>

Once more, hey,

Steffen Nurpmeso wrote in
 <20201222150957._aAW5%steffen@sdaoden.eu>:
 |Steffen Nurpmeso wrote in
 | <20201222135537.ovyl4%steffen@sdaoden.eu>:
 ||gs-cgit-lists.zx2c4.com@gluelogic.com wrote in
 || <20201222061206.GA54419@xps13>:
 |||>Steffen Nurpmeso wrote in
 |||> <20201221193127.zbZeP%steffen at sdaoden.eu>:
 |||>|John Keeping wrote in
 |||>| <X+DiDgGaPaynnocI at john.keeping.me.uk>:
 |||>||On Mon, Dec 21, 2020 at 05:26:19PM +0100, Steffen Nurpmeso wrote:
 |||>||> I discovered today that cgit no longer delivers pages, and it must
 |||>||> have been like that for some time.  The server looks show
 |||>||> successful delivery, the cgit cache is populated and rotated just
 |||>||> correctly, but all cgit delivers is that final error of main() as
 ...
 |||>||> I am pretty sure cgit delivered some weeks ago, the most notable
 |||>||> difference is that AlpineLinux switched to Lighttpd 1.4.56 then
 |||>||> .57, which seems to have brought tremendous changes under the
 |||> ...
 |||>|But the file was generated normally:
 ...
 |||>||and this may be caused by sendfile(2) failing due to some difference \
 |||>||in
 |||>||how the web server is setting up the output file descriptor.  You may
 ...

Looking at lighttpd commit history it seems he now uses splice(2)
for CGI if possible.

  ...
 |||I would like to propose an alternative and more portable solution:
 |||
 |||cgit cache should fallback to lseek, xread, xwrite if sendfile fails.

It would be cool if the sendfile(2) could be avoided via
configuration.  It seems so wasteful to always have that
system-call, context-switch and such.
What do you think of (on [master] aka
adcc4f822fe11836e5f942fc1ae0f00db4eb8d5f plus the other patch),
the following runs on my server VM now and works:

diff --git a/cache.c b/cache.c
index b09769e..8a66db9 100644
--- a/cache.c
+++ b/cache.c
@@ -25,6 +25,7 @@
 struct cache_slot {
 	const char *key;
 	size_t keylen;
+	int sndfpok;
 	int ttl;
 	cache_fill_fn fn;
 	int cache_fd;
@@ -92,7 +93,7 @@ static int print_slot(struct cache_slot *slot)
 
 	start_off = slot->keylen + 1;
 
-	do {
+	if (slot->sndfpok) for(;;) {
 		ret = sendfile(STDOUT_FILENO, slot->cache_fd, &start_off,
 				slot->cache_st.st_size - start_off);
 		if (ret < 0) {
@@ -103,7 +104,7 @@ static int print_slot(struct cache_slot *slot)
 			return errno;
 		}
 		return 0;
-	} while (1);
+	}
 #endif
 
 	i = lseek(slot->cache_fd, slot->keylen + 1, SEEK_SET);
@@ -350,7 +351,7 @@ static int process_slot(struct cache_slot *slot)
 
 /* Print cached content to stdout, generate the content if necessary. */
 int cache_process(int size, const char *path, const char *key, int ttl,
-		  cache_fill_fn fn)
+		  cache_fill_fn fn, int sndfpok)
 {
 	unsigned long hash;
 	int i;
@@ -383,6 +384,7 @@ int cache_process(int size, const char *path, const char *key, int ttl,
 	strbuf_addbuf(&lockname, &filename);
 	strbuf_addstr(&lockname, ".lock");
 	slot.fn = fn;
+	slot.sndfpok = sndfpok;
 	slot.ttl = ttl;
 	slot.stdout_fd = -1;
 	slot.cache_name = filename.buf;
diff --git a/cache.h b/cache.h
index 470da4f..3e25daa 100644
--- a/cache.h
+++ b/cache.h
@@ -17,12 +17,13 @@ typedef void (*cache_fill_fn)(void);
  *   key     the key used to lookup cache files
  *   ttl     max cache time in seconds for this key
  *   fn      content generator function for this key
+ *   sndfpok allowed to use sendfile(2)
  *
  * Return value
  *   0 indicates success, everything else is an error
  */
 extern int cache_process(int size, const char *path, const char *key, int ttl,
-			 cache_fill_fn fn);
+			 cache_fill_fn fn, int sndfpok);
 
 
 /* List info about all cache entries on stdout */
diff --git a/cgit.c b/cgit.c
index 08d81a1..684f97a 100644
--- a/cgit.c
+++ b/cgit.c
@@ -197,6 +197,8 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.enable_git_config = atoi(value);
 	else if (!strcmp(name, "max-stats"))
 		ctx.cfg.max_stats = cgit_find_stats_period(value, NULL);
+	else if (!strcmp(name, "cache-sendfile"))
+		ctx.cfg.cache_sendfile = atoi(value);
 	else if (!strcmp(name, "cache-size"))
 		ctx.cfg.cache_size = atoi(value);
 	else if (!strcmp(name, "cache-root"))
@@ -363,6 +365,7 @@ static void prepare_context(void)
 {
 	memset(&ctx, 0, sizeof(ctx));
 	ctx.cfg.agefile = "info/web/last-modified";
+	ctx.cfg.cache_sendfile = 1;
 	ctx.cfg.cache_size = 0;
 	ctx.cfg.cache_max_create_time = 5;
 	ctx.cfg.cache_root = CGIT_CACHE_ROOT;
@@ -1103,7 +1106,8 @@ int cmd_main(int argc, const char **argv)
 	if (!ctx.env.authenticated || (ctx.env.request_method && !strcmp(ctx.env.request_method, "HEAD")))
 		ctx.cfg.cache_size = 0;
 	err = cache_process(ctx.cfg.cache_size, ctx.cfg.cache_root,
-			    ctx.qry.raw, ttl, process_request);
+			    ctx.qry.raw, ttl, process_request,
+			    ctx.cfg.cache_sendfile);
 	cgit_cleanup_filters();
 	if (err)
 		cgit_print_error("Error processing page: %s (%d)",
diff --git a/cgit.h b/cgit.h
index 69b5c13..21677a6 100644
--- a/cgit.h
+++ b/cgit.h
@@ -215,6 +215,7 @@ struct cgit_config {
 	char *repository_sort;
 	char *virtual_root;	/* Always ends with '/'. */
 	char *strict_export;
+	int cache_sendfile;
 	int cache_size;
 	int cache_dynamic_ttl;
 	int cache_max_create_time;
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 33a6a8c..7ea1b81 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -83,6 +83,10 @@ cache-scanrc-ttl::
 	of scanning a path for git repositories. See also: "CACHE". Default
 	value: "15".
 
+cache-sendfile::
+	Whether sendfile(2) optimization may be used for CGI outputs, when
+	available. Default value: "1".
+
 case-sensitive-sort::
 	Sort items in the repo list case sensitively. Default value: "1".
 	See also: repository-sort, section-sort.

--steffen
|
|Der Kragenbaer,                The moon bear,
|der holt sich munter           he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)

  reply	other threads:[~2020-12-22 21:22 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-22  6:12 gs-cgit-lists.zx2c4.com
2020-12-22 13:55 ` Steffen Nurpmeso
2020-12-22 15:09   ` Steffen Nurpmeso
2020-12-22 21:22     ` Steffen Nurpmeso [this message]
2020-12-29 17:04       ` Steffen Nurpmeso
  -- strict thread matches above, loose matches on Subject: below --
2020-12-21 16:26 Steffen Nurpmeso
2020-12-21 17:57 ` John Keeping
2020-12-21 19:31   ` Steffen Nurpmeso
2020-12-21 22:23     ` Steffen Nurpmeso
2020-12-21 23:24       ` Steffen Nurpmeso
2021-01-02 18:38 ` Jon DeVree
2021-01-15 18:01   ` Jon DeVree
2021-01-15 21:51     ` Konstantin Ryabitsev
2021-01-15 22:41       ` Jon DeVree

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201222212207.3qb-M%steffen@sdaoden.eu \
    --to=steffen@sdaoden.eu \
    --cc=cgit@lists.zx2c4.com \
    --cc=gs-cgit-lists.zx2c4.com@gluelogic.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).