rbd: wrap up of initialization rework

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 <[email protected]>

---
  drivers/block/rbd.c |   33 +++++++++++++++------------------
  1 file changed, 15 insertions(+), 18 deletions(-)

drivers/block/rbd.c +4 -10

index be85d92..a528d4c 100644
@@ -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;
  }
 

/*

drivers/block/rbd.c +0 -0

@@ -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.
*/

drivers/block/rbd.c +11 -8

-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);

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 <[email protected]>

---
  drivers/block/rbd.c |   37 ++++++++++++++++++-------------------
  1 file changed, 18 insertions(+), 19 deletions(-)

drivers/block/rbd.c +18 -19

index a528d4c..4771de2 100644
@@ -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;
  }

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 <[email protected]>

---
  drivers/block/rbd.c |   62
++++++++++++++++++++++++++++++++++-----------------
  1 file changed, 41 insertions(+), 21 deletions(-)

drivers/block/rbd.c +41 -21

index 4771de2..a8ad8f8 100644
@@ -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);

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 <[email protected]>

---
  drivers/block/rbd.c |  161
+++++++++++++++++++++++++++------------------------
  1 file changed, 86 insertions(+), 75 deletions(-)

drivers/block/rbd.c +86 -75

index a8ad8f8..8d26c0f 100644
@@ -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: