[PATCH 0/5] rbd: only set up Linux files for mapped devices

[PATCH 0/5] rbd: only set up Linux files for mapped devices

[Alex Elder]

This series does a few more things to separate the
ceph and Linux side of rbd device initialization,
and to make sure things get cleaned up in a sane
way when errors occur.  The last patch makes it so
only the rbd image getting mapped has Linux device
and sysfs stuff associated with them.  The parent
rbd devices for layered images never needed this
stuff, and things have now been rearranged such
that we can do away with that.

These patches are available in the ceph-client git
repository in the branch "review/wip-rbd-cleanup-5".

					-Alex
[PATCH 1/5] rbd: drop module later
[PATCH 2/5] rbd: don't destroy rbd_dev in device release function
[PATCH 3/5] rbd: define rbd_dev_unprobe()
[PATCH 4/5] rbd: don't have device release destroy rbd_dev
[PATCH 5/5] rbd: set up devices only for mapped images

[PATCH 1/5] rbd: drop module later

[Alex Elder]

Drop the module reference at the end of rbd_remove() for symmetry
with adding a reference at the top of rbd_add().

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index ac94aa4..5904819 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4986,9 +4986,6 @@ static void rbd_dev_release(struct device *dev)
 	rbd_spec_put(rbd_dev->parent_spec);
 	kfree(rbd_dev->header_name);
 	rbd_dev_destroy(rbd_dev);
-
-	/* release module ref */
-	module_put(THIS_MODULE);
 }

 static void rbd_dev_remove_parent(struct rbd_device *rbd_dev)
@@ -5071,6 +5068,7 @@ static ssize_t rbd_remove(struct bus_type *bus,

 	rbd_remove_all_snaps(rbd_dev);
 	rbd_bus_del_dev(rbd_dev);
+	module_put(THIS_MODULE);
 done:
 	mutex_unlock(&ctl_mutex);

-- 
1.7.9.5

[PATCH 2/5] rbd: don't destroy rbd_dev in device release function

[Alex Elder]

Rename rbd_dev_probe_finish() to be rbd_dev_device_setup().  Its
purpose is to set up the Linux side of an rbd device mapping.
Rename rbd_dev_release() to be rbd_dev_device_release(), making
it more obvious it serves as the inverse of the setup function
(or it will).

Encapsulate some of what was done in rbd_dev_release() into a new
function rbd_dev_image_release(), which serves as the inverse of
setting up the ceph side of the mapped rbd image.

Define a new helper rbd_dev_clear_mapping() to simply zero out the
fields of a mapping structure--the inverse of rbd_dev_set_mapping().

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 5904819..feaa2e9 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -358,7 +358,7 @@ static int rbd_img_request_submit(struct
rbd_img_request *img_request);

 static int rbd_dev_snaps_update(struct rbd_device *rbd_dev);

-static void rbd_dev_release(struct device *dev);
+static void rbd_dev_device_release(struct device *dev);
 static void rbd_snap_destroy(struct rbd_snap *snap);

 static ssize_t rbd_add(struct bus_type *bus, const char *buf,
@@ -893,6 +893,13 @@ static void rbd_dev_mapping_clear(struct rbd_device
*rbd_dev)
 	rbd_dev->mapping.read_only = true;
 }

+static void rbd_dev_clear_mapping(struct rbd_device *rbd_dev)
+{
+	rbd_dev->mapping.size = 0;
+	rbd_dev->mapping.features = 0;
+	rbd_dev->mapping.read_only = true;
+}
+
 static void rbd_header_free(struct rbd_image_header *header)
 {
 	kfree(header->object_prefix);
@@ -4182,7 +4189,7 @@ static int rbd_bus_add_dev(struct rbd_device *rbd_dev)
 	dev->bus = &rbd_bus_type;
 	dev->type = &rbd_device_type;
 	dev->parent = &rbd_root_dev;
-	dev->release = rbd_dev_release;
+	dev->release = rbd_dev_device_release;
 	dev_set_name(dev, "%d", rbd_dev->dev_id);
 	ret = device_register(dev);

@@ -4718,7 +4725,7 @@ out_err:
 	return ret;
 }

-static int rbd_dev_probe_finish(struct rbd_device *rbd_dev)
+static int rbd_dev_device_setup(struct rbd_device *rbd_dev)
 {
 	int ret;

@@ -4800,6 +4807,15 @@ static int rbd_dev_header_name(struct rbd_device
*rbd_dev)
 	return 0;
 }

+static void rbd_dev_image_release(struct rbd_device *rbd_dev)
+{
+	rbd_header_free(&rbd_dev->header);
+	rbd_assert(rbd_dev->rbd_client != NULL);
+	rbd_spec_put(rbd_dev->parent_spec);
+	kfree(rbd_dev->header_name);
+	rbd_dev_destroy(rbd_dev);
+}
+
 /*
  * Probe for the existence of the header object for the given rbd
  * device.  For format 2 images this includes determining the image
@@ -4848,7 +4864,7 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev)
 	if (ret)
 		goto err_out_snaps;

-	ret = rbd_dev_probe_finish(rbd_dev);
+	ret = rbd_dev_device_setup(rbd_dev);
 	if (ret)
 		goto err_out_parent;

@@ -4968,24 +4984,19 @@ static struct rbd_device *__rbd_get_dev(unsigned
long dev_id)
 	return NULL;
 }

-static void rbd_dev_release(struct device *dev)
+static void rbd_dev_device_release(struct device *dev)
 {
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

-	/* clean up and free blkdev */
 	rbd_free_disk(rbd_dev);
+	clear_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags);
+	rbd_dev_clear_mapping(rbd_dev);
 	unregister_blkdev(rbd_dev->major, rbd_dev->name);
