Re: [PATCH 4/4] mm: Return void from folio_start_writeback() and related functions

From: Josef Bacik <josef@toxicpanda.com>
To: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	David Howells <dhowells@redhat.com>,
	Steve French <sfrench@samba.org>,
	linux-afs@lists.infradead.org, linux-cifs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 4/4] mm: Return void from folio_start_writeback() and related functions
Date: Fri, 17 Nov 2023 14:22:04 -0500	[thread overview]
Message-ID: <20231117192204.GB1513185@perftesting> (raw)
In-Reply-To: <20231108204605.745109-5-willy@infradead.org>

On Wed, Nov 08, 2023 at 08:46:05PM +0000, Matthew Wilcox (Oracle) wrote:
> Nobody now checks the return value from any of these functions, so
> add an assertion at the beginning of the function and return void.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  include/linux/page-flags.h |  4 +--
>  mm/folio-compat.c          |  4 +--
>  mm/page-writeback.c        | 54 ++++++++++++++++++--------------------
>  3 files changed, 29 insertions(+), 33 deletions(-)
> 
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index a440062e9386..735cddc13d20 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -772,8 +772,8 @@ static __always_inline void SetPageUptodate(struct page *page)
>  
>  CLEARPAGEFLAG(Uptodate, uptodate, PF_NO_TAIL)
>  
> -bool __folio_start_writeback(struct folio *folio, bool keep_write);
> -bool set_page_writeback(struct page *page);
> +void __folio_start_writeback(struct folio *folio, bool keep_write);
> +void set_page_writeback(struct page *page);
>  
>  #define folio_start_writeback(folio)			\
>  	__folio_start_writeback(folio, false)
> diff --git a/mm/folio-compat.c b/mm/folio-compat.c
> index 10c3247542cb..aee3b9a16828 100644
> --- a/mm/folio-compat.c
> +++ b/mm/folio-compat.c
> @@ -46,9 +46,9 @@ void mark_page_accessed(struct page *page)
>  }
>  EXPORT_SYMBOL(mark_page_accessed);
>  
> -bool set_page_writeback(struct page *page)
> +void set_page_writeback(struct page *page)
>  {
> -	return folio_start_writeback(page_folio(page));
> +	folio_start_writeback(page_folio(page));
>  }
>  EXPORT_SYMBOL(set_page_writeback);
>  
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 46f2f5d3d183..118f02b51c8d 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2982,67 +2982,63 @@ bool __folio_end_writeback(struct folio *folio)
>  	return ret;
>  }
>  
> -bool __folio_start_writeback(struct folio *folio, bool keep_write)
> +void __folio_start_writeback(struct folio *folio, bool keep_write)
>  {
>  	long nr = folio_nr_pages(folio);
>  	struct address_space *mapping = folio_mapping(folio);
> -	bool ret;
>  	int access_ret;
>  
> +	VM_BUG_ON_FOLIO(folio_test_writeback(folio), folio);
> +

At first I was writing a response asking why it was ok to expect that
folio_test_set_writeback would always return true, but then I noticed this bit.
And then I went looking around and it appears that we expect the folio to be
locked when we call this function, so this is indeed safe.  But I'm stupid and
had to go read a bunch of code to make sure this was actually safe.  Could you
add a comment to that effect, or add a

VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);

here as well to make it clear what we expect?  Otherwise the series looks good
to me, you can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef