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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
