Skip to contentSkip to conversation

rbd: don't drop the rbd_id too early

PATCH In review Alex Elder 2 participants 2 messages Feb 29, 2012 – Mar 2, 2012

Subsystems (2)
lore
Share & tools
lore

Alex Elder

author
#0 lore raw

PATCH 1/1 rbd: don't drop the rbd_id too early

Currently an rbd device's id is released when it is removed, but it is done before the code is run to clean up sysfs-related files (such as /sys/bus/rbd/devices/1).

It's possible that an rbd is still in use after the rbd_remove() call has been made. It's essentially the same as an active inode that stays around after it has been removed--until its final close operation. This means that the id shows up as free for reuse at a time it should not be.

The effect of this was seen by Jens Rehpoehler, who:
- had a filesystem mounted on an rbd device
- unmapped that filesystem (without unmounting)
- found that the mount still worked properly
- but hit a panic when he attempted to re-map a new rbd device

This re-map attempt found the previously-unmapped id available. The subsequent attempt to reuse it was met with a panic while attempting to (re-)install the sysfs entry for the new mapped device.

Fix this by holding off "putting" the rbd id, until the rbd_device release function is called--when the last reference is finally dropped.

Note: This fixes: http://tracker.newdream.net/issues/1907

---
 drivers/block/rbd.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)
drivers/block/rbd.c +9 -5
index da38804..7cae4a3 100644
@@ -2440,7 +2440,12 @@ static ssize_t rbd_add(struct bus_type *bus,
 	if (rc)
 		goto err_out_blkdev;
 
-	/* set up and announce blkdev mapping */
+	/*
+	 * At this point cleanup in the event of an error is the job
+	 * of the sysfs code (initiated by rbd_bus_del_dev()).
+	 *
+	 * Set up and announce blkdev mapping.
+	 */
 	rc = rbd_init_disk(rbd_dev);
 	if (rc)
 		goto err_out_bus;
@@ -2452,8 +2457,6 @@ static ssize_t rbd_add(struct bus_type *bus,
 	return count;
 
 err_out_bus:
-	rbd_id_put(rbd_dev);
-
 	/* this will also clean up rest of rbd_dev stuff */
 
 	rbd_bus_del_dev(rbd_dev);
@@ -2511,6 +2514,9 @@ static void rbd_dev_release(struct device *dev)
 	/* clean up and free blkdev */
 	rbd_free_disk(rbd_dev);
 	unregister_blkdev(rbd_dev->major, rbd_dev->name);
+
+	/* done with the id, and with the rbd_dev */
+	rbd_id_put(rbd_dev);
 	kfree(rbd_dev);
 
 	/* release module ref */
@@ -2543,8 +2549,6 @@ static ssize_t rbd_remove(struct bus_type *bus,
 		goto done;
 	}
 
-	rbd_id_put(rbd_dev);
-
 	__rbd_remove_all_snaps(rbd_dev);
 	rbd_bus_del_dev(rbd_dev);
 
Signature · 1 line
1.7.5.4
Reviewed-by on v1 1/1
#1 lore raw

Yay!

Quoted 87 lines · Alex Elder

Currently an rbd device's id is released when it is removed, but it is done before the code is run to clean up sysfs-related files (such as /sys/bus/rbd/devices/1).

It's possible that an rbd is still in use after the rbd_remove() call has been made. It's essentially the same as an active inode that stays around after it has been removed--until its final close operation. This means that the id shows up as free for reuse at a time it should not be.

The effect of this was seen by Jens Rehpoehler, who:
- had a filesystem mounted on an rbd device
- unmapped that filesystem (without unmounting)
- found that the mount still worked properly
- but hit a panic when he attempted to re-map a new rbd device

This re-map attempt found the previously-unmapped id available. The subsequent attempt to reuse it was met with a panic while attempting to (re-)install the sysfs entry for the new mapped device.

Fix this by holding off "putting" the rbd id, until the rbd_device release function is called--when the last reference is finally dropped.

Note: This fixes: http://tracker.newdream.net/issues/1907

---
 drivers/block/rbd.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)
drivers/block/rbd.c +9 -5
index da38804..7cae4a3 100644
@@ -2440,7 +2440,12 @@ static ssize_t rbd_add(struct bus_type *bus,
 	if (rc)
 		goto err_out_blkdev;
 
-	/* set up and announce blkdev mapping */
+	/*
+	 * At this point cleanup in the event of an error is the job
+	 * of the sysfs code (initiated by rbd_bus_del_dev()).
+	 *
+	 * Set up and announce blkdev mapping.
+	 */
 	rc = rbd_init_disk(rbd_dev);
 	if (rc)
 		goto err_out_bus;
@@ -2452,8 +2457,6 @@ static ssize_t rbd_add(struct bus_type *bus,
 	return count;
 
 err_out_bus:
-	rbd_id_put(rbd_dev);
-
 	/* this will also clean up rest of rbd_dev stuff */
 
 	rbd_bus_del_dev(rbd_dev);
@@ -2511,6 +2514,9 @@ static void rbd_dev_release(struct device *dev)
 	/* clean up and free blkdev */
 	rbd_free_disk(rbd_dev);
 	unregister_blkdev(rbd_dev->major, rbd_dev->name);
+
+	/* done with the id, and with the rbd_dev */
+	rbd_id_put(rbd_dev);
 	kfree(rbd_dev);
 
 	/* release module ref */
@@ -2543,8 +2549,6 @@ static ssize_t rbd_remove(struct bus_type *bus,
 		goto done;
 	}
 
-	rbd_id_put(rbd_dev);
-
 	__rbd_remove_all_snaps(rbd_dev);
 	rbd_bus_del_dev(rbd_dev);
 
Signature · 6 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