Re: [PATCH 6/6] VFS: Change vfs_mkdir() to return the dentry.

[Chuck Lever <chuck.lever@oracle.com>]

On 2/23/25 9:51 PM, NeilBrown wrote:
> On Sat, 22 Feb 2025, Chuck Lever wrote:
>> On 2/20/25 6:36 PM, NeilBrown wrote:
> ...
>>> +		dchild = vfs_mkdir(&nop_mnt_idmap, dirp, dchild, iap->ia_mode);
>>> +		if (IS_ERR(dchild)) {
>>> +			host_err = PTR_ERR(dchild);
>>> +		} else if (d_is_negative(dchild)) {
>>> +			err = nfserr_serverfault;
>>> +			goto out;
>>> +		} else if (unlikely(dchild != resfhp->fh_dentry)) {
>>>  			dput(resfhp->fh_dentry);
>>> -			resfhp->fh_dentry = dget(d);
>>> -			err = fh_update(resfhp);
>>
>> Hi Neil, why is this fh_update() call no longer necessary?
>>
> 
> I tried to explain that in the commit message:
> 
>                                         I removed the fh_update()
>       call as that is not needed and out-of-place.  A subsequent
>       nfsd_create_setattr() call will call fh_update() when needed.
> 
> I don't think the fh_update() was needed even when first added in 
> Commit 3819bb0d79f5 ("nfsd: vfs_mkdir() might succeed leaving dentry negative unhashed")
> 
> as there was already an fh_update() call later in the function.

Thanks for the patch description verbiage, and sorry I missed it.

Even so, IMHO this belongs in a separate patch instead of buried in this
unrelated API change. This doesn't fix a bug nor is it necessary for
changing the return value of vfs_mkdir() AFAICT. At the very least, a
separate patch makes it possible to include a sensible reference to
3819bb0d79f5, which is helpful.

IME these tiny weird looking warts often have a purpose that is revealed
only once the code is made to look reasonable.

Make the fh_update() removal a pre-requisite clean-up to this patch,
maybe?


> Thanks,
> NeilBrown
> 
> 
> 
>>
>>> -			dput(dchild);
>>> -			dchild = d;
>>> -			if (err)
>>> -				goto out;
>>> +			resfhp->fh_dentry = dget(dchild);
>>>  		}
>>>  		break;
>>>  	case S_IFCHR:
>>> @@ -1530,7 +1517,8 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>  	err = nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
>>>  
>>>  out:
>>> -	dput(dchild);
>>> +	if (!IS_ERR(dchild))
>>> +		dput(dchild);
>>>  	return err;
>>>  
>>>  out_nfserr:
>>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>>> index 21c3aaf7b274..fe493f3ed6b6 100644
>>> --- a/fs/overlayfs/dir.c
>>> +++ b/fs/overlayfs/dir.c
>>> @@ -138,37 +138,6 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
>>>  	goto out;
>>>  }
>>>  
>>> -int ovl_mkdir_real(struct ovl_fs *ofs, struct inode *dir,
>>> -		   struct dentry **newdentry, umode_t mode)
>>> -{
>>> -	int err;
>>> -	struct dentry *d, *dentry = *newdentry;
>>> -
>>> -	err = ovl_do_mkdir(ofs, dir, dentry, mode);
>>> -	if (err)
>>> -		return err;
>>> -
>>> -	if (likely(!d_unhashed(dentry)))
>>> -		return 0;
>>> -
>>> -	/*
>>> -	 * vfs_mkdir() may succeed and leave the dentry passed
>>> -	 * to it unhashed and negative. If that happens, try to
>>> -	 * lookup a new hashed and positive dentry.
>>> -	 */
>>> -	d = ovl_lookup_upper(ofs, dentry->d_name.name, dentry->d_parent,
>>> -			     dentry->d_name.len);
>>> -	if (IS_ERR(d)) {
>>> -		pr_warn("failed lookup after mkdir (%pd2, err=%i).\n",
>>> -			dentry, err);
>>> -		return PTR_ERR(d);
>>> -	}
>>> -	dput(dentry);
>>> -	*newdentry = d;
>>> -
>>> -	return 0;
>>> -}
>>> -
>>>  struct dentry *ovl_create_real(struct ovl_fs *ofs, struct inode *dir,
>>>  			       struct dentry *newdentry, struct ovl_cattr *attr)
>>>  {
>>> @@ -191,7 +160,8 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct inode *dir,
>>>  
>>>  		case S_IFDIR:
>>>  			/* mkdir is special... */
>>> -			err =  ovl_mkdir_real(ofs, dir, &newdentry, attr->mode);
>>> +			newdentry =  ovl_do_mkdir(ofs, dir, newdentry, attr->mode);
>>> +			err = PTR_ERR_OR_ZERO(newdentry);
>>>  			break;
>>>  
>>>  		case S_IFCHR:
>>> @@ -219,7 +189,8 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct inode *dir,
>>>  	}
>>>  out:
>>>  	if (err) {
>>> -		dput(newdentry);
>>> +		if (!IS_ERR(newdentry))
>>> +			dput(newdentry);
>>>  		return ERR_PTR(err);
>>>  	}
>>>  	return newdentry;
>>> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>>> index 0021e2025020..6f2f8f4cfbbc 100644
>>> --- a/fs/overlayfs/overlayfs.h
>>> +++ b/fs/overlayfs/overlayfs.h
>>> @@ -241,13 +241,14 @@ static inline int ovl_do_create(struct ovl_fs *ofs,
>>>  	return err;
>>>  }
>>>  
>>> -static inline int ovl_do_mkdir(struct ovl_fs *ofs,
>>> -			       struct inode *dir, struct dentry *dentry,
>>> -			       umode_t mode)
>>> +static inline struct dentry *ovl_do_mkdir(struct ovl_fs *ofs,
>>> +					  struct inode *dir,
>>> +					  struct dentry *dentry,
>>> +					  umode_t mode)
>>>  {
>>> -	int err = vfs_mkdir(ovl_upper_mnt_idmap(ofs), dir, dentry, mode);
>>> -	pr_debug("mkdir(%pd2, 0%o) = %i\n", dentry, mode, err);
>>> -	return err;
>>> +	dentry = vfs_mkdir(ovl_upper_mnt_idmap(ofs), dir, dentry, mode);
>>> +	pr_debug("mkdir(%pd2, 0%o) = %i\n", dentry, mode, PTR_ERR_OR_ZERO(dentry));
>>> +	return dentry;
>>>  }
>>>  
>>>  static inline int ovl_do_mknod(struct ovl_fs *ofs,
>>> @@ -838,8 +839,6 @@ struct ovl_cattr {
>>>  
>>>  #define OVL_CATTR(m) (&(struct ovl_cattr) { .mode = (m) })
>>>  
>>> -int ovl_mkdir_real(struct ovl_fs *ofs, struct inode *dir,
>>> -		   struct dentry **newdentry, umode_t mode);
>>>  struct dentry *ovl_create_real(struct ovl_fs *ofs,
>>>  			       struct inode *dir, struct dentry *newdentry,
>>>  			       struct ovl_cattr *attr);
>>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>>> index 61e21c3129e8..b63474d1b064 100644
>>> --- a/fs/overlayfs/super.c
>>> +++ b/fs/overlayfs/super.c
>>> @@ -327,9 +327,10 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
>>>  			goto retry;
>>>  		}
>>>  
>>> -		err = ovl_mkdir_real(ofs, dir, &work, attr.ia_mode);
>>> -		if (err)
>>> -			goto out_dput;
>>> +		work = ovl_do_mkdir(ofs, dir, work, attr.ia_mode);
>>> +		err = PTR_ERR(work);
>>> +		if (IS_ERR(work))
>>> +			goto out_err;
>>>  
>>>  		/* Weird filesystem returning with hashed negative (kernfs)? */
>>>  		err = -EINVAL;
>>> diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
>>> index fe29acef5872..8554aa5a1059 100644
>>> --- a/fs/smb/server/vfs.c
>>> +++ b/fs/smb/server/vfs.c
>>> @@ -206,8 +206,8 @@ int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char *name, umode_t mode)
>>>  {
>>>  	struct mnt_idmap *idmap;
>>>  	struct path path;
>>> -	struct dentry *dentry;
>>> -	int err;
>>> +	struct dentry *dentry, *d;
>>> +	int err = 0;
>>>  
>>>  	dentry = ksmbd_vfs_kern_path_create(work, name,
>>>  					    LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
>>> @@ -222,27 +222,15 @@ int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char *name, umode_t mode)
>>>  
>>>  	idmap = mnt_idmap(path.mnt);
>>>  	mode |= S_IFDIR;
>>> -	err = vfs_mkdir(idmap, d_inode(path.dentry), dentry, mode);
>>> -	if (!err && d_unhashed(dentry)) {
>>> -		struct dentry *d;
>>> -
>>> -		d = lookup_one(idmap, dentry->d_name.name, dentry->d_parent,
>>> -			       dentry->d_name.len);
>>> -		if (IS_ERR(d)) {
>>> -			err = PTR_ERR(d);
>>> -			goto out_err;
>>> -		}
>>> -		if (unlikely(d_is_negative(d))) {
>>> -			dput(d);
>>> -			err = -ENOENT;
>>> -			goto out_err;
>>> -		}
>>> -
>>> -		ksmbd_vfs_inherit_owner(work, d_inode(path.dentry), d_inode(d));
>>> -		dput(d);
>>> -	}
>>> +	d = dentry;
>>> +	dentry = vfs_mkdir(idmap, d_inode(path.dentry), dentry, mode);
>>> +	if (IS_ERR(dentry))
>>> +		err = PTR_ERR(dentry);
>>> +	else if (d_is_negative(dentry))
>>> +		err = -ENOENT;
>>> +	if (!err && dentry != d)
>>> +		ksmbd_vfs_inherit_owner(work, d_inode(path.dentry), d_inode(dentry));
>>>  
>>> -out_err:
>>>  	done_path_create(&path, dentry);
>>>  	if (err)
>>>  		pr_err("mkdir(%s): creation failed (err:%d)\n", name, err);
>>> diff --git a/fs/xfs/scrub/orphanage.c b/fs/xfs/scrub/orphanage.c
>>> index c287c755f2c5..3537f3cca6d5 100644
>>> --- a/fs/xfs/scrub/orphanage.c
>>> +++ b/fs/xfs/scrub/orphanage.c
>>> @@ -167,10 +167,11 @@ xrep_orphanage_create(
>>>  	 * directory to control access to a file we put in here.
>>>  	 */
>>>  	if (d_really_is_negative(orphanage_dentry)) {
>>> -		error = vfs_mkdir(&nop_mnt_idmap, root_inode, orphanage_dentry,
>>> -				0750);
>>> -		if (error)
>>> -			goto out_dput_orphanage;
>>> +		orphanage_dentry = vfs_mkdir(&nop_mnt_idmap, root_inode,
>>> +					     orphanage_dentry, 0750);
>>> +		error = PTR_ERR(orphanage_dentry);
>>> +		if (IS_ERR(orphanage_dentry))
>>> +			goto out_unlock_root;
>>>  	}
>>>  
>>>  	/* Not a directory? Bail out. */
>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>> index 8f4fbecd40fc..eaad8e31c0d4 100644
>>> --- a/include/linux/fs.h
>>> +++ b/include/linux/fs.h
>>> @@ -1971,8 +1971,8 @@ bool inode_owner_or_capable(struct mnt_idmap *idmap,
>>>   */
>>>  int vfs_create(struct mnt_idmap *, struct inode *,
>>>  	       struct dentry *, umode_t, bool);
>>> -int vfs_mkdir(struct mnt_idmap *, struct inode *,
>>> -	      struct dentry *, umode_t);
>>> +struct dentry *vfs_mkdir(struct mnt_idmap *, struct inode *,
>>> +			 struct dentry *, umode_t);
>>>  int vfs_mknod(struct mnt_idmap *, struct inode *, struct dentry *,
>>>                umode_t, dev_t);
>>>  int vfs_symlink(struct mnt_idmap *, struct inode *,
>>
>>
>> -- 
>> Chuck Lever
>>
> 


-- 
Chuck Lever