diff mbox series

[14/17] mm/filemap: Restructure filemap_get_pages

Message ID 20201102184312.25926-15-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 
Avoid a goto, and by the time we get to calling filemap_update_page(),
we definitely have at least one page.

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

Comments

Kent Overstreet Nov. 2, 2020, 8:05 p.m. UTC | #1 
On Mon, Nov 02, 2020 at 06:43:09PM +0000, Matthew Wilcox (Oracle) wrote:
> Avoid a goto, and by the time we get to calling filemap_update_page(),
> we definitely have at least one page.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Kent Overstreet <kent.overstreet@gmail.com>
Christoph Hellwig Nov. 3, 2020, 7:57 a.m. UTC | #2 
On Mon, Nov 02, 2020 at 06:43:09PM +0000, Matthew Wilcox (Oracle) wrote:
> Avoid a goto, and by the time we get to calling filemap_update_page(),
> we definitely have at least one page.

I find the error handling flow hard to follow and the existing but
heavily touched naming of the nr_got variable and the find_pages label
not helpful.  I'd do the following on top of this patch:

diff --git a/mm/filemap.c b/mm/filemap.c
index f16b1eb03bcad0..3ef73a58ce9456 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2344,51 +2344,54 @@ static int filemap_get_pages(struct kiocb *iocb, struct iov_iter *iter,
 	pgoff_t index = iocb->ki_pos >> PAGE_SHIFT;
 	pgoff_t last_index = (iocb->ki_pos + iter->count + PAGE_SIZE-1) >> PAGE_SHIFT;
 	struct page *page;
-	int nr_got, err = 0;
+	int nr_pages, err = 0;
 
 	nr = min_t(unsigned long, last_index - index, nr);
-find_page:
+retry:
 	if (fatal_signal_pending(current))
 		return -EINTR;
 
-	nr_got = mapping_get_read_thps(mapping, index, nr, pages);
-	if (!nr_got) {
+	nr_pages = mapping_get_read_thps(mapping, index, nr, pages);
+	if (!nr_pages) {
 		if (iocb->ki_flags & IOCB_NOIO)
 			return -EAGAIN;
 		page_cache_sync_readahead(mapping, ra, filp, index,
 				last_index - index);
-		nr_got = mapping_get_read_thps(mapping, index, nr, pages);
+		nr_pages = mapping_get_read_thps(mapping, index, nr, pages);
 	}
-	if (!nr_got) {
+	if (!nr_pages) {
 		if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_WAITQ))
 			return -EAGAIN;
 		pages[0] = filemap_create_page(filp, mapping,
 				iocb->ki_pos >> PAGE_SHIFT);
 		if (!pages[0])
-			goto find_page;
+			goto retry;
 		if (IS_ERR(pages[0]))
 			return PTR_ERR(pages[0]);
 		return 1;
 	}
 
-	page = pages[nr_got - 1];
-	if (PageReadahead(page))
+	page = pages[nr_pages - 1];
+	if (PageReadahead(page)) {
 		err = filemap_readahead(iocb, filp, mapping, page, last_index);
-	if (!err && !PageUptodate(page))
+		if (err)
+			goto error;
+	}
+	if (!PageUptodate(page)) {
 		err = filemap_update_page(iocb, mapping, iter, page,
-				nr_got == 1);
+				nr_pages == 1);
+		if (err)
+			goto error;
+	}
 
