diff mbox series

[07/17] mm/filemap: Change filemap_read_page calling conventions

Message ID 20201102184312.25926-8-willy@infradead.org (mailing list archive)
State New, archived
Headers show
Series Refactor generic_file_buffered_read | expand

Commit Message

Matthew Wilcox Nov. 2, 2020, 6:43 p.m. UTC 
Make this function more generic by passing the file instead of the iocb.
Check in the callers whether we should call readpage or not.  Also make
it return an errno / 0 / AOP_TRUNCATED_PAGE, and make calling put_page()
the caller's responsibility.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/filemap.c | 89 +++++++++++++++++++++++++---------------------------
 1 file changed, 42 insertions(+), 47 deletions(-)

Comments

Kent Overstreet Nov. 2, 2020, 7:37 p.m. UTC | #1 
On Mon, Nov 02, 2020 at 06:43:02PM +0000, Matthew Wilcox (Oracle) wrote:
> Make this function more generic by passing the file instead of the iocb.
> Check in the callers whether we should call readpage or not.  Also make
> it return an errno / 0 / AOP_TRUNCATED_PAGE, and make calling put_page()
> the caller's responsibility.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Kent Overstreet <kent.overstreet@gmail.com>
Christoph Hellwig Nov. 3, 2020, 7:34 a.m. UTC | #2 
> +static int filemap_read_page(struct file *file, struct address_space *mapping,
> +		struct page *page)

I still think we should drop the mapping argument as well.

> +	if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ)) {
> +		unlock_page(page);
> +		put_page(page);
> +		return ERR_PTR(-EAGAIN);
> +	}
> +	error = filemap_read_page(iocb->ki_filp, mapping, page);
> +	if (!error)
> +		return page;

I think a goto error for the error cases would be much more useful.
That would allow to also share the error put_page for the flag check
above and the truncated case below, but most importantly make the code
flow obvious vs the early return for the success case.

> -	return filemap_read_page(iocb, filp, mapping, page);
> +	if (!error)
> +		error = filemap_read_page(iocb->ki_filp, mapping, page);
> +	if (!error)
> +		return page;
> +	put_page(page);
> +	if (error == -EEXIST || error == AOP_TRUNCATED_PAGE)
> +		return NULL;
> +	return ERR_PTR(error);

Same here.
Matthew Wilcox Nov. 3, 2020, 3:11 p.m. UTC | #3 
On Tue, Nov 03, 2020 at 08:34:27AM +0100, Christoph Hellwig wrote:
> > +static int filemap_read_page(struct file *file, struct address_space *mapping,
> > +		struct page *page)
> 
> I still think we should drop the mapping argument as well.

Feels a little weird to have it in the caller and not pass it in.

> > +	if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ)) {
> > +		unlock_page(page);
> > +		put_page(page);
> > +		return ERR_PTR(-EAGAIN);
> > +	}
> > +	error = filemap_read_page(iocb->ki_filp, mapping, page);
> > +	if (!error)
> > +		return page;
> 
> I think a goto error for the error cases would be much more useful.
> That would allow to also share the error put_page for the flag check
> above and the truncated case below, but most importantly make the code
> flow obvious vs the early return for the success case.

In this patch, I'm trying to be obvious about the code motion between
the two functions.  This gets straightened out eventually:

        if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ)) {
                unlock_page(page);
                error = -EAGAIN;
        } else {
                error = filemap_read_page(iocb->ki_filp, mapping, page);
                if (!error)
                        return 0;
        }
error:
        put_page(page);
        return error;
diff mbox series

Patch

diff --git a/mm/filemap.c b/mm/filemap.c
index 5bafd2dc830c..171da5c592fa 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2194,56 +2194,38 @@  static unsigned mapping_get_read_thps(struct address_space *mapping,
 	return ret;
 }
 
