* Re: [PATCH 1/2] VFS: change old_dir and new_dir in struct renamedata to dentrys
2025-06-11 22:57 ` [PATCH 1/2] VFS: change old_dir and new_dir in struct renamedata to dentrys NeilBrown
@ 2025-06-11 23:38 ` Al Viro
2025-06-12 6:10 ` Miklos Szeredi
2025-06-12 6:20 ` Amir Goldstein
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Al Viro @ 2025-06-11 23:38 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Jan Kara, David Howells, Tyler Hicks,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
Kees Cook, Joel Granados, Namjae Jeon, Steve French,
Sergey Senozhatsky, netfs, linux-kernel, ecryptfs, linux-fsdevel,
linux-nfs, linux-unionfs, linux-cifs
On Thu, Jun 12, 2025 at 08:57:02AM +1000, NeilBrown wrote:
> all users of 'struct renamedata' have the dentry for the old and new
> directories, and often have no use for the inode except to store it in
> the renamedata.
>
> This patch changes struct renamedata to hold the dentry, rather than
> the inode, for the old and new directories, and changes callers to
> match.
>
> This results in the removal of several local variables and several
> dereferences of ->d_inode at the cost of adding ->d_inode dereferences
> to vfs_rename().
Umm... No objections, as long as overlayfs part is correct; it seems
to be, but I hadn't checked every chunk there...
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/2] VFS: change old_dir and new_dir in struct renamedata to dentrys
2025-06-11 23:38 ` Al Viro
@ 2025-06-12 6:10 ` Miklos Szeredi
2025-06-13 7:08 ` NeilBrown
0 siblings, 1 reply; 12+ messages in thread
From: Miklos Szeredi @ 2025-06-12 6:10 UTC (permalink / raw)
To: Al Viro
Cc: NeilBrown, Christian Brauner, Jan Kara, David Howells,
Tyler Hicks, Chuck Lever, Jeff Layton, Amir Goldstein, Kees Cook,
Joel Granados, Namjae Jeon, Steve French, Sergey Senozhatsky,
netfs, linux-kernel, ecryptfs, linux-fsdevel, linux-nfs,
linux-unionfs, linux-cifs
On Thu, 12 Jun 2025 at 01:38, Al Viro <viro@zeniv.linux.org.uk> wrote:
> Umm... No objections, as long as overlayfs part is correct; it seems
> to be, but I hadn't checked every chunk there...
Overlayfs parts looks okay too.
A followup would be nice (e.g. make ovl_cleanup() take a dentry for
the directory as well, etc) so that there's no need to have local
variables for both the inode and dentry of the directory.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] VFS: change old_dir and new_dir in struct renamedata to dentrys
2025-06-12 6:10 ` Miklos Szeredi
@ 2025-06-13 7:08 ` NeilBrown
0 siblings, 0 replies; 12+ messages in thread
From: NeilBrown @ 2025-06-13 7:08 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Al Viro, Christian Brauner, Jan Kara, David Howells, Tyler Hicks,
Chuck Lever, Jeff Layton, Amir Goldstein, Kees Cook,
Joel Granados, Namjae Jeon, Steve French, Sergey Senozhatsky,
netfs, linux-kernel, ecryptfs, linux-fsdevel, linux-nfs,
linux-unionfs, linux-cifs
On Thu, 12 Jun 2025, Miklos Szeredi wrote:
> On Thu, 12 Jun 2025 at 01:38, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> > Umm... No objections, as long as overlayfs part is correct; it seems
> > to be, but I hadn't checked every chunk there...
>
> Overlayfs parts looks okay too.
>
> A followup would be nice (e.g. make ovl_cleanup() take a dentry for
> the directory as well, etc) so that there's no need to have local
> variables for both the inode and dentry of the directory.
I am planning some followups and will include that in them.
I'll also be sure to test with fs-tests after consulting README.overlay
as you suggest elsewhere - thanks.
NeilBrown
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] VFS: change old_dir and new_dir in struct renamedata to dentrys
2025-06-11 22:57 ` [PATCH 1/2] VFS: change old_dir and new_dir in struct renamedata to dentrys NeilBrown
2025-06-11 23:38 ` Al Viro
@ 2025-06-12 6:20 ` Amir Goldstein
2025-06-12 23:22 ` NeilBrown
2025-06-12 13:03 ` Chuck Lever
2025-06-12 22:52 ` Namjae Jeon
3 siblings, 1 reply; 12+ messages in thread
From: Amir Goldstein @ 2025-06-12 6:20 UTC (permalink / raw)
To: NeilBrown
Cc: Alexander Viro, Christian Brauner, Jan Kara, David Howells,
Tyler Hicks, Chuck Lever, Jeff Layton, Miklos Szeredi, Kees Cook,
Joel Granados, Namjae Jeon, Steve French, Sergey Senozhatsky,
netfs, linux-kernel, ecryptfs, linux-fsdevel, linux-nfs,
linux-unionfs, linux-cifs
On Thu, Jun 12, 2025 at 12:59 AM NeilBrown <neil@brown.name> wrote:
>
> all users of 'struct renamedata' have the dentry for the old and new
> directories, and often have no use for the inode except to store it in
> the renamedata.
>
> This patch changes struct renamedata to hold the dentry, rather than
> the inode, for the old and new directories, and changes callers to
> match.
>
> This results in the removal of several local variables and several
> dereferences of ->d_inode at the cost of adding ->d_inode dereferences
> to vfs_rename().
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/cachefiles/namei.c | 4 ++--
> fs/ecryptfs/inode.c | 4 ++--
> fs/namei.c | 6 +++---
> fs/nfsd/vfs.c | 7 ++-----
> fs/overlayfs/copy_up.c | 6 +++---
> fs/overlayfs/dir.c | 16 ++++++++--------
> fs/overlayfs/overlayfs.h | 6 +++---
> fs/overlayfs/readdir.c | 2 +-
> fs/overlayfs/super.c | 2 +-
> fs/overlayfs/util.c | 2 +-
> fs/smb/server/vfs.c | 4 ++--
> include/linux/fs.h | 4 ++--
> 12 files changed, 30 insertions(+), 33 deletions(-)
>
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index aecfc5c37b49..053fc28b5423 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -388,10 +388,10 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
> } else {
> struct renamedata rd = {
> .old_mnt_idmap = &nop_mnt_idmap,
> - .old_dir = d_inode(dir),
> + .old_dir = dir,
> .old_dentry = rep,
> .new_mnt_idmap = &nop_mnt_idmap,
> - .new_dir = d_inode(cache->graveyard),
> + .new_dir = cache->graveyard,
> .new_dentry = grave,
> };
> trace_cachefiles_rename(object, d_inode(rep)->i_ino, why);
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 493d7f194956..c9fec8b7e000 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -635,10 +635,10 @@ ecryptfs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
> }
>
> rd.old_mnt_idmap = &nop_mnt_idmap;
> - rd.old_dir = d_inode(lower_old_dir_dentry);
> + rd.old_dir = lower_old_dir_dentry;
> rd.old_dentry = lower_old_dentry;
> rd.new_mnt_idmap = &nop_mnt_idmap;
> - rd.new_dir = d_inode(lower_new_dir_dentry);
> + rd.new_dir = lower_new_dir_dentry;
> rd.new_dentry = lower_new_dentry;
> rc = vfs_rename(&rd);
> if (rc)
> diff --git a/fs/namei.c b/fs/namei.c
> index 019073162b8a..5b0be8bca50d 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -5007,7 +5007,7 @@ SYSCALL_DEFINE2(link, const char __user *, oldname, const char __user *, newname
> int vfs_rename(struct renamedata *rd)
> {
> int error;
> - struct inode *old_dir = rd->old_dir, *new_dir = rd->new_dir;
> + struct inode *old_dir = d_inode(rd->old_dir), *new_dir = d_inode(rd->new_dir);
> struct dentry *old_dentry = rd->old_dentry;
> struct dentry *new_dentry = rd->new_dentry;
> struct inode **delegated_inode = rd->delegated_inode;
> @@ -5266,10 +5266,10 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
> if (error)
> goto exit5;
>
> - rd.old_dir = old_path.dentry->d_inode;
> + rd.old_dir = old_path.dentry;
> rd.old_dentry = old_dentry;
> rd.old_mnt_idmap = mnt_idmap(old_path.mnt);
> - rd.new_dir = new_path.dentry->d_inode;
> + rd.new_dir = new_path.dentry;
> rd.new_dentry = new_dentry;
> rd.new_mnt_idmap = mnt_idmap(new_path.mnt);
> rd.delegated_inode = &delegated_inode;
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index cd689df2ca5d..3c87fbd22c57 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1864,7 +1864,6 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> struct svc_fh *tfhp, char *tname, int tlen)
> {
> struct dentry *fdentry, *tdentry, *odentry, *ndentry, *trap;
> - struct inode *fdir, *tdir;
> int type = S_IFDIR;
> __be32 err;
> int host_err;
> @@ -1880,10 +1879,8 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> goto out;
>
> fdentry = ffhp->fh_dentry;
> - fdir = d_inode(fdentry);
>
> tdentry = tfhp->fh_dentry;
> - tdir = d_inode(tdentry);
>
> err = nfserr_perm;
> if (!flen || isdotent(fname, flen) || !tlen || isdotent(tname, tlen))
> @@ -1944,10 +1941,10 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> } else {
> struct renamedata rd = {
> .old_mnt_idmap = &nop_mnt_idmap,
> - .old_dir = fdir,
> + .old_dir = fdentry,
> .old_dentry = odentry,
> .new_mnt_idmap = &nop_mnt_idmap,
> - .new_dir = tdir,
> + .new_dir = tdentry,
> .new_dentry = ndentry,
> };
> int retries;
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index d7310fcf3888..8a3c0d18ec2e 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -563,7 +563,7 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> if (IS_ERR(index)) {
> err = PTR_ERR(index);
> } else {
> - err = ovl_do_rename(ofs, dir, temp, dir, index, 0);
> + err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
> dput(index);
> }
> out:
> @@ -762,7 +762,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> {
> struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
> struct inode *inode;
> - struct inode *udir = d_inode(c->destdir), *wdir = d_inode(c->workdir);
> + struct inode *wdir = d_inode(c->workdir);
> struct path path = { .mnt = ovl_upper_mnt(ofs) };
> struct dentry *temp, *upper, *trap;
> struct ovl_cu_creds cc;
> @@ -829,7 +829,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> if (IS_ERR(upper))
> goto cleanup;
>
> - err = ovl_do_rename(ofs, wdir, temp, udir, upper, 0);
> + err = ovl_do_rename(ofs, c->workdir, temp, c->destdir, upper, 0);
> dput(upper);
> if (err)
> goto cleanup;
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index fe493f3ed6b6..4fc221ea6480 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -107,7 +107,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> }
>
> /* Caller must hold i_mutex on both workdir and dir */
> -int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> struct dentry *dentry)
> {
> struct inode *wdir = ofs->workdir->d_inode;
> @@ -123,7 +123,7 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> if (d_is_dir(dentry))
> flags = RENAME_EXCHANGE;
>
> - err = ovl_do_rename(ofs, wdir, whiteout, dir, dentry, flags);
> + err = ovl_do_rename(ofs, ofs->workdir, whiteout, dir, dentry, flags);
> if (err)
> goto kill_whiteout;
> if (flags)
> @@ -384,7 +384,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
> if (err)
> goto out_cleanup;
>
> - err = ovl_do_rename(ofs, wdir, opaquedir, udir, upper, RENAME_EXCHANGE);
> + err = ovl_do_rename(ofs, workdir, opaquedir, upperdir, upper, RENAME_EXCHANGE);
> if (err)
> goto out_cleanup;
>
> @@ -491,14 +491,14 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
> if (err)
> goto out_cleanup;
>
> - err = ovl_do_rename(ofs, wdir, newdentry, udir, upper,
> + err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper,
> RENAME_EXCHANGE);
> if (err)
> goto out_cleanup;
>
> ovl_cleanup(ofs, wdir, upper);
> } else {
> - err = ovl_do_rename(ofs, wdir, newdentry, udir, upper, 0);
> + err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper, 0);
> if (err)
> goto out_cleanup;
> }
> @@ -774,7 +774,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
> goto out_dput_upper;
> }
>
> - err = ovl_cleanup_and_whiteout(ofs, d_inode(upperdir), upper);
> + err = ovl_cleanup_and_whiteout(ofs, upperdir, upper);
> if (err)
> goto out_d_drop;
>
> @@ -1246,8 +1246,8 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> if (err)
> goto out_dput;
>
> - err = ovl_do_rename(ofs, old_upperdir->d_inode, olddentry,
> - new_upperdir->d_inode, newdentry, flags);
> + err = ovl_do_rename(ofs, old_upperdir, olddentry,
> + new_upperdir, newdentry, flags);
> if (err)
> goto out_dput;
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 8baaba0a3fe5..65f9d51bed7c 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -353,8 +353,8 @@ static inline int ovl_do_remove_acl(struct ovl_fs *ofs, struct dentry *dentry,
> return vfs_remove_acl(ovl_upper_mnt_idmap(ofs), dentry, acl_name);
> }
>
> -static inline int ovl_do_rename(struct ovl_fs *ofs, struct inode *olddir,
> - struct dentry *olddentry, struct inode *newdir,
> +static inline int ovl_do_rename(struct ovl_fs *ofs, struct dentry *olddir,
> + struct dentry *olddentry, struct dentry *newdir,
> struct dentry *newdentry, unsigned int flags)
> {
> int err;
> @@ -826,7 +826,7 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
>
> /* dir.c */
> extern const struct inode_operations ovl_dir_inode_operations;
> -int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> struct dentry *dentry);
> struct ovl_cattr {
> dev_t rdev;
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 474c80d210d1..68cca52ae2ac 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1235,7 +1235,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
> * Whiteout orphan index to block future open by
> * handle after overlay nlink dropped to zero.
> */
> - err = ovl_cleanup_and_whiteout(ofs, dir, index);
> + err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
> } else {
> /* Cleanup orphan index entries */
> err = ovl_cleanup(ofs, dir, index);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index e19940d649ca..cf99b276fdfb 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -580,7 +580,7 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
>
> /* Name is inline and stable - using snapshot as a copy helper */
> take_dentry_name_snapshot(&name, temp);
> - err = ovl_do_rename(ofs, dir, temp, dir, dest, RENAME_WHITEOUT);
> + err = ovl_do_rename(ofs, workdir, temp, workdir, dest, RENAME_WHITEOUT);
> if (err) {
> if (err == -EINVAL)
> err = 0;
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index dcccb4b4a66c..2b4754c645ee 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1115,7 +1115,7 @@ static void ovl_cleanup_index(struct dentry *dentry)
> } else if (ovl_index_all(dentry->d_sb)) {
> /* Whiteout orphan index to block future open by handle */
> err = ovl_cleanup_and_whiteout(OVL_FS(dentry->d_sb),
> - dir, index);
> + indexdir, index);
> } else {
> /* Cleanup orphan index entries */
> err = ovl_cleanup(ofs, dir, index);
> diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
> index ba45e809555a..b8d913c61623 100644
> --- a/fs/smb/server/vfs.c
> +++ b/fs/smb/server/vfs.c
> @@ -764,10 +764,10 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
> }
>
> rd.old_mnt_idmap = mnt_idmap(old_path->mnt),
> - rd.old_dir = d_inode(old_parent),
> + rd.old_dir = old_parent,
> rd.old_dentry = old_child,
> rd.new_mnt_idmap = mnt_idmap(new_path.mnt),
> - rd.new_dir = new_path.dentry->d_inode,
> + rd.new_dir = new_path.dentry,
> rd.new_dentry = new_dentry,
> rd.flags = flags,
> rd.delegated_inode = NULL,
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 16f40a6f8264..9a83904c9d4a 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2016,10 +2016,10 @@ int vfs_unlink(struct mnt_idmap *, struct inode *, struct dentry *,
> */
> struct renamedata {
> struct mnt_idmap *old_mnt_idmap;
> - struct inode *old_dir;
> + struct dentry *old_dir;
> struct dentry *old_dentry;
> struct mnt_idmap *new_mnt_idmap;
> - struct inode *new_dir;
> + struct dentry *new_dir;
> struct dentry *new_dentry;
> struct inode **delegated_inode;
> unsigned int flags;
> --
It bothers me a bit that we are keeping the field name while changing its type.
There is a wide convention in vfs methods and helpers of using
struct inode *dir
as the parent directory inode
and often (but not always) using
struct dentry *parent
as the parent dentry
How do you feel about making struct renamedata use:
struct dentry *old_parent;
struct dentry *new_parent;
I don't think it will add any churn beyond what this patch already does.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/2] VFS: change old_dir and new_dir in struct renamedata to dentrys
2025-06-12 6:20 ` Amir Goldstein
@ 2025-06-12 23:22 ` NeilBrown
0 siblings, 0 replies; 12+ messages in thread
From: NeilBrown @ 2025-06-12 23:22 UTC (permalink / raw)
To: Amir Goldstein
Cc: Alexander Viro, Christian Brauner, Jan Kara, David Howells,
Tyler Hicks, Chuck Lever, Jeff Layton, Miklos Szeredi, Kees Cook,
Joel Granados, Namjae Jeon, Steve French, Sergey Senozhatsky,
netfs, linux-kernel, ecryptfs, linux-fsdevel, linux-nfs,
linux-unionfs, linux-cifs
On Thu, 12 Jun 2025, Amir Goldstein wrote:
> On Thu, Jun 12, 2025 at 12:59 AM NeilBrown <neil@brown.name> wrote:
> >
> > all users of 'struct renamedata' have the dentry for the old and new
> > directories, and often have no use for the inode except to store it in
> > the renamedata.
> >
> > This patch changes struct renamedata to hold the dentry, rather than
> > the inode, for the old and new directories, and changes callers to
> > match.
> >
> > This results in the removal of several local variables and several
> > dereferences of ->d_inode at the cost of adding ->d_inode dereferences
> > to vfs_rename().
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> > fs/cachefiles/namei.c | 4 ++--
> > fs/ecryptfs/inode.c | 4 ++--
> > fs/namei.c | 6 +++---
> > fs/nfsd/vfs.c | 7 ++-----
> > fs/overlayfs/copy_up.c | 6 +++---
> > fs/overlayfs/dir.c | 16 ++++++++--------
> > fs/overlayfs/overlayfs.h | 6 +++---
> > fs/overlayfs/readdir.c | 2 +-
> > fs/overlayfs/super.c | 2 +-
> > fs/overlayfs/util.c | 2 +-
> > fs/smb/server/vfs.c | 4 ++--
> > include/linux/fs.h | 4 ++--
> > 12 files changed, 30 insertions(+), 33 deletions(-)
> >
> > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> > index aecfc5c37b49..053fc28b5423 100644
> > --- a/fs/cachefiles/namei.c
> > +++ b/fs/cachefiles/namei.c
> > @@ -388,10 +388,10 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
> > } else {
> > struct renamedata rd = {
> > .old_mnt_idmap = &nop_mnt_idmap,
> > - .old_dir = d_inode(dir),
> > + .old_dir = dir,
> > .old_dentry = rep,
> > .new_mnt_idmap = &nop_mnt_idmap,
> > - .new_dir = d_inode(cache->graveyard),
> > + .new_dir = cache->graveyard,
> > .new_dentry = grave,
> > };
> > trace_cachefiles_rename(object, d_inode(rep)->i_ino, why);
> > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> > index 493d7f194956..c9fec8b7e000 100644
> > --- a/fs/ecryptfs/inode.c
> > +++ b/fs/ecryptfs/inode.c
> > @@ -635,10 +635,10 @@ ecryptfs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
> > }
> >
> > rd.old_mnt_idmap = &nop_mnt_idmap;
> > - rd.old_dir = d_inode(lower_old_dir_dentry);
> > + rd.old_dir = lower_old_dir_dentry;
> > rd.old_dentry = lower_old_dentry;
> > rd.new_mnt_idmap = &nop_mnt_idmap;
> > - rd.new_dir = d_inode(lower_new_dir_dentry);
> > + rd.new_dir = lower_new_dir_dentry;
> > rd.new_dentry = lower_new_dentry;
> > rc = vfs_rename(&rd);
> > if (rc)
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 019073162b8a..5b0be8bca50d 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -5007,7 +5007,7 @@ SYSCALL_DEFINE2(link, const char __user *, oldname, const char __user *, newname
> > int vfs_rename(struct renamedata *rd)
> > {
> > int error;
> > - struct inode *old_dir = rd->old_dir, *new_dir = rd->new_dir;
> > + struct inode *old_dir = d_inode(rd->old_dir), *new_dir = d_inode(rd->new_dir);
> > struct dentry *old_dentry = rd->old_dentry;
> > struct dentry *new_dentry = rd->new_dentry;
> > struct inode **delegated_inode = rd->delegated_inode;
> > @@ -5266,10 +5266,10 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
> > if (error)
> > goto exit5;
> >
> > - rd.old_dir = old_path.dentry->d_inode;
> > + rd.old_dir = old_path.dentry;
> > rd.old_dentry = old_dentry;
> > rd.old_mnt_idmap = mnt_idmap(old_path.mnt);
> > - rd.new_dir = new_path.dentry->d_inode;
> > + rd.new_dir = new_path.dentry;
> > rd.new_dentry = new_dentry;
> > rd.new_mnt_idmap = mnt_idmap(new_path.mnt);
> > rd.delegated_inode = &delegated_inode;
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index cd689df2ca5d..3c87fbd22c57 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1864,7 +1864,6 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> > struct svc_fh *tfhp, char *tname, int tlen)
> > {
> > struct dentry *fdentry, *tdentry, *odentry, *ndentry, *trap;
> > - struct inode *fdir, *tdir;
> > int type = S_IFDIR;
> > __be32 err;
> > int host_err;
> > @@ -1880,10 +1879,8 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> > goto out;
> >
> > fdentry = ffhp->fh_dentry;
> > - fdir = d_inode(fdentry);
> >
> > tdentry = tfhp->fh_dentry;
> > - tdir = d_inode(tdentry);
> >
> > err = nfserr_perm;
> > if (!flen || isdotent(fname, flen) || !tlen || isdotent(tname, tlen))
> > @@ -1944,10 +1941,10 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> > } else {
> > struct renamedata rd = {
> > .old_mnt_idmap = &nop_mnt_idmap,
> > - .old_dir = fdir,
> > + .old_dir = fdentry,
> > .old_dentry = odentry,
> > .new_mnt_idmap = &nop_mnt_idmap,
> > - .new_dir = tdir,
> > + .new_dir = tdentry,
> > .new_dentry = ndentry,
> > };
> > int retries;
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index d7310fcf3888..8a3c0d18ec2e 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -563,7 +563,7 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> > if (IS_ERR(index)) {
> > err = PTR_ERR(index);
> > } else {
> > - err = ovl_do_rename(ofs, dir, temp, dir, index, 0);
> > + err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
> > dput(index);
> > }
> > out:
> > @@ -762,7 +762,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> > {
> > struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
> > struct inode *inode;
> > - struct inode *udir = d_inode(c->destdir), *wdir = d_inode(c->workdir);
> > + struct inode *wdir = d_inode(c->workdir);
> > struct path path = { .mnt = ovl_upper_mnt(ofs) };
> > struct dentry *temp, *upper, *trap;
> > struct ovl_cu_creds cc;
> > @@ -829,7 +829,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> > if (IS_ERR(upper))
> > goto cleanup;
> >
> > - err = ovl_do_rename(ofs, wdir, temp, udir, upper, 0);
> > + err = ovl_do_rename(ofs, c->workdir, temp, c->destdir, upper, 0);
> > dput(upper);
> > if (err)
> > goto cleanup;
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index fe493f3ed6b6..4fc221ea6480 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -107,7 +107,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> > }
> >
> > /* Caller must hold i_mutex on both workdir and dir */
> > -int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> > +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> > struct dentry *dentry)
> > {
> > struct inode *wdir = ofs->workdir->d_inode;
> > @@ -123,7 +123,7 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> > if (d_is_dir(dentry))
> > flags = RENAME_EXCHANGE;
> >
> > - err = ovl_do_rename(ofs, wdir, whiteout, dir, dentry, flags);
> > + err = ovl_do_rename(ofs, ofs->workdir, whiteout, dir, dentry, flags);
> > if (err)
> > goto kill_whiteout;
> > if (flags)
> > @@ -384,7 +384,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
> > if (err)
> > goto out_cleanup;
> >
> > - err = ovl_do_rename(ofs, wdir, opaquedir, udir, upper, RENAME_EXCHANGE);
> > + err = ovl_do_rename(ofs, workdir, opaquedir, upperdir, upper, RENAME_EXCHANGE);
> > if (err)
> > goto out_cleanup;
> >
> > @@ -491,14 +491,14 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
> > if (err)
> > goto out_cleanup;
> >
> > - err = ovl_do_rename(ofs, wdir, newdentry, udir, upper,
> > + err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper,
> > RENAME_EXCHANGE);
> > if (err)
> > goto out_cleanup;
> >
> > ovl_cleanup(ofs, wdir, upper);
> > } else {
> > - err = ovl_do_rename(ofs, wdir, newdentry, udir, upper, 0);
> > + err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper, 0);
> > if (err)
> > goto out_cleanup;
> > }
> > @@ -774,7 +774,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
> > goto out_dput_upper;
> > }
> >
> > - err = ovl_cleanup_and_whiteout(ofs, d_inode(upperdir), upper);
> > + err = ovl_cleanup_and_whiteout(ofs, upperdir, upper);
> > if (err)
> > goto out_d_drop;
> >
> > @@ -1246,8 +1246,8 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> > if (err)
> > goto out_dput;
> >
> > - err = ovl_do_rename(ofs, old_upperdir->d_inode, olddentry,
> > - new_upperdir->d_inode, newdentry, flags);
> > + err = ovl_do_rename(ofs, old_upperdir, olddentry,
> > + new_upperdir, newdentry, flags);
> > if (err)
> > goto out_dput;
> >
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index 8baaba0a3fe5..65f9d51bed7c 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -353,8 +353,8 @@ static inline int ovl_do_remove_acl(struct ovl_fs *ofs, struct dentry *dentry,
> > return vfs_remove_acl(ovl_upper_mnt_idmap(ofs), dentry, acl_name);
> > }
> >
> > -static inline int ovl_do_rename(struct ovl_fs *ofs, struct inode *olddir,
> > - struct dentry *olddentry, struct inode *newdir,
> > +static inline int ovl_do_rename(struct ovl_fs *ofs, struct dentry *olddir,
> > + struct dentry *olddentry, struct dentry *newdir,
> > struct dentry *newdentry, unsigned int flags)
> > {
> > int err;
> > @@ -826,7 +826,7 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
> >
> > /* dir.c */
> > extern const struct inode_operations ovl_dir_inode_operations;
> > -int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> > +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> > struct dentry *dentry);
> > struct ovl_cattr {
> > dev_t rdev;
> > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > index 474c80d210d1..68cca52ae2ac 100644
> > --- a/fs/overlayfs/readdir.c
> > +++ b/fs/overlayfs/readdir.c
> > @@ -1235,7 +1235,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
> > * Whiteout orphan index to block future open by
> > * handle after overlay nlink dropped to zero.
> > */
> > - err = ovl_cleanup_and_whiteout(ofs, dir, index);
> > + err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
> > } else {
> > /* Cleanup orphan index entries */
> > err = ovl_cleanup(ofs, dir, index);
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index e19940d649ca..cf99b276fdfb 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -580,7 +580,7 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
> >
> > /* Name is inline and stable - using snapshot as a copy helper */
> > take_dentry_name_snapshot(&name, temp);
> > - err = ovl_do_rename(ofs, dir, temp, dir, dest, RENAME_WHITEOUT);
> > + err = ovl_do_rename(ofs, workdir, temp, workdir, dest, RENAME_WHITEOUT);
> > if (err) {
> > if (err == -EINVAL)
> > err = 0;
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index dcccb4b4a66c..2b4754c645ee 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -1115,7 +1115,7 @@ static void ovl_cleanup_index(struct dentry *dentry)
> > } else if (ovl_index_all(dentry->d_sb)) {
> > /* Whiteout orphan index to block future open by handle */
> > err = ovl_cleanup_and_whiteout(OVL_FS(dentry->d_sb),
> > - dir, index);
> > + indexdir, index);
> > } else {
> > /* Cleanup orphan index entries */
> > err = ovl_cleanup(ofs, dir, index);
> > diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
> > index ba45e809555a..b8d913c61623 100644
> > --- a/fs/smb/server/vfs.c
> > +++ b/fs/smb/server/vfs.c
> > @@ -764,10 +764,10 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
> > }
> >
> > rd.old_mnt_idmap = mnt_idmap(old_path->mnt),
> > - rd.old_dir = d_inode(old_parent),
> > + rd.old_dir = old_parent,
> > rd.old_dentry = old_child,
> > rd.new_mnt_idmap = mnt_idmap(new_path.mnt),
> > - rd.new_dir = new_path.dentry->d_inode,
> > + rd.new_dir = new_path.dentry,
> > rd.new_dentry = new_dentry,
> > rd.flags = flags,
> > rd.delegated_inode = NULL,
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 16f40a6f8264..9a83904c9d4a 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2016,10 +2016,10 @@ int vfs_unlink(struct mnt_idmap *, struct inode *, struct dentry *,
> > */
> > struct renamedata {
> > struct mnt_idmap *old_mnt_idmap;
> > - struct inode *old_dir;
> > + struct dentry *old_dir;
> > struct dentry *old_dentry;
> > struct mnt_idmap *new_mnt_idmap;
> > - struct inode *new_dir;
> > + struct dentry *new_dir;
> > struct dentry *new_dentry;
> > struct inode **delegated_inode;
> > unsigned int flags;
> > --
>
> It bothers me a bit that we are keeping the field name while changing its type.
>
> There is a wide convention in vfs methods and helpers of using
> struct inode *dir
> as the parent directory inode
> and often (but not always) using
> struct dentry *parent
> as the parent dentry
>
> How do you feel about making struct renamedata use:
>
> struct dentry *old_parent;
> struct dentry *new_parent;
>
> I don't think it will add any churn beyond what this patch already does.
I think that is an excellent idea - thanks.
Particularly as the kernel-doc documentation for struct renamedata
describe the fields as "parent of source" and "parent of destination".
I'll resubmit with that change and the reviewed-bys.
Thanks,
NeilBrown
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] VFS: change old_dir and new_dir in struct renamedata to dentrys
2025-06-11 22:57 ` [PATCH 1/2] VFS: change old_dir and new_dir in struct renamedata to dentrys NeilBrown
2025-06-11 23:38 ` Al Viro
2025-06-12 6:20 ` Amir Goldstein
@ 2025-06-12 13:03 ` Chuck Lever
2025-06-12 22:52 ` Namjae Jeon
3 siblings, 0 replies; 12+ messages in thread
From: Chuck Lever @ 2025-06-12 13:03 UTC (permalink / raw)
To: NeilBrown, Alexander Viro, Christian Brauner, Jan Kara
Cc: David Howells, Tyler Hicks, Jeff Layton, Miklos Szeredi,
Amir Goldstein, Kees Cook, Joel Granados, Namjae Jeon,
Steve French, Sergey Senozhatsky, netfs, linux-kernel, ecryptfs,
linux-fsdevel, linux-nfs, linux-unionfs, linux-cifs
On 6/11/25 6:57 PM, NeilBrown wrote:
> all users of 'struct renamedata' have the dentry for the old and new
> directories, and often have no use for the inode except to store it in
> the renamedata.
>
> This patch changes struct renamedata to hold the dentry, rather than
> the inode, for the old and new directories, and changes callers to
> match.
>
> This results in the removal of several local variables and several
> dereferences of ->d_inode at the cost of adding ->d_inode dereferences
> to vfs_rename().
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/cachefiles/namei.c | 4 ++--
> fs/ecryptfs/inode.c | 4 ++--
> fs/namei.c | 6 +++---
> fs/nfsd/vfs.c | 7 ++-----
> fs/overlayfs/copy_up.c | 6 +++---
> fs/overlayfs/dir.c | 16 ++++++++--------
> fs/overlayfs/overlayfs.h | 6 +++---
> fs/overlayfs/readdir.c | 2 +-
> fs/overlayfs/super.c | 2 +-
> fs/overlayfs/util.c | 2 +-
> fs/smb/server/vfs.c | 4 ++--
> include/linux/fs.h | 4 ++--
> 12 files changed, 30 insertions(+), 33 deletions(-)
>
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index aecfc5c37b49..053fc28b5423 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -388,10 +388,10 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
> } else {
> struct renamedata rd = {
> .old_mnt_idmap = &nop_mnt_idmap,
> - .old_dir = d_inode(dir),
> + .old_dir = dir,
> .old_dentry = rep,
> .new_mnt_idmap = &nop_mnt_idmap,
> - .new_dir = d_inode(cache->graveyard),
> + .new_dir = cache->graveyard,
> .new_dentry = grave,
> };
> trace_cachefiles_rename(object, d_inode(rep)->i_ino, why);
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 493d7f194956..c9fec8b7e000 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -635,10 +635,10 @@ ecryptfs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
> }
>
> rd.old_mnt_idmap = &nop_mnt_idmap;
> - rd.old_dir = d_inode(lower_old_dir_dentry);
> + rd.old_dir = lower_old_dir_dentry;
> rd.old_dentry = lower_old_dentry;
> rd.new_mnt_idmap = &nop_mnt_idmap;
> - rd.new_dir = d_inode(lower_new_dir_dentry);
> + rd.new_dir = lower_new_dir_dentry;
> rd.new_dentry = lower_new_dentry;
> rc = vfs_rename(&rd);
> if (rc)
> diff --git a/fs/namei.c b/fs/namei.c
> index 019073162b8a..5b0be8bca50d 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -5007,7 +5007,7 @@ SYSCALL_DEFINE2(link, const char __user *, oldname, const char __user *, newname
> int vfs_rename(struct renamedata *rd)
> {
> int error;
> - struct inode *old_dir = rd->old_dir, *new_dir = rd->new_dir;
> + struct inode *old_dir = d_inode(rd->old_dir), *new_dir = d_inode(rd->new_dir);
> struct dentry *old_dentry = rd->old_dentry;
> struct dentry *new_dentry = rd->new_dentry;
> struct inode **delegated_inode = rd->delegated_inode;
> @@ -5266,10 +5266,10 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
> if (error)
> goto exit5;
>
> - rd.old_dir = old_path.dentry->d_inode;
> + rd.old_dir = old_path.dentry;
> rd.old_dentry = old_dentry;
> rd.old_mnt_idmap = mnt_idmap(old_path.mnt);
> - rd.new_dir = new_path.dentry->d_inode;
> + rd.new_dir = new_path.dentry;
> rd.new_dentry = new_dentry;
> rd.new_mnt_idmap = mnt_idmap(new_path.mnt);
> rd.delegated_inode = &delegated_inode;
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index cd689df2ca5d..3c87fbd22c57 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1864,7 +1864,6 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> struct svc_fh *tfhp, char *tname, int tlen)
> {
> struct dentry *fdentry, *tdentry, *odentry, *ndentry, *trap;
> - struct inode *fdir, *tdir;
> int type = S_IFDIR;
> __be32 err;
> int host_err;
> @@ -1880,10 +1879,8 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> goto out;
>
> fdentry = ffhp->fh_dentry;
> - fdir = d_inode(fdentry);
>
> tdentry = tfhp->fh_dentry;
> - tdir = d_inode(tdentry);
>
> err = nfserr_perm;
> if (!flen || isdotent(fname, flen) || !tlen || isdotent(tname, tlen))
> @@ -1944,10 +1941,10 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> } else {
> struct renamedata rd = {
> .old_mnt_idmap = &nop_mnt_idmap,
> - .old_dir = fdir,
> + .old_dir = fdentry,
> .old_dentry = odentry,
> .new_mnt_idmap = &nop_mnt_idmap,
> - .new_dir = tdir,
> + .new_dir = tdentry,
> .new_dentry = ndentry,
> };
> int retries;
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index d7310fcf3888..8a3c0d18ec2e 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -563,7 +563,7 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> if (IS_ERR(index)) {
> err = PTR_ERR(index);
> } else {
> - err = ovl_do_rename(ofs, dir, temp, dir, index, 0);
> + err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
> dput(index);
> }
> out:
> @@ -762,7 +762,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> {
> struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
> struct inode *inode;
> - struct inode *udir = d_inode(c->destdir), *wdir = d_inode(c->workdir);
> + struct inode *wdir = d_inode(c->workdir);
> struct path path = { .mnt = ovl_upper_mnt(ofs) };
> struct dentry *temp, *upper, *trap;
> struct ovl_cu_creds cc;
> @@ -829,7 +829,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> if (IS_ERR(upper))
> goto cleanup;
>
> - err = ovl_do_rename(ofs, wdir, temp, udir, upper, 0);
> + err = ovl_do_rename(ofs, c->workdir, temp, c->destdir, upper, 0);
> dput(upper);
> if (err)
> goto cleanup;
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index fe493f3ed6b6..4fc221ea6480 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -107,7 +107,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> }
>
> /* Caller must hold i_mutex on both workdir and dir */
> -int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> struct dentry *dentry)
> {
> struct inode *wdir = ofs->workdir->d_inode;
> @@ -123,7 +123,7 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> if (d_is_dir(dentry))
> flags = RENAME_EXCHANGE;
>
> - err = ovl_do_rename(ofs, wdir, whiteout, dir, dentry, flags);
> + err = ovl_do_rename(ofs, ofs->workdir, whiteout, dir, dentry, flags);
> if (err)
> goto kill_whiteout;
> if (flags)
> @@ -384,7 +384,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
> if (err)
> goto out_cleanup;
>
> - err = ovl_do_rename(ofs, wdir, opaquedir, udir, upper, RENAME_EXCHANGE);
> + err = ovl_do_rename(ofs, workdir, opaquedir, upperdir, upper, RENAME_EXCHANGE);
> if (err)
> goto out_cleanup;
>
> @@ -491,14 +491,14 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
> if (err)
> goto out_cleanup;
>
> - err = ovl_do_rename(ofs, wdir, newdentry, udir, upper,
> + err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper,
> RENAME_EXCHANGE);
> if (err)
> goto out_cleanup;
>
> ovl_cleanup(ofs, wdir, upper);
> } else {
> - err = ovl_do_rename(ofs, wdir, newdentry, udir, upper, 0);
> + err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper, 0);
> if (err)
> goto out_cleanup;
> }
> @@ -774,7 +774,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
> goto out_dput_upper;
> }
>
> - err = ovl_cleanup_and_whiteout(ofs, d_inode(upperdir), upper);
> + err = ovl_cleanup_and_whiteout(ofs, upperdir, upper);
> if (err)
> goto out_d_drop;
>
> @@ -1246,8 +1246,8 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
> if (err)
> goto out_dput;
>
> - err = ovl_do_rename(ofs, old_upperdir->d_inode, olddentry,
> - new_upperdir->d_inode, newdentry, flags);
> + err = ovl_do_rename(ofs, old_upperdir, olddentry,
> + new_upperdir, newdentry, flags);
> if (err)
> goto out_dput;
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 8baaba0a3fe5..65f9d51bed7c 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -353,8 +353,8 @@ static inline int ovl_do_remove_acl(struct ovl_fs *ofs, struct dentry *dentry,
> return vfs_remove_acl(ovl_upper_mnt_idmap(ofs), dentry, acl_name);
> }
>
> -static inline int ovl_do_rename(struct ovl_fs *ofs, struct inode *olddir,
> - struct dentry *olddentry, struct inode *newdir,
> +static inline int ovl_do_rename(struct ovl_fs *ofs, struct dentry *olddir,
> + struct dentry *olddentry, struct dentry *newdir,
> struct dentry *newdentry, unsigned int flags)
> {
> int err;
> @@ -826,7 +826,7 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
>
> /* dir.c */
> extern const struct inode_operations ovl_dir_inode_operations;
> -int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> struct dentry *dentry);
> struct ovl_cattr {
> dev_t rdev;
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 474c80d210d1..68cca52ae2ac 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1235,7 +1235,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
> * Whiteout orphan index to block future open by
> * handle after overlay nlink dropped to zero.
> */
> - err = ovl_cleanup_and_whiteout(ofs, dir, index);
> + err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
> } else {
> /* Cleanup orphan index entries */
> err = ovl_cleanup(ofs, dir, index);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index e19940d649ca..cf99b276fdfb 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -580,7 +580,7 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
>
> /* Name is inline and stable - using snapshot as a copy helper */
> take_dentry_name_snapshot(&name, temp);
> - err = ovl_do_rename(ofs, dir, temp, dir, dest, RENAME_WHITEOUT);
> + err = ovl_do_rename(ofs, workdir, temp, workdir, dest, RENAME_WHITEOUT);
> if (err) {
> if (err == -EINVAL)
> err = 0;
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index dcccb4b4a66c..2b4754c645ee 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1115,7 +1115,7 @@ static void ovl_cleanup_index(struct dentry *dentry)
> } else if (ovl_index_all(dentry->d_sb)) {
> /* Whiteout orphan index to block future open by handle */
> err = ovl_cleanup_and_whiteout(OVL_FS(dentry->d_sb),
> - dir, index);
> + indexdir, index);
> } else {
> /* Cleanup orphan index entries */
> err = ovl_cleanup(ofs, dir, index);
> diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
> index ba45e809555a..b8d913c61623 100644
> --- a/fs/smb/server/vfs.c
> +++ b/fs/smb/server/vfs.c
> @@ -764,10 +764,10 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
> }
>
> rd.old_mnt_idmap = mnt_idmap(old_path->mnt),
> - rd.old_dir = d_inode(old_parent),
> + rd.old_dir = old_parent,
> rd.old_dentry = old_child,
> rd.new_mnt_idmap = mnt_idmap(new_path.mnt),
> - rd.new_dir = new_path.dentry->d_inode,
> + rd.new_dir = new_path.dentry,
> rd.new_dentry = new_dentry,
> rd.flags = flags,
> rd.delegated_inode = NULL,
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 16f40a6f8264..9a83904c9d4a 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2016,10 +2016,10 @@ int vfs_unlink(struct mnt_idmap *, struct inode *, struct dentry *,
> */
> struct renamedata {
> struct mnt_idmap *old_mnt_idmap;
> - struct inode *old_dir;
> + struct dentry *old_dir;
> struct dentry *old_dentry;
> struct mnt_idmap *new_mnt_idmap;
> - struct inode *new_dir;
> + struct dentry *new_dir;
> struct dentry *new_dentry;
> struct inode **delegated_inode;
> unsigned int flags;
For the NFSD hunks:
Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
--
Chuck Lever
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/2] VFS: change old_dir and new_dir in struct renamedata to dentrys
2025-06-11 22:57 ` [PATCH 1/2] VFS: change old_dir and new_dir in struct renamedata to dentrys NeilBrown
` (2 preceding siblings ...)
2025-06-12 13:03 ` Chuck Lever
@ 2025-06-12 22:52 ` Namjae Jeon
3 siblings, 0 replies; 12+ messages in thread
From: Namjae Jeon @ 2025-06-12 22:52 UTC (permalink / raw)
To: NeilBrown
Cc: Alexander Viro, Christian Brauner, Jan Kara, David Howells,
Tyler Hicks, Chuck Lever, Jeff Layton, Miklos Szeredi,
Amir Goldstein, Kees Cook, Joel Granados, Steve French,
Sergey Senozhatsky, netfs, linux-kernel, ecryptfs, linux-fsdevel,
linux-nfs, linux-unionfs, linux-cifs
On Thu, Jun 12, 2025 at 7:59 AM NeilBrown <neil@brown.name> wrote:
>
> all users of 'struct renamedata' have the dentry for the old and new
> directories, and often have no use for the inode except to store it in
> the renamedata.
>
> This patch changes struct renamedata to hold the dentry, rather than
> the inode, for the old and new directories, and changes callers to
> match.
>
> This results in the removal of several local variables and several
> dereferences of ->d_inode at the cost of adding ->d_inode dereferences
> to vfs_rename().
>
> Signed-off-by: NeilBrown <neil@brown.name>
For ksmbd part,
Reviewed-by: Namjae Jeon <linkinjeon@kernel.org>
Thanks!
^ permalink raw reply [flat|nested] 12+ messages in thread