List for cgit developers and users
 help / color / mirror / Atom feed
* cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
@ 2020-12-21 16:26 Steffen Nurpmeso
  2020-12-21 17:57 ` John Keeping
  2021-01-02 18:38 ` Jon DeVree
  0 siblings, 2 replies; 14+ messages in thread
From: Steffen Nurpmeso @ 2020-12-21 16:26 UTC (permalink / raw)
  To: cgit

Hello.

My first post here, so first a "Thank you!", i am using cgit on
lighttpd on Linux for half a decade, and it just works (after
finally having a usable configuration).

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

  <div class='error'>Error processing page: Invalid argument (22)</div>

(surely a misconfiguration that this is not a real HTML page,
i recall it took some time to figure out about about-filter, just
"doing it" (<pre>cat(1)</pre>)).

If i set "cache-size=0" then the desired page is delivered.
Note the cache itself is managed as usual with the default
cache-size=1999.

The files in the cache are 0600, i thought maybe that was it, but
the setting is actively reverted to this mask (i never looked at
cgit code except grepping the error).  It has always been so, the
oldest cache entry was

  -rw-------    1 lighttpd lighttpd      3023 Mar 18  2018 d2200000

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
hood, like HTTP/2 support and OCSP as well as support for all the
different TLS libraries, whereas before it only was OpenSSL and
compatibles, i think.  We have HTTP/2 not enabled yet.

Anything i can do about this?

--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)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
  2020-12-21 16:26 cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery Steffen Nurpmeso
@ 2020-12-21 17:57 ` John Keeping
  2020-12-21 19:31   ` Steffen Nurpmeso
  2021-01-02 18:38 ` Jon DeVree
  1 sibling, 1 reply; 14+ messages in thread
From: John Keeping @ 2020-12-21 17:57 UTC (permalink / raw)
  To: Steffen Nurpmeso; +Cc: cgit

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
> 
>   <div class='error'>Error processing page: Invalid argument (22)</div>
> 
> (surely a misconfiguration that this is not a real HTML page,
> i recall it took some time to figure out about about-filter, just
> "doing it" (<pre>cat(1)</pre>)).
> 
> If i set "cache-size=0" then the desired page is delivered.
> Note the cache itself is managed as usual with the default
> cache-size=1999.
> 
> The files in the cache are 0600, i thought maybe that was it, but
> the setting is actively reverted to this mask (i never looked at
> cgit code except grepping the error).  It has always been so, the
> oldest cache entry was
> 
>   -rw-------    1 lighttpd lighttpd      3023 Mar 18  2018 d2200000
> 
> 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
> hood, like HTTP/2 support and OCSP as well as support for all the
> different TLS libraries, whereas before it only was OpenSSL and
> compatibles, i think.  We have HTTP/2 not enabled yet.
> 
> Anything i can do about this?

Have you looked at the log output?

This error comes from cgit.c::cmd_main() as a result of
cache.c::cache_process() returning an error.  It looks like all of the
error paths there will write a more detailed error message to stderr.

I don't use lighttpd so I don't know whether that output is captured,
but normally output written to stderr will end up in a log file.

From a quick look at the code, given that the problem seems to be caused
by updating the web server, my guess is that you'll see the error:

	[cgit] error printing cache ...: Invalid argument (22)

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
want to rebuild CGit without HAVE_LINUX_SENDFILE and see if that works.

(If it does, we should add better fallback handling for the case where
stdout is not a valid output for sendfile().)


John

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
  2020-12-21 17:57 ` John Keeping
@ 2020-12-21 19:31   ` Steffen Nurpmeso
  2020-12-21 22:23     ` Steffen Nurpmeso
  0 siblings, 1 reply; 14+ messages in thread
From: Steffen Nurpmeso @ 2020-12-21 19:31 UTC (permalink / raw)
  To: John Keeping; +Cc: cgit

Hi.

