[PATCH 0/7] rbd: miscellaneous cleanups

[PATCH 0/7] rbd: miscellaneous cleanups

[Alex Elder]

Another batch of patches.  This set is available in the
branch "review/wip-rbd-cleanup-2" in the ceph-client git
repository, which is based on "review/wip-rbd-cleanup-1".

These are all simple cleanups, no substantive changes.

					-Alex

[PATCH 1/7] rbd: make rbd spec names pointer to const
[PATCH 2/7] rbd: move stripe_unit and stripe_count into header
[PATCH 3/7] rbd: use rbd_warn(), not WARN_ON()
[PATCH 4/7] rbd: define rbd snap context routines
[PATCH 5/7] rbd: make rbd_dev_destroy() match rbd_dev_create()
[PATCH 6/7] rbd: rename rbd_dev_probe()
[PATCH 7/7] rbd: refactor rbd_dev_probe_update_spec()

[PATCH 1/7] rbd: make rbd spec names pointer to const

[Alex Elder]

Make the names and image id in an rbd_spec be pointers to constant
data.  This required the use of a local variable to hold the
snapshot name in rbd_add_parse_args() to avoid a warning.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index cacbc30..d989914 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -138,13 +138,13 @@ struct rbd_image_header {
  */
 struct rbd_spec {
 	u64		pool_id;
-	char		*pool_name;
+	const char	*pool_name;

-	char		*image_id;
-	char		*image_name;
+	const char	*image_id;
+	const char	*image_name;

 	u64		snap_id;
-	char		*snap_name;
+	const char	*snap_name;

 	struct kref	kref;
 };
@@ -4375,6 +4375,7 @@ static int rbd_add_parse_args(const char *buf,
 	size_t len;
 	char *options;
 	const char *mon_addrs;
+	char *snap_name;
 	size_t mon_addrs_size;
 	struct rbd_spec *spec = NULL;
 	struct rbd_options *rbd_opts = NULL;
@@ -4433,10 +4434,11 @@ static int rbd_add_parse_args(const char *buf,
 		ret = -ENAMETOOLONG;
 		goto out_err;
 	}
-	spec->snap_name = kmemdup(buf, len + 1, GFP_KERNEL);
-	if (!spec->snap_name)
+	snap_name = kmemdup(buf, len + 1, GFP_KERNEL);
+	if (!snap_name)
 		goto out_mem;
-	*(spec->snap_name + len) = '\0';
+	*(snap_name + len) = '\0';
+	spec->snap_name = snap_name;

 	/* Initialize all rbd options to the defaults */

-- 
1.7.9.5

Re: [PATCH 1/7] rbd: make rbd spec names pointer to const

[Josh Durgin]

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

On 04/26/2013 10:59 AM, Alex Elder wrote:
> Make the names and image id in an rbd_spec be pointers to constant
> data.  This required the use of a local variable to hold the
> snapshot name in rbd_add_parse_args() to avoid a warning.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index cacbc30..d989914 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -138,13 +138,13 @@ struct rbd_image_header {
>    */
>   struct rbd_spec {
>   	u64		pool_id;
> -	char		*pool_name;
> +	const char	*pool_name;
>
> -	char		*image_id;
> -	char		*image_name;
> +	const char	*image_id;
> +	const char	*image_name;
>
>   	u64		snap_id;
> -	char		*snap_name;
> +	const char	*snap_name;
>
>   	struct kref	kref;
>   };
> @@ -4375,6 +4375,7 @@ static int rbd_add_parse_args(const char *buf,
>   	size_t len;
>   	char *options;
>   	const char *mon_addrs;
> +	char *snap_name;
>   	size_t mon_addrs_size;
>   	struct rbd_spec *spec = NULL;
>   	struct rbd_options *rbd_opts = NULL;
> @@ -4433,10 +4434,11 @@ static int rbd_add_parse_args(const char *buf,
>   		ret = -ENAMETOOLONG;
>   		goto out_err;
>   	}
> -	spec->snap_name = kmemdup(buf, len + 1, GFP_KERNEL);
> -	if (!spec->snap_name)
> +	snap_name = kmemdup(buf, len + 1, GFP_KERNEL);
> +	if (!snap_name)
>   		goto out_mem;
> -	*(spec->snap_name + len) = '\0';
> +	*(snap_name + len) = '\0';
> +	spec->snap_name = snap_name;
>
>   	/* Initialize all rbd options to the defaults */
>

[PATCH 2/7] rbd: move stripe_unit and stripe_count into header

[Alex Elder]

This commit added fetching if fancy striping parameters:
    09186ddb rbd: get and check striping parameters

They are almost unused, but the two fields storing the information
really belonged in the rbd_image_header structure.

This patch moves them there.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index d989914..fd4f678 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -108,6 +108,9 @@ struct rbd_image_header {
 	char *snap_names;
 	u64 *snap_sizes;

+	u64 stripe_unit;
+	u64 stripe_count;
+
 	u64 obj_version;
 };

@@ -316,9 +319,6 @@ struct rbd_device {
 	u64			parent_overlap;
 	struct rbd_device	*parent;

-	u64			stripe_unit;
-	u64			stripe_count;
-
 	/* protects updating the header */
 	struct rw_semaphore     header_rwsem;

@@ -3695,8 +3695,8 @@ static int rbd_dev_v2_striping_info(struct
rbd_device *rbd_dev)
 				"(got %llu want 1)", stripe_count);
 		return -EINVAL;
 	}
-	rbd_dev->stripe_unit = stripe_unit;
-	rbd_dev->stripe_count = stripe_count;
+	rbd_dev->header.stripe_unit = stripe_unit;
+	rbd_dev->header.stripe_count = stripe_count;

 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH 2/7] rbd: move stripe_unit and stripe_count into header

[Josh Durgin]

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

On 04/26/2013 11:00 AM, Alex Elder wrote:
> This commit added fetching if fancy striping parameters:
>      09186ddb rbd: get and check striping parameters
>
> They are almost unused, but the two fields storing the information
> really belonged in the rbd_image_header structure.
>
> This patch moves them there.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index d989914..fd4f678 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -108,6 +108,9 @@ struct rbd_image_header {
>   	char *snap_names;
>   	u64 *snap_sizes;
>
> +	u64 stripe_unit;
> +	u64 stripe_count;
> +
>   	u64 obj_version;
>   };
>
> @@ -316,9 +319,6 @@ struct rbd_device {
>   	u64			parent_overlap;
>   	struct rbd_device	*parent;
>
> -	u64			stripe_unit;
> -	u64			stripe_count;
> -
>   	/* protects updating the header */
>   	struct rw_semaphore     header_rwsem;
>
> @@ -3695,8 +3695,8 @@ static int rbd_dev_v2_striping_info(struct
> rbd_device *rbd_dev)
>   				"(got %llu want 1)", stripe_count);
>   		return -EINVAL;
>   	}
> -	rbd_dev->stripe_unit = stripe_unit;
> -	rbd_dev->stripe_count = stripe_count;
> +	rbd_dev->header.stripe_unit = stripe_unit;
> +	rbd_dev->header.stripe_count = stripe_count;
>
>   	return 0;
>   }
>