-static struct page *filemap_read_page(struct kiocb *iocb, struct file *filp,
-		struct address_space *mapping, struct page *page)
+static int filemap_read_page(struct file *file, struct address_space *mapping,
+		struct page *page)
 {
-	struct file_ra_state *ra = &filp->f_ra;
 	int error;
 
-	if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ)) {
-		unlock_page(page);
-		put_page(page);
-		return ERR_PTR(-EAGAIN);
-	}
-
 	/*
-	 * A previous I/O error may have been due to temporary
-	 * failures, eg. multipath errors.
-	 * PG_error will be set again if readpage fails.
+	 * A previous I/O error may have been due to temporary failures,
+	 * eg. multipath errors.  PG_error will be set again if readpage
+	 * fails.
 	 */
 	ClearPageError(page);
 	/* Start the actual read. The read will unlock the page. */
-	error = mapping->a_ops->readpage(filp, page);
-
-	if (unlikely(error)) {
-		put_page(page);
-		return error != AOP_TRUNCATED_PAGE ? ERR_PTR(error) : NULL;
-	}
+	error = mapping->a_ops->readpage(file, page);
+	if (error)
+		return error;
+	if (PageUptodate(page))
+		return 0;
 
+	error = lock_page_killable(page);
+	if (error)
+		return error;
 	if (!PageUptodate(page)) {
-		error = lock_page_killable(page);
-		if (unlikely(error)) {
-			put_page(page);
-			return ERR_PTR(error);
-		}
-		if (!PageUptodate(page)) {
-			if (page->mapping == NULL) {
-				/*
-				 * invalidate_mapping_pages got it
-				 */
-				unlock_page(page);
-				put_page(page);
-				return NULL;
-			}
-			unlock_page(page);
-			shrink_readahead_size_eio(ra);
-			put_page(page);
-			return ERR_PTR(-EIO);
+		if (page->mapping == NULL) {
+			/* page truncated */
+			error = AOP_TRUNCATED_PAGE;
+		} else {
+			shrink_readahead_size_eio(&file->f_ra);
+			error = -EIO;
 		}
-		unlock_page(page);
 	}
-
-	return page;
+	unlock_page(page);
+	return error;
 }
 
 static struct page *filemap_update_page(struct kiocb *iocb, struct file *filp,
@@ -2285,7 +2267,18 @@  static struct page *filemap_update_page(struct kiocb *iocb, struct file *filp,
 	return page;
 
 readpage:
-	return filemap_read_page(iocb, filp, mapping, page);
+	if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ)) {
+		unlock_page(page);
+		put_page(page);
+		return ERR_PTR(-EAGAIN);
+	}
+	error = filemap_read_page(iocb->ki_filp, mapping, page);
+	if (!error)
+		return page;
+	put_page(page);
+	if (error == AOP_TRUNCATED_PAGE)
+		return NULL;
+	return ERR_PTR(error);
 truncated:
 	unlock_page(page);
 	put_page(page);
@@ -2301,7 +2294,7 @@  static struct page *filemap_create_page(struct kiocb *iocb,
 	struct page *page;
 	int error;
 
-	if (iocb->ki_flags & IOCB_NOIO)
+	if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ))
 		return ERR_PTR(-EAGAIN);
 
 	page = page_cache_alloc(mapping);
@@ -2310,12 +2303,14 @@  static struct page *filemap_create_page(struct kiocb *iocb,
 
 	error = add_to_page_cache_lru(page, mapping, index,
 				      mapping_gfp_constraint(mapping, GFP_KERNEL));
-	if (error) {
-		put_page(page);
-		return error != -EEXIST ? ERR_PTR(error) : NULL;
-	}
-
-	return filemap_read_page(iocb, filp, mapping, page);
+	if (!error)
+		error = filemap_read_page(iocb->ki_filp, mapping, page);
+	if (!error)
+		return page;
+	put_page(page);
+	if (error == -EEXIST || error == AOP_TRUNCATED_PAGE)
+		return NULL;
+	return ERR_PTR(error);
 }
 
 static int filemap_get_pages(struct kiocb *iocb, struct iov_iter *iter,