RE: [PATCH 00/34] put_user_pages(): miscellaneous call sites

On Wed 07-08-19 19:36:37, Ira Weiny wrote:

quoted

On Wed, Aug 07, 2019 at 10:46:49AM +0200, Michal Hocko wrote:

quoted

quoted

So I think your debug option and my suggested renaming serve a bit
different purposes (and thus both make sense). If you do the
renaming, you can just grep to see unconverted sites. Also when
someone merges new GUP user (unaware of the new rules) while you
switch GUP to use pins instead of ordinary references, you'll get
compilation error in case of renaming instead of hard to debug
refcount leak without the renaming. And such conflict is almost
bound to happen given the size of GUP patch set... Also the
renaming serves against the "coding inertia" - i.e., GUP is around for

ages so people just use it without checking any documentation or comments.

quoted

quoted

quoted

After switching how GUP works, what used to be correct isn't
anymore so renaming the function serves as a warning that
something has really changed.

Fully agreed!

Ok Prior to this I've been basing all my work for the RDMA/FS DAX
stuff in Johns put_user_pages()...  (Including when I proposed failing
truncate with a lease in June [1])

However, based on the suggestions in that thread it became clear that
a new interface was going to need to be added to pass in the "RDMA
file" information to GUP to associate file pins with the correct processes...

I have many drawings on my white board with "a whole lot of lines" on
them to make sure that if a process opens a file, mmaps it, pins it
with RDMA, _closes_ it, and ummaps it; that the resulting file pin can
still be traced back to the RDMA context and all the processes which
may have access to it....  No matter where the original context may
have come from.  I believe I have accomplished that.

Before I go on, I would like to say that the "imbalance" of
get_user_pages() and put_page() bothers me from a purist standpoint...
However, since this discussion cropped up I went ahead and ported my
work to Linus' current master
(5.3-rc3+) and in doing so I only had to steal a bit of Johns code...
Sorry John...  :-(

I don't have the commit messages all cleaned up and I know there may
be some discussion on these new interfaces but I wanted to throw this
series out there because I think it may be what Jan and Michal are
driving at (or at least in that direction.

Right now only RDMA and DAX FS's are supported.  Other users of GUP
will still fail on a DAX file and regular files will still be at
risk.[2]

I've pushed this work (based 5.3-rc3+ (33920f1ec5bf)) here[3]:

https://github.com/weiny2/linux-kernel/tree/linus-rdmafsdax-b0-v3

I think the most relevant patch to this conversation is:

https://github.com/weiny2/linux-

kernel/commit/5d377653ba5cf11c3b716f90

quoted

4b057bee6641aaf6

I stole Jans suggestion for a name as the name I used while
prototyping was pretty bad...  So Thanks Jan...  ;-)

For your function, I'd choose a name like vaddr_pin_leased_pages() so that
association with a lease is clear from the name :)

My gut was to just change this as you suggested.  But the fact is that these calls can get used on anonymous pages as well.  So the "leased" semantic may not apply...  OTOH if a file is encountered it will fail the pin...  :-/  I'm going to leave it for now and get the patches submitted to the list...

Also I'd choose the
counterpart to be vaddr_unpin_leased_page[s](). Especially having put_page
in the name looks confusing to me...

Ah yes, totally agree with the "pin/unpin" symmetry.  I've changed from "put" to "unpin"...

Thanks,
Ira

								Honza

--
Jan Kara [off-list ref]
SUSE Labs, CR

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel