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

[Chao Yu <chao@kernel.org>]

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?


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