Al Viro <v...@zeniv.linux.org.uk> wrote:
> One general comment: I would strongly recommend splitting the iov_iter
> initializers change into a separate patch.
Done.
> > void *addr = kmap_atomic(page);
> >
> > written = copy_to_iter(addr, copy, iter);
>
> FWIW, I wonder if that one is actually a missing primitive getting
> open-coded...
Ummm... You mean the combination of kmap_atomic() and copy_to_iter()? Can I
leave these for another time?
> > memcpy(&ctx->iter, iter, sizeof(struct iov_iter));
> > ctx->len = count;
> > iov_iter_advance(iter, count);
>
> ... and so, to much greater extent, is this.
And combining memcpy() and iov_iter_advance()?
> > - dio->should_dirty = (iter->type == ITER_IOVEC);
> > + dio->should_dirty = iter_is_iovec(iter);
>
> Nope. This path *can* get both read and write iov_iter. Not an equivalent
> change.
This should really have been (iter->type == (ITER_IOVEC | READ)).
Fixed to:
dio->should_dirty = iter_is_iovec(iter) && iov_iter_rw(iter) == READ;
> > - if (iter->type == ITER_IOVEC)
> > + if (iter_is_iovec(iter))
> > dio->flags |= IOMAP_DIO_DIRTY;
>
> Ditto.
This also should've had "| READ" in there.
Fixed to:
if (iter_is_iovec(iter) && iov_iter_rw(iter) == READ)
dio->flags |= IOMAP_DIO_DIRTY;
> > @@ -417,28 +417,35 @@ int iov_iter_fault_in_readable(struct iov_iter *i,
> > size_t bytes)
> > int err;
> > struct iovec v;
> >
> > - if (!(i->type & (ITER_BVEC|ITER_KVEC))) {
> > + switch (iov_iter_type(i)) {
> > + case ITER_IOVEC:
> > + case ITER_PIPE:
> > iterate_iovec(i, bytes, v, iov, skip, ({
> > err = fault_in_pages_readable(v.iov_base, v.iov_len);
> > if (unlikely(err))
> > return err;
> > 0;}))
> > + break;
> > + case ITER_KVEC:
> > + case ITER_BVEC:
> > + break;
> > }
> > return 0;
> > }
> > EXPORT_SYMBOL(iov_iter_fault_in_readable);
>
> Huh? That makes no sense whatsoever - ITER_PIPE ones are write-only in the
> first place, so they won't be passed to that one, but feeding ITER_PIPE to
> iterate_iovec() is insane. And even if they copy-from ITER_PIPES would
> appear, why the devil would we want to fault-in anything?
Note that the condition "!(i->type & (ITER_BVEC|ITER_KVEC))" is true if type
is ITER_PIPE or ITER_IOVEC.
That said, I can make it BUG if it encounters a pipe iterator.
> > @@ -987,7 +1003,7 @@ void iov_iter_revert(struct iov_iter *i, size_t unroll)
> > return;
> > i->count += unroll;
> > - if (unlikely(i->type & ITER_PIPE)) {
> > + if (unlikely(iov_iter_is_pipe(i))) {
> > struct pipe_inode_info *pipe = i->pipe;
> ...
> > + case ITER_PIPE:
> > + BUG();
> > + }
> > }
> > EXPORT_SYMBOL(iov_iter_revert);
>
> Wha...?
It should never get to the BUG() because of the earlier if-statement.
However, the compiler gets picky sometimes, but isn't necessarily good enough
to see that this particular case should never occur, so to avoid getting a
warning about missing cases I put in a BUG. I can stick a comment on it and
make it just break.
David
