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
