On 2020-06-12 12:24, Matthew Wilcox wrote:
On Fri, May 29, 2020 at 04:43:08PM -0700, John Hubbard wrote:
+CASE 5: Pinning in order to write to the data within the page
+-------------------------------------------------------------
+Even though neither DMA nor Direct IO is involved, just a simple case of "pin,
+access page's data, unpin" can cause a problem. Case 5 may be considered a
+superset of Case 1, plus Case 2, plus anything that invokes that pattern. In
+other words, if the code is neither Case 1 nor Case 2, it may still require
+FOLL_PIN, for patterns like this:
+
+Correct (uses FOLL_PIN calls):
+ pin_user_pages()
+ access the data within the pages
+ set_page_dirty_lock()
+ unpin_user_pages()
+
+INCORRECT (uses FOLL_GET calls):
+ get_user_pages()
+ access the data within the pages
+ set_page_dirty_lock()
+ put_page()
Why does this case need to pin? Why can't it just do ...
get_user_pages()
lock_page(page);
... modify the data ...
set_page_dirty(page);
unlock_page(page);
Yes, it could do that. And that would also make a good additional "correct"
example. Especially for the case of just dealing with a single page,
lock_page() has the benefit of completely fixing the problem *today*,
without waiting for the pin_user_pages*() handling improvements to get
implemented.
And it's also another (probably better) way to fix the vhost.c problem, than
commit 690623e1b496 ("vhost: convert get_user_pages() --> pin_user_pages()").
I'm inclined to leave vhost.c alone for now, unless someone really prefers
it to be changed, but to update the Case 5 documentation with your point
above. Sound about right?
thanks,
--
John Hubbard
NVIDIA
