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@xxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >> > >> > > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html