[Toybox] [PATCH] More use of sendfile.

[Rob Landley]

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.

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

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

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

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

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

> 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