On Wed 16-12-15 10:55:25, Abhijith Das wrote:
> > > @@ -439,14 +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) {
> > > error = 0;
> > > -
> > > + if (retries++ < 3)
> > > + goto retry_lookup;
> > > + }
> >
> > I don't like this retry-three-times loop. That is still leaving the
> > possibility of 0 return just much less likely (so it will lead to even
> > weirder and harded to debug failures). IMO we should just terminate the
> > loop like we did previously if spd.nr_pages > 0 and we retry indefinitely
> > if it is the first page that failed to read.
>
> The 3-retry loop was the thing I was unsure about too. With regards to the
> indefinite retry, I was wondering if there's some corner case where we might
> get into an infinite retry loop...
Well you have two options:
1) Return incorrect value from splice_read()
2) Retry indefinitely
Option two looks better to me. Also do_generic_file_read() behaves the same
way so mirroring the behavior in __generic_file_splice_read() makes sense.
> If we're doing the retry for the first page, why not for other pages too?
> Is it because we'd potentially be increasing the odds for an infinite
> loop and/or affecting performance by doing more lookups?
Yes, that was my thought. But seeing now that do_generic_file_read()
actually retries indefinitely for every page, I'd just do the same in
__generic_file_splice_read().
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