-
-	/* release allocated disk header fields */
-	rbd_header_free(&rbd_dev->header);
-
-	/* done with the id, and with the rbd_dev */
+	rbd_dev->major = 0;
 	rbd_dev_id_put(rbd_dev);
 	rbd_dev_mapping_clear(rbd_dev);
-	rbd_assert(rbd_dev->rbd_client != NULL);
-	rbd_spec_put(rbd_dev->parent_spec);
-	kfree(rbd_dev->header_name);
-	rbd_dev_destroy(rbd_dev);
+
+	rbd_dev_image_release(rbd_dev);
 }

 static void rbd_dev_remove_parent(struct rbd_device *rbd_dev)
-- 
1.7.9.5

[PATCH 3/5] rbd: define rbd_dev_unprobe()

[Alex Elder]

Define a new function rbd_dev_unprobe() which undoes state changes
that occur from calling rbd_dev_v1_probe() or rbd_dev_v2_probe().
Note that this is a superset of rbd_header_free(), which is now
getting removed (it seems to have been used improperly anyway).

Flesh out rbd_dev_image_release() so it undoes exactly what
rbd_dev_image_probe() does.

This means that:
    - rbd_dev_device_release() gets called when the last device
      reference gets dropped;
    - that undoes everything done by the rbd_dev_device_setup() call
      at the end of rbd_dev_image_probe() (and nothing more), ending
      by calling rbd_dev_image_release(); and
    - rbd_dev_image_release() undoes everything else done by
      rbd_dev_image_probe() (and this includes a call to
      rbd_dev_unprobe().

This means the image and device portions of an rbd device are fairly
cleanly separated now, so error paths should be a little easier to
verify than they used to be.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index feaa2e9..595d7b3 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -900,18 +900,6 @@ static void rbd_dev_clear_mapping(struct rbd_device
*rbd_dev)
 	rbd_dev->mapping.read_only = true;
 }

-static void rbd_header_free(struct rbd_image_header *header)
-{
-	kfree(header->object_prefix);
-	header->object_prefix = NULL;
-	kfree(header->snap_sizes);
-	header->snap_sizes = NULL;
-	kfree(header->snap_names);
-	header->snap_names = NULL;
-	rbd_snap_context_put(header->snapc);
-	header->snapc = NULL;
-}
-
 static const char *rbd_segment_name(struct rbd_device *rbd_dev, u64 offset)
 {
 	char *name;
@@ -4588,6 +4576,27 @@ out:
 	return ret;
 }

