Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221101175326.13265-1-vishal.moola@gmail.com>
 <20221101175326.13265-3-vishal.moola@gmail.com> <Y2FkV0wogVhMHkkO@casper.infradead.org>
In-Reply-To: <Y2FkV0wogVhMHkkO@casper.infradead.org>
From:   Vishal Moola <vishal.moola@gmail.com>
Date:   Thu, 10 Nov 2022 10:36:43 -0800
Message-ID: <CAOzc2px2POjBU-6X=YB+rd-87vhDx0cUu0m8EFWA=Q_Peu1gUg@mail.gmail.com>
Subject: Re: [PATCH 2/5] fuse: Convert fuse_try_move_page() to use folios
To:     miklos@szeredi.hu
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, akpm@linux-foundation.org,
        Matthew Wilcox <willy@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Nov 1, 2022 at 11:24 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Nov 01, 2022 at 10:53:23AM -0700, Vishal Moola (Oracle) wrote:
> > Converts the function to try to move folios instead of pages. Also
> > converts fuse_check_page() to fuse_get_folio() since this is its only
> > caller. This change removes 15 calls to compound_head().
>
> This all looks good.  I wonder if we should't add an assertion that the
> page we're trying to steal is !large?  It seems to me that there are
> assumptions in this part of fuse that it's only dealing with order-0
> pages, and if someone gives it a page that's part of a large folio,
> it's going to be messy.  Miklos, any thoughts?

Miklos, could you please look over this patch?

> > Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> > ---
> >  fs/fuse/dev.c | 55 ++++++++++++++++++++++++++-------------------------
> >  1 file changed, 28 insertions(+), 27 deletions(-)
> >
> > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > index 26817a2db463..204c332cd343 100644
> > --- a/fs/fuse/dev.c
> > +++ b/fs/fuse/dev.c
> > @@ -764,11 +764,11 @@ static int fuse_copy_do(struct fuse_copy_state *cs, void **val, unsigned *size)
> >       return ncpy;
> >  }
> >
> > -static int fuse_check_page(struct page *page)
> > +static int fuse_check_folio(struct folio *folio)
> >  {
> > -     if (page_mapcount(page) ||
> > -         page->mapping != NULL ||
> > -         (page->flags & PAGE_FLAGS_CHECK_AT_PREP &
> > +     if (folio_mapped(folio) ||
> > +         folio->mapping != NULL ||
> > +         (folio->flags & PAGE_FLAGS_CHECK_AT_PREP &
> >            ~(1 << PG_locked |
> >              1 << PG_referenced |
> >              1 << PG_uptodate |
> > @@ -778,7 +778,7 @@ static int fuse_check_page(struct page *page)
> >              1 << PG_reclaim |
> >              1 << PG_waiters |
> >              LRU_GEN_MASK | LRU_REFS_MASK))) {
> > -             dump_page(page, "fuse: trying to steal weird page");
> > +             dump_page(&folio->page, "fuse: trying to steal weird page");
> >               return 1;
> >       }
> >       return 0;
> > @@ -787,11 +787,11 @@ static int fuse_check_page(struct page *page)
> >  static int fuse_try_move_page(struct fuse_copy_state *cs, struct page **pagep)
> >  {
> >       int err;
> > -     struct page *oldpage = *pagep;
> > -     struct page *newpage;
> > +     struct folio *oldfolio = page_folio(*pagep);
> > +     struct folio *newfolio;
> >       struct pipe_buffer *buf = cs->pipebufs;
> >
> > -     get_page(oldpage);
> > +     folio_get(oldfolio);
> >       err = unlock_request(cs->req);
> >       if (err)
> >               goto out_put_old;
> > @@ -814,35 +814,36 @@ static int fuse_try_move_page(struct fuse_copy_state *cs, struct page **pagep)
> >       if (!pipe_buf_try_steal(cs->pipe, buf))
> >               goto out_fallback;
> >
> > -     newpage = buf->page;
> > +     newfolio = page_folio(buf->page);
> >
> > -     if (!PageUptodate(newpage))
> > -             SetPageUptodate(newpage);
> > +     if (!folio_test_uptodate(newfolio))
> > +             folio_mark_uptodate(newfolio);
> >
> > -     ClearPageMappedToDisk(newpage);
> > +     folio_clear_mappedtodisk(newfolio);
> >
> > -     if (fuse_check_page(newpage) != 0)
> > +     if (fuse_check_folio(newfolio) != 0)
> >               goto out_fallback_unlock;
> >
> >       /*
> >        * This is a new and locked page, it shouldn't be mapped or
> >        * have any special flags on it
> >        */
> > -     if (WARN_ON(page_mapped(oldpage)))
> > +     if (WARN_ON(folio_mapped(oldfolio)))
> >               goto out_fallback_unlock;
> > -     if (WARN_ON(page_has_private(oldpage)))
> > +     if (WARN_ON(folio_has_private(oldfolio)))
> >               goto out_fallback_unlock;
> > -     if (WARN_ON(PageDirty(oldpage) || PageWriteback(oldpage)))
> > +     if (WARN_ON(folio_test_dirty(oldfolio) ||
> > +                             folio_test_writeback(oldfolio)))
> >               goto out_fallback_unlock;
> > -     if (WARN_ON(PageMlocked(oldpage)))
> > +     if (WARN_ON(folio_test_mlocked(oldfolio)))
> >               goto out_fallback_unlock;
> >
> > -     replace_page_cache_folio(page_folio(oldpage), page_folio(newpage));
> > +     replace_page_cache_folio(oldfolio, newfolio);
> >
> > -     get_page(newpage);
> > +     folio_get(newfolio);
> >
> >       if (!(buf->flags & PIPE_BUF_FLAG_LRU))
> > -             lru_cache_add(newpage);
> > +             folio_add_lru(newfolio);
> >
> >       /*
> >        * Release while we have extra ref on stolen page.  Otherwise
> > @@ -855,28 +856,28 @@ static int fuse_try_move_page(struct fuse_copy_state *cs, struct page **pagep)
> >       if (test_bit(FR_ABORTED, &cs->req->flags))
> >               err = -ENOENT;
> >       else
> > -             *pagep = newpage;
> > +             *pagep = &newfolio->page;
> >       spin_unlock(&cs->req->waitq.lock);
> >
> >       if (err) {
> > -             unlock_page(newpage);
> > -             put_page(newpage);
> > +             folio_unlock(newfolio);
> > +             folio_put(newfolio);
> >               goto out_put_old;
> >       }
> >
> > -     unlock_page(oldpage);
> > +     folio_unlock(oldfolio);
> >       /* Drop ref for ap->pages[] array */
> > -     put_page(oldpage);
> > +     folio_put(oldfolio);
> >       cs->len = 0;
> >
> >       err = 0;
> >  out_put_old:
> >       /* Drop ref obtained in this function */
> > -     put_page(oldpage);
> > +     folio_put(oldfolio);
> >       return err;
> >
> >  out_fallback_unlock:
> > -     unlock_page(newpage);
> > +     folio_unlock(newfolio);
> >  out_fallback:
> >       cs->pg = buf->page;
> >       cs->offset = buf->offset;
> > --
> > 2.38.1
> >
> >