From: Al Viro <viro@ZenIV.linux.org.uk>
To: David Howells <dhowells@redhat.com>
Cc: linux-afs@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 01/24] iov_iter: Separate type from direction and use accessor functions
Date: Sat, 20 Oct 2018 05:56:08 +0100 [thread overview]
Message-ID: <20181020045608.GH32577@ZenIV.linux.org.uk> (raw)
In-Reply-To: <153999784492.866.12128875514532051040.stgit@warthog.procyon.org.uk>
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...?
next prev parent reply other threads:[~2018-10-20 4:56 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-20 1:10 [PATCH 00/24] AFS development David Howells
2018-10-20 1:10 ` [PATCH 01/24] iov_iter: Separate type from direction and use accessor functions David Howells
2018-10-20 4:56 ` Al Viro [this message]
2018-10-22 13:00 ` David Howells
2018-10-20 1:10 ` [PATCH 02/24] iov_iter: Renumber the ITER_* constants in uio.h David Howells
2018-10-20 4:59 ` Al Viro
2018-10-22 15:54 ` David Howells
2018-10-23 13:20 ` David Howells
2018-10-20 1:10 ` [PATCH 03/24] iov_iter: Add I/O discard iterator David Howells
2018-10-20 5:05 ` Al Viro
2018-10-22 16:18 ` David Howells
2018-10-20 1:11 ` [PATCH 04/24] afs: Better tracing of protocol errors David Howells
2018-10-20 1:11 ` [PATCH 05/24] afs: Set up the iov_iter before calling afs_extract_data() David Howells
2018-10-20 1:11 ` [PATCH 06/24] afs: Improve FS server rotation error handling David Howells
2018-10-20 1:11 ` [PATCH 07/24] afs: Implement VL server rotation David Howells
2018-10-20 1:11 ` [PATCH 08/24] afs: Fix TTL on VL server and address lists David Howells
2018-10-20 1:11 ` [PATCH 09/24] afs: Handle EIO from delivery function David Howells
2018-10-20 1:11 ` [PATCH 10/24] afs: Add a couple of tracepoints to log I/O errors David Howells
2018-10-20 1:11 ` [PATCH 11/24] afs: Don't invoke the server to read data beyond EOF David Howells
2018-10-20 1:12 ` [PATCH 12/24] afs: Increase to 64-bit volume ID and 96-bit vnode ID for YFS David Howells
2018-10-20 1:12 ` [PATCH 13/24] afs: Commit the status on a new file/dir/symlink David Howells
2018-10-20 1:12 ` [PATCH 14/24] afs: Remove callback details from afs_callback_break struct David Howells
2018-10-20 1:12 ` [PATCH 15/24] afs: Implement the YFS cache manager service David Howells
2018-10-20 1:12 ` [PATCH 16/24] afs: Fix FS.FetchStatus delivery from updating wrong vnode David Howells
2018-10-20 1:12 ` [PATCH 17/24] afs: Calc callback expiry in op reply delivery David Howells
2018-10-20 1:12 ` [PATCH 18/24] afs: Get the target vnode in afs_rmdir() and get a callback on it David Howells
2018-10-20 1:12 ` [PATCH 19/24] afs: Expand data structure fields to support YFS David Howells
2018-10-20 1:13 ` [PATCH 20/24] afs: Implement YFS support in the fs client David Howells
2018-10-20 1:13 ` [PATCH 21/24] afs: Allow dumping of server cursor on operation failure David Howells
2018-10-20 1:13 ` [PATCH 22/24] afs: Eliminate the address pointer from the address list cursor David Howells
2018-10-20 1:13 ` [PATCH 23/24] afs: Fix callback handling David Howells
2018-10-20 1:13 ` [PATCH 24/24] afs: Probe multiple fileservers simultaneously David Howells
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181020045608.GH32577@ZenIV.linux.org.uk \
--to=viro@zeniv.linux.org.uk \
--cc=dhowells@redhat.com \
--cc=linux-afs@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).