[PATCH 0/7] rbd: isolate mapping info, rearrange init

[PATCH 0/7] rbd: isolate mapping info, rearrange init

[Alex Elder]

This third series begins a little more interesting code
restructuring.  The first five patches make more clear
the distinction in a struct rbd_device between information
about the underlying rbd image and information about the
mapped snapshot represented by the structure.

The last two patches (which might arguably belong in a
later sub-series) move some initialization code out of
one function and into its caller.

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

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

					-Alex

[PATCH 1/7]  f4069bf rbd: separate mapping info in rbd_dev
    This groups some related fields of an rbd device structure into
    a sub-structure.  The fields pertain to the specific mapping
    an rbd_device represents--as opposed to the base rbd image.
[PATCH 2/7]  bee627f rbd: record mapped size
    This adds the size of the image or snapshot that's mapped to an
    rbd device's mapping information.
[PATCH 3/7]  8b45382 rbd: return snap name from rbd_add_parse_args()
[PATCH 4/7]  cb93cc4 rbd: set mapping name with the rest
    These two patches make the name of the mapped snapshot be set
    along with the other mapping-related fields.
[PATCH 5/7]  8a245ce rbd: simplify snap_by_name() interface
    This does some refactoring enabled by the previous few patches.
[PATCH 6/7]  e4008bd rbd: do some header initialization earlier
    This rearranges where certain initialization is done, beginning
    the process of re-ordering some of these steps in order to allow
    both format 1 and format 2 rbd images to be handled in the same way.
[PATCH 7/7]  275ba12 rbd: simplify rbd_init_disk() a bit
    This does some refactoring enabled by the previous patch.

[PATCH 1/7] rbd: separate mapping info in rbd_dev

[Alex Elder]

Several fields in a struct rbd_dev are related to what is mapped, as
opposed to the actual base rbd image.  If the base image is mapped
these are almost unneeded, but if a snapshot is mapped they describe
information about that snapshot.

In some contexts this can be a little bit confusing.  So group these
mapping-related field into a structure to make it clear what they
are describing.

