Re: [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 10/26, Jan Kara wrote:
> Jaegeuk, Chao, any comment on this? It really looks like a filesystem
> corruption issue in f2fs when whiteouts are used...

Thanks Al and Jan for headsup.
Let us take a look as soon as possible.

> 
> 								Honza
> 
> On Tue 17-10-23 06:50:40, Al Viro wrote:
> > [f2fs folks Cc'd]
> > 
> > 	There's something very odd in f2fs_rename();
> > this:
> >         f2fs_down_write(&F2FS_I(old_inode)->i_sem);
> >         if (!old_dir_entry || whiteout)
> >                 file_lost_pino(old_inode);
> >         else   
> >                 /* adjust dir's i_pino to pass fsck check */
> >                 f2fs_i_pino_write(old_inode, new_dir->i_ino);
> >         f2fs_up_write(&F2FS_I(old_inode)->i_sem);
> > and this:
> >                 if (old_dir != new_dir && !whiteout)
> >                         f2fs_set_link(old_inode, old_dir_entry,
> >                                                 old_dir_page, new_dir);
> >                 else
> >                         f2fs_put_page(old_dir_page, 0);
> > The latter really stinks, especially considering
> > struct dentry *f2fs_get_parent(struct dentry *child)
> > {
> >         struct page *page;
> >         unsigned long ino = f2fs_inode_by_name(d_inode(child), &dotdot_name, &page);
> > 
> >         if (!ino) {
> >                 if (IS_ERR(page))
> >                         return ERR_CAST(page);
> >                 return ERR_PTR(-ENOENT);
> >         }
> >         return d_obtain_alias(f2fs_iget(child->d_sb, ino));
> > }
> > 
> > You want correct inumber in the ".." link.  And cross-directory
> > rename does move the source to new parent, even if you'd been asked
> > to leave a whiteout in the old place.
> > 
> > Why is that stuff conditional on whiteout?  AFAICS, that went into the
> > tree in the same commit that added RENAME_WHITEOUT support on f2fs,
> > mentioning "For now, we just try to follow the way that xfs/ext4 use"
> > in commit message.  But ext4 does *NOT* do anything of that sort -
> > at the time of that commit the relevant piece had been
> >         if (old.dir_bh) {
> > 		retval = ext4_rename_dir_finish(handle, &old, new.dir->i_ino);
> > and old.dir_bh is set by
> >                 retval = ext4_rename_dir_prepare(handle, &old);
> > a few lines prior, which is not conditional upon the whiteout.
> > 
> > What am I missing there?
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR