Re: [PATCH v7 0/8] iov_iter: Improve page extraction (ref, pin or just list)

From: Matthew Wilcox <willy@infradead.org>
To: Jan Kara <jack@suse.cz>
Cc: David Howells <dhowells@redhat.com>,
	John Hubbard <jhubbard@nvidia.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Christoph Hellwig <hch@infradead.org>,
	Jens Axboe <axboe@kernel.dk>, Jeff Layton <jlayton@kernel.org>,
	Logan Gunthorpe <logang@deltatee.com>,
	linux-fsdevel@vger.kernel.org, linux-block@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 0/8] iov_iter: Improve page extraction (ref, pin or just list)
Date: Mon, 23 Jan 2023 17:33:24 +0000	[thread overview]
Message-ID: <Y87E5HAo7ZoHyrbE@casper.infradead.org> (raw)
In-Reply-To: <20230123164218.qaqqg3ggbymtlwjx@quack3>

On Mon, Jan 23, 2023 at 05:42:18PM +0100, Jan Kara wrote:
> On Mon 23-01-23 16:31:32, Matthew Wilcox wrote:
> > On Fri, Jan 20, 2023 at 05:55:48PM +0000, David Howells wrote:
> > >  (3) Make the bio struct carry a pair of flags to indicate the cleanup
> > >      mode.  BIO_NO_PAGE_REF is replaced with BIO_PAGE_REFFED (equivalent to
> > >      FOLL_GET) and BIO_PAGE_PINNED (equivalent to BIO_PAGE_PINNED) is
> > >      added.
> > 
> > I think there's a simpler solution than all of this.
> > 
> > As I understand the fundamental problem here, the question is
> > when to copy a page on fork.  We have the optimisation of COW, but
> > O_DIRECT/RDMA/... breaks it.  So all this page pinning is to indicate
> > to the fork code "You can't do COW to this page".
> > 
> > Why do we want to track that information on a per-page basis?  Wouldn't it
> > be easier to have a VM_NOCOW flag in vma->vm_flags?  Set it the first
> > time somebody does an O_DIRECT read or RDMA pin.  That's it.  Pages in
> > that VMA will now never be COWed, regardless of their refcount/mapcount.
> > And the whole "did we pin or get this page" problem goes away.  Along
> > with folio->pincount.
> 
> Well, but anon COW code is not the only (planned) consumer of the pincount.
> Filesystems also need to know whether a (shared pagecache) page is pinned
> and can thus be modified behind their backs. And for that VMA tracking
> isn't really an option.

Bleh, I'd forgotten about that problem.  We really do need to keep
track of which pages are under I/O for this case, because we need to
tell the filesystem that they are now available for writeback.

That said, I don't know that we need to keep track of it in the
pages themselves.  Can't we have something similar to rmap which
keeps track of a range of pinned pages, and have it taken care of
at a higher level (ie unpin the pages in the dio_iodone_t rather
than in the BIO completion handler)?

I'm not even sure why pinned pagecache pages remain on the LRU.
They should probably go onto the unevictable list with the mlocked
pages, then on unpin get marked dirty and placed on the active list.
There's no point in writing back a pinned page since it can be
written to at any instant without any part of the kernel knowing.