Re: [PATCH v3 01/10] fuse: convert readahead to use folios

[Josef Bacik <josef@toxicpanda.com>]

On Fri, Sep 27, 2024 at 03:22:25PM -0700, Joanne Koong wrote:
> On Fri, Sep 27, 2024 at 1:45 PM Josef Bacik <josef@toxicpanda.com> wrote:
> >
> > Currently we're using the __readahead_batch() helper which populates our
> > fuse_args_pages->pages array with pages.  Convert this to use the newer
> > folio based pattern which is to call readahead_folio() to get the next
> > folio in the read ahead batch.  I've updated the code to use things like
> > folio_size() and to take into account larger folio sizes, but this is
> > purely to make that eventual work easier to do, we currently will not
> > get large folios so this is more future proofing than actual support.
> >
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/fuse/file.c | 43 ++++++++++++++++++++++++++++---------------
> >  1 file changed, 28 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index f33fbce86ae0..132528cde745 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -938,7 +938,6 @@ static void fuse_readpages_end(struct fuse_mount *fm, struct fuse_args *args,
> >                 struct folio *folio = page_folio(ap->pages[i]);
> >
> >                 folio_end_read(folio, !err);
> > -               folio_put(folio);
> >         }
> >         if (ia->ff)
> >                 fuse_file_put(ia->ff, false);
> > @@ -985,18 +984,36 @@ static void fuse_send_readpages(struct fuse_io_args *ia, struct file *file)
> >  static void fuse_readahead(struct readahead_control *rac)
> >  {
> >         struct inode *inode = rac->mapping->host;
> > +       struct fuse_inode *fi = get_fuse_inode(inode);
> >         struct fuse_conn *fc = get_fuse_conn(inode);
> > -       unsigned int i, max_pages, nr_pages = 0;
> > +       unsigned int max_pages, nr_pages;
> > +       pgoff_t first = readahead_index(rac);
> > +       pgoff_t last = first + readahead_count(rac) - 1;
> >
> >         if (fuse_is_bad(inode))
> >                 return;
> >
> > +       wait_event(fi->page_waitq, !fuse_range_is_writeback(inode, first, last));
> 
> Should this line be moved to after we check the readahead count? eg
> 
> nr_pages = readahead_count(rac);
> if (!nr_pages)
>     return;
> wait_event(fi->page_waitq, !fuse_range_is_writeback(inode, first, last));
> 
> Otherwise I think in that case you mentioned where read_pages() calls
> into readahead_folio() after it's consumed the last folio, we end up
> calling this wait_event

The first bit of read_pages covers this for us

static void read_pages(struct readahead_control *rac)
{
        const struct address_space_operations *aops = rac->mapping->a_ops;
        struct folio *folio;
        struct blk_plug plug;

        if (!readahead_count(rac))
                return;

We don't get ->readahead called unless there's pages to read.  Thanks,

Josef