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

[Jan Kara]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/