| Message ID | 531A5DBD.3040007@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 8 Mar 2014, Yan, Zheng wrote: > how about the below patch and corresponding mds change in > https://github.com/ceph/ceph/commit/617ce6761edd7264893f3638c33fd229c71751a0 I like this better, thanks! One question, though: in the LOOKUPNAME case, the client will already have a cap on both the parent and child inode, right? (IIUC, it will have done a LOOKUPINO on the fh/child, then LOOKUPPARENT to get a cap for the parent inode, and then a LOOKUPNAME to validate the name. Is the reason you still do MDCache::open_ino() because we can't assume that the parent and child are on the same MDS? Thanks! sage > > Regards > Yan, Zheng > > --- > >From 0fa1971741b1d3c236ee6fa3a7feb5a74fbd7f2f Mon Sep 17 00:00:00 2001 > From: "Yan, Zheng" <zheng.z.yan@intel.com> > Date: Thu, 6 Mar 2014 16:40:32 +0800 > Subject: [PATCH 1/3] ceph: add get_name() NFS export callback > > Use the newly introduced LOOKUPNAME MDS request to connect child > inode to its parent directory. > > Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> > --- > fs/ceph/export.c | 40 ++++++++++++++++++++++++++++++++++ > fs/ceph/inode.c | 51 +++++++++++++++++++++++++++++++++++++++++++- > fs/ceph/strings.c | 1 + > include/linux/ceph/ceph_fs.h | 1 + > 4 files changed, 92 insertions(+), 1 deletion(-) > > diff --git a/fs/ceph/export.c b/fs/ceph/export.c > index 6e611e7..bf36e7f 100644 > --- a/fs/ceph/export.c > +++ b/fs/ceph/export.c > @@ -195,9 +195,49 @@ static struct dentry *ceph_fh_to_parent(struct super_block *sb, > return dentry; > } > > +static int ceph_get_name(struct dentry *parent, char *name, > + struct dentry *child) > +{ > + struct ceph_mds_client *mdsc; > + struct ceph_mds_request *req; > + int err; > + > + mdsc = ceph_inode_to_client(child->d_inode)->mdsc; > + req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_LOOKUPNAME, > + USE_ANY_MDS); > + if (IS_ERR(req)) > + return PTR_ERR(req); > + > + mutex_lock(&parent->d_inode->i_mutex); > + > + req->r_inode = child->d_inode; > + ihold(child->d_inode); > + req->r_ino2 = ceph_vino(parent->d_inode); > + req->r_locked_dir = parent->d_inode; > + req->r_num_caps = 1; > + err = ceph_mdsc_do_request(mdsc, NULL, req); > + > + mutex_unlock(&parent->d_inode->i_mutex); > + > + if (!err) { > + struct ceph_mds_reply_info_parsed *rinfo = &req->r_reply_info; > + memcpy(name, rinfo->dname, rinfo->dname_len); > + name[rinfo->dname_len] = 0; > + dout("get_name %p ino %llx.%llx name %s\n", > + child, ceph_vinop(child->d_inode), name); > + } else { > + dout("get_name %p ino %llx.%llx err %d\n", > + child, ceph_vinop(child->d_inode), err); > + } > + > + ceph_mdsc_put_request(req); > + return err; > +} > + > const struct export_operations ceph_export_ops = { > .encode_fh = ceph_encode_fh, > .fh_to_dentry = ceph_fh_to_dentry, > .fh_to_parent = ceph_fh_to_parent, > .get_parent = ceph_get_parent, > + .get_name = ceph_get_name, > }; > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index 8bf2384..91d6c9d 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -1044,10 +1044,59 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req, > session, req->r_request_started, -1, > &req->r_caps_reservation); > if (err < 0) > - return err; > + goto done; > } else { > WARN_ON_ONCE(1); > } > + > + if (dir && req->r_op == CEPH_MDS_OP_LOOKUPNAME) { > + struct qstr dname; > + struct dentry *dn, *parent; > + > + BUG_ON(!rinfo->head->is_target); > + BUG_ON(req->r_dentry); > + > + parent = d_find_any_alias(dir); > + BUG_ON(!parent); > + > + dname.name = rinfo->dname; > + dname.len = rinfo->dname_len; > + dname.hash = full_name_hash(dname.name, dname.len); > + vino.ino = le64_to_cpu(rinfo->targeti.in->ino); > + vino.snap = le64_to_cpu(rinfo->targeti.in->snapid); > +retry_lookup: > + dn = d_lookup(parent, &dname); > + dout("d_lookup on parent=%p name=%.*s got %p\n", > + parent, dname.len, dname.name, dn); > + > + if (!dn) { > + dn = d_alloc(parent, &dname); > + dout("d_alloc %p '%.*s' = %p\n", parent, > + dname.len, dname.name, dn); > + if (dn == NULL) { > + dput(parent); > + err = -ENOMEM; > + goto done; > + } > + err = ceph_init_dentry(dn); > + if (err < 0) { > + dput(dn); > + dput(parent); > + goto done; > + } > + } else if (dn->d_inode && > + (ceph_ino(dn->d_inode) != vino.ino || > + ceph_snap(dn->d_inode) != vino.snap)) { > + dout(" dn %p points to wrong inode %p\n", > + dn, dn->d_inode); > + d_delete(dn); > + dput(dn); > + goto retry_lookup; > + } > + > + req->r_dentry = dn; > + dput(parent); > + } > } > > if (rinfo->head->is_target) { > diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c > index 4440f447..51cc23e 100644 > --- a/fs/ceph/strings.c > +++ b/fs/ceph/strings.c > @@ -54,6 +54,7 @@ const char *ceph_mds_op_name(int op) > case CEPH_MDS_OP_LOOKUPHASH: return "lookuphash"; > case CEPH_MDS_OP_LOOKUPPARENT: return "lookupparent"; > case CEPH_MDS_OP_LOOKUPINO: return "lookupino"; > + case CEPH_MDS_OP_LOOKUPNAME: return "lookupname"; > case CEPH_MDS_OP_GETATTR: return "getattr"; > case CEPH_MDS_OP_SETXATTR: return "setxattr"; > case CEPH_MDS_OP_SETATTR: return "setattr"; > diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h > index 25bfb0e..35f345f 100644 > --- a/include/linux/ceph/ceph_fs.h > +++ b/include/linux/ceph/ceph_fs.h > @@ -332,6 +332,7 @@ enum { > CEPH_MDS_OP_LOOKUPHASH = 0x00102, > CEPH_MDS_OP_LOOKUPPARENT = 0x00103, > CEPH_MDS_OP_LOOKUPINO = 0x00104, > + CEPH_MDS_OP_LOOKUPNAME = 0x00105, > > CEPH_MDS_OP_SETXATTR = 0x01105, > CEPH_MDS_OP_RMXATTR = 0x01106, > -- > 1.8.5.3 > > -- 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 Sat, 8 Mar 2014, Yan, Zheng wrote: > how about the below patch and corresponding mds change in > https://github.com/ceph/ceph/commit/617ce6761edd7264893f3638c33fd229c71751a0 > > Regards > Yan, Zheng > > --- > >From 0fa1971741b1d3c236ee6fa3a7feb5a74fbd7f2f Mon Sep 17 00:00:00 2001 > From: "Yan, Zheng" <zheng.z.yan@intel.com> > Date: Thu, 6 Mar 2014 16:40:32 +0800 > Subject: [PATCH 1/3] ceph: add get_name() NFS export callback > > Use the newly introduced LOOKUPNAME MDS request to connect child > inode to its parent directory. > > Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> > --- > fs/ceph/export.c | 40 ++++++++++++++++++++++++++++++++++ > fs/ceph/inode.c | 51 +++++++++++++++++++++++++++++++++++++++++++- > fs/ceph/strings.c | 1 + > include/linux/ceph/ceph_fs.h | 1 + > 4 files changed, 92 insertions(+), 1 deletion(-) > > diff --git a/fs/ceph/export.c b/fs/ceph/export.c > index 6e611e7..bf36e7f 100644 > --- a/fs/ceph/export.c > +++ b/fs/ceph/export.c > @@ -195,9 +195,49 @@ static struct dentry *ceph_fh_to_parent(struct super_block *sb, > return dentry; > } > > +static int ceph_get_name(struct dentry *parent, char *name, > + struct dentry *child) > +{ > + struct ceph_mds_client *mdsc; > + struct ceph_mds_request *req; > + int err; > + > + mdsc = ceph_inode_to_client(child->d_inode)->mdsc; > + req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_LOOKUPNAME, > + USE_ANY_MDS); > + if (IS_ERR(req)) > + return PTR_ERR(req); > + > + mutex_lock(&parent->d_inode->i_mutex); > + > + req->r_inode = child->d_inode; > + ihold(child->d_inode); > + req->r_ino2 = ceph_vino(parent->d_inode); > + req->r_locked_dir = parent->d_inode; > + req->r_num_caps = 1; > + err = ceph_mdsc_do_request(mdsc, NULL, req); > + > + mutex_unlock(&parent->d_inode->i_mutex); > + > + if (!err) { > + struct ceph_mds_reply_info_parsed *rinfo = &req->r_reply_info; > + memcpy(name, rinfo->dname, rinfo->dname_len); > + name[rinfo->dname_len] = 0; > + dout("get_name %p ino %llx.%llx name %s\n", > + child, ceph_vinop(child->d_inode), name); One other oddity here: the MDS is returning a bunch of metadata about the dentry, including (in most cases) a lease. The client is ignoring all of that and only needs the name itself to feed back into exportfs. I believe at this point that is fine: the client can forget leases at any time and will respond to MDS revocations accordingly. It would probably require a bit of a kludge on the MDS side to make the reply_request() respond with the dentry name but prevent a lease from being issued. Might be worth it though? Maybe a generic "no lease" flag that even regular LOOKUP could use (if for some reason the client didn't want a lease)? This needn't hold up the other patches, but I'm curious what you think about it. Thanks! sage > + } else { > + dout("get_name %p ino %llx.%llx err %d\n", > + child, ceph_vinop(child->d_inode), err); > + } > + > + ceph_mdsc_put_request(req); > + return err; > +} > + > const struct export_operations ceph_export_ops = { > .encode_fh = ceph_encode_fh, > .fh_to_dentry = ceph_fh_to_dentry, > .fh_to_parent = ceph_fh_to_parent, > .get_parent = ceph_get_parent, > + .get_name = ceph_get_name, > }; > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index 8bf2384..91d6c9d 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -1044,10 +1044,59 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req, > session, req->r_request_started, -1, > &req->r_caps_reservation); > if (err < 0) > - return err; > + goto done; > } else { > WARN_ON_ONCE(1); > } > + > + if (dir && req->r_op == CEPH_MDS_OP_LOOKUPNAME) { > + struct qstr dname; > + struct dentry *dn, *parent; > + > + BUG_ON(!rinfo->head->is_target); > + BUG_ON(req->r_dentry); > + > + parent = d_find_any_alias(dir); > + BUG_ON(!parent); > + > + dname.name = rinfo->dname; > + dname.len = rinfo->dname_len; > + dname.hash = full_name_hash(dname.name, dname.len); > + vino.ino = le64_to_cpu(rinfo->targeti.in->ino); > + vino.snap = le64_to_cpu(rinfo->targeti.in->snapid); > +retry_lookup: > + dn = d_lookup(parent, &dname); > + dout("d_lookup on parent=%p name=%.*s got %p\n", > + parent, dname.len, dname.name, dn); > + > + if (!dn) { > + dn = d_alloc(parent, &dname); > + dout("d_alloc %p '%.*s' = %p\n", parent, > + dname.len, dname.name, dn); > + if (dn == NULL) { > + dput(parent); > + err = -ENOMEM; > + goto done; > + } > + err = ceph_init_dentry(dn); > + if (err < 0) { > + dput(dn); > + dput(parent); > + goto done; > + } > + } else if (dn->d_inode && > + (ceph_ino(dn->d_inode) != vino.ino || > + ceph_snap(dn->d_inode) != vino.snap)) { > + dout(" dn %p points to wrong inode %p\n", > + dn, dn->d_inode); > + d_delete(dn); > + dput(dn); > + goto retry_lookup; > + } > + > + req->r_dentry = dn; > + dput(parent); > + } > } > > if (rinfo->head->is_target) { > diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c > index 4440f447..51cc23e 100644 > --- a/fs/ceph/strings.c > +++ b/fs/ceph/strings.c > @@ -54,6 +54,7 @@ const char *ceph_mds_op_name(int op) > case CEPH_MDS_OP_LOOKUPHASH: return "lookuphash"; > case CEPH_MDS_OP_LOOKUPPARENT: return "lookupparent"; > case CEPH_MDS_OP_LOOKUPINO: return "lookupino"; > + case CEPH_MDS_OP_LOOKUPNAME: return "lookupname"; > case CEPH_MDS_OP_GETATTR: return "getattr"; > case CEPH_MDS_OP_SETXATTR: return "setxattr"; > case CEPH_MDS_OP_SETATTR: return "setattr"; > diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h > index 25bfb0e..35f345f 100644 > --- a/include/linux/ceph/ceph_fs.h > +++ b/include/linux/ceph/ceph_fs.h > @@ -332,6 +332,7 @@ enum { > CEPH_MDS_OP_LOOKUPHASH = 0x00102, > CEPH_MDS_OP_LOOKUPPARENT = 0x00103, > CEPH_MDS_OP_LOOKUPINO = 0x00104, > + CEPH_MDS_OP_LOOKUPNAME = 0x00105, > > CEPH_MDS_OP_SETXATTR = 0x01105, > CEPH_MDS_OP_RMXATTR = 0x01106, > -- > 1.8.5.3 > > -- > 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 03/10/2014 09:43 AM, Sage Weil wrote: > On Sat, 8 Mar 2014, Yan, Zheng wrote: >> how about the below patch and corresponding mds change in >> https://github.com/ceph/ceph/commit/617ce6761edd7264893f3638c33fd229c71751a0 > > I like this better, thanks! > > One question, though: in the LOOKUPNAME case, the client will already have > a cap on both the parent and child inode, right? (IIUC, it will have done > a LOOKUPINO on the fh/child, then LOOKUPPARENT to get a cap for the > parent inode, and then a LOOKUPNAME to validate the name. > > Is the reason you still do MDCache::open_ino() because we can't assume > that the parent and child are on the same MDS? For the LOOKUPNAME case, MDCache::open_ino() is not needed. I'm lazy to write separate function for LOOKUPNAME. Regards Yan, Zheng > > Thanks! > sage > > >> >> Regards >> Yan, Zheng >> >> --- >> >From 0fa1971741b1d3c236ee6fa3a7feb5a74fbd7f2f Mon Sep 17 00:00:00 2001 >> From: "Yan, Zheng" <zheng.z.yan@intel.com> >> Date: Thu, 6 Mar 2014 16:40:32 +0800 >> Subject: [PATCH 1/3] ceph: add get_name() NFS export callback >> >> Use the newly introduced LOOKUPNAME MDS request to connect child >> inode to its parent directory. >> >> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> >> --- >> fs/ceph/export.c | 40 ++++++++++++++++++++++++++++++++++ >> fs/ceph/inode.c | 51 +++++++++++++++++++++++++++++++++++++++++++- >> fs/ceph/strings.c | 1 + >> include/linux/ceph/ceph_fs.h | 1 + >> 4 files changed, 92 insertions(+), 1 deletion(-) >> >> diff --git a/fs/ceph/export.c b/fs/ceph/export.c >> index 6e611e7..bf36e7f 100644 >> --- a/fs/ceph/export.c >> +++ b/fs/ceph/export.c >> @@ -195,9 +195,49 @@ static struct dentry *ceph_fh_to_parent(struct super_block *sb, >> return dentry; >> } >> >> +static int ceph_get_name(struct dentry *parent, char *name, >> + struct dentry *child) >> +{ >> + struct ceph_mds_client *mdsc; >> + struct ceph_mds_request *req; >> + int err; >> + >> + mdsc = ceph_inode_to_client(child->d_inode)->mdsc; >> + req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_LOOKUPNAME, >> + USE_ANY_MDS); >> + if (IS_ERR(req)) >> + return PTR_ERR(req); >> + >> + mutex_lock(&parent->d_inode->i_mutex); >> + >> + req->r_inode = child->d_inode; >> + ihold(child->d_inode); >> + req->r_ino2 = ceph_vino(parent->d_inode); >> + req->r_locked_dir = parent->d_inode; >> + req->r_num_caps = 1; >> + err = ceph_mdsc_do_request(mdsc, NULL, req); >> + >> + mutex_unlock(&parent->d_inode->i_mutex); >> + >> + if (!err) { >> + struct ceph_mds_reply_info_parsed *rinfo = &req->r_reply_info; >> + memcpy(name, rinfo->dname, rinfo->dname_len); >> + name[rinfo->dname_len] = 0; >> + dout("get_name %p ino %llx.%llx name %s\n", >> + child, ceph_vinop(child->d_inode), name); >> + } else { >> + dout("get_name %p ino %llx.%llx err %d\n", >> + child, ceph_vinop(child->d_inode), err); >> + } >> + >> + ceph_mdsc_put_request(req); >> + return err; >> +} >> + >> const struct export_operations ceph_export_ops = { >> .encode_fh = ceph_encode_fh, >> .fh_to_dentry = ceph_fh_to_dentry, >> .fh_to_parent = ceph_fh_to_parent, >> .get_parent = ceph_get_parent, >> + .get_name = ceph_get_name, >> }; >> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c >> index 8bf2384..91d6c9d 100644 >> --- a/fs/ceph/inode.c >> +++ b/fs/ceph/inode.c >> @@ -1044,10 +1044,59 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req, >> session, req->r_request_started, -1, >> &req->r_caps_reservation); >> if (err < 0) >> - return err; >> + goto done; >> } else { >> WARN_ON_ONCE(1); >> } >> + >> + if (dir && req->r_op == CEPH_MDS_OP_LOOKUPNAME) { >> + struct qstr dname; >> + struct dentry *dn, *parent; >> + >> + BUG_ON(!rinfo->head->is_target); >> + BUG_ON(req->r_dentry); >> + >> + parent = d_find_any_alias(dir); >> + BUG_ON(!parent); >> + >> + dname.name = rinfo->dname; >> + dname.len = rinfo->dname_len; >> + dname.hash = full_name_hash(dname.name, dname.len); >> + vino.ino = le64_to_cpu(rinfo->targeti.in->ino); >> + vino.snap = le64_to_cpu(rinfo->targeti.in->snapid); >> +retry_lookup: >> + dn = d_lookup(parent, &dname); >> + dout("d_lookup on parent=%p name=%.*s got %p\n", >> + parent, dname.len, dname.name, dn); >> + >> + if (!dn) { >> + dn = d_alloc(parent, &dname); >> + dout("d_alloc %p '%.*s' = %p\n", parent, >> + dname.len, dname.name, dn); >> + if (dn == NULL) { >> + dput(parent); >> + err = -ENOMEM; >> + goto done; >> + } >> + err = ceph_init_dentry(dn); >> + if (err < 0) { >> + dput(dn); >> + dput(parent); >> + goto done; >> + } >> + } else if (dn->d_inode && >> + (ceph_ino(dn->d_inode) != vino.ino || >> + ceph_snap(dn->d_inode) != vino.snap)) { >> + dout(" dn %p points to wrong inode %p\n", >> + dn, dn->d_inode); >> + d_delete(dn); >> + dput(dn); >> + goto retry_lookup; >> + } >> + >> + req->r_dentry = dn; >> + dput(parent); >> + } >> } >> >> if (rinfo->head->is_target) { >> diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c >> index 4440f447..51cc23e 100644 >> --- a/fs/ceph/strings.c >> +++ b/fs/ceph/strings.c >> @@ -54,6 +54,7 @@ const char *ceph_mds_op_name(int op) >> case CEPH_MDS_OP_LOOKUPHASH: return "lookuphash"; >> case CEPH_MDS_OP_LOOKUPPARENT: return "lookupparent"; >> case CEPH_MDS_OP_LOOKUPINO: return "lookupino"; >> + case CEPH_MDS_OP_LOOKUPNAME: return "lookupname"; >> case CEPH_MDS_OP_GETATTR: return "getattr"; >> case CEPH_MDS_OP_SETXATTR: return "setxattr"; >> case CEPH_MDS_OP_SETATTR: return "setattr"; >> diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h >> index 25bfb0e..35f345f 100644 >> --- a/include/linux/ceph/ceph_fs.h >> +++ b/include/linux/ceph/ceph_fs.h >> @@ -332,6 +332,7 @@ enum { >> CEPH_MDS_OP_LOOKUPHASH = 0x00102, >> CEPH_MDS_OP_LOOKUPPARENT = 0x00103, >> CEPH_MDS_OP_LOOKUPINO = 0x00104, >> + CEPH_MDS_OP_LOOKUPNAME = 0x00105, >> >> CEPH_MDS_OP_SETXATTR = 0x01105, >> CEPH_MDS_OP_RMXATTR = 0x01106, >> -- >> 1.8.5.3 >> >> -- 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 03/10/2014 09:49 AM, Sage Weil wrote: > On Sat, 8 Mar 2014, Yan, Zheng wrote: >> how about the below patch and corresponding mds change in >> https://github.com/ceph/ceph/commit/617ce6761edd7264893f3638c33fd229c71751a0 >> >> Regards >> Yan, Zheng >> >> --- >> >From 0fa1971741b1d3c236ee6fa3a7feb5a74fbd7f2f Mon Sep 17 00:00:00 2001 >> From: "Yan, Zheng" <zheng.z.yan@intel.com> >> Date: Thu, 6 Mar 2014 16:40:32 +0800 >> Subject: [PATCH 1/3] ceph: add get_name() NFS export callback >> >> Use the newly introduced LOOKUPNAME MDS request to connect child >> inode to its parent directory. >> >> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> >> --- >> fs/ceph/export.c | 40 ++++++++++++++++++++++++++++++++++ >> fs/ceph/inode.c | 51 +++++++++++++++++++++++++++++++++++++++++++- >> fs/ceph/strings.c | 1 + >> include/linux/ceph/ceph_fs.h | 1 + >> 4 files changed, 92 insertions(+), 1 deletion(-) >> >> diff --git a/fs/ceph/export.c b/fs/ceph/export.c >> index 6e611e7..bf36e7f 100644 >> --- a/fs/ceph/export.c >> +++ b/fs/ceph/export.c >> @@ -195,9 +195,49 @@ static struct dentry *ceph_fh_to_parent(struct super_block *sb, >> return dentry; >> } >> >> +static int ceph_get_name(struct dentry *parent, char *name, >> + struct dentry *child) >> +{ >> + struct ceph_mds_client *mdsc; >> + struct ceph_mds_request *req; >> + int err; >> + >> + mdsc = ceph_inode_to_client(child->d_inode)->mdsc; >> + req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_LOOKUPNAME, >> + USE_ANY_MDS); >> + if (IS_ERR(req)) >> + return PTR_ERR(req); >> + >> + mutex_lock(&parent->d_inode->i_mutex); >> + >> + req->r_inode = child->d_inode; >> + ihold(child->d_inode); >> + req->r_ino2 = ceph_vino(parent->d_inode); >> + req->r_locked_dir = parent->d_inode; >> + req->r_num_caps = 1; >> + err = ceph_mdsc_do_request(mdsc, NULL, req); >> + >> + mutex_unlock(&parent->d_inode->i_mutex); >> + >> + if (!err) { >> + struct ceph_mds_reply_info_parsed *rinfo = &req->r_reply_info; >> + memcpy(name, rinfo->dname, rinfo->dname_len); >> + name[rinfo->dname_len] = 0; >> + dout("get_name %p ino %llx.%llx name %s\n", >> + child, ceph_vinop(child->d_inode), name); > > One other oddity here: the MDS is returning a bunch of metadata about the > dentry, including (in most cases) a lease. The client is ignoring all of > that and only needs the name itself to feed back into exportfs. I believe > at this point that is fine: the client can forget leases at any time and > will respond to MDS revocations accordingly. > > It would probably require a bit of a kludge on the MDS side to make the > reply_request() respond with the dentry name but prevent a lease from > being issued. Might be worth it though? Maybe a generic "no lease" flag > that even regular LOOKUP could use (if for some reason the client didn't > want a lease)? > > This needn't hold up the other patches, but I'm curious what you think > about it. > the nfsd does a dentry lookup after it has got the name. the lease can avoid sending another 'lookup' request to the MDS. Regards Yan, Zheng > > >> + } else { >> + dout("get_name %p ino %llx.%llx err %d\n", >> + child, ceph_vinop(child->d_inode), err); >> + } >> + >> + ceph_mdsc_put_request(req); >> + return err; >> +} >> + >> const struct export_operations ceph_export_ops = { >> .encode_fh = ceph_encode_fh, >> .fh_to_dentry = ceph_fh_to_dentry, >> .fh_to_parent = ceph_fh_to_parent, >> .get_parent = ceph_get_parent, >> + .get_name = ceph_get_name, >> }; >> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c >> index 8bf2384..91d6c9d 100644 >> --- a/fs/ceph/inode.c >> +++ b/fs/ceph/inode.c >> @@ -1044,10 +1044,59 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req, >> session, req->r_request_started, -1, >> &req->r_caps_reservation); >> if (err < 0) >> - return err; >> + goto done; >> } else { >> WARN_ON_ONCE(1); >> } >> + >> + if (dir && req->r_op == CEPH_MDS_OP_LOOKUPNAME) { >> + struct qstr dname; >> + struct dentry *dn, *parent; >> + >> + BUG_ON(!rinfo->head->is_target); >> + BUG_ON(req->r_dentry); >> + >> + parent = d_find_any_alias(dir); >> + BUG_ON(!parent); >> + >> + dname.name = rinfo->dname; >> + dname.len = rinfo->dname_len; >> + dname.hash = full_name_hash(dname.name, dname.len); >> + vino.ino = le64_to_cpu(rinfo->targeti.in->ino); >> + vino.snap = le64_to_cpu(rinfo->targeti.in->snapid); >> +retry_lookup: >> + dn = d_lookup(parent, &dname); >> + dout("d_lookup on parent=%p name=%.*s got %p\n", >> + parent, dname.len, dname.name, dn); >> + >> + if (!dn) { >> + dn = d_alloc(parent, &dname); >> + dout("d_alloc %p '%.*s' = %p\n", parent, >> + dname.len, dname.name, dn); >> + if (dn == NULL) { >> + dput(parent); >> + err = -ENOMEM; >> + goto done; >> + } >> + err = ceph_init_dentry(dn); >> + if (err < 0) { >> + dput(dn); >> + dput(parent); >> + goto done; >> + } >> + } else if (dn->d_inode && >> + (ceph_ino(dn->d_inode) != vino.ino || >> + ceph_snap(dn->d_inode) != vino.snap)) { >> + dout(" dn %p points to wrong inode %p\n", >> + dn, dn->d_inode); >> + d_delete(dn); >> + dput(dn); >> + goto retry_lookup; >> + } >> + >> + req->r_dentry = dn; >> + dput(parent); >> + } >> } >> >> if (rinfo->head->is_target) { >> diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c >> index 4440f447..51cc23e 100644 >> --- a/fs/ceph/strings.c >> +++ b/fs/ceph/strings.c >> @@ -54,6 +54,7 @@ const char *ceph_mds_op_name(int op) >> case CEPH_MDS_OP_LOOKUPHASH: return "lookuphash"; >> case CEPH_MDS_OP_LOOKUPPARENT: return "lookupparent"; >> case CEPH_MDS_OP_LOOKUPINO: return "lookupino"; >> + case CEPH_MDS_OP_LOOKUPNAME: return "lookupname"; >> case CEPH_MDS_OP_GETATTR: return "getattr"; >> case CEPH_MDS_OP_SETXATTR: return "setxattr"; >> case CEPH_MDS_OP_SETATTR: return "setattr"; >> diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h >> index 25bfb0e..35f345f 100644 >> --- a/include/linux/ceph/ceph_fs.h >> +++ b/include/linux/ceph/ceph_fs.h >> @@ -332,6 +332,7 @@ enum { >> CEPH_MDS_OP_LOOKUPHASH = 0x00102, >> CEPH_MDS_OP_LOOKUPPARENT = 0x00103, >> CEPH_MDS_OP_LOOKUPINO = 0x00104, >> + CEPH_MDS_OP_LOOKUPNAME = 0x00105, >> >> CEPH_MDS_OP_SETXATTR = 0x01105, >> CEPH_MDS_OP_RMXATTR = 0x01106, >> -- >> 1.8.5.3 >> >> -- >> 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, 10 Mar 2014, Yan, Zheng wrote: > On 03/10/2014 09:49 AM, Sage Weil wrote: > > On Sat, 8 Mar 2014, Yan, Zheng wrote: > >> how about the below patch and corresponding mds change in > >> https://github.com/ceph/ceph/commit/617ce6761edd7264893f3638c33fd229c71751a0 > >> > >> Regards > >> Yan, Zheng > >> > >> --- > >> >From 0fa1971741b1d3c236ee6fa3a7feb5a74fbd7f2f Mon Sep 17 00:00:00 2001 > >> From: "Yan, Zheng" <zheng.z.yan@intel.com> > >> Date: Thu, 6 Mar 2014 16:40:32 +0800 > >> Subject: [PATCH 1/3] ceph: add get_name() NFS export callback > >> > >> Use the newly introduced LOOKUPNAME MDS request to connect child > >> inode to its parent directory. > >> > >> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> > >> --- > >> fs/ceph/export.c | 40 ++++++++++++++++++++++++++++++++++ > >> fs/ceph/inode.c | 51 +++++++++++++++++++++++++++++++++++++++++++- > >> fs/ceph/strings.c | 1 + > >> include/linux/ceph/ceph_fs.h | 1 + > >> 4 files changed, 92 insertions(+), 1 deletion(-) > >> > >> diff --git a/fs/ceph/export.c b/fs/ceph/export.c > >> index 6e611e7..bf36e7f 100644 > >> --- a/fs/ceph/export.c > >> +++ b/fs/ceph/export.c > >> @@ -195,9 +195,49 @@ static struct dentry *ceph_fh_to_parent(struct super_block *sb, > >> return dentry; > >> } > >> > >> +static int ceph_get_name(struct dentry *parent, char *name, > >> + struct dentry *child) > >> +{ > >> + struct ceph_mds_client *mdsc; > >> + struct ceph_mds_request *req; > >> + int err; > >> + > >> + mdsc = ceph_inode_to_client(child->d_inode)->mdsc; > >> + req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_LOOKUPNAME, > >> + USE_ANY_MDS); > >> + if (IS_ERR(req)) > >> + return PTR_ERR(req); > >> + > >> + mutex_lock(&parent->d_inode->i_mutex); > >> + > >> + req->r_inode = child->d_inode; > >> + ihold(child->d_inode); > >> + req->r_ino2 = ceph_vino(parent->d_inode); > >> + req->r_locked_dir = parent->d_inode; > >> + req->r_num_caps = 1; > >> + err = ceph_mdsc_do_request(mdsc, NULL, req); > >> + > >> + mutex_unlock(&parent->d_inode->i_mutex); > >> + > >> + if (!err) { > >> + struct ceph_mds_reply_info_parsed *rinfo = &req->r_reply_info; > >> + memcpy(name, rinfo->dname, rinfo->dname_len); > >> + name[rinfo->dname_len] = 0; > >> + dout("get_name %p ino %llx.%llx name %s\n", > >> + child, ceph_vinop(child->d_inode), name); > > > > One other oddity here: the MDS is returning a bunch of metadata about the > > dentry, including (in most cases) a lease. The client is ignoring all of > > that and only needs the name itself to feed back into exportfs. I believe > > at this point that is fine: the client can forget leases at any time and > > will respond to MDS revocations accordingly. > > > > It would probably require a bit of a kludge on the MDS side to make the > > reply_request() respond with the dentry name but prevent a lease from > > being issued. Might be worth it though? Maybe a generic "no lease" flag > > that even regular LOOKUP could use (if for some reason the client didn't > > want a lease)? > > > > This needn't hold up the other patches, but I'm curious what you think > > about it. > > > > the nfsd does a dentry lookup after it has got the name. the lease can avoid > sending another 'lookup' request to the MDS. For that to work, I think we need to preallocate the dentry and set req->r_dentry. The ceph_fill_trace() code, in fact, looks like it will BUG out if it gets a reply with a dentry but the request r_dentry isn't set (we should probably fix that too). In any case, that seems tricky because we don't know the name ahead of time, so it would need to have its own code path on reply to allocate the dentry of the proper name. Given that this is only used in rare NFS re-export corner cases, my inclination is to not bother optimizing? sage > > Regards > Yan, Zheng > > > > > > >> + } else { > >> + dout("get_name %p ino %llx.%llx err %d\n", > >> + child, ceph_vinop(child->d_inode), err); > >> + } > >> + > >> + ceph_mdsc_put_request(req); > >> + return err; > >> +} > >> + > >> const struct export_operations ceph_export_ops = { > >> .encode_fh = ceph_encode_fh, > >> .fh_to_dentry = ceph_fh_to_dentry, > >> .fh_to_parent = ceph_fh_to_parent, > >> .get_parent = ceph_get_parent, > >> + .get_name = ceph_get_name, > >> }; > >> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > >> index 8bf2384..91d6c9d 100644 > >> --- a/fs/ceph/inode.c > >> +++ b/fs/ceph/inode.c > >> @@ -1044,10 +1044,59 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req, > >> session, req->r_request_started, -1, > >> &req->r_caps_reservation); > >> if (err < 0) > >> - return err; > >> + goto done; > >> } else { > >> WARN_ON_ONCE(1); > >> } > >> + > >> + if (dir && req->r_op == CEPH_MDS_OP_LOOKUPNAME) { > >> + struct qstr dname; > >> + struct dentry *dn, *parent; > >> + > >> + BUG_ON(!rinfo->head->is_target); > >> + BUG_ON(req->r_dentry); > >> + > >> + parent = d_find_any_alias(dir); > >> + BUG_ON(!parent); > >> + > >> + dname.name = rinfo->dname; > >> + dname.len = rinfo->dname_len; > >> + dname.hash = full_name_hash(dname.name, dname.len); > >> + vino.ino = le64_to_cpu(rinfo->targeti.in->ino); > >> + vino.snap = le64_to_cpu(rinfo->targeti.in->snapid); > >> +retry_lookup: > >> + dn = d_lookup(parent, &dname); > >> + dout("d_lookup on parent=%p name=%.*s got %p\n", > >> + parent, dname.len, dname.name, dn); > >> + > >> + if (!dn) { > >> + dn = d_alloc(parent, &dname); > >> + dout("d_alloc %p '%.*s' = %p\n", parent, > >> + dname.len, dname.name, dn); > >> + if (dn == NULL) { > >> + dput(parent); > >> + err = -ENOMEM; > >> + goto done; > >> + } > >> + err = ceph_init_dentry(dn); > >> + if (err < 0) { > >> + dput(dn); > >> + dput(parent); > >> + goto done; > >> + } > >> + } else if (dn->d_inode && > >> + (ceph_ino(dn->d_inode) != vino.ino || > >> + ceph_snap(dn->d_inode) != vino.snap)) { > >> + dout(" dn %p points to wrong inode %p\n", > >> + dn, dn->d_inode); > >> + d_delete(dn); > >> + dput(dn); > >> + goto retry_lookup; > >> + } > >> + > >> + req->r_dentry = dn; > >> + dput(parent); > >> + } > >> } > >> > >> if (rinfo->head->is_target) { > >> diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c > >> index 4440f447..51cc23e 100644 > >> --- a/fs/ceph/strings.c > >> +++ b/fs/ceph/strings.c > >> @@ -54,6 +54,7 @@ const char *ceph_mds_op_name(int op) > >> case CEPH_MDS_OP_LOOKUPHASH: return "lookuphash"; > >> case CEPH_MDS_OP_LOOKUPPARENT: return "lookupparent"; > >> case CEPH_MDS_OP_LOOKUPINO: return "lookupino"; > >> + case CEPH_MDS_OP_LOOKUPNAME: return "lookupname"; > >> case CEPH_MDS_OP_GETATTR: return "getattr"; > >> case CEPH_MDS_OP_SETXATTR: return "setxattr"; > >> case CEPH_MDS_OP_SETATTR: return "setattr"; > >> diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h > >> index 25bfb0e..35f345f 100644 > >> --- a/include/linux/ceph/ceph_fs.h > >> +++ b/include/linux/ceph/ceph_fs.h > >> @@ -332,6 +332,7 @@ enum { > >> CEPH_MDS_OP_LOOKUPHASH = 0x00102, > >> CEPH_MDS_OP_LOOKUPPARENT = 0x00103, > >> CEPH_MDS_OP_LOOKUPINO = 0x00104, > >> + CEPH_MDS_OP_LOOKUPNAME = 0x00105, > >> > >> CEPH_MDS_OP_SETXATTR = 0x01105, > >> CEPH_MDS_OP_RMXATTR = 0x01106, > >> -- > >> 1.8.5.3 > >> > >> -- > >> 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 03/10/2014 12:12 PM, Sage Weil wrote: > On Mon, 10 Mar 2014, Yan, Zheng wrote: >> On 03/10/2014 09:49 AM, Sage Weil wrote: >>> On Sat, 8 Mar 2014, Yan, Zheng wrote: >>>> how about the below patch and corresponding mds change in >>>> https://github.com/ceph/ceph/commit/617ce6761edd7264893f3638c33fd229c71751a0 >>>> >>>> Regards >>>> Yan, Zheng >>>> >>>> --- >>>> >From 0fa1971741b1d3c236ee6fa3a7feb5a74fbd7f2f Mon Sep 17 00:00:00 2001 >>>> From: "Yan, Zheng" <zheng.z.yan@intel.com> >>>> Date: Thu, 6 Mar 2014 16:40:32 +0800 >>>> Subject: [PATCH 1/3] ceph: add get_name() NFS export callback >>>> >>>> Use the newly introduced LOOKUPNAME MDS request to connect child >>>> inode to its parent directory. >>>> >>>> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> >>>> --- >>>> fs/ceph/export.c | 40 ++++++++++++++++++++++++++++++++++ >>>> fs/ceph/inode.c | 51 +++++++++++++++++++++++++++++++++++++++++++- >>>> fs/ceph/strings.c | 1 + >>>> include/linux/ceph/ceph_fs.h | 1 + >>>> 4 files changed, 92 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/ceph/export.c b/fs/ceph/export.c >>>> index 6e611e7..bf36e7f 100644 >>>> --- a/fs/ceph/export.c >>>> +++ b/fs/ceph/export.c >>>> @@ -195,9 +195,49 @@ static struct dentry *ceph_fh_to_parent(struct super_block *sb, >>>> return dentry; >>>> } >>>> >>>> +static int ceph_get_name(struct dentry *parent, char *name, >>>> + struct dentry *child) >>>> +{ >>>> + struct ceph_mds_client *mdsc; >>>> + struct ceph_mds_request *req; >>>> + int err; >>>> + >>>> + mdsc = ceph_inode_to_client(child->d_inode)->mdsc; >>>> + req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_LOOKUPNAME, >>>> + USE_ANY_MDS); >>>> + if (IS_ERR(req)) >>>> + return PTR_ERR(req); >>>> + >>>> + mutex_lock(&parent->d_inode->i_mutex); >>>> + >>>> + req->r_inode = child->d_inode; >>>> + ihold(child->d_inode); >>>> + req->r_ino2 = ceph_vino(parent->d_inode); >>>> + req->r_locked_dir = parent->d_inode; >>>> + req->r_num_caps = 1; >>>> + err = ceph_mdsc_do_request(mdsc, NULL, req); >>>> + >>>> + mutex_unlock(&parent->d_inode->i_mutex); >>>> + >>>> + if (!err) { >>>> + struct ceph_mds_reply_info_parsed *rinfo = &req->r_reply_info; >>>> + memcpy(name, rinfo->dname, rinfo->dname_len); >>>> + name[rinfo->dname_len] = 0; >>>> + dout("get_name %p ino %llx.%llx name %s\n", >>>> + child, ceph_vinop(child->d_inode), name); >>> >>> One other oddity here: the MDS is returning a bunch of metadata about the >>> dentry, including (in most cases) a lease. The client is ignoring all of >>> that and only needs the name itself to feed back into exportfs. I believe >>> at this point that is fine: the client can forget leases at any time and >>> will respond to MDS revocations accordingly. >>> >>> It would probably require a bit of a kludge on the MDS side to make the >>> reply_request() respond with the dentry name but prevent a lease from >>> being issued. Might be worth it though? Maybe a generic "no lease" flag >>> that even regular LOOKUP could use (if for some reason the client didn't >>> want a lease)? >>> >>> This needn't hold up the other patches, but I'm curious what you think >>> about it. >>> >> >> the nfsd does a dentry lookup after it has got the name. the lease can avoid >> sending another 'lookup' request to the MDS. > > For that to work, I think we need to preallocate the dentry and set > req->r_dentry. The ceph_fill_trace() code, in fact, looks like it will > BUG out if it gets a reply with a dentry but the request r_dentry isn't > set (we should probably fix that too). > > In any case, that seems tricky because we don't know the name ahead of > time, so it would need to have its own code path on reply to allocate the > dentry of the proper name. Given that this is only used in rare NFS > re-export corner cases, my inclination is to not bother optimizing? > > sage > > >> >> Regards >> Yan, Zheng >> >>> >>> >>>> + } else { >>>> + dout("get_name %p ino %llx.%llx err %d\n", >>>> + child, ceph_vinop(child->d_inode), err); >>>> + } >>>> + >>>> + ceph_mdsc_put_request(req); >>>> + return err; >>>> +} >>>> + >>>> const struct export_operations ceph_export_ops = { >>>> .encode_fh = ceph_encode_fh, >>>> .fh_to_dentry = ceph_fh_to_dentry, >>>> .fh_to_parent = ceph_fh_to_parent, >>>> .get_parent = ceph_get_parent, >>>> + .get_name = ceph_get_name, >>>> }; >>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c >>>> index 8bf2384..91d6c9d 100644 >>>> --- a/fs/ceph/inode.c >>>> +++ b/fs/ceph/inode.c >>>> @@ -1044,10 +1044,59 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req, >>>> session, req->r_request_started, -1, >>>> &req->r_caps_reservation); >>>> if (err < 0) >>>> - return err; >>>> + goto done; >>>> } else { >>>> WARN_ON_ONCE(1); >>>> } >>>> + >>>> + if (dir && req->r_op == CEPH_MDS_OP_LOOKUPNAME) { >>>> + struct qstr dname; >>>> + struct dentry *dn, *parent; >>>> + >>>> + BUG_ON(!rinfo->head->is_target); >>>> + BUG_ON(req->r_dentry); >>>> + >>>> + parent = d_find_any_alias(dir); >>>> + BUG_ON(!parent); >>>> + >>>> + dname.name = rinfo->dname; >>>> + dname.len = rinfo->dname_len; >>>> + dname.hash = full_name_hash(dname.name, dname.len); >>>> + vino.ino = le64_to_cpu(rinfo->targeti.in->ino); >>>> + vino.snap = le64_to_cpu(rinfo->targeti.in->snapid); >>>> +retry_lookup: >>>> + dn = d_lookup(parent, &dname); >>>> + dout("d_lookup on parent=%p name=%.*s got %p\n", >>>> + parent, dname.len, dname.name, dn); >>>> + >>>> + if (!dn) { >>>> + dn = d_alloc(parent, &dname); >>>> + dout("d_alloc %p '%.*s' = %p\n", parent, >>>> + dname.len, dname.name, dn); >>>> + if (dn == NULL) { >>>> + dput(parent); >>>> + err = -ENOMEM; >>>> + goto done; >>>> + } >>>> + err = ceph_init_dentry(dn); >>>> + if (err < 0) { >>>> + dput(dn); >>>> + dput(parent); >>>> + goto done; >>>> + } >>>> + } else if (dn->d_inode && >>>> + (ceph_ino(dn->d_inode) != vino.ino || >>>> + ceph_snap(dn->d_inode) != vino.snap)) { >>>> + dout(" dn %p points to wrong inode %p\n", >>>> + dn, dn->d_inode); >>>> + d_delete(dn); >>>> + dput(dn); >>>> + goto retry_lookup; >>>> + } >>>> + >>>> + req->r_dentry = dn; req->r_dentry is set here. is there anything I'm missing? Regards Yan, Zheng >>>> + dput(parent); >>>> + } >>>> } >>>> >>>> if (rinfo->head->is_target) { >>>> diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c >>>> index 4440f447..51cc23e 100644 >>>> --- a/fs/ceph/strings.c >>>> +++ b/fs/ceph/strings.c >>>> @@ -54,6 +54,7 @@ const char *ceph_mds_op_name(int op) >>>> case CEPH_MDS_OP_LOOKUPHASH: return "lookuphash"; >>>> case CEPH_MDS_OP_LOOKUPPARENT: return "lookupparent"; >>>> case CEPH_MDS_OP_LOOKUPINO: return "lookupino"; >>>> + case CEPH_MDS_OP_LOOKUPNAME: return "lookupname"; >>>> case CEPH_MDS_OP_GETATTR: return "getattr"; >>>> case CEPH_MDS_OP_SETXATTR: return "setxattr"; >>>> case CEPH_MDS_OP_SETATTR: return "setattr"; >>>> diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h >>>> index 25bfb0e..35f345f 100644 >>>> --- a/include/linux/ceph/ceph_fs.h >>>> +++ b/include/linux/ceph/ceph_fs.h >>>> @@ -332,6 +332,7 @@ enum { >>>> CEPH_MDS_OP_LOOKUPHASH = 0x00102, >>>> CEPH_MDS_OP_LOOKUPPARENT = 0x00103, >>>> CEPH_MDS_OP_LOOKUPINO = 0x00104, >>>> + CEPH_MDS_OP_LOOKUPNAME = 0x00105, >>>> >>>> CEPH_MDS_OP_SETXATTR = 0x01105, >>>> CEPH_MDS_OP_RMXATTR = 0x01106, >>>> -- >>>> 1.8.5.3 >>>> >>>> -- >>>> 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, 10 Mar 2014, Yan, Zheng wrote: > On 03/10/2014 12:12 PM, Sage Weil wrote: > > On Mon, 10 Mar 2014, Yan, Zheng wrote: > >> On 03/10/2014 09:49 AM, Sage Weil wrote: > >>> On Sat, 8 Mar 2014, Yan, Zheng wrote: > >>>> how about the below patch and corresponding mds change in > >>>> https://github.com/ceph/ceph/commit/617ce6761edd7264893f3638c33fd229c71751a0 > >>>> > >>>> Regards > >>>> Yan, Zheng > >>>> > >>>> --- > >>>> >From 0fa1971741b1d3c236ee6fa3a7feb5a74fbd7f2f Mon Sep 17 00:00:00 2001 > >>>> From: "Yan, Zheng" <zheng.z.yan@intel.com> > >>>> Date: Thu, 6 Mar 2014 16:40:32 +0800 > >>>> Subject: [PATCH 1/3] ceph: add get_name() NFS export callback > >>>> > >>>> Use the newly introduced LOOKUPNAME MDS request to connect child > >>>> inode to its parent directory. > >>>> > >>>> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> > >>>> --- > >>>> fs/ceph/export.c | 40 ++++++++++++++++++++++++++++++++++ > >>>> fs/ceph/inode.c | 51 +++++++++++++++++++++++++++++++++++++++++++- > >>>> fs/ceph/strings.c | 1 + > >>>> include/linux/ceph/ceph_fs.h | 1 + > >>>> 4 files changed, 92 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/fs/ceph/export.c b/fs/ceph/export.c > >>>> index 6e611e7..bf36e7f 100644 > >>>> --- a/fs/ceph/export.c > >>>> +++ b/fs/ceph/export.c > >>>> @@ -195,9 +195,49 @@ static struct dentry *ceph_fh_to_parent(struct super_block *sb, > >>>> return dentry; > >>>> } > >>>> > >>>> +static int ceph_get_name(struct dentry *parent, char *name, > >>>> + struct dentry *child) > >>>> +{ > >>>> + struct ceph_mds_client *mdsc; > >>>> + struct ceph_mds_request *req; > >>>> + int err; > >>>> + > >>>> + mdsc = ceph_inode_to_client(child->d_inode)->mdsc; > >>>> + req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_LOOKUPNAME, > >>>> + USE_ANY_MDS); > >>>> + if (IS_ERR(req)) > >>>> + return PTR_ERR(req); > >>>> + > >>>> + mutex_lock(&parent->d_inode->i_mutex); > >>>> + > >>>> + req->r_inode = child->d_inode; > >>>> + ihold(child->d_inode); > >>>> + req->r_ino2 = ceph_vino(parent->d_inode); > >>>> + req->r_locked_dir = parent->d_inode; > >>>> + req->r_num_caps = 1; > >>>> + err = ceph_mdsc_do_request(mdsc, NULL, req); > >>>> + > >>>> + mutex_unlock(&parent->d_inode->i_mutex); > >>>> + > >>>> + if (!err) { > >>>> + struct ceph_mds_reply_info_parsed *rinfo = &req->r_reply_info; > >>>> + memcpy(name, rinfo->dname, rinfo->dname_len); > >>>> + name[rinfo->dname_len] = 0; > >>>> + dout("get_name %p ino %llx.%llx name %s\n", > >>>> + child, ceph_vinop(child->d_inode), name); > >>> > >>> One other oddity here: the MDS is returning a bunch of metadata about the > >>> dentry, including (in most cases) a lease. The client is ignoring all of > >>> that and only needs the name itself to feed back into exportfs. I believe > >>> at this point that is fine: the client can forget leases at any time and > >>> will respond to MDS revocations accordingly. > >>> > >>> It would probably require a bit of a kludge on the MDS side to make the > >>> reply_request() respond with the dentry name but prevent a lease from > >>> being issued. Might be worth it though? Maybe a generic "no lease" flag > >>> that even regular LOOKUP could use (if for some reason the client didn't > >>> want a lease)? > >>> > >>> This needn't hold up the other patches, but I'm curious what you think > >>> about it. > >>> > >> > >> the nfsd does a dentry lookup after it has got the name. the lease can avoid > >> sending another 'lookup' request to the MDS. > > > > For that to work, I think we need to preallocate the dentry and set > > req->r_dentry. The ceph_fill_trace() code, in fact, looks like it will > > BUG out if it gets a reply with a dentry but the request r_dentry isn't > > set (we should probably fix that too). > > > > In any case, that seems tricky because we don't know the name ahead of > > time, so it would need to have its own code path on reply to allocate the > > dentry of the proper name. Given that this is only used in rare NFS > > re-export corner cases, my inclination is to not bother optimizing? > > > > sage > > > > > >> > >> Regards > >> Yan, Zheng > >> > >>> > >>> > >>>> + } else { > >>>> + dout("get_name %p ino %llx.%llx err %d\n", > >>>> + child, ceph_vinop(child->d_inode), err); > >>>> + } > >>>> + > >>>> + ceph_mdsc_put_request(req); > >>>> + return err; > >>>> +} > >>>> + > >>>> const struct export_operations ceph_export_ops = { > >>>> .encode_fh = ceph_encode_fh, > >>>> .fh_to_dentry = ceph_fh_to_dentry, > >>>> .fh_to_parent = ceph_fh_to_parent, > >>>> .get_parent = ceph_get_parent, > >>>> + .get_name = ceph_get_name, > >>>> }; > >>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > >>>> index 8bf2384..91d6c9d 100644 > >>>> --- a/fs/ceph/inode.c > >>>> +++ b/fs/ceph/inode.c > >>>> @@ -1044,10 +1044,59 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req, > >>>> session, req->r_request_started, -1, > >>>> &req->r_caps_reservation); > >>>> if (err < 0) > >>>> - return err; > >>>> + goto done; > >>>> } else { > >>>> WARN_ON_ONCE(1); > >>>> } > >>>> + > >>>> + if (dir && req->r_op == CEPH_MDS_OP_LOOKUPNAME) { > >>>> + struct qstr dname; > >>>> + struct dentry *dn, *parent; > >>>> + > >>>> + BUG_ON(!rinfo->head->is_target); > >>>> + BUG_ON(req->r_dentry); > >>>> + > >>>> + parent = d_find_any_alias(dir); > >>>> + BUG_ON(!parent); > >>>> + > >>>> + dname.name = rinfo->dname; > >>>> + dname.len = rinfo->dname_len; > >>>> + dname.hash = full_name_hash(dname.name, dname.len); > >>>> + vino.ino = le64_to_cpu(rinfo->targeti.in->ino); > >>>> + vino.snap = le64_to_cpu(rinfo->targeti.in->snapid); > >>>> +retry_lookup: > >>>> + dn = d_lookup(parent, &dname); > >>>> + dout("d_lookup on parent=%p name=%.*s got %p\n", > >>>> + parent, dname.len, dname.name, dn); > >>>> + > >>>> + if (!dn) { > >>>> + dn = d_alloc(parent, &dname); > >>>> + dout("d_alloc %p '%.*s' = %p\n", parent, > >>>> + dname.len, dname.name, dn); > >>>> + if (dn == NULL) { > >>>> + dput(parent); > >>>> + err = -ENOMEM; > >>>> + goto done; > >>>> + } > >>>> + err = ceph_init_dentry(dn); > >>>> + if (err < 0) { > >>>> + dput(dn); > >>>> + dput(parent); > >>>> + goto done; > >>>> + } > >>>> + } else if (dn->d_inode && > >>>> + (ceph_ino(dn->d_inode) != vino.ino || > >>>> + ceph_snap(dn->d_inode) != vino.snap)) { > >>>> + dout(" dn %p points to wrong inode %p\n", > >>>> + dn, dn->d_inode); > >>>> + d_delete(dn); > >>>> + dput(dn); > >>>> + goto retry_lookup; > >>>> + } > >>>> + > >>>> + req->r_dentry = dn; > > req->r_dentry is set here. is there anything I'm missing? Oh! Nope, I didn't read the whole patch, sorry! This looks right. Reviewed-by: Sage Weil <sage@inktank.com> sage > > Regards > Yan, Zheng > > >>>> + dput(parent); > >>>> + } > >>>> } > >>>> > >>>> if (rinfo->head->is_target) { > >>>> diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c > >>>> index 4440f447..51cc23e 100644 > >>>> --- a/fs/ceph/strings.c > >>>> +++ b/fs/ceph/strings.c > >>>> @@ -54,6 +54,7 @@ const char *ceph_mds_op_name(int op) > >>>> case CEPH_MDS_OP_LOOKUPHASH: return "lookuphash"; > >>>> case CEPH_MDS_OP_LOOKUPPARENT: return "lookupparent"; > >>>> case CEPH_MDS_OP_LOOKUPINO: return "lookupino"; > >>>> + case CEPH_MDS_OP_LOOKUPNAME: return "lookupname"; > >>>> case CEPH_MDS_OP_GETATTR: return "getattr"; > >>>> case CEPH_MDS_OP_SETXATTR: return "setxattr"; > >>>> case CEPH_MDS_OP_SETATTR: return "setattr"; > >>>> diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h > >>>> index 25bfb0e..35f345f 100644 > >>>> --- a/include/linux/ceph/ceph_fs.h > >>>> +++ b/include/linux/ceph/ceph_fs.h > >>>> @@ -332,6 +332,7 @@ enum { > >>>> CEPH_MDS_OP_LOOKUPHASH = 0x00102, > >>>> CEPH_MDS_OP_LOOKUPPARENT = 0x00103, > >>>> CEPH_MDS_OP_LOOKUPINO = 0x00104, > >>>> + CEPH_MDS_OP_LOOKUPNAME = 0x00105, > >>>> > >>>> CEPH_MDS_OP_SETXATTR = 0x01105, > >>>> CEPH_MDS_OP_RMXATTR = 0x01106, > >>>> -- > >>>> 1.8.5.3 > >>>> > >>>> -- > >>>> 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/export.c b/fs/ceph/export.c index 6e611e7..bf36e7f 100644 --- a/fs/ceph/export.c +++ b/fs/ceph/export.c @@ -195,9 +195,49 @@ static struct dentry *ceph_fh_to_parent(struct super_block *sb, return dentry; } +static int ceph_get_name(struct dentry *parent, char *name, + struct dentry *child) +{ + struct ceph_mds_client *mdsc; + struct ceph_mds_request *req; + int err; + + mdsc = ceph_inode_to_client(child->d_inode)->mdsc; + req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_LOOKUPNAME, + USE_ANY_MDS); + if (IS_ERR(req)) + return PTR_ERR(req); + + mutex_lock(&parent->d_inode->i_mutex); + + req->r_inode = child->d_inode; + ihold(child->d_inode); + req->r_ino2 = ceph_vino(parent->d_inode); + req->r_locked_dir = parent->d_inode; + req->r_num_caps = 1; + err = ceph_mdsc_do_request(mdsc, NULL, req); + + mutex_unlock(&parent->d_inode->i_mutex); + + if (!err) { + struct ceph_mds_reply_info_parsed *rinfo = &req->r_reply_info; + memcpy(name, rinfo->dname, rinfo->dname_len); + name[rinfo->dname_len] = 0; + dout("get_name %p ino %llx.%llx name %s\n", + child, ceph_vinop(child->d_inode), name); + } else { + dout("get_name %p ino %llx.%llx err %d\n", + child, ceph_vinop(child->d_inode), err); + } + + ceph_mdsc_put_request(req); + return err; +} + const struct export_operations ceph_export_ops = { .encode_fh = ceph_encode_fh, .fh_to_dentry = ceph_fh_to_dentry, .fh_to_parent = ceph_fh_to_parent, .get_parent = ceph_get_parent, + .get_name = ceph_get_name, }; diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index 8bf2384..91d6c9d 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -1044,10 +1044,59 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req, session, req->r_request_started, -1, &req->r_caps_reservation); if (err < 0) - return err; + goto done; } else { WARN_ON_ONCE(1); } + + if (dir && req->r_op == CEPH_MDS_OP_LOOKUPNAME) { + struct qstr dname; + struct dentry *dn, *parent; + + BUG_ON(!rinfo->head->is_target); + BUG_ON(req->r_dentry); + + parent = d_find_any_alias(dir); + BUG_ON(!parent); + + dname.name = rinfo->dname; + dname.len = rinfo->dname_len; + dname.hash = full_name_hash(dname.name, dname.len); + vino.ino = le64_to_cpu(rinfo->targeti.in->ino); + vino.snap = le64_to_cpu(rinfo->targeti.in->snapid); +retry_lookup: + dn = d_lookup(parent, &dname); + dout("d_lookup on parent=%p name=%.*s got %p\n", + parent, dname.len, dname.name, dn); + + if (!dn) { + dn = d_alloc(parent, &dname); + dout("d_alloc %p '%.*s' = %p\n", parent, + dname.len, dname.name, dn); + if (dn == NULL) { + dput(parent); + err = -ENOMEM; + goto done; + } + err = ceph_init_dentry(dn); + if (err < 0) { + dput(dn); + dput(parent); + goto done; + } + } else if (dn->d_inode && + (ceph_ino(dn->d_inode) != vino.ino || + ceph_snap(dn->d_inode) != vino.snap)) { + dout(" dn %p points to wrong inode %p\n", + dn, dn->d_inode); + d_delete(dn); + dput(dn); + goto retry_lookup; + } + + req->r_dentry = dn; + dput(parent); + } } if (rinfo->head->is_target) { diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c index 4440f447..51cc23e 100644 --- a/fs/ceph/strings.c +++ b/fs/ceph/strings.c @@ -54,6 +54,7 @@ const char *ceph_mds_op_name(int op) case CEPH_MDS_OP_LOOKUPHASH: return "lookuphash"; case CEPH_MDS_OP_LOOKUPPARENT: return "lookupparent"; case CEPH_MDS_OP_LOOKUPINO: return "lookupino"; + case CEPH_MDS_OP_LOOKUPNAME: return "lookupname"; case CEPH_MDS_OP_GETATTR: return "getattr"; case CEPH_MDS_OP_SETXATTR: return "setxattr"; case CEPH_MDS_OP_SETATTR: return "setattr"; diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h index 25bfb0e..35f345f 100644 --- a/include/linux/ceph/ceph_fs.h +++ b/include/linux/ceph/ceph_fs.h @@ -332,6 +332,7 @@ enum { CEPH_MDS_OP_LOOKUPHASH = 0x00102, CEPH_MDS_OP_LOOKUPPARENT = 0x00103, CEPH_MDS_OP_LOOKUPINO = 0x00104, + CEPH_MDS_OP_LOOKUPNAME = 0x00105, CEPH_MDS_OP_SETXATTR = 0x01105, CEPH_MDS_OP_RMXATTR = 0x01106,