Re: [PATCH v2 4/7] iov_iter: new iov_iter_pin_pages*() routines

From: Al Viro <viro@zeniv.linux.org.uk>
To: Christoph Hellwig <hch@infradead.org>
Cc: Jan Kara <jack@suse.cz>, John Hubbard <jhubbard@nvidia.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Jens Axboe <axboe@kernel.dk>, Miklos Szeredi <miklos@szeredi.hu>,
	"Darrick J . Wong" <djwong@kernel.org>,
	Trond Myklebust <trond.myklebust@hammerspace.com>,
	Anna Schumaker <anna@kernel.org>,
	David Hildenbrand <david@redhat.com>,
	Logan Gunthorpe <logang@deltatee.com>,
	linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-xfs@vger.kernel.org, linux-nfs@vger.kernel.org,
	linux-mm@kvack.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 4/7] iov_iter: new iov_iter_pin_pages*() routines
Date: Fri, 23 Sep 2022 05:22:17 +0100	[thread overview]
Message-ID: <Yy00eSjyxvUIp7D5@ZenIV> (raw)
In-Reply-To: <YyxzYTlyGhbb2MOu@infradead.org>

On Thu, Sep 22, 2022 at 07:38:25AM -0700, Christoph Hellwig wrote:
> On Thu, Sep 22, 2022 at 03:22:48AM +0100, Al Viro wrote:
> > What I'd like to have is the understanding of the places where we drop
> > the references acquired by iov_iter_get_pages().  How do we decide
> > whether to unpin?
> 
> Add a iov_iter_unpin_pages that does the right thing based on the
> type.  (block will need a modified copy of it as it doesn't keep
> the pages array around, but logic will be the same).

Huh?  You want to keep the type (+ direction) of iov_iter in any structure
a page reference coming from iov_iter_get_pages might end up in?  IDGI...

BTW, speaking of lifetime rules - am I right assuming that fd_execute_rw()
does IO on pages of the scatterlist passed to it?  Where are they getting
dropped and what guarantees that IO is complete by that point?

The reason I'm asking is that here you have an ITER_BVEC possibly fed to
__blkdev_direct_IO_async(), with its
        if (iov_iter_is_bvec(iter)) {
                /*
                 * Users don't rely on the iterator being in any particular
                 * state for async I/O returning -EIOCBQUEUED, hence we can
                 * avoid expensive iov_iter_advance(). Bypass
                 * bio_iov_iter_get_pages() and set the bvec directly.
                 */
                bio_iov_bvec_set(bio, iter);
which does *not* grab the page referneces.  Sure, bio_release_pages() knows
to leave those alone and doesn't drop anything.  However, what is the
mechanism preventing the pages getting freed before the IO completion
in this case?