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,
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
>
