* [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT [not found] ` <20231012191551.GZ800259@ZenIV> @ 2023-10-17 5:50 ` Al Viro 2023-10-26 16:16 ` Jan Kara 0 siblings, 1 reply; 4+ messages in thread From: Al Viro @ 2023-10-17 5:50 UTC (permalink / raw) To: linux-f2fs-devel; +Cc: linux-fsdevel, Jan Kara, Jaegeuk Kim, Chao Yu [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? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT 2023-10-17 5:50 ` [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT Al Viro @ 2023-10-26 16:16 ` Jan Kara 2023-10-26 16:44 ` Jaegeuk Kim 2023-11-07 13:55 ` Chao Yu 0 siblings, 2 replies; 4+ messages in thread From: Jan Kara @ 2023-10-26 16:16 UTC (permalink / raw) To: Al Viro; +Cc: linux-f2fs-devel, linux-fsdevel, Jan Kara, Jaegeuk Kim, Chao Yu 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT 2023-10-26 16:16 ` Jan Kara @ 2023-10-26 16:44 ` Jaegeuk Kim 2023-11-07 13:55 ` Chao Yu 1 sibling, 0 replies; 4+ messages in thread From: Jaegeuk Kim @ 2023-10-26 16:44 UTC (permalink / raw) To: Jan Kara; +Cc: Al Viro, linux-f2fs-devel, linux-fsdevel, Chao Yu 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT 2023-10-26 16:16 ` Jan Kara 2023-10-26 16:44 ` Jaegeuk Kim @ 2023-11-07 13:55 ` Chao Yu 1 sibling, 0 replies; 4+ messages in thread From: Chao Yu @ 2023-11-07 13:55 UTC (permalink / raw) To: Jan Kara, Al Viro; +Cc: linux-f2fs-devel, linux-fsdevel, Jaegeuk Kim 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? ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-11-07 13:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20231011195620.GW800259@ZenIV>
[not found] ` <20231011203412.GA85476@ZenIV>
[not found] ` <CAHk-=wjSbompMCgMwR2-MB59QDB+OZ7Ohp878QoDc9o7z4pbNg@mail.gmail.com>
[not found] ` <20231011215138.GX800259@ZenIV>
[not found] ` <20231011230105.GA92231@ZenIV>
[not found] ` <CAHfrynNbfPtAjY4Y7N0cyWyH35dyF_BcpfR58ASCCC7=-TfSFw@mail.gmail.com>
[not found] ` <20231012050209.GY800259@ZenIV>
[not found] ` <20231012103157.mmn6sv4e6hfrqkai@quack3>
[not found] ` <20231012145758.yopnkhijksae5akp@quack3>
[not found] ` <20231012191551.GZ800259@ZenIV>
2023-10-17 5:50 ` [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT Al Viro
2023-10-26 16:16 ` Jan Kara
2023-10-26 16:44 ` Jaegeuk Kim
2023-11-07 13:55 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).