Re: [git pull] vfs fix

[Linus Torvalds]

On Sat, Nov 26, 2016 at 5:13 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> Al Viro (1):
>       fix default_file_splice_read()

Ugh. I absolutely _hate_ this:

        BUG_ON(dummy);

because it makes no sense.

I'm assuming that "dummy" here is "start_offset", and that you want to
make sure that there are no initial offsets that would affect the
nrpages calculation.

But dammit, if so, just *call* it "start_offset", not "dummy". A dummy
value is just a place-holder, it makes no sense to have BUG_ON() on
such a value.

So adding random BUG_ON() statements is evil to begin with, but when
you do it on something that is mis-named and makes no sense, that's
just wrong.

I'm further assuming that the reason we can do that is because
"iov_iter_pipe()" has set iov_offset to zero, and as a result we end
up havin g

  iov_iter_get_pages_alloc() ->
    pipe_get_pages_alloc() ->
      data_start() will set *offp to zero.

but quite frankly, you can not tell that from the code itself, which
makes no sense. You have to go digging.

I was hoping the splice code would become more readable, not filled
with more crazy nonsensical code.

So I've pulled this, but _please_:

 - rename "dummy" (which isn't dummy at all now that you *do* things
to it!) to something sane.

   Like perhaps 'pg_offset' or 'iter_offset' or something.

 - Does the "BUG_ON()" really make sense? If the issue is that you
didn't use the offset in calculations, maybe you should just do so, ie
instead of

        BUG_ON(dummy);
        nr_pages = DIV_ROUND_UP(res, PAGE_SIZE);

   just do

        nr_pages = DIV_ROUND_UP(res + iter_offset, PAGE_SIZE);

or something? Even if "iter_offset" ends up always being zero, why is
that worthy of a BUG_ON()? The BUG_ON() is more expensive than just
doing the natural math..

That's what all the other users do, and that's what should be the
"right usage pattern", afaik. The number of pages really *is*
calculated as

       int n = DIV_ROUND_UP(result + offs, PAGE_SIZE);

in other iov_iter_get_pages_alloc() callers, although tghe nfs code
open-codes it as

        npages = (result + pgbase + PAGE_SIZE - 1) / PAGE_SIZE;

so it's not a very strong pattern.

                Linus