rbd: rearrange initialization sequence

Move the assignment of the header name for an rbd image a bit later, outside rbd_add_parse_args() and into its caller.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   22 ++++++++++------------
  1 file changed, 10 insertions(+), 12 deletions(-)

drivers/block/rbd.c +0 -9

index 4dff92f..14034e3 100644
@@ -2475,15 +2475,6 @@ static char *rbd_add_parse_args(struct rbd_device
 *rbd_dev,
  	if (!rbd_dev->image_name)
  		goto out_err;
 
-	/* Create the name of the header object */
-
-	rbd_dev->header_name = kmalloc(rbd_dev->image_name_len
-						+ sizeof (RBD_SUFFIX),
-					GFP_KERNEL);
-	if (!rbd_dev->header_name)
-		goto out_err;
-	sprintf(rbd_dev->header_name, "%s%s", rbd_dev->image_name, RBD_SUFFIX);
-
  	/* Snapshot name is optional */
  	len = next_token(&buf);

  	if (!len) {

drivers/block/rbd.c +0 -2

@@ -2501,8 +2492,6 @@ dout("    SNAP_NAME is <%s>, len is %zd\n",
 snap_name, len);
  	return snap_name;
 
  out_err:
-	kfree(rbd_dev->header_name);
-	rbd_dev->header_name = NULL;
  	kfree(rbd_dev->image_name);
  	rbd_dev->image_name = NULL;

  	rbd_dev->image_name_len = 0;

drivers/block/rbd.c +10 -1

@@ -2569,6 +2558,15 @@ static ssize_t rbd_add(struct bus_type *bus,
  		goto err_out_client;
  	rbd_dev->pool_id = rc;
 
+	/* Create the name of the header object */
+
+	rbd_dev->header_name = kmalloc(rbd_dev->image_name_len
+						+ sizeof (RBD_SUFFIX),
+					GFP_KERNEL);
+	if (!rbd_dev->header_name)
+		goto err_out_client;
+	sprintf(rbd_dev->header_name, "%s%s", rbd_dev->image_name, RBD_SUFFIX);
+
  	/* register our block device */
  	rc = register_blkdev(0, rbd_dev->name);
  	if (rc < 0)

@@ -2630,11 +2628,11 @@ err_out_bus:
  err_out_blkdev:
  	unregister_blkdev(rbd_dev->major, rbd_dev->name);
  err_out_client:
+	kfree(rbd_dev->header_name);
  	rbd_put_client(rbd_dev);
  err_put_id:
  	if (rbd_dev->pool_name) {
  		kfree(rbd_dev->mapping.snap_name);
-		kfree(rbd_dev->header_name);
  		kfree(rbd_dev->image_name);
  		kfree(rbd_dev->pool_name);
  	}

When a new snapshot is found in an rbd device's updated snapshot context, __rbd_add_snap_dev() is called to create and insert an entry in the rbd devices list of snapshots. In addition, a Linux device is registered to represent the snapshot.

For version 2 rbd images, it will be undesirable to initialize the device right away. So in anticipation of that, this patch separates the insertion of a snapshot entry in the snaps list from the creation of devices for those snapshots.

To do this, create a new function rbd_dev_snaps_register() which traverses the list of snapshots and calls rbd_register_snap_dev() on any that have not yet been registered.

Rename rbd_dev_snap_devs_update() to be rbd_dev_snaps_update() to better reflect that only the entry in the snaps list and not the snapshot's device is affected by the function.

For now, call rbd_dev_snaps_register() immediately after each call to rbd_dev_snaps_update().

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   58
++++++++++++++++++++++++++++++++++++++++-----------
  1 file changed, 46 insertions(+), 12 deletions(-)

drivers/block/rbd.c +4 -1

index 14034e3..d03fb0f 100644
@@ -204,7 +204,9 @@ static DEFINE_SPINLOCK(rbd_dev_list_lock);
  static LIST_HEAD(rbd_client_list);		/* clients */
  static DEFINE_SPINLOCK(rbd_client_list_lock);
 
-static int rbd_dev_snap_devs_update(struct rbd_device *rbd_dev);
+static int rbd_dev_snaps_update(struct rbd_device *rbd_dev);
+static int rbd_dev_snaps_register(struct rbd_device *rbd_dev);
+
  static void rbd_dev_release(struct device *dev);
  static ssize_t rbd_snap_add(struct device *dev,
  			    struct device_attribute *attr,

@@ -648,6 +650,7 @@ static int rbd_header_set_snap(struct rbd_device
 *rbd_dev, char *snap_name)
  		rbd_dev->mapping.size = rbd_dev->header.image_size;
  		rbd_dev->mapping.snap_exists = false;
  		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
+		ret = 0;
  	} else {
  		ret = snap_by_name(rbd_dev, snap_name);

if (ret < 0)

drivers/block/rbd.c +0 -2

@@ -656,8 +659,6 @@ static int rbd_header_set_snap(struct rbd_device
 *rbd_dev, char *snap_name)
  		rbd_dev->mapping.read_only = true;
  	}
  	rbd_dev->mapping.snap_name = snap_name;
-
-	ret = 0;
  done:
  	return ret;

  }

drivers/block/rbd.c +3 -1

@@ -1840,7 +1841,9 @@ static int __rbd_refresh_header(struct rbd_device
 *rbd_dev, u64 *hver)
  	WARN_ON(strcmp(rbd_dev->header.object_prefix, h.object_prefix));
  	kfree(h.object_prefix);
 
-	ret = rbd_dev_snap_devs_update(rbd_dev);
+	ret = rbd_dev_snaps_update(rbd_dev);
+	if (!ret)
+		ret = rbd_dev_snaps_register(rbd_dev);
 
  	up_write(&rbd_dev->header_rwsem);

@@ -2085,10 +2088,16 @@ static struct device_type rbd_snap_device_type = {
  	.release	= rbd_snap_dev_release,
  };
 

drivers/block/rbd.c +3 -0

+		device_unregister(&snap->dev);
  }
 
  static int rbd_register_snap_dev(struct rbd_snap *snap,

@@ -2101,6 +2110,8 @@ static int rbd_register_snap_dev(struct rbd_snap
 *snap,
  	dev->parent = parent;
  	dev->release = rbd_snap_dev_release;
  	dev_set_name(dev, "snap_%s", snap->name);
+	dout("%s: registering device for snapshot %s\n", __func__, snap->name);
+
  	ret = device_register(dev);
 

return ret;

drivers/block/rbd.c +26 -6

@@ -2123,11 +2134,6 @@ static struct rbd_snap *__rbd_add_snap_dev(struct
 rbd_device *rbd_dev,
 
  	snap->size = rbd_dev->header.snap_sizes[i];
  	snap->id = rbd_dev->header.snapc->snaps[i];
-	if (device_is_registered(&rbd_dev->dev)) {
-		ret = rbd_register_snap_dev(snap, &rbd_dev->dev);
-		if (ret < 0)
-			goto err;
-	}
 
  	return snap;

@@ -2150,7 +2156,7 @@ err:
   * snapshot id, highest id first.  (Snapshots in the rbd_dev's list
   * are also maintained in that order.)
   */
-static int rbd_dev_snap_devs_update(struct rbd_device *rbd_dev)
+static int rbd_dev_snaps_update(struct rbd_device *rbd_dev)
  {
  	struct ceph_snap_context *snapc = rbd_dev->header.snapc;
  	const u32 snap_count = snapc->num_snaps;

@@ -2237,6 +2243,31 @@ static int rbd_dev_snap_devs_update(struct
 rbd_device *rbd_dev)
  	return 0;
  }
 
+/*
+ * Scan the list of snapshots and register the devices for any that
+ * have not already been registered.
+ */
+static int rbd_dev_snaps_register(struct rbd_device *rbd_dev)
+{
+	struct rbd_snap *snap;
+	int ret = 0;
+
+	dout("%s called\n", __func__);
+	if (!device_is_registered(&rbd_dev->dev))
+		return 0;
+
+	list_for_each_entry(snap, &rbd_dev->snaps, node) {
+		if (!rbd_snap_registered(snap)) {
+			ret = rbd_register_snap_dev(snap, &rbd_dev->dev);
+			if (ret < 0)
+				break;
+		}
+	}
+	dout("%s: returning %d\n", __func__, ret);
+
+	return ret;
+}
+
  static int rbd_bus_add_dev(struct rbd_device *rbd_dev)
  {

struct device *dev;

drivers/block/rbd.c +4 -1

@@ -2587,7 +2618,10 @@ static ssize_t rbd_add(struct bus_type *bus,
  	if (rc)
  		goto err_out_unlock;
 
-	rc = rbd_dev_snap_devs_update(rbd_dev);
+	rc = rbd_dev_snaps_update(rbd_dev);
+	if (rc)
+		goto err_out_unlock;
+	rc = rbd_dev_snaps_register(rbd_dev);
  	if (rc)
  		goto err_out_unlock;
 

rbd_header_set_snap() is a simple initialization routine for an rbd device's mapping. It has to be called after the snapshot context for the rbd_dev has been updated, but can be done before snapshot devices have been registered.

Change the name to rbd_dev_set_mapping() to better reflect its purpose, and call it a little sooner, before registering snapshot devices.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |    7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

drivers/block/rbd.c +4 -3

index d03fb0f..d60db25 100644
@@ -640,7 +640,7 @@ static int snap_by_name(struct rbd_device *rbd_dev,
 const char *snap_name)
  	return -ENOENT;
  }
 
-static int rbd_header_set_snap(struct rbd_device *rbd_dev, char *snap_name)
+static int rbd_dev_set_mapping(struct rbd_device *rbd_dev, char *snap_name)
  {
  	int ret;

@@ -2621,11 +2621,12 @@ static ssize_t rbd_add(struct bus_type *bus,
  	rc = rbd_dev_snaps_update(rbd_dev);
  	if (rc)
  		goto err_out_unlock;
-	rc = rbd_dev_snaps_register(rbd_dev);
+
+	rc = rbd_dev_set_mapping(rbd_dev, snap_name);
  	if (rc)
  		goto err_out_unlock;
 
-	rc = rbd_header_set_snap(rbd_dev, snap_name);
+	rc = rbd_dev_snaps_register(rbd_dev);
  	if (rc)
  		goto err_out_unlock;
 

Read the rbd header information and call rbd_dev_set_mapping() earlier--before registering the block device or setting up the sysfs entries for the image. The sysfs entries provide users access to some information that's only available after doing the rbd header initialization, so this will make sure it's valid right away.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   31 +++++++++++++++++--------------
  1 file changed, 17 insertions(+), 14 deletions(-)

drivers/block/rbd.c +17 -14

index d60db25..89d8ed7 100644
@@ -2598,10 +2598,24 @@ static ssize_t rbd_add(struct bus_type *bus,
  		goto err_out_client;
  	sprintf(rbd_dev->header_name, "%s%s", rbd_dev->image_name, RBD_SUFFIX);
 
+	/* Get information about the image being mapped */
+
+	rc = rbd_read_header(rbd_dev, &rbd_dev->header);
+	if (rc)
+		goto err_out_client;
+
+	rc = rbd_dev_snaps_update(rbd_dev);
+	if (rc)
+		goto err_out_header;
+
+	rc = rbd_dev_set_mapping(rbd_dev, snap_name);
+	if (rc)
+		goto err_out_header;
+
  	/* register our block device */
  	rc = register_blkdev(0, rbd_dev->name);
  	if (rc < 0)
-		goto err_out_client;
+		goto err_out_header;
  	rbd_dev->major = rc;
 
  	rc = rbd_bus_add_dev(rbd_dev);

@@ -2613,19 +2627,6 @@ static ssize_t rbd_add(struct bus_type *bus,
  	 * of the sysfs code (initiated by rbd_bus_del_dev()).
  	 */
 
-	/* contact OSD, request size info about the object being mapped */
-	rc = rbd_read_header(rbd_dev, &rbd_dev->header);
-	if (rc)
-		goto err_out_unlock;
-
-	rc = rbd_dev_snaps_update(rbd_dev);
-	if (rc)
-		goto err_out_unlock;
-
-	rc = rbd_dev_set_mapping(rbd_dev, snap_name);
-	if (rc)
-		goto err_out_unlock;
-
  	rc = rbd_dev_snaps_register(rbd_dev);
  	if (rc)
  		goto err_out_unlock;

@@ -2662,6 +2663,8 @@ err_out_bus:
 
  err_out_blkdev:
  	unregister_blkdev(rbd_dev->major, rbd_dev->name);
+err_out_header:
+	rbd_header_free(&rbd_dev->header);
  err_out_client:
  	kfree(rbd_dev->header_name);
  	rbd_put_client(rbd_dev);

Hold off setting the device id and formatting the device name in rbd_add() until just before it's needed.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   44 ++++++++++++++++++++++----------------------
  1 file changed, 22 insertions(+), 22 deletions(-)

drivers/block/rbd.c +22 -22

index 89d8ed7..53e5308 100644
@@ -2549,10 +2549,10 @@ static ssize_t rbd_add(struct bus_type *bus,
 
  	options = kmalloc(count, GFP_KERNEL);
  	if (!options)
-		goto err_nomem;
+		goto err_out_mem;
  	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
  	if (!rbd_dev)
-		goto err_nomem;
+		goto err_out_mem;
 
  	/* static rbd_device initialization */
  	spin_lock_init(&rbd_dev->lock);

@@ -2562,25 +2562,17 @@ static ssize_t rbd_add(struct bus_type *bus,
 
  	down_write(&rbd_dev->header_rwsem);
 
-	/* 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);
-
  	/* parse add command */
  	snap_name = rbd_add_parse_args(rbd_dev, buf,
  				&mon_addrs, &mon_addrs_size, options, count);
  	if (IS_ERR(snap_name)) {
  		rc = PTR_ERR(snap_name);
-		goto err_put_id;
+		goto err_out_mem;
  	}
 
  	rc = rbd_get_client(rbd_dev, mon_addrs, mon_addrs_size - 1, options);
  	if (rc < 0)
-		goto err_put_id;
+		goto err_out_args;
 
  	/* pick the pool */
  	osdc = &rbd_dev->rbd_client->client->osdc;

@@ -2612,10 +2604,19 @@ static ssize_t rbd_add(struct bus_type *bus,
  	if (rc)
  		goto err_out_header;
 
-	/* register our block device */
+	/* 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_header;
+		goto err_out_id;
  	rbd_dev->major = rc;
 
  	rc = rbd_bus_add_dev(rbd_dev);

@@ -2663,20 +2664,19 @@ err_out_bus:
 
  err_out_blkdev:
  	unregister_blkdev(rbd_dev->major, rbd_dev->name);
+err_out_id:
+	rbd_dev_id_put(rbd_dev);
  err_out_header:
  	rbd_header_free(&rbd_dev->header);
  err_out_client:
  	kfree(rbd_dev->header_name);
  	rbd_put_client(rbd_dev);
-err_put_id:
-	if (rbd_dev->pool_name) {
-		kfree(rbd_dev->mapping.snap_name);
-		kfree(rbd_dev->image_name);
-		kfree(rbd_dev->pool_name);
-	}
-	rbd_dev_id_put(rbd_dev);
+err_out_args:
+	kfree(rbd_dev->mapping.snap_name);
+	kfree(rbd_dev->image_name);
+	kfree(rbd_dev->pool_name);
  	up_write(&rbd_dev->header_rwsem);
-err_nomem:
+err_out_mem:
  	kfree(rbd_dev);
  	kfree(options);
 

Call rbd_init_disk() from rbd_add() as soon as we have the major device number for the mapping.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   16 +++++++++-------
  1 file changed, 9 insertions(+), 7 deletions(-)

drivers/block/rbd.c +9 -7

index 53e5308..8e82610 100644
@@ -2619,10 +2619,16 @@ static ssize_t rbd_add(struct bus_type *bus,
  		goto err_out_id;
  	rbd_dev->major = rc;
 
-	rc = rbd_bus_add_dev(rbd_dev);
+	/* 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()).

@@ -2634,12 +2640,6 @@ static ssize_t rbd_add(struct bus_type *bus,
 
  	up_write(&rbd_dev->header_rwsem);
 
-	/* Set up the blkdev mapping. */
-
-	rc = rbd_init_disk(rbd_dev);
-	if (rc)
-		goto err_out_bus;
-
  	/* Everything's ready.  Announce the disk to the world. */
 
  	set_capacity(rbd_dev->disk, rbd_dev->mapping.size / SECTOR_SIZE);

@@ -2662,6 +2662,8 @@ err_out_bus:
  	kfree(options);
  	return rc;
 
+err_out_disk:
+	rbd_free_disk(rbd_dev);
  err_out_blkdev:
  	unregister_blkdev(rbd_dev->major, rbd_dev->name);
  err_out_id:

By the time rbd_dev_snaps_register() gets called during rbd device initialization, the main device will have already been registered. Similarly, a header refresh will only occur for an rbd device whose Linux device is registered. There is therefore no need to verify the main device is registered when registering a snapshot device.

For the time being, turn the check into a WARN_ON(), but it can eventually just go away.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |    4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/block/rbd.c +2 -2

index 8e82610..e6af38f 100644
@@ -2253,8 +2253,8 @@ static int rbd_dev_snaps_register(struct
 rbd_device *rbd_dev)
  	int ret = 0;
 
  	dout("%s called\n", __func__);
-	if (!device_is_registered(&rbd_dev->dev))
-		return 0;
+	if (WARN_ON(!device_is_registered(&rbd_dev->dev)))
+		return -EIO;
 
  	list_for_each_entry(snap, &rbd_dev->snaps, node) {

  		if (!rbd_snap_registered(snap)) {

Move the setting of the initial capacity for an rbd image mapping into rb_init_disk().

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |    3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/block/rbd.c +2 -1

index e6af38f..51e1d21 100644
@@ -1901,6 +1901,8 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 
  	rbd_dev->disk = disk;
 
+	set_capacity(rbd_dev->disk, rbd_dev->mapping.size / SECTOR_SIZE);
+
  	return 0;
  out_disk:
  	put_disk(disk);

@@ -2642,7 +2644,6 @@ static ssize_t rbd_add(struct bus_type *bus,
 
  	/* Everything's ready.  Announce the disk to the world. */
 
-	set_capacity(rbd_dev->disk, rbd_dev->mapping.size / SECTOR_SIZE);
  	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);

We're ready to handle header object (refresh) events at the point we call rbd_bus_add_dev(). Set up the watch request on the rbd image header just after that, and after we've registered the devices for the snapshots for the initial snapshot context. Do this before announce the disk as available for use.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)

drivers/block/rbd.c +5 -5

index 51e1d21..daa428e3 100644
@@ -2640,23 +2640,23 @@ static ssize_t rbd_add(struct bus_type *bus,
  	if (rc)
  		goto err_out_unlock;
 
+	rc = rbd_init_watch_dev(rbd_dev);
+	if (rc)
+		goto err_out_unlock;
+
  	up_write(&rbd_dev->header_rwsem);
 
  	/* 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);
 
-	rc = rbd_init_watch_dev(rbd_dev);
-	if (rc)
-		goto err_out_bus;
-
  	return count;
 
  err_out_unlock:
  	up_write(&rbd_dev->header_rwsem);
-err_out_bus:
  	/* this will also clean up rest of rbd_dev stuff */
 
  	rbd_bus_del_dev(rbd_dev);

This fifth series rearranges the order of a bunch of steps that occur during initialization of a new rbd device structure. It consists almost entirely of simple movements of code, but it significantly separates updating a device's snapshot list from registering devices for newly-found snapshots (which will be for the benefit of format 2 image initialization).

It is available as branch "wip-rbd-review-4" on the ceph-client git repository, and is based on the branch "wip-rbd-review-3".

https://github.com/ceph/ceph-client/tree/wip-rbd-review-4

-Alex

[PATCH 1/9]  d58dd5e rbd: assign header name later
[PATCH 2/9]  c8fd9cf rbd: defer registering snapshot devices
[PATCH 3/9]  bf469fa rbd: call set_snap() before snap_devs_update()
[PATCH 4/9]  1ee626d rbd: read the header before registering device
[PATCH 5/9]  f05c12d rbd: defer setting device id
[PATCH 6/9]  8b8b8b7 rbd: call rbd_init_disk() sooner
[PATCH 7/9]  f1194c2 rbd: drop dev registration check for new snap
[PATCH 8/9]  345936d rbd: set initial capacity in rbd_init_disk()
[PATCH 9/9]  527bbdd rbd: set up watch before announcing disk