John Keeping wrote in
 <X+DiDgGaPaynnocI@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
 |> 
 |>   <div class='error'>Error processing page: Invalid argument (22)</div>
 |> 
 |> (surely a misconfiguration that this is not a real HTML page,
 |> i recall it took some time to figure out about about-filter, just
 |> "doing it" (<pre>cat(1)</pre>)).
 |> 
 |> If i set "cache-size=0" then the desired page is delivered.
 |> Note the cache itself is managed as usual with the default
 |> cache-size=1999.
 |> 
 |> The files in the cache are 0600, i thought maybe that was it, but
 |> the setting is actively reverted to this mask (i never looked at
 |> cgit code except grepping the error).  It has always been so, the
 |> oldest cache entry was
 |> 
 |>   -rw-------    1 lighttpd lighttpd      3023 Mar 18  2018 d2200000
 |> 
 |> 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
 |> hood, like HTTP/2 support and OCSP as well as support for all the
 |> different TLS libraries, whereas before it only was OpenSSL and
 |> compatibles, i think.  We have HTTP/2 not enabled yet.
 |> 
 |> Anything i can do about this?
 |
 |Have you looked at the log output?

Yes, sorry, terrible mess above. Monday and no end.

  - Logs show nothing.  I enabled
      debug.log-request-handling = "enable" 
    but that showed nothing.  But wait, there was
    server.breakagelog that was needed .. as below!

 |This error comes from cgit.c::cmd_main() as a result of
 |cache.c::cache_process() returning an error.  It looks like all of the

Yes.

 |error paths there will write a more detailed error message to stderr.

Unfortunately nothing .. but with breakaglog there is

  [cgit] error printing cache /var/lib/lighttpd/cgit/b1000000: Invalid argument (22)

But the file was generated normally:

  # ll /var/lib/lighttpd/cgit/b1000000
  -rw-------    1 lighttpd lighttpd     23417 Dec 21 20:22 /var/lib/lighttpd/cgit/b1000000

 |I don't use lighttpd so I don't know whether that output is captured,
 |but normally output written to stderr will end up in a log file.

Was server.breakagelog i had forgotten about.

 |>From a quick look at the code, given that the problem seems to be caused
 |by updating the web server, my guess is that you'll see the error:
 |
 | [cgit] error printing cache ...: Invalid argument (22)
 |
 |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
 |want to rebuild CGit without HAVE_LINUX_SENDFILE and see if that works.

I did not know about 'cgi.x-sendfile = "enable"' yet, but setting
it does not matter from cgit's point of view.

 |(If it does, we should add better fallback handling for the case where
 |stdout is not a valid output for sendfile().)

Ok, i will hm do that mirroring the ArchLinux recipe and report
back.

Thanks!

 |John
 --End of <X+DiDgGaPaynnocI@john.keeping.me.uk>

--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)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
  2020-12-21 19:31   ` Steffen Nurpmeso
@ 2020-12-21 22:23     ` Steffen Nurpmeso
  2020-12-21 23:24       ` Steffen Nurpmeso
  0 siblings, 1 reply; 14+ messages in thread
From: Steffen Nurpmeso @ 2020-12-21 22:23 UTC (permalink / raw)
  To: John Keeping, cgit

Hello.

Steffen Nurpmeso wrote in
 <20201221193127.zbZeP%steffen@sdaoden.eu>:
 |John Keeping wrote in
 | <X+DiDgGaPaynnocI@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
 ||> 
 ||>   <div class='error'>Error processing page: Invalid argument (22)</div>
 ...
 ||> 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:
 |
 |  # ll /var/lib/lighttpd/cgit/b1000000
 |  -rw-------    1 lighttpd lighttpd     23417 Dec 21 20:22 /var/lib/lightt\
 |  pd/cgit/b1000000
 ...

Slightly resorted:

 ...
 ||Have you looked at the log output?
 |  [cgit] error printing cache /var/lib/lighttpd/cgit/b1000000: Invalid \
 |  argument (22)
 ...
 ||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
 ||want to rebuild CGit without HAVE_LINUX_SENDFILE and see if that works.

So i build it with

  #alp-2020:$ diff cgit.mk.orig cgit.mk
  --- cgit.mk.orig
  +++ cgit.mk
  @@ -64,7 +64,7 @@
   endif

   ifdef HAVE_LINUX_SENDFILE
  -       CGIT_CFLAGS += -DHAVE_LINUX_SENDFILE
  +       #CGIT_CFLAGS += -DHAVE_LINUX_SENDFILE
   endif

   CGIT_OBJ_NAMES += cgit.o

and

  make NO_LUA=y NO_ICONV=y NO_GETTEXT=y NO_TCLTK=y NO_PERL=1 \
    NO_PYTHON=1 NO_SVN_TESTS=y NO_REGEX=NeedsStartEnd prefix=/usr

and .. it works.

Thank you, i will open an AlpineLinux bug report.  And lighttpd.

--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)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
  2020-12-21 22:23     ` Steffen Nurpmeso
