[PATCH REPOST 0/4] rbd: four minor patches

[PATCH REPOST 0/4] rbd: four minor patches

[Alex Elder]

I'm re-posting my patch backlog, in chunks that may or may not
match how they got posted before.  This series contains some
pretty fairly straightforward changes.

					-Alex

[PATCH REPOST 1/4] rbd: document rbd_spec structure
[PATCH REPOST 2/4] rbd: kill rbd_spec->image_name_len
[PATCH REPOST 3/4] rbd: kill rbd_spec->image_id_len
[PATCH REPOST 4/4] rbd: use kmemdup()

[PATCH REPOST 1/4] rbd: document rbd_spec structure

[Alex Elder]

I promised Josh I would document whether there were any restrictions
needed for accessing fields of an rbd_spec structure.  This adds a
big block of comments that documents the structure and how it is
used--including the fact that we don't attempt to synchronize access
to it.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 89576a0..128978c 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -119,7 +119,26 @@ struct rbd_image_header {
  * An rbd image specification.
  *
  * The tuple (pool_id, image_id, snap_id) is sufficient to uniquely
- * identify an image.
+ * identify an image.  Each rbd_dev structure includes a pointer to
+ * an rbd_spec structure that encapsulates this identity.
+ *
+ * Each of the id's in an rbd_spec has an associated name.  For a
+ * user-mapped image, the names are supplied and the id's associated
+ * with them are looked up.  For a layered image, a parent image is
+ * defined by the tuple, and the names are looked up.
+ *
+ * An rbd_dev structure contains a parent_spec pointer which is
+ * non-null if the image it represents is a child in a layered
+ * image.  This pointer will refer to the rbd_spec structure used
+ * by the parent rbd_dev for its own identity (i.e., the structure
+ * is shared between the parent and child).
+ *
+ * Since these structures are populated once, during the discovery
+ * phase of image construction, they are effectively immutable so
+ * we make no effort to synchronize access to them.
+ *
+ * Note that code herein does not assume the image name is known (it
+ * could be a null pointer).
  */
 struct rbd_spec {
 	u64		pool_id;
-- 
1.7.9.5

[PATCH REPOST 2/4] rbd: kill rbd_spec->image_name_len

[Alex Elder]

There may have been a benefit to hanging on to the length of an
image name before, but there is really none now.  The only time it's
used is when probing for rbd images, so we can just compute the
length then.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 128978c..a002984 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -147,7 +147,6 @@ struct rbd_spec {
 	char		*image_id;
 	size_t		image_id_len;
 	char		*image_name;
-	size_t		image_name_len;

 	u64		snap_id;
 	char		*snap_name;
@@ -2563,15 +2562,15 @@ static char *rbd_dev_image_name(struct
rbd_device *rbd_dev)

 	rbd_assert(!rbd_dev->spec->image_name);

-	image_id_size = sizeof (__le32) + rbd_dev->spec->image_id_len;
+	len = strlen(rbd_dev->spec->image_id);
+	image_id_size = sizeof (__le32) + len;
 	image_id = kmalloc(image_id_size, GFP_KERNEL);
 	if (!image_id)
 		return NULL;

 	p = image_id;
 	end = (char *) image_id + image_id_size;
-	ceph_encode_string(&p, end, rbd_dev->spec->image_id,
-				(u32) rbd_dev->spec->image_id_len);
+	ceph_encode_string(&p, end, rbd_dev->spec->image_id, (u32) len);

 	size = sizeof (__le32) + RBD_IMAGE_NAME_LEN_MAX;
 	reply_buf = kmalloc(size, GFP_KERNEL);
@@ -2631,14 +2630,12 @@ static int rbd_dev_probe_update_spec(struct
rbd_device *rbd_dev)
 	/* Fetch the image name; tolerate failure here */

 	name = rbd_dev_image_name(rbd_dev);
-	if (name) {
-		rbd_dev->spec->image_name_len = strlen(name);
+	if (name)
 		rbd_dev->spec->image_name = (char *) name;
-	} else {
+	else
 		pr_warning(RBD_DRV_NAME "%d "
 			"unable to get image name for image id %s\n",
 			rbd_dev->major, rbd_dev->spec->image_id);
-	}

 	/* Look up the snapshot name. */

@@ -3252,7 +3249,7 @@ static int rbd_add_parse_args(const char *buf,
 	if (!*spec->pool_name)
 		goto out_err;	/* Missing pool name */

-	spec->image_name = dup_token(&buf, &spec->image_name_len);
+	spec->image_name = dup_token(&buf, NULL);
 	if (!spec->image_name)
 		goto out_mem;
 	if (!*spec->image_name)
@@ -3342,7 +3339,7 @@ static int rbd_dev_image_id(struct rbd_device
*rbd_dev)
 	 * First, see if the format 2 image id file exists, and if
 	 * so, get the image's persistent id from it.
 	 */
-	size = sizeof (RBD_ID_PREFIX) + rbd_dev->spec->image_name_len;
+	size = sizeof (RBD_ID_PREFIX) + strlen(rbd_dev->spec->image_name);
 	object_name = kmalloc(size, GFP_NOIO);
 	if (!object_name)
 		return -ENOMEM;
@@ -3400,7 +3397,7 @@ static int rbd_dev_v1_probe(struct rbd_device
*rbd_dev)

 	/* Record the header object name for this rbd image. */

-	size = rbd_dev->spec->image_name_len + sizeof (RBD_SUFFIX);
+	size = strlen(rbd_dev->spec->image_name) + sizeof (RBD_SUFFIX);
 	rbd_dev->header_name = kmalloc(size, GFP_KERNEL);
 	if (!rbd_dev->header_name) {
 		ret = -ENOMEM;
-- 
1.7.9.5

[PATCH REPOST 3/4] rbd: kill rbd_spec->image_id_len

[Alex Elder]

There is no real benefit to keeping the length of an image id, so
get rid of it.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index a002984..e01dbb1 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -145,7 +145,6 @@ struct rbd_spec {
 	char		*pool_name;

 	char		*image_id;
-	size_t		image_id_len;
 	char		*image_name;

 	u64		snap_id;
@@ -2492,7 +2491,6 @@ static int rbd_dev_v2_parent_info(struct
rbd_device *rbd_dev)
 	void *end;
 	char *image_id;
 	u64 overlap;
-	size_t len = 0;
 	int ret;

 	parent_spec = rbd_spec_alloc();
@@ -2526,13 +2524,12 @@ static int rbd_dev_v2_parent_info(struct
rbd_device *rbd_dev)
 	if (parent_spec->pool_id == CEPH_NOPOOL)
 		goto out;	/* No parent?  No problem. */

-	image_id = ceph_extract_encoded_string(&p, end, &len, GFP_KERNEL);
+	image_id = ceph_extract_encoded_string(&p, end, NULL, GFP_KERNEL);
 	if (IS_ERR(image_id)) {
 		ret = PTR_ERR(image_id);
 		goto out_err;
 	}
 	parent_spec->image_id = image_id;
-	parent_spec->image_id_len = len;
 	ceph_decode_64_safe(&p, end, parent_spec->snap_id, out_err);
 	ceph_decode_64_safe(&p, end, overlap, out_err);

@@ -3368,8 +3365,7 @@ static int rbd_dev_image_id(struct rbd_device
*rbd_dev)
 	p = response;
 	rbd_dev->spec->image_id = ceph_extract_encoded_string(&p,
 						p + RBD_IMAGE_ID_LEN_MAX,
-						&rbd_dev->spec->image_id_len,
-						GFP_NOIO);
+						NULL, GFP_NOIO);
 	if (IS_ERR(rbd_dev->spec->image_id)) {
 		ret = PTR_ERR(rbd_dev->spec->image_id);
 		rbd_dev->spec->image_id = NULL;
@@ -3393,7 +3389,6 @@ static int rbd_dev_v1_probe(struct rbd_device
*rbd_dev)
 	rbd_dev->spec->image_id = kstrdup("", GFP_KERNEL);
 	if (!rbd_dev->spec->image_id)
 		return -ENOMEM;
-	rbd_dev->spec->image_id_len = 0;

 	/* Record the header object name for this rbd image. */

@@ -3443,7 +3438,7 @@ static int rbd_dev_v2_probe(struct rbd_device
*rbd_dev)
 	 * Image id was filled in by the caller.  Record the header
 	 * object name for this rbd image.
 	 */
-	size = sizeof (RBD_HEADER_PREFIX) + rbd_dev->spec->image_id_len;
+	size = sizeof (RBD_HEADER_PREFIX) + strlen(rbd_dev->spec->image_id);
 	rbd_dev->header_name = kmalloc(size, GFP_KERNEL);
 	if (!rbd_dev->header_name)
 		return -ENOMEM;
-- 
1.7.9.5

[PATCH REPOST 4/4] rbd: use kmemdup()

[Alex Elder]

This replaces two kmalloc()/memcpy() combinations with a single
call to kmemdup().

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index e01dbb1..d97611e 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3151,11 +3151,9 @@ static inline char *dup_token(const char **buf,
size_t *lenp)
 	size_t len;

 	len = next_token(buf);
-	dup = kmalloc(len + 1, GFP_KERNEL);
+	dup = kmemdup(*buf, len + 1, GFP_KERNEL);
 	if (!dup)
 		return NULL;
-
-	memcpy(dup, *buf, len);
 	*(dup + len) = '\0';
 	*buf += len;

@@ -3264,10 +3262,9 @@ static int rbd_add_parse_args(const char *buf,
 		ret = -ENAMETOOLONG;
 		goto out_err;
 	}
-	spec->snap_name = kmalloc(len + 1, GFP_KERNEL);
+	spec->snap_name = kmemdup(buf, len + 1, GFP_KERNEL);
 	if (!spec->snap_name)
 		goto out_mem;
-	memcpy(spec->snap_name, buf, len);
 	*(spec->snap_name + len) = '\0';

 	/* Initialize all rbd options to the defaults */
-- 
1.7.9.5

Re: [PATCH REPOST 0/4] rbd: four minor patches

[David Zafman]

I reviewed these.

Reviewed-by: David Zafman <david.zafman@inktank.com>

David Zafman
Senior Developer
david.zafman@inktank.com



On Jan 3, 2013, at 11:04 AM, Alex Elder <elder@inktank.com> wrote:

> I'm re-posting my patch backlog, in chunks that may or may not
> match how they got posted before.  This series contains some
> pretty fairly straightforward changes.
> 
> 					-Alex
> 
> [PATCH REPOST 1/4] rbd: document rbd_spec structure
> [PATCH REPOST 2/4] rbd: kill rbd_spec->image_name_len
> [PATCH REPOST 3/4] rbd: kill rbd_spec->image_id_len
> [PATCH REPOST 4/4] rbd: use kmemdup()
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH REPOST 0/4] rbd: four minor patches

[Josh Durgin]

On 01/03/2013 11:04 AM, Alex Elder wrote:
> I'm re-posting my patch backlog, in chunks that may or may not
> match how they got posted before.  This series contains some
> pretty fairly straightforward changes.
>
> 					-Alex
>
> [PATCH REPOST 1/4] rbd: document rbd_spec structure
> [PATCH REPOST 2/4] rbd: kill rbd_spec->image_name_len
> [PATCH REPOST 3/4] rbd: kill rbd_spec->image_id_len
> [PATCH REPOST 4/4] rbd: use kmemdup()

These all look good.

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