[11/13] mm: streamline the partially uptodate checks in filemap_make_page_uptodate

Commit Message

Christoph Hellwig Oct. 31, 2020, 9 a.m. UTC

Unwind the goto mess a bit.

Signed-off-by: Christoph Hellwig <[e-mail]>
---
 mm/filemap.c | 42 +++++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

Comments

Matthew Wilcox Nov. 1, 2020, 11:23 a.m. UTC | #1

On Sat, Oct 31, 2020 at 10:00:02AM +0100, Christoph Hellwig wrote:
> +	if (mapping->host->i_blkbits <= PAGE_SHIFT &&
> +	    mapping->a_ops->is_partially_uptodate &&
> +	    !iov_iter_is_pipe(iter) &&
> +	    trylock_page(page)) {
> +		loff_t pos = max(iocb->ki_pos, (loff_t)pg_index << PAGE_SHIFT);

I think the cure is worse than the disease here ;-)

Fortunately, I simplified this here:
https://git.infradead.org/users/willy/pagecache.git/commitdiff/c6b5b2540b6db91d3c8928c8ed1b5d72a402215a

by getting the page lock earlier (or dropping the reference to the page,
waiting for it to become unlocked and starting the lookup again)

Patch

diff --git a/mm/filemap.c b/mm/filemap.c
index 000f75cd359d1c..904b0a4fb9e008 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2218,7 +2218,6 @@  static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	struct address_space *mapping = file->f_mapping;
 	loff_t last = iocb->ki_pos + iter->count;
 	pgoff_t last_index = (last + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	loff_t pos = max(iocb->ki_pos, (loff_t)pg_index << PAGE_SHIFT);
 	int error;
 
 	if (PageReadahead(page)) {
@@ -2251,32 +2250,22 @@  static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	if (PageUptodate(page))
 		return 0;
 
-	if (mapping->host->i_blkbits == PAGE_SHIFT ||
-	    !mapping->a_ops->is_partially_uptodate)
-		goto page_not_up_to_date;
-	/* pipes can't handle partially uptodate pages */
-	if (unlikely(iov_iter_is_pipe(iter)))
-		goto page_not_up_to_date;
-	if (!trylock_page(page))
-		goto page_not_up_to_date;
-	/* Did it get truncated before we got the lock? */
-	if (!page->mapping)
-		goto page_not_up_to_date_locked;
-	if (!mapping->a_ops->is_partially_uptodate(page, pos & ~PAGE_MASK,
-			last - pos))
-		goto page_not_up_to_date_locked;
-
-unlock_page:
-	unlock_page(page);
-	return 0;
+	if (mapping->host->i_blkbits <= PAGE_SHIFT &&
+	    mapping->a_ops->is_partially_uptodate &&
+	    !iov_iter_is_pipe(iter) &&
+	    trylock_page(page)) {
+		loff_t pos = max(iocb->ki_pos, (loff_t)pg_index << PAGE_SHIFT);
 
-page_not_up_to_date:
-	/* Get exclusive access to the page ... */
-	error = lock_page_for_iocb(iocb, page);
-	if (unlikely(error))
-		return error;
+		if (page->mapping &&
+		    mapping->a_ops->is_partially_uptodate(page,
+				pos & ~PAGE_MASK, last - pos))
+			goto unlock_page;
+	} else {
+		error = lock_page_for_iocb(iocb, page);
+		if (unlikely(error))
+			return error;
+	}
 
-page_not_up_to_date_locked:
 	/* Did it get truncated before we got the lock? */
 	if (!page->mapping) {
 		unlock_page(page);
@@ -2287,6 +2276,9 @@  static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	if (PageUptodate(page))
 		goto unlock_page;
 	return filemap_readpage(iocb, page);
+unlock_page:
+	unlock_page(page);
+	return 0;
 }
 
 static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,