Re: [RFC][PATCHES] iov_iter stuff

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, Jun 17, 2022 at 11:48:01PM +0100, Al Viro wrote:
> On Fri, Jun 17, 2022 at 04:30:49PM -0600, Jens Axboe wrote:
> 
> > Al, looks good to me from inspection, and I ported stuffed this on top
> > of -git and my 5.20 branch, and did my send/recv/recvmsg io_uring change
> > on top and see a noticeable reduction there too for some benchmarking.
> > Feel free to add:
> > 
> > Reviewed-by: Jens Axboe <axboe@kernel.dk>
> > 
> > to the series.
> > 
> > Side note - of my initial series I played with, I still have this one
> > leftover that I do utilize for io_uring:
> > 
> > https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.20/io_uring-iter&id=a59f5c21a6eeb9506163c20aff4846dbec159f47
> > 
> > Doesn't make sense standalone, but I have it as a prep patch.
> > 
> > Can I consider your work.iov_iter stable at this point, or are you still
> > planning rebasing?
> 
> Umm...  Rebasing this part - probably no; there's a fun followup to it, though,
> I'm finishing the carve up & reorder at the moment.  Will post for review
> tonight...

	This stuff sits on top of #work.iov_iter (as posted a week ago) +
#fixes (one commit, handling of failures halfway through copy_mc_to_iter()
into ITER_PIPE, posted several days ago, backportable minimal fix) +
#work.9p (handling of RERROR on zerocopy 9P read/readdir, posted about
a week ago).  The branch is #work.iov_iter_get_pages; individual patches
in followups.

	NOTE: the older branches are unchanged, but this series on top of
them had been repeatedly carved up, reordered, etc. - there had been a lot
of recent massage, so at this point it should be treated as absolutely
untested.  It can shit over memory and/or chew your filesystems; DON'T
TRY IT OUTSIDE OF A SCRATCH KVM IMAGE.  Said that, review and (cautious)
testing would be very welcome.

	Part 1: ITER_PIPE cleanups

ITER_PIPE handling had never been pretty, but by now it has become
really obfuscated and hard to read.  Untangle it a bit.

1) splice: stop abusing iov_iter_advance() to flush a pipe
	A really odd (ab)use of iov_iter_advance() - in case of error
generic_file_splice_read() wants to free all pipe buffers ->read_iter()
has produced.  Yes, forcibly resetting ->head and ->iov_offset to
original values and calling iov_iter_advance(i, 0), will trigger
pipe_advance(), which will trigger pipe_truncate(), which will free
buffers.  Or we could just go ahead and free the same buffers;
pipe_discard_from() does exactly that, no iov_iter stuff needs to
be involved.

2) ITER_PIPE: helper for getting pipe buffer by index
	In a lot of places we want to find pipe_buffer by index;
expression is convoluted and hard to read.  Provide an inline helper
for that, convert trivial open-coded cases.  Eventually *all*
open-coded instances in iov_iter.c will get converted.

3) ITER_PIPE: helpers for adding pipe buffers
	There are only two kinds of pipe_buffer in the area used by ITER_PIPE.
* anonymous - copy_to_iter() et.al. end up creating those and copying data
  there.  They have zero ->offset, and their ->ops points to
  default_pipe_page_ops.
* zero-copy ones - those come from copy_page_to_iter(), and page comes from
  caller.  ->offset is also caller-supplied - it might be non-zero.
  ->ops points to page_cache_pipe_buf_ops.
	Move creation and insertion of those into helpers -
push_anon(pipe, size) and push_page(pipe, page, offset, size) resp., separating
them from the "could we avoid creating a new buffer by merging with the current
head?" logics.

4) ITER_PIPE: allocate buffers as we go in copy-to-pipe primitives
	New helper: append_pipe().  Extends the last buffer if possible,
allocates a new one otherwise.  Returns page and offset in it on success,
NULL on failure.  iov_iter is advanced past the data we've got.
	Use that instead of push_pipe() in copy-to-pipe primitives;
