| Message ID | 20180709141731.18136-1-cgxu519@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chengguang Xu <cgxu519@gmx.com> 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@gmx.com> > --- > 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). Patch #2 has the same issue, but ceph_symlink does look correct regarding dget/d_drop. Cheers,
On Tue, Jul 10, 2018 at 5:20 AM Luis Henriques <lhenriques@suse.com> wrote: > > Chengguang Xu <cgxu519@gmx.com> 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@gmx.com> > > --- > > 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. > 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@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
"Yan, Zheng" <ukernel@gmail.com> writes: > On Tue, Jul 10, 2018 at 5:20 AM Luis Henriques <lhenriques@suse.com> wrote: >> >> Chengguang Xu <cgxu519@gmx.com> 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@gmx.com> >> > --- >> > 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,
On Tue, Jul 10, 2018 at 5:14 PM Luis Henriques <lhenriques@suse.com> wrote: > > "Yan, Zheng" <ukernel@gmail.com> writes: > > > On Tue, Jul 10, 2018 at 5:20 AM Luis Henriques <lhenriques@suse.com> wrote: > >> > >> Chengguang Xu <cgxu519@gmx.com> 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@gmx.com> > >> > --- > >> > 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@vger.kernel.org > >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 9, 2018 at 10:20 PM Chengguang Xu <cgxu519@gmx.com> wrote: > > 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@gmx.com> > --- > Hello, > > Sorry, I haven't got resource to fully test this patch, so please review. > > 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; > } > > -- > 2.17.1 > Both patches are applied, Thanks. Yan, Zheng > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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@gmx.com> --- Hello, Sorry, I haven't got resource to fully test this patch, so please review. fs/ceph/dir.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)