Re: problem with orangefs readpage...

[Matthew Wilcox <willy@infradead.org>]

On Fri, Jan 01, 2021 at 05:15:32PM -0500, Mike Marshall wrote:
> Hi Matthew... Thanks so much for the suggestions!
> > This is some new version of orangefs_readpage(), right?
> No, that code has been upstream for a while... that readahead_control
> thing looks very interesting :-) ...

Oh, my, I was looking at a tree from before 2018 that still had
orangefs_readpages.  So, yes, I think what's happening is that
orangefs_readpage() is being called from the readahead code.
You'll hit this path:

                        next_page = find_get_page(inode->i_mapping, index);
                        if (next_page) {
                                gossip_debug(GOSSIP_FILE_DEBUG,
                                        "%s: found next page, quitting\n",
                                        __func__);
                                put_page(next_page);
                                goto out;

because readahead already allocated those pages for you and is trying
to fill them one-at-a-time.

Implementing ->readahead, even without dhowells' patch to expand
the ractl will definitely help you!

> -Mike
> 
> On Thu, Dec 31, 2020 at 11:08 PM Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Thu, Dec 31, 2020 at 04:51:53PM -0500, Mike Marshall wrote:
> > > Greetings...
> > >
> > > I hope some of you will suffer through reading this long message :-) ...
> >
> > Hi Mike!  Happy New Year!
> >
> > > Orangefs isn't built to do small IO. Reading a
> > > big file in page cache sized chunks is slow and painful.
> > > I tried to write orangefs_readpage so that it would do a reasonable
> > > sized hard IO, fill the page that was being called for, and then
> > > go ahead and fill a whole bunch of the following pages into the
> > > page cache with the extra data in the IO buffer.
> >
> > This is some new version of orangefs_readpage(), right?  I don't see
> > anything resembling this in the current codebase.  Did you disable
> > orangefs_readpages() as part of this work?  Because the behaviour you're
> > describing sounds very much like what the readahead code might do to a
> > filesystem which implements readpage and neither readahead nor readpages.
> >
> > > orangefs_readpage gets called for the first four pages and then my
> > > prefill kicks in and fills the next pages and the right data ends
> > > up in /tmp/nine. I, of course, wished and planned for orangefs_readpage
> > > to only get called once, I don't understand why it gets called four
> > > times, which results in three extraneous expensive hard IOs.
> >
> > I might suggest some judicious calling of dump_stack() to understand
> > exactly what's calling you.  My suspicion is that it's this loop in
> > read_pages():
> >
> >                 while ((page = readahead_page(rac))) {
> >                         aops->readpage(rac->file, page);
> >                         put_page(page);
> >                 }
> >
> > which doesn't test for PageUptodate before calling you.
> >
> > It'd probably be best if you implemented ->readahead, which has its own
> > ideas about which pages would be the right ones to read.  It's not always
> > correct, but generally better to have that logic in the VFS than in each
> > filesystem.
> >
> > You probably want to have a look at Dave Howells' work to allow
> > the filesystem to expand the ractl:
> >
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-iter
> >
> > specifically this patch:
> >
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=fscache-iter&id=f582790b32d5d1d8b937df95a8b2b5fdb8380e46
> >