This also includes a minor change that rearranges the fields in the
in-core image header structure so that invariant fields are at the
top, followed by those that change.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index eb6b772..dff6210 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -81,13 +81,15 @@
  * block device image metadata (in-memory version)
  */
 struct rbd_image_header {
-	u64 image_size;
+	/* These four fields never change for a given rbd image */
 	char *object_prefix;
 	__u8 obj_order;
 	__u8 crypt_type;
 	__u8 comp_type;
-	struct ceph_snap_context *snapc;

+	/* The remaining fields need to be updated occasionally */
+	u64 image_size;
+	struct ceph_snap_context *snapc;
 	char *snap_names;
 	u64 *snap_sizes;

@@ -146,6 +148,13 @@ struct rbd_snap {
 	u64			id;
 };

+struct rbd_mapping {
+	char                    *snap_name;
+	u64                     snap_id;
+	bool                    snap_exists;
+	bool			read_only;
+};
+
 /*
  * a single device
  */
@@ -174,13 +183,8 @@ struct rbd_device {

 	/* protects updating the header */
 	struct rw_semaphore     header_rwsem;
-	/* name of the snapshot this device reads from */
-	char                    *snap_name;
-	/* id of the snapshot this device reads from */
-	u64                     snap_id;	/* current snapshot id */
-	/* whether the snap_id this device reads from still exists */
-	bool                    snap_exists;
-	bool			read_only;
+
+	struct rbd_mapping	mapping;

 	struct list_head	node;

@@ -261,11 +265,11 @@ static int rbd_open(struct block_device *bdev,
fmode_t mode)
 {
 	struct rbd_device *rbd_dev = bdev->bd_disk->private_data;

-	if ((mode & FMODE_WRITE) && rbd_dev->read_only)
+	if ((mode & FMODE_WRITE) && rbd_dev->mapping.read_only)
 		return -EROFS;

 	rbd_get_dev(rbd_dev);
-	set_device_ro(bdev, rbd_dev->read_only);
+	set_device_ro(bdev, rbd_dev->mapping.read_only);

 	return 0;
 }
@@ -375,7 +379,7 @@ enum {
 static match_table_t rbd_opts_tokens = {
 	/* int args above */
 	/* string args above */
-	{Opt_read_only, "read_only"},
+	{Opt_read_only, "mapping.read_only"},
 	{Opt_read_only, "ro"},		/* Alternate spelling */
 	{Opt_read_write, "read_write"},
 	{Opt_read_write, "rw"},		/* Alternate spelling */
@@ -583,13 +587,13 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 		header->snap_sizes = NULL;
 	}

-	header->image_size = le64_to_cpu(ondisk->image_size);
 	header->obj_order = ondisk->options.order;
 	header->crypt_type = ondisk->options.crypt_type;
 	header->comp_type = ondisk->options.comp_type;

 	/* Allocate and fill in the snapshot context */

+	header->image_size = le64_to_cpu(ondisk->image_size);
 	size = sizeof (struct ceph_snap_context);
 	size += snap_count * sizeof (header->snapc->snaps[0]);
 	header->snapc = kzalloc(size, GFP_KERNEL);
@@ -645,23 +649,24 @@ static int rbd_header_set_snap(struct rbd_device
*rbd_dev, u64 *size)

 	down_write(&rbd_dev->header_rwsem);

-	if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
+	if (!memcmp(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
 		    sizeof (RBD_SNAP_HEAD_NAME))) {
-		rbd_dev->snap_id = CEPH_NOSNAP;
-		rbd_dev->snap_exists = false;
-		rbd_dev->read_only = rbd_dev->rbd_opts.read_only;
+		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
+		rbd_dev->mapping.snap_exists = false;
+		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
 		if (size)
 			*size = rbd_dev->header.image_size;
 	} else {
 		u64 snap_id = 0;

-		ret = snap_by_name(&rbd_dev->header, rbd_dev->snap_name,
+		ret = snap_by_name(&rbd_dev->header,
+					rbd_dev->mapping.snap_name,
 					&snap_id, size);
 		if (ret < 0)
 			goto done;
-		rbd_dev->snap_id = snap_id;
-		rbd_dev->snap_exists = true;
-		rbd_dev->read_only = true;	/* No choice for snapshots */
+		rbd_dev->mapping.snap_id = snap_id;
+		rbd_dev->mapping.snap_exists = true;
+		rbd_dev->mapping.read_only = true;
 	}

 	ret = 0;
@@ -1532,7 +1537,7 @@ static void rbd_rq_fn(struct request_queue *q)
 		size = blk_rq_bytes(rq);
 		ofs = blk_rq_pos(rq) * SECTOR_SIZE;
 		rq_bio = rq->bio;
-		if (do_write && rbd_dev->read_only) {
+		if (do_write && rbd_dev->mapping.read_only) {
 			__blk_end_request_all(rq, -EROFS);
 			continue;
 		}
@@ -1541,7 +1546,8 @@ static void rbd_rq_fn(struct request_queue *q)

 		down_read(&rbd_dev->header_rwsem);

-		if (rbd_dev->snap_id != CEPH_NOSNAP && !rbd_dev->snap_exists) {
+		if (rbd_dev->mapping.snap_id != CEPH_NOSNAP &&
+				!rbd_dev->mapping.snap_exists) {
 			up_read(&rbd_dev->header_rwsem);
 			dout("request for non-existent snapshot");
 			spin_lock_irq(q->queue_lock);
@@ -1595,7 +1601,7 @@ static void rbd_rq_fn(struct request_queue *q)
 					      coll, cur_seg);
 			else
 				rbd_req_read(rq, rbd_dev,
-					     rbd_dev->snap_id,
+					     rbd_dev->mapping.snap_id,
 					     ofs,
 					     op_size, bio,
 					     coll, cur_seg);
@@ -1767,7 +1773,7 @@ static int rbd_header_add_snap(struct rbd_device
*rbd_dev,
 	struct ceph_mon_client *monc;

 	/* we should create a snapshot only if we're pointing at the head */
-	if (rbd_dev->snap_id != CEPH_NOSNAP)
+	if (rbd_dev->mapping.snap_id != CEPH_NOSNAP)
 		return -EINVAL;

 	monc = &rbd_dev->rbd_client->client->monc;
@@ -1821,7 +1827,7 @@ static int __rbd_refresh_header(struct rbd_device
*rbd_dev, u64 *hver)
 	down_write(&rbd_dev->header_rwsem);

 	/* resized? */
-	if (rbd_dev->snap_id == CEPH_NOSNAP) {
+	if (rbd_dev->mapping.snap_id == CEPH_NOSNAP) {
 		sector_t size = (sector_t) h.image_size / SECTOR_SIZE;

 		dout("setting size to %llu sectors", (unsigned long long) size);
@@ -2004,7 +2010,7 @@ static ssize_t rbd_snap_show(struct device *dev,
 {
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

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

 static ssize_t rbd_image_refresh(struct device *dev,
@@ -2205,11 +2211,12 @@ static int rbd_dev_snap_devs_update(struct
rbd_device *rbd_dev)

 			/* Existing snapshot not in the new snap context */

-			if (rbd_dev->snap_id == snap->id)
-				rbd_dev->snap_exists = false;
+			if (rbd_dev->mapping.snap_id == snap->id)
+				rbd_dev->mapping.snap_exists = false;
 			__rbd_remove_snap_dev(snap);
 			dout("%ssnap id %llu has been removed\n",
-				rbd_dev->snap_id == snap->id ? "mapped " : "",
+				rbd_dev->mapping.snap_id == snap->id ?
+								"mapped " : "",
 				(unsigned long long) snap->id);

 			/* Done with this list entry; advance */
@@ -2522,18 +2529,18 @@ static int rbd_add_parse_args(struct rbd_device
*rbd_dev,
 	 * The snapshot name is optional.  If none is is supplied,
 	 * we use the default value.
 	 */
-	rbd_dev->snap_name = dup_token(&buf, &len);
-	if (!rbd_dev->snap_name)
+	rbd_dev->mapping.snap_name = dup_token(&buf, &len);
+	if (!rbd_dev->mapping.snap_name)
 		goto out_err;
 	if (!len) {
 		/* Replace the empty name with the default */
-		kfree(rbd_dev->snap_name);
-		rbd_dev->snap_name
+		kfree(rbd_dev->mapping.snap_name);
+		rbd_dev->mapping.snap_name
 			= kmalloc(sizeof (RBD_SNAP_HEAD_NAME), GFP_KERNEL);
-		if (!rbd_dev->snap_name)
+		if (!rbd_dev->mapping.snap_name)
 			goto out_err;

-		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
+		memcpy(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
 			sizeof (RBD_SNAP_HEAD_NAME));
 	}

@@ -2642,7 +2649,7 @@ err_out_client:
 	rbd_put_client(rbd_dev);
 err_put_id:
 	if (rbd_dev->pool_name) {
-		kfree(rbd_dev->snap_name);
+		kfree(rbd_dev->mapping.snap_name);
 		kfree(rbd_dev->header_name);
 		kfree(rbd_dev->image_name);
 		kfree(rbd_dev->pool_name);
@@ -2695,7 +2702,7 @@ static void rbd_dev_release(struct device *dev)
 	unregister_blkdev(rbd_dev->major, rbd_dev->name);

 	/* done with the id, and with the rbd_dev */
-	kfree(rbd_dev->snap_name);
+	kfree(rbd_dev->mapping.snap_name);
 	kfree(rbd_dev->header_name);
 	kfree(rbd_dev->pool_name);
 	kfree(rbd_dev->image_name);
-- 
1.7.9.5

Re: [PATCH 1/7] rbd: separate mapping info in rbd_dev

[Josh Durgin]

Might want to keep the comments about the fields
moved to the mapping struct, but it looks good to me.

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

On 09/07/2012 06:45 AM, Alex Elder wrote:
> Several fields in a struct rbd_dev are related to what is mapped, as
> opposed to the actual base rbd image.  If the base image is mapped
> these are almost unneeded, but if a snapshot is mapped they describe
> information about that snapshot.
>
> In some contexts this can be a little bit confusing.  So group these
> mapping-related field into a structure to make it clear what they
> are describing.
>
> This also includes a minor change that rearranges the fields in the
> in-core image header structure so that invariant fields are at the
> top, followed by those that change.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   83
> ++++++++++++++++++++++++++++-----------------------
>   1 file changed, 45 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index eb6b772..dff6210 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -81,13 +81,15 @@
>    * block device image metadata (in-memory version)
>    */
>   struct rbd_image_header {
> -	u64 image_size;
> +	/* These four fields never change for a given rbd image */
>   	char *object_prefix;
>   	__u8 obj_order;
>   	__u8 crypt_type;
>   	__u8 comp_type;
> -	struct ceph_snap_context *snapc;
>
> +	/* The remaining fields need to be updated occasionally */
> +	u64 image_size;
> +	struct ceph_snap_context *snapc;
>   	char *snap_names;
>   	u64 *snap_sizes;
>
> @@ -146,6 +148,13 @@ struct rbd_snap {
>   	u64			id;
>   };
>
> +struct rbd_mapping {
> +	char                    *snap_name;
> +	u64                     snap_id;
> +	bool                    snap_exists;
> +	bool			read_only;
> +};
> +
>   /*
>    * a single device
>    */
> @@ -174,13 +183,8 @@ struct rbd_device {
>
>   	/* protects updating the header */
>   	struct rw_semaphore     header_rwsem;
> -	/* name of the snapshot this device reads from */
> -	char                    *snap_name;
> -	/* id of the snapshot this device reads from */
> -	u64                     snap_id;	/* current snapshot id */
> -	/* whether the snap_id this device reads from still exists */
> -	bool                    snap_exists;
> -	bool			read_only;
> +
> +	struct rbd_mapping	mapping;
>
>   	struct list_head	node;
>
> @@ -261,11 +265,11 @@ static int rbd_open(struct block_device *bdev,
> fmode_t mode)
>   {
>   	struct rbd_device *rbd_dev = bdev->bd_disk->private_data;
>
> -	if ((mode & FMODE_WRITE) && rbd_dev->read_only)
> +	if ((mode & FMODE_WRITE) && rbd_dev->mapping.read_only)
>   		return -EROFS;
>
>   	rbd_get_dev(rbd_dev);
> -	set_device_ro(bdev, rbd_dev->read_only);
> +	set_device_ro(bdev, rbd_dev->mapping.read_only);
>
>   	return 0;
>   }
> @@ -375,7 +379,7 @@ enum {
>   static match_table_t rbd_opts_tokens = {
>   	/* int args above */
>   	/* string args above */
> -	{Opt_read_only, "read_only"},
> +	{Opt_read_only, "mapping.read_only"},
>   	{Opt_read_only, "ro"},		/* Alternate spelling */
>   	{Opt_read_write, "read_write"},
>   	{Opt_read_write, "rw"},		/* Alternate spelling */
> @@ -583,13 +587,13 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>   		header->snap_sizes = NULL;
>   	}
>
> -	header->image_size = le64_to_cpu(ondisk->image_size);
>   	header->obj_order = ondisk->options.order;
>   	header->crypt_type = ondisk->options.crypt_type;
>   	header->comp_type = ondisk->options.comp_type;
>
>   	/* Allocate and fill in the snapshot context */
>
> +	header->image_size = le64_to_cpu(ondisk->image_size);
>   	size = sizeof (struct ceph_snap_context);
>   	size += snap_count * sizeof (header->snapc->snaps[0]);
>   	header->snapc = kzalloc(size, GFP_KERNEL);
> @@ -645,23 +649,24 @@ static int rbd_header_set_snap(struct rbd_device
> *rbd_dev, u64 *size)
>
>   	down_write(&rbd_dev->header_rwsem);
>
> -	if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
> +	if (!memcmp(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
>   		    sizeof (RBD_SNAP_HEAD_NAME))) {
> -		rbd_dev->snap_id = CEPH_NOSNAP;
> -		rbd_dev->snap_exists = false;
> -		rbd_dev->read_only = rbd_dev->rbd_opts.read_only;
> +		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
> +		rbd_dev->mapping.snap_exists = false;
> +		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
>   		if (size)
>   			*size = rbd_dev->header.image_size;
>   	} else {
>   		u64 snap_id = 0;
>
> -		ret = snap_by_name(&rbd_dev->header, rbd_dev->snap_name,
> +		ret = snap_by_name(&rbd_dev->header,
> +					rbd_dev->mapping.snap_name,
>   					&snap_id, size);
>   		if (ret < 0)
>   			goto done;
> -		rbd_dev->snap_id = snap_id;
> -		rbd_dev->snap_exists = true;
> -		rbd_dev->read_only = true;	/* No choice for snapshots */
> +		rbd_dev->mapping.snap_id = snap_id;
> +		rbd_dev->mapping.snap_exists = true;
> +		rbd_dev->mapping.read_only = true;
>   	}
>
>   	ret = 0;
> @@ -1532,7 +1537,7 @@ static void rbd_rq_fn(struct request_queue *q)
>   		size = blk_rq_bytes(rq);
>   		ofs = blk_rq_pos(rq) * SECTOR_SIZE;
>   		rq_bio = rq->bio;
> -		if (do_write && rbd_dev->read_only) {
> +		if (do_write && rbd_dev->mapping.read_only) {
>   			__blk_end_request_all(rq, -EROFS);
>   			continue;
>   		}
> @@ -1541,7 +1546,8 @@ static void rbd_rq_fn(struct request_queue *q)
>
>   		down_read(&rbd_dev->header_rwsem);
>
> -		if (rbd_dev->snap_id != CEPH_NOSNAP && !rbd_dev->snap_exists) {
> +		if (rbd_dev->mapping.snap_id != CEPH_NOSNAP &&
> +				!rbd_dev->mapping.snap_exists) {
>   			up_read(&rbd_dev->header_rwsem);
>   			dout("request for non-existent snapshot");
>   			spin_lock_irq(q->queue_lock);
> @@ -1595,7 +1601,7 @@ static void rbd_rq_fn(struct request_queue *q)
>   					      coll, cur_seg);
>   			else
>   				rbd_req_read(rq, rbd_dev,
> -					     rbd_dev->snap_id,
> +					     rbd_dev->mapping.snap_id,
>   					     ofs,
>   					     op_size, bio,
>   					     coll, cur_seg);
> @@ -1767,7 +1773,7 @@ static int rbd_header_add_snap(struct rbd_device
> *rbd_dev,
>   	struct ceph_mon_client *monc;
>
>   	/* we should create a snapshot only if we're pointing at the head */
> -	if (rbd_dev->snap_id != CEPH_NOSNAP)
> +	if (rbd_dev->mapping.snap_id != CEPH_NOSNAP)
>   		return -EINVAL;
>
>   	monc = &rbd_dev->rbd_client->client->monc;
> @@ -1821,7 +1827,7 @@ static int __rbd_refresh_header(struct rbd_device
> *rbd_dev, u64 *hver)
>   	down_write(&rbd_dev->header_rwsem);
>
>   	/* resized? */
> -	if (rbd_dev->snap_id == CEPH_NOSNAP) {
> +	if (rbd_dev->mapping.snap_id == CEPH_NOSNAP) {
>   		sector_t size = (sector_t) h.image_size / SECTOR_SIZE;
>
>   		dout("setting size to %llu sectors", (unsigned long long) size);
> @@ -2004,7 +2010,7 @@ static ssize_t rbd_snap_show(struct device *dev,
>   {
>   	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>
> -	return sprintf(buf, "%s\n", rbd_dev->snap_name);
> +	return sprintf(buf, "%s\n", rbd_dev->mapping.snap_name);
>   }
>
>   static ssize_t rbd_image_refresh(struct device *dev,
> @@ -2205,11 +2211,12 @@ static int rbd_dev_snap_devs_update(struct
> rbd_device *rbd_dev)
>
>   			/* Existing snapshot not in the new snap context */
>
> -			if (rbd_dev->snap_id == snap->id)
> -				rbd_dev->snap_exists = false;
> +			if (rbd_dev->mapping.snap_id == snap->id)
> +				rbd_dev->mapping.snap_exists = false;
>   			__rbd_remove_snap_dev(snap);
>   			dout("%ssnap id %llu has been removed\n",
> -				rbd_dev->snap_id == snap->id ? "mapped " : "",
> +				rbd_dev->mapping.snap_id == snap->id ?
> +								"mapped " : "",
>   				(unsigned long long) snap->id);
>
>   			/* Done with this list entry; advance */
> @@ -2522,18 +2529,18 @@ static int rbd_add_parse_args(struct rbd_device
> *rbd_dev,
>   	 * The snapshot name is optional.  If none is is supplied,
>   	 * we use the default value.
>   	 */
> -	rbd_dev->snap_name = dup_token(&buf, &len);
> -	if (!rbd_dev->snap_name)
> +	rbd_dev->mapping.snap_name = dup_token(&buf, &len);
> +	if (!rbd_dev->mapping.snap_name)
>   		goto out_err;
>   	if (!len) {
>   		/* Replace the empty name with the default */
> -		kfree(rbd_dev->snap_name);
> -		rbd_dev->snap_name
> +		kfree(rbd_dev->mapping.snap_name);
> +		rbd_dev->mapping.snap_name
>   			= kmalloc(sizeof (RBD_SNAP_HEAD_NAME), GFP_KERNEL);
> -		if (!rbd_dev->snap_name)
> +		if (!rbd_dev->mapping.snap_name)
>   			goto out_err;
>
> -		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
> +		memcpy(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
>   			sizeof (RBD_SNAP_HEAD_NAME));
>   	}
>
> @@ -2642,7 +2649,7 @@ err_out_client:
>   	rbd_put_client(rbd_dev);
>   err_put_id:
>   	if (rbd_dev->pool_name) {
> -		kfree(rbd_dev->snap_name);
> +		kfree(rbd_dev->mapping.snap_name);
>   		kfree(rbd_dev->header_name);
>   		kfree(rbd_dev->image_name);
>   		kfree(rbd_dev->pool_name);
> @@ -2695,7 +2702,7 @@ static void rbd_dev_release(struct device *dev)
>   	unregister_blkdev(rbd_dev->major, rbd_dev->name);
>
>   	/* done with the id, and with the rbd_dev */
> -	kfree(rbd_dev->snap_name);
> +	kfree(rbd_dev->mapping.snap_name);
>   	kfree(rbd_dev->header_name);
>   	kfree(rbd_dev->pool_name);
>   	kfree(rbd_dev->image_name);
>

Re: [PATCH 1/7] rbd: separate mapping info in rbd_dev

[Alex Elder]

On 09/07/2012 01:58 PM, Josh Durgin wrote:
> Might want to keep the comments about the fields
> moved to the mapping struct, but it looks good to me.
> 
> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

I'm going to leave this as-is.  I'm going to definitely be
reworking all these structures in the coming weeks to more
clearly separate images from snapshots, and I'll try to
improve how things are documented as I go.

					-Alex

> On 09/07/2012 06:45 AM, Alex Elder wrote:
>> Several fields in a struct rbd_dev are related to what is mapped, as
>> opposed to the actual base rbd image.  If the base image is mapped
>> these are almost unneeded, but if a snapshot is mapped they describe
>> information about that snapshot.
>>
>> In some contexts this can be a little bit confusing.  So group these
>> mapping-related field into a structure to make it clear what they
>> are describing.
>>
>> This also includes a minor change that rearranges the fields in the
>> in-core image header structure so that invariant fields are at the
>> top, followed by those that change.
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |   83
>> ++++++++++++++++++++++++++++-----------------------
>>   1 file changed, 45 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index eb6b772..dff6210 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -81,13 +81,15 @@
>>    * block device image metadata (in-memory version)
>>    */
>>   struct rbd_image_header {
>> -    u64 image_size;
>> +    /* These four fields never change for a given rbd image */
>>       char *object_prefix;
>>       __u8 obj_order;
>>       __u8 crypt_type;
>>       __u8 comp_type;
>> -    struct ceph_snap_context *snapc;
>>
>> +    /* The remaining fields need to be updated occasionally */
>> +    u64 image_size;
>> +    struct ceph_snap_context *snapc;
>>       char *snap_names;
>>       u64 *snap_sizes;
>>
>> @@ -146,6 +148,13 @@ struct rbd_snap {
>>       u64            id;
>>   };
>>
>> +struct rbd_mapping {
>> +    char                    *snap_name;
>> +    u64                     snap_id;
>> +    bool                    snap_exists;
>> +    bool            read_only;
>> +};
>> +
>>   /*
>>    * a single device
>>    */
>> @@ -174,13 +183,8 @@ struct rbd_device {
>>
>>       /* protects updating the header */
>>       struct rw_semaphore     header_rwsem;
>> -    /* name of the snapshot this device reads from */
>> -    char                    *snap_name;
>> -    /* id of the snapshot this device reads from */
>> -    u64                     snap_id;    /* current snapshot id */
>> -    /* whether the snap_id this device reads from still exists */
>> -    bool                    snap_exists;
>> -    bool            read_only;
>> +
>> +    struct rbd_mapping    mapping;
>>
>>       struct list_head    node;
>>
>> @@ -261,11 +265,11 @@ static int rbd_open(struct block_device *bdev,
>> fmode_t mode)
>>   {
>>       struct rbd_device *rbd_dev = bdev->bd_disk->private_data;
>>
>> -    if ((mode & FMODE_WRITE) && rbd_dev->read_only)
>> +    if ((mode & FMODE_WRITE) && rbd_dev->mapping.read_only)
>>           return -EROFS;
>>
>>       rbd_get_dev(rbd_dev);
>> -    set_device_ro(bdev, rbd_dev->read_only);
>> +    set_device_ro(bdev, rbd_dev->mapping.read_only);
>>
>>       return 0;
>>   }
>> @@ -375,7 +379,7 @@ enum {
>>   static match_table_t rbd_opts_tokens = {
>>       /* int args above */
>>       /* string args above */
>> -    {Opt_read_only, "read_only"},
>> +    {Opt_read_only, "mapping.read_only"},
>>       {Opt_read_only, "ro"},        /* Alternate spelling */
>>       {Opt_read_write, "read_write"},
>>       {Opt_read_write, "rw"},        /* Alternate spelling */
>> @@ -583,13 +587,13 @@ static int rbd_header_from_disk(struct
>> rbd_image_header *header,
>>           header->snap_sizes = NULL;
>>       }
>>
>> -    header->image_size = le64_to_cpu(ondisk->image_size);
>>       header->obj_order = ondisk->options.order;
>>       header->crypt_type = ondisk->options.crypt_type;
>>       header->comp_type = ondisk->options.comp_type;
>>
>>       /* Allocate and fill in the snapshot context */
>>
>> +    header->image_size = le64_to_cpu(ondisk->image_size);
>>       size = sizeof (struct ceph_snap_context);
>>       size += snap_count * sizeof (header->snapc->snaps[0]);
>>       header->snapc = kzalloc(size, GFP_KERNEL);
>> @@ -645,23 +649,24 @@ static int rbd_header_set_snap(struct rbd_device
>> *rbd_dev, u64 *size)
>>
>>       down_write(&rbd_dev->header_rwsem);
>>
>> -    if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>> +    if (!memcmp(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
>>               sizeof (RBD_SNAP_HEAD_NAME))) {
>> -        rbd_dev->snap_id = CEPH_NOSNAP;
>> -        rbd_dev->snap_exists = false;
>> -        rbd_dev->read_only = rbd_dev->rbd_opts.read_only;
>> +        rbd_dev->mapping.snap_id = CEPH_NOSNAP;
>> +        rbd_dev->mapping.snap_exists = false;
>> +        rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
>>           if (size)
>>               *size = rbd_dev->header.image_size;
>>       } else {
>>           u64 snap_id = 0;
>>
>> -        ret = snap_by_name(&rbd_dev->header, rbd_dev->snap_name,
>> +        ret = snap_by_name(&rbd_dev->header,
>> +                    rbd_dev->mapping.snap_name,
>>                       &snap_id, size);
>>           if (ret < 0)
>>               goto done;
>> -        rbd_dev->snap_id = snap_id;
>> -        rbd_dev->snap_exists = true;
>> -        rbd_dev->read_only = true;    /* No choice for snapshots */
>> +        rbd_dev->mapping.snap_id = snap_id;
>> +        rbd_dev->mapping.snap_exists = true;
>> +        rbd_dev->mapping.read_only = true;
>>       }
>>
>>       ret = 0;
>> @@ -1532,7 +1537,7 @@ static void rbd_rq_fn(struct request_queue *q)
>>           size = blk_rq_bytes(rq);
>>           ofs = blk_rq_pos(rq) * SECTOR_SIZE;
>>           rq_bio = rq->bio;
>> -        if (do_write && rbd_dev->read_only) {
>> +        if (do_write && rbd_dev->mapping.read_only) {
>>               __blk_end_request_all(rq, -EROFS);
>>               continue;
>>           }
>> @@ -1541,7 +1546,8 @@ static void rbd_rq_fn(struct request_queue *q)
>>
>>           down_read(&rbd_dev->header_rwsem);
>>
>> -        if (rbd_dev->snap_id != CEPH_NOSNAP && !rbd_dev->snap_exists) {
>> +        if (rbd_dev->mapping.snap_id != CEPH_NOSNAP &&
>> +                !rbd_dev->mapping.snap_exists) {
>>               up_read(&rbd_dev->header_rwsem);
>>               dout("request for non-existent snapshot");
>>               spin_lock_irq(q->queue_lock);
>> @@ -1595,7 +1601,7 @@ static void rbd_rq_fn(struct request_queue *q)
>>                             coll, cur_seg);
>>               else
>>                   rbd_req_read(rq, rbd_dev,
>> -                         rbd_dev->snap_id,
>> +                         rbd_dev->mapping.snap_id,
>>                            ofs,
>>                            op_size, bio,
>>                            coll, cur_seg);
>> @@ -1767,7 +1773,7 @@ static int rbd_header_add_snap(struct rbd_device
>> *rbd_dev,
>>       struct ceph_mon_client *monc;
>>
>>       /* we should create a snapshot only if we're pointing at the
>> head */
>> -    if (rbd_dev->snap_id != CEPH_NOSNAP)
>> +    if (rbd_dev->mapping.snap_id != CEPH_NOSNAP)
>>           return -EINVAL;
>>
>>       monc = &rbd_dev->rbd_client->client->monc;
>> @@ -1821,7 +1827,7 @@ static int __rbd_refresh_header(struct rbd_device
>> *rbd_dev, u64 *hver)
>>       down_write(&rbd_dev->header_rwsem);
>>
>>       /* resized? */
>> -    if (rbd_dev->snap_id == CEPH_NOSNAP) {
>> +    if (rbd_dev->mapping.snap_id == CEPH_NOSNAP) {
>>           sector_t size = (sector_t) h.image_size / SECTOR_SIZE;
>>
>>           dout("setting size to %llu sectors", (unsigned long long)
>> size);
>> @@ -2004,7 +2010,7 @@ static ssize_t rbd_snap_show(struct device *dev,
>>   {
>>       struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>>
>> -    return sprintf(buf, "%s\n", rbd_dev->snap_name);
>> +    return sprintf(buf, "%s\n", rbd_dev->mapping.snap_name);
>>   }
>>
>>   static ssize_t rbd_image_refresh(struct device *dev,
>> @@ -2205,11 +2211,12 @@ static int rbd_dev_snap_devs_update(struct
>> rbd_device *rbd_dev)
>>
>>               /* Existing snapshot not in the new snap context */
>>
>> -            if (rbd_dev->snap_id == snap->id)
>> -                rbd_dev->snap_exists = false;
>> +            if (rbd_dev->mapping.snap_id == snap->id)
>> +                rbd_dev->mapping.snap_exists = false;
>>               __rbd_remove_snap_dev(snap);
>>               dout("%ssnap id %llu has been removed\n",
>> -                rbd_dev->snap_id == snap->id ? "mapped " : "",
>> +                rbd_dev->mapping.snap_id == snap->id ?
>> +                                "mapped " : "",
>>                   (unsigned long long) snap->id);
>>
>>               /* Done with this list entry; advance */
>> @@ -2522,18 +2529,18 @@ static int rbd_add_parse_args(struct rbd_device
>> *rbd_dev,
>>        * The snapshot name is optional.  If none is is supplied,
>>        * we use the default value.
>>        */
>> -    rbd_dev->snap_name = dup_token(&buf, &len);
>> -    if (!rbd_dev->snap_name)
>> +    rbd_dev->mapping.snap_name = dup_token(&buf, &len);
>> +    if (!rbd_dev->mapping.snap_name)
>>           goto out_err;
>>       if (!len) {
>>           /* Replace the empty name with the default */
>> -        kfree(rbd_dev->snap_name);
>> -        rbd_dev->snap_name
>> +        kfree(rbd_dev->mapping.snap_name);
>> +        rbd_dev->mapping.snap_name
>>               = kmalloc(sizeof (RBD_SNAP_HEAD_NAME), GFP_KERNEL);
>> -        if (!rbd_dev->snap_name)
>> +        if (!rbd_dev->mapping.snap_name)
>>               goto out_err;
>>
>> -        memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>> +        memcpy(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
>>               sizeof (RBD_SNAP_HEAD_NAME));
>>       }
>>
>> @@ -2642,7 +2649,7 @@ err_out_client:
>>       rbd_put_client(rbd_dev);
>>   err_put_id:
>>       if (rbd_dev->pool_name) {
>> -        kfree(rbd_dev->snap_name);
>> +        kfree(rbd_dev->mapping.snap_name);
>>           kfree(rbd_dev->header_name);
>>           kfree(rbd_dev->image_name);
>>           kfree(rbd_dev->pool_name);
>> @@ -2695,7 +2702,7 @@ static void rbd_dev_release(struct device *dev)
>>       unregister_blkdev(rbd_dev->major, rbd_dev->name);
>>
>>       /* done with the id, and with the rbd_dev */
>> -    kfree(rbd_dev->snap_name);
>> +    kfree(rbd_dev->mapping.snap_name);
>>       kfree(rbd_dev->header_name);
>>       kfree(rbd_dev->pool_name);
>>       kfree(rbd_dev->image_name);
>>
> 
> 
> 

[PATCH 2/7] rbd: record mapped size

[Alex Elder]

Add the size of the mapped image to the set of mapping-specific
fields in an rbd_device, and use it when setting the capacity of the
disk.

Rename the "seq" argument to snap_by_name() to be "snap_id" to be
consistent with other usage.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index dff6210..4377a83 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -151,6 +151,7 @@ struct rbd_snap {
 struct rbd_mapping {
 	char                    *snap_name;
 	u64                     snap_id;
+	u64                     size;
 	bool                    snap_exists;
 	bool			read_only;
 };
@@ -643,7 +644,7 @@ static int snap_by_name(struct rbd_image_header
*header, const char *snap_name,
 	return -ENOENT;
 }

-static int rbd_header_set_snap(struct rbd_device *rbd_dev, u64 *size)
+static int rbd_header_set_snap(struct rbd_device *rbd_dev)
 {
 	int ret;

@@ -652,19 +653,16 @@ static int rbd_header_set_snap(struct rbd_device
*rbd_dev, u64 *size)
 	if (!memcmp(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
 		    sizeof (RBD_SNAP_HEAD_NAME))) {
 		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
+		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;
-		if (size)
-			*size = rbd_dev->header.image_size;
 	} else {
-		u64 snap_id = 0;
-
 		ret = snap_by_name(&rbd_dev->header,
 					rbd_dev->mapping.snap_name,
-					&snap_id, size);
+					&rbd_dev->mapping.snap_id,
+					&rbd_dev->mapping.size);
 		if (ret < 0)
 			goto done;
-		rbd_dev->mapping.snap_id = snap_id;
 		rbd_dev->mapping.snap_exists = true;
 		rbd_dev->mapping.read_only = true;
 	}
@@ -1830,8 +1828,12 @@ static int __rbd_refresh_header(struct rbd_device
*rbd_dev, u64 *hver)
 	if (rbd_dev->mapping.snap_id == CEPH_NOSNAP) {
 		sector_t size = (sector_t) h.image_size / SECTOR_SIZE;

-		dout("setting size to %llu sectors", (unsigned long long) size);
-		set_capacity(rbd_dev->disk, size);
+		if (size != (sector_t) rbd_dev->mapping.size) {
+			dout("setting size to %llu sectors",
+				(unsigned long long) size);
+			rbd_dev->mapping.size = (u64) size;
+			set_capacity(rbd_dev->disk, size);
+		}
 	}

 	/* rbd_dev->header.object_prefix shouldn't change */
@@ -1875,7 +1877,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 	struct request_queue *q;
 	int rc;
 	u64 segment_size;
-	u64 total_size = 0;

 	/* contact OSD, request size info about the object being mapped */
 	rc = rbd_read_header(rbd_dev, &rbd_dev->header);
@@ -1887,7 +1888,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 	if (rc)
 		return rc;

-	rc = rbd_header_set_snap(rbd_dev, &total_size);
+	rc = rbd_header_set_snap(rbd_dev);
 	if (rc)
 		return rc;

@@ -1928,11 +1929,11 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 	rbd_dev->disk = disk;

 	/* finally, announce the disk to the world */
-	set_capacity(disk, total_size / SECTOR_SIZE);
+	set_capacity(disk, (sector_t) rbd_dev->mapping.size / SECTOR_SIZE);
 	add_disk(disk);

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

 out_disk:
-- 
1.7.9.5

Re: [PATCH 2/7] rbd: record mapped size

[Josh Durgin]

On 09/07/2012 06:45 AM, Alex Elder wrote:
> Add the size of the mapped image to the set of mapping-specific
> fields in an rbd_device, and use it when setting the capacity of the
> disk.
>
> Rename the "seq" argument to snap_by_name() to be "snap_id" to be
> consistent with other usage.

This doesn't seem to be part of this commit anymore.
Assuming the commit message is fixed,

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

>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   27 ++++++++++++++-------------
>   1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index dff6210..4377a83 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -151,6 +151,7 @@ struct rbd_snap {
>   struct rbd_mapping {
>   	char                    *snap_name;
>   	u64                     snap_id;
> +	u64                     size;
>   	bool                    snap_exists;
>   	bool			read_only;
>   };
> @@ -643,7 +644,7 @@ static int snap_by_name(struct rbd_image_header
> *header, const char *snap_name,
>   	return -ENOENT;
>   }
>
> -static int rbd_header_set_snap(struct rbd_device *rbd_dev, u64 *size)
> +static int rbd_header_set_snap(struct rbd_device *rbd_dev)
>   {
>   	int ret;
>
> @@ -652,19 +653,16 @@ static int rbd_header_set_snap(struct rbd_device
> *rbd_dev, u64 *size)
>   	if (!memcmp(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
>   		    sizeof (RBD_SNAP_HEAD_NAME))) {
>   		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
> +		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;
> -		if (size)
> -			*size = rbd_dev->header.image_size;
>   	} else {
> -		u64 snap_id = 0;
> -
>   		ret = snap_by_name(&rbd_dev->header,
>   					rbd_dev->mapping.snap_name,
> -					&snap_id, size);
> +					&rbd_dev->mapping.snap_id,
> +					&rbd_dev->mapping.size);
>   		if (ret < 0)
>   			goto done;
> -		rbd_dev->mapping.snap_id = snap_id;
>   		rbd_dev->mapping.snap_exists = true;
>   		rbd_dev->mapping.read_only = true;
>   	}
> @@ -1830,8 +1828,12 @@ static int __rbd_refresh_header(struct rbd_device
> *rbd_dev, u64 *hver)
>   	if (rbd_dev->mapping.snap_id == CEPH_NOSNAP) {
>   		sector_t size = (sector_t) h.image_size / SECTOR_SIZE;
>
> -		dout("setting size to %llu sectors", (unsigned long long) size);
> -		set_capacity(rbd_dev->disk, size);
> +		if (size != (sector_t) rbd_dev->mapping.size) {
> +			dout("setting size to %llu sectors",
> +				(unsigned long long) size);
> +			rbd_dev->mapping.size = (u64) size;
> +			set_capacity(rbd_dev->disk, size);
> +		}
>   	}
>
>   	/* rbd_dev->header.object_prefix shouldn't change */
> @@ -1875,7 +1877,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>   	struct request_queue *q;
>   	int rc;
>   	u64 segment_size;
> -	u64 total_size = 0;
>
>   	/* contact OSD, request size info about the object being mapped */
>   	rc = rbd_read_header(rbd_dev, &rbd_dev->header);
> @@ -1887,7 +1888,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>   	if (rc)
>   		return rc;
>
> -	rc = rbd_header_set_snap(rbd_dev, &total_size);
> +	rc = rbd_header_set_snap(rbd_dev);
>   	if (rc)
>   		return rc;
>
> @@ -1928,11 +1929,11 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>   	rbd_dev->disk = disk;
>
>   	/* finally, announce the disk to the world */
> -	set_capacity(disk, total_size / SECTOR_SIZE);
> +	set_capacity(disk, (sector_t) rbd_dev->mapping.size / SECTOR_SIZE);
>   	add_disk(disk);
>
>   	pr_info("%s: added with size 0x%llx\n",
> -		disk->disk_name, (unsigned long long)total_size);
> +		disk->disk_name, (unsigned long long) rbd_dev->mapping.size);
>   	return 0;
>
>   out_disk:
>

Re: [PATCH 2/7] rbd: record mapped size

[Alex Elder]

On 09/07/2012 02:10 PM, Josh Durgin wrote:
> On 09/07/2012 06:45 AM, Alex Elder wrote:
>> Add the size of the mapped image to the set of mapping-specific
>> fields in an rbd_device, and use it when setting the capacity of the
>> disk.
>>
>> Rename the "seq" argument to snap_by_name() to be "snap_id" to be
>> consistent with other usage.
> 
> This doesn't seem to be part of this commit anymore.
> Assuming the commit message is fixed,

I deleted it.	-Alex

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

. . .

[PATCH 3/7] rbd: return snap name from rbd_add_parse_args()

[Alex Elder]

This is the first of two patches aimed at isolating the code that
sets the mapping information into a single spot.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 4377a83..1a64ba2 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2477,28 +2477,31 @@ static inline char *dup_token(const char **buf,
size_t *lenp)
 }

 /*
- * This fills in the pool_name, image_name, image_name_len, snap_name,
- * rbd_dev, rbd_md_name, and name fields of the given rbd_dev, based
- * on the list of monitor addresses and other options provided via
- * /sys/bus/rbd/add.
+ * This fills in the pool_name, image_name, image_name_len, rbd_dev,
+ * rbd_md_name, and name fields of the given rbd_dev, based on the
+ * list of monitor addresses and other options provided via
+ * /sys/bus/rbd/add.  Returns a pointer to a dynamically-allocated
+ * copy of the snapshot name to map if successful, or a
+ * pointer-coded error otherwise.
  *
  * Note: rbd_dev is assumed to have been initially zero-filled.
  */
-static int rbd_add_parse_args(struct rbd_device *rbd_dev,
-			      const char *buf,
-			      const char **mon_addrs,
-			      size_t *mon_addrs_size,
-			      char *options,
-			     size_t options_size)
+static char *rbd_add_parse_args(struct rbd_device *rbd_dev,
+				const char *buf,
+				const char **mon_addrs,
+				size_t *mon_addrs_size,
+				char *options,
+				size_t options_size)
 {
 	size_t len;
-	int ret;
+	char *err_ptr = ERR_PTR(-EINVAL);
+	char *snap_name;

 	/* The first four tokens are required */

 	len = next_token(&buf);
 	if (!len)
-		return -EINVAL;
+		return err_ptr;
 	*mon_addrs_size = len + 1;
 	*mon_addrs = buf;

@@ -2506,9 +2509,9 @@ static int rbd_add_parse_args(struct rbd_device
*rbd_dev,

 	len = copy_token(&buf, options, options_size);
 	if (!len || len >= options_size)
-		return -EINVAL;
+		return err_ptr;

-	ret = -ENOMEM;
+	err_ptr = ERR_PTR(-ENOMEM);
 	rbd_dev->pool_name = dup_token(&buf, NULL);
 	if (!rbd_dev->pool_name)
 		goto out_err;
@@ -2526,26 +2529,21 @@ static int rbd_add_parse_args(struct rbd_device
*rbd_dev,
 		goto out_err;
 	sprintf(rbd_dev->header_name, "%s%s", rbd_dev->image_name, RBD_SUFFIX);

-	/*
-	 * The snapshot name is optional.  If none is is supplied,
-	 * we use the default value.
-	 */
-	rbd_dev->mapping.snap_name = dup_token(&buf, &len);
-	if (!rbd_dev->mapping.snap_name)
-		goto out_err;
+	/* Snapshot name is optional */
+	len = next_token(&buf);
 	if (!len) {
-		/* Replace the empty name with the default */
-		kfree(rbd_dev->mapping.snap_name);
-		rbd_dev->mapping.snap_name
-			= kmalloc(sizeof (RBD_SNAP_HEAD_NAME), GFP_KERNEL);
-		if (!rbd_dev->mapping.snap_name)
-			goto out_err;
-
-		memcpy(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
-			sizeof (RBD_SNAP_HEAD_NAME));
+		buf = RBD_SNAP_HEAD_NAME; /* No snapshot supplied */
+		len = sizeof (RBD_SNAP_HEAD_NAME) - 1;
 	}
+	snap_name = kmalloc(len + 1, GFP_KERNEL);
+	if (!snap_name)
+		goto out_err;
+	memcpy(snap_name, buf, len);
+	*(snap_name + len) = '\0';

-	return 0;
+dout("    SNAP_NAME is <%s>, len is %zd\n", snap_name, len);
+
+	return snap_name;

 out_err:
 	kfree(rbd_dev->header_name);
@@ -2556,7 +2554,7 @@ out_err:
 	kfree(rbd_dev->pool_name);
 	rbd_dev->pool_name = NULL;

-	return ret;
+	return err_ptr;
 }

 static ssize_t rbd_add(struct bus_type *bus,
@@ -2569,6 +2567,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	size_t mon_addrs_size = 0;
 	struct ceph_osd_client *osdc;
 	int rc = -ENOMEM;
+	char *snap_name;

 	if (!try_module_get(THIS_MODULE))
 		return -ENODEV;
@@ -2595,10 +2594,13 @@ static ssize_t rbd_add(struct bus_type *bus,
 	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->dev_id);

 	/* parse add command */
-	rc = rbd_add_parse_args(rbd_dev, buf, &mon_addrs, &mon_addrs_size,
-				options, count);
-	if (rc)
+	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;
+	}
+	rbd_dev->mapping.snap_name = snap_name;

 	rc = rbd_get_client(rbd_dev, mon_addrs, mon_addrs_size - 1, options);
 	if (rc < 0)
-- 
1.7.9.5

Re: [PATCH 3/7] rbd: return snap name from rbd_add_parse_args()

[Josh Durgin]

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

On 09/07/2012 06:45 AM, Alex Elder wrote:
> This is the first of two patches aimed at isolating the code that
> sets the mapping information into a single spot.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   72
> ++++++++++++++++++++++++++-------------------------
>   1 file changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 4377a83..1a64ba2 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2477,28 +2477,31 @@ static inline char *dup_token(const char **buf,
> size_t *lenp)
>   }
>
>   /*
> - * This fills in the pool_name, image_name, image_name_len, snap_name,
> - * rbd_dev, rbd_md_name, and name fields of the given rbd_dev, based
> - * on the list of monitor addresses and other options provided via
> - * /sys/bus/rbd/add.
> + * This fills in the pool_name, image_name, image_name_len, rbd_dev,
> + * rbd_md_name, and name fields of the given rbd_dev, based on the
> + * list of monitor addresses and other options provided via
> + * /sys/bus/rbd/add.  Returns a pointer to a dynamically-allocated
> + * copy of the snapshot name to map if successful, or a
> + * pointer-coded error otherwise.
>    *
>    * Note: rbd_dev is assumed to have been initially zero-filled.
>    */
> -static int rbd_add_parse_args(struct rbd_device *rbd_dev,
> -			      const char *buf,
> -			      const char **mon_addrs,
> -			      size_t *mon_addrs_size,
> -			      char *options,
> -			     size_t options_size)
> +static char *rbd_add_parse_args(struct rbd_device *rbd_dev,
> +				const char *buf,
> +				const char **mon_addrs,
> +				size_t *mon_addrs_size,
> +				char *options,
> +				size_t options_size)
>   {
>   	size_t len;
> -	int ret;
> +	char *err_ptr = ERR_PTR(-EINVAL);
> +	char *snap_name;
>
>   	/* The first four tokens are required */
>
>   	len = next_token(&buf);
>   	if (!len)
> -		return -EINVAL;
> +		return err_ptr;
>   	*mon_addrs_size = len + 1;
>   	*mon_addrs = buf;
>
> @@ -2506,9 +2509,9 @@ static int rbd_add_parse_args(struct rbd_device
> *rbd_dev,
>
>   	len = copy_token(&buf, options, options_size);
>   	if (!len || len >= options_size)
> -		return -EINVAL;
> +		return err_ptr;
>
> -	ret = -ENOMEM;
> +	err_ptr = ERR_PTR(-ENOMEM);
>   	rbd_dev->pool_name = dup_token(&buf, NULL);
>   	if (!rbd_dev->pool_name)
>   		goto out_err;
> @@ -2526,26 +2529,21 @@ static int rbd_add_parse_args(struct rbd_device
> *rbd_dev,
>   		goto out_err;
>   	sprintf(rbd_dev->header_name, "%s%s", rbd_dev->image_name, RBD_SUFFIX);
>
> -	/*
> -	 * The snapshot name is optional.  If none is is supplied,
> -	 * we use the default value.
> -	 */
> -	rbd_dev->mapping.snap_name = dup_token(&buf, &len);
> -	if (!rbd_dev->mapping.snap_name)
> -		goto out_err;
> +	/* Snapshot name is optional */
> +	len = next_token(&buf);
>   	if (!len) {
> -		/* Replace the empty name with the default */
> -		kfree(rbd_dev->mapping.snap_name);
> -		rbd_dev->mapping.snap_name
> -			= kmalloc(sizeof (RBD_SNAP_HEAD_NAME), GFP_KERNEL);
> -		if (!rbd_dev->mapping.snap_name)
> -			goto out_err;
> -
> -		memcpy(rbd_dev->mapping.snap_name, RBD_SNA
Here are the points to cover on this call with Alcatel Lucent.
Bryan would like for you to run the call and clarify the plan of action.

Primary objectives for Proof of Concept phase:


1. Ceph demo - on Ubuntu 12.04
Show Qemu - kvm migration and recovery
Show block device in a cloud environment

2. Training and demo of RBD access paths
Determine how that matches AL's configuration plans
P_HEAD_NAME,
> -			sizeof (RBD_SNAP_HEAD_NAME));
> +		buf = RBD_SNAP_HEAD_NAME; /* No snapshot supplied */
> +		len = sizeof (RBD_SNAP_HEAD_NAME) - 1;
>   	}
> +	snap_name = kmalloc(len + 1, GFP_KERNEL);
> +	if (!snap_name)
> +		goto out_err;
> +	memcpy(snap_name, buf, len);
> +	*(snap_name + len) = '\0';
>
> -	return 0;
> +dout("    SNAP_NAME is <%s>, len is %zd\n", snap_name, len);
> +
> +	return snap_name;
>
>   out_err:
>   	kfree(rbd_dev->header_name);
> @@ -2556,7 +2554,7 @@ out_err:
>   	kfree(rbd_dev->pool_name);
>   	rbd_dev->pool_name = NULL;
>
> -	return ret;
> +	return err_ptr;
>   }
>
>   static ssize_t rbd_add(struct bus_type *bus,
> @@ -2569,6 +2567,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	size_t mon_addrs_size = 0;
>   	struct ceph_osd_client *osdc;
>   	int rc = -ENOMEM;
> +	char *snap_name;
>
>   	if (!try_module_get(THIS_MODULE))
>   		return -ENODEV;
> @@ -2595,10 +2594,13 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->dev_id);
>
>   	/* parse add command */
> -	rc = rbd_add_parse_args(rbd_dev, buf, &mon_addrs, &mon_addrs_size,
> -				options, count);
> -	if (rc)
> +	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;
> +	}
> +	rbd_dev->mapping.snap_name = snap_name;
>
>   	rc = rbd_get_client(rbd_dev, mon_addrs, mon_addrs_size - 1, options);
>   	if (rc < 0)
>

[PATCH 4/7] rbd: set mapping name with the rest

[Alex Elder]

With the exception of the snapshot name, all of the mapping-specific
fields in an rbd device structure are set in rbd_header_set_snap().

Pass the snapshot name to be assigned into rbd_header_set_snap()
to keep all of the mapping assignments together.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 1a64ba2..23fa962 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -644,21 +644,20 @@ static int snap_by_name(struct rbd_image_header
*header, const char *snap_name,
 	return -ENOENT;
 }

-static int rbd_header_set_snap(struct rbd_device *rbd_dev)
+static int rbd_header_set_snap(struct rbd_device *rbd_dev, char *snap_name)
 {
 	int ret;

 	down_write(&rbd_dev->header_rwsem);

-	if (!memcmp(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
+	if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME,
 		    sizeof (RBD_SNAP_HEAD_NAME))) {
 		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
 		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;
 	} else {
-		ret = snap_by_name(&rbd_dev->header,
-					rbd_dev->mapping.snap_name,
+		ret = snap_by_name(&rbd_dev->header, snap_name,
 					&rbd_dev->mapping.snap_id,
 					&rbd_dev->mapping.size);
 		if (ret < 0)
@@ -666,6 +665,7 @@ static int rbd_header_set_snap(struct rbd_device
*rbd_dev)
 		rbd_dev->mapping.snap_exists = true;
 		rbd_dev->mapping.read_only = true;
 	}
+	rbd_dev->mapping.snap_name = snap_name;

 	ret = 0;
 done:
@@ -1888,7 +1888,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 	if (rc)
 		return rc;

-	rc = rbd_header_set_snap(rbd_dev);
+	rc = rbd_header_set_snap(rbd_dev, snap_name);
 	if (rc)
 		return rc;

@@ -2600,7 +2600,6 @@ static ssize_t rbd_add(struct bus_type *bus,
 		rc = PTR_ERR(snap_name);
 		goto err_put_id;
 	}
-	rbd_dev->mapping.snap_name = snap_name;

 	rc = rbd_get_client(rbd_dev, mon_addrs, mon_addrs_size - 1, options);
 	if (rc < 0)
-- 
1.7.9.5

Re: [PATCH 4/7] rbd: set mapping name with the rest

[Josh Durgin]

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

On 09/07/2012 06:45 AM, Alex Elder wrote:
> With the exception of the snapshot name, all of the mapping-specific
> fields in an rbd device structure are set in rbd_header_set_snap().
>
> Pass the snapshot name to be assigned into rbd_header_set_snap()
> to keep all of the mapping assignments together.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 1a64ba2..23fa962 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -644,21 +644,20 @@ static int snap_by_name(struct rbd_image_header
> *header, const char *snap_name,
>   	return -ENOENT;
>   }
>
> -static int rbd_header_set_snap(struct rbd_device *rbd_dev)
> +static int rbd_header_set_snap(struct rbd_device *rbd_dev, char *snap_name)
>   {
>   	int ret;
>
>   	down_write(&rbd_dev->header_rwsem);
>
> -	if (!memcmp(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
> +	if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME,
>   		    sizeof (RBD_SNAP_HEAD_NAME))) {
>   		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
>   		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;
>   	} else {
> -		ret = snap_by_name(&rbd_dev->header,
> -					rbd_dev->mapping.snap_name,
> +		ret = snap_by_name(&rbd_dev->header, snap_name,
>   					&rbd_dev->mapping.snap_id,
>   					&rbd_dev->mapping.size);
>   		if (ret < 0)
> @@ -666,6 +665,7 @@ static int rbd_header_set_snap(struct rbd_device
> *rbd_dev)
>   		rbd_dev->mapping.snap_exists = true;
>   		rbd_dev->mapping.read_only = true;
>   	}
> +	rbd_dev->mapping.snap_name = snap_name;
>
>   	ret = 0;
>   done:
> @@ -1888,7 +1888,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>   	if (rc)
>   		return rc;
>
> -	rc = rbd_header_set_snap(rbd_dev);
> +	rc = rbd_header_set_snap(rbd_dev, snap_name);
>   	if (rc)
>   		return rc;
>
> @@ -2600,7 +2600,6 @@ static ssize_t rbd_add(struct bus_type *bus,
>   		rc = PTR_ERR(snap_name);
>   		goto err_put_id;
>   	}
> -	rbd_dev->mapping.snap_name = snap_name;
>
>   	rc = rbd_get_client(rbd_dev, mon_addrs, mon_addrs_size - 1, options);
>   	if (rc < 0)
>

[PATCH 5/7] rbd: simplify snap_by_name() interface

[Alex Elder]

There is only one caller of snap_by_name(), and it passes two values
to be assigned, both of which are found within an rbd device
structure.

Change the interface so it just passes the address of the rbd_dev,
and make the assignments to its fields directly.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 23fa962..46b8f8e 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -621,10 +621,10 @@ out_err:
 	return -ENOMEM;
 }

-static int snap_by_name(struct rbd_image_header *header, const char
*snap_name,
-			u64 *seq, u64 *size)
+static int snap_by_name(struct rbd_device *rbd_dev, const char *snap_name)
 {
 	int i;
+	struct rbd_image_header *header = &rbd_dev->header;
 	char *p = header->snap_names;

 	rbd_assert(header->snapc != NULL);
@@ -633,10 +633,9 @@ static int snap_by_name(struct rbd_image_header
*header, const char *snap_name,

 			/* Found it.  Pass back its id and/or size */

-			if (seq)
-				*seq = header->snapc->snaps[i];
-			if (size)
-				*size = header->snap_sizes[i];
+			rbd_dev->mapping.snap_id = header->snapc->snaps[i];
+			rbd_dev->mapping.size = header->snap_sizes[i];
+
 			return i;
 		}
 		p += strlen(p) + 1;	/* Skip ahead to the next name */
@@ -657,9 +656,7 @@ static int rbd_header_set_snap(struct rbd_device
*rbd_dev, char *snap_name)
 		rbd_dev->mapping.snap_exists = false;
 		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
 	} else {
-		ret = snap_by_name(&rbd_dev->header, snap_name,
-					&rbd_dev->mapping.snap_id,
-					&rbd_dev->mapping.size);
+		ret = snap_by_name(rbd_dev, snap_name);
 		if (ret < 0)
 			goto done;
 		rbd_dev->mapping.snap_exists = true;
-- 
1.7.9.5

Re: [PATCH 5/7] rbd: simplify snap_by_name() interface

[Josh Durgin]

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

On 09/07/2012 06:45 AM, Alex Elder wrote:
> There is only one caller of snap_by_name(), and it passes two values
> to be assigned, both of which are found within an rbd device
> structure.
>
> Change the interface so it just passes the address of the rbd_dev,
> and make the assignments to its fields directly.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   15 ++++++---------
>   1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 23fa962..46b8f8e 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -621,10 +621,10 @@ out_err:
>   	return -ENOMEM;
>   }
>
> -static int snap_by_name(struct rbd_image_header *header, const char
> *snap_name,
> -			u64 *seq, u64 *size)
> +static int snap_by_name(struct rbd_device *rbd_dev, const char *snap_name)
>   {
>   	int i;
> +	struct rbd_image_header *header = &rbd_dev->header;
>   	char *p = header->snap_names;
>
>   	rbd_assert(header->snapc != NULL);
> @@ -633,10 +633,9 @@ static int snap_by_name(struct rbd_image_header
> *header, const char *snap_name,
>
>   			/* Found it.  Pass back its id and/or size */
>
> -			if (seq)
> -				*seq = header->snapc->snaps[i];
> -			if (size)
> -				*size = header->snap_sizes[i];
> +			rbd_dev->mapping.snap_id = header->snapc->snaps[i];
> +			rbd_dev->mapping.size = header->snap_sizes[i];
> +
>   			return i;
>   		}
>   		p += strlen(p) + 1;	/* Skip ahead to the next name */
> @@ -657,9 +656,7 @@ static int rbd_header_set_snap(struct rbd_device
> *rbd_dev, char *snap_name)
>   		rbd_dev->mapping.snap_exists = false;
>   		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
>   	} else {
> -		ret = snap_by_name(&rbd_dev->header, snap_name,
> -					&rbd_dev->mapping.snap_id,
> -					&rbd_dev->mapping.size);
> +		ret = snap_by_name(rbd_dev, snap_name);
>   		if (ret < 0)
>   			goto done;
>   		rbd_dev->mapping.snap_exists = true;
>

[PATCH 6/7] rbd: do some header initialization earlier

[Alex Elder]

Move some of the code that initializes an rbd header out of
rbd_init_disk() and into its caller.

Move the code at the end of rbd_init_disk() that sets the device
capacity and activates the Linux device out of that function and
into the caller, ensuring we still have the disk size available
where we need it.

Update rbd_free_disk() so it still aligns well as an inverse of
rbd_init_disk(), moving the rbd_header_free() call out to its
caller.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 46b8f8e..6e735a7 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1652,8 +1652,6 @@ static void rbd_free_disk(struct rbd_device *rbd_dev)
 	if (!disk)
 		return;

-	rbd_header_free(&rbd_dev->header);
-
 	if (disk->flags & GENHD_FL_UP)
 		del_gendisk(disk);
 	if (disk->queue)
@@ -1875,20 +1873,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 	int rc;
 	u64 segment_size;

-	/* contact OSD, request size info about the object being mapped */
-	rc = rbd_read_header(rbd_dev, &rbd_dev->header);
-	if (rc)
-		return rc;
-
-	/* no need to lock here, as rbd_dev is not registered yet */
-	rc = rbd_dev_snap_devs_update(rbd_dev);
-	if (rc)
-		return rc;
-
-	rc = rbd_header_set_snap(rbd_dev, snap_name);
-	if (rc)
-		return rc;
-
 	/* create gendisk info */
 	rc = -ENOMEM;
 	disk = alloc_disk(RBD_MINORS_PER_MAJOR);
@@ -1925,12 +1909,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)

 	rbd_dev->disk = disk;

-	/* finally, announce the disk to the world */
-	set_capacity(disk, (sector_t) rbd_dev->mapping.size / SECTOR_SIZE);
-	add_disk(disk);
-
-	pr_info("%s: added with size 0x%llx\n",
-		disk->disk_name, (unsigned long long) rbd_dev->mapping.size);
 	return 0;

 out_disk:
@@ -2622,13 +2600,35 @@ static ssize_t rbd_add(struct bus_type *bus,
 	/*
 	 * At this point cleanup in the event of an error is the job
 	 * of the sysfs code (initiated by rbd_bus_del_dev()).
-	 *
-	 * Set up and announce blkdev mapping.
 	 */
+
+	/* contact OSD, request size info about the object being mapped */
+	rc = rbd_read_header(rbd_dev, &rbd_dev->header);
+	if (rc)
+		goto err_out_bus;
+
+	/* no need to lock here, as rbd_dev is not registered yet */
+	rc = rbd_dev_snap_devs_update(rbd_dev);
+	if (rc)
+		goto err_out_bus;
+
+	rc = rbd_header_set_snap(rbd_dev, snap_name);
+	if (rc)
+		goto err_out_bus;
+
+	/* 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);
+	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;
@@ -2700,6 +2700,9 @@ static void rbd_dev_release(struct device *dev)
 	rbd_free_disk(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 */
 	kfree(rbd_dev->mapping.snap_name);
 	kfree(rbd_dev->header_name);
-- 
1.7.9.5

Re: [PATCH 6/7] rbd: do some header initialization earlier

[Josh Durgin]

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

On 09/07/2012 06:46 AM, Alex Elder wrote:
> Move some of the code that initializes an rbd header out of
> rbd_init_disk() and into its caller.
>
> Move the code at the end of rbd_init_disk() that sets the device
> capacity and activates the Linux device out of that function and
> into the caller, ensuring we still have the disk size available
> where we need it.
>
> Update rbd_free_disk() so it still aligns well as an inverse of
> rbd_init_disk(), moving the rbd_header_free() call out to its
> caller.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   51
> +++++++++++++++++++++++++++------------------------
>   1 file changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 46b8f8e..6e735a7 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -1652,8 +1652,6 @@ static void rbd_free_disk(struct rbd_device *rbd_dev)
>   	if (!disk)
>   		return;
>
> -	rbd_header_free(&rbd_dev->header);
> -
>   	if (disk->flags & GENHD_FL_UP)
>   		del_gendisk(disk);
>   	if (disk->queue)
> @@ -1875,20 +1873,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>   	int rc;
>   	u64 segment_size;
>
> -	/* contact OSD, request size info about the object being mapped */
> -	rc = rbd_read_header(rbd_dev, &rbd_dev->header);
> -	if (rc)
> -		return rc;
> -
> -	/* no need to lock here, as rbd_dev is not registered yet */
> -	rc = rbd_dev_snap_devs_update(rbd_dev);
> -	if (rc)
> -		return rc;
> -
> -	rc = rbd_header_set_snap(rbd_dev, snap_name);
> -	if (rc)
> -		return rc;
> -
>   	/* create gendisk info */
>   	rc = -ENOMEM;
>   	disk = alloc_disk(RBD_MINORS_PER_MAJOR);
> @@ -1925,12 +1909,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>
>   	rbd_dev->disk = disk;
>
> -	/* finally, announce the disk to the world */
> -	set_capacity(disk, (sector_t) rbd_dev->mapping.size / SECTOR_SIZE);
> -	add_disk(disk);
> -
> -	pr_info("%s: added with size 0x%llx\n",
> -		disk->disk_name, (unsigned long long) rbd_dev->mapping.size);
>   	return 0;
>
>   out_disk:
> @@ -2622,13 +2600,35 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	/*
>   	 * At this point cleanup in the event of an error is the job
>   	 * of the sysfs code (initiated by rbd_bus_del_dev()).
> -	 *
> -	 * Set up and announce blkdev mapping.
>   	 */
> +
> +	/* contact OSD, request size info about the object being mapped */
> +	rc = rbd_read_header(rbd_dev, &rbd_dev->header);
> +	if (rc)
> +		goto err_out_bus;
> +
> +	/* no need to lock here, as rbd_dev is not registered yet */
> +	rc = rbd_dev_snap_devs_update(rbd_dev);
> +	if (rc)
> +		goto err_out_bus;
> +
> +	rc = rbd_header_set_snap(rbd_dev, snap_name);
> +	if (rc)
> +		goto err_out_bus;
> +
> +	/* 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);
> +	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;
> @@ -2700,6 +2700,9 @@ static void rbd_dev_release(struct device *dev)
>   	rbd_free_disk(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 */
>   	kfree(rbd_dev->mapping.snap_name);
>   	kfree(rbd_dev->header_name);
>

[PATCH 7/7] rbd: simplify rbd_init_disk() a bit

[Alex Elder]

This just simplifies a few things in rbd_init_disk(), now that the
previous patch has moved a bunch of initialization code out if it.
Done separately to facilitate review.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 6e735a7..634a16c 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1870,14 +1870,12 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 {
 	struct gendisk *disk;
 	struct request_queue *q;
-	int rc;
 	u64 segment_size;

 	/* create gendisk info */
-	rc = -ENOMEM;
 	disk = alloc_disk(RBD_MINORS_PER_MAJOR);
 	if (!disk)
-		goto out;
+		return -ENOMEM;

 	snprintf(disk->disk_name, sizeof(disk->disk_name), RBD_DRV_NAME "%d",
 		 rbd_dev->dev_id);
@@ -1887,7 +1885,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 	disk->private_data = rbd_dev;

 	/* init rq */
-	rc = -ENOMEM;
 	q = blk_init_queue(rbd_rq_fn, &rbd_dev->lock);
 	if (!q)
 		goto out_disk;
@@ -1910,11 +1907,10 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 	rbd_dev->disk = disk;

 	return 0;
-
 out_disk:
 	put_disk(disk);
-out:
-	return rc;
+
+	return -ENOMEM;
 }

 /*
-- 
1.7.9.5

Re: [PATCH 7/7] rbd: simplify rbd_init_disk() a bit

[Josh Durgin]

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

On 09/07/2012 06:46 AM, Alex Elder wrote:
> This just simplifies a few things in rbd_init_disk(), now that the
> previous patch has moved a bunch of initialization code out if it.
> Done separately to facilitate review.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 6e735a7..634a16c 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -1870,14 +1870,12 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>   {
>   	struct gendisk *disk;
>   	struct request_queue *q;
> -	int rc;
>   	u64 segment_size;
>
>   	/* create gendisk info */
> -	rc = -ENOMEM;
>   	disk = alloc_disk(RBD_MINORS_PER_MAJOR);
>   	if (!disk)
> -		goto out;
> +		return -ENOMEM;
>
>   	snprintf(disk->disk_name, sizeof(disk->disk_name), RBD_DRV_NAME "%d",
>   		 rbd_dev->dev_id);
> @@ -1887,7 +1885,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>   	disk->private_data = rbd_dev;
>
>   	/* init rq */
> -	rc = -ENOMEM;
>   	q = blk_init_queue(rbd_rq_fn, &rbd_dev->lock);
>   	if (!q)
>   		goto out_disk;
> @@ -1910,11 +1907,10 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>   	rbd_dev->disk = disk;
>
>   	return 0;
> -
>   out_disk:
>   	put_disk(disk);
> -out:
> -	return rc;
> +
> +	return -ENOMEM;
>   }
>
>   /*
>