Re: [RFC PATCH] cifs: Transition from ->readpages() to ->readahead()

[Matthew Wilcox <willy@infradead.org>]

On Mon, Jan 24, 2022 at 03:14:41PM +0000, David Howells wrote:
> Matthew Wilcox <willy@infradead.org> wrote:
> 
> > > Questions for Willy:
> > >  - Can we get a function to return the number of pages in a readahead
> > >    batch?
> > > 
> > >  - Can we get a function to commit a readahead batch?  Currently, this is
> > >    done when we call __readahead_batch(), but that means ractl->_nr_pages
> > >    isn't up to date at the point we need it to be.  However, we want to
> > >    check to see if the ractl is empty, then get server credits and only
> > >    *then* call __readahead_batch() as we don't know how big a batch we're
> > >    allowed till we have the credits.
> > 
> > If you insist on using the primitives in a way that nobody else uses
> > them, you're going to find they don't work.  What's wrong with the
> > way that FUSE uses them in fuse_readahead()?
> 
> You mean doing this?
> 
> 		nr_pages = readahead_count(rac) - nr_pages;
> 
> that would seem to indicate that the readahead interface is wrong.  Why should
> readahead_count() need correction?  I think I can see *why* the batching stuff
> is hidden, but it seems that the comment for readahead_count() needs to
> mention this if you aren't going to fix it.
> 
> Would it be possible to make readahead_count() do:
> 
> 	return rac->_nr_pages - rac->_batch_count;
> 
> maybe?

Yes, I think that would make sense.  Do we also need to change
readhead_length()?  It seems to me that it's only ever called once at
initialisation, so it should be possible to keep the two in sync.
Can you write & test such a patch?  I'll support it going upstream
(either by taking it myself or giving you a R-b to take it through your
tree).