"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. (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