diff mbox series

[05/13] mm: simplify generic_file_buffered_read_no_cached_page

Message ID 20201031090004.452516-6-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [01/13] mm: simplify generic_file_buffered_read_readpage | expand

Commit Message

Christoph Hellwig Oct. 31, 2020, 8:59 a.m. UTC 
Return an errno and add a new by reference argument for the allocated
page, which allows to cleanup the error unwindining in the function
and the caller.  Also rename the function to filemap_new_page which is
both shorter and more descriptive.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 53 ++++++++++++++++------------------------------------
 1 file changed, 16 insertions(+), 37 deletions(-)

Comments

Matthew Wilcox Oct. 31, 2020, 4:20 p.m. UTC | #1 
On Sat, Oct 31, 2020 at 09:59:56AM +0100, Christoph Hellwig wrote:
> -	page = page_cache_alloc(mapping);
> +	*page = page_cache_alloc(mapping);
>  	if (!page)

	if (!*page)
Matthew Wilcox Oct. 31, 2020, 4:28 p.m. UTC | #2 
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.
Christoph Hellwig Nov. 1, 2020, 10:29 a.m. UTC | #3 
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;
 }
 
 /**
diff mbox series

Patch

diff --git a/mm/filemap.c b/mm/filemap.c
index 5cdf8090d4e12c..9e1cc18afe1134 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2300,41 +2300,27 @@  static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	return error;
 }
 
-static struct page *
-generic_file_buffered_read_no_cached_page(struct kiocb *iocb,
-					  struct iov_iter *iter)
+static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
+		struct page **page)
 {
-	struct file *filp = iocb->ki_filp;
-	struct address_space *mapping = filp->f_mapping;
+	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 ERR_PTR(-EAGAIN);
+		return -EAGAIN;
 
-	/*
-	 * Ok, it wasn't cached, so we need to create a new
-	 * page..
-	 */
-	page = page_cache_alloc(mapping);
+	*page = page_cache_alloc(mapping);
 	if (!page)
-		return ERR_PTR(-ENOMEM);
-
-	error = add_to_page_cache_lru(page, mapping, index,
-				      mapping_gfp_constraint(mapping, GFP_KERNEL));
+		return -ENOMEM;
+	error = add_to_page_cache_lru(*page, mapping, index, gfp);
 	if (error) {
-		put_page(page);
-		return error != -EEXIST ? ERR_PTR(error) : NULL;
+		put_page(*page);
+		return error;
 	}
 
-	error = filemap_readpage(iocb, page);
-	if (error) {
-		if (error == AOP_TRUNCATED_PAGE)
-			return NULL;
-		return ERR_PTR(error);
-	}
-	return page;
+	return filemap_readpage(iocb, *page);
 }
 
 static int generic_file_buffered_read_get_pages(struct kiocb *iocb,
@@ -2366,18 +2352,14 @@  static int generic_file_buffered_read_get_pages(struct kiocb *iocb,
 	nr_got = find_get_pages_contig(mapping, index, nr, pages);
 	if (nr_got)
 		goto got_pages;
-
-	pages[0] = generic_file_buffered_read_no_cached_page(iocb, iter);
-	err = PTR_ERR_OR_ZERO(pages[0]);
-	if (!IS_ERR_OR_NULL(pages[0]))
+	err = filemap_new_page(iocb, iter, &pages[0]);
+	if (!err)
 		nr_got = 1;
 got_pages:
 	for (i = 0; i < nr_got; i++) {
 		err = filemap_make_page_uptodate(iocb, iter, pages[i],
 				index + i, i == 0);
 		if (err) {
-			if (err == AOP_TRUNCATED_PAGE)
-				err = 0;
 			for (j = i + 1; j < nr_got; j++)
 				put_page(pages[j]);
 			nr_got = i;
@@ -2387,12 +2369,9 @@  static int generic_file_buffered_read_get_pages(struct kiocb *iocb,
 
 	if (likely(nr_got))
 		return nr_got;
-	if (err)
-		return err;
-	/*
-	 * No pages and no error means we raced and should retry:
-	 */
-	goto find_page;
+	if (err == -EEXIST || err == AOP_TRUNCATED_PAGE)
+		goto find_page;
+	return err;
 }
 
 /**