[PATCH 3/7] rbd: use rbd_warn(), not WARN_ON()

[Alex Elder]

Change some calls to WARN_ON() so they use rbd_warn() instead, so we
get consistent messaging.  A few remain but they can probably just
go away eventually.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index fd4f678..fe84975 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -777,7 +777,6 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 			header->snap_sizes[i] =
 				le64_to_cpu(ondisk->snaps[i].image_size);
 	} else {
-		WARN_ON(ondisk->snap_names_len);
 		header->snap_names = NULL;
 		header->snap_sizes = NULL;
 	}
@@ -2755,8 +2754,11 @@ static void rbd_request_fn(struct request_queue *q)
 		}

 		result = -EINVAL;
-		if (WARN_ON(offset && length > U64_MAX - offset + 1))
+		if (offset && length > U64_MAX - offset + 1) {
+			rbd_warn(rbd_dev, "bad request range (%llu~%llu)\n",
+				offset, length);
 			goto end_request;	/* Shouldn't happen */
+		}

 		result = -ENOMEM;
 		img_request = rbd_img_request_create(rbd_dev, offset, length,
@@ -2955,7 +2957,7 @@ rbd_dev_v1_header_read(struct rbd_device *rbd_dev,
u64 *version)
 				       0, size, ondisk, version);
 		if (ret < 0)
 			goto out_err;
