diff mbox series

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

Message ID 20221101175326.13265-3-vishal.moola@gmail.com (mailing list archive)
State New
Headers show
Series Removing the lru_cache_add() wrapper | expand

Commit Message

Vishal Moola Nov. 1, 2022, 5:53 p.m. UTC 
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(-)

Comments

Matthew Wilcox Nov. 1, 2022, 6:24 p.m. UTC | #1 
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
> 
>
Vishal Moola Nov. 10, 2022, 6:36 p.m. UTC | #2 
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
> >
> >
Miklos Szeredi Nov. 14, 2022, 1:25 p.m. UTC | #3 
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
diff mbox series

Patch

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;