[Toybox] [PATCH] More use of sendfile.

[enh]

On Tue, Mar 17, 2020 at 8:10 PM Rob Landley <rob at landley.net> wrote:
>
>
>
> On 3/17/20 6:41 PM, enh wrote:
> > 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.
>
> Yes, I was proposing the shorter phrasing as being possibly easier for
> non-english speakers.

ah, i see. because i sent the patch as an attachment rather than
inline, i didn't even notice that you'd changed that from what i'd
written, so i guess that's a implicit "lgtm" :-)

> >> 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.
>
> Sorry I've been slow on this, I was packing/stressing for my trip to tokyo,
> which got pushed back a month and a half at the last minute. (In part because I
> caught what is probably the flu. Coughing and short of breath, but no fever. So
> I'm still in the states, but not really programming on all cylinders just now...)

yeah, no worries. none of this is a regression. (fwiw, i've been
deliberately keeping my hexedit rebase to myself until after the
release, and my massive work-in-progress man is looking fairly decent
at this point, and i've still got the "extract readelf's guts into
lib/elf.c and then reuse it for file/size/nm too", and i have a pretty
decent strace [minus -f], but i'm trying to keep to smaller stuff for
now. i do miss my "fancy" hexedit every time i'm looking at an ELF or
zip file on the wrong machine though --- the others i can just use the
Debian tools!)

> >> 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...
>
> I remember sendfile not sending along error messages reliably, but I tested it
> years ago...

no, it's not sendfile(2) that's the issue --- it's that xsendfile()
had a bunch of special cases where it would ignore errors. in
particular, it never reports errors on stdout (!) which is why there's
a cat test that fails if i switch cat to xsendfile().

> >> 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.
>
> I don't object to anything it's doing, I just haven't got the brain to wrap my
> head around it at the moment.
>
> > 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.
>
> It's a good thing to do, yes.

(i could send that separately, but then we'd have to rebase all this,
and these are the least complicated parts of the change, so i'm
assuming it's more trouble than it's worth for now.)

> > 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,
>
> I made sendfile() library functions on the theory that they _would_ use actual
> sendfile at some point (possibly via portability.c), but every time I sat down
> and tried to make it do so I hit complications.
>
> Alas, I don't seem to have blogged about it, which is sad because I don't
> currently remember the majority of my reasoning. (See "sick", above.) Looks like
> I added it as part of writing patch in 2007 (commit bc07865a504c).
>
> I do know that tar and cp shouldn't exit early if you extract a file too long
> for a VFAT partition: it should still copy/extract the remaining files. I
> remember a redesign of this whole area being underway at some point and probably
> something that was lost like the various dd.c rewrites in a "got busy, got
> distracted, flushed the tree when I came back because it had been so long  it
> was less work to start over than reverse engineer my own work to figure out
> where I left off"...

isn't that the difference between the perror_exit and perror_msg
variants that i've effectively added? (that seemed both safer and more
intention-revealing that the bunch of heuristics for when to just
ignore errors.)

> That's why tar was using sendfile_len() instead of xsendfile_len(), because it's
> tar's job to notice that sent and len don't match. Which in _theory_ is why:
>
> -    len -= sendfile_len(TT.fd, fd, len, &sent);
> +    sent = xsendfile_len(TT.fd, fd, len, 1);
> +    len -= sent;
>      used += sent;
>
> was two different values, and if they didn't match
>
>        if (len+used>TT.hdr.size) error_exit("sparse overflow");
>
> was supposed to trigger? I think?
>
> Looks unfinished either way. Needs more test cases, as usual. (There are places
> I go to some effort to make the tarball tracking recover and continue in the
> face of other disk errors, this does not appear to be one of them. Sparse files
> were added kinda quickly at the last minute while I was juggling too many other
> balls...)

yeah, this was a place where GNU tar tells you that something unusual
happened but toybox was just quietly pretending everything was fine.
(there's a visible difference in terms of error messages with some of
the existing tests, and this patch seems to bring us closer to
coreutils.)

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