[PATCH 0/6] rbd: parent probe cleanup

[PATCH 0/6] rbd: parent probe cleanup

[Alex Elder]

This series cleans up some code in the are of probing
for parent images.

					-Alex

[PATCH 1/6] rbd: encapsulate probing for parent devices
[PATCH 2/6] rbd: encapsulate removing parent devices
[PATCH 3/6] rbd: kill __rbd_remove()
[PATCH 4/6] rbd: fix rbd_dev_remove_parent()
[PATCH 5/6] rbd: remove parent devices on probe error
[PATCH 6/6] rbd: probe for the parent earlier

[PATCH 1/6] rbd: encapsulate probing for parent devices

[Alex Elder]

Encapsulate the code that probes for an rbd device's parent images
into a new function, rbd_dev_probe_parent().

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   77
+++++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 33 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index b10348c..88cd5f4 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4696,11 +4696,49 @@ out_err:
 	return ret;
 }

-static int rbd_dev_probe_finish(struct rbd_device *rbd_dev)
+static int rbd_dev_probe_parent(struct rbd_device *rbd_dev)
 {
 	struct rbd_device *parent = NULL;
-	struct rbd_spec *parent_spec = NULL;
-	struct rbd_client *rbdc = NULL;
+	struct rbd_spec *parent_spec;
+	struct rbd_client *rbdc;
+	int ret;
+
+	if (!rbd_dev->parent_spec)
+		return 0;
+	/*
+	 * We need to pass a reference to the client and the parent
+	 * spec when creating the parent rbd_dev.  Images related by
+	 * parent/child relationships always share both.
+	 */
+	parent_spec = rbd_spec_get(rbd_dev->parent_spec);
+	rbdc = __rbd_get_client(rbd_dev->rbd_client);
+
+	ret = -ENOMEM;
+	parent = rbd_dev_create(rbdc, parent_spec);
+	if (!parent)
+		goto out_err;
+
+	ret = rbd_dev_probe_image(parent);
+	if (ret < 0)
+		goto out_err;
+	rbd_dev->parent = parent;
+
+	return 0;
+out_err:
+	if (parent) {
+		rbd_spec_put(rbd_dev->parent_spec);
+		kfree(rbd_dev->header_name);
+		rbd_dev_destroy(parent);
+	} else {
+		rbd_put_client(rbdc);
+		rbd_spec_put(parent_spec);
+	}
+
+	return ret;
+}
+
+static int rbd_dev_probe_finish(struct rbd_device *rbd_dev)
+{
 	int ret;

 	/* no need to lock here, as rbd_dev is not registered yet */
@@ -4741,30 +4779,10 @@ static int rbd_dev_probe_finish(struct
rbd_device *rbd_dev)
 	 * At this point cleanup in the event of an error is the job
 	 * of the sysfs code (initiated by rbd_bus_del_dev()).
 	 */
-	/* Probe the parent if there is one */
-
-	if (rbd_dev->parent_spec) {
-		/*
-		 * We need to pass a reference to the client and the
-		 * parent spec when creating the parent rbd_dev.
-		 * Images related by parent/child relationships
-		 * always share both.
-		 */
-		parent_spec = rbd_spec_get(rbd_dev->parent_spec);
-		rbdc = __rbd_get_client(rbd_dev->rbd_client);

-		parent = rbd_dev_create(rbdc, parent_spec);
-		if (!parent) {
-			ret = -ENOMEM;
-			goto err_out_spec;
-		}
-		rbdc = NULL;		/* parent now owns reference */
-		parent_spec = NULL;	/* parent now owns reference */
-		ret = rbd_dev_probe_image(parent);
-		if (ret < 0)
-			goto err_out_parent;
-		rbd_dev->parent = parent;
-	}
+	ret = rbd_dev_probe_parent(rbd_dev);
+	if (ret)
+		goto err_out_bus;

 	ret = rbd_dev_header_watch_sync(rbd_dev, 1);
 	if (ret)
@@ -4786,13 +4804,6 @@ static int rbd_dev_probe_finish(struct rbd_device
*rbd_dev)

 	return ret;

-err_out_parent:
-	rbd_spec_put(rbd_dev->parent_spec);
-	kfree(rbd_dev->header_name);
-	rbd_dev_destroy(parent);
-err_out_spec:
-	rbd_spec_put(parent_spec);
-	rbd_put_client(rbdc);
 err_out_bus:
 	/* this will also clean up rest of rbd_dev stuff */

-- 
1.7.9.5

[PATCH 2/6] rbd: encapsulate removing parent devices

[Alex Elder]

Encapsulate the code that removes an rbd device's parent images into
a new function, rbd_dev_remove_parent().

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   46 +++++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 88cd5f4..fa703db 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -427,8 +427,9 @@ void rbd_warn(struct rbd_device *rbd_dev, const char
*fmt, ...)
 #  define rbd_assert(expr)	((void) 0)
 #endif /* !RBD_DEBUG */

-static void rbd_img_parent_read(struct rbd_obj_request *obj_request);
 static int rbd_img_obj_request_submit(struct rbd_obj_request *obj_request);
+static void rbd_img_parent_read(struct rbd_obj_request *obj_request);
+static void rbd_dev_remove_parent(struct rbd_device *rbd_dev);

 static int rbd_dev_refresh(struct rbd_device *rbd_dev, u64 *hver);
 static int rbd_dev_v2_refresh(struct rbd_device *rbd_dev, u64 *hver);
@@ -4987,6 +4988,29 @@ static void __rbd_remove(struct rbd_device *rbd_dev)
 	rbd_bus_del_dev(rbd_dev);
 }

