All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: linux-f2fs-devel@lists.sourceforge.net
Cc: linux-fsdevel@vger.kernel.org, Jan Kara <jack@suse.cz>,
	Jaegeuk Kim <jaegeuk@kernel.org>, Chao Yu <chao@kernel.org>
Subject: [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT
Date: Tue, 17 Oct 2023 06:50:40 +0100	[thread overview]
Message-ID: <20231017055040.GN800259@ZenIV> (raw)
In-Reply-To: <20231012191551.GZ800259@ZenIV>

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

WARNING: multiple messages have this Message-ID (diff)
From: Al Viro <viro@zeniv.linux.org.uk>
To: linux-f2fs-devel@lists.sourceforge.net
Cc: linux-fsdevel@vger.kernel.org, Jaegeuk Kim <jaegeuk@kernel.org>,
	Jan Kara <jack@suse.cz>
Subject: [f2fs-dev] [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT
Date: Tue, 17 Oct 2023 06:50:40 +0100	[thread overview]
Message-ID: <20231017055040.GN800259@ZenIV> (raw)
In-Reply-To: <20231012191551.GZ800259@ZenIV>

[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

       reply	other threads:[~2023-10-17  5:50 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [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                   ` Al Viro [this message]
2023-10-17  5:50                     ` [f2fs-dev] [RFC] weirdness in f2fs_rename() with RENAME_WHITEOUT Al Viro
2023-10-26 16:16                     ` Jan Kara
2023-10-26 16:16                       ` [f2fs-dev] " Jan Kara
2023-10-26 16:44                       ` Jaegeuk Kim
2023-10-26 16:44                         ` [f2fs-dev] " Jaegeuk Kim
2023-11-07 13:55                       ` Chao Yu
2023-11-07 13:55                         ` [f2fs-dev] " Chao Yu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231017055040.GN800259@ZenIV \
    --to=viro@zeniv.linux.org.uk \
    --cc=chao@kernel.org \
    --cc=jack@suse.cz \
    --cc=jaegeuk@kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fsdevel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.