[f2fs-dev] cleanup AOP_WRITEPAGE_ACTIVATE use in f2fs

[f2fs-dev] cleanup AOP_WRITEPAGE_ACTIVATE use in f2fs

[Christoph Hellwig]

Hi all,

this almost entirely cleans up usage of AOP_WRITEPAGE_ACTIVATE in f2fs.

f2fs_sync_node_pages can still return it in a way that is not handled
by any caller and eventually is propagated to userspace.  This does look
like a bug and needs attention by someone who actually knows the code.

Diffstat:
 checkpoint.c |   22 +++++++++++-----------
 compress.c   |    5 +----
 data.c       |   13 ++++---------
 node.c       |   43 ++++++++++++++++++++-----------------------
 4 files changed, 36 insertions(+), 47 deletions(-)


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 1/4] f2fs: return bool from __f2fs_write_meta_folio

[Christoph Hellwig]

__f2fs_write_meta_folio can only return 0 or AOP_WRITEPAGE_ACTIVATE.
As part of phasing out AOP_WRITEPAGE_ACTIVATE, switch to a bool return
instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/f2fs/checkpoint.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index e42ed62fa45c..595d6e87aa2f 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -340,7 +340,7 @@ void f2fs_ra_meta_pages_cond(struct f2fs_sb_info *sbi, pgoff_t index,
 		f2fs_ra_meta_pages(sbi, index, ra_blocks, META_POR, true);
 }
 
