Messages in this thread | | | Subject | Re: [PATCH] scsi: st: convert convert get_user_pages() --> pin_user_pages() | From | John Hubbard <> | Date | Thu, 21 May 2020 12:57:01 -0700 |
| |
On 2020-05-21 12:47, Bart Van Assche wrote: > On 2020-05-18 21:55, John Hubbard wrote: >> This code was using get_user_pages*(), in a "Case 2" scenario >> (DMA/RDMA), using the categorization from [1]. That means that it's >> time to convert the get_user_pages*() + put_page() calls to >> pin_user_pages*() + unpin_user_pages() calls. >> >> There is some helpful background in [2]: basically, this is a small >> part of fixing a long-standing disconnect between pinning pages, and >> file systems' use of those pages. >> >> Note that this effectively changes the code's behavior as well: it now >> ultimately calls set_page_dirty_lock(), instead of SetPageDirty().This >> is probably more accurate. >> >> As Christoph Hellwig put it, "set_page_dirty() is only safe if we are >> dealing with a file backed page where we have reference on the inode it >> hangs off." [3] >> >> Also, this deletes one of the two FIXME comments (about refcounting), >> because there is nothing wrong with the refcounting at this point. >> >> [1] Documentation/core-api/pin_user_pages.rst >> >> [2] "Explicit pinning of user-space pages": >> https://lwn.net/Articles/807108/ >> >> [3] https://lore.kernel.org/r/20190723153640.GB720@lst.de > > Kai, why is the st driver calling get_user_pages_fast() directly instead > of calling blk_rq_map_user()? blk_rq_map_user() is already used in > st_scsi_execute(). I think that the blk_rq_map_user() implementation is > also based on get_user_pages_fast(). See also iov_iter_get_pages_alloc() > in lib/iov_iter.c. > > John, why are the get_user_pages_fast() calls in the st driver modified > but not the blk_rq_map_user() call? Are you sure that the modified code > is a "case 2" scenario and not a "case 1" scenario? >
No, I am not sure. I thought this was a DMA case (I'm not a SCSI Tape user, so it *seemed* reasonable that a DMA engine was involved), but if it's really direct IO, then we need to just drop this patch entirely. Because: I need to convert the block/biovec code, including iov_iter_get_pages_alloc() and friends, in order to handle direct IO. I'm working on that but it's not ready yet.
(I was trying to get the smaller, non-direct-IO cases converted first.)
Thanks for spotting the discrepancy, and apologies for the confusion on this end.
Also, I doubt if it's worth it, but do you want a patch to change SetPageDirty() to set_page_dirty_lock(), meanwhile? It seems like if that's never come up, then it's mostly a theoretical bug.
thanks, -- John Hubbard NVIDIA
| |