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