+/* Undo whatever state changes are made by v1 or v2 image probe */
+
+static void rbd_dev_unprobe(struct rbd_device *rbd_dev)
+{
+	struct rbd_image_header	*header;
+
+	rbd_dev_remove_parent(rbd_dev);
+	rbd_spec_put(rbd_dev->parent_spec);
+	rbd_dev->parent_spec = NULL;
+	rbd_dev->parent_overlap = 0;
+
+	/* Free dynamic fields from the header, then zero it out */
+
+	header = &rbd_dev->header;
+	rbd_snap_context_put(header->snapc);
+	kfree(header->snap_sizes);
+	kfree(header->snap_names);
+	kfree(header->object_prefix);
+	memset(header, 0, sizeof (*header));
+}
+
 static int rbd_dev_v1_probe(struct rbd_device *rbd_dev)
 {
 	int ret;
@@ -4809,10 +4818,20 @@ static int rbd_dev_header_name(struct rbd_device
*rbd_dev)

 static void rbd_dev_image_release(struct rbd_device *rbd_dev)
 {
-	rbd_header_free(&rbd_dev->header);
-	rbd_assert(rbd_dev->rbd_client != NULL);
-	rbd_spec_put(rbd_dev->parent_spec);
+	int ret;
+
+	rbd_dev_remove_parent(rbd_dev);
+	rbd_remove_all_snaps(rbd_dev);
+	rbd_dev_unprobe(rbd_dev);
+	ret = rbd_dev_header_watch_sync(rbd_dev, 0);
+	if (ret)
+		rbd_warn(rbd_dev, "failed to cancel watch event (%d)\n", ret);
 	kfree(rbd_dev->header_name);
+	rbd_dev->header_name = NULL;
+	rbd_dev->image_format = 0;
+	kfree(rbd_dev->spec->image_id);
+	rbd_dev->spec->image_id = NULL;
+
 	rbd_dev_destroy(rbd_dev);
 }

@@ -4854,7 +4873,7 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev)

 	ret = rbd_dev_snaps_update(rbd_dev);
 	if (ret)
-		goto err_out_watch;
+		goto err_out_probe;

 	ret = rbd_dev_spec_update(rbd_dev);
 	if (ret)
@@ -4865,15 +4884,14 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev)
 		goto err_out_snaps;

 	ret = rbd_dev_device_setup(rbd_dev);
-	if (ret)
-		goto err_out_parent;
+	if (!ret)
+		return 0;

-	return ret;
-err_out_parent:
 	rbd_dev_remove_parent(rbd_dev);
-	rbd_header_free(&rbd_dev->header);
 err_out_snaps:
 	rbd_remove_all_snaps(rbd_dev);
+err_out_probe:
+	rbd_dev_unprobe(rbd_dev);
 err_out_watch:
 	tmp = rbd_dev_header_watch_sync(rbd_dev, 0);
 	if (tmp)
@@ -5005,7 +5023,6 @@ static void rbd_dev_remove_parent(struct
rbd_device *rbd_dev)
 		struct rbd_device *first = rbd_dev;
 		struct rbd_device *second = first->parent;
 		struct rbd_device *third;
