| Message ID | 20200526182709.99599-1-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 08e9cbe75facfe08b9017eca37c9f1c5a6490b4a |
| Headers | show |
| Series | [v2] scsi: st: convert convert get_user_pages() --> pin_user_pages() | expand |
For some reason, the "convert convert" subject line is really hard to get rid of from my scsi st patch. In this case, I'd dropped the patch entirely, and recreated it with the old subject line somehow. Sorry about that persistent typo! I'll send a v3 if necessary, to correct that. thanks, John Hubbard NVIDIA On 2020-05-26 11:27, John Hubbard 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@lst.de > > Cc: "Kai Mäkisara (Kolumbus)" <kai.makisara@kolumbus.fi> > Cc: Bart Van Assche <bvanassche@acm.org> > Cc: James E.J. Bottomley <jejb@linux.ibm.com> > Cc: Martin K. Petersen <martin.petersen@oracle.com> > Cc: linux-scsi@vger.kernel.org > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- > > 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@nvidia.com/ > > thanks, > John Hubbard > NVIDIA > > > 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; > >
John, > For some reason, the "convert convert" subject line is really hard to > get rid of from my scsi st patch. In this case, I'd dropped the patch > entirely, and recreated it with the old subject line somehow. Sorry > about that persistent typo! > > I'll send a v3 if necessary, to correct that. I'll fix it up when I apply.
> 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. Kai: Please review. Thanks!
> On 26. May 2020, at 21.27, John Hubbard <jhubbard@nvidia.com> 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@lst.de > > Cc: "Kai Mäkisara (Kolumbus)" <kai.makisara@kolumbus.fi> > Cc: Bart Van Assche <bvanassche@acm.org> > Cc: James E.J. Bottomley <jejb@linux.ibm.com> > Cc: Martin K. Petersen <martin.petersen@oracle.com> > Cc: linux-scsi@vger.kernel.org > Signed-off-by: John Hubbard <jhubbard@nvidia.com> Acked-by: Kai Mäkisara <kai.makisara@kolumbus.fi> > --- > > 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@nvidia.com/ > > 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 >
On Tue, 26 May 2020 11:27:09 -0700, John Hubbard 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. > > [...] Applied to 5.8/scsi-queue, thanks! [1/1] scsi: st: Convert convert get_user_pages() --> pin_user_pages() https://git.kernel.org/mkp/scsi/c/08e9cbe75fac
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;
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@lst.de Cc: "Kai Mäkisara (Kolumbus)" <kai.makisara@kolumbus.fi> Cc: Bart Van Assche <bvanassche@acm.org> Cc: James E.J. Bottomley <jejb@linux.ibm.com> Cc: Martin K. Petersen <martin.petersen@oracle.com> Cc: linux-scsi@vger.kernel.org Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- 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@nvidia.com/ thanks, John Hubbard NVIDIA drivers/scsi/st.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-)