rbd: little cleanups

These are a handful of fairly minor cleanup items I've been putting off sending out until some of the meatier stuff got done.

-Alex

[PATCH 1/5] rbd: document rbd_spec structure [PATCH 2/5] rbd: kill rbd_spec->image_name_len [PATCH 3/5] rbd: kill rbd_spec->image_id_len [PATCH 4/5] rbd: don't use ENOTSUPP
[PATCH 5/5] rbd: use kmemdup()

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.

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

index 842caf4..32ed6e6 100644
@@ -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

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.

---
 drivers/block/rbd.c |   19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

index 32ed6e6..2e1d181 100644
@@ -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;

@@ -2565,15 +2564,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);
@@ -2633,14 +2632,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. */

@@ -3254,7 +3251,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)
@@ -3344,7 +3341,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;
@@ -3402,7 +3399,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

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

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

index 2e1d181..6512a8e 100644
@@ -145,7 +145,6 @@ struct rbd_spec {
 	char		*pool_name;
 
 	char		*image_id;
-	size_t		image_id_len;
 	char		*image_name;
 
 	u64		snap_id;

@@ -2494,7 +2493,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();
@@ -2528,13 +2526,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);

@@ -3370,8 +3367,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;
@@ -3395,7 +3391,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. */

@@ -3445,7 +3440,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

ENOTSUPP is not a standard errno (it shows up as "Unknown error 524" in an error message). This is what was getting produced when the the local rbd code does not implement features required by a discovered rbd image.

Change the error code returned in this case to ENXIO.

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

index 6512a8e..3378963 100644
@@ -2465,7 +2465,7 @@ static int _rbd_dev_v2_snap_features(struct

rbd_device *rbd_dev, u64 snap_id,

incompat = le64_to_cpu(features_buf.incompat); if (incompat & ~RBD_FEATURES_ALL)
- return -ENOTSUPP;
+ return -ENXIO;

*snap_features = le64_to_cpu(features_buf.features);

1.7.9.5

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

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

index 3378963..cf7b405 100644
@@ -3153,11 +3153,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;

@@ -3266,10 +3264,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