On Wed 07-08-19 19:36:37, Ira Weiny wrote:
On Wed, Aug 07, 2019 at 10:46:49AM +0200, Michal Hocko wrote:
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. 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/5d377653ba5cf11c3b716f904b057b...
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 :) Also I'd choose the counterpart to be vaddr_unpin_leased_page[s](). Especially having put_page in the name looks confusing to me...
Honza