| Message ID | 1450143229-40537-1-git-send-email-adas@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Abhi, Comment below. ----- Original Message ----- > 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. > > Signed-off-by: Abhi Das <adas@redhat.com> > --- > fs/splice.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/fs/splice.c b/fs/splice.c > index 801c21c..c424d44 100644 > --- a/fs/splice.c > +++ b/fs/splice.c > @@ -387,6 +387,7 @@ __generic_file_splice_read(struct file *in, loff_t *ppos, > spd.nr_pages = 0; > for (page_nr = 0; page_nr < nr_pages; page_nr++) { > unsigned int this_len; > + int retries = 0; > > if (!len) > break; > @@ -415,6 +416,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 +441,13 @@ __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) > + if (error == AOP_TRUNCATED_PAGE) { > + if (retries++ < 3) > + goto retry_lookup; > error = 0; I have no problem with doing the retry. However, I think we should probably set error = 0 before we decide whether to retry here. Otherwise, if error is AOP_TRUNCATED_PAGE, it might jump to retry_lookup, but then discover that the page is PageUptodate(), goto fill_it with error still set to AOP_TRUNCATED_PAGE, then end up returning that to the caller. Regards, Bob Peterson Red Hat File Systems -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/splice.c b/fs/splice.c index 801c21c..c424d44 100644 --- a/fs/splice.c +++ b/fs/splice.c @@ -387,6 +387,7 @@ __generic_file_splice_read(struct file *in, loff_t *ppos, spd.nr_pages = 0; for (page_nr = 0; page_nr < nr_pages; page_nr++) { unsigned int this_len; + int retries = 0; if (!len) break; @@ -415,6 +416,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 +441,13 @@ __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) + if (error == AOP_TRUNCATED_PAGE) { + if (retries++ < 3) + goto retry_lookup; error = 0; + } break; }
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. Signed-off-by: Abhi Das <adas@redhat.com> --- fs/splice.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)