-static int __f2fs_write_meta_folio(struct folio *folio,
+static bool __f2fs_write_meta_folio(struct folio *folio,
 				struct writeback_control *wbc,
 				enum iostat_type io_type)
 {
@@ -353,7 +353,7 @@ static int __f2fs_write_meta_folio(struct folio *folio,
 			folio_clear_uptodate(folio);
 			dec_page_count(sbi, F2FS_DIRTY_META);
 			folio_unlock(folio);
-			return 0;
+			return true;
 		}
 		goto redirty_out;
 	}
@@ -373,11 +373,11 @@ static int __f2fs_write_meta_folio(struct folio *folio,
 	if (unlikely(f2fs_cp_error(sbi)))
 		f2fs_submit_merged_write(sbi, META);
 
-	return 0;
+	return true;
 
 redirty_out:
 	folio_redirty_for_writepage(wbc, folio);
-	return AOP_WRITEPAGE_ACTIVATE;
+	return false;
 }
 
 static int f2fs_write_meta_pages(struct address_space *mapping,
@@ -461,7 +461,7 @@ long f2fs_sync_meta_pages(struct f2fs_sb_info *sbi, enum page_type type,
 			if (!folio_clear_dirty_for_io(folio))
 				goto continue_unlock;
 
-			if (__f2fs_write_meta_folio(folio, &wbc,
+			if (!__f2fs_write_meta_folio(folio, &wbc,
 						io_type)) {
 				folio_unlock(folio);
 				break;
@@ -1409,7 +1409,6 @@ static void commit_checkpoint(struct f2fs_sb_info *sbi,
 	 * f2fs_sync_meta_pages are combined in this function.
 	 */
 	struct folio *folio = f2fs_grab_meta_folio(sbi, blk_addr);
-	int err;
 
 	memcpy(folio_address(folio), src, PAGE_SIZE);
 
@@ -1418,13 +1417,14 @@ static void commit_checkpoint(struct f2fs_sb_info *sbi,
 		f2fs_bug_on(sbi, 1);
 
 	/* writeout cp pack 2 page */
-	err = __f2fs_write_meta_folio(folio, &wbc, FS_CP_META_IO);
-	if (unlikely(err && f2fs_cp_error(sbi))) {
-		f2fs_folio_put(folio, true);
-		return;
+	if (unlikely(!__f2fs_write_meta_folio(folio, &wbc, FS_CP_META_IO))) {
+		if (f2fs_cp_error(sbi)) {
+			f2fs_folio_put(folio, true);
+			return;
+		}
+		f2fs_bug_on(sbi, true);
 	}
 
-	f2fs_bug_on(sbi, err);
 	f2fs_folio_put(folio, false);
 
 	/* submit checkpoint (with barrier if NOBARRIER is not set) */
-- 
2.47.2



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/4] f2fs: return bool from __f2fs_write_meta_folio

[Chao Yu via Linux-f2fs-devel]

On 5/5/25 17:25, Christoph Hellwig wrote:
> __f2fs_write_meta_folio can only return 0 or AOP_WRITEPAGE_ACTIVATE.
> As part of phasing out AOP_WRITEPAGE_ACTIVATE, switch to a bool return
> instead.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/4] f2fs: return bool from __f2fs_write_meta_folio

[patchwork-bot+f2fs--- via Linux-f2fs-devel]

Hello:

This series was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:

On Mon,  5 May 2025 11:25:58 +0200 you wrote:
> __f2fs_write_meta_folio can only return 0 or AOP_WRITEPAGE_ACTIVATE.
> As part of phasing out AOP_WRITEPAGE_ACTIVATE, switch to a bool return
> instead.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/f2fs/checkpoint.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)

Here is the summary with links:
  - [f2fs-dev,1/4] f2fs: return bool from __f2fs_write_meta_folio
    https://git.kernel.org/jaegeuk/f2fs/c/39122e454419
  - [f2fs-dev,2/4] f2fs: don't return AOP_WRITEPAGE_ACTIVATE from f2fs_write_single_data_page
    (no matching commit)
  - [f2fs-dev,3/4] f2fs: simplify return value handling in f2fs_fsync_node_pages
    (no matching commit)
  - [f2fs-dev,4/4] f2f2: return bool from __write_node_folio
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html




_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 2/4] f2fs: don't return AOP_WRITEPAGE_ACTIVATE from f2fs_write_single_data_page

[Christoph Hellwig]

Instead unlock the pages locally where that would happen and thus
consolidate the code in the callers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/f2fs/compress.c |  5 +----
 fs/f2fs/data.c     | 13 ++++---------
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index e016b0f96313..ce63b3bfb28f 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -1565,10 +1565,7 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
 						NULL, NULL, wbc, io_type,
 						compr_blocks, false);
 		if (ret) {
-			if (ret == AOP_WRITEPAGE_ACTIVATE) {
-				folio_unlock(folio);
-				ret = 0;
-			} else if (ret == -EAGAIN) {
+			if (ret == -EAGAIN) {
 				ret = 0;
 				/*
 				 * for quota file, just redirty left pages to
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 08a8a107adcb..e32c9cf5b4f5 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2930,10 +2930,10 @@ int f2fs_write_single_data_page(struct folio *folio, int *submitted,
 	 * file_write_and_wait_range() will see EIO error, which is critical
 	 * to return value of fsync() followed by atomic_write failure to user.
 	 */
-	if (!err || wbc->for_reclaim)
-		return AOP_WRITEPAGE_ACTIVATE;
 	folio_unlock(folio);
-	return err;
+	if (err && !wbc->for_reclaim)
+		return err;
+	return 0;
 }
 
 /*
@@ -3146,8 +3146,6 @@ static int f2fs_write_cache_pages(struct address_space *mapping,
 			ret = f2fs_write_single_data_page(folio,
 					&submitted, &bio, &last_block,
 					wbc, io_type, 0, true);
-			if (ret == AOP_WRITEPAGE_ACTIVATE)
-				folio_unlock(folio);
 #ifdef CONFIG_F2FS_FS_COMPRESSION
 result:
 #endif
@@ -3159,10 +3157,7 @@ static int f2fs_write_cache_pages(struct address_space *mapping,
 				 * keep nr_to_write, since vfs uses this to
 				 * get # of written pages.
 				 */
-				if (ret == AOP_WRITEPAGE_ACTIVATE) {
-					ret = 0;
-					goto next;
-				} else if (ret == -EAGAIN) {
+				if (ret == -EAGAIN) {
 					ret = 0;
 					if (wbc->sync_mode == WB_SYNC_ALL) {
 						f2fs_io_schedule_timeout(
-- 
2.47.2



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/4] f2fs: don't return AOP_WRITEPAGE_ACTIVATE from f2fs_write_single_data_page

[Chao Yu via Linux-f2fs-devel]

On 5/5/25 17:25, Christoph Hellwig wrote:
> Instead unlock the pages locally where that would happen and thus
> consolidate the code in the callers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/f2fs/compress.c |  5 +----
>  fs/f2fs/data.c     | 13 ++++---------
>  2 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index e016b0f96313..ce63b3bfb28f 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1565,10 +1565,7 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
>  						NULL, NULL, wbc, io_type,
>  						compr_blocks, false);
>  		if (ret) {
> -			if (ret == AOP_WRITEPAGE_ACTIVATE) {
> -				folio_unlock(folio);
> -				ret = 0;

Previously, for this case, it will goto out label rather than writing
left pages?

Thanks,

> -			} else if (ret == -EAGAIN) {
> +			if (ret == -EAGAIN) {
>  				ret = 0;
>  				/*
>  				 * for quota file, just redirty left pages to
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 08a8a107adcb..e32c9cf5b4f5 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2930,10 +2930,10 @@ int f2fs_write_single_data_page(struct folio *folio, int *submitted,
>  	 * file_write_and_wait_range() will see EIO error, which is critical
>  	 * to return value of fsync() followed by atomic_write failure to user.
>  	 */
> -	if (!err || wbc->for_reclaim)
> -		return AOP_WRITEPAGE_ACTIVATE;
>  	folio_unlock(folio);
> -	return err;
> +	if (err && !wbc->for_reclaim)
> +		return err;
> +	return 0;
>  }
>  
>  /*
> @@ -3146,8 +3146,6 @@ static int f2fs_write_cache_pages(struct address_space *mapping,
>  			ret = f2fs_write_single_data_page(folio,
>  					&submitted, &bio, &last_block,
>  					wbc, io_type, 0, true);
> -			if (ret == AOP_WRITEPAGE_ACTIVATE)
> -				folio_unlock(folio);
>  #ifdef CONFIG_F2FS_FS_COMPRESSION
>  result:
>  #endif
> @@ -3159,10 +3157,7 @@ static int f2fs_write_cache_pages(struct address_space *mapping,
>  				 * keep nr_to_write, since vfs uses this to
>  				 * get # of written pages.
>  				 */
> -				if (ret == AOP_WRITEPAGE_ACTIVATE) {
> -					ret = 0;
> -					goto next;
> -				} else if (ret == -EAGAIN) {
> +				if (ret == -EAGAIN) {
>  					ret = 0;
>  					if (wbc->sync_mode == WB_SYNC_ALL) {
>  						f2fs_io_schedule_timeout(



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/4] f2fs: don't return AOP_WRITEPAGE_ACTIVATE from f2fs_write_single_data_page

[Christoph Hellwig]

On Wed, May 07, 2025 at 02:28:55PM +0800, Chao Yu wrote:
> > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> > index e016b0f96313..ce63b3bfb28f 100644
> > --- a/fs/f2fs/compress.c
> > +++ b/fs/f2fs/compress.c
> > @@ -1565,10 +1565,7 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
> >  						NULL, NULL, wbc, io_type,
> >  						compr_blocks, false);
> >  		if (ret) {
> > -			if (ret == AOP_WRITEPAGE_ACTIVATE) {
> > -				folio_unlock(folio);
> > -				ret = 0;
> 
> Previously, for this case, it will goto out label rather than writing
> left pages?

Indeed.  Is that the right thing to do here?


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/4] f2fs: don't return AOP_WRITEPAGE_ACTIVATE from f2fs_write_single_data_page

[Chao Yu via Linux-f2fs-devel]

On 5/7/25 14:44, Christoph Hellwig wrote:
> On Wed, May 07, 2025 at 02:28:55PM +0800, Chao Yu wrote:
>>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
>>> index e016b0f96313..ce63b3bfb28f 100644
>>> --- a/fs/f2fs/compress.c
>>> +++ b/fs/f2fs/compress.c
>>> @@ -1565,10 +1565,7 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
>>>  						NULL, NULL, wbc, io_type,
>>>  						compr_blocks, false);
>>>  		if (ret) {
>>> -			if (ret == AOP_WRITEPAGE_ACTIVATE) {
>>> -				folio_unlock(folio);
>>> -				ret = 0;
>>
>> Previously, for this case, it will goto out label rather than writing
>> left pages?
> 
> Indeed.  Is that the right thing to do here?

IIRC, once it failed to write one page, it redirties all left pages, and tries
to rewrite them again, it can avoid fragment as much as possible.

So can we keep original implementation here?

Thanks,



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 3/4] f2fs: simplify return value handling in f2fs_fsync_node_pages

[Christoph Hellwig]

Always assign ret where the error happens, and jump to out instead
of multiple loop exit conditions to prepare for changes in the
__write_node_folio calling convention.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/f2fs/node.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index ec74eb9982a5..b9d9519c3da4 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1882,17 +1882,17 @@ int f2fs_fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
 			if (!folio_clear_dirty_for_io(folio))
 				goto continue_unlock;
 
-			ret = __write_node_folio(folio, atomic &&
+			if (__write_node_folio(folio, atomic &&
 						folio == last_folio,
 						&submitted, wbc, true,
-						FS_NODE_IO, seq_id);
-			if (ret) {
+						FS_NODE_IO, seq_id)) {
 				folio_unlock(folio);
 				f2fs_folio_put(last_folio, false);
-				break;
-			} else if (submitted) {
-				nwritten++;
+				ret = -EIO;
+				goto out;
 			}
+			if (submitted)
+				nwritten++;
 
 			if (folio == last_folio) {
 				f2fs_folio_put(folio, false);
@@ -1903,10 +1903,10 @@ int f2fs_fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
 		folio_batch_release(&fbatch);
 		cond_resched();
 
-		if (ret || marked)
+		if (marked)
 			break;
 	}
-	if (!ret && atomic && !marked) {
+	if (atomic && !marked) {
 		f2fs_debug(sbi, "Retry to write fsync mark: ino=%u, idx=%lx",
 			   ino, last_folio->index);
 		folio_lock(last_folio);
@@ -1918,7 +1918,7 @@ int f2fs_fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
 out:
 	if (nwritten)
 		f2fs_submit_merged_write_cond(sbi, NULL, NULL, ino, NODE);
-	return ret ? -EIO : 0;
+	return ret;
 }
 
 static int f2fs_match_ino(struct inode *inode, unsigned long ino, void *data)
-- 
2.47.2



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 3/4] f2fs: simplify return value handling in f2fs_fsync_node_pages

[Chao Yu via Linux-f2fs-devel]

On 5/5/25 17:26, Christoph Hellwig wrote:
> Always assign ret where the error happens, and jump to out instead
> of multiple loop exit conditions to prepare for changes in the
> __write_node_folio calling convention.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/f2fs/node.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index ec74eb9982a5..b9d9519c3da4 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1882,17 +1882,17 @@ int f2fs_fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>  			if (!folio_clear_dirty_for_io(folio))
>  				goto continue_unlock;
>  
> -			ret = __write_node_folio(folio, atomic &&
> +			if (__write_node_folio(folio, atomic &&
>  						folio == last_folio,
>  						&submitted, wbc, true,
> -						FS_NODE_IO, seq_id);
> -			if (ret) {
> +						FS_NODE_IO, seq_id)) {
>  				folio_unlock(folio);
>  				f2fs_folio_put(last_folio, false);
> -				break;
> -			} else if (submitted) {
> -				nwritten++;
> +				ret = -EIO;
> +				goto out;

It missed to call folio_batch_release() here?

Thanks,

>  			}
> +			if (submitted)
> +				nwritten++;
>  
>  			if (folio == last_folio) {
>  				f2fs_folio_put(folio, false);
> @@ -1903,10 +1903,10 @@ int f2fs_fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>  		folio_batch_release(&fbatch);
>  		cond_resched();
>  
> -		if (ret || marked)
> +		if (marked)
>  			break;
>  	}
> -	if (!ret && atomic && !marked) {
> +	if (atomic && !marked) {
>  		f2fs_debug(sbi, "Retry to write fsync mark: ino=%u, idx=%lx",
>  			   ino, last_folio->index);
>  		folio_lock(last_folio);
> @@ -1918,7 +1918,7 @@ int f2fs_fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>  out:
>  	if (nwritten)
>  		f2fs_submit_merged_write_cond(sbi, NULL, NULL, ino, NODE);
> -	return ret ? -EIO : 0;
> +	return ret;
>  }
>  
>  static int f2fs_match_ino(struct inode *inode, unsigned long ino, void *data)



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 4/4] f2f2: return bool from __write_node_folio

[Christoph Hellwig]

__write_node_folio can only return 0 or AOP_WRITEPAGE_ACTIVATE.
As part of phasing out AOP_WRITEPAGE_ACTIVATE, switch to a bool return
instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/f2fs/node.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index b9d9519c3da4..4008e09c3d58 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1651,7 +1651,7 @@ static struct folio *last_fsync_dnode(struct f2fs_sb_info *sbi, nid_t ino)
 	return last_folio;
 }
 
-static int __write_node_folio(struct folio *folio, bool atomic, bool *submitted,
+static bool __write_node_folio(struct folio *folio, bool atomic, bool *submitted,
 				struct writeback_control *wbc, bool do_balance,
 				enum iostat_type io_type, unsigned int *seq_id)
 {
@@ -1681,7 +1681,7 @@ static int __write_node_folio(struct folio *folio, bool atomic, bool *submitted,
 		folio_clear_uptodate(folio);
 		dec_page_count(sbi, F2FS_DIRTY_NODES);
 		folio_unlock(folio);
-		return 0;
+		return true;
 	}
 
 	if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
@@ -1712,7 +1712,7 @@ static int __write_node_folio(struct folio *folio, bool atomic, bool *submitted,
 		dec_page_count(sbi, F2FS_DIRTY_NODES);
 		f2fs_up_read(&sbi->node_write);
 		folio_unlock(folio);
-		return 0;
+		return true;
 	}
 
 	if (__is_valid_data_blkaddr(ni.blk_addr) &&
@@ -1756,11 +1756,12 @@ static int __write_node_folio(struct folio *folio, bool atomic, bool *submitted,
 
 	if (do_balance)
 		f2fs_balance_fs(sbi, false);
-	return 0;
+	return true;
 
 redirty_out:
 	folio_redirty_for_writepage(wbc, folio);
-	return AOP_WRITEPAGE_ACTIVATE;
+	folio_unlock(folio);
+	return false;
 }
 
 int f2fs_move_node_folio(struct folio *node_folio, int gc_type)
@@ -1783,11 +1784,9 @@ int f2fs_move_node_folio(struct folio *node_folio, int gc_type)
 			goto out_page;
 		}
 
-		if (__write_node_folio(node_folio, false, NULL,
-					&wbc, false, FS_GC_NODE_IO, NULL)) {
+		if (!__write_node_folio(node_folio, false, NULL,
+					&wbc, false, FS_GC_NODE_IO, NULL))
 			err = -EAGAIN;
-			folio_unlock(node_folio);
-		}
 		goto release_page;
 	} else {
 		/* set page dirty and write it */
@@ -1882,11 +1881,10 @@ int f2fs_fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
 			if (!folio_clear_dirty_for_io(folio))
 				goto continue_unlock;
 
-			if (__write_node_folio(folio, atomic &&
+			if (!__write_node_folio(folio, atomic &&
 						folio == last_folio,
 						&submitted, wbc, true,
 						FS_NODE_IO, seq_id)) {
-				folio_unlock(folio);
 				f2fs_folio_put(last_folio, false);
 				ret = -EIO;
 				goto out;
@@ -2090,10 +2088,9 @@ int f2fs_sync_node_pages(struct f2fs_sb_info *sbi,
 			set_fsync_mark(&folio->page, 0);
 			set_dentry_mark(&folio->page, 0);
 
-			ret = __write_node_folio(folio, false, &submitted,
-						wbc, do_balance, io_type, NULL);
-			if (ret)
-				folio_unlock(folio);
+			if (!__write_node_folio(folio, false, &submitted,
+						wbc, do_balance, io_type, NULL))
+				ret = AOP_WRITEPAGE_ACTIVATE;
 			else if (submitted)
 				nwritten++;
 
-- 
2.47.2



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 4/4] f2f2: return bool from __write_node_folio

[Chao Yu via Linux-f2fs-devel]

On 5/5/25 17:26, Christoph Hellwig wrote:
> __write_node_folio can only return 0 or AOP_WRITEPAGE_ACTIVATE.
> As part of phasing out AOP_WRITEPAGE_ACTIVATE, switch to a bool return
> instead.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] cleanup AOP_WRITEPAGE_ACTIVATE use in f2fs

[Chao Yu via Linux-f2fs-devel]

On 5/5/25 17:25, Christoph Hellwig wrote:
> Hi all,
> 
> this almost entirely cleans up usage of AOP_WRITEPAGE_ACTIVATE in f2fs.
> 
> f2fs_sync_node_pages can still return it in a way that is not handled
> by any caller and eventually is propagated to userspace.  This does look
> like a bug and needs attention by someone who actually knows the code.

Oh, I guess this is a bug, thanks for catching this.

Anyway, let me fix this based on your patchset.

Thanks,

> 
> Diffstat:
>  checkpoint.c |   22 +++++++++++-----------
>  compress.c   |    5 +----
>  data.c       |   13 ++++---------
>  node.c       |   43 ++++++++++++++++++++-----------------------
>  4 files changed, 36 insertions(+), 47 deletions(-)



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] cleanup AOP_WRITEPAGE_ACTIVATE use in f2fs

[Christoph Hellwig]

On Wed, May 07, 2025 at 03:38:20PM +0800, Chao Yu wrote:
> On 5/5/25 17:25, Christoph Hellwig wrote:
> > Hi all,
> > 
> > this almost entirely cleans up usage of AOP_WRITEPAGE_ACTIVATE in f2fs.
> > 
> > f2fs_sync_node_pages can still return it in a way that is not handled
> > by any caller and eventually is propagated to userspace.  This does look
> > like a bug and needs attention by someone who actually knows the code.
> 
> Oh, I guess this is a bug, thanks for catching this.
> 
> Anyway, let me fix this based on your patchset.

I'll resend a fixed version later today, maybe wait for that.



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] cleanup AOP_WRITEPAGE_ACTIVATE use in f2fs

[Chao Yu via Linux-f2fs-devel]

On 5/7/25 15:48, Christoph Hellwig wrote:
> On Wed, May 07, 2025 at 03:38:20PM +0800, Chao Yu wrote:
>> On 5/5/25 17:25, Christoph Hellwig wrote:
>>> Hi all,
>>>
>>> this almost entirely cleans up usage of AOP_WRITEPAGE_ACTIVATE in f2fs.
>>>
>>> f2fs_sync_node_pages can still return it in a way that is not handled
>>> by any caller and eventually is propagated to userspace.  This does look
>>> like a bug and needs attention by someone who actually knows the code.
>>
>> Oh, I guess this is a bug, thanks for catching this.
>>
>> Anyway, let me fix this based on your patchset.
> 
> I'll resend a fixed version later today, maybe wait for that.

I've submitted a patch, maybe you can rebase new version on it?

https://lore.kernel.org/linux-f2fs-devel/20250507080838.882657-1-chao@kernel.org

https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/commit/?h=bugfix/common&id=a0b7dfb634f98b88875e7bc3166159d2abc7f164

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel