linux-kernel - Re: [PATCH 2/5] fuse: Convert fuse_try_move

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y2FkV0wogVhMHkkO@casper.infradead.org>
Date:   Tue, 1 Nov 2022 18:24:23 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     "Vishal Moola (Oracle)" <vishal.moola@...il.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, akpm@...ux-foundation.org,
        miklos@...redi.hu
Subject: Re: [PATCH 2/5] fuse: Convert fuse_try_move_page() to use folios

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@...il.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
> 
>