rbd: wrap up of initialization rework

rbd: wrap up of initialization rework

[Alex Elder]

These four patches sort of finish out this bunch of patches
that have been reworking how rbd initialization of devices
is done.  With this foundation in place, probing for
parents of layered device fits in fairly neatly.

[PATCH 1/4] rbd: don't pass rbd_dev to rbd_get_client()
    This makes getting a reference to a client not depend
    on a particular rbd_dev.
[PATCH 2/4] rbd: consolidate rbd_dev init in rbd_add()
    This just moves some code around to make the next patch
    simpler.
[PATCH 3/4] rbd: define rbd_dev_{create,destroy}() helpers
    These helpers isolate some initialization and make
    cleanup in error paths easier.
[PATCH 4/4] rbd: encapsulate last part of probe
    This moves a big hunk of code into its own function,
    leaving rbd_add() much shorter and easier to follow.

					-Alex

[PATCH 1/4] rbd: don't pass rbd_dev to rbd_get_client()

[Alex Elder]

The only reason rbd_dev is passed to rbd_get_client() is so its
rbd_client field can get assigned.  Instead, just return the
rbd_client pointer as a result and have the caller do the
assignment.

Change rbd_put_client() so it takes an rbd_client structure,
so follows the more typical symmetry with rbd_get_client().

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index be85d92..a528d4c 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -467,23 +467,17 @@ static int parse_rbd_opts_token(char *c, void
*private)
  * Get a ceph client with specific addr and configuration, if one does
  * not exist create it.
  */
-static int rbd_get_client(struct rbd_device *rbd_dev,
-				struct ceph_options *ceph_opts)
+static struct rbd_client *rbd_get_client(struct ceph_options *ceph_opts)
 {
 	struct rbd_client *rbdc;

 	rbdc = rbd_client_find(ceph_opts);
-	if (rbdc) {
-		/* using an existing client */
+	if (rbdc)	/* using an existing client */
 		ceph_destroy_options(ceph_opts);
-	} else {
+	else
 		rbdc = rbd_client_create(ceph_opts);
-		if (IS_ERR(rbdc))
-			return PTR_ERR(rbdc);
-	}
-	rbd_dev->rbd_client = rbdc;

-	return 0;
+	return rbdc;
 }

 /*
@@ -508,10 +502,9 @@ static void rbd_client_release(struct kref *kref)
  * Drop reference to ceph client node. If it's not referenced anymore,
release
  * it.
  */
-static void rbd_put_client(struct rbd_device *rbd_dev)
+static void rbd_put_client(struct rbd_client *rbdc)
 {
-	kref_put(&rbd_dev->rbd_client->kref, rbd_client_release);
-	rbd_dev->rbd_client = NULL;
+	kref_put(&rbdc->kref, rbd_client_release);
 }

 /*
@@ -3232,6 +3225,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	struct ceph_options *ceph_opts = NULL;
 	struct rbd_options *rbd_opts = NULL;
 	struct rbd_spec *spec = NULL;
+	struct rbd_client *rbdc;
 	struct ceph_osd_client *osdc;
 	int rc = -ENOMEM;

@@ -3255,13 +3249,16 @@ static ssize_t rbd_add(struct bus_type *bus,

 	rbd_dev->mapping.read_only = rbd_opts->read_only;

-	rc = rbd_get_client(rbd_dev, ceph_opts);
-	if (rc < 0)
+	rbdc = rbd_get_client(ceph_opts);
+	if (IS_ERR(rbdc)) {
+		rc = PTR_ERR(rbdc);
 		goto err_out_args;
+	}
+	rbd_dev->rbd_client = rbdc;
 	ceph_opts = NULL;	/* ceph_opts now owned by rbd_dev client */

 	/* pick the pool */
-	osdc = &rbd_dev->rbd_client->client->osdc;
+	osdc = &rbdc->client->osdc;
 	rc = ceph_pg_poolid_by_name(osdc->osdmap, spec->pool_name);
 	if (rc < 0)
 		goto err_out_client;
@@ -3353,7 +3350,7 @@ err_out_probe:
 	rbd_header_free(&rbd_dev->header);
 err_out_client:
 	kfree(rbd_dev->header_name);
-	rbd_put_client(rbd_dev);
+	rbd_put_client(rbdc);
 err_out_args:
 	if (ceph_opts)
 		ceph_destroy_options(ceph_opts);
@@ -3398,7 +3395,7 @@ static void rbd_dev_release(struct device *dev)
 	if (rbd_dev->watch_event)
 		rbd_req_sync_unwatch(rbd_dev);

-	rbd_put_client(rbd_dev);
+	rbd_put_client(rbd_dev->rbd_client);

 	/* clean up and free blkdev */
 	rbd_free_disk(rbd_dev);
-- 
1.7.9.5

[PATCH 2/4] rbd: consolidate rbd_dev init in rbd_add()

[Alex Elder]

