Skip to contentSkip to conversation

rbd: don't hold spinlock during messenger flush

PATCH In review Alex Elder 2 participants 2 messages Apr 5, 2012

Subsystems (2)
Block layer RADOS block device (RBD)
lore
Share & tools
lore

Alex Elder

author
#0 lore raw

PATCH 1/1 rbd: don't hold spinlock during messenger flush

A recent change made changes to the rbd_client_list be protected by a spinlock. Unfortunately in rbd_put_client(), the lock is taken before possibly dropping the last reference to an rbd_client, and on the last reference that eventually calls flush_workqueue() which can sleep.

The problem was flagged by a debug spinlock warning:

    BUG: spinlock wrong CPU on CPU#3, rbd/27814

The solution is to move the spinlock acquisition and release inside rbd_client_release(), which is the spot where it's really needed for protecting the removal of the rbd_client from the client list.

Signed-off-by Alex Elder <[email protected]>
---
 drivers/block/rbd.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/block/rbd.c +2 -2
index 013c7a5..c1f7701 100644
@@ -450,7 +450,9 @@ static void rbd_client_release(struct kref *kref)
 	struct rbd_client *rbdc = container_of(kref, struct rbd_client, kref);
 
 	dout("rbd_release_client %p\n", rbdc);
+	spin_lock(&rbd_client_list_lock);
 	list_del(&rbdc->node);
+	spin_unlock(&rbd_client_list_lock);
 
 	ceph_destroy_client(rbdc->client);
 	kfree(rbdc->rbd_opts);

@@ -463,9 +465,7 @@ static void rbd_client_release(struct kref *kref)
  */
 static void rbd_put_client(struct rbd_device *rbd_dev)
 {
-	spin_lock(&rbd_client_list_lock);
 	kref_put(&rbd_dev->rbd_client->kref, rbd_client_release);
-	spin_unlock(&rbd_client_list_lock);
 	rbd_dev->rbd_client = NULL;
 }
Signature · 1 line
1.7.5.4
Reviewed-by on v1 1/1
#1 lore raw
Quoted 33 lines · Alex Elder

A recent change made changes to the rbd_client_list be protected by a spinlock. Unfortunately in rbd_put_client(), the lock is taken before possibly dropping the last reference to an rbd_client, and on the last reference that eventually calls flush_workqueue() which can sleep.

The problem was flagged by a debug spinlock warning:

    BUG: spinlock wrong CPU on CPU#3, rbd/27814

The solution is to move the spinlock acquisition and release inside rbd_client_release(), which is the spot where it's really needed for protecting the removal of the rbd_client from the client list.

Signed-off-by Alex Elder <[email protected]>
---
 drivers/block/rbd.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/block/rbd.c +2 -0
index 013c7a5..c1f7701 100644
@@ -450,7 +450,9 @@ static void rbd_client_release(struct kref *kref)
 	struct rbd_client *rbdc = container_of(kref, struct rbd_client, kref);
 
 	dout("rbd_release_client %p\n", rbdc);
+	spin_lock(&rbd_client_list_lock);
 	list_del(&rbdc->node);
+	spin_unlock(&rbd_client_list_lock);
 
 	ceph_destroy_client(rbdc->client);
 	kfree(rbdc->rbd_opts);

drivers/block/rbd.c static void rbd_client_release(struct kref *kref) · line 470

Quoted 8 lines
drivers/block/rbd.c +0 -2
@@ -463,9 +465,7 @@ static void rbd_client_release(struct kref *kref)
  */
 static void rbd_put_client(struct rbd_device *rbd_dev)
 {
-	spin_lock(&rbd_client_list_lock);
 	kref_put(&rbd_dev->rbd_client->kref, rbd_client_release);
-	spin_unlock(&rbd_client_list_lock);
 	rbd_dev->rbd_client = NULL;
 }

This looks right to me. Let's get it into testing...

Reviewed-by Sage Weil <[email protected]>
Quoted 10 lines
Signature · 7 lines
-- 
1.7.5.4

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html