-		if (WARN_ON((size_t) ret < size)) {
+		if ((size_t)ret < size) {
 			ret = -ENXIO;
 			rbd_warn(rbd_dev, "short header read (want %zd got %d)",
 				size, ret);
@@ -3057,7 +3059,8 @@ static int rbd_dev_v1_refresh(struct rbd_device
*rbd_dev, u64 *hver)
 	rbd_dev->header.snap_names = h.snap_names;
 	rbd_dev->header.snap_sizes = h.snap_sizes;
 	/* Free the extra copy of the object prefix */
-	WARN_ON(strcmp(rbd_dev->header.object_prefix, h.object_prefix));
+	if (strcmp(rbd_dev->header.object_prefix, h.object_prefix))
+		rbd_warn(rbd_dev, "object prefix changed (ignoring)");
 	kfree(h.object_prefix);

 	ret = rbd_dev_snaps_update(rbd_dev);
@@ -3627,8 +3630,11 @@ static int rbd_dev_v2_parent_info(struct
rbd_device *rbd_dev)
 	/* The ceph file layout needs to fit pool id in 32 bits */

 	ret = -EIO;
-	if (WARN_ON(parent_spec->pool_id > (u64)U32_MAX))
+	if (parent_spec->pool_id > (u64)U32_MAX) {
+		rbd_warn(NULL, "parent pool id too large (%llu > %u)\n",
+			(unsigned long long)parent_spec->pool_id, U32_MAX);
 		goto out_err;
+	}

 	image_id = ceph_extract_encoded_string(&p, end, NULL, GFP_KERNEL);
 	if (IS_ERR(image_id)) {
@@ -4859,11 +4865,13 @@ static ssize_t rbd_add(struct bus_type *bus,
 	rc = ceph_pg_poolid_by_name(osdc->osdmap, spec->pool_name);
 	if (rc < 0)
 		goto err_out_client;
-	spec->pool_id = (u64) rc;
+	spec->pool_id = (u64)rc;

 	/* The ceph file layout needs to fit pool id in 32 bits */

-	if (WARN_ON(spec->pool_id > (u64) U32_MAX)) {
+	if (spec->pool_id > (u64)U32_MAX) {
+		rbd_warn(NULL, "pool id too large (%llu > %u)\n",
+				(unsigned long long)spec->pool_id, U32_MAX);
 		rc = -EIO;
 		goto err_out_client;
 	}
@@ -4897,7 +4905,7 @@ err_out_module:

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

-	return (ssize_t) rc;
+	return (ssize_t)rc;
 }

 static struct rbd_device *__rbd_get_dev(unsigned long dev_id)
-- 
1.7.9.5

Re: [PATCH 3/7] rbd: use rbd_warn(), not WARN_ON()

[Josh Durgin]

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

On 04/26/2013 11:00 AM, Alex Elder wrote:
> Change some calls to WARN_ON() so they use rbd_warn() instead, so we
> get consistent messaging.  A few remain but they can probably just
> go away eventually.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index fd4f678..fe84975 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -777,7 +777,6 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>   			header->snap_sizes[i] =
>   				le64_to_cpu(ondisk->snaps[i].image_size);
>   	} else {
> -		WARN_ON(ondisk->snap_names_len);
>   		header->snap_names = NULL;
>   		header->snap_sizes = NULL;
>   	}
> @@ -2755,8 +2754,11 @@ static void rbd_request_fn(struct request_queue *q)
>   		}
>
>   		result = -EINVAL;
> -		if (WARN_ON(offset && length > U64_MAX - offset + 1))
> +		if (offset && length > U64_MAX - offset + 1) {
> +			rbd_warn(rbd_dev, "bad request range (%llu~%llu)\n",
> +				offset, length);
>   			goto end_request;	/* Shouldn't happen */
> +		}
>
>   		result = -ENOMEM;
>   		img_request = rbd_img_request_create(rbd_dev, offset, length,
> @@ -2955,7 +2957,7 @@ rbd_dev_v1_header_read(struct rbd_device *rbd_dev,
> u64 *version)
>   				       0, size, ondisk, version);
>   		if (ret < 0)
>   			goto out_err;
> -		if (WARN_ON((size_t) ret < size)) {
> +		if ((size_t)ret < size) {
>   			ret = -ENXIO;
>   			rbd_warn(rbd_dev, "short header read (want %zd got %d)",
>   				size, ret);
> @@ -3057,7 +3059,8 @@ static int rbd_dev_v1_refresh(struct rbd_device
> *rbd_dev, u64 *hver)
>   	rbd_dev->header.snap_names = h.snap_names;
>   	rbd_dev->header.snap_sizes = h.snap_sizes;
>   	/* Free the extra copy of the object prefix */
> -	WARN_ON(strcmp(rbd_dev->header.object_prefix, h.object_prefix));
> +	if (strcmp(rbd_dev->header.object_prefix, h.object_prefix))
> +		rbd_warn(rbd_dev, "object prefix changed (ignoring)");
>   	kfree(h.object_prefix);
>
>   	ret = rbd_dev_snaps_update(rbd_dev);
> @@ -3627,8 +3630,11 @@ static int rbd_dev_v2_parent_info(struct
> rbd_device *rbd_dev)
>   	/* The ceph file layout needs to fit pool id in 32 bits */
>
>   	ret = -EIO;
> -	if (WARN_ON(parent_spec->pool_id > (u64)U32_MAX))
> +	if (parent_spec->pool_id > (u64)U32_MAX) {
> +		rbd_warn(NULL, "parent pool id too large (%llu > %u)\n",
> +			(unsigned long long)parent_spec->pool_id, U32_MAX);
>   		goto out_err;
> +	}
>
>   	image_id = ceph_extract_encoded_string(&p, end, NULL, GFP_KERNEL);
>   	if (IS_ERR(image_id)) {
> @@ -4859,11 +4865,13 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	rc = ceph_pg_poolid_by_name(osdc->osdmap, spec->pool_name);
>   	if (rc < 0)
>   		goto err_out_client;
> -	spec->pool_id = (u64) rc;
> +	spec->pool_id = (u64)rc;
>
>   	/* The ceph file layout needs to fit pool id in 32 bits */
>
> -	if (WARN_ON(spec->pool_id > (u64) U32_MAX)) {
> +	if (spec->pool_id > (u64)U32_MAX) {
> +		rbd_warn(NULL, "pool id too large (%llu > %u)\n",
> +				(unsigned long long)spec->pool_id, U32_MAX);
>   		rc = -EIO;
>   		goto err_out_client;
>   	}
> @@ -4897,7 +4905,7 @@ err_out_module:
>
>   	dout("Error adding device %s\n", buf);
>
> -	return (ssize_t) rc;
> +	return (ssize_t)rc;
>   }
>
>   static struct rbd_device *__rbd_get_dev(unsigned long dev_id)
>

[PATCH 4/7] rbd: define rbd snap context routines

[Alex Elder]

Encapsulate the creation of a snapshot context for rbd in a new
function rbd_snap_context_create().  Define rbd wrappers for getting
and dropping references to them once they're created.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index fe84975..63040fd 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -671,6 +671,35 @@ static void rbd_client_release(struct kref *kref)
 	kfree(rbdc);
 }

+/* Caller has to fill in snapc->seq and snapc->snaps[0..snap_count-1] */
+
+static struct ceph_snap_context *rbd_snap_context_create(u32 snap_count)
+{
+	struct ceph_snap_context *snapc;
+	size_t size;
+
+	size = sizeof (struct ceph_snap_context);
+	size += snap_count * sizeof (snapc->snaps[0]);
+	snapc = kzalloc(size, GFP_KERNEL);
+	if (!snapc)
+		return NULL;
+
+	atomic_set(&snapc->nref, 1);
+	snapc->num_snaps = snap_count;
+
+	return snapc;
+}
+
+static inline void rbd_snap_context_get(struct ceph_snap_context *snapc)
+{
+	(void)ceph_get_snap_context(snapc);
+}
+
+static inline void rbd_snap_context_put(struct ceph_snap_context *snapc)
+{
+	ceph_put_snap_context(snapc);
+}
+
 /*
  * Drop reference to ceph client node. If it's not referenced anymore,
release
  * it.
@@ -789,18 +818,13 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 	/* 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);
+
+	header->snapc = rbd_snap_context_create(snap_count);
 	if (!header->snapc)
 		goto out_err;
-
-	atomic_set(&header->snapc->nref, 1);
 	header->snapc->seq = le64_to_cpu(ondisk->snap_seq);
-	header->snapc->num_snaps = snap_count;
 	for (i = 0; i < snap_count; i++)
-		header->snapc->snaps[i] =
-			le64_to_cpu(ondisk->snaps[i].id);
+		header->snapc->snaps[i] = le64_to_cpu(ondisk->snaps[i].id);

 	return 0;

@@ -870,7 +894,7 @@ static void rbd_header_free(struct rbd_image_header
*header)
 	header->snap_sizes = NULL;
 	kfree(header->snap_names);
 	header->snap_names = NULL;
-	ceph_put_snap_context(header->snapc);
+	rbd_snap_context_put(header->snapc);
 	header->snapc = NULL;
 }

@@ -1720,7 +1744,6 @@ static struct rbd_img_request *rbd_img_request_create(
 					bool child_request)
 {
 	struct rbd_img_request *img_request;
-	struct ceph_snap_context *snapc = NULL;

 	img_request = kmalloc(sizeof (*img_request), GFP_ATOMIC);
 	if (!img_request)
@@ -1728,13 +1751,8 @@ static struct rbd_img_request
*rbd_img_request_create(

 	if (write_request) {
 		down_read(&rbd_dev->header_rwsem);
-		snapc = ceph_get_snap_context(rbd_dev->header.snapc);
+		rbd_snap_context_get(rbd_dev->header.snapc);
 		up_read(&rbd_dev->header_rwsem);
-		if (WARN_ON(!snapc)) {
-			kfree(img_request);
-			return NULL;	/* Shouldn't happen */
-		}
-
 	}

 	img_request->rq = NULL;
@@ -1744,7 +1762,7 @@ static struct rbd_img_request *rbd_img_request_create(
 	img_request->flags = 0;
 	if (write_request) {
 		img_request_write_set(img_request);
-		img_request->snapc = snapc;
+		img_request->snapc = rbd_dev->header.snapc;
 	} else {
 		img_request->snap_id = rbd_dev->spec->snap_id;
 	}
@@ -1785,7 +1803,7 @@ static void rbd_img_request_destroy(struct kref *kref)
 	rbd_assert(img_request->obj_request_count == 0);

 	if (img_request_write_test(img_request))
-		ceph_put_snap_context(img_request->snapc);
+		rbd_snap_context_put(img_request->snapc);

 	if (img_request_child_test(img_request))
 		rbd_obj_request_put(img_request->obj_request);
@@ -3049,7 +3067,7 @@ static int rbd_dev_v1_refresh(struct rbd_device
*rbd_dev, u64 *hver)
 	kfree(rbd_dev->header.snap_sizes);
 	kfree(rbd_dev->header.snap_names);
 	/* osd requests may still refer to snapc */
-	ceph_put_snap_context(rbd_dev->header.snapc);
+	rbd_snap_context_put(rbd_dev->header.snapc);

 	if (hver)
 		*hver = h.obj_version;
@@ -3889,19 +3907,14 @@ static int rbd_dev_v2_snap_context(struct
rbd_device *rbd_dev, u64 *ver)
 	}
 	if (!ceph_has_room(&p, end, snap_count * sizeof (__le64)))
 		goto out;
+	ret = 0;

-	size = sizeof (struct ceph_snap_context) +
-				snap_count * sizeof (snapc->snaps[0]);
-	snapc = kmalloc(size, GFP_KERNEL);
+	snapc = rbd_snap_context_create(snap_count);
 	if (!snapc) {
 		ret = -ENOMEM;
 		goto out;
 	}
-	ret = 0;
-
-	atomic_set(&snapc->nref, 1);
 	snapc->seq = seq;
-	snapc->num_snaps = snap_count;
 	for (i = 0; i < snap_count; i++)
 		snapc->snaps[i] = ceph_decode_64(&p);

-- 
1.7.9.5

Re: [PATCH 4/7] rbd: define rbd snap context routines

[Josh Durgin]

On 04/26/2013 11:00 AM, Alex Elder wrote:
> Encapsulate the creation of a snapshot context for rbd in a new
> function rbd_snap_context_create().  Define rbd wrappers for getting
> and dropping references to them once they're created.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   65
> ++++++++++++++++++++++++++++++---------------------
>   1 file changed, 39 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index fe84975..63040fd 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -671,6 +671,35 @@ static void rbd_client_release(struct kref *kref)
>   	kfree(rbdc);
>   }
>
> +/* Caller has to fill in snapc->seq and snapc->snaps[0..snap_count-1] */
> +
> +static struct ceph_snap_context *rbd_snap_context_create(u32 snap_count)
> +{
> +	struct ceph_snap_context *snapc;
> +	size_t size;
> +
> +	size = sizeof (struct ceph_snap_context);
> +	size += snap_count * sizeof (snapc->snaps[0]);
> +	snapc = kzalloc(size, GFP_KERNEL);
> +	if (!snapc)
> +		return NULL;
> +
> +	atomic_set(&snapc->nref, 1);
> +	snapc->num_snaps = snap_count;
> +
> +	return snapc;
> +}

It seems like this should go in libceph.h, alongside
ceph_[get|put]_snap_context(). fs/ceph/snap.c has very similar code too.
I wouldn't expect rbd to need anything specialized with its snapshot
contexts.

> +static inline void rbd_snap_context_get(struct ceph_snap_context *snapc)
> +{
> +	(void)ceph_get_snap_context(snapc);
> +}
> +
> +static inline void rbd_snap_context_put(struct ceph_snap_context *snapc)
> +{
> +	ceph_put_snap_context(snapc);
> +}
> +
>   /*
>    * Drop reference to ceph client node. If it's not referenced anymore,
> release
>    * it.
> @@ -789,18 +818,13 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>   	/* 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);
> +
> +	header->snapc = rbd_snap_context_create(snap_count);
>   	if (!header->snapc)
>   		goto out_err;
> -
> -	atomic_set(&header->snapc->nref, 1);
>   	header->snapc->seq = le64_to_cpu(ondisk->snap_seq);
> -	header->snapc->num_snaps = snap_count;
>   	for (i = 0; i < snap_count; i++)
> -		header->snapc->snaps[i] =
> -			le64_to_cpu(ondisk->snaps[i].id);
> +		header->snapc->snaps[i] = le64_to_cpu(ondisk->snaps[i].id);
>
>   	return 0;
>
> @@ -870,7 +894,7 @@ static void rbd_header_free(struct rbd_image_header
> *header)
>   	header->snap_sizes = NULL;
>   	kfree(header->snap_names);
>   	header->snap_names = NULL;
> -	ceph_put_snap_context(header->snapc);
> +	rbd_snap_context_put(header->snapc);
>   	header->snapc = NULL;
>   }
>
> @@ -1720,7 +1744,6 @@ static struct rbd_img_request *rbd_img_request_create(
>   					bool child_request)
>   {
>   	struct rbd_img_request *img_request;
> -	struct ceph_snap_context *snapc = NULL;
>
>   	img_request = kmalloc(sizeof (*img_request), GFP_ATOMIC);
>   	if (!img_request)
> @@ -1728,13 +1751,8 @@ static struct rbd_img_request
> *rbd_img_request_create(
>
>   	if (write_request) {
>   		down_read(&rbd_dev->header_rwsem);
> -		snapc = ceph_get_snap_context(rbd_dev->header.snapc);
> +		rbd_snap_context_get(rbd_dev->header.snapc);
>   		up_read(&rbd_dev->header_rwsem);
> -		if (WARN_ON(!snapc)) {
> -			kfree(img_request);
> -			return NULL;	/* Shouldn't happen */
> -		}
> -
>   	}
>
>   	img_request->rq = NULL;
> @@ -1744,7 +1762,7 @@ static struct rbd_img_request *rbd_img_request_create(
>   	img_request->flags = 0;
>   	if (write_request) {
>   		img_request_write_set(img_request);
> -		img_request->snapc = snapc;
> +		img_request->snapc = rbd_dev->header.snapc;
>   	} else {
>   		img_request->snap_id = rbd_dev->spec->snap_id;
>   	}
> @@ -1785,7 +1803,7 @@ static void rbd_img_request_destroy(struct kref *kref)
>   	rbd_assert(img_request->obj_request_count == 0);
>
>   	if (img_request_write_test(img_request))
> -		ceph_put_snap_context(img_request->snapc);
> +		rbd_snap_context_put(img_request->snapc);
>
>   	if (img_request_child_test(img_request))
>   		rbd_obj_request_put(img_request->obj_request);
> @@ -3049,7 +3067,7 @@ static int rbd_dev_v1_refresh(struct rbd_device
> *rbd_dev, u64 *hver)
>   	kfree(rbd_dev->header.snap_sizes);
>   	kfree(rbd_dev->header.snap_names);
>   	/* osd requests may still refer to snapc */
> -	ceph_put_snap_context(rbd_dev->header.snapc);
> +	rbd_snap_context_put(rbd_dev->header.snapc);
>
>   	if (hver)
>   		*hver = h.obj_version;
> @@ -3889,19 +3907,14 @@ static int rbd_dev_v2_snap_context(struct
> rbd_device *rbd_dev, u64 *ver)
>   	}
>   	if (!ceph_has_room(&p, end, snap_count * sizeof (__le64)))
>   		goto out;
> +	ret = 0;
>
> -	size = sizeof (struct ceph_snap_context) +
> -				snap_count * sizeof (snapc->snaps[0]);
> -	snapc = kmalloc(size, GFP_KERNEL);
> +	snapc = rbd_snap_context_create(snap_count);
>   	if (!snapc) {
>   		ret = -ENOMEM;
>   		goto out;
>   	}
> -	ret = 0;
> -
> -	atomic_set(&snapc->nref, 1);
>   	snapc->seq = seq;
> -	snapc->num_snaps = snap_count;
>   	for (i = 0; i < snap_count; i++)
>   		snapc->snaps[i] = ceph_decode_64(&p);
>

