[PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages()

[PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages()

[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.

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;
 
-- 
2.26.2

Re: [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages()

[John Hubbard]

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;
>   
> 

Re: [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages()

[Martin K. Petersen]

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

Re: [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages()

[Martin K. Petersen]

> 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

Re: [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages()

["Kai Mäkisara (Kolumbus)"]

> 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
> 

Re: [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages()

[Martin K. Petersen]

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