+static void rbd_dev_remove_parent(struct rbd_device *rbd_dev)
+{
+	while (rbd_dev->parent_spec) {
+		struct rbd_device *first = rbd_dev;
+		struct rbd_device *second = first->parent;
+		struct rbd_device *third;
+
+		/*
+		 * Follow to the parent with no grandparent and
+		 * remove it.
+		 */
+		while (second && (third = second->parent)) {
+			first = second;
+			second = third;
+		}
+		__rbd_remove(second);
+		rbd_spec_put(first->parent_spec);
+		first->parent_spec = NULL;
+		first->parent_overlap = 0;
+		first->parent = NULL;
+	}
+}
+
 static ssize_t rbd_remove(struct bus_type *bus,
 			  const char *buf,
 			  size_t count)
@@ -5022,25 +5046,9 @@ static ssize_t rbd_remove(struct bus_type *bus,
 	if (ret < 0)
 		goto done;

-	while (rbd_dev->parent_spec) {
-		struct rbd_device *first = rbd_dev;
-		struct rbd_device *second = first->parent;
-		struct rbd_device *third;
+	if (rbd_dev->parent)
+		rbd_dev_remove_parent(rbd_dev);

-		/*
-		 * Follow to the parent with no grandparent and
-		 * remove it.
-		 */
-		while (second && (third = second->parent)) {
-			first = second;
-			second = third;
-		}
-		__rbd_remove(second);
-		rbd_spec_put(first->parent_spec);
-		first->parent_spec = NULL;
-		first->parent_overlap = 0;
-		first->parent = NULL;
-	}
 	__rbd_remove(rbd_dev);

 done:
-- 
1.7.9.5

[PATCH 3/6] rbd: kill __rbd_remove()

[Alex Elder]

The function __rbd_remove() is used in two spots, and it's fairly
simple.  It combines cleanup of part of the ceph-side state as well
as cleaning up the Linux-side state.  Just open code it in the two
callers and eliminate the function.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index fa703db..9936067 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4982,12 +4982,6 @@ static void rbd_dev_release(struct device *dev)
 	module_put(THIS_MODULE);
 }