-		int ret;

 		/*
 		 * Follow to the parent with no grandparent and
@@ -5016,11 +5033,6 @@ static void rbd_dev_remove_parent(struct
rbd_device *rbd_dev)
 			second = third;
 		}
 		rbd_assert(second);
-		ret = rbd_dev_header_watch_sync(rbd_dev, 0);
-		if (ret)
-			rbd_warn(rbd_dev,
-				"failed to cancel watch event (%d)\n", ret);
-		rbd_remove_all_snaps(second);
 		rbd_bus_del_dev(second);
 		first->parent = NULL;
 		first->parent_overlap = 0;
@@ -5065,19 +5077,7 @@ static ssize_t rbd_remove(struct bus_type *bus,
 	spin_unlock_irq(&rbd_dev->lock);
 	if (ret < 0)
 		goto done;
-
-	ret = rbd_dev_header_watch_sync(rbd_dev, 0);
-	if (ret) {
-		rbd_warn(rbd_dev, "failed to cancel watch event (%d)\n", ret);
-		clear_bit(RBD_DEV_FLAG_REMOVING, &rbd_dev->flags);
-		smp_mb();
-		return ret;
-	}
 	ret = count;
-
-	rbd_dev_remove_parent(rbd_dev);
-
-	rbd_remove_all_snaps(rbd_dev);
 	rbd_bus_del_dev(rbd_dev);
 	module_put(THIS_MODULE);
 done:
-- 
1.7.9.5

[PATCH 4/5] rbd: don't have device release destroy rbd_dev

[Alex Elder]

Currently an rbd_device structure gets destroyed from the release
routine for the device embedded within it.  Stop doing that, instead
calling rbd_dev_image_release() right after rbd_bus_del_dev()
wherever the latter is called.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 595d7b3..ff850e9 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -5013,8 +5013,6 @@ static void rbd_dev_device_release(struct device *dev)
 	rbd_dev->major = 0;
 	rbd_dev_id_put(rbd_dev);
 	rbd_dev_mapping_clear(rbd_dev);
-
-	rbd_dev_image_release(rbd_dev);
 }

 static void rbd_dev_remove_parent(struct rbd_device *rbd_dev)
@@ -5034,6 +5032,7 @@ static void rbd_dev_remove_parent(struct
rbd_device *rbd_dev)
 		}
 		rbd_assert(second);
 		rbd_bus_del_dev(second);
+		rbd_dev_image_release(second);
 		first->parent = NULL;
 		first->parent_overlap = 0;

@@ -5079,6 +5078,7 @@ static ssize_t rbd_remove(struct bus_type *bus,
 		goto done;
 	ret = count;
 	rbd_bus_del_dev(rbd_dev);
+	rbd_dev_image_release(rbd_dev);
 	module_put(THIS_MODULE);
 done:
 	mutex_unlock(&ctl_mutex);
-- 
1.7.9.5

[PATCH 5/5] rbd: set up devices only for mapped images

[Alex Elder]

Stop setting up Linux devices during the image probe operation.
Instead, set up the devices as a separate step after the image
probe, in rbd_add().

A consequence of this is that only mapped images get devices
assigned to them, which is pretty sweet.

This resolves:
    http://tracker.ceph.com/issues/4774

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index ff850e9..8b680ad 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4880,14 +4880,9 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev)
 		goto err_out_snaps;

 	ret = rbd_dev_probe_parent(rbd_dev);
-	if (ret)
-		goto err_out_snaps;
-
-	ret = rbd_dev_device_setup(rbd_dev);
 	if (!ret)
 		return 0;

-	rbd_dev_remove_parent(rbd_dev);
 err_out_snaps:
 	rbd_remove_all_snaps(rbd_dev);
 err_out_probe:
@@ -4966,9 +4961,12 @@ static ssize_t rbd_add(struct bus_type *bus,
 	if (rc < 0)
 		goto err_out_rbd_dev;

-	return count;
+	rc = rbd_dev_device_setup(rbd_dev);
+	if (!rc)
+		return count;
+
+	rbd_dev_image_release(rbd_dev);
 err_out_rbd_dev:
-	kfree(rbd_dev->header_name);
 	rbd_dev_destroy(rbd_dev);
 err_out_client:
 	rbd_put_client(rbdc);
@@ -5031,7 +5029,6 @@ static void rbd_dev_remove_parent(struct
rbd_device *rbd_dev)
 			second = third;
 		}
 		rbd_assert(second);
-		rbd_bus_del_dev(second);
 		rbd_dev_image_release(second);
 		first->parent = NULL;
 		first->parent_overlap = 0;
-- 
1.7.9.5

Re: [PATCH 3/5] rbd: define rbd_dev_unprobe()

[Josh Durgin]

There are a couple calls that look unnecessary noted below.
Other than that:

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

On 04/29/2013 11:00 AM, Alex Elder wrote:
> Define a new function rbd_dev_unprobe() which undoes state changes
> that occur from calling rbd_dev_v1_probe() or rbd_dev_v2_probe().
> Note that this is a superset of rbd_header_free(), which is now
> getting removed (it seems to have been used improperly anyway).
>
> Flesh out rbd_dev_image_release() so it undoes exactly what
> rbd_dev_image_probe() does.
>
> This means that:
>      - rbd_dev_device_release() gets called when the last device
>        reference gets dropped;
>      - that undoes everything done by the rbd_dev_device_setup() call
>        at the end of rbd_dev_image_probe() (and nothing more), ending
>        by calling rbd_dev_image_release(); and
>      - rbd_dev_image_release() undoes everything else done by
>        rbd_dev_image_probe() (and this includes a call to
>        rbd_dev_unprobe().
>
> This means the image and device portions of an rbd device are fairly
> cleanly separated now, so error paths should be a little easier to
> verify than they used to be.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   78
> +++++++++++++++++++++++++--------------------------
>   1 file changed, 39 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index feaa2e9..595d7b3 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -900,18 +900,6 @@ static void rbd_dev_clear_mapping(struct rbd_device
> *rbd_dev)
>   	rbd_dev->mapping.read_only = true;
>   }
>
> -static void rbd_header_free(struct rbd_image_header *header)
> -{
> -	kfree(header->object_prefix);
> -	header->object_prefix = NULL;
> -	kfree(header->snap_sizes);
> -	header->snap_sizes = NULL;
> -	kfree(header->snap_names);
> -	header->snap_names = NULL;
> -	rbd_snap_context_put(header->snapc);
> -	header->snapc = NULL;
> -}
> -
>   static const char *rbd_segment_name(struct rbd_device *rbd_dev, u64 offset)
>   {
>   	char *name;
> @@ -4588,6 +4576,27 @@ out:
>   	return ret;
>   }
>
> +/* Undo whatever state changes are made by v1 or v2 image probe */
> +
> +static void rbd_dev_unprobe(struct rbd_device *rbd_dev)
> +{
> +	struct rbd_image_header	*header;
> +
> +	rbd_dev_remove_parent(rbd_dev);
> +	rbd_spec_put(rbd_dev->parent_spec);
> +	rbd_dev->parent_spec = NULL;
> +	rbd_dev->parent_overlap = 0;
> +
> +	/* Free dynamic fields from the header, then zero it out */
> +
> +	header = &rbd_dev->header;
> +	rbd_snap_context_put(header->snapc);
> +	kfree(header->snap_sizes);
> +	kfree(header->snap_names);
> +	kfree(header->object_prefix);
> +	memset(header, 0, sizeof (*header));
> +}
> +
>   static int rbd_dev_v1_probe(struct rbd_device *rbd_dev)
>   {
>   	int ret;
> @@ -4809,10 +4818,20 @@ static int rbd_dev_header_name(struct rbd_device
> *rbd_dev)
>
>   static void rbd_dev_image_release(struct rbd_device *rbd_dev)
>   {
> -	rbd_header_free(&rbd_dev->header);
> -	rbd_assert(rbd_dev->rbd_client != NULL);
> -	rbd_spec_put(rbd_dev->parent_spec);
> +	int ret;
> +
> +	rbd_dev_remove_parent(rbd_dev);

This was just moved to rbd_dev_unprobe(), which is called in two lines.
It seems like this call should go away, since rbd_dev_unprobe() is also
used in an error path.

> +	rbd_remove_all_snaps(rbd_dev);
> +	rbd_dev_unprobe(rbd_dev);
> +	ret = rbd_dev_header_watch_sync(rbd_dev, 0);
> +	if (ret)
> +		rbd_warn(rbd_dev, "failed to cancel watch event (%d)\n", ret);
>   	kfree(rbd_dev->header_name);
> +	rbd_dev->header_name = NULL;
> +	rbd_dev->image_format = 0;
> +	kfree(rbd_dev->spec->image_id);
> +	rbd_dev->spec->image_id = NULL;
> +
>   	rbd_dev_destroy(rbd_dev);
>   }
>
> @@ -4854,7 +4873,7 @@ static int rbd_dev_image_probe(struct rbd_device
> *rbd_dev)
>
>   	ret = rbd_dev_snaps_update(rbd_dev);
>   	if (ret)
> -		goto err_out_watch;
> +		goto err_out_probe;
>
>   	ret = rbd_dev_spec_update(rbd_dev);
>   	if (ret)
> @@ -4865,15 +4884,14 @@ static int rbd_dev_image_probe(struct rbd_device
> *rbd_dev)
>   		goto err_out_snaps;
>
>   	ret = rbd_dev_device_setup(rbd_dev);
> -	if (ret)
> -		goto err_out_parent;
> +	if (!ret)
> +		return 0;
>
> -	return ret;
> -err_out_parent:
>   	rbd_dev_remove_parent(rbd_dev);

This call is removed in the next patch, but should probably be removed
here, since rbd_dev_unprobe() calls it now.

> -	rbd_header_free(&rbd_dev->header);
>   err_out_snaps:
>   	rbd_remove_all_snaps(rbd_dev);
> +err_out_probe:
> +	rbd_dev_unprobe(rbd_dev);
>   err_out_watch:
>   	tmp = rbd_dev_header_watch_sync(rbd_dev, 0);
>   	if (tmp)
> @@ -5005,7 +5023,6 @@ static void rbd_dev_remove_parent(struct
> rbd_device *rbd_dev)
>   		struct rbd_device *first = rbd_dev;
>   		struct rbd_device *second = first->parent;
>   		struct rbd_device *third;
> -		int ret;
>
>   		/*
>   		 * Follow to the parent with no grandparent and
> @@ -5016,11 +5033,6 @@ static void rbd_dev_remove_parent(struct
> rbd_device *rbd_dev)
>   			second = third;
>   		}
>   		rbd_assert(second);
> -		ret = rbd_dev_header_watch_sync(rbd_dev, 0);
> -		if (ret)
> -			rbd_warn(rbd_dev,
> -				"failed to cancel watch event (%d)\n", ret);
> -		rbd_remove_all_snaps(second);
>   		rbd_bus_del_dev(second);
>   		first->parent = NULL;
>   		first->parent_overlap = 0;
> @@ -5065,19 +5077,7 @@ static ssize_t rbd_remove(struct bus_type *bus,
>   	spin_unlock_irq(&rbd_dev->lock);
>   	if (ret < 0)
>   		goto done;
> -
> -	ret = rbd_dev_header_watch_sync(rbd_dev, 0);
> -	if (ret) {
> -		rbd_warn(rbd_dev, "failed to cancel watch event (%d)\n", ret);
> -		clear_bit(RBD_DEV_FLAG_REMOVING, &rbd_dev->flags);
> -		smp_mb();
> -		return ret;
> -	}
>   	ret = count;
> -
> -	rbd_dev_remove_parent(rbd_dev);
> -
> -	rbd_remove_all_snaps(rbd_dev);
>   	rbd_bus_del_dev(rbd_dev);
>   	module_put(THIS_MODULE);
>   done:
>

Re: [PATCH 0/5] rbd: only set up Linux files for mapped devices

[Josh Durgin]

On 04/29/2013 10:59 AM, Alex Elder wrote:
> This series does a few more things to separate the
> ceph and Linux side of rbd device initialization,
> and to make sure things get cleaned up in a sane
> way when errors occur.  The last patch makes it so
> only the rbd image getting mapped has Linux device
> and sysfs stuff associated with them.  The parent
> rbd devices for layered images never needed this
> stuff, and things have now been rearranged such
> that we can do away with that.
>
> These patches are available in the ceph-client git
> repository in the branch "review/wip-rbd-cleanup-5".
>
> 					-Alex
> [PATCH 1/5] rbd: drop module later
> [PATCH 2/5] rbd: don't destroy rbd_dev in device release function
> [PATCH 3/5] rbd: define rbd_dev_unprobe()
> [PATCH 4/5] rbd: don't have device release destroy rbd_dev
> [PATCH 5/5] rbd: set up devices only for mapped images

A couple notes on 3/5, otherwise looks good.

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

Re: [PATCH 3/5] rbd: define rbd_dev_unprobe()

[Alex Elder]

On 04/30/2013 08:11 PM, Josh Durgin wrote:
> There are a couple calls that look unnecessary noted below.

You were correct, in the two places that you mentioned,
rbd_parent_remove() should go away.  I'll do that before
I commit.

					-Alex

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