| Message ID | 20250220234630.983190-4-neilb@suse.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Change ->mkdir() and vfs_mkdir() to return a dentry. | expand |
On Fri, 2025-02-21 at 10:36 +1100, NeilBrown wrote: > ceph already splices the correct dentry (in splice_dentry()) from the > result of mkdir but does nothing more with it. > > Now that ->mkdir can return a dentry, return the correct dentry. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/ceph/dir.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > index 39e0f240de06..c1a1c168bb27 100644 > --- a/fs/ceph/dir.c > +++ b/fs/ceph/dir.c > @@ -1099,6 +1099,7 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > struct ceph_client *cl = mdsc->fsc->client; > struct ceph_mds_request *req; > struct ceph_acl_sec_ctx as_ctx = {}; > + struct dentry *ret = NULL; I believe that it makes sense to initialize pointer by error here and always return ret as output. If something goes wrong in the logic, then we already have error. > int err; > int op; > > @@ -1166,14 +1167,20 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > !req->r_reply_info.head->is_dentry) > err = ceph_handle_notrace_create(dir, dentry); > out_req: > + if (!err && req->r_dentry != dentry) > + /* Some other dentry was spliced in */ > + ret = dget(req->r_dentry); > ceph_mdsc_put_request(req); > out: > if (!err) > + /* Should this use 'ret' ?? */ Could we make a decision should or shouldn't? :) It looks not good to leave this comment instead of proper implementation. Do we have some obstacles to make this decision? > ceph_init_inode_acls(d_inode(dentry), &as_ctx); > else > d_drop(dentry); > ceph_release_acl_sec_ctx(&as_ctx); > - return ERR_PTR(err); > + if (err) > + return ERR_PTR(err); > + return ret; What's about this? return err ? ERR_PTR(err) : ret; Thanks, Slava. > } > > static int ceph_link(struct dentry *old_dentry, struct inode *dir,
On Fri, 2025-02-21 at 10:36 +1100, NeilBrown wrote: > ceph already splices the correct dentry (in splice_dentry()) from the > result of mkdir but does nothing more with it. > > Now that ->mkdir can return a dentry, return the correct dentry. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/ceph/dir.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > index 39e0f240de06..c1a1c168bb27 100644 > --- a/fs/ceph/dir.c > +++ b/fs/ceph/dir.c > @@ -1099,6 +1099,7 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > struct ceph_client *cl = mdsc->fsc->client; > struct ceph_mds_request *req; > struct ceph_acl_sec_ctx as_ctx = {}; > + struct dentry *ret = NULL; > int err; > int op; > > @@ -1166,14 +1167,20 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > !req->r_reply_info.head->is_dentry) > err = ceph_handle_notrace_create(dir, dentry); > out_req: > + if (!err && req->r_dentry != dentry) > + /* Some other dentry was spliced in */ > + ret = dget(req->r_dentry); > ceph_mdsc_put_request(req); > out: > if (!err) > + /* Should this use 'ret' ?? */ Probably? Is there a guarantee that "dentry" will even have an inode attached if it got replaced by an disconnected one in the dcache? > ceph_init_inode_acls(d_inode(dentry), &as_ctx); > else > d_drop(dentry); > ceph_release_acl_sec_ctx(&as_ctx); > - return ERR_PTR(err); > + if (err) > + return ERR_PTR(err); > + return ret; > } > > static int ceph_link(struct dentry *old_dentry, struct inode *dir,
On Fri, 21 Feb 2025, Viacheslav Dubeyko wrote: > On Fri, 2025-02-21 at 10:36 +1100, NeilBrown wrote: > > ceph already splices the correct dentry (in splice_dentry()) from the > > result of mkdir but does nothing more with it. > > > > Now that ->mkdir can return a dentry, return the correct dentry. > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > fs/ceph/dir.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > > index 39e0f240de06..c1a1c168bb27 100644 > > --- a/fs/ceph/dir.c > > +++ b/fs/ceph/dir.c > > @@ -1099,6 +1099,7 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > struct ceph_client *cl = mdsc->fsc->client; > > struct ceph_mds_request *req; > > struct ceph_acl_sec_ctx as_ctx = {}; > > + struct dentry *ret = NULL; > > I believe that it makes sense to initialize pointer by error here and always > return ret as output. If something goes wrong in the logic, then we already have > error. I'm not certain that I understand, but I have made a change which seems to be consistent with the above and included it below. Please let me know if it is what you intended. > > > int err; > > int op; > > > > @@ -1166,14 +1167,20 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > !req->r_reply_info.head->is_dentry) > > err = ceph_handle_notrace_create(dir, dentry); > > out_req: > > + if (!err && req->r_dentry != dentry) > > + /* Some other dentry was spliced in */ > > + ret = dget(req->r_dentry); > > ceph_mdsc_put_request(req); > > out: > > if (!err) > > + /* Should this use 'ret' ?? */ > > Could we make a decision should or shouldn't? :) > It looks not good to leave this comment instead of proper implementation. Do we > have some obstacles to make this decision? I suspect we should use ret, but I didn't want to make a change which wasn't directly required by my needed. So I highlighted this which looks to me like a possible bug, hoping that someone more familiar with the code would give an opinion. Do you agree that 'ret' (i.e. ->r_dentry) should be used when ret is not NULL? > > > ceph_init_inode_acls(d_inode(dentry), &as_ctx); > > else > > d_drop(dentry); > > ceph_release_acl_sec_ctx(&as_ctx); > > - return ERR_PTR(err); > > + if (err) > > + return ERR_PTR(err); > > + return ret; > > What's about this? > > return err ? ERR_PTR(err) : ret; We could do that, but you said above that you thought we should always return 'ret' - which does make some sense. What do you think of the following alternate patch? Thanks, NeilBrown diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index 39e0f240de06..d2e5c557df83 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -1099,6 +1099,7 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, struct ceph_client *cl = mdsc->fsc->client; struct ceph_mds_request *req; struct ceph_acl_sec_ctx as_ctx = {}; + struct dentry *ret; int err; int op; @@ -1116,32 +1117,32 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, ceph_vinop(dir), dentry, dentry, mode); op = CEPH_MDS_OP_MKDIR; } else { - err = -EROFS; + ret = ERR_PTR(-EROFS); goto out; } if (op == CEPH_MDS_OP_MKDIR && ceph_quota_is_max_files_exceeded(dir)) { - err = -EDQUOT; + ret = ERR_PTR(-EDQUOT); goto out; } if ((op == CEPH_MDS_OP_MKSNAP) && IS_ENCRYPTED(dir) && !fscrypt_has_encryption_key(dir)) { - err = -ENOKEY; + ret = ERR_PTR(-ENOKEY); goto out; } req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS); if (IS_ERR(req)) { - err = PTR_ERR(req); + ret = ERR_CAST(req); goto out; } mode |= S_IFDIR; req->r_new_inode = ceph_new_inode(dir, dentry, &mode, &as_ctx); if (IS_ERR(req->r_new_inode)) { - err = PTR_ERR(req->r_new_inode); + ret = ERR_CAST(req->r_new_inode); req->r_new_inode = NULL; goto out_req; } @@ -1165,15 +1166,23 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, !req->r_reply_info.head->is_target && !req->r_reply_info.head->is_dentry) err = ceph_handle_notrace_create(dir, dentry); + ret = ERR_PTR(err); out_req: + if (!IS_ERR(ret) && req->r_dentry != dentry) + /* Some other dentry was spliced in */ + ret = dget(req->r_dentry); ceph_mdsc_put_request(req); out: - if (!err) - ceph_init_inode_acls(d_inode(dentry), &as_ctx); - else + if (!IS_ERR(ret)) { + if (ret) + ceph_init_inode_acls(d_inode(ret), &as_ctx); + else + ceph_init_inode_acls(d_inode(dentry), &as_ctx); + } else { d_drop(dentry); + } ceph_release_acl_sec_ctx(&as_ctx); - return ERR_PTR(err); + return ret; } static int ceph_link(struct dentry *old_dentry, struct inode *dir,
On Mon, 2025-02-24 at 13:15 +1100, NeilBrown wrote: > On Fri, 21 Feb 2025, Viacheslav Dubeyko wrote: > > On Fri, 2025-02-21 at 10:36 +1100, NeilBrown wrote: > > > ceph already splices the correct dentry (in splice_dentry()) from the > > > result of mkdir but does nothing more with it. > > > > > > Now that ->mkdir can return a dentry, return the correct dentry. > > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > --- > > > fs/ceph/dir.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > > > index 39e0f240de06..c1a1c168bb27 100644 > > > --- a/fs/ceph/dir.c > > > +++ b/fs/ceph/dir.c > > > @@ -1099,6 +1099,7 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > > struct ceph_client *cl = mdsc->fsc->client; > > > struct ceph_mds_request *req; > > > struct ceph_acl_sec_ctx as_ctx = {}; > > > + struct dentry *ret = NULL; > > > > I believe that it makes sense to initialize pointer by error here and always > > return ret as output. If something goes wrong in the logic, then we already have > > error. > > I'm not certain that I understand, but I have made a change which seems > to be consistent with the above and included it below. Please let me > know if it is what you intended. > > > > > > int err; > > > int op; > > > > > > @@ -1166,14 +1167,20 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > > !req->r_reply_info.head->is_dentry) > > > err = ceph_handle_notrace_create(dir, dentry); > > > out_req: > > > + if (!err && req->r_dentry != dentry) > > > + /* Some other dentry was spliced in */ > > > + ret = dget(req->r_dentry); > > > ceph_mdsc_put_request(req); > > > out: > > > if (!err) > > > + /* Should this use 'ret' ?? */ > > > > Could we make a decision should or shouldn't? :) > > It looks not good to leave this comment instead of proper implementation. Do we > > have some obstacles to make this decision? > > I suspect we should use ret, but I didn't want to make a change which > wasn't directly required by my needed. So I highlighted this which > looks to me like a possible bug, hoping that someone more familiar with > the code would give an opinion. Do you agree that 'ret' (i.e. > ->r_dentry) should be used when ret is not NULL? > I think if we are going to return ret as a dentry, then it makes sense to call the ceph_init_inode_acls() for d_inode(ret). I don't see the point to call ceph_init_inode_acls() for d_inode(dentry) then. > > > > > ceph_init_inode_acls(d_inode(dentry), &as_ctx); > > > else > > > d_drop(dentry); > > > ceph_release_acl_sec_ctx(&as_ctx); > > > - return ERR_PTR(err); > > > + if (err) > > > + return ERR_PTR(err); > > > + return ret; > > > > What's about this? > > > > return err ? ERR_PTR(err) : ret; > > We could do that, but you said above that you thought we should always > return 'ret' - which does make some sense. > > What do you think of the following alternate patch? > Patch looks good to me. Thanks. Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com> > Thanks, > NeilBrown > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > index 39e0f240de06..d2e5c557df83 100644 > --- a/fs/ceph/dir.c > +++ b/fs/ceph/dir.c > @@ -1099,6 +1099,7 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > struct ceph_client *cl = mdsc->fsc->client; > struct ceph_mds_request *req; > struct ceph_acl_sec_ctx as_ctx = {}; > + struct dentry *ret; > int err; > int op; > > @@ -1116,32 +1117,32 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > ceph_vinop(dir), dentry, dentry, mode); > op = CEPH_MDS_OP_MKDIR; > } else { > - err = -EROFS; > + ret = ERR_PTR(-EROFS); > goto out; > } > > if (op == CEPH_MDS_OP_MKDIR && > ceph_quota_is_max_files_exceeded(dir)) { > - err = -EDQUOT; > + ret = ERR_PTR(-EDQUOT); > goto out; > } > if ((op == CEPH_MDS_OP_MKSNAP) && IS_ENCRYPTED(dir) && > !fscrypt_has_encryption_key(dir)) { > - err = -ENOKEY; > + ret = ERR_PTR(-ENOKEY); > goto out; > } > > > req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS); > if (IS_ERR(req)) { > - err = PTR_ERR(req); > + ret = ERR_CAST(req); > goto out; > } > > mode |= S_IFDIR; > req->r_new_inode = ceph_new_inode(dir, dentry, &mode, &as_ctx); > if (IS_ERR(req->r_new_inode)) { > - err = PTR_ERR(req->r_new_inode); > + ret = ERR_CAST(req->r_new_inode); > req->r_new_inode = NULL; > goto out_req; > } > @@ -1165,15 +1166,23 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > !req->r_reply_info.head->is_target && > !req->r_reply_info.head->is_dentry) > err = ceph_handle_notrace_create(dir, dentry); > + ret = ERR_PTR(err); > out_req: > + if (!IS_ERR(ret) && req->r_dentry != dentry) > + /* Some other dentry was spliced in */ > + ret = dget(req->r_dentry); > ceph_mdsc_put_request(req); > out: > - if (!err) > - ceph_init_inode_acls(d_inode(dentry), &as_ctx); > - else > + if (!IS_ERR(ret)) { > + if (ret) > + ceph_init_inode_acls(d_inode(ret), &as_ctx); > + else > + ceph_init_inode_acls(d_inode(dentry), &as_ctx); > + } else { > d_drop(dentry); > + } > ceph_release_acl_sec_ctx(&as_ctx); > - return ERR_PTR(err); > + return ret; > } > > static int ceph_link(struct dentry *old_dentry, struct inode *dir, > Thanks, Slava.
On Mon, 2025-02-24 at 22:09 +0000, Viacheslav Dubeyko wrote: > On Mon, 2025-02-24 at 13:15 +1100, NeilBrown wrote: > > On Fri, 21 Feb 2025, Viacheslav Dubeyko wrote: > > > On Fri, 2025-02-21 at 10:36 +1100, NeilBrown wrote: > > > > ceph already splices the correct dentry (in splice_dentry()) from the > > > > result of mkdir but does nothing more with it. > > > > > > > > Now that ->mkdir can return a dentry, return the correct dentry. > > > > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > > --- > > > > fs/ceph/dir.c | 9 ++++++++- > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > > > > index 39e0f240de06..c1a1c168bb27 100644 > > > > --- a/fs/ceph/dir.c > > > > +++ b/fs/ceph/dir.c > > > > @@ -1099,6 +1099,7 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > > > struct ceph_client *cl = mdsc->fsc->client; > > > > struct ceph_mds_request *req; > > > > struct ceph_acl_sec_ctx as_ctx = {}; > > > > + struct dentry *ret = NULL; > > > > > > I believe that it makes sense to initialize pointer by error here and always > > > return ret as output. If something goes wrong in the logic, then we already have > > > error. > > > > I'm not certain that I understand, but I have made a change which seems > > to be consistent with the above and included it below. Please let me > > know if it is what you intended. > > > > > > > > > int err; > > > > int op; > > > > > > > > @@ -1166,14 +1167,20 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > > > !req->r_reply_info.head->is_dentry) > > > > err = ceph_handle_notrace_create(dir, dentry); > > > > out_req: > > > > + if (!err && req->r_dentry != dentry) > > > > + /* Some other dentry was spliced in */ > > > > + ret = dget(req->r_dentry); > > > > ceph_mdsc_put_request(req); > > > > out: > > > > if (!err) > > > > + /* Should this use 'ret' ?? */ > > > > > > Could we make a decision should or shouldn't? :) > > > It looks not good to leave this comment instead of proper implementation. Do we > > > have some obstacles to make this decision? > > > > I suspect we should use ret, but I didn't want to make a change which > > wasn't directly required by my needed. So I highlighted this which > > looks to me like a possible bug, hoping that someone more familiar with > > the code would give an opinion. Do you agree that 'ret' (i.e. > > ->r_dentry) should be used when ret is not NULL? > > > > I think if we are going to return ret as a dentry, then it makes sense to call > the ceph_init_inode_acls() for d_inode(ret). I don't see the point to call > ceph_init_inode_acls() for d_inode(dentry) then. > My assumption when looking at this was that they should point to the same inode. That said, working with d_inode(ret) after that point is less confusing to the casual reader. > > > > > > > ceph_init_inode_acls(d_inode(dentry), &as_ctx); > > > > else > > > > d_drop(dentry); > > > > ceph_release_acl_sec_ctx(&as_ctx); > > > > - return ERR_PTR(err); > > > > + if (err) > > > > + return ERR_PTR(err); > > > > + return ret; > > > > > > What's about this? > > > > > > return err ? ERR_PTR(err) : ret; > > > > We could do that, but you said above that you thought we should always > > return 'ret' - which does make some sense. > > > > What do you think of the following alternate patch? > > > > Patch looks good to me. Thanks. > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com> > > > Thanks, > > NeilBrown > > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > > index 39e0f240de06..d2e5c557df83 100644 > > --- a/fs/ceph/dir.c > > +++ b/fs/ceph/dir.c > > @@ -1099,6 +1099,7 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > struct ceph_client *cl = mdsc->fsc->client; > > struct ceph_mds_request *req; > > struct ceph_acl_sec_ctx as_ctx = {}; > > + struct dentry *ret; > > int err; > > int op; > > > > @@ -1116,32 +1117,32 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > ceph_vinop(dir), dentry, dentry, mode); > > op = CEPH_MDS_OP_MKDIR; > > } else { > > - err = -EROFS; > > + ret = ERR_PTR(-EROFS); > > goto out; > > } > > > > if (op == CEPH_MDS_OP_MKDIR && > > ceph_quota_is_max_files_exceeded(dir)) { > > - err = -EDQUOT; > > + ret = ERR_PTR(-EDQUOT); > > goto out; > > } > > if ((op == CEPH_MDS_OP_MKSNAP) && IS_ENCRYPTED(dir) && > > !fscrypt_has_encryption_key(dir)) { > > - err = -ENOKEY; > > + ret = ERR_PTR(-ENOKEY); > > goto out; > > } > > > > > > req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS); > > if (IS_ERR(req)) { > > - err = PTR_ERR(req); > > + ret = ERR_CAST(req); > > goto out; > > } > > > > mode |= S_IFDIR; > > req->r_new_inode = ceph_new_inode(dir, dentry, &mode, &as_ctx); > > if (IS_ERR(req->r_new_inode)) { > > - err = PTR_ERR(req->r_new_inode); > > + ret = ERR_CAST(req->r_new_inode); > > req->r_new_inode = NULL; > > goto out_req; > > } > > @@ -1165,15 +1166,23 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > !req->r_reply_info.head->is_target && > > !req->r_reply_info.head->is_dentry) > > err = ceph_handle_notrace_create(dir, dentry); > > + ret = ERR_PTR(err); > > out_req: > > + if (!IS_ERR(ret) && req->r_dentry != dentry) > > + /* Some other dentry was spliced in */ > > + ret = dget(req->r_dentry); > > ceph_mdsc_put_request(req); > > out: > > - if (!err) > > - ceph_init_inode_acls(d_inode(dentry), &as_ctx); > > - else > > + if (!IS_ERR(ret)) { > > + if (ret) > > + ceph_init_inode_acls(d_inode(ret), &as_ctx); > > + else > > + ceph_init_inode_acls(d_inode(dentry), &as_ctx); > > + } else { > > d_drop(dentry); > > + } > > ceph_release_acl_sec_ctx(&as_ctx); > > - return ERR_PTR(err); > > + return ret; > > } > > > > static int ceph_link(struct dentry *old_dentry, struct inode *dir, > > > > Thanks, > Slava. >
On Tue, 25 Feb 2025, Viacheslav Dubeyko wrote: > On Mon, 2025-02-24 at 13:15 +1100, NeilBrown wrote: > > On Fri, 21 Feb 2025, Viacheslav Dubeyko wrote: > > > On Fri, 2025-02-21 at 10:36 +1100, NeilBrown wrote: > > > > ceph already splices the correct dentry (in splice_dentry()) from the > > > > result of mkdir but does nothing more with it. > > > > > > > > Now that ->mkdir can return a dentry, return the correct dentry. > > > > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > > --- > > > > fs/ceph/dir.c | 9 ++++++++- > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > > > > index 39e0f240de06..c1a1c168bb27 100644 > > > > --- a/fs/ceph/dir.c > > > > +++ b/fs/ceph/dir.c > > > > @@ -1099,6 +1099,7 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > > > struct ceph_client *cl = mdsc->fsc->client; > > > > struct ceph_mds_request *req; > > > > struct ceph_acl_sec_ctx as_ctx = {}; > > > > + struct dentry *ret = NULL; > > > > > > I believe that it makes sense to initialize pointer by error here and always > > > return ret as output. If something goes wrong in the logic, then we already have > > > error. > > > > I'm not certain that I understand, but I have made a change which seems > > to be consistent with the above and included it below. Please let me > > know if it is what you intended. > > > > > > > > > int err; > > > > int op; > > > > > > > > @@ -1166,14 +1167,20 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > > > !req->r_reply_info.head->is_dentry) > > > > err = ceph_handle_notrace_create(dir, dentry); > > > > out_req: > > > > + if (!err && req->r_dentry != dentry) > > > > + /* Some other dentry was spliced in */ > > > > + ret = dget(req->r_dentry); > > > > ceph_mdsc_put_request(req); > > > > out: > > > > if (!err) > > > > + /* Should this use 'ret' ?? */ > > > > > > Could we make a decision should or shouldn't? :) > > > It looks not good to leave this comment instead of proper implementation. Do we > > > have some obstacles to make this decision? > > > > I suspect we should use ret, but I didn't want to make a change which > > wasn't directly required by my needed. So I highlighted this which > > looks to me like a possible bug, hoping that someone more familiar with > > the code would give an opinion. Do you agree that 'ret' (i.e. > > ->r_dentry) should be used when ret is not NULL? > > > > I think if we are going to return ret as a dentry, then it makes sense to call > the ceph_init_inode_acls() for d_inode(ret). I don't see the point to call > ceph_init_inode_acls() for d_inode(dentry) then. If the mkdir used the original dentry, then ->mkdir returns NULL so ret is NULL. If the mkdir used a different dentry it returns that, so ret is not NULL. I'll try to re-organise the code so that "dentry" is the correct dentry on success, and "ret" is the returned dentry, which might be NULL. Thanks, NeilBrown > > > > > > > > ceph_init_inode_acls(d_inode(dentry), &as_ctx); > > > > else > > > > d_drop(dentry); > > > > ceph_release_acl_sec_ctx(&as_ctx); > > > > - return ERR_PTR(err); > > > > + if (err) > > > > + return ERR_PTR(err); > > > > + return ret; > > > > > > What's about this? > > > > > > return err ? ERR_PTR(err) : ret; > > > > We could do that, but you said above that you thought we should always > > return 'ret' - which does make some sense. > > > > What do you think of the following alternate patch? > > > > Patch looks good to me. Thanks. > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com> > > > Thanks, > > NeilBrown > > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > > index 39e0f240de06..d2e5c557df83 100644 > > --- a/fs/ceph/dir.c > > +++ b/fs/ceph/dir.c > > @@ -1099,6 +1099,7 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > struct ceph_client *cl = mdsc->fsc->client; > > struct ceph_mds_request *req; > > struct ceph_acl_sec_ctx as_ctx = {}; > > + struct dentry *ret; > > int err; > > int op; > > > > @@ -1116,32 +1117,32 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > ceph_vinop(dir), dentry, dentry, mode); > > op = CEPH_MDS_OP_MKDIR; > > } else { > > - err = -EROFS; > > + ret = ERR_PTR(-EROFS); > > goto out; > > } > > > > if (op == CEPH_MDS_OP_MKDIR && > > ceph_quota_is_max_files_exceeded(dir)) { > > - err = -EDQUOT; > > + ret = ERR_PTR(-EDQUOT); > > goto out; > > } > > if ((op == CEPH_MDS_OP_MKSNAP) && IS_ENCRYPTED(dir) && > > !fscrypt_has_encryption_key(dir)) { > > - err = -ENOKEY; > > + ret = ERR_PTR(-ENOKEY); > > goto out; > > } > > > > > > req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS); > > if (IS_ERR(req)) { > > - err = PTR_ERR(req); > > + ret = ERR_CAST(req); > > goto out; > > } > > > > mode |= S_IFDIR; > > req->r_new_inode = ceph_new_inode(dir, dentry, &mode, &as_ctx); > > if (IS_ERR(req->r_new_inode)) { > > - err = PTR_ERR(req->r_new_inode); > > + ret = ERR_CAST(req->r_new_inode); > > req->r_new_inode = NULL; > > goto out_req; > > } > > @@ -1165,15 +1166,23 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, > > !req->r_reply_info.head->is_target && > > !req->r_reply_info.head->is_dentry) > > err = ceph_handle_notrace_create(dir, dentry); > > + ret = ERR_PTR(err); > > out_req: > > + if (!IS_ERR(ret) && req->r_dentry != dentry) > > + /* Some other dentry was spliced in */ > > + ret = dget(req->r_dentry); > > ceph_mdsc_put_request(req); > > out: > > - if (!err) > > - ceph_init_inode_acls(d_inode(dentry), &as_ctx); > > - else > > + if (!IS_ERR(ret)) { > > + if (ret) > > + ceph_init_inode_acls(d_inode(ret), &as_ctx); > > + else > > + ceph_init_inode_acls(d_inode(dentry), &as_ctx); > > + } else { > > d_drop(dentry); > > + } > > ceph_release_acl_sec_ctx(&as_ctx); > > - return ERR_PTR(err); > > + return ret; > > } > > > > static int ceph_link(struct dentry *old_dentry, struct inode *dir, > > > > Thanks, > Slava. > >
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index 39e0f240de06..c1a1c168bb27 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -1099,6 +1099,7 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, struct ceph_client *cl = mdsc->fsc->client; struct ceph_mds_request *req; struct ceph_acl_sec_ctx as_ctx = {}; + struct dentry *ret = NULL; int err; int op; @@ -1166,14 +1167,20 @@ static struct dentry *ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir, !req->r_reply_info.head->is_dentry) err = ceph_handle_notrace_create(dir, dentry); out_req: + if (!err && req->r_dentry != dentry) + /* Some other dentry was spliced in */ + ret = dget(req->r_dentry); ceph_mdsc_put_request(req); out: if (!err) + /* Should this use 'ret' ?? */ ceph_init_inode_acls(d_inode(dentry), &as_ctx); else d_drop(dentry); ceph_release_acl_sec_ctx(&as_ctx); - return ERR_PTR(err); + if (err) + return ERR_PTR(err); + return ret; } static int ceph_link(struct dentry *old_dentry, struct inode *dir,
ceph already splices the correct dentry (in splice_dentry()) from the result of mkdir but does nothing more with it. Now that ->mkdir can return a dentry, return the correct dentry. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/ceph/dir.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)