Group the allocation and initialization of fields of the rbd device
structure created in rbd_add().  Move the grouped code down later in
the function, just prior to the call to rbd_dev_probe().  This is
for the most part simple code movement.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index a528d4c..4771de2 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3232,29 +3232,16 @@ static ssize_t rbd_add(struct bus_type *bus,
 	if (!try_module_get(THIS_MODULE))
 		return -ENODEV;

-	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
-	if (!rbd_dev)
-		return -ENOMEM;
-
-	/* static rbd_device initialization */
-	spin_lock_init(&rbd_dev->lock);
-	INIT_LIST_HEAD(&rbd_dev->node);
-	INIT_LIST_HEAD(&rbd_dev->snaps);
-	init_rwsem(&rbd_dev->header_rwsem);
-
 	/* parse add command */
 	rc = rbd_add_parse_args(buf, &ceph_opts, &rbd_opts, &spec);
 	if (rc < 0)
-		goto err_out_mem;
-
-	rbd_dev->mapping.read_only = rbd_opts->read_only;
+		goto err_out_module;

 	rbdc = rbd_get_client(ceph_opts);
 	if (IS_ERR(rbdc)) {
 		rc = PTR_ERR(rbdc);
 		goto err_out_args;
 	}
-	rbd_dev->rbd_client = rbdc;
 	ceph_opts = NULL;	/* ceph_opts now owned by rbd_dev client */

 	/* pick the pool */
@@ -3264,11 +3251,22 @@ static ssize_t rbd_add(struct bus_type *bus,
 		goto err_out_client;
 	spec->pool_id = (u64) rc;

+	rbd_dev = kzalloc(sizeof (*rbd_dev), GFP_KERNEL);
+	if (!rbd_dev)
+		goto err_out_client;
+
+	spin_lock_init(&rbd_dev->lock);
+	INIT_LIST_HEAD(&rbd_dev->node);
+	INIT_LIST_HEAD(&rbd_dev->snaps);
+	init_rwsem(&rbd_dev->header_rwsem);
+	rbd_dev->rbd_client = rbdc;
 	rbd_dev->spec = spec;

+	rbd_dev->mapping.read_only = rbd_opts->read_only;
+
 	rc = rbd_dev_probe(rbd_dev);
 	if (rc < 0)
-		goto err_out_client;
+		goto err_out_mem;

 	/* no need to lock here, as rbd_dev is not registered yet */
 	rc = rbd_dev_snaps_update(rbd_dev);
@@ -3348,19 +3346,20 @@ err_out_snaps:
 	rbd_remove_all_snaps(rbd_dev);
 err_out_probe:
 	rbd_header_free(&rbd_dev->header);
-err_out_client:
 	kfree(rbd_dev->header_name);
+err_out_mem:
+	kfree(rbd_dev);
+err_out_client:
 	rbd_put_client(rbdc);
 err_out_args:
 	if (ceph_opts)
 		ceph_destroy_options(ceph_opts);
 	kfree(rbd_opts);
 	rbd_spec_put(spec);
-err_out_mem:
-	kfree(rbd_dev);
+err_out_module:
+	module_put(THIS_MODULE);

 	dout("Error adding device %s\n", buf);
-	module_put(THIS_MODULE);

 	return (ssize_t) rc;
 }
-- 
1.7.9.5

[PATCH 3/4] rbd: define rbd_dev_{create,destroy}() helpers

[Alex Elder]

Encapsulate the creation/initialization and destruction of rbd
device structures.  The rbd_client and the rbd_spec structures
provided on creation hold references whose ownership is transferred
to the new rbd_device structure.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 4771de2..a8ad8f8 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -504,7 +504,8 @@ static void rbd_client_release(struct kref *kref)
  */
 static void rbd_put_client(struct rbd_client *rbdc)
 {
-	kref_put(&rbdc->kref, rbd_client_release);
+	if (rbdc)
+		kref_put(&rbdc->kref, rbd_client_release);
 }

 /*
@@ -2166,6 +2167,34 @@ static void rbd_spec_free(struct kref *kref)
 	kfree(spec);
 }

+struct rbd_device *rbd_dev_create(struct rbd_client *rbdc,
+				struct rbd_spec *spec)
+{
+	struct rbd_device *rbd_dev;
+
+	rbd_dev = kzalloc(sizeof (*rbd_dev), GFP_KERNEL);
+	if (!rbd_dev)
+		return NULL;
+
+	spin_lock_init(&rbd_dev->lock);
+	INIT_LIST_HEAD(&rbd_dev->node);
+	INIT_LIST_HEAD(&rbd_dev->snaps);
+	init_rwsem(&rbd_dev->header_rwsem);
+
+	rbd_dev->spec = spec;
+	rbd_dev->rbd_client = rbdc;
+
+	return rbd_dev;
+}
+
+static void rbd_dev_destroy(struct rbd_device *rbd_dev)
+{
+	kfree(rbd_dev->header_name);
+	rbd_put_client(rbd_dev->rbd_client);
+	rbd_spec_put(rbd_dev->spec);
+	kfree(rbd_dev);
+}
+
 static bool rbd_snap_registered(struct rbd_snap *snap)
 {
 	bool ret = snap->dev.type == &rbd_snap_device_type;
@@ -3242,7 +3271,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 		rc = PTR_ERR(rbdc);
 		goto err_out_args;
 	}
-	ceph_opts = NULL;	/* ceph_opts now owned by rbd_dev client */
+	ceph_opts = NULL;	/* rbd_dev client now owns this */

 	/* pick the pool */
 	osdc = &rbdc->client->osdc;
@@ -3251,22 +3280,19 @@ static ssize_t rbd_add(struct bus_type *bus,
 		goto err_out_client;
 	spec->pool_id = (u64) rc;

-	rbd_dev = kzalloc(sizeof (*rbd_dev), GFP_KERNEL);
+	rbd_dev = rbd_dev_create(rbdc, spec);
 	if (!rbd_dev)
 		goto err_out_client;
-
-	spin_lock_init(&rbd_dev->lock);
-	INIT_LIST_HEAD(&rbd_dev->node);
-	INIT_LIST_HEAD(&rbd_dev->snaps);
-	init_rwsem(&rbd_dev->header_rwsem);
-	rbd_dev->rbd_client = rbdc;
-	rbd_dev->spec = spec;
+	rbdc = NULL;		/* rbd_dev now owns this */
+	spec = NULL;		/* rbd_dev now owns this */

 	rbd_dev->mapping.read_only = rbd_opts->read_only;
+	kfree(rbd_opts);
+	rbd_opts = NULL;	/* done with this */

 	rc = rbd_dev_probe(rbd_dev);
 	if (rc < 0)
-		goto err_out_mem;
+		goto err_out_rbd_dev;

 	/* no need to lock here, as rbd_dev is not registered yet */
 	rc = rbd_dev_snaps_update(rbd_dev);
@@ -3317,8 +3343,6 @@ static ssize_t rbd_add(struct bus_type *bus,
 	if (rc)
 		goto err_out_bus;

-	kfree(rbd_opts);
-
 	/* Everything's ready.  Announce the disk to the world. */

 	add_disk(rbd_dev->disk);
@@ -3332,7 +3356,6 @@ err_out_bus:
 	/* this will also clean up rest of rbd_dev stuff */

 	rbd_bus_del_dev(rbd_dev);
-	kfree(rbd_opts);

 	return rc;

@@ -3346,9 +3369,8 @@ err_out_snaps:
 	rbd_remove_all_snaps(rbd_dev);
 err_out_probe:
 	rbd_header_free(&rbd_dev->header);
-	kfree(rbd_dev->header_name);
-err_out_mem:
-	kfree(rbd_dev);
+err_out_rbd_dev:
+	rbd_dev_destroy(rbd_dev);
 err_out_client:
 	rbd_put_client(rbdc);
 err_out_args:
@@ -3394,7 +3416,6 @@ static void rbd_dev_release(struct device *dev)
 	if (rbd_dev->watch_event)
 		rbd_req_sync_unwatch(rbd_dev);

-	rbd_put_client(rbd_dev->rbd_client);

 	/* clean up and free blkdev */
 	rbd_free_disk(rbd_dev);
@@ -3404,10 +3425,9 @@ static void rbd_dev_release(struct device *dev)
 	rbd_header_free(&rbd_dev->header);

 	/* done with the id, and with the rbd_dev */
-	kfree(rbd_dev->header_name);
 	rbd_dev_id_put(rbd_dev);
-	rbd_spec_put(rbd_dev->spec);
-	kfree(rbd_dev);
+	rbd_assert(rbd_dev->rbd_client != NULL);
+	rbd_dev_destroy(rbd_dev);

 	/* release module ref */
 	module_put(THIS_MODULE);
-- 
1.7.9.5

[PATCH 4/4] rbd: encapsulate last part of probe

[Alex Elder]

Group the activities that now take place after an rbd_dev_probe()
call into a single function, and move the call to that function
into rbd_dev_probe() itself.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index a8ad8f8..8d26c0f 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3221,6 +3221,84 @@ out_err:
 	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 */
+	ret = rbd_dev_snaps_update(rbd_dev);
+	if (ret)
+		return ret;
+
+	ret = rbd_dev_set_mapping(rbd_dev);
+	if (ret)
+		goto err_out_snaps;
+
+	/* generate unique id: find highest unique id, add one */
+	rbd_dev_id_get(rbd_dev);
+
+	/* Fill in the device name, now that we have its id. */
+	BUILD_BUG_ON(DEV_NAME_LEN
+			< sizeof (RBD_DRV_NAME) + MAX_INT_FORMAT_WIDTH);
+	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->dev_id);
+
+	/* Get our block major device number. */
+
+	ret = register_blkdev(0, rbd_dev->name);
+	if (ret < 0)
+		goto err_out_id;
+	rbd_dev->major = ret;
+
+	/* Set up the blkdev mapping. */
+
+	ret = rbd_init_disk(rbd_dev);
+	if (ret)
+		goto err_out_blkdev;
+
+	ret = rbd_bus_add_dev(rbd_dev);
+	if (ret)
+		goto err_out_disk;
+
+	/*
+	 * At this point cleanup in the event of an error is the job
+	 * of the sysfs code (initiated by rbd_bus_del_dev()).
+	 */
+	down_write(&rbd_dev->header_rwsem);
+	ret = rbd_dev_snaps_register(rbd_dev);
+	up_write(&rbd_dev->header_rwsem);
+	if (ret)
+		goto err_out_bus;
+
+	ret = rbd_init_watch_dev(rbd_dev);
+	if (ret)
+		goto err_out_bus;
+
+	/* Everything's ready.  Announce the disk to the world. */
+
+	add_disk(rbd_dev->disk);
+
+	pr_info("%s: added with size 0x%llx\n", rbd_dev->disk->disk_name,
+		(unsigned long long) rbd_dev->mapping.size);
+
+	return ret;
+err_out_bus:
+	/* this will also clean up rest of rbd_dev stuff */
+
+	rbd_bus_del_dev(rbd_dev);
+
+	return ret;
+err_out_disk:
+	rbd_free_disk(rbd_dev);
+err_out_blkdev:
+	unregister_blkdev(rbd_dev->major, rbd_dev->name);
+err_out_id:
+	rbd_dev_id_put(rbd_dev);
+err_out_snaps:
+	rbd_remove_all_snaps(rbd_dev);
+
+	return ret;
+}
+
 /*
  * Probe for the existence of the header object for the given rbd
  * device.  For format 2 images this includes determining the image
@@ -3240,9 +3318,16 @@ static int rbd_dev_probe(struct rbd_device *rbd_dev)
 		ret = rbd_dev_v1_probe(rbd_dev);
 	else
 		ret = rbd_dev_v2_probe(rbd_dev);
-	if (ret)
+	if (ret) {
 		dout("probe failed, returning %d\n", ret);

+		return ret;
+	}
+
+	ret = rbd_dev_probe_finish(rbd_dev);
+	if (ret)
+		rbd_header_free(&rbd_dev->header);
+
 	return ret;
 }

@@ -3294,81 +3379,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	if (rc < 0)
 		goto err_out_rbd_dev;

-	/* no need to lock here, as rbd_dev is not registered yet */
-	rc = rbd_dev_snaps_update(rbd_dev);
-	if (rc)
-		goto err_out_probe;
-
-	rc = rbd_dev_set_mapping(rbd_dev);
-	if (rc)
-		goto err_out_snaps;
-
-	/* generate unique id: find highest unique id, add one */
-	rbd_dev_id_get(rbd_dev);
-
-	/* Fill in the device name, now that we have its id. */
-	BUILD_BUG_ON(DEV_NAME_LEN
-			< sizeof (RBD_DRV_NAME) + MAX_INT_FORMAT_WIDTH);
-	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->dev_id);
-
-	/* Get our block major device number. */
-
-	rc = register_blkdev(0, rbd_dev->name);
-	if (rc < 0)
-		goto err_out_id;
-	rbd_dev->major = rc;
-
-	/* Set up the blkdev mapping. */
-
-	rc = rbd_init_disk(rbd_dev);
-	if (rc)
-		goto err_out_blkdev;
-
-	rc = rbd_bus_add_dev(rbd_dev);
-	if (rc)
-		goto err_out_disk;
-
-	/*
-	 * At this point cleanup in the event of an error is the job
-	 * of the sysfs code (initiated by rbd_bus_del_dev()).
-	 */
-
-	down_write(&rbd_dev->header_rwsem);
-	rc = rbd_dev_snaps_register(rbd_dev);
-	up_write(&rbd_dev->header_rwsem);
-	if (rc)
-		goto err_out_bus;
-
-	rc = rbd_init_watch_dev(rbd_dev);
-	if (rc)
-		goto err_out_bus;
-
-	/* Everything's ready.  Announce the disk to the world. */
-
-	add_disk(rbd_dev->disk);
-
-	pr_info("%s: added with size 0x%llx\n", rbd_dev->disk->disk_name,
-		(unsigned long long) rbd_dev->mapping.size);
-
 	return count;