[PATCH 5/7] rbd: make rbd_dev_destroy() match rbd_dev_create()

[Alex Elder]

Currently, rbd_dev_destroy() does more than just the inverse of what
rbd_dev_create() does.  Stop doing that, and move the two extra
things it does into the three call sites.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 63040fd..0356bba 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3425,8 +3425,6 @@ static struct rbd_device *rbd_dev_create(struct
rbd_client *rbdc,

 static void rbd_dev_destroy(struct rbd_device *rbd_dev)
 {
-	rbd_spec_put(rbd_dev->parent_spec);
-	kfree(rbd_dev->header_name);
 	rbd_put_client(rbd_dev->rbd_client);
 	rbd_spec_put(rbd_dev->spec);
 	kfree(rbd_dev);
@@ -4784,6 +4782,8 @@ static int rbd_dev_probe_finish(struct rbd_device
*rbd_dev)
 	return ret;

 err_out_parent:
+	rbd_spec_put(rbd_dev->parent_spec);
+	kfree(rbd_dev->header_name);
 	rbd_dev_destroy(parent);
 err_out_spec:
 	rbd_spec_put(parent_spec);
@@ -4905,6 +4905,8 @@ static ssize_t rbd_add(struct bus_type *bus,

 	return count;
 err_out_rbd_dev:
+	rbd_spec_put(rbd_dev->parent_spec);
+	kfree(rbd_dev->header_name);
 	rbd_dev_destroy(rbd_dev);
 err_out_client:
 	rbd_put_client(rbdc);
@@ -4955,6 +4957,8 @@ static void rbd_dev_release(struct device *dev)
 	/* done with the id, and with the rbd_dev */
 	rbd_dev_id_put(rbd_dev);
 	rbd_assert(rbd_dev->rbd_client != NULL);
+	rbd_spec_put(rbd_dev->parent_spec);
+	kfree(rbd_dev->header_name);
 	rbd_dev_destroy(rbd_dev);

 	/* release module ref */
-- 
1.7.9.5

Re: [PATCH 5/7] rbd: make rbd_dev_destroy() match rbd_dev_create()

[Josh Durgin]

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

On 04/26/2013 11:00 AM, Alex Elder wrote:
> Currently, rbd_dev_destroy() does more than just the inverse of what
> rbd_dev_create() does.  Stop doing that, and move the two extra
> things it does into the three call sites.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 63040fd..0356bba 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -3425,8 +3425,6 @@ static struct rbd_device *rbd_dev_create(struct
> rbd_client *rbdc,
>
>   static void rbd_dev_destroy(struct rbd_device *rbd_dev)
>   {
> -	rbd_spec_put(rbd_dev->parent_spec);
> -	kfree(rbd_dev->header_name);
>   	rbd_put_client(rbd_dev->rbd_client);
>   	rbd_spec_put(rbd_dev->spec);
>   	kfree(rbd_dev);
> @@ -4784,6 +4782,8 @@ static int rbd_dev_probe_finish(struct rbd_device
> *rbd_dev)
>   	return ret;
>
>   err_out_parent:
> +	rbd_spec_put(rbd_dev->parent_spec);
> +	kfree(rbd_dev->header_name);
>   	rbd_dev_destroy(parent);
>   err_out_spec:
>   	rbd_spec_put(parent_spec);
> @@ -4905,6 +4905,8 @@ static ssize_t rbd_add(struct bus_type *bus,
>
>   	return count;
>   err_out_rbd_dev:
> +	rbd_spec_put(rbd_dev->parent_spec);
> +	kfree(rbd_dev->header_name);
>   	rbd_dev_destroy(rbd_dev);
>   err_out_client:
>   	rbd_put_client(rbdc);
> @@ -4955,6 +4957,8 @@ static void rbd_dev_release(struct device *dev)
>   	/* done with the id, and with the rbd_dev */
>   	rbd_dev_id_put(rbd_dev);
>   	rbd_assert(rbd_dev->rbd_client != NULL);
> +	rbd_spec_put(rbd_dev->parent_spec);
> +	kfree(rbd_dev->header_name);
>   	rbd_dev_destroy(rbd_dev);
>
>   	/* release module ref */
>

[PATCH 6/7] rbd: rename rbd_dev_probe()

[Alex Elder]

Rename rbd_dev_probe() to be rbd_dev_probe_image().  Its purpose
will eventually be to probe for the existence of a valid rbd image
for the rbd device--focusing only on the ceph side and not the Linux
device side of initialization.

For now the two "sides" are not fully separated, and this function
is still the entry point for initializing the full rbd device.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 0356bba..f65310c6 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -365,7 +365,7 @@ static ssize_t rbd_add(struct bus_type *bus, const
char *buf,
 		       size_t count);
 static ssize_t rbd_remove(struct bus_type *bus, const char *buf,
 			  size_t count);
-static int rbd_dev_probe(struct rbd_device *rbd_dev);
+static int rbd_dev_probe_image(struct rbd_device *rbd_dev);

 static struct bus_attribute rbd_bus_attrs[] = {
 	__ATTR(add, S_IWUSR, NULL, rbd_add),
@@ -4762,7 +4762,7 @@ static int rbd_dev_probe_finish(struct rbd_device
*rbd_dev)
 		}
 		rbdc = NULL;		/* parent now owns reference */
 		parent_spec = NULL;	/* parent now owns reference */
-		ret = rbd_dev_probe(parent);
+		ret = rbd_dev_probe_image(parent);
 		if (ret < 0)
 			goto err_out_parent;
 		rbd_dev->parent = parent;
@@ -4811,7 +4811,7 @@ err_out_snaps:
  * device.  For format 2 images this includes determining the image
  * id.
  */
-static int rbd_dev_probe(struct rbd_device *rbd_dev)
+static int rbd_dev_probe_image(struct rbd_device *rbd_dev)
 {
 	int ret;

@@ -4899,7 +4899,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	kfree(rbd_opts);
 	rbd_opts = NULL;	/* done with this */

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

-- 
1.7.9.5

Re: [PATCH 6/7] rbd: rename rbd_dev_probe()

[Josh Durgin]

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

On 04/26/2013 11:01 AM, Alex Elder wrote:
> Rename rbd_dev_probe() to be rbd_dev_probe_image().  Its purpose
> will eventually be to probe for the existence of a valid rbd image
> for the rbd device--focusing only on the ceph side and not the Linux
> device side of initialization.
>
> For now the two "sides" are not fully separated, and this function
> is still the entry point for initializing the full rbd device.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 0356bba..f65310c6 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -365,7 +365,7 @@ static ssize_t rbd_add(struct bus_type *bus, const
> char *buf,
>   		       size_t count);
>   static ssize_t rbd_remove(struct bus_type *bus, const char *buf,
>   			  size_t count);
> -static int rbd_dev_probe(struct rbd_device *rbd_dev);
> +static int rbd_dev_probe_image(struct rbd_device *rbd_dev);
>
>   static struct bus_attribute rbd_bus_attrs[] = {
>   	__ATTR(add, S_IWUSR, NULL, rbd_add),
> @@ -4762,7 +4762,7 @@ static int rbd_dev_probe_finish(struct rbd_device
> *rbd_dev)
>   		}
>   		rbdc = NULL;		/* parent now owns reference */
>   		parent_spec = NULL;	/* parent now owns reference */
> -		ret = rbd_dev_probe(parent);
> +		ret = rbd_dev_probe_image(parent);
>   		if (ret < 0)
>   			goto err_out_parent;
>   		rbd_dev->parent = parent;
> @@ -4811,7 +4811,7 @@ err_out_snaps:
>    * device.  For format 2 images this includes determining the image
>    * id.
>    */
> -static int rbd_dev_probe(struct rbd_device *rbd_dev)
> +static int rbd_dev_probe_image(struct rbd_device *rbd_dev)
>   {
>   	int ret;
>
> @@ -4899,7 +4899,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	kfree(rbd_opts);
>   	rbd_opts = NULL;	/* done with this */
>
> -	rc = rbd_dev_probe(rbd_dev);
> +	rc = rbd_dev_probe_image(rbd_dev);
>   	if (rc < 0)
>   		goto err_out_rbd_dev;
>

[PATCH 7/7] rbd: refactor rbd_dev_probe_update_spec()

[Alex Elder]

Fairly straightforward refactoring of rbd_dev_probe_update_spec().
The name is changed to rbd_dev_spec_update().

Rearrange it so nothing gets assigned to the spec until all of the
names have been successfully acquired.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index f65310c6..5918e0b 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3774,83 +3774,86 @@ out:
 }

 /*
- * When a parent image gets probed, we only have the pool, image,
- * and snapshot ids but not the names of any of them.  This call
- * is made later to fill in those names.  It has to be done after
- * rbd_dev_snaps_update() has completed because some of the
- * information (in particular, snapshot name) is not available
- * until then.
+ * When an rbd image has a parent image, it is identified by the
+ * pool, image, and snapshot ids (not names).  This function fills
+ * in the names for those ids.  (It's OK if we can't figure out the
+ * name for an image id, but the pool and snapshot ids should always
+ * exist and have names.)  All names in an rbd spec are dynamically
+ * allocated.
  *
  * When an image being mapped (not a parent) is probed, we have the
  * pool name and pool id, image name and image id, and the snapshot
  * name.  The only thing we're missing is the snapshot id.
+ *
+ * The set of snapshots for an image is not known until they have
+ * been read by rbd_dev_snaps_update(), so we can't completely fill
+ * in this information until after that has been called.
  */
-static int rbd_dev_probe_update_spec(struct rbd_device *rbd_dev)
+static int rbd_dev_spec_update(struct rbd_device *rbd_dev)
 {
-	struct ceph_osd_client *osdc;
-	const char *name;
-	void *reply_buf = NULL;
+	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
+	struct rbd_spec *spec = rbd_dev->spec;
+	const char *pool_name;
+	const char *image_name;
+	const char *snap_name;
 	int ret;

 	/*
 	 * An image being mapped will have the pool name (etc.), but
 	 * we need to look up the snapshot id.
 	 */
-	if (rbd_dev->spec->pool_name) {
-		if (strcmp(rbd_dev->spec->snap_name, RBD_SNAP_HEAD_NAME)) {
+	if (spec->pool_name) {
+		if (strcmp(spec->snap_name, RBD_SNAP_HEAD_NAME)) {
 			struct rbd_snap *snap;

-			snap = snap_by_name(rbd_dev, rbd_dev->spec->snap_name);
+			snap = snap_by_name(rbd_dev, spec->snap_name);
 			if (!snap)
 				return -ENOENT;
-			rbd_dev->spec->snap_id = snap->id;
+			spec->snap_id = snap->id;
 		} else {
-			rbd_dev->spec->snap_id = CEPH_NOSNAP;
+			spec->snap_id = CEPH_NOSNAP;
 		}

 		return 0;
 	}

-	/* Look up the pool name */
+	/* Get the pool name; we have to make our own copy of this */

-	osdc = &rbd_dev->rbd_client->client->osdc;
-	name = ceph_pg_pool_name_by_id(osdc->osdmap, rbd_dev->spec->pool_id);
-	if (!name) {
-		rbd_warn(rbd_dev, "there is no pool with id %llu",
-			rbd_dev->spec->pool_id);	/* Really a BUG() */
+	pool_name = ceph_pg_pool_name_by_id(osdc->osdmap, spec->pool_id);
+	if (!pool_name) {
+		rbd_warn(rbd_dev, "no pool with id %llu", spec->pool_id);
 		return -EIO;
 	}
-
-	rbd_dev->spec->pool_name = kstrdup(name, GFP_KERNEL);
-	if (!rbd_dev->spec->pool_name)
+	pool_name = kstrdup(pool_name, GFP_KERNEL);
+	if (!pool_name)
 		return -ENOMEM;

 	/* Fetch the image name; tolerate failure here */

-	name = rbd_dev_image_name(rbd_dev);
-	if (name)
-		rbd_dev->spec->image_name = (char *)name;
-	else
+	image_name = rbd_dev_image_name(rbd_dev);
+	if (!image_name)
 		rbd_warn(rbd_dev, "unable to get image name");

-	/* Look up the snapshot name. */
+	/* Look up the snapshot name, and make a copy */

-	name = rbd_snap_name(rbd_dev, rbd_dev->spec->snap_id);
-	if (!name) {
-		rbd_warn(rbd_dev, "no snapshot with id %llu",
-			rbd_dev->spec->snap_id);	/* Really a BUG() */
+	snap_name = rbd_snap_name(rbd_dev, spec->snap_id);
+	if (!snap_name) {
+		rbd_warn(rbd_dev, "no snapshot with id %llu", spec->snap_id);
 		ret = -EIO;
 		goto out_err;
 	}
-	rbd_dev->spec->snap_name = kstrdup(name, GFP_KERNEL);
-	if(!rbd_dev->spec->snap_name)
+	snap_name = kstrdup(snap_name, GFP_KERNEL);
+	if (!snap_name)
 		goto out_err;

+	spec->pool_name = pool_name;
+	spec->image_name = image_name;
+	spec->snap_name = snap_name;
+
 	return 0;
 out_err:
-	kfree(reply_buf);
-	kfree(rbd_dev->spec->pool_name);
-	rbd_dev->spec->pool_name = NULL;
+	kfree(image_name);
+	kfree(pool_name);

 	return ret;
 }
@@ -4706,7 +4709,7 @@ static int rbd_dev_probe_finish(struct rbd_device
*rbd_dev)
 	if (ret)
 		return ret;

-	ret = rbd_dev_probe_update_spec(rbd_dev);
+	ret = rbd_dev_spec_update(rbd_dev);
 	if (ret)
 		goto err_out_snaps;

-- 
1.7.9.5

Re: [PATCH 7/7] rbd: refactor rbd_dev_probe_update_spec()

[Josh Durgin]

On 04/26/2013 11:01 AM, Alex Elder wrote:
> Fairly straightforward refactoring of rbd_dev_probe_update_spec().
> The name is changed to rbd_dev_spec_update().
>
> Rearrange it so nothing gets assigned to the spec until all of the
> names have been successfully acquired.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   81
> ++++++++++++++++++++++++++-------------------------
>   1 file changed, 42 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index f65310c6..5918e0b 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -3774,83 +3774,86 @@ out:
>   }
>
>   /*
> - * When a parent image gets probed, we only have the pool, image,
> - * and snapshot ids but not the names of any of them.  This call
> - * is made later to fill in those names.  It has to be done after
> - * rbd_dev_snaps_update() has completed because some of the
> - * information (in particular, snapshot name) is not available
> - * until then.
> + * When an rbd image has a parent image, it is identified by the
> + * pool, image, and snapshot ids (not names).  This function fills
> + * in the names for those ids.  (It's OK if we can't figure out the
> + * name for an image id, but the pool and snapshot ids should always
> + * exist and have names.)  All names in an rbd spec are dynamically
> + * allocated.
>    *
>    * When an image being mapped (not a parent) is probed, we have the
>    * pool name and pool id, image name and image id, and the snapshot
>    * name.  The only thing we're missing is the snapshot id.
> + *
> + * The set of snapshots for an image is not known until they have
> + * been read by rbd_dev_snaps_update(), so we can't completely fill
> + * in this information until after that has been called.
>    */
> -static int rbd_dev_probe_update_spec(struct rbd_device *rbd_dev)
> +static int rbd_dev_spec_update(struct rbd_device *rbd_dev)
>   {
> -	struct ceph_osd_client *osdc;
> -	const char *name;
> -	void *reply_buf = NULL;
> +	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
> +	struct rbd_spec *spec = rbd_dev->spec;
> +	const char *pool_name;
> +	const char *image_name;
> +	const char *snap_name;
>   	int ret;
>
>   	/*
>   	 * An image being mapped will have the pool name (etc.), but
>   	 * we need to look up the snapshot id.
>   	 */
> -	if (rbd_dev->spec->pool_name) {
> -		if (strcmp(rbd_dev->spec->snap_name, RBD_SNAP_HEAD_NAME)) {
> +	if (spec->pool_name) {
> +		if (strcmp(spec->snap_name, RBD_SNAP_HEAD_NAME)) {
>   			struct rbd_snap *snap;
>
> -			snap = snap_by_name(rbd_dev, rbd_dev->spec->snap_name);
> +			snap = snap_by_name(rbd_dev, spec->snap_name);
>   			if (!snap)
>   				return -ENOENT;
> -			rbd_dev->spec->snap_id = snap->id;
> +			spec->snap_id = snap->id;
>   		} else {
> -			rbd_dev->spec->snap_id = CEPH_NOSNAP;
> +			spec->snap_id = CEPH_NOSNAP;
>   		}
>
>   		return 0;
>   	}
>
> -	/* Look up the pool name */
> +	/* Get the pool name; we have to make our own copy of this */
>
> -	osdc = &rbd_dev->rbd_client->client->osdc;
> -	name = ceph_pg_pool_name_by_id(osdc->osdmap, rbd_dev->spec->pool_id);
> -	if (!name) {
> -		rbd_warn(rbd_dev, "there is no pool with id %llu",
> -			rbd_dev->spec->pool_id);	/* Really a BUG() */
> +	pool_name = ceph_pg_pool_name_by_id(osdc->osdmap, spec->pool_id);
> +	if (!pool_name) {
> +		rbd_warn(rbd_dev, "no pool with id %llu", spec->pool_id);
>   		return -EIO;
>   	}
> -
> -	rbd_dev->spec->pool_name = kstrdup(name, GFP_KERNEL);
> -	if (!rbd_dev->spec->pool_name)
> +	pool_name = kstrdup(pool_name, GFP_KERNEL);
> +	if (!pool_name)
>   		return -ENOMEM;
>
>   	/* Fetch the image name; tolerate failure here */
>
> -	name = rbd_dev_image_name(rbd_dev);
> -	if (name)
> -		rbd_dev->spec->image_name = (char *)name;
> -	else
> +	image_name = rbd_dev_image_name(rbd_dev);
> +	if (!image_name)
>   		rbd_warn(rbd_dev, "unable to get image name");
>
> -	/* Look up the snapshot name. */
> +	/* Look up the snapshot name, and make a copy */
>
> -	name = rbd_snap_name(rbd_dev, rbd_dev->spec->snap_id);
> -	if (!name) {
> -		rbd_warn(rbd_dev, "no snapshot with id %llu",
> -			rbd_dev->spec->snap_id);	/* Really a BUG() */
> +	snap_name = rbd_snap_name(rbd_dev, spec->snap_id);
> +	if (!snap_name) {
> +		rbd_warn(rbd_dev, "no snapshot with id %llu", spec->snap_id);
>   		ret = -EIO;
>   		goto out_err;
>   	}
> -	rbd_dev->spec->snap_name = kstrdup(name, GFP_KERNEL);
> -	if(!rbd_dev->spec->snap_name)
> +	snap_name = kstrdup(snap_name, GFP_KERNEL);
> +	if (!snap_name)

Probably want ret = -ENOMEM here. With that fixed:

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

>   		goto out_err;
>
> +	spec->pool_name = pool_name;
> +	spec->image_name = image_name;
> +	spec->snap_name = snap_name;
> +
>   	return 0;
>   out_err:
> -	kfree(reply_buf);
> -	kfree(rbd_dev->spec->pool_name);
> -	rbd_dev->spec->pool_name = NULL;
> +	kfree(image_name);
> +	kfree(pool_name);
>
>   	return ret;
>   }
> @@ -4706,7 +4709,7 @@ static int rbd_dev_probe_finish(struct rbd_device
> *rbd_dev)
>   	if (ret)
>   		return ret;
>
> -	ret = rbd_dev_probe_update_spec(rbd_dev);
> +	ret = rbd_dev_spec_update(rbd_dev);
>   	if (ret)
>   		goto err_out_snaps;
>