[PATCH 0/4] ceph: allow ceph_d_revalidate to handle some cases under LOOKUP_RCU

[PATCH 0/4] ceph: allow ceph_d_revalidate to handle some cases under LOOKUP_RCU

[Jeff Layton]

The basic idea with this set is to allow the kernel client to allow the
client to do more d_revalidate functionality under the rcu_read_lock.
The idea is basically to do what we can there while not blocking, and
then return -ECHILD for the rest.

The only really questionable bit is the part where we need to take the
d_lock in ceph_d_release. I think it should be largely uncontended, but
it's still less than ideal. Maybe there is some way to handle that
locklessly?

Also, it would be nice to not have to drop out of rcuwalk mode in order
to run the lease renewal. It seems like the kind of thing that would be
ideal to queue to a workqueue, but I don't see a way to do that without
introducing a potential deadlock.

My teuthology run with it passed all but one test, which seems to have
been an unrelated problem (though I could be wrong there).

Jeff Layton (4):
  ceph: remove ceph_mdsc_lease_release
  ceph: clear d_fsinfo pointer under d_lock
  ceph: allow dentry_lease_is_valid to work under RCU walk
  ceph: handle LOOKUP_RCU in ceph_d_revalidate

 fs/ceph/dir.c        | 67 +++++++++++++++++++++++++++++++++++-----------------
 fs/ceph/mds_client.c | 41 --------------------------------
 fs/ceph/mds_client.h |  4 ----
 3 files changed, 45 insertions(+), 67 deletions(-)

-- 
2.5.5

[PATCH 1/4] ceph: remove ceph_mdsc_lease_release

[Jeff Layton]

Nothing calls it.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ceph/mds_client.c | 41 -----------------------------------------
 fs/ceph/mds_client.h |  4 ----
 2 files changed, 45 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 0d4bb24c670a..78a3495a11be 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -3292,47 +3292,6 @@ void ceph_mdsc_lease_send_msg(struct ceph_mds_session *session,
 }
 
 /*
- * Preemptively release a lease we expect to invalidate anyway.
- * Pass @inode always, @dentry is optional.
- */
-void ceph_mdsc_lease_release(struct ceph_mds_client *mdsc, struct inode *inode,
-			     struct dentry *dentry)
-{
-	struct ceph_dentry_info *di;
-	struct ceph_mds_session *session;
-	u32 seq;
-
-	BUG_ON(inode == NULL);
-	BUG_ON(dentry == NULL);
-
-	/* is dentry lease valid? */
-	spin_lock(&dentry->d_lock);
-	di = ceph_dentry(dentry);
-	if (!di || !di->lease_session ||
-	    di->lease_session->s_mds < 0 ||
-	    di->lease_gen != di->lease_session->s_cap_gen ||
-	    !time_before(jiffies, di->time)) {
-		dout("lease_release inode %p dentry %p -- "
-		     "no lease\n",
-		     inode, dentry);
-		spin_unlock(&dentry->d_lock);
-		return;
-	}
-
-	/* we do have a lease on this dentry; note mds and seq */
-	session = ceph_get_mds_session(di->lease_session);
-	seq = di->lease_seq;
-	__ceph_mdsc_drop_dentry_lease(dentry);
-	spin_unlock(&dentry->d_lock);
-
-	dout("lease_release inode %p dentry %p to mds%d\n",
-	     inode, dentry, session->s_mds);
-	ceph_mdsc_lease_send_msg(session, inode, dentry,
-				 CEPH_MDS_LEASE_RELEASE, seq);
-	ceph_put_mds_session(session);
-}
-
-/*
  * drop all leases (and dentry refs) in preparation for umount
  */
 static void drop_leases(struct ceph_mds_client *mdsc)
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 2ce8e9f9bfc9..9dd2c82379f8 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -385,10 +385,6 @@ extern void ceph_mdsc_destroy(struct ceph_fs_client *fsc);
 
 extern void ceph_mdsc_sync(struct ceph_mds_client *mdsc);
 
-extern void ceph_mdsc_lease_release(struct ceph_mds_client *mdsc,
-				    struct inode *inode,
-				    struct dentry *dn);
-
 extern void ceph_invalidate_dir_request(struct ceph_mds_request *req);
 extern int ceph_alloc_readdir_reply_buffer(struct ceph_mds_request *req,
 					   struct inode *dir);
