Message-ID: <YylJU+BKw5R8u7dw@ZenIV>
Date:   Tue, 20 Sep 2022 06:02:11 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Jan Kara <jack@...e.cz>
Cc:     Christoph Hellwig <hch@...radead.org>,
        John Hubbard <jhubbard@...dia.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jens Axboe <axboe@...nel.dk>,
        Miklos Szeredi <miklos@...redi.hu>,
        "Darrick J . Wong" <djwong@...nel.org>,
        Trond Myklebust <trond.myklebust@...merspace.com>,
        Anna Schumaker <anna@...nel.org>,
        David Hildenbrand <david@...hat.com>,
        Logan Gunthorpe <logang@...tatee.com>,
        linux-block@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-xfs@...r.kernel.org, linux-nfs@...r.kernel.org,
        linux-mm@...ck.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 4/7] iov_iter: new iov_iter_pin_pages*() routines

On Fri, Sep 16, 2022 at 02:55:53AM +0100, Al Viro wrote:
> 	* READ vs. WRITE turned out to be an awful way to specify iov_iter
> data direction.  Local iov_iter branch so far:
> 	get rid of unlikely() on page_copy_sane() calls
> 	csum_and_copy_to_iter(): handle ITER_DISCARD
> 	[s390] copy_oldmem_kernel() - WRITE is "data source", not destination
> 	[fsi] WRITE is "data source", not destination...
> 	[infiniband] READ is "data destination", not source...
> 	[s390] zcore: WRITE is "data source", not destination...
> 	[target] fix iov_iter_bvec() "direction" argument
> 	[vhost] fix 'direction' argument of iov_iter_{init,bvec}()
> 	[xen] fix "direction" argument of iov_iter_kvec()
> 	[trace] READ means "data destination", not source...
> 	iov_iter: saner checks for attempt to copy to/from iterator
> 	use less confusing names for iov_iter direction initializers
> those 8 commits in the middle consist of fixes, some of them with more than
> one call site affected.  Folks keep going "oh, we are going to copy data
> into that iterator, must be WRITE".  Wrong - WRITE means "as for write(2)",
> i.e. the data _source_, not data destination.  And the same kind of bugs
> goes in the opposite direction, of course.
> 	I think something like ITER_DEST vs. ITER_SOURCE would be less
> confusing.
> 
> 	* anything that goes with ITER_SOURCE doesn't need pin.
> 	* ITER_IOVEC/ITER_UBUF need pin for get_pages and for nothing else.
> Need to grab reference on get_pages, obviously.
> 	* even more obviously, ITER_DISCARD is irrelevant here.
> 	* ITER_PIPE only modifies anonymous pages that had been allocated
> by iov_iter primitives and hadn't been observed by anything outside until
> we are done with said ITER_PIPE.
> 	* quite a few instances are similar to e.g. REQ_OP_READ handling in
> /dev/loop - we work with ITER_BVEC there and we do modify the page contents,
> but the damn thing would better be given to us locked and stay locked until
> all involved modifications (be it real IO/decoding/whatever) is complete.
> That ought to be safe, unless I'm missing something.
> 
> That doesn't cover everything; still going through the list...

More:

nvme target: nvme read requests end up with somebody allocating and filling
sglist, followed by reading from file into it (using ITER_BVEC).  Then the
pages are sent out, presumably.  I would be very surprised if it turned out
to be anything other than anon pages allocated by the driver, but I'd like
to see that confirmed by nvme folks.  Probably doesn't need pinning.

->read_folio() instances - page locked by caller, not unlocked until we are done.

->readahead() instances - pages are in the segment of page cache that had been
populated and locked by the caller; some are ITER_BVEC (with page(s) extracted
by readahead_page()), some - ITER_XARRAY.
other similar places (some of ->write_begin() instances, after having grabbed
a locked page, etc.)

->issue_read() instances - the call graph is scary (in particular, recursion
prevention there is non-obvious), but unless netfs folks say otherwise, I'd
assume that all pages involved are supposed to be locked by the caller.
swap reads (ending up at __swap_read_unplug()) - pages locked by callers.

in some cases (cifs) pages are privately allocated and not visible to anyone
else.

io_import_fixed() sets ITER_BVEC over pinned pages; see io_pin_pages() for
the place where that's done.

In cifs_send_async_read() we take the pages that will eventually go into
ITER_BVEC iterator from iov_iter_get_pages() - that one wants pinning if
the type of ctx->iter would demand so.  The same goes for setup_aio_ctx_iter() -
iov_iter_get_pages() is used to make an ITER_BVEC counterpart of the
iov_iter passed to ->read_iter(), with the same considerations re pinning.
The same goes for ceph __iter_get_bvecs().

Haven't done yet:

drivers/target/target_core_file.c:292:  iov_iter_bvec(&iter, is_write, aio_cmd->bvecs, sgl_nents, len);
drivers/vhost/vringh.c:1198:            iov_iter_bvec(&iter, ITER_DEST, iov, ret, translated);
fs/afs/dir.c:308:       iov_iter_xarray(&req->def_iter, ITER_DEST, &dvnode->netfs.inode.i_mapping->i_pages,
net/ceph/messenger_v1.c:52:     iov_iter_bvec(&msg.msg_iter, ITER_DEST, &bvec, 1, length);
net/ceph/messenger_v2.c:236:    iov_iter_bvec(&con->v2.in_iter, ITER_DEST, &con->v2.in_bvec, 1, bv->bv_len);
net/sunrpc/svcsock.c:263:       iov_iter_bvec(&msg.msg_iter, ITER_DEST, bvec, i, buflen);
net/sunrpc/xprtsock.c:376:      iov_iter_bvec(&msg->msg_iter, ITER_DEST, bvec, nr, count);

The picture so far looks like we mostly need to take care of pinning when
we obtain the references from iov_iter_get_pages().  What's more, it looks
like ITER_BVEC/ITER_XARRAY/ITER_PIPE we really don't need to pin anything on
get_pages/pin_pages - they are already protected (or, in case of ITER_PIPE,
allocated by iov_iter itself and not reachable by anybody outside).
Might or might not be true for the remaining 7 call sites...

NOTE: all of the above assumes that callers with pre-locked pages are
either synchronous or do not unlock until the completion callbacks.
It does appear to be true; if it is true, I really wonder if we need
to even grab references in iov_iter_pin_pages() for anything other
than ITER_IOVEC/ITER_UBUF.  The right primitive might be
	if user-backed
		pin pages
	else
		just copy the pointers; any lifetime-related issues are
		up to the caller.
	advance iterator in either case

Powered by blists - more mailing lists