-
-err_out_bus:
-	/* this will also clean up rest of rbd_dev stuff */
-
-	rbd_bus_del_dev(rbd_dev);
-
-	return rc;
-
-err_out_disk:
-	rbd_free_disk(rbd_dev);
-err_out_blkdev:
-	unregister_blkdev(rbd_dev->major, rbd_dev->name);
-err_out_id:
-	rbd_dev_id_put(rbd_dev);
-err_out_snaps:
-	rbd_remove_all_snaps(rbd_dev);
-err_out_probe:
-	rbd_header_free(&rbd_dev->header);
 err_out_rbd_dev:
 	rbd_dev_destroy(rbd_dev);
 err_out_client:
-- 
1.7.9.5

Re: [PATCH 1/4] rbd: don't pass rbd_dev to rbd_get_client()

[Josh Durgin]

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

On 10/30/2012 02:14 PM, Alex Elder wrote:
> The only reason rbd_dev is passed to rbd_get_client() is so its
> rbd_client field can get assigned.  Instead, just return the
> rbd_client pointer as a result and have the caller do the
> assignment.
>
> Change rbd_put_client() so it takes an rbd_client structure,
> so follows the more typical symmetry with rbd_get_client().
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   33 +++++++++++++++------------------
>   1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index be85d92..a528d4c 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -467,23 +467,17 @@ static int parse_rbd_opts_token(char *c, void
> *private)
>    * Get a ceph client with specific addr and configuration, if one does
>    * not exist create it.
>    */
> -static int rbd_get_client(struct rbd_device *rbd_dev,
> -				struct ceph_options *ceph_opts)
> +static struct rbd_client *rbd_get_client(struct ceph_options *ceph_opts)
>   {
>   	struct rbd_client *rbdc;
>
>   	rbdc = rbd_client_find(ceph_opts);
> -	if (rbdc) {
> -		/* using an existing client */
> +	if (rbdc)	/* using an existing client */
>   		ceph_destroy_options(ceph_opts);
> -	} else {
> +	else
>   		rbdc = rbd_client_create(ceph_opts);
> -		if (IS_ERR(rbdc))
> -			return PTR_ERR(rbdc);
> -	}
> -	rbd_dev->rbd_client = rbdc;
>
> -	return 0;
> +	return rbdc;
>   }
>
>   /*
> @@ -508,10 +502,9 @@ static void rbd_client_release(struct kref *kref)
>    * Drop reference to ceph client node. If it's not referenced anymore,
> release
>    * it.
>    */
> -static void rbd_put_client(struct rbd_device *rbd_dev)
> +static void rbd_put_client(struct rbd_client *rbdc)
>   {
> -	kref_put(&rbd_dev->rbd_client->kref, rbd_client_release);
> -	rbd_dev->rbd_client = NULL;
> +	kref_put(&rbdc->kref, rbd_client_release);
>   }
>
>   /*
> @@ -3232,6 +3225,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	struct ceph_options *ceph_opts = NULL;
>   	struct rbd_options *rbd_opts = NULL;
>   	struct rbd_spec *spec = NULL;
> +	struct rbd_client *rbdc;
>   	struct ceph_osd_client *osdc;
>   	int rc = -ENOMEM;
>
> @@ -3255,13 +3249,16 @@ static ssize_t rbd_add(struct bus_type *bus,
>
>   	rbd_dev->mapping.read_only = rbd_opts->read_only;
>
> -	rc = rbd_get_client(rbd_dev, ceph_opts);
> -	if (rc < 0)
> +	rbdc = rbd_get_client(ceph_opts);
> +	if (IS_ERR(rbdc)) {
> +		rc = PTR_ERR(rbdc);
>   		goto err_out_args;
> +	}
> +	rbd_dev->rbd_client = rbdc;
>   	ceph_opts = NULL;	/* ceph_opts now owned by rbd_dev client */
>
>   	/* pick the pool */
> -	osdc = &rbd_dev->rbd_client->client->osdc;
> +	osdc = &rbdc->client->osdc;
>   	rc = ceph_pg_poolid_by_name(osdc->osdmap, spec->pool_name);
>   	if (rc < 0)
>   		goto err_out_client;
> @@ -3353,7 +3350,7 @@ err_out_probe:
>   	rbd_header_free(&rbd_dev->header);
>   err_out_client:
>   	kfree(rbd_dev->header_name);
> -	rbd_put_client(rbd_dev);
> +	rbd_put_client(rbdc);
>   err_out_args:
>   	if (ceph_opts)
>   		ceph_destroy_options(ceph_opts);
> @@ -3398,7 +3395,7 @@ static void rbd_dev_release(struct device *dev)
>   	if (rbd_dev->watch_event)
>   		rbd_req_sync_unwatch(rbd_dev);
>
> -	rbd_put_client(rbd_dev);
> +	rbd_put_client(rbd_dev->rbd_client);
>
>   	/* clean up and free blkdev */
>   	rbd_free_disk(rbd_dev);
>

