| Message ID | 1393687411-2457-2-git-send-email-zheng.z.yan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 1 Mar 2014, Yan, Zheng wrote: > ceph_fh_to_parent() finds the inode that corresponds to the 'ino' field > of struct ceph_nfs_confh. This is wrong, it should find the inode that > corresponds to the 'parent_ino' field. > > Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> It's been a while since I've looked at this, but I'm a bit confused. If we have a get_parent() operation that will reliably get the parent directory for an inode, why do we want/need the ceph_nfs_confh? If we *do* have that struct and look up the parent_ino, we have no guarantee that it is still the parent if there has been an intervening rename. Would it be better to always use ceph_nfs_fh, and rely on LOOKUPPARENT instead? Thanks- sage > --- > fs/ceph/export.c | 38 +++++--------------------------------- > 1 file changed, 5 insertions(+), 33 deletions(-) > > diff --git a/fs/ceph/export.c b/fs/ceph/export.c > index 905d7f2..017af26 100644 > --- a/fs/ceph/export.c > +++ b/fs/ceph/export.c > @@ -122,49 +122,21 @@ static struct dentry *ceph_fh_to_dentry(struct super_block *sb, > } > > /* > - * get parent, if possible. > - * > - * FIXME: we could do better by querying the mds to discover the > - * parent. > + * convert regular fh to parent > */ > static struct dentry *ceph_fh_to_parent(struct super_block *sb, > - struct fid *fid, > + struct fid *fid, > int fh_len, int fh_type) > { > struct ceph_nfs_confh *cfh = (void *)fid->raw; > - struct ceph_vino vino; > - struct inode *inode; > - struct dentry *dentry; > - int err; > > - if (fh_type == 1) > + if (fh_type != FILEID_INO32_GEN_PARENT) > return ERR_PTR(-ESTALE); > if (fh_len < sizeof(*cfh) / 4) > return ERR_PTR(-ESTALE); > > - pr_debug("fh_to_parent %llx/%d\n", cfh->parent_ino, > - cfh->parent_name_hash); > - > - vino.ino = cfh->ino; > - vino.snap = CEPH_NOSNAP; > - inode = ceph_find_inode(sb, vino); > - if (!inode) > - return ERR_PTR(-ESTALE); > - > - dentry = d_obtain_alias(inode); > - if (IS_ERR(dentry)) { > - pr_err("fh_to_parent %llx -- inode %p but ENOMEM\n", > - cfh->ino, inode); > - iput(inode); > - return dentry; > - } > - err = ceph_init_dentry(dentry); > - if (err < 0) { > - iput(inode); > - return ERR_PTR(err); > - } > - dout("fh_to_parent %llx %p dentry %p\n", cfh->ino, inode, dentry); > - return dentry; > + pr_debug("fh_to_parent %llx\n", cfh->parent_ino); > + return __fh_to_dentry(sb, cfh->parent_ino); > } > > const struct export_operations ceph_export_ops = { > -- > 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/06/2014 01:17 AM, Sage Weil wrote: > On Sat, 1 Mar 2014, Yan, Zheng wrote: >> ceph_fh_to_parent() finds the inode that corresponds to the 'ino' field >> of struct ceph_nfs_confh. This is wrong, it should find the inode that >> corresponds to the 'parent_ino' field. >> >> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> > > It's been a while since I've looked at this, but I'm a bit confused. > > If we have a get_parent() operation that will reliably get the parent > directory for an inode, why do we want/need the ceph_nfs_confh? If we > *do* have that struct and look up the parent_ino, we have no guarantee > that it is still the parent if there has been an intervening rename. > Would it be better to always use ceph_nfs_fh, and rely on LOOKUPPARENT > instead? > fh_to_parent() is used when reconnecting non-directory inode. (get_parent() is used when reconnecting directory inode). The problem of using LOOKUPPARENT is that the inode may have multiple links. if the primary link of the inode is in stray directory, LOOKUPPARENT return -ESTALE. > Thanks- > sage > > >> --- >> fs/ceph/export.c | 38 +++++--------------------------------- >> 1 file changed, 5 insertions(+), 33 deletions(-) >> >> diff --git a/fs/ceph/export.c b/fs/ceph/export.c >> index 905d7f2..017af26 100644 >> --- a/fs/ceph/export.c >> +++ b/fs/ceph/export.c >> @@ -122,49 +122,21 @@ static struct dentry *ceph_fh_to_dentry(struct super_block *sb, >> } >> >> /* >> - * get parent, if possible. >> - * >> - * FIXME: we could do better by querying the mds to discover the >> - * parent. >> + * convert regular fh to parent >> */ >> static struct dentry *ceph_fh_to_parent(struct super_block *sb, >> - struct fid *fid, >> + struct fid *fid, >> int fh_len, int fh_type) >> { >> struct ceph_nfs_confh *cfh = (void *)fid->raw; >> - struct ceph_vino vino; >> - struct inode *inode; >> - struct dentry *dentry; >> - int err; >> >> - if (fh_type == 1) >> + if (fh_type != FILEID_INO32_GEN_PARENT) >> return ERR_PTR(-ESTALE); >> if (fh_len < sizeof(*cfh) / 4) >> return ERR_PTR(-ESTALE); >> >> - pr_debug("fh_to_parent %llx/%d\n", cfh->parent_ino, >> - cfh->parent_name_hash); >> - >> - vino.ino = cfh->ino; >> - vino.snap = CEPH_NOSNAP; >> - inode = ceph_find_inode(sb, vino); >> - if (!inode) >> - return ERR_PTR(-ESTALE); >> - >> - dentry = d_obtain_alias(inode); >> - if (IS_ERR(dentry)) { >> - pr_err("fh_to_parent %llx -- inode %p but ENOMEM\n", >> - cfh->ino, inode); >> - iput(inode); >> - return dentry; >> - } >> - err = ceph_init_dentry(dentry); >> - if (err < 0) { >> - iput(inode); >> - return ERR_PTR(err); >> - } >> - dout("fh_to_parent %llx %p dentry %p\n", cfh->ino, inode, dentry); >> - return dentry; >> + pr_debug("fh_to_parent %llx\n", cfh->parent_ino); >> + return __fh_to_dentry(sb, cfh->parent_ino); >> } >> >> const struct export_operations ceph_export_ops = { >> -- >> 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 Thu, 6 Mar 2014, Yan, Zheng wrote: > On 03/06/2014 01:17 AM, Sage Weil wrote: > > On Sat, 1 Mar 2014, Yan, Zheng wrote: > >> ceph_fh_to_parent() finds the inode that corresponds to the 'ino' field > >> of struct ceph_nfs_confh. This is wrong, it should find the inode that > >> corresponds to the 'parent_ino' field. > >> > >> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> > > > > It's been a while since I've looked at this, but I'm a bit confused. > > > > If we have a get_parent() operation that will reliably get the parent > > directory for an inode, why do we want/need the ceph_nfs_confh? If we > > *do* have that struct and look up the parent_ino, we have no guarantee > > that it is still the parent if there has been an intervening rename. > > Would it be better to always use ceph_nfs_fh, and rely on LOOKUPPARENT > > instead? > > > > fh_to_parent() is used when reconnecting non-directory inode. (get_parent() > is used when reconnecting directory inode). The problem of using LOOKUPPARENT > is that the inode may have multiple links. if the primary link of the inode is > in stray directory, LOOKUPPARENT return -ESTALE. Ah, that's true. On the other hand, though, if the file has been renamed then we will return the wrong parent. Is the idea that that is okay? I suppose nfsd will repeat the lookup at some point if it needs to revalidate that dentry? sage > > > Thanks- > > sage > > > > > >> --- > >> fs/ceph/export.c | 38 +++++--------------------------------- > >> 1 file changed, 5 insertions(+), 33 deletions(-) > >> > >> diff --git a/fs/ceph/export.c b/fs/ceph/export.c > >> index 905d7f2..017af26 100644 > >> --- a/fs/ceph/export.c > >> +++ b/fs/ceph/export.c > >> @@ -122,49 +122,21 @@ static struct dentry *ceph_fh_to_dentry(struct super_block *sb, > >> } > >> > >> /* > >> - * get parent, if possible. > >> - * > >> - * FIXME: we could do better by querying the mds to discover the > >> - * parent. > >> + * convert regular fh to parent > >> */ > >> static struct dentry *ceph_fh_to_parent(struct super_block *sb, > >> - struct fid *fid, > >> + struct fid *fid, > >> int fh_len, int fh_type) > >> { > >> struct ceph_nfs_confh *cfh = (void *)fid->raw; > >> - struct ceph_vino vino; > >> - struct inode *inode; > >> - struct dentry *dentry; > >> - int err; > >> > >> - if (fh_type == 1) > >> + if (fh_type != FILEID_INO32_GEN_PARENT) > >> return ERR_PTR(-ESTALE); > >> if (fh_len < sizeof(*cfh) / 4) > >> return ERR_PTR(-ESTALE); > >> > >> - pr_debug("fh_to_parent %llx/%d\n", cfh->parent_ino, > >> - cfh->parent_name_hash); > >> - > >> - vino.ino = cfh->ino; > >> - vino.snap = CEPH_NOSNAP; > >> - inode = ceph_find_inode(sb, vino); > >> - if (!inode) > >> - return ERR_PTR(-ESTALE); > >> - > >> - dentry = d_obtain_alias(inode); > >> - if (IS_ERR(dentry)) { > >> - pr_err("fh_to_parent %llx -- inode %p but ENOMEM\n", > >> - cfh->ino, inode); > >> - iput(inode); > >> - return dentry; > >> - } > >> - err = ceph_init_dentry(dentry); > >> - if (err < 0) { > >> - iput(inode); > >> - return ERR_PTR(err); > >> - } > >> - dout("fh_to_parent %llx %p dentry %p\n", cfh->ino, inode, dentry); > >> - return dentry; > >> + pr_debug("fh_to_parent %llx\n", cfh->parent_ino); > >> + return __fh_to_dentry(sb, cfh->parent_ino); > >> } > >> > >> const struct export_operations ceph_export_ops = { > >> -- > >> 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 > > -- 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 905d7f2..017af26 100644 --- a/fs/ceph/export.c +++ b/fs/ceph/export.c @@ -122,49 +122,21 @@ static struct dentry *ceph_fh_to_dentry(struct super_block *sb, } /* - * get parent, if possible. - * - * FIXME: we could do better by querying the mds to discover the - * parent. + * convert regular fh to parent */ static struct dentry *ceph_fh_to_parent(struct super_block *sb, - struct fid *fid, + struct fid *fid, int fh_len, int fh_type) { struct ceph_nfs_confh *cfh = (void *)fid->raw; - struct ceph_vino vino; - struct inode *inode; - struct dentry *dentry; - int err; - if (fh_type == 1) + if (fh_type != FILEID_INO32_GEN_PARENT) return ERR_PTR(-ESTALE); if (fh_len < sizeof(*cfh) / 4) return ERR_PTR(-ESTALE); - pr_debug("fh_to_parent %llx/%d\n", cfh->parent_ino, - cfh->parent_name_hash); - - vino.ino = cfh->ino; - vino.snap = CEPH_NOSNAP; - inode = ceph_find_inode(sb, vino); - if (!inode) - return ERR_PTR(-ESTALE); - - dentry = d_obtain_alias(inode); - if (IS_ERR(dentry)) { - pr_err("fh_to_parent %llx -- inode %p but ENOMEM\n", - cfh->ino, inode); - iput(inode); - return dentry; - } - err = ceph_init_dentry(dentry); - if (err < 0) { - iput(inode); - return ERR_PTR(err); - } - dout("fh_to_parent %llx %p dentry %p\n", cfh->ino, inode, dentry); - return dentry; + pr_debug("fh_to_parent %llx\n", cfh->parent_ino); + return __fh_to_dentry(sb, cfh->parent_ino); } const struct export_operations ceph_export_ops = {
ceph_fh_to_parent() finds the inode that corresponds to the 'ino' field of struct ceph_nfs_confh. This is wrong, it should find the inode that corresponds to the 'parent_ino' field. Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> --- fs/ceph/export.c | 38 +++++--------------------------------- 1 file changed, 5 insertions(+), 33 deletions(-)