Re: [PATCH RFC v2 04/28] vfs: allow mkdir to wait for delegation break on parent

[Jeff Layton <jlayton@kernel.org>]

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@kernel.org>
> 
> 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@kernel.org>