diff mbox series

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

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

Commit Message

Matthew Wilcox Nov. 4, 2020, 8:42 p.m. UTC 
Use AOP_TRUNCATED_PAGE to indicate that no error occurred, but the
page we looked up is no longer valid.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Kent Overstreet <kent.overstreet@gmail.com>
---
 mm/filemap.c | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

Comments

Christoph Hellwig Nov. 6, 2020, 8:14 a.m. UTC | #1 
On Wed, Nov 04, 2020 at 08:42:12PM +0000, Matthew Wilcox (Oracle) wrote:
> Use AOP_TRUNCATED_PAGE to indicate that no error occurred, but the
> page we looked up is no longer valid.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Reviewed-by: Kent Overstreet <kent.overstreet@gmail.com>
> ---
>  mm/filemap.c | 42 +++++++++++++++++++-----------------------
>  1 file changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 25945fefdd39..93c054f51677 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2231,24 +2231,21 @@ static int filemap_read_page(struct file *file, struct address_space *mapping,
>  	return error;
>  }
>  
> +static int filemap_update_page(struct kiocb *iocb,
> +		struct address_space *mapping, struct iov_iter *iter,
> +		struct page *page, loff_t pos, loff_t count)
>  {
>  	struct inode *inode = mapping->host;
>  	int error;
>  
>  	if (iocb->ki_flags & IOCB_WAITQ) {
>  		error = lock_page_async(page, iocb->ki_waitq);
> +		if (error)
> +			goto error;
>  	} else {
>  		if (!trylock_page(page)) {
>  			put_and_wait_on_page_locked(page, TASK_KILLABLE);
> +			return AOP_TRUNCATED_PAGE;
>  		}
>  	}
>  
> @@ -2267,25 +2264,24 @@ static struct page *filemap_update_page(struct kiocb *iocb, struct file *filp,
>  		goto readpage;
>  uptodate:
>  	unlock_page(page);
> +	return 0;
>  
>  readpage:
>  	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;
>  truncated:
>  	unlock_page(page);
>  	put_page(page);
> +	return AOP_TRUNCATED_PAGE;

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:

> +			err = filemap_update_page(iocb, mapping, iter, page,
>  					pg_pos, pg_count);
> +			if (err)
>  				pvec->nr--;

But otherwise this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Christoph Hellwig Nov. 6, 2020, 8:37 a.m. UTC | #2 
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;
Matthew Wilcox Nov. 9, 2020, 1:29 p.m. UTC | #3 
On Fri, Nov 06, 2020 at 09:37:46AM +0100, Christoph Hellwig wrote:
>  
> -	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;
>  }

It works out to be a little more complicated than that because
filemap_read_page() can also return AOP_TRUNCATED_PAGE.  Here's what I
currently have:

        if (!page->mapping)
                goto truncated;

        error = 0;
        if (filemap_range_uptodate(iocb, mapping, iter, page))
                goto unlock;

        error = -EAGAIN;
        if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ))
                goto unlock;

        error = filemap_read_page(iocb->ki_filp, mapping, page);
        if (error == AOP_TRUNCATED_PAGE)
                put_page(page);
        return error;
truncated:
        error = AOP_TRUNCATED_PAGE;
        put_page(page);
unlock:
        unlock_page(page);
        return error;
}
diff mbox series

Patch

diff --git a/mm/filemap.c b/mm/filemap.c
index 25945fefdd39..93c054f51677 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2231,24 +2231,21 @@  static int filemap_read_page(struct file *file, struct address_space *mapping,
 	return error;
 }
 
-static struct page *filemap_update_page(struct kiocb *iocb, struct file *filp,
-		struct iov_iter *iter, struct page *page, loff_t pos,
-		loff_t count)
+static int filemap_update_page(struct kiocb *iocb,
+		struct address_space *mapping, struct iov_iter *iter,
+		struct page *page, loff_t pos, loff_t count)
 {
-	struct address_space *mapping = filp->f_mapping;
 	struct inode *inode = mapping->host;
 	int error;
 
 	if (iocb->ki_flags & IOCB_WAITQ) {
 		error = lock_page_async(page, iocb->ki_waitq);
-		if (error) {
-			put_page(page);
-			return ERR_PTR(error);
-		}
+		if (error)
+			goto error;
 	} else {
 		if (!trylock_page(page)) {
 			put_and_wait_on_page_locked(page, TASK_KILLABLE);
-			return NULL;
+			return AOP_TRUNCATED_PAGE;
 		}
 	}
 
@@ -2267,25 +2264,24 @@  static struct page *filemap_update_page(struct kiocb *iocb, struct file *filp,
 		goto readpage;
 uptodate:
 	unlock_page(page);
-	return page;
+	return 0;
 
 readpage:
 	if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ)) {
 		unlock_page(page);
-		put_page(page);
-		return ERR_PTR(-EAGAIN);
+		error = -EAGAIN;
+	} else {
+		error = filemap_read_page(iocb->ki_filp, mapping, page);
+		if (!error)
+			return 0;
 	}
-	error = filemap_read_page(iocb->ki_filp, mapping, page);
-	if (!error)
-		return page;
+error:
 	put_page(page);
-	if (error == AOP_TRUNCATED_PAGE)
-		return NULL;
-	return ERR_PTR(error);
+	return error;
 truncated:
 	unlock_page(page);
 	put_page(page);
-	return NULL;
+	return AOP_TRUNCATED_PAGE;
 }
 
 static int filemap_create_page(struct file *file,
@@ -2380,18 +2376,18 @@  static int filemap_get_pages(struct kiocb *iocb, struct iov_iter *iter,
 				goto err;
 			}
 
-			page = filemap_update_page(iocb, filp, iter, page,
+			err = filemap_update_page(iocb, mapping, iter, page,
 					pg_pos, pg_count);
-			if (IS_ERR_OR_NULL(page)) {
+			if (err)
 				pvec->nr--;
-				err = PTR_ERR_OR_ZERO(page);
-			}
 		}
 	}
 
 err:
 	if (likely(pvec->nr))
 		return 0;
+	if (err == AOP_TRUNCATED_PAGE)
+		goto find_page;
 	if (err)
 		return err;
 	/*