they get simpler that way.  Handling of short copy (in "mc" one)
is done simply by iov_iter_revert() - iov_iter is in consistent
state after that one, so we can use that.

5) ITER_PIPE: fold push_pipe() into __pipe_get_pages()
	Expand the only remaining call of push_pipe() (in
__pipe_get_pages()), combine it with the page-collecting loop there.
We don't need to bother with i->count checks or calculation of offset
in the first page - the caller already has done that.
	Note that the only reason it's not a loop doing append_pipe()
is that append_pipe() is advancing, while iov_iter_get_pages() is not.
As soon as it switches to saner semantics, this thing will switch
to using append_pipe().

6) ITER_PIPE: lose iter_head argument of __pipe_get_pages()
	Always equal to pipe->head - 1.

7) ITER_PIPE: clean pipe_advance() up
	Don't bother with pipe_truncate(); adjust the buffer
length just as we decide it'll be the last one, then use
pipe_discard_from() to release buffers past that one.

8) ITER_PIPE: clean iov_iter_revert()
	Fold pipe_truncate() in there, clean the things up.

9) ITER_PIPE: cache the type of last buffer
	We often need to find whether the last buffer is anon or not, and
currently it's rather clumsy:
	check if ->iov_offset is non-zero (i.e. that pipe is not empty)
	if so, get the corresponding pipe_buffer and check its ->ops
	if it's &default_pipe_buf_ops, we have an anon buffer.
Let's replace the use of ->iov_offset (which is nowhere near similar to
its role for other flavours) with signed field (->last_offset), with
the following rules:
	empty, no buffers occupied:		0
	anon, with bytes up to N-1 filled:	N
	zero-copy, with bytes up to N-1 filled:	-N
That way abs(i->last_offset) is equal to what used to be in i->iov_offset
and empty vs. anon vs. zero-copy can be distinguished by the sign of
i->last_offset.
	Checks for "should we extend the last buffer or should we start
a new one?" become easier to follow that way.
	Note that most of the operations can only be done in a sane
state - i.e. when the pipe has nothing past the current position of
iterator.  About the only thing that could be done outside of that
state is iov_iter_advance(), which transitions to the sane state by
truncating the pipe.  There are only two cases where we leave the
sane state:
	1) iov_iter_get_pages()/iov_iter_get_pages_alloc().  Will be
dealt with later, when we make get_pages advancing - the callers are
actually happier that way.
	2) iov_iter copied, then something is put into the copy.  Since
they share the underlying pipe, the original gets behind.  When we
decide that we are done with the copy (original is not usable until then)
we advance the original.  direct_io used to be done that way; nowadays
it operates on the original and we do iov_iter_revert() to discard
the excessive data.  At the moment there's nothing in the kernel that
could do that to ITER_PIPE iterators, so this reason for insane state
is theoretical right now.

10) ITER_PIPE: fold data_start() and pipe_space_for_user() together
	All their callers are next to each other; all of them
want the total amount of pages and, possibly, the
offset in the partial final buffer.
	Combine into a new helper (pipe_npages()), fix the
bogosity in pipe_space_for_user(), while we are at it.

	Part 2: iov_iter_get_pages()/iov_iter_get_pages_alloc() unification

	There's a lot of duplication between iov_iter_get_pages() and
iov_iter_get_pages_alloc().  With some massage it can be eliminated,
along with some of the cruft accumulated there.

	Flavour-independent arguments validation and, for ..._alloc(),
cleanup handling on failure:
11) iov_iter_get_pages{,_alloc}(): cap the maxsize with LONG_MAX
12) iov_iter_get_pages_alloc(): lift freeing pages array on failure exits into wrapper
13) iov_iter_get_pages(): sanity-check arguments

	Mechanically merge parallel ..._get_pages() and ..._get_pages_alloc().
14) unify pipe_get_pages() and pipe_get_pages_alloc()
15) unify xarray_get_pages() and xarray_get_pages_alloc()
16) unify the rest of iov_iter_get_pages()/iov_iter_get_pages_alloc() guts

	Decrufting for XARRAY:
