Re: [PATCH 05/13] mm: simplify generic_file_buffered_read_no_cached_page

[Christoph Hellwig <hch@lst.de>]

On Sat, Oct 31, 2020 at 04:28:13PM +0000, Matthew Wilcox wrote:
> On Sat, Oct 31, 2020 at 09:59:56AM +0100, Christoph Hellwig wrote:
> > +static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
> > +		struct page **page)
> 
> I don't like this calling convention.  It's too easy to get it wrong,
> as you demonstrated.  I preferred the way Kent had it with returning
> an ERR_PTR.

I guess for this function we can do that, here is the untested patch
on top of my series to show what we'd end up with:

diff --git a/mm/filemap.c b/mm/filemap.c
index b45f0bafdbaebf..1ac1fcd0067bf1 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2281,38 +2281,40 @@ static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	return 0;
 }
 
-static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
-		struct page **page)
+static struct page *filemap_new_page(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct address_space *mapping = iocb->ki_filp->f_mapping;
 	gfp_t gfp = mapping_gfp_constraint(mapping, GFP_KERNEL);
 	pgoff_t index = iocb->ki_pos >> PAGE_SHIFT;
+	struct page *page;
 	int error;
 
 	if (iocb->ki_flags & IOCB_NOIO)
-		return -EAGAIN;
+		return ERR_PTR(-EAGAIN);
 
-	*page = page_cache_alloc(mapping);
+	page = page_cache_alloc(mapping);
 	if (!page)
-		return -ENOMEM;
-	error = add_to_page_cache_lru(*page, mapping, index, gfp);
+		return ERR_PTR(-ENOMEM);
+	error = add_to_page_cache_lru(page, mapping, index, gfp);
 	if (error)
 		goto put_page;
-	error = filemap_readpage(iocb, *page);
+	error = filemap_readpage(iocb, page);
 	if (error)
 		goto put_page;
-	if (PageReadahead(*page)) {
+	if (PageReadahead(page)) {
 		error = -EAGAIN;
 		if (iocb->ki_flags & IOCB_NOIO)
 			goto put_page;
 		page_cache_async_readahead(mapping, &iocb->ki_filp->f_ra,
-				iocb->ki_filp, *page, index,
+				iocb->ki_filp, page, index,
 				(iter->count + PAGE_SIZE - 1) >> PAGE_SHIFT);
 	}
 	return 0;
 put_page:
-	put_page(*page);
-	return error;
+	put_page(page);
+	if (error == -EEXIST || error == AOP_TRUNCATED_PAGE)
+		return NULL;
+	return ERR_PTR(error);
 }
 
 static int filemap_find_get_pages(struct kiocb *iocb, struct iov_iter *iter,
@@ -2347,28 +2349,30 @@ static int filemap_read_pages(struct kiocb *iocb, struct iov_iter *iter,
 		return -EINTR;
 
 	nr_pages = filemap_find_get_pages(iocb, iter, pages, nr);
-	if (nr_pages) {
-		for (i = 0; i < nr_pages; i++) {
-			err = filemap_make_page_uptodate(iocb, iter, pages[i],
-					index + i, i == 0);
-			if (err) {
-				for (j = i; j < nr_pages; j++)
-					put_page(pages[j]);
-				nr_pages = i;
-				break;
-			}
+	if (!nr_pages) {
+		pages[0] = filemap_new_page(iocb, iter);
+		if (pages[0] == NULL)
+			goto retry;
+		if (IS_ERR(pages[0]))
+			return PTR_ERR(pages[0]);
+		return 1;
+	}
+
+	for (i = 0; i < nr_pages; i++) {
+		err = filemap_make_page_uptodate(iocb, iter, pages[i],
+				index + i, i == 0);
+		if (err) {
+			for (j = i; j < nr_pages; j++)
+				put_page(pages[j]);
+			if (likely(i > 0))
+				return i;
+			if (err == AOP_TRUNCATED_PAGE)
+				goto retry;
+			return err;
 		}
-	} else {
-		err = filemap_new_page(iocb, iter, &pages[0]);
-		if (!err)
-			nr_pages = 1;
 	}
 
-	if (likely(nr_pages))
-		return nr_pages;
-	if (err == -EEXIST || err == AOP_TRUNCATED_PAGE)
-		goto retry;
-	return err;
+	return nr_pages;
 }
 
 /**