* Re: [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT
@ 2023-10-26 16:16 ` Jan Kara
0 siblings, 0 replies; 8+ 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] 8+ messages in thread* Re: [f2fs-dev] [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT
2023-10-26 16:16 ` Jan Kara
@ 2023-10-26 16:44 ` Jaegeuk Kim
-1 siblings, 0 replies; 8+ messages in thread
From: Jaegeuk Kim @ 2023-10-26 16:44 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-fsdevel, Al Viro, linux-f2fs-devel
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
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT
@ 2023-10-26 16:44 ` Jaegeuk Kim
0 siblings, 0 replies; 8+ 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] 8+ messages in thread
* Re: [f2fs-dev] [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT
2023-10-26 16:16 ` Jan Kara
@ 2023-11-07 13:55 ` Chao Yu
-1 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2023-11-07 13:55 UTC (permalink / raw)
To: Jan Kara, Al Viro; +Cc: linux-fsdevel, Jaegeuk Kim, linux-f2fs-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT
@ 2023-11-07 13:55 ` Chao Yu
0 siblings, 0 replies; 8+ 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] 8+ messages in thread