Re: [PATCH 2/4] rbd: consolidate rbd_dev init in rbd_add()

[Josh Durgin]

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

On 10/30/2012 02:14 PM, Alex Elder wrote:
> Group the allocation and initialization of fields of the rbd device
> structure created in rbd_add().  Move the grouped code down later in
> the function, just prior to the call to rbd_dev_probe().  This is
> for the most part simple code movement.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   37 ++++++++++++++++++-------------------
>   1 file changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index a528d4c..4771de2 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -3232,29 +3232,16 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	if (!try_module_get(THIS_MODULE))
>   		return -ENODEV;
>
> -	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
> -	if (!rbd_dev)
> -		return -ENOMEM;
> -
> -	/* static rbd_device initialization */
> -	spin_lock_init(&rbd_dev->lock);
> -	INIT_LIST_HEAD(&rbd_dev->node);
> -	INIT_LIST_HEAD(&rbd_dev->snaps);
> -	init_rwsem(&rbd_dev->header_rwsem);
> -
>   	/* parse add command */
>   	rc = rbd_add_parse_args(buf, &ceph_opts, &rbd_opts, &spec);
>   	if (rc < 0)
> -		goto err_out_mem;
> -
> -	rbd_dev->mapping.read_only = rbd_opts->read_only;
> +		goto err_out_module;
>
>   	rbdc = rbd_get_client(ceph_opts);
>   	if (IS_ERR(rbdc)) {
>   		rc = PTR_ERR(rbdc);
>   		goto err_out_args;
>   	}
> -	rbd_dev->rbd_client = rbdc;
>   	ceph_opts = NULL;	/* ceph_opts now owned by rbd_dev client */
>
>   	/* pick the pool */
> @@ -3264,11 +3251,22 @@ static ssize_t rbd_add(struct bus_type *bus,
>   		goto err_out_client;
>   	spec->pool_id = (u64) rc;
>
> +	rbd_dev = kzalloc(sizeof (*rbd_dev), GFP_KERNEL);
> +	if (!rbd_dev)
> +		goto err_out_client;
> +
> +	spin_lock_init(&rbd_dev->lock);
> +	INIT_LIST_HEAD(&rbd_dev->node);
> +	INIT_LIST_HEAD(&rbd_dev->snaps);
> +	init_rwsem(&rbd_dev->header_rwsem);
> +	rbd_dev->rbd_client = rbdc;
>   	rbd_dev->spec = spec;
>
> +	rbd_dev->mapping.read_only = rbd_opts->read_only;
> +
>   	rc = rbd_dev_probe(rbd_dev);
>   	if (rc < 0)
> -		goto err_out_client;
> +		goto err_out_mem;
>
>   	/* no need to lock here, as rbd_dev is not registered yet */
>   	rc = rbd_dev_snaps_update(rbd_dev);
> @@ -3348,19 +3346,20 @@ err_out_snaps:
>   	rbd_remove_all_snaps(rbd_dev);
>   err_out_probe:
>   	rbd_header_free(&rbd_dev->header);
> -err_out_client:
>   	kfree(rbd_dev->header_name);
> +err_out_mem:
> +	kfree(rbd_dev);
> +err_out_client:
>   	rbd_put_client(rbdc);
>   err_out_args:
>   	if (ceph_opts)
>   		ceph_destroy_options(ceph_opts);
>   	kfree(rbd_opts);
>   	rbd_spec_put(spec);
> -err_out_mem:
> -	kfree(rbd_dev);
> +err_out_module:
> +	module_put(THIS_MODULE);
>
>   	dout("Error adding device %s\n", buf);
> -	module_put(THIS_MODULE);
>
>   	return (ssize_t) rc;
>   }
>

