* Re: [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages()
2020-05-26 18:27 [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages() John Hubbard
@ 2020-05-27 1:05 ` John Hubbard
2020-05-27 1:08 ` Martin K. Petersen
2020-06-03 1:29 ` Martin K. Petersen
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: John Hubbard @ 2020-05-27 1:05 UTC (permalink / raw)
To: LKML
Cc: Souptick Joarder, Kai Mäkisara (Kolumbus),
Bart Van Assche, James E . J . Bottomley, Martin K . Petersen,
linux-scsi
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;
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages()
2020-05-27 1:05 ` John Hubbard
@ 2020-05-27 1:08 ` Martin K. Petersen
0 siblings, 0 replies; 6+ messages in thread
From: Martin K. Petersen @ 2020-05-27 1:08 UTC (permalink / raw)
To: John Hubbard
Cc: LKML, Souptick Joarder, Kai Mäkisara (Kolumbus),
Bart Van Assche, James E . J . Bottomley, Martin K . Petersen,
linux-scsi
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.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages()
2020-05-26 18:27 [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages() John Hubbard
2020-05-27 1:05 ` John Hubbard
@ 2020-06-03 1:29 ` Martin K. Petersen
2020-06-05 10:17 ` "Kai Mäkisara (Kolumbus)"
2020-06-10 2:02 ` Martin K. Petersen
3 siblings, 0 replies; 6+ messages in thread
From: Martin K. Petersen @ 2020-06-03 1:29 UTC (permalink / raw)
To: Kai Mäkisara (Kolumbus)
Cc: LKML, Souptick Joarder, Bart Van Assche, James E . J . Bottomley,
Martin K . Petersen, linux-scsi, John Hubbard
> 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!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages()
2020-05-26 18:27 [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages() John Hubbard
2020-05-27 1:05 ` John Hubbard
2020-06-03 1:29 ` Martin K. Petersen
@ 2020-06-05 10:17 ` "Kai Mäkisara (Kolumbus)"
2020-06-10 2:02 ` Martin K. Petersen
3 siblings, 0 replies; 6+ messages in thread
From: "Kai Mäkisara (Kolumbus)" @ 2020-06-05 10:17 UTC (permalink / raw)
To: John Hubbard
Cc: LKML, Souptick Joarder, Bart Van Assche, James E . J . Bottomley,
Martin K . Petersen, linux-scsi
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages()
2020-05-26 18:27 [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages() John Hubbard
` (2 preceding siblings ...)
2020-06-05 10:17 ` "Kai Mäkisara (Kolumbus)"
@ 2020-06-10 2:02 ` Martin K. Petersen
3 siblings, 0 replies; 6+ messages in thread
From: Martin K. Petersen @ 2020-06-10 2:02 UTC (permalink / raw)
To: LKML, John Hubbard
Cc: Martin K . Petersen, James E . J . Bottomley, Kai Mäkisara,
Souptick Joarder, Bart Van Assche, linux-scsi
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
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 6+ messages in thread