Re: [f2fs-dev] [PATCH v2] f2fs: return symlink writeback errors

[Jaegeuk Kim via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

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()?


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