-	if (err)
-		nr_got--;
-	if (likely(nr_got))
-		return nr_got;
-	if (err < 0)
-		return err;
-	err = 0;
-	/*
-	 * No pages and no error means we raced and should retry:
-	 */
-	goto find_page;
+	return nr_pages;
+
+error:
+	if (nr_pages > 1)
+		return nr_pages - 1;
+	if (err == AOP_TRUNCATED_PAGE)
+		goto retry;
+	return err;
 }
 
 /**
Matthew Wilcox Nov. 3, 2020, 2:46 p.m. UTC | #3 
On Tue, Nov 03, 2020 at 08:57:36AM +0100, Christoph Hellwig wrote:
> On Mon, Nov 02, 2020 at 06:43:09PM +0000, Matthew Wilcox (Oracle) wrote:
> > Avoid a goto, and by the time we get to calling filemap_update_page(),
> > we definitely have at least one page.
> 
> I find the error handling flow hard to follow and the existing but
> heavily touched naming of the nr_got variable and the find_pages label
> not helpful.  I'd do the following on top of this patch:

I've removed nr_got entirely in my current tree ...

diff --git a/mm/filemap.c b/mm/filemap.c
index 8264bcdb99f4..ea0cd0df638b 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2166,21 +2166,17 @@ static void shrink_readahead_size_eio(struct file_ra_state *ra)
 	ra->ra_pages /= 4;
 }
 
-static unsigned mapping_get_read_thps(struct address_space *mapping,
-		pgoff_t index, unsigned int nr_pages, struct page **pages)
+static void mapping_get_read_thps(struct address_space *mapping,
+		pgoff_t index, pgoff_t max, struct pagevec *pvec)
 {
 	XA_STATE(xas, &mapping->i_pages, index);
 	struct page *head;
-	unsigned int ret = 0;
-
-	if (unlikely(!nr_pages))
-		return 0;
 
 	rcu_read_lock();
 	for (head = xas_load(&xas); head; head = xas_next(&xas)) {
 		if (xas_retry(&xas, head))
 			continue;
-		if (xa_is_value(head))
+		if (xas.xa_index > max || xa_is_value(head))
 			break;
 		if (!page_cache_get_speculative(head))
 			goto retry;
@@ -2189,8 +2185,7 @@ static unsigned mapping_get_read_thps(struct address_space *mapping,
 		if (unlikely(head != xas_reload(&xas)))
 			goto put_page;
 
-		pages[ret++] = head;
-		if (ret == nr_pages)
+		if (!pagevec_add(pvec, head))
 			break;
 		if (!PageUptodate(head))
 			break;
@@ -2205,7 +2200,6 @@ static unsigned mapping_get_read_thps(struct address_space *mapping,
 		xas_reset(&xas);
 	}
 	rcu_read_unlock();
-	return ret;
 }
 
 static int filemap_read_page(struct file *file, struct address_space *mapping,
@@ -2343,52 +2337,53 @@ static int filemap_readahead(struct kiocb *iocb, struct file *file,
 }
 
 static int filemap_get_pages(struct kiocb *iocb, struct iov_iter *iter,
-		struct page **pages, unsigned int nr)
+		struct pagevec *pvec)
 {
 	struct file *filp = iocb->ki_filp;
 	struct address_space *mapping = filp->f_mapping;
 	struct file_ra_state *ra = &filp->f_ra;
 	pgoff_t index = iocb->ki_pos >> PAGE_SHIFT;
-	pgoff_t last_index = (iocb->ki_pos + iter->count + PAGE_SIZE-1) >> PAGE_SHIFT;
+	pgoff_t maxindex = DIV_ROUND_UP(iocb->ki_pos + iter->count, PAGE_SIZE);
 	struct page *page;
-	int nr_got, err = 0;
+	int err = 0;
 
-	nr = min_t(unsigned long, last_index - index, nr);
 find_page:
 	if (fatal_signal_pending(current))
 		return -EINTR;
 
-	nr_got = mapping_get_read_thps(mapping, index, nr, pages);
-	if (!nr_got) {
+	pagevec_init(pvec);
+	mapping_get_read_thps(mapping, index, maxindex, pvec);
+	if (!pagevec_count(pvec)) {
 		if (iocb->ki_flags & IOCB_NOIO)
 			return -EAGAIN;
 		page_cache_sync_readahead(mapping, ra, filp, index,
-				last_index - index);
-		nr_got = mapping_get_read_thps(mapping, index, nr, pages);
+				maxindex - index);
+		mapping_get_read_thps(mapping, index, maxindex, pvec);
 	}
-	if (!nr_got) {
+	if (!pagevec_count(pvec)) {
 		if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_WAITQ))
 			return -EAGAIN;
-		pages[0] = filemap_create_page(filp, mapping,
+		page = filemap_create_page(filp, mapping,
 				iocb->ki_pos >> PAGE_SHIFT);
-		if (!pages[0])
+		if (!page)
 			goto find_page;
-		if (IS_ERR(pages[0]))
-			return PTR_ERR(pages[0]);
-		return 1;
+		if (IS_ERR(page))
+			return PTR_ERR(page);
+		pagevec_add(pvec, page);
+		return 0;
 	}
 
-	page = pages[nr_got - 1];
+	page = pvec->pages[pagevec_count(pvec) - 1];
 	if (PageReadahead(page))
-		err = filemap_readahead(iocb, filp, mapping, page, last_index);
+		err = filemap_readahead(iocb, filp, mapping, page, maxindex);
 	if (!err && !PageUptodate(page))
 		err = filemap_update_page(iocb, mapping, iter, page,
-				nr_got == 1);
+				pagevec_count(pvec) == 1);
 
 	if (err)
-		nr_got--;
-	if (likely(nr_got))
-		return nr_got;
+		pvec->nr--;
+	if (likely(pagevec_count(pvec)))
+		return 0;
 	if (err < 0)
 		return err;
 	err = 0;
@@ -2418,11 +2413,8 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
 	struct file_ra_state *ra = &filp->f_ra;
 	struct address_space *mapping = filp->f_mapping;
 	struct inode *inode = mapping->host;
-	struct page *pages_onstack[PAGEVEC_SIZE], **pages = NULL;
-	unsigned int nr_pages = min_t(unsigned int, 512,
-			((iocb->ki_pos + iter->count + PAGE_SIZE - 1) >> PAGE_SHIFT) -
-			(iocb->ki_pos >> PAGE_SHIFT));
-	int i, pg_nr, error = 0;
+	struct pagevec pvec;
+	int i, error = 0;
 	bool writably_mapped;
 	loff_t isize, end_offset;
 
@@ -2430,14 +2422,6 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
 		return 0;
 	iov_iter_truncate(iter, inode->i_sb->s_maxbytes);
 
-	if (nr_pages > ARRAY_SIZE(pages_onstack))
-		pages = kmalloc_array(nr_pages, sizeof(void *), GFP_KERNEL);
-
-	if (!pages) {
-		pages = pages_onstack;
-		nr_pages = min_t(unsigned int, nr_pages, ARRAY_SIZE(pages_onstack));
-	}
-
 	do {
 		cond_resched();
 
@@ -2449,12 +2433,9 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
 		if ((iocb->ki_flags & IOCB_WAITQ) && already_read)
 			iocb->ki_flags |= IOCB_NOWAIT;
 
-		i = 0;
-		pg_nr = filemap_get_pages(iocb, iter, pages, nr_pages);
-		if (pg_nr < 0) {
-			error = pg_nr;
+		error = filemap_get_pages(iocb, iter, &pvec);
+		if (error < 0)
 			break;
-		}
 
 		/*
 		 * i_size must be checked after we know the pages are Uptodate.
@@ -2467,13 +2448,8 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
 		isize = i_size_read(inode);
 		if (unlikely(iocb->ki_pos >= isize))
 			goto put_pages;
-
 		end_offset = min_t(loff_t, isize, iocb->ki_pos + iter->count);
 
-		while ((iocb->ki_pos >> PAGE_SHIFT) + pg_nr >
-		       (end_offset + PAGE_SIZE - 1) >> PAGE_SHIFT)
-			put_page(pages[--pg_nr]);
-
 		/*
 		 * Once we start copying data, we don't want to be touching any
 		 * cachelines that might be contended:
@@ -2486,18 +2462,20 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
 		 */
 		if (iocb->ki_pos >> PAGE_SHIFT !=
 		    ra->prev_pos >> PAGE_SHIFT)