@ 2020-12-21 23:24       ` Steffen Nurpmeso
  0 siblings, 0 replies; 14+ messages in thread
From: Steffen Nurpmeso @ 2020-12-21 23:24 UTC (permalink / raw)
  To: John Keeping, cgit

Steffen Nurpmeso wrote in
 <20201221222300.z9Vio%steffen@sdaoden.eu>:
 |Steffen Nurpmeso wrote in
 | <20201221193127.zbZeP%steffen@sdaoden.eu>:
 ||John Keeping wrote in
 || <X+DiDgGaPaynnocI@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
 |||> 
 |||>   <div class='error'>Error processing page: Invalid argument (22)</div>
 ...
 |||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
 |||want to rebuild CGit without HAVE_LINUX_SENDFILE and see if that works.
 |
 |So i build it with
 ...
 |  -       CGIT_CFLAGS += -DHAVE_LINUX_SENDFILE
 |  +       #CGIT_CFLAGS += -DHAVE_LINUX_SENDFILE
 ...
 |and
 |
 |  make NO_LUA=y NO_ICONV=y NO_GETTEXT=y NO_TCLTK=y NO_PERL=1 \
 |    NO_PYTHON=1 NO_SVN_TESTS=y NO_REGEX=NeedsStartEnd prefix=/usr
 |
 |and .. it works.
 |
 |Thank you, i will open an AlpineLinux bug report.  And lighttpd.

Did that.  Though redmine lighttpd threw me out after password
reset, i posted in #lighttpd irc.freenode.net a few minutes ago.
FreeBSD also has a sendfile by the way, had that already twenty
years ago, and worked great!  Slightly different semantics though.
But doable (by then).  The guys from nginx strive for tweaking the
last little thing out of that btw.  Just last week iirc they added
NUMA specific ioctls to be able to gain more locality.

I think i will go again soon, now that this is worked out :)
Thanks,
Ciao and greetings from Germany,

--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)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
  2020-12-21 16:26 cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery Steffen Nurpmeso
  2020-12-21 17:57 ` John Keeping
@ 2021-01-02 18:38 ` Jon DeVree
  2021-01-15 18:01   ` Jon DeVree
  1 sibling, 1 reply; 14+ messages in thread
From: Jon DeVree @ 2021-01-02 18:38 UTC (permalink / raw)
  To: cgit

I think that this is a bug in the 5.10 kernel.

I just hit it on a previously working cgit host by upgrading the kernel
from 5.9.15 to 5.10.4. Downgrading the kernel back to 5.9.15 fixes it.

I'm guessing it was broken by 36e2c7421f02 ("fs: don't allow splice
read/write without explicit ops") because if I crank up kernel logging I
get:

    splice write not supported for file  (pid: 2522 comm: cgit.cgi)

-- 
Jon
Doge Wrangler
X(7): A program for managing terminal windows. See also screen(1) and tmux(1).

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
  2021-01-02 18:38 ` Jon DeVree
@ 2021-01-15 18:01   ` Jon DeVree
  2021-01-15 21:51     ` Konstantin Ryabitsev
  0 siblings, 1 reply; 14+ messages in thread
From: Jon DeVree @ 2021-01-15 18:01 UTC (permalink / raw)
  To: cgit

On Sat, Jan 02, 2021 at 13:38:39 -0500, Jon DeVree wrote:
> I think that this is a bug in the 5.10 kernel.
> 

I reported this upstream.

https://bugzilla.kernel.org/show_bug.cgi?id=211217

-- 
Jon
Doge Wrangler
X(7): A program for managing terminal windows. See also screen(1) and tmux(1).

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
  2021-01-15 18:01   ` Jon DeVree
