[PATCH 0/4] Make folio_start_writeback return void

[PATCH 0/4] Make folio_start_writeback return void

[Matthew Wilcox (Oracle)]

Most of the folio flag-setting functions return void.
folio_start_writeback is gratuitously different; the only two filesystems
that do anything with the return value emit debug messages if it's already
set, and we can (and should) do that internally without bothering the
filesystem to do it.

Matthew Wilcox (Oracle) (4):
  mm: Remove test_set_page_writeback()
  afs: Do not test the return value of folio_start_writeback()
  smb: Do not test the return value of folio_start_writeback()
  mm: Return void from folio_start_writeback() and related functions

 fs/afs/write.c             |  6 ++---
 fs/smb/client/file.c       |  6 ++---
 include/linux/page-flags.h |  9 ++-----
 mm/folio-compat.c          |  4 +--
 mm/page-writeback.c        | 54 ++++++++++++++++++--------------------
 5 files changed, 33 insertions(+), 46 deletions(-)

-- 
2.42.0

[PATCH 1/4] mm: Remove test_set_page_writeback()

[Matthew Wilcox (Oracle)]

There are no more callers of this wrapper.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/page-flags.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index a88e64acebfe..a440062e9386 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -780,11 +780,6 @@ bool set_page_writeback(struct page *page);
 #define folio_start_writeback_keepwrite(folio)	\
 	__folio_start_writeback(folio, true)
 
-static inline bool test_set_page_writeback(struct page *page)
-{
-	return set_page_writeback(page);
-}
-
 static __always_inline bool folio_test_head(struct folio *folio)
 {
 	return test_bit(PG_head, folio_flags(folio, FOLIO_PF_ANY));
-- 
2.42.0

[PATCH 2/4] afs: Do not test the return value of folio_start_writeback()

[Matthew Wilcox (Oracle)]

In preparation for removing the return value entirely, stop testing it
in afs.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/afs/write.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/afs/write.c b/fs/afs/write.c
index 4a168781936b..57d05d67f0c2 100644
--- a/fs/afs/write.c
+++ b/fs/afs/write.c
@@ -559,8 +559,7 @@ static void afs_extend_writeback(struct address_space *mapping,
 
 			if (!folio_clear_dirty_for_io(folio))
 				BUG();
-			if (folio_start_writeback(folio))
-				BUG();
+			folio_start_writeback(folio);
 			afs_folio_start_fscache(caching, folio);
 
 			*_count -= folio_nr_pages(folio);
@@ -595,8 +594,7 @@ static ssize_t afs_write_back_from_locked_folio(struct address_space *mapping,
 
 	_enter(",%lx,%llx-%llx", folio_index(folio), start, end);
 
-	if (folio_start_writeback(folio))
-		BUG();
+	folio_start_writeback(folio);
 	afs_folio_start_fscache(caching, folio);
 
 	count -= folio_nr_pages(folio);
-- 
2.42.0

[PATCH 3/4] smb: Do not test the return value of folio_start_writeback()

[Matthew Wilcox (Oracle)]

In preparation for removing the return value entirely, stop testing it
in smb.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/smb/client/file.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index cf17e3dd703e..45ca492c141c 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -2706,8 +2706,7 @@ static void cifs_extend_writeback(struct address_space *mapping,
 			 */
 			if (!folio_clear_dirty_for_io(folio))
 				WARN_ON(1);
-			if (folio_start_writeback(folio))
-				WARN_ON(1);
+			folio_start_writeback(folio);
 
 			*_count -= folio_nr_pages(folio);
 			folio_unlock(folio);
@@ -2742,8 +2741,7 @@ static ssize_t cifs_write_back_from_locked_folio(struct address_space *mapping,
 	int rc;
 
 	/* The folio should be locked, dirty and not undergoing writeback. */
-	if (folio_start_writeback(folio))
-		WARN_ON(1);
+	folio_start_writeback(folio);
 
 	count -= folio_nr_pages(folio);
 	len = folio_size(folio);
-- 
2.42.0

Re: [PATCH 3/4] smb: Do not test the return value of folio_start_writeback()

[Paulo Alcantara]

"Matthew Wilcox (Oracle)" <willy@infradead.org> writes:

> In preparation for removing the return value entirely, stop testing it
> in smb.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/smb/client/file.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Paulo Alcantara (SUSE) <pc@manguebit.com>

Re: [PATCH 3/4] smb: Do not test the return value of folio_start_writeback()

[Steve French]

Acked-by: Steve French <stfrench@microsoft.com>

On Wed, Nov 8, 2023 at 9:46 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> "Matthew Wilcox (Oracle)" <willy@infradead.org> writes:
>
> > In preparation for removing the return value entirely, stop testing it
> > in smb.
> >
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >  fs/smb/client/file.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
>
> Reviewed-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
>


-- 
Thanks,

Steve

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

[Matthew Wilcox (Oracle)]

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);
+
 	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);
 
-- 
2.42.0

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

[Josef Bacik]

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