Re: [PATCH 3/4] rbd: define rbd_dev_{create,destroy}() helpers

[Josh Durgin]

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

On 10/30/2012 02:14 PM, Alex Elder wrote:
> Encapsulate the creation/initialization and destruction of rbd
> device structures.  The rbd_client and the rbd_spec structures
> provided on creation hold references whose ownership is transferred
> to the new rbd_device structure.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   62
> ++++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 41 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 4771de2..a8ad8f8 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -504,7 +504,8 @@ static void rbd_client_release(struct kref *kref)
>    */
>   static void rbd_put_client(struct rbd_client *rbdc)
>   {
> -	kref_put(&rbdc->kref, rbd_client_release);
> +	if (rbdc)
> +		kref_put(&rbdc->kref, rbd_client_release);
>   }
>
>   /*
> @@ -2166,6 +2167,34 @@ static void rbd_spec_free(struct kref *kref)
>   	kfree(spec);
>   }
>
> +struct rbd_device *rbd_dev_create(struct rbd_client *rbdc,
> +				struct rbd_spec *spec)
> +{
> +	struct rbd_device *rbd_dev;
> +
> +	rbd_dev = kzalloc(sizeof (*rbd_dev), GFP_KERNEL);
> +	if (!rbd_dev)
> +		return NULL;
> +
> +	spin_lock_init(&rbd_dev->lock);
> +	INIT_LIST_HEAD(&rbd_dev->node);
> +	INIT_LIST_HEAD(&rbd_dev->snaps);
> +	init_rwsem(&rbd_dev->header_rwsem);
> +
> +	rbd_dev->spec = spec;
> +	rbd_dev->rbd_client = rbdc;
> +
> +	return rbd_dev;
> +}
> +
> +static void rbd_dev_destroy(struct rbd_device *rbd_dev)
> +{
> +	kfree(rbd_dev->header_name);
> +	rbd_put_client(rbd_dev->rbd_client);
> +	rbd_spec_put(rbd_dev->spec);
> +	kfree(rbd_dev);
> +}
> +
>   static bool rbd_snap_registered(struct rbd_snap *snap)
>   {
>   	bool ret = snap->dev.type == &rbd_snap_device_type;
> @@ -3242,7 +3271,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   		rc = PTR_ERR(rbdc);
>   		goto err_out_args;
>   	}
> -	ceph_opts = NULL;	/* ceph_opts now owned by rbd_dev client */
> +	ceph_opts = NULL;	/* rbd_dev client now owns this */
>
>   	/* pick the pool */
>   	osdc = &rbdc->client->osdc;
> @@ -3251,22 +3280,19 @@ static ssize_t rbd_add(struct bus_type *bus,
>   		goto err_out_client;
>   	spec->pool_id = (u64) rc;
>
> -	rbd_dev = kzalloc(sizeof (*rbd_dev), GFP_KERNEL);
> +	rbd_dev = rbd_dev_create(rbdc, spec);
>   	if (!rbd_dev)
>   		goto err_out_client;
> -
> -	spin_lock_init(&rbd_dev->lock);
> -	INIT_LIST_HEAD(&rbd_dev->node);
> -	INIT_LIST_HEAD(&rbd_dev->snaps);
> -	init_rwsem(&rbd_dev->header_rwsem);
> -	rbd_dev->rbd_client = rbdc;
> -	rbd_dev->spec = spec;
> +	rbdc = NULL;		/* rbd_dev now owns this */
> +	spec = NULL;		/* rbd_dev now owns this */
>
>   	rbd_dev->mapping.read_only = rbd_opts->read_only;
> +	kfree(rbd_opts);
> +	rbd_opts = NULL;	/* done with this */
>
>   	rc = rbd_dev_probe(rbd_dev);
>   	if (rc < 0)
> -		goto err_out_mem;
> +		goto err_out_rbd_dev;
>
>   	/* no need to lock here, as rbd_dev is not registered yet */
>   	rc = rbd_dev_snaps_update(rbd_dev);
> @@ -3317,8 +3343,6 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	if (rc)
>   		goto err_out_bus;
>
> -	kfree(rbd_opts);
> -
>   	/* Everything's ready.  Announce the disk to the world. */
>
>   	add_disk(rbd_dev->disk);
> @@ -3332,7 +3356,6 @@ err_out_bus:
>   	/* this will also clean up rest of rbd_dev stuff */
>
>   	rbd_bus_del_dev(rbd_dev);
> -	kfree(rbd_opts);
>
>   	return rc;
>
> @@ -3346,9 +3369,8 @@ err_out_snaps:
>   	rbd_remove_all_snaps(rbd_dev);
>   err_out_probe:
>   	rbd_header_free(&rbd_dev->header);
> -	kfree(rbd_dev->header_name);
> -err_out_mem:
> -	kfree(rbd_dev);
> +err_out_rbd_dev:
> +	rbd_dev_destroy(rbd_dev);
>   err_out_client:
>   	rbd_put_client(rbdc);
>   err_out_args:
> @@ -3394,7 +3416,6 @@ static void rbd_dev_release(struct device *dev)
>   	if (rbd_dev->watch_event)
>   		rbd_req_sync_unwatch(rbd_dev);
>
> -	rbd_put_client(rbd_dev->rbd_client);
>
>   	/* clean up and free blkdev */
>   	rbd_free_disk(rbd_dev);
> @@ -3404,10 +3425,9 @@ static void rbd_dev_release(struct device *dev)
>   	rbd_header_free(&rbd_dev->header);
>
>   	/* done with the id, and with the rbd_dev */
> -	kfree(rbd_dev->header_name);
>   	rbd_dev_id_put(rbd_dev);
> -	rbd_spec_put(rbd_dev->spec);
> -	kfree(rbd_dev);
> +	rbd_assert(rbd_dev->rbd_client != NULL);
> +	rbd_dev_destroy(rbd_dev);
>
>   	/* release module ref */
>   	module_put(THIS_MODULE);
>

Re: [PATCH 4/4] rbd: encapsulate last part of probe

[Josh Durgin]

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

On 10/30/2012 02:14 PM, Alex Elder wrote:
> Group the activities that now take place after an rbd_dev_probe()
> call into a single function, and move the call to that function
> into rbd_dev_probe() itself.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |  161
> +++++++++++++++++++++++++++------------------------
>   1 file changed, 86 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index a8ad8f8..8d26c0f 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -3221,6 +3221,84 @@ out_err:
>   	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 */
> +	ret = rbd_dev_snaps_update(rbd_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = rbd_dev_set_mapping(rbd_dev);
> +	if (ret)
> +		goto err_out_snaps;
> +
> +	/* generate unique id: find highest unique id, add one */
> +	rbd_dev_id_get(rbd_dev);
> +
> +	/* Fill in the device name, now that we have its id. */
> +	BUILD_BUG_ON(DEV_NAME_LEN
> +			< sizeof (RBD_DRV_NAME) + MAX_INT_FORMAT_WIDTH);
> +	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->dev_id);
> +
> +	/* Get our block major device number. */
> +
> +	ret = register_blkdev(0, rbd_dev->name);
> +	if (ret < 0)
> +		goto err_out_id;
> +	rbd_dev->major = ret;
> +
> +	/* Set up the blkdev mapping. */
> +
> +	ret = rbd_init_disk(rbd_dev);
> +	if (ret)
> +		goto err_out_blkdev;
> +
> +	ret = rbd_bus_add_dev(rbd_dev);
> +	if (ret)
> +		goto err_out_disk;
> +
> +	/*
> +	 * At this point cleanup in the event of an error is the job
> +	 * of the sysfs code (initiated by rbd_bus_del_dev()).
> +	 */
> +	down_write(&rbd_dev->header_rwsem);
> +	ret = rbd_dev_snaps_register(rbd_dev);
> +	up_write(&rbd_dev->header_rwsem);
> +	if (ret)
> +		goto err_out_bus;
> +
> +	ret = rbd_init_watch_dev(rbd_dev);
> +	if (ret)
> +		goto err_out_bus;
> +
> +	/* Everything's ready.  Announce the disk to the world. */
> +
> +	add_disk(rbd_dev->disk);
> +
> +	pr_info("%s: added with size 0x%llx\n", rbd_dev->disk->disk_name,
> +		(unsigned long long) rbd_dev->mapping.size);
> +
> +	return ret;
> +err_out_bus:
> +	/* this will also clean up rest of rbd_dev stuff */
> +
> +	rbd_bus_del_dev(rbd_dev);
> +
> +	return ret;
> +err_out_disk:
> +	rbd_free_disk(rbd_dev);
> +err_out_blkdev:
> +	unregister_blkdev(rbd_dev->major, rbd_dev->name);
> +err_out_id:
> +	rbd_dev_id_put(rbd_dev);
> +err_out_snaps:
> +	rbd_remove_all_snaps(rbd_dev);
> +
> +	return ret;
> +}
> +
>   /*
>    * Probe for the existence of the header object for the given rbd
>    * device.  For format 2 images this includes determining the image
> @@ -3240,9 +3318,16 @@ static int rbd_dev_probe(struct rbd_device *rbd_dev)
>   		ret = rbd_dev_v1_probe(rbd_dev);
>   	else
>   		ret = rbd_dev_v2_probe(rbd_dev);
> -	if (ret)
> +	if (ret) {
>   		dout("probe failed, returning %d\n", ret);
>
> +		return ret;
> +	}
> +
> +	ret = rbd_dev_probe_finish(rbd_dev);
> +	if (ret)
> +		rbd_header_free(&rbd_dev->header);
> +
>   	return ret;
>   }
>
> @@ -3294,81 +3379,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	if (rc < 0)
>   		goto err_out_rbd_dev;
>
> -	/* no need to lock here, as rbd_dev is not registered yet */
> -	rc = rbd_dev_snaps_update(rbd_dev);
> -	if (rc)
> -		goto err_out_probe;
> -
> -	rc = rbd_dev_set_mapping(rbd_dev);
> -	if (rc)
> -		goto err_out_snaps;
> -
> -	/* generate unique id: find highest unique id, add one */
> -	rbd_dev_id_get(rbd_dev);
> -
> -	/* Fill in the device name, now that we have its id. */
> -	BUILD_BUG_ON(DEV_NAME_LEN
> -			< sizeof (RBD_DRV_NAME) + MAX_INT_FORMAT_WIDTH);
> -	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->dev_id);
> -
> -	/* Get our block major device number. */
> -
> -	rc = register_blkdev(0, rbd_dev->name);
> -	if (rc < 0)
> -		goto err_out_id;
> -	rbd_dev->major = rc;
> -
> -	/* Set up the blkdev mapping. */
> -
> -	rc = rbd_init_disk(rbd_dev);
> -	if (rc)
> -		goto err_out_blkdev;
> -
> -	rc = rbd_bus_add_dev(rbd_dev);
> -	if (rc)
> -		goto err_out_disk;
> -
> -	/*
> -	 * At this point cleanup in the event of an error is the job
> -	 * of the sysfs code (initiated by rbd_bus_del_dev()).
> -	 */
> -
> -	down_write(&rbd_dev->header_rwsem);
> -	rc = rbd_dev_snaps_register(rbd_dev);
> -	up_write(&rbd_dev->header_rwsem);
> -	if (rc)
> -		goto err_out_bus;
> -
> -	rc = rbd_init_watch_dev(rbd_dev);
> -	if (rc)
> -		goto err_out_bus;
> -
> -	/* Everything's ready.  Announce the disk to the world. */
> -
> -	add_disk(rbd_dev->disk);
> -
> -	pr_info("%s: added with size 0x%llx\n", rbd_dev->disk->disk_name,
> -		(unsigned long long) rbd_dev->mapping.size);
> -
>   	return count;
> -
> -err_out_bus:
> -	/* this will also clean up rest of rbd_dev stuff */
> -
> -	rbd_bus_del_dev(rbd_dev);
> -
> -	return rc;
> -
> -err_out_disk:
> -	rbd_free_disk(rbd_dev);
> -err_out_blkdev:
> -	unregister_blkdev(rbd_dev->major, rbd_dev->name);
> -err_out_id:
> -	rbd_dev_id_put(rbd_dev);
> -err_out_snaps:
> -	rbd_remove_all_snaps(rbd_dev);
> -err_out_probe:
> -	rbd_header_free(&rbd_dev->header);
>   err_out_rbd_dev:
>   	rbd_dev_destroy(rbd_dev);
>   err_out_client:
>