@ 2021-01-15 21:51     ` Konstantin Ryabitsev
  2021-01-15 22:41       ` Jon DeVree
  0 siblings, 1 reply; 14+ messages in thread
From: Konstantin Ryabitsev @ 2021-01-15 21:51 UTC (permalink / raw)
  To: cgit

On Fri, Jan 15, 2021 at 01:01:02PM -0500, Jon DeVree wrote:
> On Sat, Jan 02, 2021 at 13:38:39 -0500, Jon DeVree wrote:
> > I think that this is a bug in the 5.10 kernel.
> > 
> 
> I reported this upstream.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=211217

So, bugzilla.k.o is not *really* upstream, because not all components use it.
I know this is dumb, but I'm not in the right situation to fix it (see this
discussion here:
https://lore.kernel.org/linux-doc/20210110121033.130504-1-linux@leemhuis.info/)

Going through bug backlinks, there's a claim that 5.10.4 fixes the problem --
is that accurate, or is the bug still present?

-K

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
  2021-01-15 21:51     ` Konstantin Ryabitsev
@ 2021-01-15 22:41       ` Jon DeVree
  0 siblings, 0 replies; 14+ messages in thread
From: Jon DeVree @ 2021-01-15 22:41 UTC (permalink / raw)
  To: cgit

On Fri, Jan 15, 2021 at 16:51:10 -0500, Konstantin Ryabitsev wrote:
> Going through bug backlinks, there's a claim that 5.10.4 fixes the problem --
> is that accurate, or is the bug still present?
> 

5.10.4 was the first 5.10 kernel uploaded to Debian Sid which is how I
discovered the issue.

It was still broken on the vanilla 5.10.7 kernel that I installed this
morning to see if it was fixed yet.

-- 
Jon
Doge Wrangler
X(7): A program for managing terminal windows. See also screen(1) and tmux(1).

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
  2020-12-22 21:22     ` Steffen Nurpmeso
@ 2020-12-29 17:04       ` Steffen Nurpmeso
  0 siblings, 0 replies; 14+ messages in thread
From: Steffen Nurpmeso @ 2020-12-29 17:04 UTC (permalink / raw)
  To: cgit; +Cc: gs-cgit-lists.zx2c4.com

Steffen Nurpmeso wrote in
 <20201222212207.3qb-M%steffen@sdaoden.eu>:
 |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.

On AlpineLinux with kernel 10.3 the unmodified cgit still fails.
I just want to note there is no kernel bugzilla entry regarding
sendfile and/or splice.

I assume the sendfile(2) failure fallback code path becomes
implemented and will unsubscribe again now.

Thanks.

--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)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
  2020-12-22 15:09   ` Steffen Nurpmeso
@ 2020-12-22 21:22     ` Steffen Nurpmeso
  2020-12-29 17:04       ` Steffen Nurpmeso
  0 siblings, 1 reply; 14+ messages in thread
From: Steffen Nurpmeso @ 2020-12-22 21:22 UTC (permalink / raw)
  To: gs-cgit-lists.zx2c4.com; +Cc: cgit

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)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
  2020-12-22 13:55 ` Steffen Nurpmeso
@ 2020-12-22 15:09   ` Steffen Nurpmeso
  2020-12-22 21:22     ` Steffen Nurpmeso
  0 siblings, 1 reply; 14+ messages in thread
From: Steffen Nurpmeso @ 2020-12-22 15:09 UTC (permalink / raw)
  To: gs-cgit-lists.zx2c4.com; +Cc: cgit

