Re: [PATCH 2/7] mm: Rewrite shmem_seek_hole_data

[Matthew Wilcox]

On Thu, Aug 20, 2020 at 07:45:46PM +0300, Mike Rapoport wrote:
> > - * llseek SEEK_DATA or SEEK_HOLE through the page cache.
> > + * llseek SEEK_DATA or SEEK_HOLE through the page cache.  We don't need
> > + * to get a reference on the page because this interface is racy anyway.
> > + * The page we find will have had the state at some point.
> 
> For my non-native ear "will have had" is too complex ;-)

Fair!  How about simply "The page we find did have the state at some point".

But now I'm thinking about it some more, and I'm not so sure of it now.
What if we put a page in the cache that was !Uptodate, then we do a
lookup, find this pointer, then the page is removed from the cache,
then it's allocated somewhere else, marked Uptodate, and now we're
scheduled back in and find it's Uptodate, when there was never a page
at this location that was Uptodate?

So maybe I have to retract this patch after all.

> >   */
> >  static pgoff_t shmem_seek_hole_data(struct address_space *mapping,
> >  				    pgoff_t index, pgoff_t end, int whence)
> >  {
> > +	XA_STATE(xas, &mapping->i_pages, index);
> >  	struct page *page;
> > -	struct pagevec pvec;
> > -	pgoff_t indices[PAGEVEC_SIZE];
> > -	bool done = false;
> > -	int i;
> >  
> > -	pagevec_init(&pvec);
> > -	pvec.nr = 1;		/* start small: we may be there already */
> > -	while (!done) {
> > -		pvec.nr = find_get_entries(mapping, index,
> > -					pvec.nr, pvec.pages, indices);
> > -		if (!pvec.nr) {
> > -			if (whence == SEEK_DATA)
> > -				index = end;
> > -			break;
> > +	rcu_read_lock();
> > +	if (whence == SEEK_DATA) {
> > +		for (;;) {
> > +			page = xas_find(&xas, end);
> > +			if (xas_retry(&xas, page))
> > +				continue;
> > +			if (!page || xa_is_value(page) || PageUptodate(page))
> > +				break;
> >  		}
> > -		for (i = 0; i < pvec.nr; i++, index++) {
> > -			if (index < indices[i]) {
> > -				if (whence == SEEK_HOLE) {
> > -					done = true;
> > -					break;
> > -				}
> > -				index = indices[i];
> > -			}
> > -			page = pvec.pages[i];
> > -			if (page && !xa_is_value(page)) {
> > -				if (!PageUptodate(page))
> > -					page = NULL;
> > -			}
> > -			if (index >= end ||
> > -			    (page && whence == SEEK_DATA) ||
> > -			    (!page && whence == SEEK_HOLE)) {
> > -				done = true;
> > +	} else /* SEEK_HOLE */ {
> > +		for (;;) {
> > +			page = xas_next(&xas);
> > +			if (xas_retry(&xas, page))
> > +				continue;
> > +			if (!xa_is_value(page) &&
> > +					(!page || !PageUptodate(page)))
> > +				break;
> > +			if (xas.xa_index >= end)
> >  				break;
> > -			}
> >  		}
> > -		pagevec_remove_exceptionals(&pvec);
> > -		pagevec_release(&pvec);
> > -		pvec.nr = PAGEVEC_SIZE;
> > -		cond_resched();
> >  	}
> > -	return index;
> > +	rcu_read_unlock();
> > +
> > +	return xas.xa_index;
> >  }
> >  
> >  static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence)
> > -- 
> > 2.28.0
> > 
> > 
> 
> -- 
> Sincerely yours,
> Mike.