Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   John Hubbard <jhubbard@nvidia.com>
To:     LKML <linux-kernel@vger.kernel.org>
CC:     Souptick Joarder <jrdr.linux@gmail.com>,
        John Hubbard <jhubbard@nvidia.com>,
        =?UTF-8?q?Kai=20M=C3=A4kisara=20=28Kolumbus=29?= 
        <kai.makisara@kolumbus.fi>, Bart Van Assche <bvanassche@acm.org>,
        "James E . J . Bottomley" <jejb@linux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@oracle.com>,
        <linux-scsi@vger.kernel.org>
Subject: [PATCH v2] scsi: st: convert convert get_user_pages() --> pin_user_pages()
Date:   Tue, 26 May 2020 11:27:09 -0700
Message-ID: <20200526182709.99599-1-jhubbard@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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=C3=A4kisara (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 =3D (uaddr + count + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	unsigned long start =3D uaddr >> PAGE_SHIFT;
 	const int nr_pages =3D end - start;
-	int res, i, j;
+	int res, i;
 	struct page **pages;
 	struct rq_map_data *mdata =3D &STbp->map_data;
=20
@@ -4944,7 +4944,7 @@ static int sgl_map_user_pages(struct st_buffer *STbp,
=20
         /* Try to fault in all of the necessary pages */
         /* rw=3D=3DREAD means read from drive, write into memory area */
-	res =3D get_user_pages_fast(uaddr, nr_pages, rw =3D=3D READ ? FOLL_WRITE =
: 0,
+	res =3D pin_user_pages_fast(uaddr, nr_pages, rw =3D=3D READ ? FOLL_WRITE =
: 0,
 				  pages);
=20
 	/* 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=3D0; j < res; j++)
-			put_page(pages[j]);
+		unpin_user_pages(pages, res);
 		res =3D 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=3D0; i < nr_pages; i++) {
-		struct page *page =3D STbp->mapped_pages[i];
+	/* FIXME: cache flush missing for rw=3D=3DREAD */
+	unpin_user_pages_dirty_lock(STbp->mapped_pages, nr_pages, dirtied);
=20
-		if (dirtied)
-			SetPageDirty(page);
-		/* FIXME: cache flush missing for rw=3D=3DREAD
-		 * FIXME: call the correct reference counting function
-		 */
-		put_page(page);
-	}
 	kfree(STbp->mapped_pages);
 	STbp->mapped_pages =3D NULL;
=20
--=20
2.26.2