On Tue, Jul 10, 2018 at 5:14 PM Luis Henriques <lhenriques@xxxxxxxx> wrote:
>
> "Yan, Zheng" <ukernel@xxxxxxxxx> writes:
>
> > On Tue, Jul 10, 2018 at 5:20 AM Luis Henriques <lhenriques@xxxxxxxx> wrote:
> >>
> >> Chengguang Xu <cgxu519@xxxxxxx> writes:
> >>
> >> > When file num exceeds quota limit or fails from ceph_per_init_acls()
> >> > should call d_drop to drop dentry from cache as well.
> >> >
> >> > Signed-off-by: Chengguang Xu <cgxu519@xxxxxxx>
> >> > ---
> >> > Hello,
> >> >
> >> > Sorry, I haven't got resource to fully test this patch, so please review.
> >>
> >> I think this patch isn't correct. In fact, function ceph_mknod seems to
> >> be already buggy as it should only call d_drop if dget is also executed
> >> (which isn't true if ceph_mdsc_create_request or kstrdup fails).
> >>
> > why? I think caller (vfs) of ceph_mknod already holds a reference.
>
> Hmm.. Ok, I haven't look too deep but, if that's true, shouldn't the VFS
> layer itself be doing the d_drop when ->mknod() returns an error?
> ext4_mknod, for example, doesn't do the d_drop.
>
I think only network filesystems do this. The negative dentry has no
harm for local filesystem.
Regards
Yan, Zheng
> (Also, in my previous reply I mistakenly referred to a kstrdup failure,
> which doesn't exist in ceph_mknod().)
>
> Cheers,
> --
> Luis
>
>
> >
> >
> >> Patch #2 has the same issue, but ceph_symlink does look correct
> >> regarding dget/d_drop.
> >>
> >> Cheers,
> >> --
> >> Luis
> >>
> >>
> >> >
> >> > fs/ceph/dir.c | 18 +++++++++++-------
> >> > 1 file changed, 11 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> >> > index 036ac0f3a393..813c01e8ad05 100644
> >> > --- a/fs/ceph/dir.c
> >> > +++ b/fs/ceph/dir.c
> >> > @@ -827,19 +827,21 @@ static int ceph_mknod(struct inode *dir, struct dentry *dentry,
> >> > if (ceph_snap(dir) != CEPH_NOSNAP)
> >> > return -EROFS;
> >> >
> >> > - if (ceph_quota_is_max_files_exceeded(dir))
> >> > - return -EDQUOT;
> >> > + if (ceph_quota_is_max_files_exceeded(dir)) {
> >> > + err = -EDQUOT;
> >> > + goto out;
> >> > + }
> >> >
> >> > err = ceph_pre_init_acls(dir, &mode, &acls);
> >> > if (err < 0)
> >> > - return err;
> >> > + goto out;
> >> >
> >> > dout("mknod in dir %p dentry %p mode 0%ho rdev %d\n",
> >> > dir, dentry, mode, rdev);
> >> > req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_MKNOD, USE_AUTH_MDS);
> >> > if (IS_ERR(req)) {
> >> > err = PTR_ERR(req);
> >> > - goto out;
> >> > + goto out2;
> >> > }
> >> > req->r_dentry = dget(dentry);
> >> > req->r_num_caps = 2;
> >> > @@ -857,12 +859,14 @@ static int ceph_mknod(struct inode *dir, struct dentry *dentry,
> >> > if (!err && !req->r_reply_info.head->is_dentry)
> >> > err = ceph_handle_notrace_create(dir, dentry);
> >> > ceph_mdsc_put_request(req);
> >> > -out:
> >> > +
> >> > if (!err)
> >> > ceph_init_inode_acls(d_inode(dentry), &acls);
> >> > - else
> >> > - d_drop(dentry);
> >> > +out2:
> >> > ceph_release_acls_info(&acls);
> >> > +out:
> >> > + if (err)
> >> > + d_drop(dentry);
> >> > return err;
> >> > }
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> >> the body of a message to majordomo@xxxxxxxxxxxxxxx
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
