Re: [PATCH v1 00/15] Keep track of GUPed pages in fs and block

From: Boaz Harrosh <openosd@gmail.com>
To: Jerome Glisse <jglisse@redhat.com>, Boaz Harrosh <boaz@plexistor.com>
Cc: "Dan Williams" <dan.j.williams@intel.com>,
	"Kent Overstreet" <kent.overstreet@gmail.com>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	linux-block@vger.kernel.org, "Linux MM" <linux-mm@kvack.org>,
	"John Hubbard" <jhubbard@nvidia.com>, "Jan Kara" <jack@suse.cz>,
	"Alexander Viro" <viro@zeniv.linux.org.uk>,
	"Johannes Thumshirn" <jthumshirn@suse.de>,
	"Christoph Hellwig" <hch@lst.de>, "Jens Axboe" <axboe@kernel.dk>,
	"Ming Lei" <ming.lei@redhat.com>,
	"Jason Gunthorpe" <jgg@ziepe.ca>,
	"Matthew Wilcox" <willy@infradead.org>,
	"Steve French" <sfrench@samba.org>,
	linux-cifs@vger.kernel.org, "Yan Zheng" <zyan@redhat.com>,
	"Sage Weil" <sage@redhat.com>,
	"Ilya Dryomov" <idryomov@gmail.com>,
	"Alex Elder" <elder@kernel.org>,
	ceph-devel@vger.kernel.org,
	"Eric Van Hensbergen" <ericvh@gmail.com>,
	"Latchesar Ionkov" <lucho@ionkov.net>,
	"Mike Marshall" <hubcap@omnibond.com>,
	"Martin Brandenburg" <martin@omnibond.com>,
	"Dominique Martinet" <asmadeus@codewreck.org>,
	v9fs-developer@lists.sourceforge.net, "Coly Li" <colyli@suse.de>,
	linux-bcache@vger.kernel.org,
	"Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>
Subject: Re: [PATCH v1 00/15] Keep track of GUPed pages in fs and block
Date: Wed, 17 Apr 2019 01:09:22 +0300	[thread overview]
Message-ID: <41e2d7e1-104b-a006-2824-015ca8c76cc8@gmail.com> (raw)
In-Reply-To: <20190416195735.GE21526@redhat.com>

On 16/04/19 22:57, Jerome Glisse wrote:
<>
> 
> A very long thread on this:
> 
> https://lkml.org/lkml/2018/12/3/1128
> 
> especialy all the reply to this first one
> 
> There is also:
> 
> https://lkml.org/lkml/2019/3/26/1395
> https://lwn.net/Articles/753027/
> 

OK I have re-read this patchset and a little bit of the threads above (not all)

As I understand the long term plan is to keep two separate ref-counts one
for GUP-ref and one for the regular page-state/ownership ref.
Currently looking at page-ref we do not know if we have a GUP currently held.
With the new plan we can (Still not sure what's the full plan with this new info)

But if you make it such as the first GUP-ref also takes a page_ref and the
last GUp-dec also does put_page. Then the all of these becomes a matter of
matching every call to get_user_pages or iov_iter_get_pages() with a new
put_user_pages or iov_iter_put_pages().

Then if much below us an LLD takes a get_page() say an skb below the iscsi
driver, and so on. We do not care and we keep doing a put_page because we know
the GUP-ref holds the page for us.

The current block layer is transparent to any page-ref it does not take any
nor put_page any. It is only the higher users that have done GUP that take care of that.

The patterns I see are:

  iov_iter_get_pages()

	IO(sync)

  for(numpages)
	put_page()

Or

  iov_iter_get_pages()

	IO (async)
		->	foo_end_io()
				put_page

(Same with get_user_pages)
(IO need not be block layer. It can be networking and so on like in NFS or CIFS
 and so on)

The first pattern is easy just add the proper new api for
it, so for every iov_iter_get_pages() you have an iov_iter_put_pages() and remove
lots of cooked up for loops. Also the all iov_iter_get_pages_use_gup() just drops.
(Same at get_user_pages sites use put_user_pages)

The second pattern is a bit harder because it is possible that the foo_end_io()
is currently used for GUP as well as none-GUP cases. this is easy to fix. But the
even harder case is if the same foo_end_io() call has some pages GUPed and some not
in the same call.

staring at this patchset and the call sites I did not see any such places. Do you know
of any?
(We can always force such mixed-case users to always GUP-ref the pages and code
 foo_end_io() to GUP-dec)

So with a very careful coding I think you need not touch the block / scatter-list layers
nor any LLD drivers. The only code affected is the code around the get_user_pages and friends.
Changing the API will surface all those.
(IE. introduce a new API, convert one by one, Remove old API)

Am I smoking?

BTW: Are you aware of the users of iov_iter_get_pages_alloc() Do they need fixing too?

> Cheers,
> Jérôme
> 

Thanks
Boaz