Re: [PATCH v2 11/18] mm/filemap: Convert filemap_update_page to return an errno

[Christoph Hellwig <hch@xxxxxx>]

On Fri, Nov 06, 2020 at 09:14:20AM +0100, Christoph Hellwig wrote:
> We could still consolidate the page unlocking by having another label.
> Or even better move the put_page into the caller like I did in my
> series, which would conceputally fit in pretty nicely here:

FYI, this is what we should do for putting the page (on top of your
whole series), which also catches the leak for the readahead NOIO case:

diff --git a/mm/filemap.c b/mm/filemap.c
index 500e9fd4232cf9..dacee60b92d3d9 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2268,40 +2268,35 @@ static int filemap_update_page(struct kiocb *iocb,
 		struct address_space *mapping, struct iov_iter *iter,
 		struct page *page)
 {
-	int error = -EAGAIN;
+	int error = 0;
 
 	if (!trylock_page(page)) {
 		if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_NOIO))
-			goto error;
+			return -EAGAIN;
 		if (!(iocb->ki_flags & IOCB_WAITQ)) {
 			put_and_wait_on_page_locked(page, TASK_KILLABLE);
 			return AOP_TRUNCATED_PAGE;
 		}
 		error = __lock_page_async(page, iocb->ki_waitq);
 		if (error)
-			goto error;
+			return error;
 	}
 
-	error = AOP_TRUNCATED_PAGE;
-	if (!page->mapping)
-		goto unlock;
-	if (filemap_range_uptodate(iocb, mapping, iter, page)) {
+	if (!page->mapping) {
 		unlock_page(page);
-		return 0;
+		put_page(page);
+		return AOP_TRUNCATED_PAGE;
 	}
 
-	error = -EAGAIN;
-	if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ))
-		goto unlock;
+	if (!filemap_range_uptodate(iocb, mapping, iter, page)) {
+		error = -EAGAIN;
+		if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ))
+			goto unlock;
+		return filemap_read_page(iocb->ki_filp, mapping, page);
+	}
 
-	error = filemap_read_page(iocb->ki_filp, mapping, page);
-	if (error)
-		goto error;
-	return 0;
 unlock:
 	unlock_page(page);
-error:
-	put_page(page);
 	return error;
 }
 
@@ -2396,8 +2391,9 @@ static int filemap_get_pages(struct kiocb *iocb, struct iov_iter *iter,
 
 	return 0;
 err:
-	pvec->nr--;
-	if (likely(pvec->nr))
+	if (err < 0)
+		put_page(page);
+	if (likely(--pvec->nr))
 		return 0;
 	if (err == AOP_TRUNCATED_PAGE)
 		goto retry;