Hello!

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
 ||>||> 
 ||>||>   <div class='error'>Error processing page: Invalid argument \
 ||>||>   (22)</div\
 ||>||>>
 ||> ...
 ||>||> 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:
 ||>|
 ||>|  # ll /var/lib/lighttpd/cgit/b1000000
 ||>|  -rw-------    1 lighttpd lighttpd     23417 Dec 21 20:22 /var/lib/ligh\
 ||>|  t\
 ||>|  tpd/cgit/b1000000
 ||> ...
 ||>
 ||>Slightly resorted:
 ||>
 ||> ...
 ||>||Have you looked at the log output?
 ||>|  [cgit] error printing cache /var/lib/lighttpd/cgit/b1000000: Invalid \
 ||>|  argument (22)
 ||> ...
 ||>||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
 ||>||want to rebuild CGit without HAVE_LINUX_SENDFILE and see if that works.
 ||>
 ||>So i build it with
 | ...
 ||>  -       CGIT_CFLAGS += -DHAVE_LINUX_SENDFILE
 ||>  +       #CGIT_CFLAGS += -DHAVE_LINUX_SENDFILE
 | ...
 ||>and .. it works.
 ||>
 ||>Thank you, i will open an AlpineLinux bug report.  And lighttpd.
 | ...
 |
 ||I would like to propose an alternative and more portable solution:
 ||
 ||cgit cache should fallback to lseek, xread, xwrite if sendfile fails.
 |
 |(Yes.)
 ....

  diff --git a/cache.c b/cache.c
  index 2c70be7..7fbab66 100644
  --- a/cache.c
  +++ b/cache.c
  @@ -85,6 +85,7 @@ static int close_slot(struct cache_slot *slot)
   /* Print the content of the active cache slot (but skip the key). */
   static int print_slot(struct cache_slot *slot)
   {
  +       ssize_t i, j;
   #ifdef HAVE_LINUX_SENDFILE
          off_t start_off;
          int ret;
  @@ -97,12 +98,13 @@ static int print_slot(struct cache_slot *slot)
                  if (ret < 0) {
                          if (errno == EAGAIN || errno == EINTR)
                                  continue;
  +                       if (start_off == slot->keylen + 1)
  +                               break;

I can confirm this hunk fixes the bug.
If i remove it i get the error, with it you are fine.

                          return errno;
                  }
                  return 0;
          } while (1);
  -#else
  -       ssize_t i, j;
  +#endif
  
          i = lseek(slot->cache_fd, slot->keylen + 1, SEEK_SET);
          if (i != slot->keylen + 1)
  @@ -118,7 +120,6 @@ static int print_slot(struct cache_slot *slot)
                  return errno;
          else
                  return 0;
  -#endif
   }
  
   /* Check if the slot has expired */
  
Thanks :), and ciao from Germany,

--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)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
  2020-12-22  6:12 gs-cgit-lists.zx2c4.com
@ 2020-12-22 13:55 ` Steffen Nurpmeso
  2020-12-22 15:09   ` Steffen Nurpmeso
  0 siblings, 1 reply; 14+ messages in thread
From: Steffen Nurpmeso @ 2020-12-22 13:55 UTC (permalink / raw)
  To: gs-cgit-lists.zx2c4.com; +Cc: cgit

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
 |>||> 
 |>||>   <div class='error'>Error processing page: Invalid argument (22)</div\
 |>||>   >
 |> ...
 |>||> 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:
 |>|
 |>|  # ll /var/lib/lighttpd/cgit/b1000000
 |>|  -rw-------    1 lighttpd lighttpd     23417 Dec 21 20:22 /var/lib/light\
 |>|  tpd/cgit/b1000000
 |> ...
 |>
 |>Slightly resorted:
 |>
 |> ...
 |>||Have you looked at the log output?
 |>|  [cgit] error printing cache /var/lib/lighttpd/cgit/b1000000: Invalid \
 |>|  argument (22)
 |> ...
 |>||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
 |>||want to rebuild CGit without HAVE_LINUX_SENDFILE and see if that works.
 |>
 |>So i build it with
 ...
 |>  -       CGIT_CFLAGS += -DHAVE_LINUX_SENDFILE
 |>  +       #CGIT_CFLAGS += -DHAVE_LINUX_SENDFILE
 ...
 |>and .. it works.
 |>
 |>Thank you, i will open an AlpineLinux bug report.  And lighttpd.
 ...

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

(Yes.)

 |Even if the kernel supports sendfile() and cgit is built with
 |HAVE_LINUX_SENDFILE defined, certain filesystem types might not support
 |sendfile() operations.

