[2/5] fuse: Convert fuse_try_move_page() to use folios

  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().

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
---
 fs/fuse/dev.c | 55 ++++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 27 deletions(-)
  

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?

> 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
> 
>
  

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
> >
> >
  

On Thu, 10 Nov 2022 at 19:36, Vishal Moola <vishal.moola@gmail.com> wrote:
>
> 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?

I think this should take care of the large folio case in fuse_try_move_page():

    if (cs->len != PAGE_SIZE)
        goto out_fallback;

The patch looks okay.

Acked-by: Miklos Szeredi <mszeredi@redhat.com>

Thanks,
Miklos