-			mark_page_accessed(pages[0]);
-		for (i = 1; i < pg_nr; i++)
-			mark_page_accessed(pages[i]);
+			mark_page_accessed(pvec.pages[0]);
 
-		for (i = 0; i < pg_nr; i++) {
-			struct page *page = pages[i];
+		for (i = 0; i < pagevec_count(&pvec); i++) {
+			struct page *page = pvec.pages[i];
 			size_t page_size = thp_size(page);
 			size_t offset = iocb->ki_pos & (page_size - 1);
 			size_t bytes = min_t(loff_t, end_offset - iocb->ki_pos,
 					     page_size - offset);
 			size_t copied;
 
+			if (end_offset < page_offset(page))
+				break;
+			if (i > 0)
+				mark_page_accessed(page);
 			/*
 			 * If users can be writing to this page using arbitrary
 			 * virtual addresses, take care about potential aliasing
@@ -2522,15 +2500,11 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
 			}
 		}
 put_pages:
-		for (i = 0; i < pg_nr; i++)
-			put_page(pages[i]);
+		pagevec_release(&pvec);
 	} while (iov_iter_count(iter) && iocb->ki_pos < isize && !error);
 
 	file_accessed(filp);
 
-	if (pages != pages_onstack)
-		kfree(pages);
-
 	return already_read ? already_read : error;
 }
 EXPORT_SYMBOL_GPL(filemap_read);

I like a lot of the restructuring you did there.  I'll incorporate it.
Christoph Hellwig Nov. 3, 2020, 3:29 p.m. UTC | #4 
On Tue, Nov 03, 2020 at 02:46:19PM +0000, Matthew Wilcox wrote:
> On Tue, Nov 03, 2020 at 08:57:36AM +0100, Christoph Hellwig wrote:
> > On Mon, Nov 02, 2020 at 06:43:09PM +0000, Matthew Wilcox (Oracle) wrote:
> > > Avoid a goto, and by the time we get to calling filemap_update_page(),
> > > we definitely have at least one page.
> > 
> > I find the error handling flow hard to follow and the existing but
> > heavily touched naming of the nr_got variable and the find_pages label
> > not helpful.  I'd do the following on top of this patch:
> 
> I've removed nr_got entirely in my current tree ...

Even better.  The pagevec usage looks pretty nice!

> +static void mapping_get_read_thps(struct address_space *mapping,

Maybe call this pagevec_get_read_thps?

> +	pgoff_t maxindex = DIV_ROUND_UP(iocb->ki_pos + iter->count, PAGE_SIZE);
>  	struct page *page;
> +	int err = 0;
>  
>  find_page:
>  	if (fatal_signal_pending(current))
>  		return -EINTR;
>  
> +	pagevec_init(pvec);
> +	mapping_get_read_thps(mapping, index, maxindex, pvec);
> +	if (!pagevec_count(pvec)) {
>  		if (iocb->ki_flags & IOCB_NOIO)
>  			return -EAGAIN;
>  		page_cache_sync_readahead(mapping, ra, filp, index,
> +				maxindex - index);
> +		mapping_get_read_thps(mapping, index, maxindex, pvec);
>  	}
> +	if (!pagevec_count(pvec)) {
>  		if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_WAITQ))
>  			return -EAGAIN;
> +		page = filemap_create_page(filp, mapping,
>  				iocb->ki_pos >> PAGE_SHIFT);
> +		if (!page)
>  			goto find_page;
> +		if (IS_ERR(page))
> +			return PTR_ERR(page);
> +		pagevec_add(pvec, page);
> +		return 0;

I'd pass the pagevec to filemap_create_page and just return an errno,
which should be a little easier to follow.

> -	page = pages[nr_got - 1];
> +	page = pvec->pages[pagevec_count(pvec) - 1];

I wonder if a pagevec_last_page() helper would be neat for things
like this? (might be a bit of over engineering..)
diff mbox series

Patch

diff --git a/mm/filemap.c b/mm/filemap.c
index 0ae8305ccb97..f16b1eb03bca 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2343,6 +2343,7 @@  static int filemap_get_pages(struct kiocb *iocb, struct iov_iter *iter,
 	struct file_ra_state *ra = &filp->f_ra;
 	pgoff_t index = iocb->ki_pos >> PAGE_SHIFT;
 	pgoff_t last_index = (iocb->ki_pos + iter->count + PAGE_SIZE-1) >> PAGE_SHIFT;
+	struct page *page;
 	int nr_got, err = 0;
 
 	nr = min_t(unsigned long, last_index - index, nr);
@@ -2351,15 +2352,13 @@  static int filemap_get_pages(struct kiocb *iocb, struct iov_iter *iter,
 		return -EINTR;
 
 	nr_got = mapping_get_read_thps(mapping, index, nr, pages);
-	if (nr_got)
-		goto got_pages;
-
-	if (iocb->ki_flags & IOCB_NOIO)
-		return -EAGAIN;
-
-	page_cache_sync_readahead(mapping, ra, filp, index, last_index - index);
-
-	nr_got = mapping_get_read_thps(mapping, index, nr, pages);
+	if (!nr_got) {
+		if (iocb->ki_flags & IOCB_NOIO)
+			return -EAGAIN;
+		page_cache_sync_readahead(mapping, ra, filp, index,
+				last_index - index);
+		nr_got = mapping_get_read_thps(mapping, index, nr, pages);
+	}
 	if (!nr_got) {
 		if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_WAITQ))
 			return -EAGAIN;
@@ -2371,20 +2370,16 @@  static int filemap_get_pages(struct kiocb *iocb, struct iov_iter *iter,
 			return PTR_ERR(pages[0]);
 		return 1;
 	}
-got_pages:
-	if (nr_got > 0) {
-		struct page *page = pages[nr_got - 1];
 
-		if (PageReadahead(page))
-			err = filemap_readahead(iocb, filp, mapping, page,
-					last_index);
-		if (!err && !PageUptodate(page))
-			err = filemap_update_page(iocb, mapping, iter, page,
-					nr_got == 1);
-		if (err)
-			nr_got--;
-	}
+	page = pages[nr_got - 1];
+	if (PageReadahead(page))
+		err = filemap_readahead(iocb, filp, mapping, page, last_index);
+	if (!err && !PageUptodate(page))
+		err = filemap_update_page(iocb, mapping, iter, page,
+				nr_got == 1);
 
+	if (err)
+		nr_got--;
 	if (likely(nr_got))
 		return nr_got;
 	if (err < 0)