Messages in this thread | | | Date | Sat, 20 Oct 2018 05:56:08 +0100 | From | Al Viro <> | Subject | Re: [PATCH 01/24] iov_iter: Separate type from direction and use accessor functions |
| |
On Sat, Oct 20, 2018 at 02:10:44AM +0100, David Howells wrote:
One general comment: I would strongly recommend splitting the iov_iter initializers change into a separate patch.
> index 8d41ca7bfcf1..dcdbcb6f09f8 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -2990,7 +2990,7 @@ cifs_readdata_to_iov(struct cifs_readdata *rdata, struct iov_iter *iter) > size_t copy = min_t(size_t, remaining, PAGE_SIZE); > size_t written; > > - if (unlikely(iter->type & ITER_PIPE)) { > + if (unlikely(iov_iter_is_pipe(iter))) { > 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...
> @@ -786,7 +786,7 @@ setup_aio_ctx_iter(struct cifs_aio_ctx *ctx, struct iov_iter *iter, int rw) > struct page **pages = NULL; > struct bio_vec *bv = NULL; > > - if (iter->type & ITER_KVEC) { > + if (iov_iter_is_kvec(iter)) { > memcpy(&ctx->iter, iter, sizeof(struct iov_iter)); > ctx->len = count; > iov_iter_advance(iter, count);
... and so, to much greater extent, is this.
> @@ -2054,14 +2054,22 @@ int smbd_recv(struct smbd_connection *info, struct msghdr *msg)
> + switch (iov_iter_type(&msg->msg_iter)) { > + case ITER_KVEC: > buf = msg->msg_iter.kvec->iov_base; > to_read = msg->msg_iter.kvec->iov_len; > rc = smbd_recv_buf(info, buf, to_read); > break; > > - case READ | ITER_BVEC: > + case ITER_BVEC: > page = msg->msg_iter.bvec->bv_page; > page_offset = msg->msg_iter.bvec->bv_offset; > to_read = msg->msg_iter.bvec->bv_len;
Incidentally, this is bollocks - looks like a fallout of RDMA patches of some sort, but AFAICS there's no reason have separate bvec and kvec paths there - smbd_recv_buf() can bloody well use copy_to_iter(), eliminating the need for kmap_atomic, sleep avoidance, etc. As well as this branching on iterator flavour... Anyway, not your headache.
> @@ -1313,7 +1313,7 @@ do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode, > spin_lock_init(&dio->bio_lock); > dio->refcount = 1; > > - 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.
> @@ -1795,7 +1795,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, > if (pos >= dio->i_size) > goto out_free_dio; > > - if (iter->type == ITER_IOVEC) > + if (iter_is_iovec(iter)) > dio->flags |= IOMAP_DIO_DIRTY;
Ditto.
> @@ -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?
> @@ -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...?
| |