[Toybox] [PATCH] More use of sendfile.

[enh]

On Mon, Mar 16, 2020 at 5:50 AM Rob Landley <rob at landley.net> wrote:
>
> On 3/15/20 12:44 AM, enh via Toybox wrote:
> > This patch started by removing the CAT_V config option and using FLAG(),
> > and switching to xsendfile() for the no-options fast path.
> >
> > This pointed out that the 'x' in the xsendfile family was significantly
>
> Hang on, I need a beverage.
>
> The patch touches 8 files. Right....
>
> > weaker than the usual guarantee, silently ignoring a variety of errors.
> > It also made me realize that xsendfile() wasn't actually utilizing
> > sendfile(2).
>
> Has it be 7 years already?
>
>   https://lkml.org/lkml/2013/9/17/25
>
> Hmmm, 2.6.33:
>
>   http://man7.org/linux/man-pages/man2/sendfile.2.html
>
> Yes it has. Ok.
>
> > This patch removes sendfile_len (without the 'x') which only had one
> > user that wasn't checking for failure and looks like it should have used
> > the 'x' form instead.
>
> The idea was that httpd shouldn't exit if one transaction fails, cp shouldn't
> exit if one file it's copying fails... These days I've added a WARN_ONLY flag to
> the xopen() flag set, but that doesn't help for things that don't take open flags.
>
> > It also changes xsendfile_len() to take a boolean
> > argument to specify whether you want exactly that number of bytes (in
> > tar, say) or up to that many bytes (as in head, which this patch also
> > switches over to xsendfile).
>
> It's the middle of the night so I should not be reviewing this, but a quick
> first pass:
>
> I tend to do wrappers that eliminate unnecessary arguments, but you have exactly
> one user of each codepath. Sigh.
>
> Long english error message, "sent %lld (of %lld) bytes" is less of a lift for
> non-english speakers?

it's information that you otherwise can't access, and it's basically
what coreutils tar outputs.

> Moving the size = down later accomplishes what?

nothing much. just means we don't waste time initializing size if
we're on the fast path where we won't use it, since i was in here in
the first place trying to work out why our cat was slower than
everyone else's. but, yeah, the switch to actually using sendfile(2)
is the order of magnitude change.

> Sigh, this patch is completely unreadable. (This is why I want brahm cohen's
> algorithm for toys/pending.diff.c)
>
> > All the cat, head, and tar tests pass (the cat test needed a minor
> > modification now the error message is slightly different,
>
> This is why I don't test error messages. (I wonder what Elie De Brauwer is up to
> these days? Google says working at "Dermascan"...)

aye, but it was important that we did test an error case, or we'd
never have seen that xsendfile() was happy to just silently swallow
ENOSPC on write and call that a success...

> > but the new
> > version of the test also passes with coreutils cat where the old one
> > didn't).
> >
> > cat and head -c are significantly faster than they were, and are now
> > faster than coreutils. (coreutils doesn't use sendfile(2), it seems,
> > but was faster by virtue of using a 128KiB buffer than the 4KiB
> > toybuf/libbuf).
>
> Lemme look at this again when I'm more awake. It does multiple things...

yeah, if you're still struggling by the weekend, i'll break it up. it
started simply, snowballed, and then i felt like the context was
helpful to understand the motivation.

a timeline might help:

1. it started with a very small change to have the simple case in
cat(1) just call xsendfile.
2. though i switched over to FLAG() while i was there
3. and i applied your "you shouldn't have to know how toybox was
compiled to know what options it has" from your recent patch to cp, to
cat.

4. but then it turned out that (a) xsendfile actually ignores a
surprising number of errors (given that the usual meaning of 'x' is
"exit on error"), which worries me greatly for all the existing
callers,
5. and (b) it was never actually calling sendfile(2).

6. trying to understand the existing  toybox sendfile functions, i
realized that tar was ignoring errors from the non-x sendfile.

7. i wanted to move `head -c` over to xsendfile too, as a second
opinion that the new API works for the most obvious remaining case
that sometimes just boils down to a "copy this fd to that fd".

> Rob