> On 26. May 2020, at 21.27, John Hubbard <jhubbard@xxxxxxxxxx> wrote: > > This code was using get_user_pages*(), in a "Case 1" scenario > (Direct IO), 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@xxxxxx > > Cc: "Kai Mäkisara (Kolumbus)" <kai.makisara@xxxxxxxxxxx> > Cc: Bart Van Assche <bvanassche@xxxxxxx> > Cc: James E.J. Bottomley <jejb@xxxxxxxxxxxxx> > Cc: Martin K. Petersen <martin.petersen@xxxxxxxxxx> > Cc: linux-scsi@xxxxxxxxxxxxxxx > Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx> Acked-by: Kai Mäkisara <kai.makisara@xxxxxxxxxxx> > --- > > Hi, > > As mentioned in the v1 review thread, we probably still want/need > this. Or so I claim. :) Please see what you think... > > Changes since v1: changed the commit log, to refer to Direct IO > (Case 1), instead of DMA/RDMA (Case 2). And added Bart to Cc. > > v1: > https://lore.kernel.org/linux-scsi/20200519045525.2446851-1-jhubbard@xxxxxxxxxx/ > > thanks, > John Hubbard > NVIDIA I am not a memory management expert, but looks correct and necessary to me with the current gup implementation. (You can changed Acked-by to Reviewed-by if it is necessary and you accept this as a review.) Thanks, Kai > > drivers/scsi/st.c | 20 +++++--------------- > 1 file changed, 5 insertions(+), 15 deletions(-) > > diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c > index c5f9b348b438..1e3eda9fa231 100644 > --- a/drivers/scsi/st.c > +++ b/drivers/scsi/st.c > @@ -4922,7 +4922,7 @@ static int sgl_map_user_pages(struct st_buffer *STbp, > unsigned long end = (uaddr + count + PAGE_SIZE - 1) >> PAGE_SHIFT; > unsigned long start = uaddr >> PAGE_SHIFT; > const int nr_pages = end - start; > - int res, i, j; > + int res, i; > struct page **pages; > struct rq_map_data *mdata = &STbp->map_data; > > @@ -4944,7 +4944,7 @@ static int sgl_map_user_pages(struct st_buffer *STbp, > > /* Try to fault in all of the necessary pages */ > /* rw==READ means read from drive, write into memory area */ > - res = get_user_pages_fast(uaddr, nr_pages, rw == READ ? FOLL_WRITE : 0, > + res = pin_user_pages_fast(uaddr, nr_pages, rw == READ ? FOLL_WRITE : 0, > pages); > > /* Errors and no page mapped should return here */ > @@ -4964,8 +4964,7 @@ static int sgl_map_user_pages(struct st_buffer *STbp, > return nr_pages; > out_unmap: > if (res > 0) { > - for (j=0; j < res; j++) > - put_page(pages[j]); > + unpin_user_pages(pages, res); > res = 0; > } > kfree(pages); > @@ -4977,18 +4976,9 @@ static int sgl_map_user_pages(struct st_buffer *STbp, > static int sgl_unmap_user_pages(struct st_buffer *STbp, > const unsigned int nr_pages, int dirtied) > { > - int i; > - > - for (i=0; i < nr_pages; i++) { > - struct page *page = STbp->mapped_pages[i]; > + /* FIXME: cache flush missing for rw==READ */ > + unpin_user_pages_dirty_lock(STbp->mapped_pages, nr_pages, dirtied); > > - if (dirtied) > - SetPageDirty(page); > - /* FIXME: cache flush missing for rw==READ > - * FIXME: call the correct reference counting function > - */ > - put_page(page); > - } > kfree(STbp->mapped_pages); > STbp->mapped_pages = NULL; > > -- > 2.26.2 >