-static void __rbd_remove(struct rbd_device *rbd_dev)
-{
-	rbd_remove_all_snaps(rbd_dev);
-	rbd_bus_del_dev(rbd_dev);
-}
-
 static void rbd_dev_remove_parent(struct rbd_device *rbd_dev)
 {
 	while (rbd_dev->parent_spec) {
@@ -5003,7 +4997,8 @@ static void rbd_dev_remove_parent(struct
rbd_device *rbd_dev)
 			first = second;
 			second = third;
 		}
-		__rbd_remove(second);
+		rbd_remove_all_snaps(second);
+		rbd_bus_del_dev(second);
 		rbd_spec_put(first->parent_spec);
 		first->parent_spec = NULL;
 		first->parent_overlap = 0;
@@ -5049,8 +5044,8 @@ static ssize_t rbd_remove(struct bus_type *bus,
 	if (rbd_dev->parent)
 		rbd_dev_remove_parent(rbd_dev);

-	__rbd_remove(rbd_dev);
-
+	rbd_remove_all_snaps(rbd_dev);
+	rbd_bus_del_dev(rbd_dev);
 done:
 	mutex_unlock(&ctl_mutex);

-- 
1.7.9.5

[PATCH 4/6] rbd: fix rbd_dev_remove_parent()

[Alex Elder]

In certain error paths, it is possible for an rbd device to have a
parent spec but no parent rbd_dev.  In rbd_dev_remove_parent() use
the parent field rather than parent_spec in determining whether to
try to remove any parent devices.  Use assertions to indicate that
any non-null parent pointer has parent_spec associated with it.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 9936067..99d231b 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4984,7 +4984,7 @@ static void rbd_dev_release(struct device *dev)

 static void rbd_dev_remove_parent(struct rbd_device *rbd_dev)
 {
-	while (rbd_dev->parent_spec) {
+	while (rbd_dev->parent) {
 		struct rbd_device *first = rbd_dev;
 		struct rbd_device *second = first->parent;
 		struct rbd_device *third;
@@ -4997,12 +4997,15 @@ static void rbd_dev_remove_parent(struct
rbd_device *rbd_dev)
 			first = second;
 			second = third;
 		}
+		rbd_assert(second);
 		rbd_remove_all_snaps(second);
 		rbd_bus_del_dev(second);
+		first->parent = NULL;
+		first->parent_overlap = 0;
+
+		rbd_assert(first->parent_spec);
 		rbd_spec_put(first->parent_spec);
 		first->parent_spec = NULL;
-		first->parent_overlap = 0;
-		first->parent = NULL;
 	}
 }

-- 
1.7.9.5

[PATCH 5/6] rbd: remove parent devices on probe error

[Alex Elder]

When an error occurs while finishing probing a device it is assumed
that parent devices get cleaned up when deleting a device.  They
don't.  Add a call to clean them up.  Note that this means the
parent spec will already be cleaned up so it doesn't have to be
in one of the rbd_add() error paths.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 99d231b..c81b319 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4806,8 +4806,8 @@ static int rbd_dev_probe_finish(struct rbd_device
*rbd_dev)
 	return ret;

 err_out_bus:
-	/* this will also clean up rest of rbd_dev stuff */
-
+	if (rbd_dev->parent)
+		rbd_dev_remove_parent(rbd_dev);
 	rbd_bus_del_dev(rbd_dev);

 	return ret;
@@ -4922,7 +4922,6 @@ static ssize_t rbd_add(struct bus_type *bus,

 	return count;
 err_out_rbd_dev:
-	rbd_spec_put(rbd_dev->parent_spec);
 	kfree(rbd_dev->header_name);
 	rbd_dev_destroy(rbd_dev);
 err_out_client:
-- 
1.7.9.5

[PATCH 6/6] rbd: probe for the parent earlier

[Alex Elder]

Probe for a parent device earlier in rbd_dev_probe_finish(), before
starting to set up the Linux side of the rbd device.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index c81b319..5fd923f 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4751,6 +4751,10 @@ static int rbd_dev_probe_finish(struct rbd_device
*rbd_dev)
 	if (ret)
 		goto err_out_snaps;

+	ret = rbd_dev_probe_parent(rbd_dev);
+	if (ret)
+		goto err_out_snaps;
+
 	/* generate unique id: find highest unique id, add one */
 	rbd_dev_id_get(rbd_dev);

@@ -4781,10 +4785,6 @@ static int rbd_dev_probe_finish(struct rbd_device
*rbd_dev)
 	 * of the sysfs code (initiated by rbd_bus_del_dev()).
 	 */

-	ret = rbd_dev_probe_parent(rbd_dev);
-	if (ret)
-		goto err_out_bus;
-
 	ret = rbd_dev_header_watch_sync(rbd_dev, 1);
 	if (ret)
 		goto err_out_bus;
@@ -4806,8 +4806,6 @@ static int rbd_dev_probe_finish(struct rbd_device
*rbd_dev)
 	return ret;

 err_out_bus:
-	if (rbd_dev->parent)
-		rbd_dev_remove_parent(rbd_dev);
 	rbd_bus_del_dev(rbd_dev);

 	return ret;
@@ -4817,6 +4815,8 @@ err_out_blkdev:
 	unregister_blkdev(rbd_dev->major, rbd_dev->name);
 err_out_id:
 	rbd_dev_id_put(rbd_dev);
+	if (rbd_dev->parent);
+		rbd_dev_remove_parent(rbd_dev);
 err_out_snaps:
 	rbd_remove_all_snaps(rbd_dev);

-- 
1.7.9.5

Re: [PATCH 0/6] rbd: parent probe cleanup

[Josh Durgin]

On 04/27/2013 05:35 AM, Alex Elder wrote:
> This series cleans up some code in the are of probing
> for parent images.
>
> 					-Alex
>
> [PATCH 1/6] rbd: encapsulate probing for parent devices
> [PATCH 2/6] rbd: encapsulate removing parent devices
> [PATCH 3/6] rbd: kill __rbd_remove()
> [PATCH 4/6] rbd: fix rbd_dev_remove_parent()
> [PATCH 5/6] rbd: remove parent devices on probe error
> [PATCH 6/6] rbd: probe for the parent earlier

These all look good.

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>