This is ext3 and the overall system has not changed for years (but
following AlpineLinux [edge]).

 |The following patch falls back to lseek, xread, xwrite if the *initial*
 |call to sendfile() fails.

I will use it this afternoon, and do some more testing on the
descriptor (fstat, i don't know) because gstrauss (lighttpd
maintainer) is "driving up the walls" because he cannot reproduce
the issue it seems.  He says nothing changed regarding the
descriptor handling, mysterious, maybe a kernel bug, 5.10.1 it is
at the moment.

 |diff --git a/cache.c b/cache.c
 ...

Thanks, and
Ciao,

--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)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery
@ 2020-12-22  6:12 gs-cgit-lists.zx2c4.com
  2020-12-22 13:55 ` Steffen Nurpmeso
  0 siblings, 1 reply; 14+ messages in thread
From: gs-cgit-lists.zx2c4.com @ 2020-12-22  6:12 UTC (permalink / raw)
  To: cgit

>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
> ||> 
> ||>   <div class='error'>Error processing page: Invalid argument (22)</div>
> ...
> ||> 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:
> |
> |  # ll /var/lib/lighttpd/cgit/b1000000
> |  -rw-------    1 lighttpd lighttpd     23417 Dec 21 20:22 /var/lib/lighttpd/cgit/b1000000
> ...
>
>Slightly resorted:
>
> ...
> ||Have you looked at the log output?
> |  [cgit] error printing cache /var/lib/lighttpd/cgit/b1000000: Invalid argument (22)
> ...
> ||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
> ||want to rebuild CGit without HAVE_LINUX_SENDFILE and see if that works.
>
>So i build it with
>
>  #alp-2020:$ diff cgit.mk.orig cgit.mk
>  --- cgit.mk.orig
>  +++ cgit.mk
>  @@ -64,7 +64,7 @@
>   endif
>
>   ifdef HAVE_LINUX_SENDFILE
>  -       CGIT_CFLAGS += -DHAVE_LINUX_SENDFILE
>  +       #CGIT_CFLAGS += -DHAVE_LINUX_SENDFILE
>   endif
>
>   CGIT_OBJ_NAMES += cgit.o
>
>and
>
>  make NO_LUA=y NO_ICONV=y NO_GETTEXT=y NO_TCLTK=y NO_PERL=1 \
>    NO_PYTHON=1 NO_SVN_TESTS=y NO_REGEX=NeedsStartEnd prefix=/usr
>
>and .. it works.
>
>Thank you, i will open an AlpineLinux bug report.  And lighttpd.
>
>--steffen

I would like to propose an alternative and more portable solution:

cgit cache should fallback to lseek, xread, xwrite if sendfile fails.

Even if the kernel supports sendfile() and cgit is built with
HAVE_LINUX_SENDFILE defined, certain filesystem types might not support
sendfile() operations.

The following patch falls back to lseek, xread, xwrite if the *initial*
call to sendfile() fails.

diff --git a/cache.c b/cache.c
index 2c70be7..7fbab66 100644
--- a/cache.c
+++ b/cache.c
@@ -85,6 +85,7 @@ static int close_slot(struct cache_slot *slot)
 /* Print the content of the active cache slot (but skip the key). */
 static int print_slot(struct cache_slot *slot)
 {
+       ssize_t i, j;
 #ifdef HAVE_LINUX_SENDFILE
        off_t start_off;
        int ret;
@@ -97,12 +98,13 @@ static int print_slot(struct cache_slot *slot)
                if (ret < 0) {
                        if (errno == EAGAIN || errno == EINTR)
                                continue;
+                       if (start_off == slot->keylen + 1)
+                               break;
                        return errno;
                }
                return 0;
        } while (1);
-#else
-       ssize_t i, j;
+#endif
 
        i = lseek(slot->cache_fd, slot->keylen + 1, SEEK_SET);
        if (i != slot->keylen + 1)
@@ -118,7 +120,6 @@ static int print_slot(struct cache_slot *slot)
                return errno;
        else
                return 0;
-#endif
 }
 
 /* Check if the slot has expired */

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-01-15 22:41 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-21 16:26 cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery 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
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
2020-12-29 17:04       ` Steffen Nurpmeso

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).