On Thu, 2025-06-05 at 13:19 +0200, Jan Kara wrote:
> On Mon 02-06-25 10:01:47, Jeff Layton wrote:
> > In order to add directory delegation support, we need to break
> > delegations on the parent whenever there is going to be a change in the
> > directory.
> >
> > Rename the existing vfs_mkdir to __vfs_mkdir, make it static and add a
> > new delegated_inode parameter. Add a new exported vfs_mkdir wrapper
> > around it that passes a NULL pointer for delegated_inode.
> >
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>
> FWIW I went through the changes adding breaking of delegations to VFS
> directory functions and they look ok to me. Just I dislike the addition of
> __vfs_mkdir() (and similar) helpers because over longer term the helpers
> tend to pile up and the maze of functions (already hard to follow in VFS)
> gets unwieldy. Either I'd try to give it a proper name or (if exposing the
> functionality to the external world is fine - which seems it is) you could
> just add the argument to vfs_mkdir() and change all the callers? I've
> checked and for each of the modified functions there's less than 10 callers
> so the churn shouldn't be that big. What do others think?
>
Good point -- I'm always terrible with naming functions. I'm fine with
either approach, but just adding the argument does sound simple enough.
I'll plan to do that unless anyone objects.
Thanks for taking a look!
> Honza
>
> > ---
> > fs/namei.c | 67 +++++++++++++++++++++++++++++++++++++++-----------------------
> > 1 file changed, 42 insertions(+), 25 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 0fea12860036162c01a291558e068fde9c986142..7c9e237ed1b1a535934ffe5e523424bb035e7ae0 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -4318,29 +4318,9 @@ SYSCALL_DEFINE3(mknod, const char __user *, filename, umode_t, mode, unsigned, d
> > return do_mknodat(AT_FDCWD, getname(filename), mode, dev);
> > }
> >
> > -/**
> > - * vfs_mkdir - create directory returning correct dentry if possible
> > - * @idmap: idmap of the mount the inode was found from
> > - * @dir: inode of the parent directory
> > - * @dentry: dentry of the child directory
> > - * @mode: mode of the child directory
> > - *
> > - * Create a directory.
> > - *
> > - * If the inode has been found through an idmapped mount the idmap of
> > - * the vfsmount must be passed through @idmap. This function will then take
> > - * care to map the inode according to @idmap before checking permissions.
> > - * On non-idmapped mounts or if permission checking is to be performed on the
> > - * raw inode simply pass @nop_mnt_idmap.
> > - *
> > - * In the event that the filesystem does not use the *@dentry but leaves it
> > - * negative or unhashes it and possibly splices a different one returning it,
> > - * the original dentry is dput() and the alternate is returned.
> > - *
> > - * In case of an error the dentry is dput() and an ERR_PTR() is returned.
> > - */
> > -struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> > - struct dentry *dentry, umode_t mode)
> > +static struct dentry *__vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> > + struct dentry *dentry, umode_t mode,
> > + struct inode **delegated_inode)
> > {
> > int error;
> > unsigned max_links = dir->i_sb->s_max_links;
> > @@ -4363,6 +4343,10 @@ struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> > if (max_links && dir->i_nlink >= max_links)
> > goto err;
> >
> > + error = try_break_deleg(dir, delegated_inode);
> > + if (error)
> > + goto err;
> > +
> > de = dir->i_op->mkdir(idmap, dir, dentry, mode);
> > error = PTR_ERR(de);
> > if (IS_ERR(de))
> > @@ -4378,6 +4362,33 @@ struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> > dput(dentry);
> > return ERR_PTR(error);
> > }
> > +
> > +/**
> > + * vfs_mkdir - create directory returning correct dentry if possible
> > + * @idmap: idmap of the mount the inode was found from
> > + * @dir: inode of the parent directory
> > + * @dentry: dentry of the child directory
> > + * @mode: mode of the child directory
> > + *
> > + * Create a directory.
> > + *
> > + * If the inode has been found through an idmapped mount the idmap of
> > + * the vfsmount must be passed through @idmap. This function will then take
> > + * care to map the inode according to @idmap before checking permissions.
> > + * On non-idmapped mounts or if permission checking is to be performed on the
> > + * raw inode simply pass @nop_mnt_idmap.
> > + *
> > + * In the event that the filesystem does not use the *@dentry but leaves it
> > + * negative or unhashes it and possibly splices a different one returning it,
> > + * the original dentry is dput() and the alternate is returned.
> > + *
> > + * In case of an error the dentry is dput() and an ERR_PTR() is returned.
> > + */
> > +struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> > + struct dentry *dentry, umode_t mode)
> > +{
> > + return __vfs_mkdir(idmap, dir, dentry, mode, NULL);
> > +}
> > EXPORT_SYMBOL(vfs_mkdir);
> >
> > int do_mkdirat(int dfd, struct filename *name, umode_t mode)
> > @@ -4386,6 +4397,7 @@ int do_mkdirat(int dfd, struct filename *name, umode_t mode)
> > struct path path;
> > int error;
> > unsigned int lookup_flags = LOOKUP_DIRECTORY;
> > + struct inode *delegated_inode = NULL;
> >
> > retry:
> > dentry = filename_create(dfd, name, &path, lookup_flags);
> > @@ -4396,12 +4408,17 @@ int do_mkdirat(int dfd, struct filename *name, umode_t mode)
> > error = security_path_mkdir(&path, dentry,
> > mode_strip_umask(path.dentry->d_inode, mode));
> > if (!error) {
> > - dentry = vfs_mkdir(mnt_idmap(path.mnt), path.dentry->d_inode,
> > - dentry, mode);
> > + dentry = __vfs_mkdir(mnt_idmap(path.mnt), path.dentry->d_inode,
> > + dentry, mode, &delegated_inode);
> > if (IS_ERR(dentry))
> > error = PTR_ERR(dentry);
> > }
> > done_path_create(&path, dentry);
> > + if (delegated_inode) {
> > + error = break_deleg_wait(&delegated_inode);
> > + if (!error)
> > + goto retry;
> > + }
> > if (retry_estale(error, lookup_flags)) {
> > lookup_flags |= LOOKUP_REVAL;
> > goto retry;
> >
> > --
> > 2.49.0
> >
--
Jeff Layton <jlayton@xxxxxxxxxx>