-- 
2.5.5

[PATCH 2/4] ceph: clear d_fsinfo pointer under d_lock

[Jeff Layton]

To check for a valid dentry lease, we need to get at the
ceph_dentry_info. Under rcuwalk though, we may end up with a dentry that
is on its way to destruction. Since we need to take the d_lock in
dentry_lease_is_valid already, we can just ensure that we clear the
d_fsinfo pointer out under the same lock before destroying it.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ceph/dir.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 8ff7bcc7fc88..904dc671580f 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1286,10 +1286,14 @@ static void ceph_d_release(struct dentry *dentry)
 
 	dout("d_release %p\n", dentry);
 	ceph_dentry_lru_del(dentry);
+
+	spin_lock(&dentry->d_lock);
+	dentry->d_fsdata = NULL;
+	spin_unlock(&dentry->d_lock);
+
 	if (di->lease_session)
 		ceph_put_mds_session(di->lease_session);
 	kmem_cache_free(ceph_dentry_cachep, di);
-	dentry->d_fsdata = NULL;
 }
 
 static int ceph_snapdir_d_revalidate(struct dentry *dentry,
-- 
2.5.5

[PATCH 3/4] ceph: allow dentry_lease_is_valid to work under RCU walk

[Jeff Layton]

Under rcuwalk, we need to take extra care when dereferencing d_parent.
We want to do that once and pass a pointer to dentry_lease_is_valid.

Also, we must ensure that that function can handle the case where we're
racing with d_release. Check whether "di" is NULL under the d_lock, and
just return 0 if so.

Finally, we still need to kick off a renewal job if the lease is getting
close to expiration. If that's the case, then just drop out of rcuwalk
mode since that could block.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ceph/dir.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 904dc671580f..60dcca163ba0 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1133,7 +1133,8 @@ void ceph_invalidate_dentry_lease(struct dentry *dentry)
  * Check if dentry lease is valid.  If not, delete the lease.  Try to
  * renew if the least is more than half up.
  */
-static int dentry_lease_is_valid(struct dentry *dentry)
+static int dentry_lease_is_valid(struct dentry *dentry, unsigned int flags,
+				 struct inode *dir)
 {
 	struct ceph_dentry_info *di;
 	struct ceph_mds_session *s;
@@ -1141,12 +1142,11 @@ static int dentry_lease_is_valid(struct dentry *dentry)
 	u32 gen;
 	unsigned long ttl;
 	struct ceph_mds_session *session = NULL;
-	struct inode *dir = NULL;
 	u32 seq = 0;
 
 	spin_lock(&dentry->d_lock);
 	di = ceph_dentry(dentry);
-	if (di->lease_session) {
+	if (di && di->lease_session) {
 		s = di->lease_session;
 		spin_lock(&s->s_gen_ttl_lock);
 		gen = s->s_cap_gen;
@@ -1159,12 +1159,19 @@ static int dentry_lease_is_valid(struct dentry *dentry)
 			valid = 1;
 			if (di->lease_renew_after &&
 			    time_after(jiffies, di->lease_renew_after)) {
-				/* we should renew */
-				dir = d_inode(dentry->d_parent);
-				session = ceph_get_mds_session(s);
-				seq = di->lease_seq;
-				di->lease_renew_after = 0;
-				di->lease_renew_from = jiffies;
+				/*
+				 * We should renew. If we're in RCU walk mode
+				 * though, we can't do that so just return
+				 * -ECHILD.
+				 */
+				if (flags & LOOKUP_RCU) {
+					valid = -ECHILD;
+				} else {
+					session = ceph_get_mds_session(s);
+					seq = di->lease_seq;
+					di->lease_renew_after = 0;
+					di->lease_renew_from = jiffies;
+				}
 			}
 		}
 	}
@@ -1224,12 +1231,16 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
 	} else if (d_really_is_positive(dentry) &&
 		   ceph_snap(d_inode(dentry)) == CEPH_SNAPDIR) {
 		valid = 1;
-	} else if (dentry_lease_is_valid(dentry) ||
-		   dir_lease_is_valid(dir, dentry)) {
-		if (d_really_is_positive(dentry))
-			valid = ceph_is_any_caps(d_inode(dentry));
-		else
-			valid = 1;
+	} else {
+		valid = dentry_lease_is_valid(dentry, flags, dir);
+		if (valid == -ECHILD)
+			return valid;
+		if (valid || dir_lease_is_valid(dir, dentry)) {
+			if (d_really_is_positive(dentry))
+				valid = ceph_is_any_caps(d_inode(dentry));
+			else
+				valid = 1;
+		}
 	}
 
 	if (!valid) {
-- 
2.5.5

[PATCH 4/4] ceph: handle LOOKUP_RCU in ceph_d_revalidate

[Jeff Layton]

We can now handle the snapshot cases under RCU, as well as the
non-snapshot case when we don't need to queue up a lease renewal.
Allow LOOKUP_RCU walks to proceed under those conditions.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ceph/dir.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 60dcca163ba0..c64a0b794d49 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1214,15 +1214,19 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
 	struct dentry *parent;
 	struct inode *dir;
 
-	if (flags & LOOKUP_RCU)
-		return -ECHILD;
+	if (flags & LOOKUP_RCU) {
+		parent = ACCESS_ONCE(dentry->d_parent);
+		dir = d_inode_rcu(parent);
+		if (!dir)
+			return -ECHILD;
+	} else {
+		parent = dget_parent(dentry);
+		dir = d_inode(parent);
+	}
 
 	dout("d_revalidate %p '%pd' inode %p offset %lld\n", dentry,
 	     dentry, d_inode(dentry), ceph_dentry(dentry)->offset);
 
-	parent = dget_parent(dentry);
-	dir = d_inode(parent);
-
 	/* always trust cached snapped dentries, snapdir dentry */
 	if (ceph_snap(dir) != CEPH_NOSNAP) {
 		dout("d_revalidate %p '%pd' inode %p is SNAPPED\n", dentry,
@@ -1249,6 +1253,9 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
 		struct ceph_mds_request *req;
 		int op, mask, err;
 
+		if (flags & LOOKUP_RCU)
+			return -ECHILD;
+
 		op = ceph_snap(dir) == CEPH_SNAPDIR ?
 			CEPH_MDS_OP_LOOKUPSNAP : CEPH_MDS_OP_LOOKUP;
 		req = ceph_mdsc_create_request(mdsc, op, USE_ANY_MDS);
@@ -1284,7 +1291,8 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
 		ceph_dir_clear_complete(dir);
 	}
 
-	dput(parent);
+	if (!(flags & LOOKUP_RCU))
+		dput(parent);
 	return valid;
 }
 
-- 
2.5.5

Re: [PATCH 0/4] ceph: allow ceph_d_revalidate to handle some cases under LOOKUP_RCU

[Yan, Zheng]

> On Jul 6, 2016, at 00:27, Jeff Layton <jlayton@redhat.com> wrote:
> 
> The basic idea with this set is to allow the kernel client to allow the
> client to do more d_revalidate functionality under the rcu_read_lock.
> The idea is basically to do what we can there while not blocking, and
> then return -ECHILD for the rest.
> 
> The only really questionable bit is the part where we need to take the
> d_lock in ceph_d_release. I think it should be largely uncontended, but
> it's still less than ideal. Maybe there is some way to handle that
> locklessly?
> 
> Also, it would be nice to not have to drop out of rcuwalk mode in order
> to run the lease renewal. It seems like the kind of thing that would be
> ideal to queue to a workqueue, but I don't see a way to do that without
> introducing a potential deadlock.
> 
> My teuthology run with it passed all but one test, which seems to have
> been an unrelated problem (though I could be wrong there).
> 
> Jeff Layton (4):
>  ceph: remove ceph_mdsc_lease_release
>  ceph: clear d_fsinfo pointer under d_lock
>  ceph: allow dentry_lease_is_valid to work under RCU walk
>  ceph: handle LOOKUP_RCU in ceph_d_revalidate
> 
> fs/ceph/dir.c        | 67 +++++++++++++++++++++++++++++++++++-----------------
> fs/ceph/mds_client.c | 41 --------------------------------
> fs/ceph/mds_client.h |  4 ----
> 3 files changed, 45 insertions(+), 67 deletions(-)

Reviewed-by: Yan, Zheng <zyan@redhat.com>


> 
> -- 
> 2.5.5
>