fs: __generic_file_splice_read retry lookup on AOP_TRUNCATED_PAGE
RFC PATCH v3 1/1 fs: __generic_file_splice_read retry lookup on AOP_TRUNCATED_PAGE
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.
--- fs/splice.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
fs/splice.c +3 -5
@@ -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 ; }
Signature · 1 line
2.4.3
Quoted 14 lines · Abhi Das
During testing, I discovered that
__generic_file_splice_read()returns 0 (EOF) whenaops->readpagefails withAOP_TRUNCATED_PAGEon 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_PAGEworks 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.
Quoted 6 lines
This version removes the thrice-retry loop and instead indefinitely retries lookups on
AOP_TRUNCATED_PAGEerrors from ->readpage.Signed-off-by Abhi Das <[email protected]>Cc Bob Peterson <[email protected]>Cc Jan Kara <[email protected]>
The patch looks good to me. You can add:
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
Quoted 36 lines
--- fs/splice.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)fs/splice.c +3
-5@@ -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 ; }Signature · 1 line
2.4.3
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR