Date: Wed, 16 Dec 2015 17:47:45 +0100
From: Jan Kara <jack@suse.cz>
To: Abhijith Das <adas@redhat.com>
Cc: Jan Kara <jack@suse.cz>, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, Bob Peterson <rpeterso@redhat.com>
Subject: Re: [RFC v2 PATCH] fs: __generic_file_splice_read retry lookup on
 AOP_TRUNCATED_PAGE
Message-ID: <20151216164745.GA29214@quack.suse.cz>
References: <1450194205-44587-1-git-send-email-adas@redhat.com>
 <20151216153404.GC16918@quack.suse.cz>
 <1471602670.34527814.1450281325831.JavaMail.zimbra@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1471602670.34527814.1450281325831.JavaMail.zimbra@redhat.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2187
Lines: 55

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@suse.com>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/