[PATCH v2 0/5] ceph: fixes for nfs export

[PATCH v2 0/5] ceph: fixes for nfs export

[Yan, Zheng]

Changes since v1:
 make ceph_fh_to_parent() use LOOKUPPARENT to find parent dentry.
 if LOOKUPPARENT fails, retry finding the dentry that corresponds
 to the 'parent_ino'

[PATCH v2 1/5] ceph: simplify ceph_fh_to_dentry()

[Yan, Zheng]

MDS handles LOOKUPHASH and LOOKUPINO MDS requests in the same way.
So __cfh_to_dentry() is redundant.

Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
 fs/ceph/export.c | 167 +++++++++++--------------------------------------------
 1 file changed, 32 insertions(+), 135 deletions(-)

diff --git a/fs/ceph/export.c b/fs/ceph/export.c
index 16796be..976d341 100644
--- a/fs/ceph/export.c
+++ b/fs/ceph/export.c
@@ -8,23 +8,6 @@
 #include "mds_client.h"
 
 /*
- * NFS export support
- *
- * NFS re-export of a ceph mount is, at present, only semireliable.
- * The basic issue is that the Ceph architectures doesn't lend itself
- * well to generating filehandles that will remain valid forever.
- *
- * So, we do our best.  If you're lucky, your inode will be in the
- * client's cache.  If it's not, and you have a connectable fh, then
- * the MDS server may be able to find it for you.  Otherwise, you get
- * ESTALE.
- *
- * There are ways to this more reliable, but in the non-connectable fh
- * case, we won't every work perfectly, and in the connectable case,
- * some changes are needed on the MDS side to work better.
- */
-
-/*
  * Basic fh
  */
 struct ceph_nfs_fh {
@@ -32,22 +15,12 @@ struct ceph_nfs_fh {
 } __attribute__ ((packed));
 
 /*
- * Larger 'connectable' fh that includes parent ino and name hash.
- * Use this whenever possible, as it works more reliably.
+ * Larger fh that includes parent ino.
  */
 struct ceph_nfs_confh {
 	u64 ino, parent_ino;
-	u32 parent_name_hash;
 } __attribute__ ((packed));
 
-/*
- * The presence of @parent_inode here tells us whether NFS wants a
- * connectable file handle.  However, we want to make a connectionable
- * file handle unconditionally so that the MDS gets as much of a hint
- * as possible.  That means we only use @parent_dentry to indicate
- * whether nfsd wants a connectable fh, and whether we should indicate
- * failure from a too-small @max_len.
- */
 static int ceph_encode_fh(struct inode *inode, u32 *rawfh, int *max_len,
 			  struct inode *parent_inode)
 {
@@ -56,54 +29,36 @@ static int ceph_encode_fh(struct inode *inode, u32 *rawfh, int *max_len,
 	struct ceph_nfs_confh *cfh = (void *)rawfh;
 	int connected_handle_length = sizeof(*cfh)/4;
 	int handle_length = sizeof(*fh)/4;
-	struct dentry *dentry;
-	struct dentry *parent;
 
 	/* don't re-export snaps */
 	if (ceph_snap(inode) != CEPH_NOSNAP)
 		return -EINVAL;
 
-	dentry = d_find_alias(inode);
+	if (parent_inode && (*max_len < connected_handle_length)) {
+		*max_len = connected_handle_length;
+		return FILEID_INVALID;
+	} else if (*max_len < handle_length) {
+		*max_len = handle_length;
+		return FILEID_INVALID;
+	}
 
-	/* if we found an alias, generate a connectable fh */
-	if (*max_len >= connected_handle_length && dentry) {
-		dout("encode_fh %p connectable\n", dentry);
-		spin_lock(&dentry->d_lock);
-		parent = dentry->d_parent;
+	if (parent_inode) {
+		dout("encode_fh %llx with parent %llx\n",
+		     ceph_ino(inode), ceph_ino(parent_inode));
 		cfh->ino = ceph_ino(inode);
-		cfh->parent_ino = ceph_ino(parent->d_inode);
-		cfh->parent_name_hash = ceph_dentry_hash(parent->d_inode,
-							 dentry);
+		cfh->parent_ino = ceph_ino(parent_inode);
 		*max_len = connected_handle_length;
-		type = 2;
-		spin_unlock(&dentry->d_lock);
-	} else if (*max_len >= handle_length) {
-		if (parent_inode) {
-			/* nfsd wants connectable */
-			*max_len = connected_handle_length;
-			type = FILEID_INVALID;
-		} else {
-			dout("encode_fh %p\n", dentry);
-			fh->ino = ceph_ino(inode);
-			*max_len = handle_length;
-			type = 1;
-		}
+		type = FILEID_INO32_GEN_PARENT;
 	} else {
+		dout("encode_fh %llx\n", ceph_ino(inode));
+		fh->ino = ceph_ino(inode);
 		*max_len = handle_length;
-		type = FILEID_INVALID;
+		type = FILEID_INO32_GEN;
 	}
-	if (dentry)
-		dput(dentry);
 	return type;
 }
 
-/*
- * convert regular fh to dentry
- *
- * FIXME: we should try harder by querying the mds for the ino.
- */
-static struct dentry *__fh_to_dentry(struct super_block *sb,
-				     struct ceph_nfs_fh *fh, int fh_len)
+static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino)
 {
 	struct ceph_mds_client *mdsc = ceph_sb_to_client(sb)->mdsc;
 	struct inode *inode;
@@ -111,11 +66,7 @@ static struct dentry *__fh_to_dentry(struct super_block *sb,
 	struct ceph_vino vino;
 	int err;
 
-	if (fh_len < sizeof(*fh) / 4)
-		return ERR_PTR(-ESTALE);
-
-	dout("__fh_to_dentry %llx\n", fh->ino);
-	vino.ino = fh->ino;
+	vino.ino = ino;
 	vino.snap = CEPH_NOSNAP;
 	inode = ceph_find_inode(sb, vino);
 	if (!inode) {
@@ -139,89 +90,35 @@ static struct dentry *__fh_to_dentry(struct super_block *sb,
 
 	dentry = d_obtain_alias(inode);
 	if (IS_ERR(dentry)) {
-		pr_err("fh_to_dentry %llx -- inode %p but ENOMEM\n",
-		       fh->ino, inode);
 		iput(inode);
 		return dentry;
 	}
 	err = ceph_init_dentry(dentry);
 	if (err < 0) {
-		iput(inode);
+		dput(dentry);
 		return ERR_PTR(err);
 	}
-	dout("__fh_to_dentry %llx %p dentry %p\n", fh->ino, inode, dentry);
+	dout("__fh_to_dentry %llx %p dentry %p\n", ino, inode, dentry);
 	return dentry;
 }
 
 /*
- * convert connectable fh to dentry
+ * convert regular fh to dentry
  */
-static struct dentry *__cfh_to_dentry(struct super_block *sb,
-				      struct ceph_nfs_confh *cfh, int fh_len)
+static struct dentry *ceph_fh_to_dentry(struct super_block *sb,
+					struct fid *fid,
+					int fh_len, int fh_type)
 {
-	struct ceph_mds_client *mdsc = ceph_sb_to_client(sb)->mdsc;
-	struct inode *inode;
-	struct dentry *dentry;
-	struct ceph_vino vino;
-	int err;
-
-	if (fh_len < sizeof(*cfh) / 4)
-		return ERR_PTR(-ESTALE);
-
-	dout("__cfh_to_dentry %llx (%llx/%x)\n",
-	     cfh->ino, cfh->parent_ino, cfh->parent_name_hash);
-
-	vino.ino = cfh->ino;
-	vino.snap = CEPH_NOSNAP;
-	inode = ceph_find_inode(sb, vino);
-	if (!inode) {
-		struct ceph_mds_request *req;
-
-		req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_LOOKUPHASH,
-					       USE_ANY_MDS);
-		if (IS_ERR(req))
-			return ERR_CAST(req);
-
-		req->r_ino1 = vino;
-		req->r_ino2.ino = cfh->parent_ino;
-		req->r_ino2.snap = CEPH_NOSNAP;
-		req->r_path2 = kmalloc(16, GFP_NOFS);
-		snprintf(req->r_path2, 16, "%d", cfh->parent_name_hash);
-		req->r_num_caps = 1;
-		err = ceph_mdsc_do_request(mdsc, NULL, req);
-		inode = req->r_target_inode;
-		if (inode)
-			ihold(inode);
-		ceph_mdsc_put_request(req);
-		if (!inode)
-			return ERR_PTR(err ? err : -ESTALE);
-	}
+	struct ceph_nfs_fh *fh = (void *)fid->raw;
 
-	dentry = d_obtain_alias(inode);
-	if (IS_ERR(dentry)) {
-		pr_err("cfh_to_dentry %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("__cfh_to_dentry %llx %p dentry %p\n", cfh->ino, inode, dentry);
-	return dentry;
-}
+	if (fh_type != FILEID_INO32_GEN  &&
+	    fh_type != FILEID_INO32_GEN_PARENT)
+		return NULL;
+	if (fh_len < sizeof(*fh) / 4)
+		return NULL;
 
-static struct dentry *ceph_fh_to_dentry(struct super_block *sb, struct fid *fid,
-					int fh_len, int fh_type)
-{
-	if (fh_type == 1)
-		return __fh_to_dentry(sb, (struct ceph_nfs_fh *)fid->raw,
-								fh_len);
-	else
-		return __cfh_to_dentry(sb, (struct ceph_nfs_confh *)fid->raw,
-								fh_len);
+	dout("fh_to_dentry %llx\n", fh->ino);
+	return __fh_to_dentry(sb, fh->ino);
 }
 
 /*
-- 
1.8.5.3

[PATCH v2 2/5] ceph: add get_parent() NFS export callback

[Yan, Zheng]

The callback uses LOOKUPPARENT MDS request to find parent.

Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
 fs/ceph/export.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/fs/ceph/export.c b/fs/ceph/export.c
index 976d341..3794255 100644
--- a/fs/ceph/export.c
+++ b/fs/ceph/export.c
@@ -121,6 +121,58 @@ static struct dentry *ceph_fh_to_dentry(struct super_block *sb,
 	return __fh_to_dentry(sb, fh->ino);
 }
 
+static struct dentry *__get_parent(struct super_block *sb, u64 ino)
+{
+	struct ceph_mds_client *mdsc = ceph_sb_to_client(sb)->mdsc;
+	struct ceph_mds_request *req;
+	struct inode *inode;
+	struct dentry *dentry;
+	int err;
+
+	req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_LOOKUPPARENT,
+				       USE_ANY_MDS);
+	if (IS_ERR(req))
+		return ERR_CAST(req);
+
+	req->r_ino1 = (struct ceph_vino) {
+		.ino = ino,
+		.snap = CEPH_NOSNAP,
+	};
+	req->r_num_caps = 1;
+	err = ceph_mdsc_do_request(mdsc, NULL, req);
+	inode = req->r_target_inode;
+	if (inode)
+		ihold(inode);
+	ceph_mdsc_put_request(req);
+	if (!inode)
+		return ERR_PTR(-ENOENT);
+
+	dentry = d_obtain_alias(inode);
+	if (IS_ERR(dentry)) {
+		iput(inode);
+		return dentry;
+	}
+	err = ceph_init_dentry(dentry);
+	if (err < 0) {
+		dput(dentry);
+		return ERR_PTR(err);
+	}
+	dout("__get_parent ino %llx parent %p ino %llx.%llx\n",
+	     ino, dentry, ceph_vinop(inode));
+	return dentry;
+}
+
+struct dentry *ceph_get_parent(struct dentry *child)
+{
+	/* don't re-export snaps */
+	if (ceph_snap(child->d_inode) != CEPH_NOSNAP)
+		return ERR_PTR(-EINVAL);
+
+	dout("get_parent %p ino %llx.%llx\n",
+	     child, ceph_vinop(child->d_inode));
+	return __get_parent(child->d_sb, ceph_ino(child->d_inode));
+}
+
 /*
  * get parent, if possible.
  *
@@ -171,4 +223,5 @@ 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,
 };
-- 
1.8.5.3

[PATCH v2 3/5] ceph: fix ceph_fh_to_parent()

[Yan, Zheng]

ceph_fh_to_parent() returns dentry that corresponds to the 'ino' field
of struct ceph_nfs_confh. This is wrong, it should return dentry that
corresponds to the 'parent_ino' field.

Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
 fs/ceph/export.c | 42 +++++++++---------------------------------
 1 file changed, 9 insertions(+), 33 deletions(-)

diff --git a/fs/ceph/export.c b/fs/ceph/export.c
index 3794255..6e611e7 100644
--- a/fs/ceph/export.c
+++ b/fs/ceph/export.c
@@ -174,48 +174,24 @@ struct dentry *ceph_get_parent(struct dentry *child)
 }
 
 /*
- * 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)
-		return ERR_PTR(-ESTALE);
+	if (fh_type != FILEID_INO32_GEN_PARENT)
+		return NULL;
 	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);
+		return NULL;
 
-	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);
+	dout("fh_to_parent %llx\n", cfh->parent_ino);
+	dentry = __get_parent(sb, cfh->ino);
+	if (IS_ERR(dentry) && PTR_ERR(dentry) == -ENOENT)
+		dentry = __fh_to_dentry(sb, cfh->parent_ino);
 	return dentry;
 }
 
-- 
1.8.5.3

[PATCH v2 4/5] ceph: add get_name() NFS export callback

[Yan, Zheng]

Set WANT_DENTRY flag for the LOOKUPINO MDS request. The flag makes
MDS reply contain dentry trace.

Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
 fs/ceph/export.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 fs/ceph/inode.c  | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/export.c b/fs/ceph/export.c
index 6e611e7..d0722a1 100644
--- a/fs/ceph/export.c
+++ b/fs/ceph/export.c
@@ -195,9 +195,54 @@ 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_LOOKUPINO,
+				       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;
+		if (rinfo->head->is_dentry) {
+			memcpy(name, rinfo->dname, rinfo->dname_len);
+			name[rinfo->dname_len] = 0;
+		} else {
+			err = -EOPNOTSUPP;
+		}
+	}
+	ceph_mdsc_put_request(req);
+
+	if (!err)
+		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);
+	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..ff3ee3e 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_LOOKUPINO) {
+			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) {
-- 
1.8.5.3

[PATCH v2 5/5] ceph: print inode number for LOOKUPINO request

[Yan, Zheng]

Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
 fs/ceph/debugfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
index 8c6f313..16b54aa 100644
--- a/fs/ceph/debugfs.c
+++ b/fs/ceph/debugfs.c
@@ -93,6 +93,8 @@ static int mdsc_show(struct seq_file *s, void *p)
 		} else if (req->r_path1) {
 			seq_printf(s, " #%llx/%s", req->r_ino1.ino,
 				   req->r_path1);
+		} else {
+			seq_printf(s, " #%llx", req->r_ino1.ino);
 		}
 
 		if (req->r_old_dentry) {
-- 
1.8.5.3

Re: [PATCH v2 0/5] ceph: fixes for nfs export

[Sage Weil]

On Fri, 7 Mar 2014, Yan, Zheng wrote:
> Changes since v1:
>  make ceph_fh_to_parent() use LOOKUPPARENT to find parent dentry.
>  if LOOKUPPARENT fails, retry finding the dentry that corresponds
>  to the 'parent_ino'

Okay, all of these changes finally make sense to me now (Thanks, Zheng and 
Christoph).

My only concern now is that the way that get_name is using LOOKUPINO feels 
like it is overloading the existing operation in an awkward way.  Normally 
LOOKUPINO is used when we have an ino but no reference/cap, and returns 
just the cap for the ino.  LOOKUPPARENT works similarly now where it 
returns just a cap for the parent directory but no connecting dentry; that 
makes sense now too.

In the case of get_name, though, if I am following, we have both a cap on 
the directory and the child inode and just need the connecting dentry.  It 
seems like we are better off with a new MDS op that does exactly that 
instead of making LOOKUPINO's behavior change wildly due to a flag (as in

https://github.com/ceph/ceph/commit/3cb6168b0285acfa39c1895bc4dcf6fee408ae4b#diff-eecf840e09d7481f7b8f415789852c7dR2386

).  We should make the client get_name handle EOPNOTSUPP by translating 
to ESTALE, probably, in case the MDS is old.

This a bit more work, but I think it'll keep the semantics for all of 
these ops cleaner.  What do you think?

(And sorry I'm still catching up here!  This would have been more useful 
feedback 2 days ago.)

sage

Re: [PATCH v2 0/5] ceph: fixes for nfs export

[Yan, Zheng]

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);
+	} 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

Re: [PATCH v2 0/5] ceph: fixes for nfs export

[Sage Weil]

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
> 
> 

Re: [PATCH v2 0/5] ceph: fixes for nfs export

[Sage Weil]

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
> 
> 

Re: [PATCH v2 0/5] ceph: fixes for nfs export

[Yan, Zheng]

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
>>
>>

Re: [PATCH v2 0/5] ceph: fixes for nfs export

[Yan, Zheng]

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
>>
>>

Re: [PATCH v2 0/5] ceph: fixes for nfs export

[Sage Weil]

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
> >>
> >>
> 
> 

Re: [PATCH v2 0/5] ceph: fixes for nfs export

[Yan, Zheng]

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
>>>>
>>>>
>>
>>

Re: [PATCH v2 0/5] ceph: fixes for nfs export

[Sage Weil]

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
> >>>>
> >>>>
> >>
> >>
> 
>