F2FS writes long symlink data with page_symlink() and then flushes the
symlink mapping to reduce the chance of exposing a broken symlink.

That flush result is currently ignored. If the writeback fails, symlink()
still returns success even though the symlink is not durable and the same
operation can already surface -EIO through syncfs().

Return the writeback error to userspace and skip the dirsync flush once the
symlink data flush has failed.

Fixes: d0cae97cb600 ("f2fs: flush symlink path to avoid broken symlink after POR")
Cc: stable@kernel.org
Signed-off-by: Wenjie Qi <qiwenjie@xiaomi.com>
---
Changes since v1:
- fix the Fixes tag to point to the real introducing commit

Reproducer:
- enable FAULT_WRITE_IO on the mounted F2FS filesystem
- create a long symlink so the target is written through page_symlink()
- old kernel: symlink() returns 0, syncfs() returns -EIO, and the symlink is
  absent after remount
- new kernel: symlink() returns -EIO

 fs/f2fs/namei.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 7ffdf23cea5e..a8db3928c8b5 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -707,15 +707,16 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	 * performance regression.
 	 */
 	if (!err) {
-		filemap_write_and_wait_range(inode->i_mapping, 0,
-							disk_link.len - 1);
+		err = filemap_write_and_wait_range(inode->i_mapping, 0,
+						   disk_link.len - 1);
 
-		if (IS_DIRSYNC(dir))
+		if (!err && IS_DIRSYNC(dir))
 			f2fs_sync_fs(sbi->sb, 1);
-	} else {
-		f2fs_unlink(dir, dentry);
 	}
 
+	if (err)
+		f2fs_unlink(dir, dentry);
+
 	f2fs_balance_fs(sbi, true);
 	goto out_free_encrypted_link;
 
-- 
2.43.0

On Mon, Aug 10, 2026 at 09:38:32PM +0800, Wenjie Qi wrote:
> F2FS writes long symlink data with page_symlink() and then flushes the
> symlink mapping to reduce the chance of exposing a broken symlink.
> 
> That flush result is currently ignored. If the writeback fails, symlink()
> still returns success even though the symlink is not durable and the same
> operation can already surface -EIO through syncfs().
> 
> Return the writeback error to userspace and skip the dirsync flush once the
> symlink data flush has failed.

>  	if (!err) {
> -		filemap_write_and_wait_range(inode->i_mapping, 0,
> -							disk_link.len - 1);
> +		err = filemap_write_and_wait_range(inode->i_mapping, 0,
> +						   disk_link.len - 1);
>  
> -		if (IS_DIRSYNC(dir))
> +		if (!err && IS_DIRSYNC(dir))
>  			f2fs_sync_fs(sbi->sb, 1);
> -	} else {
> -		f2fs_unlink(dir, dentry);
>  	}
>  
> +	if (err)
> +		f2fs_unlink(dir, dentry);

That looks fishy.  At that point you already have dentry hashed and
AFAICS f2fs_unlink() will leave it hashed and attached to the same
inode; sure, memory pressure will eventually evict the sucker, but
until that point any lookups will simply pick it from dcache.

It's not introduced by this patch; the same issue, AFAICS, already exists
in mainline.  Why do we even bother with d_instantiate_new() before we
know that everything's fine, nevermind doing that when we already know
the operation has failed?

Incidentally, is there any reason to add a directory entry before the
inode is set up?  Usually that's the last step, and cleanup tends to
be simpler that way; are there f2fs-specific reasons to do it in the
unusual order?

On 08/15, Al Viro wrote:
> On Mon, Aug 10, 2026 at 09:38:32PM +0800, Wenjie Qi wrote:
> > F2FS writes long symlink data with page_symlink() and then flushes the
> > symlink mapping to reduce the chance of exposing a broken symlink.
> > 
> > That flush result is currently ignored. If the writeback fails, symlink()
> > still returns success even though the symlink is not durable and the same
> > operation can already surface -EIO through syncfs().
> > 
> > Return the writeback error to userspace and skip the dirsync flush once the
> > symlink data flush has failed.
> 
> >  	if (!err) {
> > -		filemap_write_and_wait_range(inode->i_mapping, 0,
> > -							disk_link.len - 1);
> > +		err = filemap_write_and_wait_range(inode->i_mapping, 0,
> > +						   disk_link.len - 1);
> >  
> > -		if (IS_DIRSYNC(dir))
> > +		if (!err && IS_DIRSYNC(dir))
> >  			f2fs_sync_fs(sbi->sb, 1);
> > -	} else {
> > -		f2fs_unlink(dir, dentry);
> >  	}
> >  
> > +	if (err)
> > +		f2fs_unlink(dir, dentry);
> 
> That looks fishy.  At that point you already have dentry hashed and
> AFAICS f2fs_unlink() will leave it hashed and attached to the same
> inode; sure, memory pressure will eventually evict the sucker, but
> until that point any lookups will simply pick it from dcache.

Thanks, yeah..it seems we don't need to do f2fs_unlink() at this stage, since
IMHO, this code block is a nice-to-succeed as there'll be another chance to
flush dirty pages containing the symlink path.

IMO, we need to handle the error like this:

    1. f2fs_new_inode
    2. f2fs_add_link
    3. page_symlink
      -> if it fails, we should unlink and drop the inode

    4. flush dirty pages and or checkpoint
      -> leave as is and wait for writeback again

RFC: https://lore.kernel.org/linux-f2fs-devel/20260818200121.2684318-1-jaegeuk@kernel.org/T/#u

> 
> It's not introduced by this patch; the same issue, AFAICS, already exists
> in mainline.  Why do we even bother with d_instantiate_new() before we
> know that everything's fine, nevermind doing that when we already know
> the operation has failed?
> 
> Incidentally, is there any reason to add a directory entry before the
> inode is set up?  Usually that's the last step, and cleanup tends to
> be simpler that way; are there f2fs-specific reasons to do it in the
> unusual order?

I don't think there's a special reason in f2fs to do so. It seems ext4 does
it similarily like calling d_instantiate_new() in ext4_add_nondir() after
ext4_add_entry()?

On 8/10/26 21:38, Wenjie Qi wrote:
> F2FS writes long symlink data with page_symlink() and then flushes the
> symlink mapping to reduce the chance of exposing a broken symlink.
> 
> That flush result is currently ignored. If the writeback fails, symlink()
> still returns success even though the symlink is not durable and the same
> operation can already surface -EIO through syncfs().
> 
> Return the writeback error to userspace and skip the dirsync flush once the
> symlink data flush has failed.
> 
> Fixes: d0cae97cb600 ("f2fs: flush symlink path to avoid broken symlink after POR")
> Cc: stable@kernel.org
> Signed-off-by: Wenjie Qi <qiwenjie@xiaomi.com>
Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,