Messages in this thread Patch in this message | | | Date | Tue, 15 Dec 2015 08:45:20 -0500 (EST) | From | Bob Peterson <> | Subject | Re: [RFC PATCH] fs: __generic_file_splice_read retry lookup on AOP_TRUNCATED_PAGE |
| |
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
| |