[RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT

[RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT

[Al Viro]

[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?

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

[Jan Kara]

Jaegeuk, Chao, any comment on this? It really looks like a filesystem
corruption issue in f2fs when whiteouts are used...

								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

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

[Jaegeuk Kim]

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

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

[Chao Yu]

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

Sorry for delay reply, I was busy handling product issues these days...

Let me check this ASAP.

Thanks,

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