17) ITER_XARRAY: don't open-code DIV_ROUND_UP()
	Decrufting for iBUF/IOVEC/BVEC:
18) iov_iter: lift dealing with maxpages out of first_{iovec,bvec}_segment()
19) iov_iter: massage calling conventions for first_{iovec,bvec}_segment()
20) found_iovec_segment(): just return address
	Decrufting for PIPE:
21) fold __pipe_get_pages() into pipe_get_pages()

	Collapsing the bits that differ for get_pages and get_pages_alloc
cases into a common helper:
22) iov_iter: saner helper for page array allocation

	Part 3: making iov_iter_get_pages{,_alloc}() advancing

	Most of the callers follow successful ...get_pages... with advance
by the amount it had reported.  For some it's unconditional, for some it
might end up being less in some cases.  All of them would be fine with
advancing variants of those primitives - those that might want to advance
by less than reported could easily use revert by the difference of those
amounts.
	Rather than doing a flagday change (they are exported and signatures
remain unchanged), replacement variants are added (iov_iter_get_pages2()
and iov_iter_get_pages_alloc2(), initially as wrappers).  By the end of
the series everything is converted to those and the old ones are removed.

23) iov_iter: advancing variants of iov_iter_get_pages{,_alloc}()
24) block: convert to advancing variants of iov_iter_get_pages{,_alloc}()
25) iter_to_pipe(): switch to advancing variant of iov_iter_get_pages()
26) af_alg_make_sg(): switch to advancing variant of iov_iter_get_pages()
27) 9p: convert to advancing variant of iov_iter_get_pages_alloc()
28) ceph: switch the last caller of iov_iter_get_pages_alloc()
29) get rid of non-advancing variants

	Part 4: cleanups
30) pipe_get_pages(): switch to append_pipe()
31) expand those iov_iter_advance()...

Overall diffstat:

 arch/powerpc/include/asm/uaccess.h |   2 +-
 arch/s390/include/asm/uaccess.h    |   4 +-
 block/bio.c                        |  15 +-
 block/blk-map.c                    |   7 +-
 block/fops.c                       |   8 +-
 crypto/af_alg.c                    |   3 +-
 crypto/algif_hash.c                |   5 +-
 drivers/nvme/target/io-cmd-file.c  |   2 +-
 drivers/vhost/scsi.c               |   4 +-
 fs/aio.c                           |   2 +-
 fs/btrfs/file.c                    |  19 +-
 fs/btrfs/inode.c                   |   3 +-
 fs/ceph/addr.c                     |   2 +-
 fs/ceph/file.c                     |   5 +-
 fs/cifs/file.c                     |   8 +-
 fs/cifs/misc.c                     |   3 +-
 fs/direct-io.c                     |   7 +-
 fs/fcntl.c                         |   1 +
 fs/file_table.c                    |  17 +-
 fs/fuse/dev.c                      |   7 +-
 fs/fuse/file.c                     |   7 +-
 fs/gfs2/file.c                     |   2 +-
 fs/io_uring.c                      |   2 +-
 fs/iomap/direct-io.c               |  21 +-
 fs/nfs/direct.c                    |   8 +-
 fs/open.c                          |   1 +
 fs/read_write.c                    |   6 +-
 fs/splice.c                        |  54 +-
 fs/zonefs/super.c                  |   2 +-
 include/linux/fs.h                 |  21 +-
 include/linux/iomap.h              |   6 +
 include/linux/pipe_fs_i.h          |  29 +-
 include/linux/uaccess.h            |   4 +-
 include/linux/uio.h                |  50 +-
 lib/iov_iter.c                     | 978 ++++++++++++++-----------------------
 mm/shmem.c                         |   2 +-
 net/9p/client.c                    | 125 +----
 net/9p/protocol.c                  |   3 +-
 net/9p/trans_virtio.c              |  37 +-
 net/core/datagram.c                |   3 +-
 net/core/skmsg.c                   |   3 +-
 net/rds/message.c                  |   3 +-
 net/tls/tls_sw.c                   |   4 +-
 43 files changed, 589 insertions(+), 906 deletions(-)