[RFC v3 PATCH] fs: __generic_file_splice_read retry lookup on AOP_TRUNCATED_PAGE

[RFC v3 PATCH] fs: __generic_file_splice_read retry lookup on AOP_TRUNCATED_PAGE

[Abhi Das]

During testing, I discovered that __generic_file_splice_read() returns
0 (EOF) when aops->readpage fails with AOP_TRUNCATED_PAGE on the first
page of a single/multi-page splice read operation. This EOF return code
causes the userspace test to (correctly) report a zero-length read error
when it was expecting otherwise.

The current strategy of returning a partial non-zero read when ->readpage
returns AOP_TRUNCATED_PAGE works only when the failed page is not the
first of the lot being processed.

This patch attempts to retry lookup and call ->readpage again on pages
that had previously failed with AOP_TRUNCATED_PAGE. With this patch, my
tests pass and I haven't noticed any unwanted side effects.

This version removes the thrice-retry loop and instead indefinitely
retries lookups on AOP_TRUNCATED_PAGE errors from ->readpage.

Signed-off-by: Abhi Das <adas@redhat.com>
Cc: Bob Peterson <rpeterso@redhat.com>
Cc: Jan Kara <jack@suse.com>
---
 fs/splice.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/splice.c b/fs/splice.c
index 801c21c..277df71 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -415,6 +415,7 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
 			 */
 			if (!page->mapping) {
 				unlock_page(page);
+retry_lookup:
 				page = find_or_create_page(mapping, index,
 						mapping_gfp_mask(mapping));
 
@@ -439,13 +440,10 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
 			error = mapping->a_ops->readpage(in, page);
 			if (unlikely(error)) {
 				/*
-				 * We really should re-lookup the page here,
-				 * but it complicates things a lot. Instead
-				 * lets just do what we already stored, and
-				 * we'll get it the next time we are called.
+				 * Re-lookup the page
 				 */
 				if (error == AOP_TRUNCATED_PAGE)
-					error = 0;
+					goto retry_lookup;
 
 				break;
 			}
-- 
2.4.3

Re: [RFC v3 PATCH] fs: __generic_file_splice_read retry lookup on AOP_TRUNCATED_PAGE

[Jan Kara]

On Wed 16-12-15 13:02:52, Abhi Das wrote:
> During testing, I discovered that __generic_file_splice_read() returns
> 0 (EOF) when aops->readpage fails with AOP_TRUNCATED_PAGE on the first
> page of a single/multi-page splice read operation. This EOF return code
> causes the userspace test to (correctly) report a zero-length read error
> when it was expecting otherwise.
> 
> The current strategy of returning a partial non-zero read when ->readpage
> returns AOP_TRUNCATED_PAGE works only when the failed page is not the
> first of the lot being processed.
> 
> This patch attempts to retry lookup and call ->readpage again on pages
> that had previously failed with AOP_TRUNCATED_PAGE. With this patch, my
> tests pass and I haven't noticed any unwanted side effects.
> 
> This version removes the thrice-retry loop and instead indefinitely
> retries lookups on AOP_TRUNCATED_PAGE errors from ->readpage.
> 
> Signed-off-by: Abhi Das <adas@redhat.com>
> Cc: Bob Peterson <rpeterso@redhat.com>
> Cc: Jan Kara <jack@suse.com>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

BTW, you should send the patch to Al Viro as he is the VFS maintainer so he
should merge the patch. It might be also useful to mention in the changelog
that do_generic_file_read() behaves the same way.

								Honza
> ---
>  fs/splice.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/splice.c b/fs/splice.c
> index 801c21c..277df71 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -415,6 +415,7 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
>  			 */
>  			if (!page->mapping) {
>  				unlock_page(page);
> +retry_lookup:
>  				page = find_or_create_page(mapping, index,
>  						mapping_gfp_mask(mapping));
>  
> @@ -439,13 +440,10 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
>  			error = mapping->a_ops->readpage(in, page);
>  			if (unlikely(error)) {
>  				/*
> -				 * We really should re-lookup the page here,
> -				 * but it complicates things a lot. Instead
> -				 * lets just do what we already stored, and
> -				 * we'll get it the next time we are called.
> +				 * Re-lookup the page
>  				 */
>  				if (error == AOP_TRUNCATED_PAGE)
> -					error = 0;
> +					goto retry_lookup;
>  
>  				break;
>  			}
> -- 
> 2.4.3
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR