diff mbox series

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

Message ID 20231108204605.745109-5-willy@infradead.org (mailing list archive)
State New, archived
Headers show
Series Make folio_start_writeback return void | expand

Commit Message

Matthew Wilcox Nov. 8, 2023, 8:46 p.m. UTC 
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(-)

Comments

Josef Bacik Nov. 17, 2023, 7:22 p.m. UTC | #1 
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
diff mbox series

Patch

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);
+
 	folio_memcg_lock(folio);
 	if (mapping && mapping_use_writeback_tags(mapping)) {
 		XA_STATE(xas, &mapping->i_pages, folio_index(folio));
 		struct inode *inode = mapping->host;
 		struct backing_dev_info *bdi = inode_to_bdi(inode);
 		unsigned long flags;
+		bool on_wblist;
 
 		xas_lock_irqsave(&xas, flags);
 		xas_load(&xas);
-		ret = folio_test_set_writeback(folio);
-		if (!ret) {
-			bool on_wblist;
+		folio_test_set_writeback(folio);
 
-			on_wblist = mapping_tagged(mapping,
-						   PAGECACHE_TAG_WRITEBACK);
+		on_wblist = mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
 
-			xas_set_mark(&xas, PAGECACHE_TAG_WRITEBACK);
-			if (bdi->capabilities & BDI_CAP_WRITEBACK_ACCT) {
-				struct bdi_writeback *wb = inode_to_wb(inode);
-
-				wb_stat_mod(wb, WB_WRITEBACK, nr);
-				if (!on_wblist)
-					wb_inode_writeback_start(wb);
-			}
+		xas_set_mark(&xas, PAGECACHE_TAG_WRITEBACK);
+		if (bdi->capabilities & BDI_CAP_WRITEBACK_ACCT) {
+			struct bdi_writeback *wb = inode_to_wb(inode);
 
-			/*
-			 * We can come through here when swapping
-			 * anonymous folios, so we don't necessarily
-			 * have an inode to track for sync.
-			 */
-			if (mapping->host && !on_wblist)
-				sb_mark_inode_writeback(mapping->host);
+			wb_stat_mod(wb, WB_WRITEBACK, nr);
+			if (!on_wblist)
+				wb_inode_writeback_start(wb);
 		}
+
+		/*
+		 * We can come through here when swapping anonymous
+		 * folios, so we don't necessarily have an inode to
+		 * track for sync.
+		 */
+		if (mapping->host && !on_wblist)
+			sb_mark_inode_writeback(mapping->host);
 		if (!folio_test_dirty(folio))
 			xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);
 		if (!keep_write)
 			xas_clear_mark(&xas, PAGECACHE_TAG_TOWRITE);
 		xas_unlock_irqrestore(&xas, flags);
 	} else {
-		ret = folio_test_set_writeback(folio);
-	}
-	if (!ret) {
-		lruvec_stat_mod_folio(folio, NR_WRITEBACK, nr);
-		zone_stat_mod_folio(folio, NR_ZONE_WRITE_PENDING, nr);
+		folio_test_set_writeback(folio);
 	}
+
+	lruvec_stat_mod_folio(folio, NR_WRITEBACK, nr);
+	zone_stat_mod_folio(folio, NR_ZONE_WRITE_PENDING, nr);
 	folio_memcg_unlock(folio);
+
 	access_ret = arch_make_folio_accessible(folio);
 	/*
 	 * If writeback has been triggered on a page that cannot be made
 	 * accessible, it is too late to recover here.
 	 */
 	VM_BUG_ON_FOLIO(access_ret != 0, folio);
-
-	return ret;
 }
 EXPORT_SYMBOL(__folio_start_writeback);