Ocfs2 devel - Feb 2020 - [PATCH v7 21/24] iomap: Restructure iomap_readpages_actor

From: "Matthew Wilcox (Oracle)" <willy at infradead.org>

By putting the 'have we reached the end of the page' condition at the
end
of the loop instead of the beginning, we can remove the 'submit the last
page' code from iomap_readpages().  Also check that iomap_readpage_actor()
didn't return 0, which would lead to an endless loop.

Signed-off-by: Matthew Wilcox (Oracle) <willy at infradead.org>
---
 fs/iomap/buffered-io.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index cb3511eb152a..31899e6cb0f8 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -400,15 +400,9 @@ iomap_readpages_actor(struct inode *inode, loff_t pos,
loff_t length,
 		void *data, struct iomap *iomap, struct iomap *srcmap)
 {
 	struct iomap_readpage_ctx *ctx = data;
-	loff_t done, ret;
-
-	for (done = 0; done < length; done += ret) {
-		if (ctx->cur_page && offset_in_page(pos + done) == 0) {
-			if (!ctx->cur_page_in_bio)
-				unlock_page(ctx->cur_page);
-			put_page(ctx->cur_page);
-			ctx->cur_page = NULL;
-		}
+	loff_t ret, done = 0;
+
+	while (done < length) {
 		if (!ctx->cur_page) {
 			ctx->cur_page = iomap_next_page(inode, ctx->pages,
 					pos, length, &done);
@@ -418,6 +412,20 @@ iomap_readpages_actor(struct inode *inode, loff_t pos,
loff_t length,
 		}
 		ret = iomap_readpage_actor(inode, pos + done, length - done,
 				ctx, iomap, srcmap);
+		done += ret;
+
+		/* Keep working on a partial page */
+		if (ret && offset_in_page(pos + done))
+			continue;
+
+		if (!ctx->cur_page_in_bio)
+			unlock_page(ctx->cur_page);
+		put_page(ctx->cur_page);
+		ctx->cur_page = NULL;
+
+		/* Don't loop forever if we made no progress */
+		if (WARN_ON(!ret))
+			break;
 	}
 
 	return done;
@@ -451,11 +459,7 @@ iomap_readpages(struct address_space *mapping, struct
list_head *pages,
 done:
 	if (ctx.bio)
 		submit_bio(ctx.bio);
-	if (ctx.cur_page) {
-		if (!ctx.cur_page_in_bio)
-			unlock_page(ctx.cur_page);
-		put_page(ctx.cur_page);
-	}
+	BUG_ON(ctx.cur_page);
 
 	/*
 	 * Check that we didn't lose a page due to the arcance calling
-- 
2.25.0

On Wed, Feb 19, 2020 at 01:01:00PM -0800, Matthew Wilcox
wrote:
> From: "Matthew Wilcox (Oracle)" <willy at infradead.org>
> 
> By putting the 'have we reached the end of the page' condition at
the end
> of the loop instead of the beginning, we can remove the 'submit the
last
> page' code from iomap_readpages().  Also check that
iomap_readpage_actor()
> didn't return 0, which would lead to an endless loop.
I'm obviously biassed a I wrote the original code, but I find the new
very much harder to understand (not that the previous one was easy, this
is tricky code..).

On Wed, Feb 19, 2020 at 01:01:00PM -0800, Matthew Wilcox
wrote:
> From: "Matthew Wilcox (Oracle)" <willy at infradead.org>
> 
> By putting the 'have we reached the end of the page' condition at
the end
> of the loop instead of the beginning, we can remove the 'submit the
last
> page' code from iomap_readpages().  Also check that
iomap_readpage_actor()
> didn't return 0, which would lead to an endless loop.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy at infradead.org>
> ---
>  fs/iomap/buffered-io.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index cb3511eb152a..31899e6cb0f8 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -400,15 +400,9 @@ iomap_readpages_actor(struct inode *inode, loff_t pos,
loff_t length,
>  		void *data, struct iomap *iomap, struct iomap *srcmap)
>  {
>  	struct iomap_readpage_ctx *ctx = data;
> -	loff_t done, ret;
> -
> -	for (done = 0; done < length; done += ret) {
> -		if (ctx->cur_page && offset_in_page(pos + done) == 0) {
> -			if (!ctx->cur_page_in_bio)
> -				unlock_page(ctx->cur_page);
> -			put_page(ctx->cur_page);
> -			ctx->cur_page = NULL;
> -		}
> +	loff_t ret, done = 0;
> +
> +	while (done < length) {
>  		if (!ctx->cur_page) {
>  			ctx->cur_page = iomap_next_page(inode, ctx->pages,
>  					pos, length, &done);
> @@ -418,6 +412,20 @@ iomap_readpages_actor(struct inode *inode, loff_t pos,
loff_t length,
>  		}
>  		ret = iomap_readpage_actor(inode, pos + done, length - done,
>  				ctx, iomap, srcmap);
> +		done += ret;
> +
> +		/* Keep working on a partial page */
> +		if (ret && offset_in_page(pos + done))
> +			continue;
> +
> +		if (!ctx->cur_page_in_bio)
> +			unlock_page(ctx->cur_page);
> +		put_page(ctx->cur_page);
> +		ctx->cur_page = NULL;
> +
> +		/* Don't loop forever if we made no progress */
> +		if (WARN_ON(!ret))
> +			break;
>  	}
>  
>  	return done;
> @@ -451,11 +459,7 @@ iomap_readpages(struct address_space *mapping, struct
list_head *pages,
>  done:
>  	if (ctx.bio)
>  		submit_bio(ctx.bio);
> -	if (ctx.cur_page) {
> -		if (!ctx.cur_page_in_bio)
> -			unlock_page(ctx.cur_page);
> -		put_page(ctx.cur_page);
> -	}
> +	BUG_ON(ctx.cur_page);
Whoah, is the system totally unrecoverably hosed at this point?

I get that this /shouldn't/ happen, but should we somehow end up with a
page here, are we unable either to release it or even just leak it?  I'd
have thought a WARN_ON would be just fine here.

--D
>  
>  	/*
>  	 * Check that we didn't lose a page due to the arcance calling
> -- 
> 2.25.0
>