[PATCH 0/2] rbd: separating mapping info from rbd_device

[PATCH 0/2] rbd: separating mapping info from rbd_device

[Alex Elder]

The rbd device structure has up to now contained information about
the rbd image (and possibly snapshot id) that backs it as well as
the device interface it represents in Linux when it is mapped.  In
order to add layering support we're going to want to separate the
mapping information from the image information more clearly.  This
was begun recently but these two patches do the bulk of the movement
of the rest of the mapping-related fields.

					-Alex

[PATCH 1/2] rbd: move dev_id and list links into mapping
[PATCH 2/2] rbd: move more stuff into mapping

[PATCH 1/2] rbd: move dev_id and list links into mapping

[Alex Elder]

The device id is a property of the mapping, not an rbd image.

Move the dev_id field to reflect that.  The "node" field in an rbd
device structure link together structures recording known device ids
rbd_dev_list.  Move those links into the mapping structure also,
and rename the field "id_list".

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index cf5d109..8189841 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -169,14 +169,15 @@ struct rbd_mapping {
  	u64                     features;
  	bool                    snap_exists;
  	bool			read_only;
+
+	int			dev_id;		/* blkdev unique id */
+	struct list_head	id_list;
  };

  /*
   * a single device
   */
  struct rbd_device {
-	int			dev_id;		/* blkdev unique id */
-
  	int			major;		/* blkdev assigned major */
  	struct gendisk		*disk;		/* blkdev's gendisk and rq */

@@ -205,8 +206,6 @@ struct rbd_device {

  	struct rbd_mapping	mapping;

-	struct list_head	node;
-
  	/* list of snapshots */
  	struct list_head	snaps;

@@ -666,6 +665,7 @@ static int rbd_dev_set_mapping(struct rbd_device 
*rbd_dev, char *snap_name)
  {
  	int ret;

+	INIT_LIST_HEAD(&rbd_dev->mapping.id_list);
  	if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME,
  		    sizeof (RBD_SNAP_HEAD_NAME))) {
  		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
@@ -1771,7 +1771,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
  		return -ENOMEM;

  	snprintf(disk->disk_name, sizeof(disk->disk_name), RBD_DRV_NAME "%d",
-		 rbd_dev->dev_id);
+		 rbd_dev->mapping.dev_id);
  	disk->major = rbd_dev->major;
  	disk->first_minor = 0;
  	disk->fops = &rbd_bd_ops;
@@ -2560,7 +2560,7 @@ static int rbd_bus_add_dev(struct rbd_device *rbd_dev)
  	dev->type = &rbd_device_type;
  	dev->parent = &rbd_root_dev;
  	dev->release = rbd_dev_release;
-	dev_set_name(dev, "%d", rbd_dev->dev_id);
+	dev_set_name(dev, "%d", rbd_dev->mapping.dev_id);
  	ret = device_register(dev);

  	mutex_unlock(&ctl_mutex);
@@ -2595,33 +2595,33 @@ static atomic64_t rbd_dev_id_max = ATOMIC64_INIT(0);
   * Get a unique rbd identifier for the given new rbd_dev, and add
   * the rbd_dev to the global list.  The minimum rbd id is 1.
   */
-static void rbd_dev_id_get(struct rbd_device *rbd_dev)
+static void rbd_dev_id_get(struct rbd_mapping *mapping)
  {
-	rbd_dev->dev_id = atomic64_inc_return(&rbd_dev_id_max);
+	mapping->dev_id = atomic64_inc_return(&rbd_dev_id_max);

  	spin_lock(&rbd_dev_list_lock);
-	list_add_tail(&rbd_dev->node, &rbd_dev_list);
+	list_add_tail(&mapping->id_list, &rbd_dev_list);
  	spin_unlock(&rbd_dev_list_lock);
-	dout("rbd_dev %p given dev id %llu\n", rbd_dev,
-		(unsigned long long) rbd_dev->dev_id);
+	dout("mapping %p given dev id %llu\n", mapping,
+		(unsigned long long) mapping->dev_id);
  }

  /*
   * Remove an rbd_dev from the global list, and record that its
   * identifier is no longer in use.
   */
-static void rbd_dev_id_put(struct rbd_device *rbd_dev)
+static void rbd_dev_id_put(struct rbd_mapping *mapping)
  {
  	struct list_head *tmp;
-	int rbd_id = rbd_dev->dev_id;
+	int rbd_id = mapping->dev_id;
  	int max_id;

  	rbd_assert(rbd_id > 0);

-	dout("rbd_dev %p released dev id %llu\n", rbd_dev,
-		(unsigned long long) rbd_dev->dev_id);
+	dout("mapping %p released dev id %llu\n", mapping,
+		(unsigned long long) mapping->dev_id);
  	spin_lock(&rbd_dev_list_lock);
-	list_del_init(&rbd_dev->node);
+	list_del_init(&mapping->id_list);

  	/*
  	 * If the id being "put" is not the current maximum, there
@@ -2639,11 +2639,11 @@ static void rbd_dev_id_put(struct rbd_device 
*rbd_dev)
  	 */
  	max_id = 0;
  	list_for_each_prev(tmp, &rbd_dev_list) {
-		struct rbd_device *rbd_dev;
+		struct rbd_mapping *rbd_map;

-		rbd_dev = list_entry(tmp, struct rbd_device, node);
-		if (rbd_dev->dev_id > max_id)
-			max_id = rbd_dev->dev_id;
+		rbd_map = list_entry(tmp, struct rbd_mapping, id_list);
+		if (rbd_map->dev_id > max_id)
+			max_id = rbd_map->dev_id;
  	}
  	spin_unlock(&rbd_dev_list_lock);

@@ -3035,7 +3035,6 @@ static ssize_t rbd_add(struct bus_type *bus,

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

@@ -3072,12 +3071,12 @@ static ssize_t rbd_add(struct bus_type *bus,
  		goto err_out_header;

  	/* generate unique id: find highest unique id, add one */
-	rbd_dev_id_get(rbd_dev);
+	rbd_dev_id_get(&rbd_dev->mapping);

  	/* 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);
+	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->mapping.dev_id);

  	/* Get our block major device number. */

@@ -3132,7 +3131,7 @@ err_out_disk:
  err_out_blkdev:
  	unregister_blkdev(rbd_dev->major, rbd_dev->name);
  err_out_id:
-	rbd_dev_id_put(rbd_dev);
+	rbd_dev_id_put(&rbd_dev->mapping);
  err_out_header:
  	rbd_header_free(&rbd_dev->header);
  err_out_client:
@@ -3156,17 +3155,20 @@ err_out_mem:
  static struct rbd_device *__rbd_get_dev(unsigned long dev_id)
  {
  	struct list_head *tmp;
-	struct rbd_device *rbd_dev;

  	spin_lock(&rbd_dev_list_lock);
  	list_for_each(tmp, &rbd_dev_list) {
-		rbd_dev = list_entry(tmp, struct rbd_device, node);
-		if (rbd_dev->dev_id == dev_id) {
-			spin_unlock(&rbd_dev_list_lock);
-			return rbd_dev;
-		}
+		struct rbd_mapping *mapping;
+
+		mapping = list_entry(tmp, struct rbd_mapping, id_list);
+		if (mapping->dev_id != dev_id)
+			continue;
+		spin_unlock(&rbd_dev_list_lock);
+
+		return container_of(mapping, struct rbd_device, mapping);
  	}
  	spin_unlock(&rbd_dev_list_lock);
+
  	return NULL;
  }

@@ -3198,7 +3200,7 @@ static void rbd_dev_release(struct device *dev)
  	kfree(rbd_dev->header_name);
  	kfree(rbd_dev->pool_name);
  	kfree(rbd_dev->image_name);
-	rbd_dev_id_put(rbd_dev);
+	rbd_dev_id_put(&rbd_dev->mapping);
  	kfree(rbd_dev);

  	/* release module ref */
-- 
1.7.9.5

[PATCH 2/2] rbd: move more stuff into mapping

[Alex Elder]

This moves a bunch of other fields out of the rbd device structure
and into the mapping structure.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 8189841..250446d 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -172,23 +172,22 @@ struct rbd_mapping {

  	int			dev_id;		/* blkdev unique id */
  	struct list_head	id_list;
+	char			name[DEV_NAME_LEN]; /* blkdev name, e.g. rbd3 */
+
+	int			major;		/* blkdev assigned major */
+	spinlock_t		lock;		/* queue lock */
+	struct gendisk		*disk;		/* blkdev's gendisk and rq */
+
+	struct rbd_options	rbd_opts;
  };

  /*
   * a single device
   */
  struct rbd_device {
-	int			major;		/* blkdev assigned major */
-	struct gendisk		*disk;		/* blkdev's gendisk and rq */
-
  	u32			image_format;	/* Either 1 or 2 */
-	struct rbd_options	rbd_opts;
  	struct rbd_client	*rbd_client;

-	char			name[DEV_NAME_LEN]; /* blkdev name, e.g. rbd3 */
-
-	spinlock_t		lock;		/* queue lock */
-
  	struct rbd_image_header	header;
  	char			*image_id;
  	size_t			image_id_len;
@@ -452,7 +451,7 @@ static int parse_rbd_opts_token(char *c, void *private)
  static int rbd_get_client(struct rbd_device *rbd_dev, const char 
*mon_addr,
  				size_t mon_addr_len, char *options)
  {
-	struct rbd_options *rbd_opts = &rbd_dev->rbd_opts;
+	struct rbd_options *rbd_opts = &rbd_dev->mapping.rbd_opts;
  	struct ceph_options *ceph_opts;
  	struct rbd_client *rbdc;

@@ -672,7 +671,8 @@ static int rbd_dev_set_mapping(struct rbd_device 
*rbd_dev, char *snap_name)
  		rbd_dev->mapping.size = rbd_dev->header.image_size;
  		rbd_dev->mapping.features = rbd_dev->header.features;
  		rbd_dev->mapping.snap_exists = false;
-		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
+		rbd_dev->mapping.read_only =
+			rbd_dev->mapping.rbd_opts.read_only;
  		ret = 0;
  	} else {
  		ret = snap_by_name(rbd_dev, snap_name);
@@ -1293,7 +1293,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id, 
u8 opcode, void *data)
  	rc = rbd_dev_refresh(rbd_dev, &hver);
  	if (rc)
  		pr_warning(RBD_DRV_NAME "%d got notification but failed to "
-			   " update snaps: %d\n", rbd_dev->major, rc);
+			   " update snaps: %d\n", rbd_dev->mapping.major, rc);

  	rbd_req_sync_notify_ack(rbd_dev, hver, notify_id);
  }
@@ -1573,7 +1573,7 @@ static int rbd_merge_bvec(struct request_queue *q, 
struct bvec_merge_data *bmd,

  static void rbd_free_disk(struct rbd_device *rbd_dev)
  {
-	struct gendisk *disk = rbd_dev->disk;
+	struct gendisk *disk = rbd_dev->mapping.disk;

  	if (!disk)
  		return;
@@ -1697,7 +1697,7 @@ static void rbd_update_mapping_size(struct 
rbd_device *rbd_dev)
  	size = (sector_t) rbd_dev->header.image_size / SECTOR_SIZE;
  	dout("setting size to %llu sectors", (unsigned long long) size);
  	rbd_dev->mapping.size = (u64) size;
-	set_capacity(rbd_dev->disk, size);
+	set_capacity(rbd_dev->mapping.disk, size);
  }

  /*
@@ -1772,13 +1772,14 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)

  	snprintf(disk->disk_name, sizeof(disk->disk_name), RBD_DRV_NAME "%d",
  		 rbd_dev->mapping.dev_id);
-	disk->major = rbd_dev->major;
+	disk->major = rbd_dev->mapping.major;
  	disk->first_minor = 0;
  	disk->fops = &rbd_bd_ops;
  	disk->private_data = rbd_dev;

  	/* init rq */
-	q = blk_init_queue(rbd_rq_fn, &rbd_dev->lock);
+	spin_lock_init(&rbd_dev->mapping.lock);
+	q = blk_init_queue(rbd_rq_fn, &rbd_dev->mapping.lock);
  	if (!q)
  		goto out_disk;

@@ -1797,9 +1798,10 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)

  	q->queuedata = rbd_dev;

-	rbd_dev->disk = disk;
+	rbd_dev->mapping.disk = disk;

-	set_capacity(rbd_dev->disk, rbd_dev->mapping.size / SECTOR_SIZE);
+	set_capacity(rbd_dev->mapping.disk,
+			rbd_dev->mapping.size / SECTOR_SIZE);

  	return 0;
  out_disk:
@@ -1824,7 +1826,7 @@ static ssize_t rbd_size_show(struct device *dev,
  	sector_t size;

  	down_read(&rbd_dev->header_rwsem);
-	size = get_capacity(rbd_dev->disk);
+	size = get_capacity(rbd_dev->mapping.disk);
  	up_read(&rbd_dev->header_rwsem);

  	return sprintf(buf, "%llu\n", (unsigned long long) size * SECTOR_SIZE);
@@ -1848,7 +1850,7 @@ static ssize_t rbd_major_show(struct device *dev,
  {
  	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

-	return sprintf(buf, "%d\n", rbd_dev->major);
+	return sprintf(buf, "%d\n", rbd_dev->mapping.major);
  }

  static ssize_t rbd_client_id_show(struct device *dev,
@@ -3034,7 +3036,6 @@ static ssize_t rbd_add(struct bus_type *bus,
  		goto err_out_mem;

  	/* static rbd_device initialization */
-	spin_lock_init(&rbd_dev->lock);
  	INIT_LIST_HEAD(&rbd_dev->snaps);
  	init_rwsem(&rbd_dev->header_rwsem);

@@ -3076,14 +3077,15 @@ static ssize_t rbd_add(struct bus_type *bus,
  	/* 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->mapping.dev_id);
+	sprintf(rbd_dev->mapping.name, "%s%d",
+			RBD_DRV_NAME, rbd_dev->mapping.dev_id);

  	/* Get our block major device number. */

-	rc = register_blkdev(0, rbd_dev->name);
+	rc = register_blkdev(0, rbd_dev->mapping.name);
  	if (rc < 0)
  		goto err_out_id;
-	rbd_dev->major = rc;
+	rbd_dev->mapping.major = rc;

  	/* Set up the blkdev mapping. */

@@ -3112,9 +3114,10 @@ static ssize_t rbd_add(struct bus_type *bus,

  	/* Everything's ready.  Announce the disk to the world. */

-	add_disk(rbd_dev->disk);
+	add_disk(rbd_dev->mapping.disk);

-	pr_info("%s: added with size 0x%llx\n", rbd_dev->disk->disk_name,
+	pr_info("%s: added with size 0x%llx\n",
+		rbd_dev->mapping.disk->disk_name,
  		(unsigned long long) rbd_dev->mapping.size);

  	return count;
@@ -3129,7 +3132,7 @@ err_out_bus:
  err_out_disk:
  	rbd_free_disk(rbd_dev);
  err_out_blkdev:
-	unregister_blkdev(rbd_dev->major, rbd_dev->name);
+	unregister_blkdev(rbd_dev->mapping.major, rbd_dev->mapping.name);
  err_out_id:
  	rbd_dev_id_put(&rbd_dev->mapping);
  err_out_header:
@@ -3189,7 +3192,7 @@ static void rbd_dev_release(struct device *dev)

  	/* clean up and free blkdev */
  	rbd_free_disk(rbd_dev);
-	unregister_blkdev(rbd_dev->major, rbd_dev->name);
+	unregister_blkdev(rbd_dev->mapping.major, rbd_dev->mapping.name);

  	/* release allocated disk header fields */
  	rbd_header_free(&rbd_dev->header);
-- 
1.7.9.5

Re: [PATCH 0/2] rbd: separating mapping info from rbd_device

[Alex Elder]

On 10/11/2012 10:42 AM, Alex Elder wrote:
> The rbd device structure has up to now contained information about
> the rbd image (and possibly snapshot id) that backs it as well as
> the device interface it represents in Linux when it is mapped.  In
> order to add layering support we're going to want to separate the
> mapping information from the image information more clearly.  This
> was begun recently but these two patches do the bulk of the movement
> of the rest of the mapping-related fields.
> 
>                     -Alex
> 
> [PATCH 1/2] rbd: move dev_id and list links into mapping
> [PATCH 2/2] rbd: move more stuff into mapping

I'm retracting this series, I no longer think it's quite the
right way to proceed.

					-Alex