[PATCH 00/16] rbd: updates and enhancements

[PATCH 00/16] rbd: updates and enhancements

[Alex Elder]

The following series of patches constitute a few minor
fixes and cleanups, as well as some new utility routines
in the rbd code.  Some of these comprise a foundation
that's being used to add support for the "new" rbd image
format.

We've agreed to refer to the "old" and "new" formats as
format 1 and 2, respectively.

					-Alex

[PATCH 01/16] libceph: fix off-by-one bug in ceph_encode_filepath()
    This is a minor bug fix.

[PATCH 02/16] rbd: drop a useless local variable
    This is a minor cleanup.

[PATCH 03/16] libceph: define ceph_decode_string()
[PATCH 04/16] libceph: define ceph_extract_encoded_string()
[PATCH 05/16] rbd: define dup_token()
    Each of these defines a new utility routine.  They are
    all going to be used in some upcoming rbd patches, but
    they're ready for review now.

[PATCH 06/16] rbd: rename rbd_dev->block_name
[PATCH 07/16] rbd: dynamically allocate object prefix
    These update the "block_name" field of a rbd_dev structure
    to be named "object_prefix", which is consistent with the
    way the new rbd format describes it.  It also removes the
    unnecessary fixed limit on the length of that string.

[PATCH 08/16] rbd: don't store pool name in struct rbd_dev
    Since a pool name can change, this patch makes sure the
    rbd client code doesn't save a copy of it--relying instead
    on the invariant pool id.  The "pool" entry has been
    replaced by "pool_id" in /sys/bus/rbd/<N>.

[PATCH 09/16] rbd: dynamically allocate image header name
[PATCH 10/16] rbd: dynamically allocate image name
[PATCH 11/16] rbd: dynamically allocate snapshot name
    These three change some more fields for in-core data
    structures so they aren't arbitrarily limited to a fixed
    size.

[PATCH 12/16] rbd: use rbd_dev consistently
    This makes all variables representing a struct rbd_device
    be named "rbd_dev".

[PATCH 13/16] rbd: rename some fields in struct rbd_dev
[PATCH 14/16] rbd: more symbol renames
[PATCH 15/16] rbd: option symbol renames
    These change the names of some fields and symbols so
    they are (hopefully) a bit more descriptive.

[PATCH 16/16] rbd: kill num_reply parameters
    This eliminates an unused parameter that was defined in
    a number of functions.

[PATCH 01/16] libceph: fix off-by-one bug in ceph_encode_filepath()

[Alex Elder]

There is a BUG_ON() call that doesn't account for the single byte
structure version at the start of an encoded filepath in
ceph_encode_filepath().  Fix that.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 include/linux/ceph/decode.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
index d8615de..bcbd66c 100644
--- a/include/linux/ceph/decode.h
+++ b/include/linux/ceph/decode.h
@@ -151,7 +151,7 @@ static inline void ceph_encode_filepath(void **p,
void *end,
 					u64 ino, const char *path)
 {
 	u32 len = path ? strlen(path) : 0;
-	BUG_ON(*p + sizeof(ino) + sizeof(len) + len > end);
+	BUG_ON(*p + 1 + sizeof(ino) + sizeof(len) + len > end);
 	ceph_encode_8(p, 1);
 	ceph_encode_64(p, ino);
 	ceph_encode_32(p, len);
-- 
1.7.5.4

[PATCH 02/16] rbd: drop a useless local variable

[Alex Elder]

In rbd_req_sync_notify_ack(), a local variable was needlessly being
used to hold a null pointer.  Just pass NULL instead.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 8f428a8..2ae3bb0 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1187,7 +1187,6 @@ static int rbd_req_sync_notify_ack(struct
rbd_device *dev,
 				   const char *obj)
 {
 	struct ceph_osd_req_op *ops;
-	struct page **pages = NULL;
 	int ret;

 	ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_NOTIFY_ACK, 0);
@@ -1200,7 +1199,7 @@ static int rbd_req_sync_notify_ack(struct
rbd_device *dev,

 	ret = rbd_do_request(NULL, dev, NULL, CEPH_NOSNAP,
 			  obj, 0, 0, NULL,
-			  pages, 0,
+			  NULL, 0,
 			  CEPH_OSD_FLAG_READ,
 			  ops,
 			  1,
-- 
1.7.5.4

[PATCH 03/16] libceph: define ceph_decode_string()

[Alex Elder]

There is no string decoding function defined in <decode.h>, so this
defines one.

This function is a little different from the others in that the
length of the encoded string is not known a priori.  So the
interface is defined a bit like snprintf(), where the value returned
indicates the space required--even if it's more than the space
allotted.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 include/linux/ceph/decode.h |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
index bcbd66c..7ead11fc 100644
--- a/include/linux/ceph/decode.h
+++ b/include/linux/ceph/decode.h
@@ -44,6 +44,42 @@ static inline void ceph_decode_copy(void **p, void
*pv, size_t n)
 }

 /*
+ * Decode the wire-encoded string at *p into the buffer "s"
+ * provided, whose size is indicated by "size".  Note that "s" can
+ * be a null pointer if size is 0.  If it fits, the resulting string
+ * will always be terminated with '\0'; otherwise the buffer will
+ * be unchanged.
+ *
+ * Returns the length of the encoded string (which may be greater
+ * than or equal to the buffer size).  The return value does not
+ * include the terminating '\0'.
+ *
+ * If the the return value is less than the size provided, *p will
+ * be advanced past the decoded data; otherwise it is unchanged.
+ * This allows for a two call sequence to be used to allocate
+ * sufficient space for the string.
+ *
+ * NB  It is assumed that *p refers to a block of valid memory
+ *     sufficient to hold the length field followed by the number
+ *     of bytes indicated by that field.
+ */
+static inline size_t ceph_decode_string(void **p, char *s, size_t size)
+{
+	size_t len;
+
+	len = get_unaligned_le32(*p);
+	if (size < len + 1)
+		return len;
+
+	if (len)
+		memcpy(s, (char *) *p + sizeof (u32), len);
+	*(s + len) = '\0';
+	*p += sizeof (u32) + len;
+
+	return len;
+}
+
+/*
  * bounds check input.
  */
 static inline int ceph_has_room(void **p, void *end, size_t n)
-- 
1.7.5.4

[PATCH 04/16] libceph: define ceph_extract_encoded_string()

[Alex Elder]

This adds a new utility routine which will return a dynamically-
allocated buffer containing a string that has been decoded from ceph
over-the-wire format.  It also returns the length of the string
if the address of a size variable is supplied to receive it.

For now, no gfp_flags parameter is defined (GFP_KERNEL is used) but
it could be easily be added if needed.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 include/linux/ceph/decode.h |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
index 7ead11fc..7759164 100644
--- a/include/linux/ceph/decode.h
+++ b/include/linux/ceph/decode.h
@@ -80,6 +80,35 @@ static inline size_t ceph_decode_string(void **p,
char *s, size_t size)
 }

 /*
+ * Allocate a buffer big enough to hold the wire-encoded string, and
+ * decode the string into it.  The resulting string will always be
+ * terminated with '\0'.  If successful, *p will be advanced
+ * past the decoded data.  Also, if lenp is not a null pointer, the
+ * length (not including the terminating '\0') will be recorded in
+ * it.  Note that a zero-length string is a valid return value.
+ *
+ * Returns a pointer to the newly-allocated string buffer, or a
+ * null pointer if memory could not be allocated for the result.
+ * Neither of the arguments is updated if NULL is returned.
+ */
+static inline char *ceph_extract_encoded_string(void **p, size_t *lenp)
+{
+	size_t len;
+	char *buf;
+
+	len = ceph_decode_string(p, NULL, 0);
+	buf = kmalloc(len + 1, GFP_KERNEL);
+	if (!buf)
+		return NULL;
+
+	(void) ceph_decode_string(p, buf, len + 1);
+	if (lenp)
+		*lenp = len;
+
+	return buf;
+}
+
+/*
  * bounds check input.
  */
 static inline int ceph_has_room(void **p, void *end, size_t n)
-- 
1.7.5.4

[PATCH 05/16] rbd: define dup_token()

[Alex Elder]

Define a new function dup_token(), to be used during argument
parsing for making dynamically-allocated copies of tokens being
parsed.

For now, no gfp_flags parameter is defined (GFP_KERNEL is used) but
it could be easily be added if needed.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 2ae3bb0..63c132f 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2281,6 +2281,42 @@ static inline size_t copy_token(const char **buf,
 }

 /*
+ * Finds the next token in *buf, dynamically allocates a buffer big
+ * enough to hold a copy of it, and copies the token into the new
+ * buffer.  The copy is guaranteed to be terminated with '\0'.  Note
+ * that a duplicate buffer is created even for a zero-length token.
+ *
+ * Returns a pointer to the newly-allocated duplicate, or a null
+ * pointer if memory for the duplicate was not available.  If
+ * the lenp argument is a non-null pointer, the length of the token
+ * (not including the '\0') is returned in *lenp.
+ *
+ * If successful, the *buf pointer will be updated to point beyond
+ * the end of the found token.
+ *
+ * Note:  For now, the memory is allocated using GFP_KERNEL.
+ */
+static inline char *dup_token(const char **buf, size_t *lenp)
+{
+	char *dup;
+	size_t len;
+
+	len = next_token(buf);
+	dup = kmalloc(len + 1, GFP_KERNEL);
+	if (!dup)
+		return NULL;
+
+	memcpy(dup, *buf, len);
+	*(dup + len) = '\0';
+	*buf += len;
+
+	if (lenp)
+		*lenp = len;
+
+	return dup;
+}
+
+/*
  * This fills in the pool_name, obj, obj_len, snap_name, obj_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
-- 
1.7.5.4

[PATCH 06/16] rbd: rename rbd_dev->block_name

[Alex Elder]

Each rbd image has a name that forms the basis of all data objects
backing the device.  Old (format 1) images refer to this name as the
"block name," while new (format 2) images use the term "object
prefix" for this.

Change the field name in the in-core rbd image header structure to
reflect the more modern usage.  We intentionally keep the the name
"block_name" in the on-disk definition for format 1 image headers.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 63c132f..57d264c 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -78,7 +78,7 @@
  */
 struct rbd_image_header {
 	u64 image_size;
-	char block_name[32];
+	char object_prefix[32];
 	__u8 obj_order;
 	__u8 crypt_type;
 	__u8 comp_type;
@@ -518,7 +518,7 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 		header->snap_names = NULL;
 		header->snap_sizes = NULL;
 	}
-	memcpy(header->block_name, ondisk->block_name,
+	memcpy(header->object_prefix, ondisk->block_name,
 	       sizeof(ondisk->block_name));

 	header->image_size = le64_to_cpu(ondisk->image_size);
@@ -620,7 +620,7 @@ static void rbd_header_free(struct rbd_image_header
*header)
  * get the actual striped segment name, offset and length
  */
 static u64 rbd_get_segment(struct rbd_image_header *header,
-			   const char *block_name,
+			   const char *object_prefix,
 			   u64 ofs, u64 len,
 			   char *seg_name, u64 *segofs)
 {
@@ -628,7 +628,7 @@ static u64 rbd_get_segment(struct rbd_image_header
*header,

 	if (seg_name)
 		snprintf(seg_name, RBD_MAX_SEG_NAME_LEN,
-			 "%s.%012llx", block_name, seg);
+			 "%s.%012llx", object_prefix, seg);

 	ofs = ofs & ((1 << header->obj_order) - 1);
 	len = min_t(u64, len, (1 << header->obj_order) - ofs);
@@ -1091,7 +1091,7 @@ static int rbd_do_op(struct request *rq,
 		return -ENOMEM;

 	seg_len = rbd_get_segment(&rbd_dev->header,
-				  rbd_dev->header.block_name,
+				  rbd_dev->header.object_prefix,
 				  ofs, len,
 				  seg_name, &seg_ofs);

@@ -1482,7 +1482,7 @@ static void rbd_rq_fn(struct request_queue *q)
 			/* a bio clone to be passed down to OSD req */
 			dout("rq->bio->bi_vcnt=%d\n", rq->bio->bi_vcnt);
 			op_size = rbd_get_segment(&rbd_dev->header,
-						  rbd_dev->header.block_name,
+						  rbd_dev->header.object_prefix,
 						  ofs, size,
 						  NULL, NULL);
 			kref_get(&coll->kref);
-- 
1.7.5.4

[PATCH 07/16] rbd: dynamically allocate object prefix

[Alex Elder]

There is no need to impose a small limit the length of the object
prefix recorded for an rbd image in a struct rbd_image_header.
Remove the limitation by allocating space for the object prefix
dynamically.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 57d264c..3aa0ca0 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -78,7 +78,7 @@
  */
 struct rbd_image_header {
 	u64 image_size;
-	char object_prefix[32];
+	char *object_prefix;
 	__u8 obj_order;
 	__u8 crypt_type;
 	__u8 comp_type;
@@ -518,8 +518,15 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 		header->snap_names = NULL;
 		header->snap_sizes = NULL;
 	}
+
+	header->object_prefix = kmalloc(sizeof (ondisk->block_name) + 1,
+					gfp_flags);
+	if (!header->object_prefix)
+		goto err_sizes;
+
 	memcpy(header->object_prefix, ondisk->block_name,
 	       sizeof(ondisk->block_name));
+	header->object_prefix[sizeof (ondisk->block_name)] = '\0';

 	header->image_size = le64_to_cpu(ondisk->image_size);
 	header->obj_order = ondisk->options.order;
@@ -546,6 +553,8 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,

 	return 0;

+err_sizes:
+	kfree(header->snap_sizes);
 err_names:
 	kfree(header->snap_names);
 err_snapc:
@@ -611,9 +620,10 @@ done:

 static void rbd_header_free(struct rbd_image_header *header)
 {
-	kfree(header->snapc);
-	kfree(header->snap_names);
+	kfree(header->object_prefix);
 	kfree(header->snap_sizes);
+	kfree(header->snap_names);
+	kfree(header->snapc);
 }

 /*
@@ -1711,15 +1721,17 @@ static int __rbd_refresh_header(struct
rbd_device *rbd_dev)
 		   if head moves */
 		follow_seq = 1;

-	kfree(rbd_dev->header.snapc);
-	kfree(rbd_dev->header.snap_names);
+	kfree(rbd_dev->header.object_prefix);
 	kfree(rbd_dev->header.snap_sizes);
+	kfree(rbd_dev->header.snap_names);
+	kfree(rbd_dev->header.snapc);

 	rbd_dev->header.total_snaps = h.total_snaps;
 	rbd_dev->header.snapc = h.snapc;
 	rbd_dev->header.snap_names = h.snap_names;
 	rbd_dev->header.snap_names_len = h.snap_names_len;
 	rbd_dev->header.snap_sizes = h.snap_sizes;
+	rbd_dev->header.object_prefix = h.object_prefix;
 	if (follow_seq)
 		rbd_dev->header.snapc->seq = rbd_dev->header.snapc->snaps[0];
 	else
-- 
1.7.5.4

[PATCH 08/16] rbd: don't store pool name in struct rbd_dev

[Alex Elder]

An rbd device's pool name is used to specify the pool to use for an
rbd image when it gets mapped (via rbd_add()).  However, only the
pool id can be relied on to be invariant--the name of a pool can
change at any time.

This means that the name of the pool is potentially stale as soon as
the image is mapped, so it's a bad idea to record this information.
So only store the pool id, not its name, in an rbd_dev.

Here are a few specific notes about this change:
    - The "pool" name device attribute (/sys/bus/rbd/devices/<N>/pool)
      goes away.  In its place is a "pool_id" attribute that provide
      the numeric pool id for the rbd image.
    - rbd_add_parse_args() now returns a pointer to a dynamically-
      allocated copy of the pool name taken from the arguments.
    - rbd_add() has been changed to handle freeing the pool name,
      both in error cases and in the normal case after the pool id
      has been recorded.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 3aa0ca0..76e978c 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -56,7 +56,6 @@
 #define RBD_MINORS_PER_MAJOR	256		/* max minors per blkdev */

 #define RBD_MAX_MD_NAME_LEN	(RBD_MAX_OBJ_NAME_LEN + sizeof(RBD_SUFFIX))
-#define RBD_MAX_POOL_NAME_LEN	64
 #define RBD_MAX_SNAP_NAME_LEN	32
 #define RBD_MAX_OPT_LEN		1024

@@ -166,8 +165,7 @@ struct rbd_device {
 	char			obj[RBD_MAX_OBJ_NAME_LEN]; /* rbd image name */
 	int			obj_len;
 	char			obj_md_name[RBD_MAX_MD_NAME_LEN]; /* hdr nm. */
-	char			pool_name[RBD_MAX_POOL_NAME_LEN];
-	int			poolid;
+	int			pool_id;

 	struct ceph_osd_event   *watch_event;
 	struct ceph_osd_request *watch_request;
@@ -930,7 +928,7 @@ static int rbd_do_request(struct request *rq,
 	layout->fl_stripe_unit = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
 	layout->fl_stripe_count = cpu_to_le32(1);
 	layout->fl_object_size = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
-	layout->fl_pg_pool = cpu_to_le32(dev->poolid);
+	layout->fl_pg_pool = cpu_to_le32(dev->pool_id);
 	ceph_calc_raw_layout(osdc, layout, snapid, ofs, &len, &bno,
 				req, ops);

@@ -1653,7 +1651,7 @@ static int rbd_header_add_snap(struct rbd_device *dev,
 		return -EINVAL;

 	monc = &dev->rbd_client->client->monc;
-	ret = ceph_monc_create_snapid(monc, dev->poolid, &new_snapid);
+	ret = ceph_monc_create_snapid(monc, dev->pool_id, &new_snapid);
 	dout("created snapid=%lld\n", new_snapid);
 	if (ret < 0)
 		return ret;
@@ -1851,12 +1849,12 @@ static ssize_t rbd_client_id_show(struct device
*dev,
 			ceph_client_id(rbd_dev->rbd_client->client));
 }

-static ssize_t rbd_pool_show(struct device *dev,
+static ssize_t rbd_pool_id_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

-	return sprintf(buf, "%s\n", rbd_dev->pool_name);
+	return sprintf(buf, "%d\n", rbd_dev->pool_id);
 }

 static ssize_t rbd_name_show(struct device *dev,
@@ -1898,7 +1896,7 @@ static ssize_t rbd_image_refresh(struct device *dev,
 static DEVICE_ATTR(size, S_IRUGO, rbd_size_show, NULL);
 static DEVICE_ATTR(major, S_IRUGO, rbd_major_show, NULL);
 static DEVICE_ATTR(client_id, S_IRUGO, rbd_client_id_show, NULL);
-static DEVICE_ATTR(pool, S_IRUGO, rbd_pool_show, NULL);
+static DEVICE_ATTR(pool_id, S_IRUGO, rbd_pool_id_show, NULL);
 static DEVICE_ATTR(name, S_IRUGO, rbd_name_show, NULL);
 static DEVICE_ATTR(refresh, S_IWUSR, NULL, rbd_image_refresh);
 static DEVICE_ATTR(current_snap, S_IRUGO, rbd_snap_show, NULL);
@@ -1908,7 +1906,7 @@ static struct attribute *rbd_attrs[] = {
 	&dev_attr_size.attr,
 	&dev_attr_major.attr,
 	&dev_attr_client_id.attr,
-	&dev_attr_pool.attr,
+	&dev_attr_pool_id.attr,
 	&dev_attr_name.attr,
 	&dev_attr_current_snap.attr,
 	&dev_attr_refresh.attr,
@@ -2329,25 +2327,31 @@ static inline char *dup_token(const char **buf,
size_t *lenp)
 }

 /*
- * This fills in the pool_name, obj, obj_len, snap_name, obj_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.
+ * This fills in the obj, obj_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.
+ *
+ * Returns a pointer to a dynamically-allocated buffer containing
+ * the pool name provided, or a pointer-coded errno if an error
+ * occurs.
  */
-static int rbd_add_parse_args(struct rbd_device *rbd_dev,
+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;
+	size_t len;
+	int ret = -EINVAL;
+	char *pool_name = NULL;

 	/* The first four tokens are required */

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

@@ -2355,15 +2359,17 @@ 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;
+		goto out_err;

-	len = copy_token(&buf, rbd_dev->pool_name, sizeof (rbd_dev->pool_name));
-	if (!len || len >= sizeof (rbd_dev->pool_name))
-		return -EINVAL;
+	pool_name = dup_token(&buf, NULL);
+	if (!pool_name) {
+		ret = -ENOMEM;
+		goto out_err;
+	}

 	len = copy_token(&buf, rbd_dev->obj, sizeof (rbd_dev->obj));
 	if (!len || len >= sizeof (rbd_dev->obj))
-		return -EINVAL;
+		goto out_err;

 	/* We have the object length in hand, save it. */

@@ -2382,9 +2388,14 @@ static int rbd_add_parse_args(struct rbd_device
*rbd_dev,
 		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
 			sizeof (RBD_SNAP_HEAD_NAME));
 	else if (len >= sizeof (rbd_dev->snap_name))
-		return -EINVAL;
+		goto out_err;

-	return 0;
+	return pool_name;
+
+out_err:
+	kfree(pool_name);
+
+	return ERR_PTR(ret);
 }

 static ssize_t rbd_add(struct bus_type *bus,
@@ -2396,6 +2407,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	size_t mon_addrs_size = 0;
 	char *options = NULL;
 	struct ceph_osd_client *osdc;
+	char *pool_name;
 	int rc = -ENOMEM;

 	if (!try_module_get(THIS_MODULE))
@@ -2425,10 +2437,14 @@ static ssize_t rbd_add(struct bus_type *bus,
 	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->id);

 	/* parse add command */
-	rc = rbd_add_parse_args(rbd_dev, buf, &mon_addrs, &mon_addrs_size,
-				options, count);
-	if (rc)
+	pool_name = rbd_add_parse_args(rbd_dev, buf,
+					&mon_addrs, &mon_addrs_size,
+					options, count);
+	if (IS_ERR(pool_name)) {
+		rc = PTR_ERR(pool_name);
+		pool_name = NULL;
 		goto err_put_id;
+	}

 	rbd_dev->rbd_client = rbd_get_client(mon_addrs, mon_addrs_size - 1,
 						options);
@@ -2439,10 +2455,10 @@ static ssize_t rbd_add(struct bus_type *bus,

 	/* pick the pool */
 	osdc = &rbd_dev->rbd_client->client->osdc;
-	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name);
+	rc = ceph_pg_poolid_by_name(osdc->osdmap, pool_name);
 	if (rc < 0)
 		goto err_out_client;
-	rbd_dev->poolid = rc;
+	rbd_dev->pool_id = rc;

 	/* register our block device */
 	rc = register_blkdev(0, rbd_dev->name);
@@ -2453,6 +2469,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	rc = rbd_bus_add_dev(rbd_dev);
 	if (rc)
 		goto err_out_blkdev;
+	kfree(pool_name);

 	/*
 	 * At this point cleanup in the event of an error is the job
@@ -2482,6 +2499,7 @@ err_out_blkdev:
 err_out_client:
 	rbd_put_client(rbd_dev);
 err_put_id:
+	kfree(pool_name);
 	rbd_id_put(rbd_dev);
 err_nomem:
 	kfree(options);
-- 
1.7.5.4

[PATCH 09/16] rbd: dynamically allocate image header name

[Alex Elder]

There is no need to impose a small limit the length of the header
name recorded for an rbd image in a struct rbd_dev.  Remove the
limitation by allocating space for the header name dynamically.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 76e978c..4d11337 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -55,7 +55,6 @@

 #define RBD_MINORS_PER_MAJOR	256		/* max minors per blkdev */

-#define RBD_MAX_MD_NAME_LEN	(RBD_MAX_OBJ_NAME_LEN + sizeof(RBD_SUFFIX))
 #define RBD_MAX_SNAP_NAME_LEN	32
 #define RBD_MAX_OPT_LEN		1024

@@ -164,7 +163,7 @@ struct rbd_device {
 	struct rbd_image_header	header;
 	char			obj[RBD_MAX_OBJ_NAME_LEN]; /* rbd image name */
 	int			obj_len;
-	char			obj_md_name[RBD_MAX_MD_NAME_LEN]; /* hdr nm. */
+	char			*obj_md_name; /* hdr nm. */
 	int			pool_id;

 	struct ceph_osd_event   *watch_event;
@@ -2375,8 +2374,13 @@ static char *rbd_add_parse_args(struct rbd_device
*rbd_dev,

 	rbd_dev->obj_len = len;

-	BUILD_BUG_ON(RBD_MAX_MD_NAME_LEN
-				< RBD_MAX_OBJ_NAME_LEN + sizeof (RBD_SUFFIX));
+	/* Create the name of the header object */
+
+	rbd_dev->obj_md_name = kmalloc(len + sizeof (RBD_SUFFIX), GFP_KERNEL);
+	if (!rbd_dev->obj_md_name) {
+		ret = -ENOMEM;
+		goto out_err;
+	}
 	sprintf(rbd_dev->obj_md_name, "%s%s", rbd_dev->obj, RBD_SUFFIX);

 	/*
@@ -2393,6 +2397,7 @@ static char *rbd_add_parse_args(struct rbd_device
*rbd_dev,
 	return pool_name;

 out_err:
+	kfree(rbd_dev->obj_md_name);
 	kfree(pool_name);

 	return ERR_PTR(ret);
@@ -2402,23 +2407,23 @@ static ssize_t rbd_add(struct bus_type *bus,
 		       const char *buf,
 		       size_t count)
 {
-	struct rbd_device *rbd_dev;
+	char *options;
+	struct rbd_device *rbd_dev = NULL;
 	const char *mon_addrs = NULL;
 	size_t mon_addrs_size = 0;
-	char *options = NULL;
-	struct ceph_osd_client *osdc;
 	char *pool_name;
+	struct ceph_osd_client *osdc;
 	int rc = -ENOMEM;

 	if (!try_module_get(THIS_MODULE))
 		return -ENODEV;

-	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
-	if (!rbd_dev)
-		goto err_nomem;
 	options = kmalloc(count, GFP_KERNEL);
 	if (!options)
 		goto err_nomem;
+	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
+	if (!rbd_dev)
+		goto err_nomem;

 	/* static rbd_device initialization */
 	spin_lock_init(&rbd_dev->lock);
@@ -2499,11 +2504,14 @@ err_out_blkdev:
 err_out_client:
 	rbd_put_client(rbd_dev);
 err_put_id:
-	kfree(pool_name);
+	if (pool_name) {
+		kfree(rbd_dev->obj_md_name);
+		kfree(pool_name);
+	}
 	rbd_id_put(rbd_dev);
 err_nomem:
-	kfree(options);
 	kfree(rbd_dev);
+	kfree(options);

 	dout("Error adding device %s\n", buf);
 	module_put(THIS_MODULE);
@@ -2548,6 +2556,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->obj_md_name);
 	rbd_id_put(rbd_dev);
 	kfree(rbd_dev);

-- 
1.7.5.4

[PATCH 10/16] rbd: dynamically allocate image name

[Alex Elder]

There is no need to impose a small limit the length of the rbd image
name recorded in a struct rbd_dev.  Remove the limitation by
allocating space for the image name dynamically.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c       |   34 ++++++++++++++++++----------------
 drivers/block/rbd_types.h |    1 -
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 4d11337..28afff9 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -161,8 +161,8 @@ struct rbd_device {
 	spinlock_t		lock;		/* queue lock */

 	struct rbd_image_header	header;
-	char			obj[RBD_MAX_OBJ_NAME_LEN]; /* rbd image name */
-	int			obj_len;
+	char			*obj; /* rbd image name */
+	size_t			obj_len;
 	char			*obj_md_name; /* hdr nm. */
 	int			pool_id;

@@ -2333,6 +2333,8 @@ static inline char *dup_token(const char **buf,
size_t *lenp)
  * Returns a pointer to a dynamically-allocated buffer containing
  * the pool name provided, or a pointer-coded errno if an error
  * occurs.
+ *
+ * Note: rbd_dev is assumed to have been initially zero-filled.
  */
 static char *rbd_add_parse_args(struct rbd_device *rbd_dev,
 			      const char *buf,
@@ -2360,27 +2362,22 @@ static char *rbd_add_parse_args(struct
rbd_device *rbd_dev,
 	if (!len || len >= options_size)
 		goto out_err;

+	ret = -ENOMEM;
 	pool_name = dup_token(&buf, NULL);
-	if (!pool_name) {
-		ret = -ENOMEM;
+	if (!pool_name)
 		goto out_err;
-	}

-	len = copy_token(&buf, rbd_dev->obj, sizeof (rbd_dev->obj));
-	if (!len || len >= sizeof (rbd_dev->obj))
+	rbd_dev->obj = dup_token(&buf, &rbd_dev->obj_len);
+	if (!rbd_dev->obj)
 		goto out_err;

-	/* We have the object length in hand, save it. */
-
-	rbd_dev->obj_len = len;
-
 	/* Create the name of the header object */

-	rbd_dev->obj_md_name = kmalloc(len + sizeof (RBD_SUFFIX), GFP_KERNEL);
-	if (!rbd_dev->obj_md_name) {
-		ret = -ENOMEM;
+	rbd_dev->obj_md_name = kmalloc(rbd_dev->obj_len
+						+ sizeof (RBD_SUFFIX),
+					GFP_KERNEL);
+	if (!rbd_dev->obj_md_name)
 		goto out_err;
-	}
 	sprintf(rbd_dev->obj_md_name, "%s%s", rbd_dev->obj, RBD_SUFFIX);

 	/*
@@ -2391,13 +2388,16 @@ static char *rbd_add_parse_args(struct
rbd_device *rbd_dev,
 	if (!len)
 		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
 			sizeof (RBD_SNAP_HEAD_NAME));
-	else if (len >= sizeof (rbd_dev->snap_name))
+	else if (len >= sizeof (rbd_dev->snap_name)) {
+		ret = -EINVAL;
 		goto out_err;
+	}

 	return pool_name;

 out_err:
 	kfree(rbd_dev->obj_md_name);
+	kfree(rbd_dev->obj);
 	kfree(pool_name);

 	return ERR_PTR(ret);
@@ -2506,6 +2506,7 @@ err_out_client:
 err_put_id:
 	if (pool_name) {
 		kfree(rbd_dev->obj_md_name);
+		kfree(rbd_dev->obj);
 		kfree(pool_name);
 	}
 	rbd_id_put(rbd_dev);
@@ -2557,6 +2558,7 @@ static void rbd_dev_release(struct device *dev)

 	/* done with the id, and with the rbd_dev */
 	kfree(rbd_dev->obj_md_name);
+	kfree(rbd_dev->obj);
 	rbd_id_put(rbd_dev);
 	kfree(rbd_dev);

diff --git a/drivers/block/rbd_types.h b/drivers/block/rbd_types.h
index 9507086..0924e9e 100644
--- a/drivers/block/rbd_types.h
+++ b/drivers/block/rbd_types.h
@@ -31,7 +31,6 @@
 #define RBD_MIN_OBJ_ORDER       16
 #define RBD_MAX_OBJ_ORDER       30

-#define RBD_MAX_OBJ_NAME_LEN	96
 #define RBD_MAX_SEG_NAME_LEN	128

 #define RBD_COMP_NONE		0
-- 
1.7.5.4

[PATCH 11/16] rbd: dynamically allocate snapshot name

[Alex Elder]

There is no need to impose a small limit the length of the snapshot
name recorded for an rbd image in a struct rbd_dev.  Remove the
limitation by allocating space for the snapshot name dynamically.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 28afff9..90fb388 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -171,7 +171,7 @@ struct rbd_device {

 	/* protects updating the header */
 	struct rw_semaphore     header_rwsem;
-	char                    snap_name[RBD_MAX_SNAP_NAME_LEN];
+	char                    *snap_name;
 	u64                     snap_id;	/* current snapshot id */
 	int read_only;

@@ -587,8 +587,6 @@ static int rbd_header_set_snap(struct rbd_device
*dev, u64 *size)
 	struct ceph_snap_context *snapc = header->snapc;
 	int ret = -ENOENT;

-	BUILD_BUG_ON(sizeof (dev->snap_name) < sizeof (RBD_SNAP_HEAD_NAME));
-
 	down_write(&dev->header_rwsem);

 	if (!memcmp(dev->snap_name, RBD_SNAP_HEAD_NAME,
@@ -2381,16 +2379,22 @@ static char *rbd_add_parse_args(struct
rbd_device *rbd_dev,
 	sprintf(rbd_dev->obj_md_name, "%s%s", rbd_dev->obj, RBD_SUFFIX);

 	/*
-	 * The snapshot name is optional, but it's an error if it's
-	 * too long.  If no snapshot is supplied, fill in the default.
+	 * The snapshot name is optional.  If none is is supplied,
+	 * we use the default value.
 	 */
-	len = copy_token(&buf, rbd_dev->snap_name, sizeof (rbd_dev->snap_name));
-	if (!len)
+	rbd_dev->snap_name = dup_token(&buf, &len);
+	if (!rbd_dev->snap_name)
+		goto out_err;
+	if (!len) {
+		/* Replace the empty name with the default */
+		kfree(rbd_dev->snap_name);
+		rbd_dev->snap_name
+			= kmalloc(sizeof (RBD_SNAP_HEAD_NAME), GFP_KERNEL);
+		if (!rbd_dev->snap_name)
+			goto out_err;
+
 		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
 			sizeof (RBD_SNAP_HEAD_NAME));
-	else if (len >= sizeof (rbd_dev->snap_name)) {
-		ret = -EINVAL;
-		goto out_err;
 	}

 	return pool_name;
@@ -2505,6 +2509,7 @@ err_out_client:
 	rbd_put_client(rbd_dev);
 err_put_id:
 	if (pool_name) {
+		kfree(rbd_dev->snap_name);
 		kfree(rbd_dev->obj_md_name);
 		kfree(rbd_dev->obj);
 		kfree(pool_name);
@@ -2557,6 +2562,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->obj_md_name);
 	kfree(rbd_dev->obj);
 	rbd_id_put(rbd_dev);
-- 
1.7.5.4

[PATCH 12/16] rbd: use rbd_dev consistently

[Alex Elder]

Most variables that represent a struct rbd_device are named
"rbd_dev", but in some cases "dev" is used instead.  Change all the
"dev" references so they use "rbd_dev" consistently, to make it
clear from the name that we're working with an RBD device (as
opposed to, for example, a struct device).  Similarly, change the
name of the "dev" field in struct rbd_notify_info to be "rbd_dev".

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 90fb388..c38246b 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -581,35 +581,36 @@ 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 *dev, u64 *size)
+static int rbd_header_set_snap(struct rbd_device *rbd_dev, u64 *size)
 {
-	struct rbd_image_header *header = &dev->header;
+	struct rbd_image_header *header = &rbd_dev->header;
 	struct ceph_snap_context *snapc = header->snapc;
 	int ret = -ENOENT;

-	down_write(&dev->header_rwsem);
+	down_write(&rbd_dev->header_rwsem);

-	if (!memcmp(dev->snap_name, RBD_SNAP_HEAD_NAME,
+	if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
 		    sizeof (RBD_SNAP_HEAD_NAME))) {
 		if (header->total_snaps)
 			snapc->seq = header->snap_seq;
 		else
 			snapc->seq = 0;
-		dev->snap_id = CEPH_NOSNAP;
-		dev->read_only = 0;
+		rbd_dev->snap_id = CEPH_NOSNAP;
+		rbd_dev->read_only = 0;
 		if (size)
 			*size = header->image_size;
 	} else {
-		ret = snap_by_name(header, dev->snap_name, &snapc->seq, size);
+		ret = snap_by_name(header, rbd_dev->snap_name,
+					&snapc->seq, size);
 		if (ret < 0)
 			goto done;
-		dev->snap_id = snapc->seq;
-		dev->read_only = 1;
+		rbd_dev->snap_id = snapc->seq;
+		rbd_dev->read_only = 1;
 	}

 	ret = 0;
 done:
-	up_write(&dev->header_rwsem);
+	up_write(&rbd_dev->header_rwsem);
 	return ret;
 }

@@ -853,7 +854,7 @@ static void rbd_coll_end_req(struct rbd_request *req,
  * Send ceph osd request
  */
 static int rbd_do_request(struct request *rq,
-			  struct rbd_device *dev,
+			  struct rbd_device *rbd_dev,
 			  struct ceph_snap_context *snapc,
 			  u64 snapid,
 			  const char *obj, u64 ofs, u64 len,
@@ -894,13 +895,13 @@ static int rbd_do_request(struct request *rq,

 	dout("rbd_do_request obj=%s ofs=%lld len=%lld\n", obj, len, ofs);

-	down_read(&dev->header_rwsem);
+	down_read(&rbd_dev->header_rwsem);

-	osdc = &dev->rbd_client->client->osdc;
+	osdc = &rbd_dev->rbd_client->client->osdc;
 	req = ceph_osdc_alloc_request(osdc, flags, snapc, ops,
 					false, GFP_NOIO, pages, bio);
 	if (!req) {
-		up_read(&dev->header_rwsem);
+		up_read(&rbd_dev->header_rwsem);
 		ret = -ENOMEM;
 		goto done_pages;
 	}
@@ -925,7 +926,7 @@ static int rbd_do_request(struct request *rq,
 	layout->fl_stripe_unit = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
 	layout->fl_stripe_count = cpu_to_le32(1);
 	layout->fl_object_size = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
-	layout->fl_pg_pool = cpu_to_le32(dev->pool_id);
+	layout->fl_pg_pool = cpu_to_le32(rbd_dev->pool_id);
 	ceph_calc_raw_layout(osdc, layout, snapid, ofs, &len, &bno,
 				req, ops);

@@ -934,7 +935,7 @@ static int rbd_do_request(struct request *rq,
 				snapc,
 				&mtime,
 				req->r_oid, req->r_oid_len);
-	up_read(&dev->header_rwsem);
+	up_read(&rbd_dev->header_rwsem);

 	if (linger_req) {
 		ceph_osdc_set_request_linger(osdc, req);
@@ -1011,7 +1012,7 @@ static void rbd_simple_req_cb(struct
ceph_osd_request *req, struct ceph_msg *msg
 /*
  * Do a synchronous ceph osd operation
  */
-static int rbd_req_sync_op(struct rbd_device *dev,
+static int rbd_req_sync_op(struct rbd_device *rbd_dev,
 			   struct ceph_snap_context *snapc,
 			   u64 snapid,
 			   int opcode,
@@ -1048,7 +1049,7 @@ static int rbd_req_sync_op(struct rbd_device *dev,
 		}
 	}

-	ret = rbd_do_request(NULL, dev, snapc, snapid,
+	ret = rbd_do_request(NULL, rbd_dev, snapc, snapid,
 			  obj, ofs, len, NULL,
 			  pages, num_pages,
 			  flags,
@@ -1075,7 +1076,7 @@ done:
  * Do an asynchronous ceph osd operation
  */
 static int rbd_do_op(struct request *rq,
-		     struct rbd_device *rbd_dev ,
+		     struct rbd_device *rbd_dev,
 		     struct ceph_snap_context *snapc,
 		     u64 snapid,
 		     int opcode, int flags, int num_reply,
@@ -1167,7 +1168,7 @@ static int rbd_req_read(struct request *rq,
 /*
  * Request sync osd read
  */
-static int rbd_req_sync_read(struct rbd_device *dev,
+static int rbd_req_sync_read(struct rbd_device *rbd_dev,
 			  struct ceph_snap_context *snapc,
 			  u64 snapid,
 			  const char *obj,
@@ -1175,7 +1176,7 @@ static int rbd_req_sync_read(struct rbd_device *dev,
 			  char *buf,
 			  u64 *ver)
 {
-	return rbd_req_sync_op(dev, NULL,
+	return rbd_req_sync_op(rbd_dev, NULL,
 			       snapid,
 			       CEPH_OSD_OP_READ,
 			       CEPH_OSD_FLAG_READ,
@@ -1186,7 +1187,7 @@ static int rbd_req_sync_read(struct rbd_device *dev,
 /*
  * Request sync osd watch
  */
-static int rbd_req_sync_notify_ack(struct rbd_device *dev,
+static int rbd_req_sync_notify_ack(struct rbd_device *rbd_dev,
 				   u64 ver,
 				   u64 notify_id,
 				   const char *obj)
@@ -1198,11 +1199,11 @@ static int rbd_req_sync_notify_ack(struct
rbd_device *dev,
 	if (ret < 0)
 		return ret;

-	ops[0].watch.ver = cpu_to_le64(dev->header.obj_version);
+	ops[0].watch.ver = cpu_to_le64(rbd_dev->header.obj_version);
 	ops[0].watch.cookie = notify_id;
 	ops[0].watch.flag = 0;

-	ret = rbd_do_request(NULL, dev, NULL, CEPH_NOSNAP,
+	ret = rbd_do_request(NULL, rbd_dev, NULL, CEPH_NOSNAP,
 			  obj, 0, 0, NULL,
 			  NULL, 0,
 			  CEPH_OSD_FLAG_READ,
@@ -1217,54 +1218,54 @@ static int rbd_req_sync_notify_ack(struct
rbd_device *dev,

 static void rbd_watch_cb(u64 ver, u64 notify_id, u8 opcode, void *data)
 {
-	struct rbd_device *dev = (struct rbd_device *)data;
+	struct rbd_device *rbd_dev = (struct rbd_device *)data;
 	int rc;

-	if (!dev)
+	if (!rbd_dev)
 		return;

-	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n", dev->obj_md_name,
+	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n", rbd_dev->obj_md_name,
 		notify_id, (int)opcode);
 	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-	rc = __rbd_refresh_header(dev);
+	rc = __rbd_refresh_header(rbd_dev);
 	mutex_unlock(&ctl_mutex);
 	if (rc)
 		pr_warning(RBD_DRV_NAME "%d got notification but failed to "
-			   " update snaps: %d\n", dev->major, rc);
+			   " update snaps: %d\n", rbd_dev->major, rc);

-	rbd_req_sync_notify_ack(dev, ver, notify_id, dev->obj_md_name);
+	rbd_req_sync_notify_ack(rbd_dev, ver, notify_id, rbd_dev->obj_md_name);
 }

 /*
  * Request sync osd watch
  */
-static int rbd_req_sync_watch(struct rbd_device *dev,
+static int rbd_req_sync_watch(struct rbd_device *rbd_dev,
 			      const char *obj,
 			      u64 ver)
 {
 	struct ceph_osd_req_op *ops;
-	struct ceph_osd_client *osdc = &dev->rbd_client->client->osdc;
+	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;

 	int ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_WATCH, 0);
 	if (ret < 0)
 		return ret;

 	ret = ceph_osdc_create_event(osdc, rbd_watch_cb, 0,
-				     (void *)dev, &dev->watch_event);
+				     (void *)rbd_dev, &rbd_dev->watch_event);
 	if (ret < 0)
 		goto fail;

 	ops[0].watch.ver = cpu_to_le64(ver);
-	ops[0].watch.cookie = cpu_to_le64(dev->watch_event->cookie);
+	ops[0].watch.cookie = cpu_to_le64(rbd_dev->watch_event->cookie);
 	ops[0].watch.flag = 1;

-	ret = rbd_req_sync_op(dev, NULL,
+	ret = rbd_req_sync_op(rbd_dev, NULL,
 			      CEPH_NOSNAP,
 			      0,
 			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			      ops,
 			      1, obj, 0, 0, NULL,
-			      &dev->watch_request, NULL);
+			      &rbd_dev->watch_request, NULL);

 	if (ret < 0)
 		goto fail_event;
@@ -1273,8 +1274,8 @@ static int rbd_req_sync_watch(struct rbd_device *dev,
 	return 0;

 fail_event:
-	ceph_osdc_cancel_event(dev->watch_event);
-	dev->watch_event = NULL;
+	ceph_osdc_cancel_event(rbd_dev->watch_event);
+	rbd_dev->watch_event = NULL;
 fail:
 	rbd_destroy_ops(ops);
 	return ret;
@@ -1283,7 +1284,7 @@ fail:
 /*
  * Request sync osd unwatch
  */
-static int rbd_req_sync_unwatch(struct rbd_device *dev,
+static int rbd_req_sync_unwatch(struct rbd_device *rbd_dev,
 				const char *obj)
 {
 	struct ceph_osd_req_op *ops;
@@ -1293,10 +1294,10 @@ static int rbd_req_sync_unwatch(struct
rbd_device *dev,
 		return ret;

 	ops[0].watch.ver = 0;
-	ops[0].watch.cookie = cpu_to_le64(dev->watch_event->cookie);
+	ops[0].watch.cookie = cpu_to_le64(rbd_dev->watch_event->cookie);
 	ops[0].watch.flag = 0;

-	ret = rbd_req_sync_op(dev, NULL,
+	ret = rbd_req_sync_op(rbd_dev, NULL,
 			      CEPH_NOSNAP,
 			      0,
 			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
@@ -1304,33 +1305,34 @@ static int rbd_req_sync_unwatch(struct
rbd_device *dev,
 			      1, obj, 0, 0, NULL, NULL, NULL);

 	rbd_destroy_ops(ops);
-	ceph_osdc_cancel_event(dev->watch_event);
-	dev->watch_event = NULL;
+	ceph_osdc_cancel_event(rbd_dev->watch_event);
+	rbd_dev->watch_event = NULL;
 	return ret;
 }

 struct rbd_notify_info {
-	struct rbd_device *dev;
+	struct rbd_device *rbd_dev;
 };

 static void rbd_notify_cb(u64 ver, u64 notify_id, u8 opcode, void *data)
 {
-	struct rbd_device *dev = (struct rbd_device *)data;
-	if (!dev)
+	struct rbd_device *rbd_dev = (struct rbd_device *)data;
+	if (!rbd_dev)
 		return;

-	dout("rbd_notify_cb %s notify_id=%lld opcode=%d\n", dev->obj_md_name,
+	dout("rbd_notify_cb %s notify_id=%lld opcode=%d\n",
+				rbd_dev->obj_md_name,
 		notify_id, (int)opcode);
 }

 /*
  * Request sync osd notify
  */
-static int rbd_req_sync_notify(struct rbd_device *dev,
+static int rbd_req_sync_notify(struct rbd_device *rbd_dev,
 		          const char *obj)
 {
 	struct ceph_osd_req_op *ops;
-	struct ceph_osd_client *osdc = &dev->rbd_client->client->osdc;
+	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
 	struct ceph_osd_event *event;
 	struct rbd_notify_info info;
 	int payload_len = sizeof(u32) + sizeof(u32);
@@ -1340,7 +1342,7 @@ static int rbd_req_sync_notify(struct rbd_device *dev,
 	if (ret < 0)
 		return ret;

-	info.dev = dev;
+	info.rbd_dev = rbd_dev;

 	ret = ceph_osdc_create_event(osdc, rbd_notify_cb, 1,
 				     (void *)&info, &event);
@@ -1353,7 +1355,7 @@ static int rbd_req_sync_notify(struct rbd_device *dev,
 	ops[0].watch.prot_ver = RADOS_NOTIFY_VER;
 	ops[0].watch.timeout = 12;

-	ret = rbd_req_sync_op(dev, NULL,
+	ret = rbd_req_sync_op(rbd_dev, NULL,
 			       CEPH_NOSNAP,
 			       0,
 			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
@@ -1377,7 +1379,7 @@ fail:
 /*
  * Request sync osd read
  */
-static int rbd_req_sync_exec(struct rbd_device *dev,
+static int rbd_req_sync_exec(struct rbd_device *rbd_dev,
 			     const char *obj,
 			     const char *cls,
 			     const char *method,
@@ -1401,7 +1403,7 @@ static int rbd_req_sync_exec(struct rbd_device *dev,
 	ops[0].cls.indata = data;
 	ops[0].cls.indata_len = len;

-	ret = rbd_req_sync_op(dev, NULL,
+	ret = rbd_req_sync_op(rbd_dev, NULL,
 			       CEPH_NOSNAP,
 			       0,
 			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
@@ -1632,7 +1634,7 @@ out_dh:
 /*
  * create a snapshot
  */
-static int rbd_header_add_snap(struct rbd_device *dev,
+static int rbd_header_add_snap(struct rbd_device *rbd_dev,
 			       const char *snap_name,
 			       gfp_t gfp_flags)
 {
@@ -1644,11 +1646,11 @@ static int rbd_header_add_snap(struct rbd_device
*dev,
 	struct ceph_mon_client *monc;

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

-	monc = &dev->rbd_client->client->monc;
-	ret = ceph_monc_create_snapid(monc, dev->pool_id, &new_snapid);
+	monc = &rbd_dev->rbd_client->client->monc;
+	ret = ceph_monc_create_snapid(monc, rbd_dev->pool_id, &new_snapid);
 	dout("created snapid=%lld\n", new_snapid);
 	if (ret < 0)
 		return ret;
@@ -1663,7 +1665,8 @@ static int rbd_header_add_snap(struct rbd_device *dev,
 	ceph_encode_string_safe(&p, e, snap_name, name_len, bad);
 	ceph_encode_64_safe(&p, e, new_snapid, bad);

-	ret = rbd_req_sync_exec(dev, dev->obj_md_name, "rbd", "snap_add",
+	ret = rbd_req_sync_exec(rbd_dev, rbd_dev->obj_md_name,
+				"rbd", "snap_add",
 				data, p - data, &ver);

 	kfree(data);
@@ -1671,9 +1674,9 @@ static int rbd_header_add_snap(struct rbd_device *dev,
 	if (ret < 0)
 		return ret;

-	down_write(&dev->header_rwsem);
-	dev->header.snapc->seq = new_snapid;
-	up_write(&dev->header_rwsem);
+	down_write(&rbd_dev->header_rwsem);
+	rbd_dev->header.snapc->seq = new_snapid;
+	up_write(&rbd_dev->header_rwsem);

 	return 0;
 bad:
-- 
1.7.5.4

[PATCH 13/16] rbd: rename some fields in struct rbd_dev

[Alex Elder]

An rbd image is not a single object, but a logical construct made up
of an aggregation of objects.

Rename some fields in struct rbd_dev, in hopes of reinforcing this.
    obj         --> image_name
    obj_len     --> image_name_len
    obj_md_name --> header_name

Also rename a related symbol name:
    RBD_MAX_MD_NAME_LEN --> RBD_MAX_HEADER_NAME_LEN

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index c38246b..14e675c 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -161,9 +161,9 @@ struct rbd_device {
 	spinlock_t		lock;		/* queue lock */

 	struct rbd_image_header	header;
-	char			*obj; /* rbd image name */
-	size_t			obj_len;
-	char			*obj_md_name; /* hdr nm. */
+	char			*image_name;
+	size_t			image_name_len;
+	char			*header_name;
 	int			pool_id;

 	struct ceph_osd_event   *watch_event;
@@ -1224,8 +1224,8 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
u8 opcode, void *data)
 	if (!rbd_dev)
 		return;

-	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n", rbd_dev->obj_md_name,
-		notify_id, (int)opcode);
+	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n",
+		rbd_dev->header_name, notify_id, (int) opcode);
 	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
 	rc = __rbd_refresh_header(rbd_dev);
 	mutex_unlock(&ctl_mutex);
@@ -1233,7 +1233,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
u8 opcode, void *data)
 		pr_warning(RBD_DRV_NAME "%d got notification but failed to "
 			   " update snaps: %d\n", rbd_dev->major, rc);

-	rbd_req_sync_notify_ack(rbd_dev, ver, notify_id, rbd_dev->obj_md_name);
+	rbd_req_sync_notify_ack(rbd_dev, ver, notify_id, rbd_dev->header_name);
 }

 /*
@@ -1321,7 +1321,7 @@ static void rbd_notify_cb(u64 ver, u64 notify_id,
u8 opcode, void *data)
 		return;

 	dout("rbd_notify_cb %s notify_id=%lld opcode=%d\n",
-				rbd_dev->obj_md_name,
+				rbd_dev->header_name,
 		notify_id, (int)opcode);
 }

@@ -1599,7 +1599,7 @@ static int rbd_read_header(struct rbd_device *rbd_dev,

 		rc = rbd_req_sync_read(rbd_dev,
 				       NULL, CEPH_NOSNAP,
-				       rbd_dev->obj_md_name,
+				       rbd_dev->header_name,
 				       0, len,
 				       (char *)dh, &ver);
 		if (rc < 0)
@@ -1609,7 +1609,8 @@ static int rbd_read_header(struct rbd_device *rbd_dev,
 		if (rc < 0) {
 			if (rc == -ENXIO)
 				pr_warning("unrecognized header format"
-					   " for image %s", rbd_dev->obj);
+					   " for image %s\n",
+					   rbd_dev->image_name);
 			goto out_dh;
 		}

@@ -1665,7 +1666,7 @@ static int rbd_header_add_snap(struct rbd_device
*rbd_dev,
 	ceph_encode_string_safe(&p, e, snap_name, name_len, bad);
 	ceph_encode_64_safe(&p, e, new_snapid, bad);

-	ret = rbd_req_sync_exec(rbd_dev, rbd_dev->obj_md_name,
+	ret = rbd_req_sync_exec(rbd_dev, rbd_dev->header_name,
 				"rbd", "snap_add",
 				data, p - data, &ver);

@@ -1862,7 +1863,7 @@ static ssize_t rbd_name_show(struct device *dev,
 {
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

-	return sprintf(buf, "%s\n", rbd_dev->obj);
+	return sprintf(buf, "%s\n", rbd_dev->image_name);
 }

 static ssize_t rbd_snap_show(struct device *dev,
@@ -2164,7 +2165,7 @@ static int rbd_init_watch_dev(struct rbd_device
*rbd_dev)
 	int ret, rc;

 	do {
-		ret = rbd_req_sync_watch(rbd_dev, rbd_dev->obj_md_name,
+		ret = rbd_req_sync_watch(rbd_dev, rbd_dev->header_name,
 					 rbd_dev->header.obj_version);
 		if (ret == -ERANGE) {
 			mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
@@ -2327,7 +2328,7 @@ static inline char *dup_token(const char **buf,
size_t *lenp)
 }

 /*
- * This fills in the obj, obj_len, snap_name, rbd_dev, rbd_md_name,
+ * This fills in the image, image_len, snap_name, rbd_dev, header_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.
  *
@@ -2368,18 +2369,18 @@ static char *rbd_add_parse_args(struct
rbd_device *rbd_dev,
 	if (!pool_name)
 		goto out_err;

-	rbd_dev->obj = dup_token(&buf, &rbd_dev->obj_len);
-	if (!rbd_dev->obj)
+	rbd_dev->image_name = dup_token(&buf, &rbd_dev->image_name_len);
+	if (!rbd_dev->image_name)
 		goto out_err;

 	/* Create the name of the header object */

-	rbd_dev->obj_md_name = kmalloc(rbd_dev->obj_len
+	rbd_dev->header_name = kmalloc(rbd_dev->image_name_len
 						+ sizeof (RBD_SUFFIX),
 					GFP_KERNEL);
-	if (!rbd_dev->obj_md_name)
+	if (!rbd_dev->header_name)
 		goto out_err;
-	sprintf(rbd_dev->obj_md_name, "%s%s", rbd_dev->obj, RBD_SUFFIX);
+	sprintf(rbd_dev->header_name, "%s%s", rbd_dev->image_name, RBD_SUFFIX);

 	/*
 	 * The snapshot name is optional.  If none is is supplied,
@@ -2403,8 +2404,8 @@ static char *rbd_add_parse_args(struct rbd_device
*rbd_dev,
 	return pool_name;

 out_err:
-	kfree(rbd_dev->obj_md_name);
-	kfree(rbd_dev->obj);
+	kfree(rbd_dev->header_name);
+	kfree(rbd_dev->image_name);
 	kfree(pool_name);

 	return ERR_PTR(ret);
@@ -2513,8 +2514,8 @@ err_out_client:
 err_put_id:
 	if (pool_name) {
 		kfree(rbd_dev->snap_name);
-		kfree(rbd_dev->obj_md_name);
-		kfree(rbd_dev->obj);
+		kfree(rbd_dev->header_name);
+		kfree(rbd_dev->image_name);
 		kfree(pool_name);
 	}
 	rbd_id_put(rbd_dev);
@@ -2556,7 +2557,7 @@ static void rbd_dev_release(struct device *dev)
 						    rbd_dev->watch_request);
 	}
 	if (rbd_dev->watch_event)
-		rbd_req_sync_unwatch(rbd_dev, rbd_dev->obj_md_name);
+		rbd_req_sync_unwatch(rbd_dev, rbd_dev->header_name);

 	rbd_put_client(rbd_dev);

@@ -2566,8 +2567,8 @@ static void rbd_dev_release(struct device *dev)

 	/* done with the id, and with the rbd_dev */
 	kfree(rbd_dev->snap_name);
-	kfree(rbd_dev->obj_md_name);
-	kfree(rbd_dev->obj);
+	kfree(rbd_dev->header_name);
+	kfree(rbd_dev->image_name);
 	rbd_id_put(rbd_dev);
 	kfree(rbd_dev);

@@ -2638,7 +2639,7 @@ static ssize_t rbd_snap_add(struct device *dev,
 	mutex_unlock(&ctl_mutex);

 	/* make a best effort, don't error if failed */
-	rbd_req_sync_notify(rbd_dev, rbd_dev->obj_md_name);
+	rbd_req_sync_notify(rbd_dev, rbd_dev->header_name);

 	ret = count;
 	kfree(name);
-- 
1.7.5.4

[PATCH 14/16] rbd: more symbol renames

[Alex Elder]

Rename variables named "obj" which represent object names so they're
consistenly named "object_name".

Rename the "cls" and "method" parameters in rbd_req_sync_exec()
to be "class_name" and "method_name", and make similar changes
to the names of local variables in that function representing
the lengths of those names.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 14e675c..d29c864 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -857,7 +857,7 @@ static int rbd_do_request(struct request *rq,
 			  struct rbd_device *rbd_dev,
 			  struct ceph_snap_context *snapc,
 			  u64 snapid,
-			  const char *obj, u64 ofs, u64 len,
+			  const char *object_name, u64 ofs, u64 len,
 			  struct bio *bio,
 			  struct page **pages,
 			  int num_pages,
@@ -893,7 +893,8 @@ static int rbd_do_request(struct request *rq,
 		req_data->coll_index = coll_index;
 	}

-	dout("rbd_do_request obj=%s ofs=%lld len=%lld\n", obj, len, ofs);
+	dout("rbd_do_request object_name=%s ofs=%lld len=%lld\n",
+		object_name, len, ofs);

 	down_read(&rbd_dev->header_rwsem);

@@ -918,7 +919,7 @@ static int rbd_do_request(struct request *rq,
 	reqhead = req->r_request->front.iov_base;
 	reqhead->snapid = cpu_to_le64(CEPH_NOSNAP);

-	strncpy(req->r_oid, obj, sizeof(req->r_oid));
+	strncpy(req->r_oid, object_name, sizeof(req->r_oid));
 	req->r_oid_len = strlen(req->r_oid);

 	layout = &req->r_file_layout;
@@ -1019,7 +1020,7 @@ static int rbd_req_sync_op(struct rbd_device *rbd_dev,
 			   int flags,
 			   struct ceph_osd_req_op *orig_ops,
 			   int num_reply,
-			   const char *obj,
+			   const char *object_name,
 			   u64 ofs, u64 len,
 			   char *buf,
 			   struct ceph_osd_request **linger_req,
@@ -1050,7 +1051,7 @@ static int rbd_req_sync_op(struct rbd_device *rbd_dev,
 	}

 	ret = rbd_do_request(NULL, rbd_dev, snapc, snapid,
-			  obj, ofs, len, NULL,
+			  object_name, ofs, len, NULL,
 			  pages, num_pages,
 			  flags,
 			  ops,
@@ -1171,7 +1172,7 @@ static int rbd_req_read(struct request *rq,
 static int rbd_req_sync_read(struct rbd_device *rbd_dev,
 			  struct ceph_snap_context *snapc,
 			  u64 snapid,
-			  const char *obj,
+			  const char *object_name,
 			  u64 ofs, u64 len,
 			  char *buf,
 			  u64 *ver)
@@ -1181,7 +1182,7 @@ static int rbd_req_sync_read(struct rbd_device
*rbd_dev,
 			       CEPH_OSD_OP_READ,
 			       CEPH_OSD_FLAG_READ,
 			       NULL,
-			       1, obj, ofs, len, buf, NULL, ver);
+			       1, object_name, ofs, len, buf, NULL, ver);
 }

 /*
@@ -1190,7 +1191,7 @@ static int rbd_req_sync_read(struct rbd_device
*rbd_dev,
 static int rbd_req_sync_notify_ack(struct rbd_device *rbd_dev,
 				   u64 ver,
 				   u64 notify_id,
-				   const char *obj)
+				   const char *object_name)
 {
 	struct ceph_osd_req_op *ops;
 	int ret;
@@ -1204,7 +1205,7 @@ static int rbd_req_sync_notify_ack(struct
rbd_device *rbd_dev,
 	ops[0].watch.flag = 0;

 	ret = rbd_do_request(NULL, rbd_dev, NULL, CEPH_NOSNAP,
-			  obj, 0, 0, NULL,
+			  object_name, 0, 0, NULL,
 			  NULL, 0,
 			  CEPH_OSD_FLAG_READ,
 			  ops,
@@ -1240,7 +1241,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
u8 opcode, void *data)
  * Request sync osd watch
  */
 static int rbd_req_sync_watch(struct rbd_device *rbd_dev,
-			      const char *obj,
+			      const char *object_name,
 			      u64 ver)
 {
 	struct ceph_osd_req_op *ops;
@@ -1264,7 +1265,7 @@ static int rbd_req_sync_watch(struct rbd_device
*rbd_dev,
 			      0,
 			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			      ops,
-			      1, obj, 0, 0, NULL,
+			      1, object_name, 0, 0, NULL,
 			      &rbd_dev->watch_request, NULL);

 	if (ret < 0)
@@ -1285,7 +1286,7 @@ fail:
  * Request sync osd unwatch
  */
 static int rbd_req_sync_unwatch(struct rbd_device *rbd_dev,
-				const char *obj)
+				const char *object_name)
 {
 	struct ceph_osd_req_op *ops;

@@ -1302,7 +1303,7 @@ static int rbd_req_sync_unwatch(struct rbd_device
*rbd_dev,
 			      0,
 			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			      ops,
-			      1, obj, 0, 0, NULL, NULL, NULL);
+			      1, object_name, 0, 0, NULL, NULL, NULL);

 	rbd_destroy_ops(ops);
 	ceph_osdc_cancel_event(rbd_dev->watch_event);
@@ -1329,7 +1330,7 @@ static void rbd_notify_cb(u64 ver, u64 notify_id,
u8 opcode, void *data)
  * Request sync osd notify
  */
 static int rbd_req_sync_notify(struct rbd_device *rbd_dev,
-		          const char *obj)
+		          const char *object_name)
 {
 	struct ceph_osd_req_op *ops;
 	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
@@ -1360,7 +1361,7 @@ static int rbd_req_sync_notify(struct rbd_device
*rbd_dev,
 			       0,
 			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			       ops,
-			       1, obj, 0, 0, NULL, NULL, NULL);
+			       1, object_name, 0, 0, NULL, NULL, NULL);
 	if (ret < 0)
 		goto fail_event;

@@ -1380,25 +1381,25 @@ fail:
  * Request sync osd read
  */
 static int rbd_req_sync_exec(struct rbd_device *rbd_dev,
-			     const char *obj,
-			     const char *cls,
-			     const char *method,
+			     const char *object_name,
+			     const char *class_name,
+			     const char *method_name,
 			     const char *data,
 			     int len,
 			     u64 *ver)
 {
 	struct ceph_osd_req_op *ops;
-	int cls_len = strlen(cls);
-	int method_len = strlen(method);
+	int class_name_len = strlen(class_name);
+	int method_name_len = strlen(method_name);
 	int ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_CALL,
-				    cls_len + method_len + len);
+				    class_name_len + method_name_len + len);
 	if (ret < 0)
 		return ret;

-	ops[0].cls.class_name = cls;
-	ops[0].cls.class_len = (__u8)cls_len;
-	ops[0].cls.method_name = method;
-	ops[0].cls.method_len = (__u8)method_len;
+	ops[0].cls.class_name = class_name;
+	ops[0].cls.class_len = (__u8) class_name_len;
+	ops[0].cls.method_name = method_name;
+	ops[0].cls.method_len = (__u8) method_name_len;
 	ops[0].cls.argc = 0;
 	ops[0].cls.indata = data;
 	ops[0].cls.indata_len = len;
@@ -1408,7 +1409,7 @@ static int rbd_req_sync_exec(struct rbd_device
*rbd_dev,
 			       0,
 			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			       ops,
-			       1, obj, 0, 0, NULL, NULL, ver);
+			       1, object_name, 0, 0, NULL, NULL, ver);

 	rbd_destroy_ops(ops);

-- 
1.7.5.4

[PATCH 15/16] rbd: option symbol renames

[Alex Elder]

Use the name "ceph_opts" consistently (rather than just "opt") for
pointers to a ceph_options structure.

Change the few spots that don't use "rbd_opts" for a rbd_options
pointer to match the rest.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index d29c864..c99ea08 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -270,9 +270,9 @@ static const struct block_device_operations
rbd_bd_ops = {

 /*
  * Initialize an rbd client instance.
- * We own *opt.
+ * We own *ceph_opts.
  */
-static struct rbd_client *rbd_client_create(struct ceph_options *opt,
+static struct rbd_client *rbd_client_create(struct ceph_options *ceph_opts,
 					    struct rbd_options *rbd_opts)
 {
 	struct rbd_client *rbdc;
@@ -288,10 +288,10 @@ static struct rbd_client *rbd_client_create(struct
ceph_options *opt,

 	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);

-	rbdc->client = ceph_create_client(opt, rbdc, 0, 0);
+	rbdc->client = ceph_create_client(ceph_opts, rbdc, 0, 0);
 	if (IS_ERR(rbdc->client))
 		goto out_mutex;
-	opt = NULL; /* Now rbdc->client is responsible for opt */
+	ceph_opts = NULL; /* Now rbdc->client is responsible for ceph_opts */

 	ret = ceph_open_session(rbdc->client);
 	if (ret < 0)
@@ -314,23 +314,23 @@ out_mutex:
 	mutex_unlock(&ctl_mutex);
 	kfree(rbdc);
 out_opt:
-	if (opt)
-		ceph_destroy_options(opt);
+	if (ceph_opts)
+		ceph_destroy_options(ceph_opts);
 	return ERR_PTR(ret);
 }

 /*
  * Find a ceph client with specific addr and configuration.
  */
-static struct rbd_client *__rbd_client_find(struct ceph_options *opt)
+static struct rbd_client *__rbd_client_find(struct ceph_options *ceph_opts)
 {
 	struct rbd_client *client_node;

-	if (opt->flags & CEPH_OPT_NOSHARE)
+	if (ceph_opts->flags & CEPH_OPT_NOSHARE)
 		return NULL;

 	list_for_each_entry(client_node, &rbd_client_list, node)
-		if (ceph_compare_options(opt, client_node->client) == 0)
+		if (!ceph_compare_options(ceph_opts, client_node->client))
 			return client_node;
 	return NULL;
 }
@@ -346,7 +346,7 @@ enum {
 	/* string args above */
 };

-static match_table_t rbdopt_tokens = {
+static match_table_t rbd_opts_tokens = {
 	{Opt_notify_timeout, "notify_timeout=%d"},
 	/* int args above */
 	/* string args above */
@@ -355,11 +355,11 @@ static match_table_t rbdopt_tokens = {

 static int parse_rbd_opts_token(char *c, void *private)
 {
-	struct rbd_options *rbdopt = private;
+	struct rbd_options *rbd_opts = private;
 	substring_t argstr[MAX_OPT_ARGS];
 	int token, intval, ret;

-	token = match_token(c, rbdopt_tokens, argstr);
+	token = match_token(c, rbd_opts_tokens, argstr);
 	if (token < 0)
 		return -EINVAL;

@@ -380,7 +380,7 @@ static int parse_rbd_opts_token(char *c, void *private)

 	switch (token) {
 	case Opt_notify_timeout:
-		rbdopt->notify_timeout = intval;
+		rbd_opts->notify_timeout = intval;
 		break;
 	default:
 		BUG_ON(token);
@@ -397,7 +397,7 @@ static struct rbd_client *rbd_get_client(const char
*mon_addr,
 					 char *options)
 {
 	struct rbd_client *rbdc;
-	struct ceph_options *opt;
+	struct ceph_options *ceph_opts;
 	struct rbd_options *rbd_opts;

 	rbd_opts = kzalloc(sizeof(*rbd_opts), GFP_KERNEL);
@@ -406,29 +406,29 @@ static struct rbd_client *rbd_get_client(const
char *mon_addr,

 	rbd_opts->notify_timeout = RBD_NOTIFY_TIMEOUT_DEFAULT;

-	opt = ceph_parse_options(options, mon_addr,
+	ceph_opts = ceph_parse_options(options, mon_addr,
 				mon_addr + mon_addr_len,
 				parse_rbd_opts_token, rbd_opts);
-	if (IS_ERR(opt)) {
+	if (IS_ERR(ceph_opts)) {
 		kfree(rbd_opts);
-		return ERR_CAST(opt);
+		return ERR_CAST(ceph_opts);
 	}

 	spin_lock(&rbd_client_list_lock);
-	rbdc = __rbd_client_find(opt);
+	rbdc = __rbd_client_find(ceph_opts);
 	if (rbdc) {
 		/* using an existing client */
 		kref_get(&rbdc->kref);
 		spin_unlock(&rbd_client_list_lock);

-		ceph_destroy_options(opt);
+		ceph_destroy_options(ceph_opts);
 		kfree(rbd_opts);

 		return rbdc;
 	}
 	spin_unlock(&rbd_client_list_lock);

-	rbdc = rbd_client_create(opt, rbd_opts);
+	rbdc = rbd_client_create(ceph_opts, rbd_opts);

 	if (IS_ERR(rbdc))
 		kfree(rbd_opts);
-- 
1.7.5.4

[PATCH 16/16] rbd: kill num_reply parameters

[Alex Elder]

Several functions include a num_reply parameter, but it is never
used.  Just get rid of it everywhere--it seems to be something
that never got fully implemented.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index c99ea08..c023be5 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -863,7 +863,6 @@ static int rbd_do_request(struct request *rq,
 			  int num_pages,
 			  int flags,
 			  struct ceph_osd_req_op *ops,
-			  int num_reply,
 			  struct rbd_req_coll *coll,
 			  int coll_index,
 			  void (*rbd_cb)(struct ceph_osd_request *req,
@@ -1019,7 +1018,6 @@ static int rbd_req_sync_op(struct rbd_device *rbd_dev,
 			   int opcode,
 			   int flags,
 			   struct ceph_osd_req_op *orig_ops,
-			   int num_reply,
 			   const char *object_name,
 			   u64 ofs, u64 len,
 			   char *buf,
@@ -1055,7 +1053,6 @@ static int rbd_req_sync_op(struct rbd_device *rbd_dev,
 			  pages, num_pages,
 			  flags,
 			  ops,
-			  2,
 			  NULL, 0,
 			  NULL,
 			  linger_req, ver);
@@ -1080,7 +1077,7 @@ static int rbd_do_op(struct request *rq,
 		     struct rbd_device *rbd_dev,
 		     struct ceph_snap_context *snapc,
 		     u64 snapid,
-		     int opcode, int flags, int num_reply,
+		     int opcode, int flags,
 		     u64 ofs, u64 len,
 		     struct bio *bio,
 		     struct rbd_req_coll *coll,
@@ -1119,7 +1116,6 @@ static int rbd_do_op(struct request *rq,
 			     NULL, 0,
 			     flags,
 			     ops,
-			     num_reply,
 			     coll, coll_index,
 			     rbd_req_cb, 0, NULL);

@@ -1143,7 +1139,6 @@ static int rbd_req_write(struct request *rq,
 	return rbd_do_op(rq, rbd_dev, snapc, CEPH_NOSNAP,
 			 CEPH_OSD_OP_WRITE,
 			 CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
-			 2,
 			 ofs, len, bio, coll, coll_index);
 }

@@ -1162,7 +1157,6 @@ static int rbd_req_read(struct request *rq,
 			 snapid,
 			 CEPH_OSD_OP_READ,
 			 CEPH_OSD_FLAG_READ,
-			 2,
 			 ofs, len, bio, coll, coll_index);
 }

@@ -1182,7 +1176,7 @@ static int rbd_req_sync_read(struct rbd_device
*rbd_dev,
 			       CEPH_OSD_OP_READ,
 			       CEPH_OSD_FLAG_READ,
 			       NULL,
-			       1, object_name, ofs, len, buf, NULL, ver);
+			       object_name, ofs, len, buf, NULL, ver);
 }

 /*
@@ -1209,7 +1203,6 @@ static int rbd_req_sync_notify_ack(struct
rbd_device *rbd_dev,
 			  NULL, 0,
 			  CEPH_OSD_FLAG_READ,
 			  ops,
-			  1,
 			  NULL, 0,
 			  rbd_simple_req_cb, 0, NULL);

@@ -1265,7 +1258,7 @@ static int rbd_req_sync_watch(struct rbd_device
*rbd_dev,
 			      0,
 			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			      ops,
-			      1, object_name, 0, 0, NULL,
+			      object_name, 0, 0, NULL,
 			      &rbd_dev->watch_request, NULL);

 	if (ret < 0)
@@ -1303,7 +1296,7 @@ static int rbd_req_sync_unwatch(struct rbd_device
*rbd_dev,
 			      0,
 			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			      ops,
-			      1, object_name, 0, 0, NULL, NULL, NULL);
+			      object_name, 0, 0, NULL, NULL, NULL);

 	rbd_destroy_ops(ops);
 	ceph_osdc_cancel_event(rbd_dev->watch_event);
@@ -1361,7 +1354,7 @@ static int rbd_req_sync_notify(struct rbd_device
*rbd_dev,
 			       0,
 			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			       ops,
-			       1, object_name, 0, 0, NULL, NULL, NULL);
+			       object_name, 0, 0, NULL, NULL, NULL);
 	if (ret < 0)
 		goto fail_event;

@@ -1409,7 +1402,7 @@ static int rbd_req_sync_exec(struct rbd_device
*rbd_dev,
 			       0,
 			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			       ops,
-			       1, object_name, 0, 0, NULL, NULL, ver);
+			       object_name, 0, 0, NULL, NULL, ver);

 	rbd_destroy_ops(ops);

-- 
1.7.5.4

Re: [PATCH 02/16] rbd: drop a useless local variable

[Yehuda Sadeh Weinraub]

Reviewed-by: Yehuda Sadeh <yehuda@inktank.com>

On Wed, Jul 11, 2012 at 7:00 AM, Alex Elder <elder@inktank.com> wrote:
> In rbd_req_sync_notify_ack(), a local variable was needlessly being
> used to hold a null pointer.  Just pass NULL instead.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>  drivers/block/rbd.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 8f428a8..2ae3bb0 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -1187,7 +1187,6 @@ static int rbd_req_sync_notify_ack(struct
> rbd_device *dev,
>                                    const char *obj)
>  {
>         struct ceph_osd_req_op *ops;
> -       struct page **pages = NULL;
>         int ret;
>
>         ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_NOTIFY_ACK, 0);
> @@ -1200,7 +1199,7 @@ static int rbd_req_sync_notify_ack(struct
> rbd_device *dev,
>
>         ret = rbd_do_request(NULL, dev, NULL, CEPH_NOSNAP,
>                           obj, 0, 0, NULL,
> -                         pages, 0,
> +                         NULL, 0,
>                           CEPH_OSD_FLAG_READ,
>                           ops,
>                           1,
> --
> 1.7.5.4
>
> --
> 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 01/16] libceph: fix off-by-one bug in ceph_encode_filepath()

[Yehuda Sadeh]

Reviewed-by: Yehuda Sadeh <yehuda@inktank.com>

On Wed, Jul 11, 2012 at 7:00 AM, Alex Elder <elder@inktank.com> wrote:
> There is a BUG_ON() call that doesn't account for the single byte
> structure version at the start of an encoded filepath in
> ceph_encode_filepath().  Fix that.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>  include/linux/ceph/decode.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
> index d8615de..bcbd66c 100644
> --- a/include/linux/ceph/decode.h
> +++ b/include/linux/ceph/decode.h
> @@ -151,7 +151,7 @@ static inline void ceph_encode_filepath(void **p,
> void *end,
>                                         u64 ino, const char *path)
>  {
>         u32 len = path ? strlen(path) : 0;
> -       BUG_ON(*p + sizeof(ino) + sizeof(len) + len > end);
> +       BUG_ON(*p + 1 + sizeof(ino) + sizeof(len) + len > end);
>         ceph_encode_8(p, 1);
>         ceph_encode_64(p, ino);
>         ceph_encode_32(p, len);
> --
> 1.7.5.4
>
> --
> 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 03/16] libceph: define ceph_decode_string()

[Yehuda Sadeh]

Reviewed-by: Yehuda Sadeh <yehuda@inktank.com>

On Wed, Jul 11, 2012 at 7:00 AM, Alex Elder <elder@inktank.com> wrote:
> There is no string decoding function defined in <decode.h>, so this
> defines one.
>
> This function is a little different from the others in that the
> length of the encoded string is not known a priori.  So the
> interface is defined a bit like snprintf(), where the value returned
> indicates the space required--even if it's more than the space
> allotted.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>  include/linux/ceph/decode.h |   36 ++++++++++++++++++++++++++++++++++++
>  1 files changed, 36 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
> index bcbd66c..7ead11fc 100644
> --- a/include/linux/ceph/decode.h
> +++ b/include/linux/ceph/decode.h
> @@ -44,6 +44,42 @@ static inline void ceph_decode_copy(void **p, void
> *pv, size_t n)
>  }
>
>  /*
> + * Decode the wire-encoded string at *p into the buffer "s"
> + * provided, whose size is indicated by "size".  Note that "s" can
> + * be a null pointer if size is 0.  If it fits, the resulting string
> + * will always be terminated with '\0'; otherwise the buffer will
> + * be unchanged.
> + *
> + * Returns the length of the encoded string (which may be greater
> + * than or equal to the buffer size).  The return value does not
> + * include the terminating '\0'.
> + *
> + * If the the return value is less than the size provided, *p will
> + * be advanced past the decoded data; otherwise it is unchanged.
> + * This allows for a two call sequence to be used to allocate
> + * sufficient space for the string.
> + *
> + * NB  It is assumed that *p refers to a block of valid memory
> + *     sufficient to hold the length field followed by the number
> + *     of bytes indicated by that field.
> + */
> +static inline size_t ceph_decode_string(void **p, char *s, size_t size)
> +{
> +       size_t len;
> +
> +       len = get_unaligned_le32(*p);
> +       if (size < len + 1)
> +               return len;
> +
> +       if (len)
> +               memcpy(s, (char *) *p + sizeof (u32), len);
> +       *(s + len) = '\0';
> +       *p += sizeof (u32) + len;
> +
> +       return len;
> +}
> +
> +/*
>   * bounds check input.
>   */
>  static inline int ceph_has_room(void **p, void *end, size_t n)
> --
> 1.7.5.4
>
> --
> 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 04/16] libceph: define ceph_extract_encoded_string()

[Yehuda Sadeh]

On Wed, Jul 11, 2012 at 7:01 AM, Alex Elder <elder@inktank.com> wrote:
> This adds a new utility routine which will return a dynamically-
> allocated buffer containing a string that has been decoded from ceph
> over-the-wire format.  It also returns the length of the string
> if the address of a size variable is supplied to receive it.
>
> For now, no gfp_flags parameter is defined (GFP_KERNEL is used) but
> it could be easily be added if needed.
>

I'd rather have it upfront, will help avoiding future errors.

> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>  include/linux/ceph/decode.h |   29 +++++++++++++++++++++++++++++
>  1 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
> index 7ead11fc..7759164 100644
> --- a/include/linux/ceph/decode.h
> +++ b/include/linux/ceph/decode.h
> @@ -80,6 +80,35 @@ static inline size_t ceph_decode_string(void **p,
> char *s, size_t size)
>  }
>
>  /*
> + * Allocate a buffer big enough to hold the wire-encoded string, and
> + * decode the string into it.  The resulting string will always be
> + * terminated with '\0'.  If successful, *p will be advanced
> + * past the decoded data.  Also, if lenp is not a null pointer, the
> + * length (not including the terminating '\0') will be recorded in
> + * it.  Note that a zero-length string is a valid return value.
> + *
> + * Returns a pointer to the newly-allocated string buffer, or a
> + * null pointer if memory could not be allocated for the result.
> + * Neither of the arguments is updated if NULL is returned.
> + */
> +static inline char *ceph_extract_encoded_string(void **p, size_t *lenp)
> +{
> +       size_t len;
> +       char *buf;
> +
> +       len = ceph_decode_string(p, NULL, 0);
> +       buf = kmalloc(len + 1, GFP_KERNEL);
> +       if (!buf)
> +               return NULL;
> +
> +       (void) ceph_decode_string(p, buf, len + 1);
> +       if (lenp)
> +               *lenp = len;
> +
> +       return buf;
> +}
> +
> +/*

We don't make an effort here to check whether encoded string buffer is
valid. While we may be checking it somewhere up the stack, this seem
like a generic enough function that could be naively used. Either make
it clear that it's an internal function, or make it check p bounds.

>   * bounds check input.
>   */
>  static inline int ceph_has_room(void **p, void *end, size_t n)
> --
> 1.7.5.4
>
> --
> 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 04/16] libceph: define ceph_extract_encoded_string()

[Sage Weil]

On Wed, 11 Jul 2012, Yehuda Sadeh wrote:
> On Wed, Jul 11, 2012 at 7:01 AM, Alex Elder <elder@inktank.com> wrote:
> > This adds a new utility routine which will return a dynamically-
> > allocated buffer containing a string that has been decoded from ceph
> > over-the-wire format.  It also returns the length of the string
> > if the address of a size variable is supplied to receive it.
> >
> > For now, no gfp_flags parameter is defined (GFP_KERNEL is used) but
> > it could be easily be added if needed.
> >
> 
> I'd rather have it upfront, will help avoiding future errors.
> 
> > Signed-off-by: Alex Elder <elder@inktank.com>
> > ---
> >  include/linux/ceph/decode.h |   29 +++++++++++++++++++++++++++++
> >  1 files changed, 29 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
> > index 7ead11fc..7759164 100644
> > --- a/include/linux/ceph/decode.h
> > +++ b/include/linux/ceph/decode.h
> > @@ -80,6 +80,35 @@ static inline size_t ceph_decode_string(void **p,
> > char *s, size_t size)
> >  }
> >
> >  /*
> > + * Allocate a buffer big enough to hold the wire-encoded string, and
> > + * decode the string into it.  The resulting string will always be
> > + * terminated with '\0'.  If successful, *p will be advanced
> > + * past the decoded data.  Also, if lenp is not a null pointer, the
> > + * length (not including the terminating '\0') will be recorded in
> > + * it.  Note that a zero-length string is a valid return value.
> > + *
> > + * Returns a pointer to the newly-allocated string buffer, or a
> > + * null pointer if memory could not be allocated for the result.
> > + * Neither of the arguments is updated if NULL is returned.
> > + */
> > +static inline char *ceph_extract_encoded_string(void **p, size_t *lenp)
> > +{
> > +       size_t len;
> > +       char *buf;
> > +
> > +       len = ceph_decode_string(p, NULL, 0);
> > +       buf = kmalloc(len + 1, GFP_KERNEL);
> > +       if (!buf)
> > +               return NULL;
> > +
> > +       (void) ceph_decode_string(p, buf, len + 1);
> > +       if (lenp)
> > +               *lenp = len;
> > +
> > +       return buf;
> > +}
> > +
> > +/*
> 
> We don't make an effort here to check whether encoded string buffer is
> valid. While we may be checking it somewhere up the stack, this seem
> like a generic enough function that could be naively used. Either make
> it clear that it's an internal function, or make it check p bounds.

Ditto.  The caller doesn't see the length, so it can't check that it 
doesn't walk past the end of the buffer.  We either need to pass down the 
bounds so that it can return an error, or just open code the callers (as 
we have been doing).



> 
> >   * bounds check input.
> >   */
> >  static inline int ceph_has_room(void **p, void *end, size_t n)
> > --
> > 1.7.5.4
> >
> > --
> > 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
> --
> 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 05/16] rbd: define dup_token()

[Yehuda Sadeh]

On Wed, Jul 11, 2012 at 7:01 AM, Alex Elder <elder@inktank.com> wrote:
> Define a new function dup_token(), to be used during argument
> parsing for making dynamically-allocated copies of tokens being
> parsed.
>
> For now, no gfp_flags parameter is defined (GFP_KERNEL is used) but
> it could be easily be added if needed.

I assume this is specialized enough so that there's no risk in reusing
it in a different context, so for this one we can keep it this way.

Reviewed-by: Yehuda Sadeh <yehuda@inktank.com>


>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>  drivers/block/rbd.c |   36 ++++++++++++++++++++++++++++++++++++
>  1 files changed, 36 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 2ae3bb0..63c132f 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2281,6 +2281,42 @@ static inline size_t copy_token(const char **buf,
>  }
>
>  /*
> + * Finds the next token in *buf, dynamically allocates a buffer big
> + * enough to hold a copy of it, and copies the token into the new
> + * buffer.  The copy is guaranteed to be terminated with '\0'.  Note
> + * that a duplicate buffer is created even for a zero-length token.
> + *
> + * Returns a pointer to the newly-allocated duplicate, or a null
> + * pointer if memory for the duplicate was not available.  If
> + * the lenp argument is a non-null pointer, the length of the token
> + * (not including the '\0') is returned in *lenp.
> + *
> + * If successful, the *buf pointer will be updated to point beyond
> + * the end of the found token.
> + *
> + * Note:  For now, the memory is allocated using GFP_KERNEL.
> + */
> +static inline char *dup_token(const char **buf, size_t *lenp)
> +{
> +       char *dup;
> +       size_t len;
> +
> +       len = next_token(buf);
> +       dup = kmalloc(len + 1, GFP_KERNEL);
> +       if (!dup)
> +               return NULL;
> +
> +       memcpy(dup, *buf, len);
> +       *(dup + len) = '\0';
> +       *buf += len;
> +
> +       if (lenp)
> +               *lenp = len;
> +
> +       return dup;
> +}
> +
> +/*
>   * This fills in the pool_name, obj, obj_len, snap_name, obj_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
> --
> 1.7.5.4
>
> --
> 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 06/16] rbd: rename rbd_dev->block_name

[Yehuda Sadeh]

Reviewed-by: Yehuda Sadeh <yehuda@inktank.com>

On Wed, Jul 11, 2012 at 7:01 AM, Alex Elder <elder@inktank.com> wrote:
> Each rbd image has a name that forms the basis of all data objects
> backing the device.  Old (format 1) images refer to this name as the
> "block name," while new (format 2) images use the term "object
> prefix" for this.
>
> Change the field name in the in-core rbd image header structure to
> reflect the more modern usage.  We intentionally keep the the name
> "block_name" in the on-disk definition for format 1 image headers.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>  drivers/block/rbd.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 63c132f..57d264c 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -78,7 +78,7 @@
>   */
>  struct rbd_image_header {
>         u64 image_size;
> -       char block_name[32];
> +       char object_prefix[32];
>         __u8 obj_order;
>         __u8 crypt_type;
>         __u8 comp_type;
> @@ -518,7 +518,7 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>                 header->snap_names = NULL;
>                 header->snap_sizes = NULL;
>         }
> -       memcpy(header->block_name, ondisk->block_name,
> +       memcpy(header->object_prefix, ondisk->block_name,
>                sizeof(ondisk->block_name));
>
>         header->image_size = le64_to_cpu(ondisk->image_size);
> @@ -620,7 +620,7 @@ static void rbd_header_free(struct rbd_image_header
> *header)
>   * get the actual striped segment name, offset and length
>   */
>  static u64 rbd_get_segment(struct rbd_image_header *header,
> -                          const char *block_name,
> +                          const char *object_prefix,
>                            u64 ofs, u64 len,
>                            char *seg_name, u64 *segofs)
>  {
> @@ -628,7 +628,7 @@ static u64 rbd_get_segment(struct rbd_image_header
> *header,
>
>         if (seg_name)
>                 snprintf(seg_name, RBD_MAX_SEG_NAME_LEN,
> -                        "%s.%012llx", block_name, seg);
> +                        "%s.%012llx", object_prefix, seg);
>
>         ofs = ofs & ((1 << header->obj_order) - 1);
>         len = min_t(u64, len, (1 << header->obj_order) - ofs);
> @@ -1091,7 +1091,7 @@ static int rbd_do_op(struct request *rq,
>                 return -ENOMEM;
>
>         seg_len = rbd_get_segment(&rbd_dev->header,
> -                                 rbd_dev->header.block_name,
> +                                 rbd_dev->header.object_prefix,
>                                   ofs, len,
>                                   seg_name, &seg_ofs);
>
> @@ -1482,7 +1482,7 @@ static void rbd_rq_fn(struct request_queue *q)
>                         /* a bio clone to be passed down to OSD req */
>                         dout("rq->bio->bi_vcnt=%d\n", rq->bio->bi_vcnt);
>                         op_size = rbd_get_segment(&rbd_dev->header,
> -                                                 rbd_dev->header.block_name,
> +                                                 rbd_dev->header.object_prefix,
>                                                   ofs, size,
>                                                   NULL, NULL);
>                         kref_get(&coll->kref);
> --
> 1.7.5.4
>
> --
> 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 01/16] libceph: fix off-by-one bug in ceph_encode_filepath()

[Josh Durgin]

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

On 07/11/2012 07:00 AM, Alex Elder wrote:
> There is a BUG_ON() call that doesn't account for the single byte
> structure version at the start of an encoded filepath in
> ceph_encode_filepath().  Fix that.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   include/linux/ceph/decode.h |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
> index d8615de..bcbd66c 100644
> --- a/include/linux/ceph/decode.h
> +++ b/include/linux/ceph/decode.h
> @@ -151,7 +151,7 @@ static inline void ceph_encode_filepath(void **p,
> void *end,
>   					u64 ino, const char *path)
>   {
>   	u32 len = path ? strlen(path) : 0;
> -	BUG_ON(*p + sizeof(ino) + sizeof(len) + len>  end);
> +	BUG_ON(*p + 1 + sizeof(ino) + sizeof(len) + len>  end);
>   	ceph_encode_8(p, 1);
>   	ceph_encode_64(p, ino);
>   	ceph_encode_32(p, len);

Re: [PATCH 02/16] rbd: drop a useless local variable

[Josh Durgin]

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

On 07/11/2012 07:00 AM, Alex Elder wrote:
> In rbd_req_sync_notify_ack(), a local variable was needlessly being
> used to hold a null pointer.  Just pass NULL instead.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c |    3 +--
>   1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 8f428a8..2ae3bb0 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -1187,7 +1187,6 @@ static int rbd_req_sync_notify_ack(struct
> rbd_device *dev,
>   				   const char *obj)
>   {
>   	struct ceph_osd_req_op *ops;
> -	struct page **pages = NULL;
>   	int ret;
>
>   	ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_NOTIFY_ACK, 0);
> @@ -1200,7 +1199,7 @@ static int rbd_req_sync_notify_ack(struct
> rbd_device *dev,
>
>   	ret = rbd_do_request(NULL, dev, NULL, CEPH_NOSNAP,
>   			  obj, 0, 0, NULL,
> -			  pages, 0,
> +			  NULL, 0,
>   			  CEPH_OSD_FLAG_READ,
>   			  ops,
>   			  1,

Re: [PATCH 03/16] libceph: define ceph_decode_string()

[Josh Durgin]

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

On 07/11/2012 07:00 AM, Alex Elder wrote:
> There is no string decoding function defined in<decode.h>, so this
> defines one.
>
> This function is a little different from the others in that the
> length of the encoded string is not known a priori.  So the
> interface is defined a bit like snprintf(), where the value returned
> indicates the space required--even if it's more than the space
> allotted.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   include/linux/ceph/decode.h |   36 ++++++++++++++++++++++++++++++++++++
>   1 files changed, 36 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
> index bcbd66c..7ead11fc 100644
> --- a/include/linux/ceph/decode.h
> +++ b/include/linux/ceph/decode.h
> @@ -44,6 +44,42 @@ static inline void ceph_decode_copy(void **p, void
> *pv, size_t n)
>   }
>
>   /*
> + * Decode the wire-encoded string at *p into the buffer "s"
> + * provided, whose size is indicated by "size".  Note that "s" can
> + * be a null pointer if size is 0.  If it fits, the resulting string
> + * will always be terminated with '\0'; otherwise the buffer will
> + * be unchanged.
> + *
> + * Returns the length of the encoded string (which may be greater
> + * than or equal to the buffer size).  The return value does not
> + * include the terminating '\0'.
> + *
> + * If the the return value is less than the size provided, *p will
> + * be advanced past the decoded data; otherwise it is unchanged.
> + * This allows for a two call sequence to be used to allocate
> + * sufficient space for the string.
> + *
> + * NB  It is assumed that *p refers to a block of valid memory
> + *     sufficient to hold the length field followed by the number
> + *     of bytes indicated by that field.
> + */
> +static inline size_t ceph_decode_string(void **p, char *s, size_t size)
> +{
> +	size_t len;
> +
> +	len = get_unaligned_le32(*p);
> +	if (size<  len + 1)
> +		return len;
> +
> +	if (len)
> +		memcpy(s, (char *) *p + sizeof (u32), len);
> +	*(s + len) = '\0';
> +	*p += sizeof (u32) + len;
> +
> +	return len;
> +}
> +
> +/*
>    * bounds check input.
>    */
>   static inline int ceph_has_room(void **p, void *end, size_t n)

Re: [PATCH 05/16] rbd: define dup_token()

[Josh Durgin]

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

On 07/11/2012 07:01 AM, Alex Elder wrote:
> Define a new function dup_token(), to be used during argument
> parsing for making dynamically-allocated copies of tokens being
> parsed.
>
> For now, no gfp_flags parameter is defined (GFP_KERNEL is used) but
> it could be easily be added if needed.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c |   36 ++++++++++++++++++++++++++++++++++++
>   1 files changed, 36 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 2ae3bb0..63c132f 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2281,6 +2281,42 @@ static inline size_t copy_token(const char **buf,
>   }
>
>   /*
> + * Finds the next token in *buf, dynamically allocates a buffer big
> + * enough to hold a copy of it, and copies the token into the new
> + * buffer.  The copy is guaranteed to be terminated with '\0'.  Note
> + * that a duplicate buffer is created even for a zero-length token.
> + *
> + * Returns a pointer to the newly-allocated duplicate, or a null
> + * pointer if memory for the duplicate was not available.  If
> + * the lenp argument is a non-null pointer, the length of the token
> + * (not including the '\0') is returned in *lenp.
> + *
> + * If successful, the *buf pointer will be updated to point beyond
> + * the end of the found token.
> + *
> + * Note:  For now, the memory is allocated using GFP_KERNEL.
> + */
> +static inline char *dup_token(const char **buf, size_t *lenp)
> +{
> +	char *dup;
> +	size_t len;
> +
> +	len = next_token(buf);
> +	dup = kmalloc(len + 1, GFP_KERNEL);
> +	if (!dup)
> +		return NULL;
> +
> +	memcpy(dup, *buf, len);
> +	*(dup + len) = '\0';
> +	*buf += len;
> +
> +	if (lenp)
> +		*lenp = len;
> +
> +	return dup;
> +}
> +
> +/*
>    * This fills in the pool_name, obj, obj_len, snap_name, obj_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

Re: [PATCH 06/16] rbd: rename rbd_dev->block_name

[Josh Durgin]

Might want to rename block_name in struct rbd_image_header_ondisk too.

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

On 07/11/2012 07:01 AM, Alex Elder wrote:
> Each rbd image has a name that forms the basis of all data objects
> backing the device.  Old (format 1) images refer to this name as the
> "block name," while new (format 2) images use the term "object
> prefix" for this.
>
> Change the field name in the in-core rbd image header structure to
> reflect the more modern usage.  We intentionally keep the the name
> "block_name" in the on-disk definition for format 1 image headers.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c |   12 ++++++------
>   1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 63c132f..57d264c 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -78,7 +78,7 @@
>    */
>   struct rbd_image_header {
>   	u64 image_size;
> -	char block_name[32];
> +	char object_prefix[32];
>   	__u8 obj_order;
>   	__u8 crypt_type;
>   	__u8 comp_type;
> @@ -518,7 +518,7 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>   		header->snap_names = NULL;
>   		header->snap_sizes = NULL;
>   	}
> -	memcpy(header->block_name, ondisk->block_name,
> +	memcpy(header->object_prefix, ondisk->block_name,
>   	       sizeof(ondisk->block_name));
>
>   	header->image_size = le64_to_cpu(ondisk->image_size);
> @@ -620,7 +620,7 @@ static void rbd_header_free(struct rbd_image_header
> *header)
>    * get the actual striped segment name, offset and length
>    */
>   static u64 rbd_get_segment(struct rbd_image_header *header,
> -			   const char *block_name,
> +			   const char *object_prefix,
>   			   u64 ofs, u64 len,
>   			   char *seg_name, u64 *segofs)
>   {
> @@ -628,7 +628,7 @@ static u64 rbd_get_segment(struct rbd_image_header
> *header,
>
>   	if (seg_name)
>   		snprintf(seg_name, RBD_MAX_SEG_NAME_LEN,
> -			 "%s.%012llx", block_name, seg);
> +			 "%s.%012llx", object_prefix, seg);
>
>   	ofs = ofs&  ((1<<  header->obj_order) - 1);
>   	len = min_t(u64, len, (1<<  header->obj_order) - ofs);
> @@ -1091,7 +1091,7 @@ static int rbd_do_op(struct request *rq,
>   		return -ENOMEM;
>
>   	seg_len = rbd_get_segment(&rbd_dev->header,
> -				  rbd_dev->header.block_name,
> +				  rbd_dev->header.object_prefix,
>   				  ofs, len,
>   				  seg_name,&seg_ofs);
>
> @@ -1482,7 +1482,7 @@ static void rbd_rq_fn(struct request_queue *q)
>   			/* a bio clone to be passed down to OSD req */
>   			dout("rq->bio->bi_vcnt=%d\n", rq->bio->bi_vcnt);
>   			op_size = rbd_get_segment(&rbd_dev->header,
> -						  rbd_dev->header.block_name,
> +						  rbd_dev->header.object_prefix,
>   						  ofs, size,
>   						  NULL, NULL);
>   			kref_get(&coll->kref);

Re: [PATCH 07/16] rbd: dynamically allocate object prefix

[Josh Durgin]

On 07/11/2012 07:01 AM, Alex Elder wrote:
> There is no need to impose a small limit the length of the object
> prefix recorded for an rbd image in a struct rbd_image_header.
> Remove the limitation by allocating space for the object prefix
> dynamically.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c |   22 +++++++++++++++++-----
>   1 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 57d264c..3aa0ca0 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -78,7 +78,7 @@
>    */
>   struct rbd_image_header {
>   	u64 image_size;
> -	char object_prefix[32];
> +	char *object_prefix;
>   	__u8 obj_order;
>   	__u8 crypt_type;
>   	__u8 comp_type;
> @@ -518,8 +518,15 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>   		header->snap_names = NULL;
>   		header->snap_sizes = NULL;
>   	}
> +
> +	header->object_prefix = kmalloc(sizeof (ondisk->block_name) + 1,
> +					gfp_flags);
> +	if (!header->object_prefix)
> +		goto err_sizes;
> +
>   	memcpy(header->object_prefix, ondisk->block_name,
>   	       sizeof(ondisk->block_name));
> +	header->object_prefix[sizeof (ondisk->block_name)] = '\0';
>
>   	header->image_size = le64_to_cpu(ondisk->image_size);
>   	header->obj_order = ondisk->options.order;
> @@ -546,6 +553,8 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>
>   	return 0;
>
> +err_sizes:
> +	kfree(header->snap_sizes);
>   err_names:
>   	kfree(header->snap_names);
>   err_snapc:
> @@ -611,9 +620,10 @@ done:
>
>   static void rbd_header_free(struct rbd_image_header *header)
>   {
> -	kfree(header->snapc);
> -	kfree(header->snap_names);
> +	kfree(header->object_prefix);
>   	kfree(header->snap_sizes);
> +	kfree(header->snap_names);
> +	kfree(header->snapc);
>   }
>
>   /*
> @@ -1711,15 +1721,17 @@ static int __rbd_refresh_header(struct
> rbd_device *rbd_dev)
>   		   if head moves */
>   		follow_seq = 1;
>
> -	kfree(rbd_dev->header.snapc);
> -	kfree(rbd_dev->header.snap_names);
> +	kfree(rbd_dev->header.object_prefix);

Object prefix isn't going to change, so it doesn't need to be reset
here. Otherwise looks good.

>   	kfree(rbd_dev->header.snap_sizes);
> +	kfree(rbd_dev->header.snap_names);
> +	kfree(rbd_dev->header.snapc);
>
>   	rbd_dev->header.total_snaps = h.total_snaps;
>   	rbd_dev->header.snapc = h.snapc;
>   	rbd_dev->header.snap_names = h.snap_names;
>   	rbd_dev->header.snap_names_len = h.snap_names_len;
>   	rbd_dev->header.snap_sizes = h.snap_sizes;
> +	rbd_dev->header.object_prefix = h.object_prefix;
>   	if (follow_seq)
>   		rbd_dev->header.snapc->seq = rbd_dev->header.snapc->snaps[0];
>   	else

Re: [PATCH 04/16] libceph: define ceph_extract_encoded_string()

[Alex Elder]

On 07/11/2012 12:20 PM, Yehuda Sadeh wrote:
> On Wed, Jul 11, 2012 at 7:01 AM, Alex Elder <elder@inktank.com> wrote:
>> This adds a new utility routine which will return a dynamically-
>> allocated buffer containing a string that has been decoded from ceph
>> over-the-wire format.  It also returns the length of the string
>> if the address of a size variable is supplied to receive it.
>>
>> For now, no gfp_flags parameter is defined (GFP_KERNEL is used) but
>> it could be easily be added if needed.
>>
> 
> I'd rather have it upfront, will help avoiding future errors.

I actually wanted to do exactly that before I sent it but I
forgot.  I guess I got all caught up in the excitement.

>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>  include/linux/ceph/decode.h |   29 +++++++++++++++++++++++++++++
>>  1 files changed, 29 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
>> index 7ead11fc..7759164 100644
>> --- a/include/linux/ceph/decode.h
>> +++ b/include/linux/ceph/decode.h
>> @@ -80,6 +80,35 @@ static inline size_t ceph_decode_string(void **p,
>> char *s, size_t size)
>>  }
>>
>>  /*
>> + * Allocate a buffer big enough to hold the wire-encoded string, and
>> + * decode the string into it.  The resulting string will always be
>> + * terminated with '\0'.  If successful, *p will be advanced
>> + * past the decoded data.  Also, if lenp is not a null pointer, the
>> + * length (not including the terminating '\0') will be recorded in
>> + * it.  Note that a zero-length string is a valid return value.
>> + *
>> + * Returns a pointer to the newly-allocated string buffer, or a
>> + * null pointer if memory could not be allocated for the result.
>> + * Neither of the arguments is updated if NULL is returned.
>> + */
>> +static inline char *ceph_extract_encoded_string(void **p, size_t *lenp)
>> +{
>> +       size_t len;
>> +       char *buf;
>> +
>> +       len = ceph_decode_string(p, NULL, 0);
>> +       buf = kmalloc(len + 1, GFP_KERNEL);
>> +       if (!buf)
>> +               return NULL;
>> +
>> +       (void) ceph_decode_string(p, buf, len + 1);
>> +       if (lenp)
>> +               *lenp = len;
>> +
>> +       return buf;
>> +}
>> +
>> +/*
> 
> We don't make an effort here to check whether encoded string buffer is
> valid. While we may be checking it somewhere up the stack, this seem
> like a generic enough function that could be naively used. Either make
> it clear that it's an internal function, or make it check p bounds.

Are you saying I should have the caller provide the length of the
buffer, and ensure we don't exceed it?

Should I assume that applies to the previous patch also?

In any case, I will rework it, but I want to make sure I understand
what you're saying.

					-Alex

>>   * bounds check input.
>>   */
>>  static inline int ceph_has_room(void **p, void *end, size_t n)
>> --
>> 1.7.5.4
>>
>> --
>> 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 07/16] rbd: dynamically allocate object prefix

[Alex Elder]

On 07/11/2012 02:12 PM, Josh Durgin wrote:
>>   /*
>> @@ -1711,15 +1721,17 @@ static int __rbd_refresh_header(struct
>> rbd_device *rbd_dev)
>>              if head moves */
>>           follow_seq = 1;
>>
>> -    kfree(rbd_dev->header.snapc);
>> -    kfree(rbd_dev->header.snap_names);
>> +    kfree(rbd_dev->header.object_prefix);
> 
> Object prefix isn't going to change, so it doesn't need to be reset
> here. Otherwise looks good.


In that case I will verify that and warn if they do differ.	-Alex

Re: [PATCH 04/16] libceph: define ceph_extract_encoded_string()

[Yehuda Sadeh]

On Wed, Jul 11, 2012 at 12:14 PM, Alex Elder <elder@inktank.com> wrote:
> On 07/11/2012 12:20 PM, Yehuda Sadeh wrote:
>> On Wed, Jul 11, 2012 at 7:01 AM, Alex Elder <elder@inktank.com> wrote:
>>> This adds a new utility routine which will return a dynamically-
>>> allocated buffer containing a string that has been decoded from ceph
>>> over-the-wire format.  It also returns the length of the string
>>> if the address of a size variable is supplied to receive it.
>>>
>>> For now, no gfp_flags parameter is defined (GFP_KERNEL is used) but
>>> it could be easily be added if needed.
>>>
>>
>> I'd rather have it upfront, will help avoiding future errors.
>
> I actually wanted to do exactly that before I sent it but I
> forgot.  I guess I got all caught up in the excitement.
>
>>> Signed-off-by: Alex Elder <elder@inktank.com>
>>> ---
>>>  include/linux/ceph/decode.h |   29 +++++++++++++++++++++++++++++
>>>  1 files changed, 29 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
>>> index 7ead11fc..7759164 100644
>>> --- a/include/linux/ceph/decode.h
>>> +++ b/include/linux/ceph/decode.h
>>> @@ -80,6 +80,35 @@ static inline size_t ceph_decode_string(void **p,
>>> char *s, size_t size)
>>>  }
>>>
>>>  /*
>>> + * Allocate a buffer big enough to hold the wire-encoded string, and
>>> + * decode the string into it.  The resulting string will always be
>>> + * terminated with '\0'.  If successful, *p will be advanced
>>> + * past the decoded data.  Also, if lenp is not a null pointer, the
>>> + * length (not including the terminating '\0') will be recorded in
>>> + * it.  Note that a zero-length string is a valid return value.
>>> + *
>>> + * Returns a pointer to the newly-allocated string buffer, or a
>>> + * null pointer if memory could not be allocated for the result.
>>> + * Neither of the arguments is updated if NULL is returned.
>>> + */
>>> +static inline char *ceph_extract_encoded_string(void **p, size_t *lenp)
>>> +{
>>> +       size_t len;
>>> +       char *buf;
>>> +
>>> +       len = ceph_decode_string(p, NULL, 0);
>>> +       buf = kmalloc(len + 1, GFP_KERNEL);
>>> +       if (!buf)
>>> +               return NULL;
>>> +
>>> +       (void) ceph_decode_string(p, buf, len + 1);
>>> +       if (lenp)
>>> +               *lenp = len;
>>> +
>>> +       return buf;
>>> +}
>>> +
>>> +/*
>>
>> We don't make an effort here to check whether encoded string buffer is
>> valid. While we may be checking it somewhere up the stack, this seem
>> like a generic enough function that could be naively used. Either make
>> it clear that it's an internal function, or make it check p bounds.
>
> Are you saying I should have the caller provide the length of the
> buffer, and ensure we don't exceed it?

Yeah. There are several examples of us doing it (look at ceph_decode_need).

>
> Should I assume that applies to the previous patch also?

Right. Good call.


Yehuda

Re: [PATCH 08/16] rbd: don't store pool name in struct rbd_dev

[Josh Durgin]

On 07/11/2012 07:02 AM, Alex Elder wrote:
> An rbd device's pool name is used to specify the pool to use for an
> rbd image when it gets mapped (via rbd_add()).  However, only the
> pool id can be relied on to be invariant--the name of a pool can
> change at any time.
>
> This means that the name of the pool is potentially stale as soon as
> the image is mapped, so it's a bad idea to record this information.
> So only store the pool id, not its name, in an rbd_dev.
>
> Here are a few specific notes about this change:
>      - The "pool" name device attribute (/sys/bus/rbd/devices/<N>/pool)
>        goes away.  In its place is a "pool_id" attribute that provide
>        the numeric pool id for the rbd image.

We're using the pool name for udev to provide a predictable device name
(/dev/rbd/poolname/imagename[@snapname]), so we probably want to keep
the sysfs attribute. I don't think there's a good way for us to detect
pool renames right now though, so we should document that the pool
name reported does not reflect any potential renames.

>      - rbd_add_parse_args() now returns a pointer to a dynamically-
>        allocated copy of the pool name taken from the arguments.
>      - rbd_add() has been changed to handle freeing the pool name,
>        both in error cases and in the normal case after the pool id
>        has been recorded.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c |   74
> +++++++++++++++++++++++++++++++-------------------
>   1 files changed, 46 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 3aa0ca0..76e978c 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -56,7 +56,6 @@
>   #define RBD_MINORS_PER_MAJOR	256		/* max minors per blkdev */
>
>   #define RBD_MAX_MD_NAME_LEN	(RBD_MAX_OBJ_NAME_LEN + sizeof(RBD_SUFFIX))
> -#define RBD_MAX_POOL_NAME_LEN	64
>   #define RBD_MAX_SNAP_NAME_LEN	32
>   #define RBD_MAX_OPT_LEN		1024
>
> @@ -166,8 +165,7 @@ struct rbd_device {
>   	char			obj[RBD_MAX_OBJ_NAME_LEN]; /* rbd image name */
>   	int			obj_len;
>   	char			obj_md_name[RBD_MAX_MD_NAME_LEN]; /* hdr nm. */
> -	char			pool_name[RBD_MAX_POOL_NAME_LEN];
> -	int			poolid;
> +	int			pool_id;
>
>   	struct ceph_osd_event   *watch_event;
>   	struct ceph_osd_request *watch_request;
> @@ -930,7 +928,7 @@ static int rbd_do_request(struct request *rq,
>   	layout->fl_stripe_unit = cpu_to_le32(1<<  RBD_MAX_OBJ_ORDER);
>   	layout->fl_stripe_count = cpu_to_le32(1);
>   	layout->fl_object_size = cpu_to_le32(1<<  RBD_MAX_OBJ_ORDER);
> -	layout->fl_pg_pool = cpu_to_le32(dev->poolid);
> +	layout->fl_pg_pool = cpu_to_le32(dev->pool_id);
>   	ceph_calc_raw_layout(osdc, layout, snapid, ofs,&len,&bno,
>   				req, ops);
>
> @@ -1653,7 +1651,7 @@ static int rbd_header_add_snap(struct rbd_device *dev,
>   		return -EINVAL;
>
>   	monc =&dev->rbd_client->client->monc;
> -	ret = ceph_monc_create_snapid(monc, dev->poolid,&new_snapid);
> +	ret = ceph_monc_create_snapid(monc, dev->pool_id,&new_snapid);
>   	dout("created snapid=%lld\n", new_snapid);
>   	if (ret<  0)
>   		return ret;
> @@ -1851,12 +1849,12 @@ static ssize_t rbd_client_id_show(struct device
> *dev,
>   			ceph_client_id(rbd_dev->rbd_client->client));
>   }
>
> -static ssize_t rbd_pool_show(struct device *dev,
> +static ssize_t rbd_pool_id_show(struct device *dev,
>   			     struct device_attribute *attr, char *buf)
>   {
>   	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>
> -	return sprintf(buf, "%s\n", rbd_dev->pool_name);
> +	return sprintf(buf, "%d\n", rbd_dev->pool_id);
>   }
>
>   static ssize_t rbd_name_show(struct device *dev,
> @@ -1898,7 +1896,7 @@ static ssize_t rbd_image_refresh(struct device *dev,
>   static DEVICE_ATTR(size, S_IRUGO, rbd_size_show, NULL);
>   static DEVICE_ATTR(major, S_IRUGO, rbd_major_show, NULL);
>   static DEVICE_ATTR(client_id, S_IRUGO, rbd_client_id_show, NULL);
> -static DEVICE_ATTR(pool, S_IRUGO, rbd_pool_show, NULL);
> +static DEVICE_ATTR(pool_id, S_IRUGO, rbd_pool_id_show, NULL);
>   static DEVICE_ATTR(name, S_IRUGO, rbd_name_show, NULL);
>   static DEVICE_ATTR(refresh, S_IWUSR, NULL, rbd_image_refresh);
>   static DEVICE_ATTR(current_snap, S_IRUGO, rbd_snap_show, NULL);
> @@ -1908,7 +1906,7 @@ static struct attribute *rbd_attrs[] = {
>   	&dev_attr_size.attr,
>   	&dev_attr_major.attr,
>   	&dev_attr_client_id.attr,
> -	&dev_attr_pool.attr,
> +	&dev_attr_pool_id.attr,
>   	&dev_attr_name.attr,
>   	&dev_attr_current_snap.attr,
>   	&dev_attr_refresh.attr,
> @@ -2329,25 +2327,31 @@ static inline char *dup_token(const char **buf,
> size_t *lenp)
>   }
>
>   /*
> - * This fills in the pool_name, obj, obj_len, snap_name, obj_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.
> + * This fills in the obj, obj_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.
> + *
> + * Returns a pointer to a dynamically-allocated buffer containing
> + * the pool name provided, or a pointer-coded errno if an error
> + * occurs.
>    */
> -static int rbd_add_parse_args(struct rbd_device *rbd_dev,
> +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;
> +	size_t len;
> +	int ret = -EINVAL;
> +	char *pool_name = NULL;
>
>   	/* The first four tokens are required */
>
>   	len = next_token(&buf);
>   	if (!len)
> -		return -EINVAL;
> +		goto out_err;
> +
>   	*mon_addrs_size = len + 1;
>   	*mon_addrs = buf;
>
> @@ -2355,15 +2359,17 @@ 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;
> +		goto out_err;
>
> -	len = copy_token(&buf, rbd_dev->pool_name, sizeof (rbd_dev->pool_name));
> -	if (!len || len>= sizeof (rbd_dev->pool_name))
> -		return -EINVAL;
> +	pool_name = dup_token(&buf, NULL);
> +	if (!pool_name) {
> +		ret = -ENOMEM;
> +		goto out_err;
> +	}
>
>   	len = copy_token(&buf, rbd_dev->obj, sizeof (rbd_dev->obj));
>   	if (!len || len>= sizeof (rbd_dev->obj))
> -		return -EINVAL;
> +		goto out_err;
>
>   	/* We have the object length in hand, save it. */
>
> @@ -2382,9 +2388,14 @@ static int rbd_add_parse_args(struct rbd_device
> *rbd_dev,
>   		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>   			sizeof (RBD_SNAP_HEAD_NAME));
>   	else if (len>= sizeof (rbd_dev->snap_name))
> -		return -EINVAL;
> +		goto out_err;
>
> -	return 0;
> +	return pool_name;
> +
> +out_err:
> +	kfree(pool_name);
> +
> +	return ERR_PTR(ret);
>   }
>
>   static ssize_t rbd_add(struct bus_type *bus,
> @@ -2396,6 +2407,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	size_t mon_addrs_size = 0;
>   	char *options = NULL;
>   	struct ceph_osd_client *osdc;
> +	char *pool_name;
>   	int rc = -ENOMEM;
>
>   	if (!try_module_get(THIS_MODULE))
> @@ -2425,10 +2437,14 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->id);
>
>   	/* parse add command */
> -	rc = rbd_add_parse_args(rbd_dev, buf,&mon_addrs,&mon_addrs_size,
> -				options, count);
> -	if (rc)
> +	pool_name = rbd_add_parse_args(rbd_dev, buf,
> +					&mon_addrs,&mon_addrs_size,
> +					options, count);
> +	if (IS_ERR(pool_name)) {
> +		rc = PTR_ERR(pool_name);
> +		pool_name = NULL;
>   		goto err_put_id;
> +	}
>
>   	rbd_dev->rbd_client = rbd_get_client(mon_addrs, mon_addrs_size - 1,
>   						options);
> @@ -2439,10 +2455,10 @@ static ssize_t rbd_add(struct bus_type *bus,
>
>   	/* pick the pool */
>   	osdc =&rbd_dev->rbd_client->client->osdc;
> -	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name);
> +	rc = ceph_pg_poolid_by_name(osdc->osdmap, pool_name);
>   	if (rc<  0)
>   		goto err_out_client;
> -	rbd_dev->poolid = rc;
> +	rbd_dev->pool_id = rc;
>
>   	/* register our block device */
>   	rc = register_blkdev(0, rbd_dev->name);
> @@ -2453,6 +2469,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	rc = rbd_bus_add_dev(rbd_dev);
>   	if (rc)
>   		goto err_out_blkdev;
> +	kfree(pool_name);
>
>   	/*
>   	 * At this point cleanup in the event of an error is the job
> @@ -2482,6 +2499,7 @@ err_out_blkdev:
>   err_out_client:
>   	rbd_put_client(rbd_dev);
>   err_put_id:
> +	kfree(pool_name);
>   	rbd_id_put(rbd_dev);
>   err_nomem:
>   	kfree(options);

Re: [PATCH 08/16] rbd: don't store pool name in struct rbd_dev

[Sage Weil]

On Wed, 11 Jul 2012, Josh Durgin wrote:
> On 07/11/2012 07:02 AM, Alex Elder wrote:
> > An rbd device's pool name is used to specify the pool to use for an
> > rbd image when it gets mapped (via rbd_add()).  However, only the
> > pool id can be relied on to be invariant--the name of a pool can
> > change at any time.
> > 
> > This means that the name of the pool is potentially stale as soon as
> > the image is mapped, so it's a bad idea to record this information.
> > So only store the pool id, not its name, in an rbd_dev.
> > 
> > Here are a few specific notes about this change:
> >      - The "pool" name device attribute (/sys/bus/rbd/devices/<N>/pool)
> >        goes away.  In its place is a "pool_id" attribute that provide
> >        the numeric pool id for the rbd image.
> 
> We're using the pool name for udev to provide a predictable device name
> (/dev/rbd/poolname/imagename[@snapname]), so we probably want to keep
> the sysfs attribute. I don't think there's a good way for us to detect
> pool renames right now though, so we should document that the pool
> name reported does not reflect any potential renames.

Whoops.  Let's just make sure that we don't use it for anything other than 
sysfs.

> 
> >      - rbd_add_parse_args() now returns a pointer to a dynamically-
> >        allocated copy of the pool name taken from the arguments.
> >      - rbd_add() has been changed to handle freeing the pool name,
> >        both in error cases and in the normal case after the pool id
> >        has been recorded.
> > 
> > Signed-off-by: Alex Elder<elder@inktank.com>
> > ---
> >   drivers/block/rbd.c |   74
> > +++++++++++++++++++++++++++++++-------------------
> >   1 files changed, 46 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> > index 3aa0ca0..76e978c 100644
> > --- a/drivers/block/rbd.c
> > +++ b/drivers/block/rbd.c
> > @@ -56,7 +56,6 @@
> >   #define RBD_MINORS_PER_MAJOR	256		/* max minors per
> > blkdev */
> > 
> >   #define RBD_MAX_MD_NAME_LEN	(RBD_MAX_OBJ_NAME_LEN + sizeof(RBD_SUFFIX))
> > -#define RBD_MAX_POOL_NAME_LEN	64
> >   #define RBD_MAX_SNAP_NAME_LEN	32
> >   #define RBD_MAX_OPT_LEN		1024
> > 
> > @@ -166,8 +165,7 @@ struct rbd_device {
> >   	char			obj[RBD_MAX_OBJ_NAME_LEN]; /* rbd image name
> > */
> >   	int			obj_len;
> >   	char			obj_md_name[RBD_MAX_MD_NAME_LEN]; /* hdr nm.
> > */
> > -	char			pool_name[RBD_MAX_POOL_NAME_LEN];
> > -	int			poolid;
> > +	int			pool_id;
> > 
> >   	struct ceph_osd_event   *watch_event;
> >   	struct ceph_osd_request *watch_request;
> > @@ -930,7 +928,7 @@ static int rbd_do_request(struct request *rq,
> >   	layout->fl_stripe_unit = cpu_to_le32(1<<  RBD_MAX_OBJ_ORDER);
> >   	layout->fl_stripe_count = cpu_to_le32(1);
> >   	layout->fl_object_size = cpu_to_le32(1<<  RBD_MAX_OBJ_ORDER);
> > -	layout->fl_pg_pool = cpu_to_le32(dev->poolid);
> > +	layout->fl_pg_pool = cpu_to_le32(dev->pool_id);
> >   	ceph_calc_raw_layout(osdc, layout, snapid, ofs,&len,&bno,
> >   				req, ops);
> > 
> > @@ -1653,7 +1651,7 @@ static int rbd_header_add_snap(struct rbd_device *dev,
> >   		return -EINVAL;
> > 
> >   	monc =&dev->rbd_client->client->monc;
> > -	ret = ceph_monc_create_snapid(monc, dev->poolid,&new_snapid);
> > +	ret = ceph_monc_create_snapid(monc, dev->pool_id,&new_snapid);
> >   	dout("created snapid=%lld\n", new_snapid);
> >   	if (ret<  0)
> >   		return ret;
> > @@ -1851,12 +1849,12 @@ static ssize_t rbd_client_id_show(struct device
> > *dev,
> >   			ceph_client_id(rbd_dev->rbd_client->client));
> >   }
> > 
> > -static ssize_t rbd_pool_show(struct device *dev,
> > +static ssize_t rbd_pool_id_show(struct device *dev,
> >   			     struct device_attribute *attr, char *buf)
> >   {
> >   	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
> > 
> > -	return sprintf(buf, "%s\n", rbd_dev->pool_name);
> > +	return sprintf(buf, "%d\n", rbd_dev->pool_id);
> >   }
> > 
> >   static ssize_t rbd_name_show(struct device *dev,
> > @@ -1898,7 +1896,7 @@ static ssize_t rbd_image_refresh(struct device *dev,
> >   static DEVICE_ATTR(size, S_IRUGO, rbd_size_show, NULL);
> >   static DEVICE_ATTR(major, S_IRUGO, rbd_major_show, NULL);
> >   static DEVICE_ATTR(client_id, S_IRUGO, rbd_client_id_show, NULL);
> > -static DEVICE_ATTR(pool, S_IRUGO, rbd_pool_show, NULL);
> > +static DEVICE_ATTR(pool_id, S_IRUGO, rbd_pool_id_show, NULL);
> >   static DEVICE_ATTR(name, S_IRUGO, rbd_name_show, NULL);
> >   static DEVICE_ATTR(refresh, S_IWUSR, NULL, rbd_image_refresh);
> >   static DEVICE_ATTR(current_snap, S_IRUGO, rbd_snap_show, NULL);
> > @@ -1908,7 +1906,7 @@ static struct attribute *rbd_attrs[] = {
> >   	&dev_attr_size.attr,
> >   	&dev_attr_major.attr,
> >   	&dev_attr_client_id.attr,
> > -	&dev_attr_pool.attr,
> > +	&dev_attr_pool_id.attr,
> >   	&dev_attr_name.attr,
> >   	&dev_attr_current_snap.attr,
> >   	&dev_attr_refresh.attr,
> > @@ -2329,25 +2327,31 @@ static inline char *dup_token(const char **buf,
> > size_t *lenp)
> >   }
> > 
> >   /*
> > - * This fills in the pool_name, obj, obj_len, snap_name, obj_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.
> > + * This fills in the obj, obj_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.
> > + *
> > + * Returns a pointer to a dynamically-allocated buffer containing
> > + * the pool name provided, or a pointer-coded errno if an error
> > + * occurs.
> >    */
> > -static int rbd_add_parse_args(struct rbd_device *rbd_dev,
> > +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;
> > +	size_t len;
> > +	int ret = -EINVAL;
> > +	char *pool_name = NULL;
> > 
> >   	/* The first four tokens are required */
> > 
> >   	len = next_token(&buf);
> >   	if (!len)
> > -		return -EINVAL;
> > +		goto out_err;
> > +
> >   	*mon_addrs_size = len + 1;
> >   	*mon_addrs = buf;
> > 
> > @@ -2355,15 +2359,17 @@ 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;
> > +		goto out_err;
> > 
> > -	len = copy_token(&buf, rbd_dev->pool_name, sizeof
> > (rbd_dev->pool_name));
> > -	if (!len || len>= sizeof (rbd_dev->pool_name))
> > -		return -EINVAL;
> > +	pool_name = dup_token(&buf, NULL);
> > +	if (!pool_name) {
> > +		ret = -ENOMEM;
> > +		goto out_err;
> > +	}
> > 
> >   	len = copy_token(&buf, rbd_dev->obj, sizeof (rbd_dev->obj));
> >   	if (!len || len>= sizeof (rbd_dev->obj))
> > -		return -EINVAL;
> > +		goto out_err;
> > 
> >   	/* We have the object length in hand, save it. */
> > 
> > @@ -2382,9 +2388,14 @@ static int rbd_add_parse_args(struct rbd_device
> > *rbd_dev,
> >   		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
> >   			sizeof (RBD_SNAP_HEAD_NAME));
> >   	else if (len>= sizeof (rbd_dev->snap_name))
> > -		return -EINVAL;
> > +		goto out_err;
> > 
> > -	return 0;
> > +	return pool_name;
> > +
> > +out_err:
> > +	kfree(pool_name);
> > +
> > +	return ERR_PTR(ret);
> >   }
> > 
> >   static ssize_t rbd_add(struct bus_type *bus,
> > @@ -2396,6 +2407,7 @@ static ssize_t rbd_add(struct bus_type *bus,
> >   	size_t mon_addrs_size = 0;
> >   	char *options = NULL;
> >   	struct ceph_osd_client *osdc;
> > +	char *pool_name;
> >   	int rc = -ENOMEM;
> > 
> >   	if (!try_module_get(THIS_MODULE))
> > @@ -2425,10 +2437,14 @@ static ssize_t rbd_add(struct bus_type *bus,
> >   	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->id);
> > 
> >   	/* parse add command */
> > -	rc = rbd_add_parse_args(rbd_dev, buf,&mon_addrs,&mon_addrs_size,
> > -				options, count);
> > -	if (rc)
> > +	pool_name = rbd_add_parse_args(rbd_dev, buf,
> > +					&mon_addrs,&mon_addrs_size,
> > +					options, count);
> > +	if (IS_ERR(pool_name)) {
> > +		rc = PTR_ERR(pool_name);
> > +		pool_name = NULL;
> >   		goto err_put_id;
> > +	}
> > 
> >   	rbd_dev->rbd_client = rbd_get_client(mon_addrs, mon_addrs_size - 1,
> >   						options);
> > @@ -2439,10 +2455,10 @@ static ssize_t rbd_add(struct bus_type *bus,
> > 
> >   	/* pick the pool */
> >   	osdc =&rbd_dev->rbd_client->client->osdc;
> > -	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name);
> > +	rc = ceph_pg_poolid_by_name(osdc->osdmap, pool_name);
> >   	if (rc<  0)
> >   		goto err_out_client;
> > -	rbd_dev->poolid = rc;
> > +	rbd_dev->pool_id = rc;
> > 
> >   	/* register our block device */
> >   	rc = register_blkdev(0, rbd_dev->name);
> > @@ -2453,6 +2469,7 @@ static ssize_t rbd_add(struct bus_type *bus,
> >   	rc = rbd_bus_add_dev(rbd_dev);
> >   	if (rc)
> >   		goto err_out_blkdev;
> > +	kfree(pool_name);
> > 
> >   	/*
> >   	 * At this point cleanup in the event of an error is the job
> > @@ -2482,6 +2499,7 @@ err_out_blkdev:
> >   err_out_client:
> >   	rbd_put_client(rbd_dev);
> >   err_put_id:
> > +	kfree(pool_name);
> >   	rbd_id_put(rbd_dev);
> >   err_nomem:
> >   	kfree(options);
> 
> --
> 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 09/16] rbd: dynamically allocate image header name

[Josh Durgin]

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

On 07/11/2012 07:02 AM, Alex Elder wrote:
> There is no need to impose a small limit the length of the header
> name recorded for an rbd image in a struct rbd_dev.  Remove the
> limitation by allocating space for the header name dynamically.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c |   33 +++++++++++++++++++++------------
>   1 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 76e978c..4d11337 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -55,7 +55,6 @@
>
>   #define RBD_MINORS_PER_MAJOR	256		/* max minors per blkdev */
>
> -#define RBD_MAX_MD_NAME_LEN	(RBD_MAX_OBJ_NAME_LEN + sizeof(RBD_SUFFIX))
>   #define RBD_MAX_SNAP_NAME_LEN	32
>   #define RBD_MAX_OPT_LEN		1024
>
> @@ -164,7 +163,7 @@ struct rbd_device {
>   	struct rbd_image_header	header;
>   	char			obj[RBD_MAX_OBJ_NAME_LEN]; /* rbd image name */
>   	int			obj_len;
> -	char			obj_md_name[RBD_MAX_MD_NAME_LEN]; /* hdr nm. */
> +	char			*obj_md_name; /* hdr nm. */
>   	int			pool_id;
>
>   	struct ceph_osd_event   *watch_event;
> @@ -2375,8 +2374,13 @@ static char *rbd_add_parse_args(struct rbd_device
> *rbd_dev,
>
>   	rbd_dev->obj_len = len;
>
> -	BUILD_BUG_ON(RBD_MAX_MD_NAME_LEN
> -				<  RBD_MAX_OBJ_NAME_LEN + sizeof (RBD_SUFFIX));
> +	/* Create the name of the header object */
> +
> +	rbd_dev->obj_md_name = kmalloc(len + sizeof (RBD_SUFFIX), GFP_KERNEL);
> +	if (!rbd_dev->obj_md_name) {
> +		ret = -ENOMEM;
> +		goto out_err;
> +	}
>   	sprintf(rbd_dev->obj_md_name, "%s%s", rbd_dev->obj, RBD_SUFFIX);
>
>   	/*
> @@ -2393,6 +2397,7 @@ static char *rbd_add_parse_args(struct rbd_device
> *rbd_dev,
>   	return pool_name;
>
>   out_err:
> +	kfree(rbd_dev->obj_md_name);
>   	kfree(pool_name);
>
>   	return ERR_PTR(ret);
> @@ -2402,23 +2407,23 @@ static ssize_t rbd_add(struct bus_type *bus,
>   		       const char *buf,
>   		       size_t count)
>   {
> -	struct rbd_device *rbd_dev;
> +	char *options;
> +	struct rbd_device *rbd_dev = NULL;
>   	const char *mon_addrs = NULL;
>   	size_t mon_addrs_size = 0;
> -	char *options = NULL;
> -	struct ceph_osd_client *osdc;
>   	char *pool_name;
> +	struct ceph_osd_client *osdc;
>   	int rc = -ENOMEM;
>
>   	if (!try_module_get(THIS_MODULE))
>   		return -ENODEV;
>
> -	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
> -	if (!rbd_dev)
> -		goto err_nomem;
>   	options = kmalloc(count, GFP_KERNEL);
>   	if (!options)
>   		goto err_nomem;
> +	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
> +	if (!rbd_dev)
> +		goto err_nomem;
>
>   	/* static rbd_device initialization */
>   	spin_lock_init(&rbd_dev->lock);
> @@ -2499,11 +2504,14 @@ err_out_blkdev:
>   err_out_client:
>   	rbd_put_client(rbd_dev);
>   err_put_id:
> -	kfree(pool_name);
> +	if (pool_name) {
> +		kfree(rbd_dev->obj_md_name);
> +		kfree(pool_name);
> +	}
>   	rbd_id_put(rbd_dev);
>   err_nomem:
> -	kfree(options);
>   	kfree(rbd_dev);
> +	kfree(options);
>
>   	dout("Error adding device %s\n", buf);
>   	module_put(THIS_MODULE);
> @@ -2548,6 +2556,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->obj_md_name);
>   	rbd_id_put(rbd_dev);
>   	kfree(rbd_dev);
>

Re: [PATCH 10/16] rbd: dynamically allocate image name

[Josh Durgin]

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

On 07/11/2012 07:02 AM, Alex Elder wrote:
> There is no need to impose a small limit the length of the rbd image
> name recorded in a struct rbd_dev.  Remove the limitation by
> allocating space for the image name dynamically.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c       |   34 ++++++++++++++++++----------------
>   drivers/block/rbd_types.h |    1 -
>   2 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 4d11337..28afff9 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -161,8 +161,8 @@ struct rbd_device {
>   	spinlock_t		lock;		/* queue lock */
>
>   	struct rbd_image_header	header;
> -	char			obj[RBD_MAX_OBJ_NAME_LEN]; /* rbd image name */
> -	int			obj_len;
> +	char			*obj; /* rbd image name */
> +	size_t			obj_len;
>   	char			*obj_md_name; /* hdr nm. */
>   	int			pool_id;
>
> @@ -2333,6 +2333,8 @@ static inline char *dup_token(const char **buf,
> size_t *lenp)
>    * Returns a pointer to a dynamically-allocated buffer containing
>    * the pool name provided, or a pointer-coded errno if an error
>    * occurs.
> + *
> + * Note: rbd_dev is assumed to have been initially zero-filled.
>    */
>   static char *rbd_add_parse_args(struct rbd_device *rbd_dev,
>   			      const char *buf,
> @@ -2360,27 +2362,22 @@ static char *rbd_add_parse_args(struct
> rbd_device *rbd_dev,
>   	if (!len || len>= options_size)
>   		goto out_err;
>
> +	ret = -ENOMEM;
>   	pool_name = dup_token(&buf, NULL);
> -	if (!pool_name) {
> -		ret = -ENOMEM;
> +	if (!pool_name)
>   		goto out_err;
> -	}
>
> -	len = copy_token(&buf, rbd_dev->obj, sizeof (rbd_dev->obj));
> -	if (!len || len>= sizeof (rbd_dev->obj))
> +	rbd_dev->obj = dup_token(&buf,&rbd_dev->obj_len);
> +	if (!rbd_dev->obj)
>   		goto out_err;
>
> -	/* We have the object length in hand, save it. */
> -
> -	rbd_dev->obj_len = len;
> -
>   	/* Create the name of the header object */
>
> -	rbd_dev->obj_md_name = kmalloc(len + sizeof (RBD_SUFFIX), GFP_KERNEL);
> -	if (!rbd_dev->obj_md_name) {
> -		ret = -ENOMEM;
> +	rbd_dev->obj_md_name = kmalloc(rbd_dev->obj_len
> +						+ sizeof (RBD_SUFFIX),
> +					GFP_KERNEL);
> +	if (!rbd_dev->obj_md_name)
>   		goto out_err;
> -	}
>   	sprintf(rbd_dev->obj_md_name, "%s%s", rbd_dev->obj, RBD_SUFFIX);
>
>   	/*
> @@ -2391,13 +2388,16 @@ static char *rbd_add_parse_args(struct
> rbd_device *rbd_dev,
>   	if (!len)
>   		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>   			sizeof (RBD_SNAP_HEAD_NAME));
> -	else if (len>= sizeof (rbd_dev->snap_name))
> +	else if (len>= sizeof (rbd_dev->snap_name)) {
> +		ret = -EINVAL;
>   		goto out_err;
> +	}
>
>   	return pool_name;
>
>   out_err:
>   	kfree(rbd_dev->obj_md_name);
> +	kfree(rbd_dev->obj);
>   	kfree(pool_name);
>
>   	return ERR_PTR(ret);
> @@ -2506,6 +2506,7 @@ err_out_client:
>   err_put_id:
>   	if (pool_name) {
>   		kfree(rbd_dev->obj_md_name);
> +		kfree(rbd_dev->obj);
>   		kfree(pool_name);
>   	}
>   	rbd_id_put(rbd_dev);
> @@ -2557,6 +2558,7 @@ static void rbd_dev_release(struct device *dev)
>
>   	/* done with the id, and with the rbd_dev */
>   	kfree(rbd_dev->obj_md_name);
> +	kfree(rbd_dev->obj);
>   	rbd_id_put(rbd_dev);
>   	kfree(rbd_dev);
>
> diff --git a/drivers/block/rbd_types.h b/drivers/block/rbd_types.h
> index 9507086..0924e9e 100644
> --- a/drivers/block/rbd_types.h
> +++ b/drivers/block/rbd_types.h
> @@ -31,7 +31,6 @@
>   #define RBD_MIN_OBJ_ORDER       16
>   #define RBD_MAX_OBJ_ORDER       30
>
> -#define RBD_MAX_OBJ_NAME_LEN	96
>   #define RBD_MAX_SEG_NAME_LEN	128
>
>   #define RBD_COMP_NONE		0

Re: [PATCH 10/16] rbd: dynamically allocate image name

[Josh Durgin]

On 07/11/2012 01:49 PM, Josh Durgin wrote:
> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
>
> On 07/11/2012 07:02 AM, Alex Elder wrote:
>> There is no need to impose a small limit the length of the rbd image
>> name recorded in a struct rbd_dev. Remove the limitation by
>> allocating space for the image name dynamically.
>>
>> Signed-off-by: Alex Elder<elder@inktank.com>
>> ---
>> drivers/block/rbd.c | 34 ++++++++++++++++++----------------
>> drivers/block/rbd_types.h | 1 -
>> 2 files changed, 18 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 4d11337..28afff9 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -161,8 +161,8 @@ struct rbd_device {
>> spinlock_t lock; /* queue lock */
>>
>> struct rbd_image_header header;
>> - char obj[RBD_MAX_OBJ_NAME_LEN]; /* rbd image name */
>> - int obj_len;
>> + char *obj; /* rbd image name */
>> + size_t obj_len;

On second look, obj_len can be a local variable. It's not used
outside of option parsing.

>> char *obj_md_name; /* hdr nm. */
>> int pool_id;
>>
>> @@ -2333,6 +2333,8 @@ static inline char *dup_token(const char **buf,
>> size_t *lenp)
>> * Returns a pointer to a dynamically-allocated buffer containing
>> * the pool name provided, or a pointer-coded errno if an error
>> * occurs.
>> + *
>> + * Note: rbd_dev is assumed to have been initially zero-filled.
>> */
>> static char *rbd_add_parse_args(struct rbd_device *rbd_dev,
>> const char *buf,
>> @@ -2360,27 +2362,22 @@ static char *rbd_add_parse_args(struct
>> rbd_device *rbd_dev,
>> if (!len || len>= options_size)
>> goto out_err;
>>
>> + ret = -ENOMEM;
>> pool_name = dup_token(&buf, NULL);
>> - if (!pool_name) {
>> - ret = -ENOMEM;
>> + if (!pool_name)
>> goto out_err;
>> - }
>>
>> - len = copy_token(&buf, rbd_dev->obj, sizeof (rbd_dev->obj));
>> - if (!len || len>= sizeof (rbd_dev->obj))
>> + rbd_dev->obj = dup_token(&buf,&rbd_dev->obj_len);
>> + if (!rbd_dev->obj)
>> goto out_err;
>>
>> - /* We have the object length in hand, save it. */
>> -
>> - rbd_dev->obj_len = len;
>> -
>> /* Create the name of the header object */
>>
>> - rbd_dev->obj_md_name = kmalloc(len + sizeof (RBD_SUFFIX), GFP_KERNEL);
>> - if (!rbd_dev->obj_md_name) {
>> - ret = -ENOMEM;
>> + rbd_dev->obj_md_name = kmalloc(rbd_dev->obj_len
>> + + sizeof (RBD_SUFFIX),
>> + GFP_KERNEL);
>> + if (!rbd_dev->obj_md_name)
>> goto out_err;
>> - }
>> sprintf(rbd_dev->obj_md_name, "%s%s", rbd_dev->obj, RBD_SUFFIX);
>>
>> /*
>> @@ -2391,13 +2388,16 @@ static char *rbd_add_parse_args(struct
>> rbd_device *rbd_dev,
>> if (!len)
>> memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>> sizeof (RBD_SNAP_HEAD_NAME));
>> - else if (len>= sizeof (rbd_dev->snap_name))
>> + else if (len>= sizeof (rbd_dev->snap_name)) {
>> + ret = -EINVAL;
>> goto out_err;
>> + }
>>
>> return pool_name;
>>
>> out_err:
>> kfree(rbd_dev->obj_md_name);
>> + kfree(rbd_dev->obj);
>> kfree(pool_name);
>>
>> return ERR_PTR(ret);
>> @@ -2506,6 +2506,7 @@ err_out_client:
>> err_put_id:
>> if (pool_name) {
>> kfree(rbd_dev->obj_md_name);
>> + kfree(rbd_dev->obj);
>> kfree(pool_name);
>> }
>> rbd_id_put(rbd_dev);
>> @@ -2557,6 +2558,7 @@ static void rbd_dev_release(struct device *dev)
>>
>> /* done with the id, and with the rbd_dev */
>> kfree(rbd_dev->obj_md_name);
>> + kfree(rbd_dev->obj);
>> rbd_id_put(rbd_dev);
>> kfree(rbd_dev);
>>
>> diff --git a/drivers/block/rbd_types.h b/drivers/block/rbd_types.h
>> index 9507086..0924e9e 100644
>> --- a/drivers/block/rbd_types.h
>> +++ b/drivers/block/rbd_types.h
>> @@ -31,7 +31,6 @@
>> #define RBD_MIN_OBJ_ORDER 16
>> #define RBD_MAX_OBJ_ORDER 30
>>
>> -#define RBD_MAX_OBJ_NAME_LEN 96
>> #define RBD_MAX_SEG_NAME_LEN 128
>>
>> #define RBD_COMP_NONE 0
>
> --
> 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 11/16] rbd: dynamically allocate snapshot name

[Josh Durgin]

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

On 07/11/2012 07:02 AM, Alex Elder wrote:
> There is no need to impose a small limit the length of the snapshot
> name recorded for an rbd image in a struct rbd_dev.  Remove the
> limitation by allocating space for the snapshot name dynamically.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c |   26 ++++++++++++++++----------
>   1 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 28afff9..90fb388 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -171,7 +171,7 @@ struct rbd_device {
>
>   	/* protects updating the header */
>   	struct rw_semaphore     header_rwsem;
> -	char                    snap_name[RBD_MAX_SNAP_NAME_LEN];
> +	char                    *snap_name;
>   	u64                     snap_id;	/* current snapshot id */
>   	int read_only;
>
> @@ -587,8 +587,6 @@ static int rbd_header_set_snap(struct rbd_device
> *dev, u64 *size)
>   	struct ceph_snap_context *snapc = header->snapc;
>   	int ret = -ENOENT;
>
> -	BUILD_BUG_ON(sizeof (dev->snap_name)<  sizeof (RBD_SNAP_HEAD_NAME));
> -
>   	down_write(&dev->header_rwsem);
>
>   	if (!memcmp(dev->snap_name, RBD_SNAP_HEAD_NAME,
> @@ -2381,16 +2379,22 @@ static char *rbd_add_parse_args(struct
> rbd_device *rbd_dev,
>   	sprintf(rbd_dev->obj_md_name, "%s%s", rbd_dev->obj, RBD_SUFFIX);
>
>   	/*
> -	 * The snapshot name is optional, but it's an error if it's
> -	 * too long.  If no snapshot is supplied, fill in the default.
> +	 * The snapshot name is optional.  If none is is supplied,
> +	 * we use the default value.
>   	 */
> -	len = copy_token(&buf, rbd_dev->snap_name, sizeof (rbd_dev->snap_name));
> -	if (!len)
> +	rbd_dev->snap_name = dup_token(&buf,&len);
> +	if (!rbd_dev->snap_name)
> +		goto out_err;
> +	if (!len) {
> +		/* Replace the empty name with the default */
> +		kfree(rbd_dev->snap_name);
> +		rbd_dev->snap_name
> +			= kmalloc(sizeof (RBD_SNAP_HEAD_NAME), GFP_KERNEL);
> +		if (!rbd_dev->snap_name)
> +			goto out_err;
> +
>   		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>   			sizeof (RBD_SNAP_HEAD_NAME));
> -	else if (len>= sizeof (rbd_dev->snap_name)) {
> -		ret = -EINVAL;
> -		goto out_err;
>   	}
>
>   	return pool_name;
> @@ -2505,6 +2509,7 @@ err_out_client:
>   	rbd_put_client(rbd_dev);
>   err_put_id:
>   	if (pool_name) {
> +		kfree(rbd_dev->snap_name);
>   		kfree(rbd_dev->obj_md_name);
>   		kfree(rbd_dev->obj);
>   		kfree(pool_name);
> @@ -2557,6 +2562,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->obj_md_name);
>   	kfree(rbd_dev->obj);
>   	rbd_id_put(rbd_dev);

Re: [PATCH 12/16] rbd: use rbd_dev consistently

[Josh Durgin]

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

On 07/11/2012 07:02 AM, Alex Elder wrote:
> Most variables that represent a struct rbd_device are named
> "rbd_dev", but in some cases "dev" is used instead.  Change all the
> "dev" references so they use "rbd_dev" consistently, to make it
> clear from the name that we're working with an RBD device (as
> opposed to, for example, a struct device).  Similarly, change the
> name of the "dev" field in struct rbd_notify_info to be "rbd_dev".
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c |  125
> ++++++++++++++++++++++++++-------------------------
>   1 files changed, 64 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 90fb388..c38246b 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -581,35 +581,36 @@ 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 *dev, u64 *size)
> +static int rbd_header_set_snap(struct rbd_device *rbd_dev, u64 *size)
>   {
> -	struct rbd_image_header *header =&dev->header;
> +	struct rbd_image_header *header =&rbd_dev->header;
>   	struct ceph_snap_context *snapc = header->snapc;
>   	int ret = -ENOENT;
>
> -	down_write(&dev->header_rwsem);
> +	down_write(&rbd_dev->header_rwsem);
>
> -	if (!memcmp(dev->snap_name, RBD_SNAP_HEAD_NAME,
> +	if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>   		    sizeof (RBD_SNAP_HEAD_NAME))) {
>   		if (header->total_snaps)
>   			snapc->seq = header->snap_seq;
>   		else
>   			snapc->seq = 0;
> -		dev->snap_id = CEPH_NOSNAP;
> -		dev->read_only = 0;
> +		rbd_dev->snap_id = CEPH_NOSNAP;
> +		rbd_dev->read_only = 0;
>   		if (size)
>   			*size = header->image_size;
>   	} else {
> -		ret = snap_by_name(header, dev->snap_name,&snapc->seq, size);
> +		ret = snap_by_name(header, rbd_dev->snap_name,
> +					&snapc->seq, size);
>   		if (ret<  0)
>   			goto done;
> -		dev->snap_id = snapc->seq;
> -		dev->read_only = 1;
> +		rbd_dev->snap_id = snapc->seq;
> +		rbd_dev->read_only = 1;
>   	}
>
>   	ret = 0;
>   done:
> -	up_write(&dev->header_rwsem);
> +	up_write(&rbd_dev->header_rwsem);
>   	return ret;
>   }
>
> @@ -853,7 +854,7 @@ static void rbd_coll_end_req(struct rbd_request *req,
>    * Send ceph osd request
>    */
>   static int rbd_do_request(struct request *rq,
> -			  struct rbd_device *dev,
> +			  struct rbd_device *rbd_dev,
>   			  struct ceph_snap_context *snapc,
>   			  u64 snapid,
>   			  const char *obj, u64 ofs, u64 len,
> @@ -894,13 +895,13 @@ static int rbd_do_request(struct request *rq,
>
>   	dout("rbd_do_request obj=%s ofs=%lld len=%lld\n", obj, len, ofs);
>
> -	down_read(&dev->header_rwsem);
> +	down_read(&rbd_dev->header_rwsem);
>
> -	osdc =&dev->rbd_client->client->osdc;
> +	osdc =&rbd_dev->rbd_client->client->osdc;
>   	req = ceph_osdc_alloc_request(osdc, flags, snapc, ops,
>   					false, GFP_NOIO, pages, bio);
>   	if (!req) {
> -		up_read(&dev->header_rwsem);
> +		up_read(&rbd_dev->header_rwsem);
>   		ret = -ENOMEM;
>   		goto done_pages;
>   	}
> @@ -925,7 +926,7 @@ static int rbd_do_request(struct request *rq,
>   	layout->fl_stripe_unit = cpu_to_le32(1<<  RBD_MAX_OBJ_ORDER);
>   	layout->fl_stripe_count = cpu_to_le32(1);
>   	layout->fl_object_size = cpu_to_le32(1<<  RBD_MAX_OBJ_ORDER);
> -	layout->fl_pg_pool = cpu_to_le32(dev->pool_id);
> +	layout->fl_pg_pool = cpu_to_le32(rbd_dev->pool_id);
>   	ceph_calc_raw_layout(osdc, layout, snapid, ofs,&len,&bno,
>   				req, ops);
>
> @@ -934,7 +935,7 @@ static int rbd_do_request(struct request *rq,
>   				snapc,
>   				&mtime,
>   				req->r_oid, req->r_oid_len);
> -	up_read(&dev->header_rwsem);
> +	up_read(&rbd_dev->header_rwsem);
>
>   	if (linger_req) {
>   		ceph_osdc_set_request_linger(osdc, req);
> @@ -1011,7 +1012,7 @@ static void rbd_simple_req_cb(struct
> ceph_osd_request *req, struct ceph_msg *msg
>   /*
>    * Do a synchronous ceph osd operation
>    */
> -static int rbd_req_sync_op(struct rbd_device *dev,
> +static int rbd_req_sync_op(struct rbd_device *rbd_dev,
>   			   struct ceph_snap_context *snapc,
>   			   u64 snapid,
>   			   int opcode,
> @@ -1048,7 +1049,7 @@ static int rbd_req_sync_op(struct rbd_device *dev,
>   		}
>   	}
>
> -	ret = rbd_do_request(NULL, dev, snapc, snapid,
> +	ret = rbd_do_request(NULL, rbd_dev, snapc, snapid,
>   			  obj, ofs, len, NULL,
>   			  pages, num_pages,
>   			  flags,
> @@ -1075,7 +1076,7 @@ done:
>    * Do an asynchronous ceph osd operation
>    */
>   static int rbd_do_op(struct request *rq,
> -		     struct rbd_device *rbd_dev ,
> +		     struct rbd_device *rbd_dev,
>   		     struct ceph_snap_context *snapc,
>   		     u64 snapid,
>   		     int opcode, int flags, int num_reply,
> @@ -1167,7 +1168,7 @@ static int rbd_req_read(struct request *rq,
>   /*
>    * Request sync osd read
>    */
> -static int rbd_req_sync_read(struct rbd_device *dev,
> +static int rbd_req_sync_read(struct rbd_device *rbd_dev,
>   			  struct ceph_snap_context *snapc,
>   			  u64 snapid,
>   			  const char *obj,
> @@ -1175,7 +1176,7 @@ static int rbd_req_sync_read(struct rbd_device *dev,
>   			  char *buf,
>   			  u64 *ver)
>   {
> -	return rbd_req_sync_op(dev, NULL,
> +	return rbd_req_sync_op(rbd_dev, NULL,
>   			       snapid,
>   			       CEPH_OSD_OP_READ,
>   			       CEPH_OSD_FLAG_READ,
> @@ -1186,7 +1187,7 @@ static int rbd_req_sync_read(struct rbd_device *dev,
>   /*
>    * Request sync osd watch
>    */
> -static int rbd_req_sync_notify_ack(struct rbd_device *dev,
> +static int rbd_req_sync_notify_ack(struct rbd_device *rbd_dev,
>   				   u64 ver,
>   				   u64 notify_id,
>   				   const char *obj)
> @@ -1198,11 +1199,11 @@ static int rbd_req_sync_notify_ack(struct
> rbd_device *dev,
>   	if (ret<  0)
>   		return ret;
>
> -	ops[0].watch.ver = cpu_to_le64(dev->header.obj_version);
> +	ops[0].watch.ver = cpu_to_le64(rbd_dev->header.obj_version);
>   	ops[0].watch.cookie = notify_id;
>   	ops[0].watch.flag = 0;
>
> -	ret = rbd_do_request(NULL, dev, NULL, CEPH_NOSNAP,
> +	ret = rbd_do_request(NULL, rbd_dev, NULL, CEPH_NOSNAP,
>   			  obj, 0, 0, NULL,
>   			  NULL, 0,
>   			  CEPH_OSD_FLAG_READ,
> @@ -1217,54 +1218,54 @@ static int rbd_req_sync_notify_ack(struct
> rbd_device *dev,
>
>   static void rbd_watch_cb(u64 ver, u64 notify_id, u8 opcode, void *data)
>   {
> -	struct rbd_device *dev = (struct rbd_device *)data;
> +	struct rbd_device *rbd_dev = (struct rbd_device *)data;
>   	int rc;
>
> -	if (!dev)
> +	if (!rbd_dev)
>   		return;
>
> -	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n", dev->obj_md_name,
> +	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n", rbd_dev->obj_md_name,
>   		notify_id, (int)opcode);
>   	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> -	rc = __rbd_refresh_header(dev);
> +	rc = __rbd_refresh_header(rbd_dev);
>   	mutex_unlock(&ctl_mutex);
>   	if (rc)
>   		pr_warning(RBD_DRV_NAME "%d got notification but failed to "
> -			   " update snaps: %d\n", dev->major, rc);
> +			   " update snaps: %d\n", rbd_dev->major, rc);
>
> -	rbd_req_sync_notify_ack(dev, ver, notify_id, dev->obj_md_name);
> +	rbd_req_sync_notify_ack(rbd_dev, ver, notify_id, rbd_dev->obj_md_name);
>   }
>
>   /*
>    * Request sync osd watch
>    */
> -static int rbd_req_sync_watch(struct rbd_device *dev,
> +static int rbd_req_sync_watch(struct rbd_device *rbd_dev,
>   			      const char *obj,
>   			      u64 ver)
>   {
>   	struct ceph_osd_req_op *ops;
> -	struct ceph_osd_client *osdc =&dev->rbd_client->client->osdc;
> +	struct ceph_osd_client *osdc =&rbd_dev->rbd_client->client->osdc;
>
>   	int ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_WATCH, 0);
>   	if (ret<  0)
>   		return ret;
>
>   	ret = ceph_osdc_create_event(osdc, rbd_watch_cb, 0,
> -				     (void *)dev,&dev->watch_event);
> +				     (void *)rbd_dev,&rbd_dev->watch_event);
>   	if (ret<  0)
>   		goto fail;
>
>   	ops[0].watch.ver = cpu_to_le64(ver);
> -	ops[0].watch.cookie = cpu_to_le64(dev->watch_event->cookie);
> +	ops[0].watch.cookie = cpu_to_le64(rbd_dev->watch_event->cookie);
>   	ops[0].watch.flag = 1;
>
> -	ret = rbd_req_sync_op(dev, NULL,
> +	ret = rbd_req_sync_op(rbd_dev, NULL,
>   			      CEPH_NOSNAP,
>   			      0,
>   			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
>   			      ops,
>   			      1, obj, 0, 0, NULL,
> -			&dev->watch_request, NULL);
> +			&rbd_dev->watch_request, NULL);
>
>   	if (ret<  0)
>   		goto fail_event;
> @@ -1273,8 +1274,8 @@ static int rbd_req_sync_watch(struct rbd_device *dev,
>   	return 0;
>
>   fail_event:
> -	ceph_osdc_cancel_event(dev->watch_event);
> -	dev->watch_event = NULL;
> +	ceph_osdc_cancel_event(rbd_dev->watch_event);
> +	rbd_dev->watch_event = NULL;
>   fail:
>   	rbd_destroy_ops(ops);
>   	return ret;
> @@ -1283,7 +1284,7 @@ fail:
>   /*
>    * Request sync osd unwatch
>    */
> -static int rbd_req_sync_unwatch(struct rbd_device *dev,
> +static int rbd_req_sync_unwatch(struct rbd_device *rbd_dev,
>   				const char *obj)
>   {
>   	struct ceph_osd_req_op *ops;
> @@ -1293,10 +1294,10 @@ static int rbd_req_sync_unwatch(struct
> rbd_device *dev,
>   		return ret;
>
>   	ops[0].watch.ver = 0;
> -	ops[0].watch.cookie = cpu_to_le64(dev->watch_event->cookie);
> +	ops[0].watch.cookie = cpu_to_le64(rbd_dev->watch_event->cookie);
>   	ops[0].watch.flag = 0;
>
> -	ret = rbd_req_sync_op(dev, NULL,
> +	ret = rbd_req_sync_op(rbd_dev, NULL,
>   			      CEPH_NOSNAP,
>   			      0,
>   			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
> @@ -1304,33 +1305,34 @@ static int rbd_req_sync_unwatch(struct
> rbd_device *dev,
>   			      1, obj, 0, 0, NULL, NULL, NULL);
>
>   	rbd_destroy_ops(ops);
> -	ceph_osdc_cancel_event(dev->watch_event);
> -	dev->watch_event = NULL;
> +	ceph_osdc_cancel_event(rbd_dev->watch_event);
> +	rbd_dev->watch_event = NULL;
>   	return ret;
>   }
>
>   struct rbd_notify_info {
> -	struct rbd_device *dev;
> +	struct rbd_device *rbd_dev;
>   };
>
>   static void rbd_notify_cb(u64 ver, u64 notify_id, u8 opcode, void *data)
>   {
> -	struct rbd_device *dev = (struct rbd_device *)data;
> -	if (!dev)
> +	struct rbd_device *rbd_dev = (struct rbd_device *)data;
> +	if (!rbd_dev)
>   		return;
>
> -	dout("rbd_notify_cb %s notify_id=%lld opcode=%d\n", dev->obj_md_name,
> +	dout("rbd_notify_cb %s notify_id=%lld opcode=%d\n",
> +				rbd_dev->obj_md_name,
>   		notify_id, (int)opcode);
>   }
>
>   /*
>    * Request sync osd notify
>    */
> -static int rbd_req_sync_notify(struct rbd_device *dev,
> +static int rbd_req_sync_notify(struct rbd_device *rbd_dev,
>   		          const char *obj)
>   {
>   	struct ceph_osd_req_op *ops;
> -	struct ceph_osd_client *osdc =&dev->rbd_client->client->osdc;
> +	struct ceph_osd_client *osdc =&rbd_dev->rbd_client->client->osdc;
>   	struct ceph_osd_event *event;
>   	struct rbd_notify_info info;
>   	int payload_len = sizeof(u32) + sizeof(u32);
> @@ -1340,7 +1342,7 @@ static int rbd_req_sync_notify(struct rbd_device *dev,
>   	if (ret<  0)
>   		return ret;
>
> -	info.dev = dev;
> +	info.rbd_dev = rbd_dev;
>
>   	ret = ceph_osdc_create_event(osdc, rbd_notify_cb, 1,
>   				     (void *)&info,&event);
> @@ -1353,7 +1355,7 @@ static int rbd_req_sync_notify(struct rbd_device *dev,
>   	ops[0].watch.prot_ver = RADOS_NOTIFY_VER;
>   	ops[0].watch.timeout = 12;
>
> -	ret = rbd_req_sync_op(dev, NULL,
> +	ret = rbd_req_sync_op(rbd_dev, NULL,
>   			       CEPH_NOSNAP,
>   			       0,
>   			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
> @@ -1377,7 +1379,7 @@ fail:
>   /*
>    * Request sync osd read
>    */
> -static int rbd_req_sync_exec(struct rbd_device *dev,
> +static int rbd_req_sync_exec(struct rbd_device *rbd_dev,
>   			     const char *obj,
>   			     const char *cls,
>   			     const char *method,
> @@ -1401,7 +1403,7 @@ static int rbd_req_sync_exec(struct rbd_device *dev,
>   	ops[0].cls.indata = data;
>   	ops[0].cls.indata_len = len;
>
> -	ret = rbd_req_sync_op(dev, NULL,
> +	ret = rbd_req_sync_op(rbd_dev, NULL,
>   			       CEPH_NOSNAP,
>   			       0,
>   			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
> @@ -1632,7 +1634,7 @@ out_dh:
>   /*
>    * create a snapshot
>    */
> -static int rbd_header_add_snap(struct rbd_device *dev,
> +static int rbd_header_add_snap(struct rbd_device *rbd_dev,
>   			       const char *snap_name,
>   			       gfp_t gfp_flags)
>   {
> @@ -1644,11 +1646,11 @@ static int rbd_header_add_snap(struct rbd_device
> *dev,
>   	struct ceph_mon_client *monc;
>
>   	/* we should create a snapshot only if we're pointing at the head */
> -	if (dev->snap_id != CEPH_NOSNAP)
> +	if (rbd_dev->snap_id != CEPH_NOSNAP)
>   		return -EINVAL;
>
> -	monc =&dev->rbd_client->client->monc;
> -	ret = ceph_monc_create_snapid(monc, dev->pool_id,&new_snapid);
> +	monc =&rbd_dev->rbd_client->client->monc;
> +	ret = ceph_monc_create_snapid(monc, rbd_dev->pool_id,&new_snapid);
>   	dout("created snapid=%lld\n", new_snapid);
>   	if (ret<  0)
>   		return ret;
> @@ -1663,7 +1665,8 @@ static int rbd_header_add_snap(struct rbd_device *dev,
>   	ceph_encode_string_safe(&p, e, snap_name, name_len, bad);
>   	ceph_encode_64_safe(&p, e, new_snapid, bad);
>
> -	ret = rbd_req_sync_exec(dev, dev->obj_md_name, "rbd", "snap_add",
> +	ret = rbd_req_sync_exec(rbd_dev, rbd_dev->obj_md_name,
> +				"rbd", "snap_add",
>   				data, p - data,&ver);
>
>   	kfree(data);
> @@ -1671,9 +1674,9 @@ static int rbd_header_add_snap(struct rbd_device *dev,
>   	if (ret<  0)
>   		return ret;
>
> -	down_write(&dev->header_rwsem);
> -	dev->header.snapc->seq = new_snapid;
> -	up_write(&dev->header_rwsem);
> +	down_write(&rbd_dev->header_rwsem);
> +	rbd_dev->header.snapc->seq = new_snapid;
> +	up_write(&rbd_dev->header_rwsem);
>
>   	return 0;
>   bad:

Re: [PATCH 13/16] rbd: rename some fields in struct rbd_dev

[Josh Durgin]

On 07/11/2012 07:02 AM, Alex Elder wrote:
> An rbd image is not a single object, but a logical construct made up
> of an aggregation of objects.
>
> Rename some fields in struct rbd_dev, in hopes of reinforcing this.
>      obj         -->  image_name
>      obj_len     -->  image_name_len

As mentioned before, this can be turned into a local variable instead.

>      obj_md_name -->  header_name
>
> Also rename a related symbol name:
>      RBD_MAX_MD_NAME_LEN -->  RBD_MAX_HEADER_NAME_LEN

Looks like this got removed in patch 9.

>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c |   53
> ++++++++++++++++++++++++++-------------------------
>   1 files changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index c38246b..14e675c 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -161,9 +161,9 @@ struct rbd_device {
>   	spinlock_t		lock;		/* queue lock */
>
>   	struct rbd_image_header	header;
> -	char			*obj; /* rbd image name */
> -	size_t			obj_len;
> -	char			*obj_md_name; /* hdr nm. */
> +	char			*image_name;
> +	size_t			image_name_len;
> +	char			*header_name;
>   	int			pool_id;
>
>   	struct ceph_osd_event   *watch_event;
> @@ -1224,8 +1224,8 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
> u8 opcode, void *data)
>   	if (!rbd_dev)
>   		return;
>
> -	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n", rbd_dev->obj_md_name,
> -		notify_id, (int)opcode);
> +	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n",
> +		rbd_dev->header_name, notify_id, (int) opcode);
>   	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
>   	rc = __rbd_refresh_header(rbd_dev);
>   	mutex_unlock(&ctl_mutex);
> @@ -1233,7 +1233,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
> u8 opcode, void *data)
>   		pr_warning(RBD_DRV_NAME "%d got notification but failed to "
>   			   " update snaps: %d\n", rbd_dev->major, rc);
>
> -	rbd_req_sync_notify_ack(rbd_dev, ver, notify_id, rbd_dev->obj_md_name);
> +	rbd_req_sync_notify_ack(rbd_dev, ver, notify_id, rbd_dev->header_name);
>   }
>
>   /*
> @@ -1321,7 +1321,7 @@ static void rbd_notify_cb(u64 ver, u64 notify_id,
> u8 opcode, void *data)
>   		return;
>
>   	dout("rbd_notify_cb %s notify_id=%lld opcode=%d\n",
> -				rbd_dev->obj_md_name,
> +				rbd_dev->header_name,
>   		notify_id, (int)opcode);
>   }
>
> @@ -1599,7 +1599,7 @@ static int rbd_read_header(struct rbd_device *rbd_dev,
>
>   		rc = rbd_req_sync_read(rbd_dev,
>   				       NULL, CEPH_NOSNAP,
> -				       rbd_dev->obj_md_name,
> +				       rbd_dev->header_name,
>   				       0, len,
>   				       (char *)dh,&ver);
>   		if (rc<  0)
> @@ -1609,7 +1609,8 @@ static int rbd_read_header(struct rbd_device *rbd_dev,
>   		if (rc<  0) {
>   			if (rc == -ENXIO)
>   				pr_warning("unrecognized header format"
> -					   " for image %s", rbd_dev->obj);
> +					   " for image %s\n",
> +					   rbd_dev->image_name);
>   			goto out_dh;
>   		}
>
> @@ -1665,7 +1666,7 @@ static int rbd_header_add_snap(struct rbd_device
> *rbd_dev,
>   	ceph_encode_string_safe(&p, e, snap_name, name_len, bad);
>   	ceph_encode_64_safe(&p, e, new_snapid, bad);
>
> -	ret = rbd_req_sync_exec(rbd_dev, rbd_dev->obj_md_name,
> +	ret = rbd_req_sync_exec(rbd_dev, rbd_dev->header_name,
>   				"rbd", "snap_add",
>   				data, p - data,&ver);
>
> @@ -1862,7 +1863,7 @@ static ssize_t rbd_name_show(struct device *dev,
>   {
>   	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>
> -	return sprintf(buf, "%s\n", rbd_dev->obj);
> +	return sprintf(buf, "%s\n", rbd_dev->image_name);
>   }
>
>   static ssize_t rbd_snap_show(struct device *dev,
> @@ -2164,7 +2165,7 @@ static int rbd_init_watch_dev(struct rbd_device
> *rbd_dev)
>   	int ret, rc;
>
>   	do {
> -		ret = rbd_req_sync_watch(rbd_dev, rbd_dev->obj_md_name,
> +		ret = rbd_req_sync_watch(rbd_dev, rbd_dev->header_name,
>   					 rbd_dev->header.obj_version);
>   		if (ret == -ERANGE) {
>   			mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> @@ -2327,7 +2328,7 @@ static inline char *dup_token(const char **buf,
> size_t *lenp)
>   }
>
>   /*
> - * This fills in the obj, obj_len, snap_name, rbd_dev, rbd_md_name,
> + * This fills in the image, image_len, snap_name, rbd_dev, header_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.
>    *
> @@ -2368,18 +2369,18 @@ static char *rbd_add_parse_args(struct
> rbd_device *rbd_dev,
>   	if (!pool_name)
>   		goto out_err;
>
> -	rbd_dev->obj = dup_token(&buf,&rbd_dev->obj_len);
> -	if (!rbd_dev->obj)
> +	rbd_dev->image_name = dup_token(&buf,&rbd_dev->image_name_len);
> +	if (!rbd_dev->image_name)
>   		goto out_err;
>
>   	/* Create the name of the header object */
>
> -	rbd_dev->obj_md_name = kmalloc(rbd_dev->obj_len
> +	rbd_dev->header_name = kmalloc(rbd_dev->image_name_len
>   						+ sizeof (RBD_SUFFIX),
>   					GFP_KERNEL);
> -	if (!rbd_dev->obj_md_name)
> +	if (!rbd_dev->header_name)
>   		goto out_err;
> -	sprintf(rbd_dev->obj_md_name, "%s%s", rbd_dev->obj, RBD_SUFFIX);
> +	sprintf(rbd_dev->header_name, "%s%s", rbd_dev->image_name, RBD_SUFFIX);
>
>   	/*
>   	 * The snapshot name is optional.  If none is is supplied,
> @@ -2403,8 +2404,8 @@ static char *rbd_add_parse_args(struct rbd_device
> *rbd_dev,
>   	return pool_name;
>
>   out_err:
> -	kfree(rbd_dev->obj_md_name);
> -	kfree(rbd_dev->obj);
> +	kfree(rbd_dev->header_name);
> +	kfree(rbd_dev->image_name);
>   	kfree(pool_name);
>
>   	return ERR_PTR(ret);
> @@ -2513,8 +2514,8 @@ err_out_client:
>   err_put_id:
>   	if (pool_name) {
>   		kfree(rbd_dev->snap_name);
> -		kfree(rbd_dev->obj_md_name);
> -		kfree(rbd_dev->obj);
> +		kfree(rbd_dev->header_name);
> +		kfree(rbd_dev->image_name);
>   		kfree(pool_name);
>   	}
>   	rbd_id_put(rbd_dev);
> @@ -2556,7 +2557,7 @@ static void rbd_dev_release(struct device *dev)
>   						    rbd_dev->watch_request);
>   	}
>   	if (rbd_dev->watch_event)
> -		rbd_req_sync_unwatch(rbd_dev, rbd_dev->obj_md_name);
> +		rbd_req_sync_unwatch(rbd_dev, rbd_dev->header_name);
>
>   	rbd_put_client(rbd_dev);
>
> @@ -2566,8 +2567,8 @@ static void rbd_dev_release(struct device *dev)
>
>   	/* done with the id, and with the rbd_dev */
>   	kfree(rbd_dev->snap_name);
> -	kfree(rbd_dev->obj_md_name);
> -	kfree(rbd_dev->obj);
> +	kfree(rbd_dev->header_name);
> +	kfree(rbd_dev->image_name);
>   	rbd_id_put(rbd_dev);
>   	kfree(rbd_dev);
>
> @@ -2638,7 +2639,7 @@ static ssize_t rbd_snap_add(struct device *dev,
>   	mutex_unlock(&ctl_mutex);
>
>   	/* make a best effort, don't error if failed */
> -	rbd_req_sync_notify(rbd_dev, rbd_dev->obj_md_name);
> +	rbd_req_sync_notify(rbd_dev, rbd_dev->header_name);
>
>   	ret = count;
>   	kfree(name);

Re: [PATCH 14/16] rbd: more symbol renames

[Josh Durgin]

On 07/11/2012 07:02 AM, Alex Elder wrote:
> Rename variables named "obj" which represent object names so they're
> consistenly named "object_name".

typo

Re: [PATCH 15/16] rbd: option symbol renames

[Josh Durgin]

On 07/11/2012 07:03 AM, Alex Elder wrote:
> Use the name "ceph_opts" consistently (rather than just "opt") for
> pointers to a ceph_options structure.
>
> Change the few spots that don't use "rbd_opts" for a rbd_options
> pointer to match the rest.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c |   40 ++++++++++++++++++++--------------------
>   1 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index d29c864..c99ea08 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -270,9 +270,9 @@ static const struct block_device_operations
> rbd_bd_ops = {
>
>   /*
>    * Initialize an rbd client instance.
> - * We own *opt.
> + * We own *ceph_opts.
>    */
> -static struct rbd_client *rbd_client_create(struct ceph_options *opt,
> +static struct rbd_client *rbd_client_create(struct ceph_options *ceph_opts,
>   					    struct rbd_options *rbd_opts)
>   {
>   	struct rbd_client *rbdc;
> @@ -288,10 +288,10 @@ static struct rbd_client *rbd_client_create(struct
> ceph_options *opt,
>
>   	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
>
> -	rbdc->client = ceph_create_client(opt, rbdc, 0, 0);
> +	rbdc->client = ceph_create_client(ceph_opts, rbdc, 0, 0);
>   	if (IS_ERR(rbdc->client))
>   		goto out_mutex;
> -	opt = NULL; /* Now rbdc->client is responsible for opt */
> +	ceph_opts = NULL; /* Now rbdc->client is responsible for ceph_opts */
>
>   	ret = ceph_open_session(rbdc->client);
>   	if (ret<  0)
> @@ -314,23 +314,23 @@ out_mutex:
>   	mutex_unlock(&ctl_mutex);
>   	kfree(rbdc);
>   out_opt:
> -	if (opt)
> -		ceph_destroy_options(opt);
> +	if (ceph_opts)
> +		ceph_destroy_options(ceph_opts);
>   	return ERR_PTR(ret);
>   }
>
>   /*
>    * Find a ceph client with specific addr and configuration.
>    */
> -static struct rbd_client *__rbd_client_find(struct ceph_options *opt)
> +static struct rbd_client *__rbd_client_find(struct ceph_options *ceph_opts)
>   {
>   	struct rbd_client *client_node;
>
> -	if (opt->flags&  CEPH_OPT_NOSHARE)
> +	if (ceph_opts->flags&  CEPH_OPT_NOSHARE)
>   		return NULL;
>
>   	list_for_each_entry(client_node,&rbd_client_list, node)
> -		if (ceph_compare_options(opt, client_node->client) == 0)
> +		if (!ceph_compare_options(ceph_opts, client_node->client))
>   			return client_node;
>   	return NULL;
>   }
> @@ -346,7 +346,7 @@ enum {
>   	/* string args above */
>   };
>
> -static match_table_t rbdopt_tokens = {
> +static match_table_t rbd_opts_tokens = {
>   	{Opt_notify_timeout, "notify_timeout=%d"},
>   	/* int args above */
>   	/* string args above */
> @@ -355,11 +355,11 @@ static match_table_t rbdopt_tokens = {
>
>   static int parse_rbd_opts_token(char *c, void *private)
>   {
> -	struct rbd_options *rbdopt = private;
> +	struct rbd_options *rbd_opts = private;
>   	substring_t argstr[MAX_OPT_ARGS];
>   	int token, intval, ret;
>
> -	token = match_token(c, rbdopt_tokens, argstr);
> +	token = match_token(c, rbd_opts_tokens, argstr);
>   	if (token<  0)
>   		return -EINVAL;
>
> @@ -380,7 +380,7 @@ static int parse_rbd_opts_token(char *c, void *private)
>
>   	switch (token) {
>   	case Opt_notify_timeout:
> -		rbdopt->notify_timeout = intval;
> +		rbd_opts->notify_timeout = intval;
>   		break;
>   	default:
>   		BUG_ON(token);
> @@ -397,7 +397,7 @@ static struct rbd_client *rbd_get_client(const char
> *mon_addr,
>   					 char *options)
>   {
>   	struct rbd_client *rbdc;
> -	struct ceph_options *opt;
> +	struct ceph_options *ceph_opts;
>   	struct rbd_options *rbd_opts;
>
>   	rbd_opts = kzalloc(sizeof(*rbd_opts), GFP_KERNEL);
> @@ -406,29 +406,29 @@ static struct rbd_client *rbd_get_client(const
> char *mon_addr,
>
>   	rbd_opts->notify_timeout = RBD_NOTIFY_TIMEOUT_DEFAULT;
>
> -	opt = ceph_parse_options(options, mon_addr,
> +	ceph_opts = ceph_parse_options(options, mon_addr,
>   				mon_addr + mon_addr_len,
>   				parse_rbd_opts_token, rbd_opts);

indentation

> -	if (IS_ERR(opt)) {
> +	if (IS_ERR(ceph_opts)) {
>   		kfree(rbd_opts);
> -		return ERR_CAST(opt);
> +		return ERR_CAST(ceph_opts);
>   	}
>
>   	spin_lock(&rbd_client_list_lock);
> -	rbdc = __rbd_client_find(opt);
> +	rbdc = __rbd_client_find(ceph_opts);
>   	if (rbdc) {
>   		/* using an existing client */
>   		kref_get(&rbdc->kref);
>   		spin_unlock(&rbd_client_list_lock);
>
> -		ceph_destroy_options(opt);
> +		ceph_destroy_options(ceph_opts);
>   		kfree(rbd_opts);
>
>   		return rbdc;
>   	}
>   	spin_unlock(&rbd_client_list_lock);
>
> -	rbdc = rbd_client_create(opt, rbd_opts);
> +	rbdc = rbd_client_create(ceph_opts, rbd_opts);
>
>   	if (IS_ERR(rbdc))
>   		kfree(rbd_opts);

Re: [PATCH 16/16] rbd: kill num_reply parameters

[Josh Durgin]

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

On 07/11/2012 07:03 AM, Alex Elder wrote:
> Several functions include a num_reply parameter, but it is never
> used.  Just get rid of it everywhere--it seems to be something
> that never got fully implemented.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c |   19 ++++++-------------
>   1 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index c99ea08..c023be5 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -863,7 +863,6 @@ static int rbd_do_request(struct request *rq,
>   			  int num_pages,
>   			  int flags,
>   			  struct ceph_osd_req_op *ops,
> -			  int num_reply,
>   			  struct rbd_req_coll *coll,
>   			  int coll_index,
>   			  void (*rbd_cb)(struct ceph_osd_request *req,
> @@ -1019,7 +1018,6 @@ static int rbd_req_sync_op(struct rbd_device *rbd_dev,
>   			   int opcode,
>   			   int flags,
>   			   struct ceph_osd_req_op *orig_ops,
> -			   int num_reply,
>   			   const char *object_name,
>   			   u64 ofs, u64 len,
>   			   char *buf,
> @@ -1055,7 +1053,6 @@ static int rbd_req_sync_op(struct rbd_device *rbd_dev,
>   			  pages, num_pages,
>   			  flags,
>   			  ops,
> -			  2,
>   			  NULL, 0,
>   			  NULL,
>   			  linger_req, ver);
> @@ -1080,7 +1077,7 @@ static int rbd_do_op(struct request *rq,
>   		     struct rbd_device *rbd_dev,
>   		     struct ceph_snap_context *snapc,
>   		     u64 snapid,
> -		     int opcode, int flags, int num_reply,
> +		     int opcode, int flags,
>   		     u64 ofs, u64 len,
>   		     struct bio *bio,
>   		     struct rbd_req_coll *coll,
> @@ -1119,7 +1116,6 @@ static int rbd_do_op(struct request *rq,
>   			     NULL, 0,
>   			     flags,
>   			     ops,
> -			     num_reply,
>   			     coll, coll_index,
>   			     rbd_req_cb, 0, NULL);
>
> @@ -1143,7 +1139,6 @@ static int rbd_req_write(struct request *rq,
>   	return rbd_do_op(rq, rbd_dev, snapc, CEPH_NOSNAP,
>   			 CEPH_OSD_OP_WRITE,
>   			 CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
> -			 2,
>   			 ofs, len, bio, coll, coll_index);
>   }
>
> @@ -1162,7 +1157,6 @@ static int rbd_req_read(struct request *rq,
>   			 snapid,
>   			 CEPH_OSD_OP_READ,
>   			 CEPH_OSD_FLAG_READ,
> -			 2,
>   			 ofs, len, bio, coll, coll_index);
>   }
>
> @@ -1182,7 +1176,7 @@ static int rbd_req_sync_read(struct rbd_device
> *rbd_dev,
>   			       CEPH_OSD_OP_READ,
>   			       CEPH_OSD_FLAG_READ,
>   			       NULL,
> -			       1, object_name, ofs, len, buf, NULL, ver);
> +			       object_name, ofs, len, buf, NULL, ver);
>   }
>
>   /*
> @@ -1209,7 +1203,6 @@ static int rbd_req_sync_notify_ack(struct
> rbd_device *rbd_dev,
>   			  NULL, 0,
>   			  CEPH_OSD_FLAG_READ,
>   			  ops,
> -			  1,
>   			  NULL, 0,
>   			  rbd_simple_req_cb, 0, NULL);
>
> @@ -1265,7 +1258,7 @@ static int rbd_req_sync_watch(struct rbd_device
> *rbd_dev,
>   			      0,
>   			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
>   			      ops,
> -			      1, object_name, 0, 0, NULL,
> +			      object_name, 0, 0, NULL,
>   			&rbd_dev->watch_request, NULL);
>
>   	if (ret<  0)
> @@ -1303,7 +1296,7 @@ static int rbd_req_sync_unwatch(struct rbd_device
> *rbd_dev,
>   			      0,
>   			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
>   			      ops,
> -			      1, object_name, 0, 0, NULL, NULL, NULL);
> +			      object_name, 0, 0, NULL, NULL, NULL);
>
>   	rbd_destroy_ops(ops);
>   	ceph_osdc_cancel_event(rbd_dev->watch_event);
> @@ -1361,7 +1354,7 @@ static int rbd_req_sync_notify(struct rbd_device
> *rbd_dev,
>   			       0,
>   			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
>   			       ops,
> -			       1, object_name, 0, 0, NULL, NULL, NULL);
> +			       object_name, 0, 0, NULL, NULL, NULL);
>   	if (ret<  0)
>   		goto fail_event;
>
> @@ -1409,7 +1402,7 @@ static int rbd_req_sync_exec(struct rbd_device
> *rbd_dev,
>   			       0,
>   			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
>   			       ops,
> -			       1, object_name, 0, 0, NULL, NULL, ver);
> +			       object_name, 0, 0, NULL, NULL, ver);
>
>   	rbd_destroy_ops(ops);
>

Re: [PATCH 05/16] rbd: define dup_token()

[Alex Elder]

On 07/11/2012 12:48 PM, Yehuda Sadeh wrote:
> I assume this is specialized enough so that there's no risk in reusing
> it in a different context, so for this one we can keep it this way.

I agree on this one.  It's only  used in argument parsing right now.
If we find we want to use it in a place that needs better control it's
easy enough to add the gfp argument.

					-Alex

[PATCH v2 03/16] libceph: define ceph_decode_string()

[Alex Elder]

There is no string decoding function defined in <decode.h>, so this
defines one.

This function is a little different from the others in that the
length of the encoded string is not known a priori.  So the
interface is defined a bit like snprintf(), where the value returned
indicates the space required--even if it's more than the space
allotted.

The function also avoids overrunning the end of the memory range
being converted.

Signed-off-by: Alex Elder <elder@inktank.com>
---
v2: Made the function safe from overrunning the source memory

 include/linux/ceph/decode.h |   45
++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

Index: b/include/linux/ceph/decode.h
===================================================================
--- a/include/linux/ceph/decode.h
+++ b/include/linux/ceph/decode.h
@@ -85,6 +85,51 @@ static inline int ceph_has_room(void **p
 	} while (0)

 /*
+ * Decode the wire-encoded string at *p into the buffer "s"
+ * provided, whose size is indicated by "size".  Note that "s" can
+ * be a null pointer if size is 0.  If it fits, the resulting string
+ * will always be terminated with '\0'; otherwise the buffer will
+ * be unchanged.
+ *
+ * Care is taken to ensure the result of decoding the string will
+ * not touch anything at or beyond the "end" address provided.  If
+ * it would, -ERANGE is returned.
+ *
+ * Otherwise, returns the length of the encoded string (which may be
+ * greater than or equal to the buffer size).  The return value does
+ * not include the terminating '\0'.
+ *
+ * If the the return value is not negative and is less than the size
+ * provided, *p will be advanced past the decoded data; otherwise it
+ * is unchanged.  This allows for a two call sequence to be used to
+ * allocate sufficient space for the string.
+ *
+ */
+static inline ssize_t ceph_decode_string(void **p, void *end,
+					char *s, size_t size)
+{
+	size_t len;
+	void *cp = *p;
+
+	ceph_decode_32_safe(&cp, end, len, bad);
+	if (!ceph_has_room(&cp, end, len))
+	    	goto bad;
+
+	if (size < len + 1)
+		return len;	/* Not enough room */
+
+	if (len)
+		memcpy(s, cp, len);
+	*(s + len) = '\0';
+
+	*p = (char *) *p + sizeof (u32) + len;
+
+    	return (ssize_t) len;
+bad:
+    	return (ssize_t) -ERANGE;
+}
+
+/*
  * struct ceph_timespec <-> struct timespec
  */
 static inline void ceph_decode_timespec(struct timespec *ts,

[PATCH v2 04/16] libceph: define ceph_extract_encoded_string()

[Alex Elder]

This adds a new utility routine which will return a dynamically-
allocated buffer containing a string that has been decoded from ceph
over-the-wire format.  It also returns the length of the string
if the address of a size variable is supplied to receive it.

Signed-off-by: Alex Elder <elder@inktank.com>
---
v2: Made the function safe from overrunning the source memory, and
    and added a gfp argument to pass to kmalloc()

 include/linux/ceph/decode.h |   38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

Index: b/include/linux/ceph/decode.h
===================================================================
--- a/include/linux/ceph/decode.h
+++ b/include/linux/ceph/decode.h
@@ -130,6 +130,44 @@ bad:
 }

 /*
+ * Allocate a buffer big enough to hold the wire-encoded string, and
+ * decode the string into it.  The resulting string will always be
+ * terminated with '\0'.  If successful, *p will be advanced
+ * past the decoded data.  Also, if lenp is not a null pointer, the
+ * length (not including the terminating '\0') will be recorded in
+ * it.  Note that a zero-length string is a valid return value.
+ *
+ * Returns a pointer to the newly-allocated string buffer, or a null
+ * pointer if an error occurs.  Neither "p" nor "end" will be updated
+ * if NULL is returned.
+ *
+ * There are two possible failures:
+ *   - memory could not be allocated for the result
+ *   - converting the string would require accessing memory at or
+ *     beyond the "end" pointer provided
+ */
+static inline char *ceph_extract_encoded_string(void **p, void *end,
+						size_t *lenp, gfp_t gfp)
+{
+	ssize_t len;
+	char *buf;
+
+	len = ceph_decode_string(p, end, NULL, 0);
+	if (len < 0)
+	    	return NULL;
+
+	buf = kmalloc(len + 1, gfp);
+	if (!buf)
+		return NULL;
+
+	(void) ceph_decode_string(p, end, buf, len + 1);
+	if (lenp)
+		*lenp = (size_t) len;
+
+	return buf;
+}
+
+/*
  * struct ceph_timespec <-> struct timespec
  */
 static inline void ceph_decode_timespec(struct timespec *ts,

Re: [PATCH 06/16] rbd: rename rbd_dev->block_name

[Alex Elder]

On 07/11/2012 02:02 PM, Josh Durgin wrote:
> Might want to rename block_name in struct rbd_image_header_ondisk too.

I thought about that, but explicitly decided against it.  It's kind
of arbitrary.  In part I didn't want to mess with definitions that
are common to both user space and kernel.  But it also left the
old definition intact, which seemed worthwhile.

					-Alex

Re: [PATCH 08/16] rbd: don't store pool name in struct rbd_dev

[Alex Elder]

On 07/11/2012 02:36 PM, Josh Durgin wrote:
> On 07/11/2012 07:02 AM, Alex Elder wrote:
>> An rbd device's pool name is used to specify the pool to use for an
>> rbd image when it gets mapped (via rbd_add()).  However, only the
>> pool id can be relied on to be invariant--the name of a pool can
>> change at any time.
>>
>> This means that the name of the pool is potentially stale as soon as
>> the image is mapped, so it's a bad idea to record this information.
>> So only store the pool id, not its name, in an rbd_dev.
>>
>> Here are a few specific notes about this change:
>>      - The "pool" name device attribute (/sys/bus/rbd/devices/<N>/pool)
>>        goes away.  In its place is a "pool_id" attribute that provide
>>        the numeric pool id for the rbd image.
> 
> We're using the pool name for udev to provide a predictable device name
> (/dev/rbd/poolname/imagename[@snapname]), so we probably want to keep
> the sysfs attribute. I don't think there's a good way for us to detect
> pool renames right now though, so we should document that the pool
> name reported does not reflect any potential renames.

OK.  I'll put the "pool" name entry back, but will still include the
new "pool_id" entry as well.  Do you want me to re-post after that,
or can I consider this reviewed?

					-Alex

>>      - rbd_add_parse_args() now returns a pointer to a dynamically-
>>        allocated copy of the pool name taken from the arguments.
>>      - rbd_add() has been changed to handle freeing the pool name,
>>        both in error cases and in the normal case after the pool id
>>        has been recorded.
>>
>> Signed-off-by: Alex Elder<elder@inktank.com>

. . .

Re: [PATCH 08/16] rbd: don't store pool name in struct rbd_dev

[Josh Durgin]

On 07/11/2012 03:25 PM, Alex Elder wrote:
> On 07/11/2012 02:36 PM, Josh Durgin wrote:
>> On 07/11/2012 07:02 AM, Alex Elder wrote:
>>> An rbd device's pool name is used to specify the pool to use for an
>>> rbd image when it gets mapped (via rbd_add()).  However, only the
>>> pool id can be relied on to be invariant--the name of a pool can
>>> change at any time.
>>>
>>> This means that the name of the pool is potentially stale as soon as
>>> the image is mapped, so it's a bad idea to record this information.
>>> So only store the pool id, not its name, in an rbd_dev.
>>>
>>> Here are a few specific notes about this change:
>>>       - The "pool" name device attribute (/sys/bus/rbd/devices/<N>/pool)
>>>         goes away.  In its place is a "pool_id" attribute that provide
>>>         the numeric pool id for the rbd image.
>>
>> We're using the pool name for udev to provide a predictable device name
>> (/dev/rbd/poolname/imagename[@snapname]), so we probably want to keep
>> the sysfs attribute. I don't think there's a good way for us to detect
>> pool renames right now though, so we should document that the pool
>> name reported does not reflect any potential renames.
>
> OK.  I'll put the "pool" name entry back, but will still include the
> new "pool_id" entry as well.  Do you want me to re-post after that,
> or can I consider this reviewed?

Consider it reviewed. The same goes for the rest of this series,
since it was just minor changes.

Re: [PATCH 08/16] rbd: don't store pool name in struct rbd_dev

[Alex Elder]

On 07/11/2012 02:36 PM, Josh Durgin wrote:
> On 07/11/2012 07:02 AM, Alex Elder wrote:
>> An rbd device's pool name is used to specify the pool to use for an
>> rbd image when it gets mapped (via rbd_add()).  However, only the
>> pool id can be relied on to be invariant--the name of a pool can
>> change at any time.
>>
>> This means that the name of the pool is potentially stale as soon as
>> the image is mapped, so it's a bad idea to record this information.
>> So only store the pool id, not its name, in an rbd_dev.
>>
>> Here are a few specific notes about this change:
>>      - The "pool" name device attribute (/sys/bus/rbd/devices/<N>/pool)
>>        goes away.  In its place is a "pool_id" attribute that provide
>>        the numeric pool id for the rbd image.
> 
> We're using the pool name for udev to provide a predictable device name
> (/dev/rbd/poolname/imagename[@snapname]), so we probably want to keep
> the sysfs attribute. I don't think there's a good way for us to detect
> pool renames right now though, so we should document that the pool
> name reported does not reflect any potential renames.

OK, now that I've thought about this...

I'm going to pull this patch from the series entirely.
In its place, I'll do a patch that makes the pool name
get allocated dynamically (like the patches that follow
this one).  I will also put in place a patch that exposes
the pool id via the /sys/bus/rbd/devices/<N>/pool_id though.

Long term we could switch to using the pool id for the
predictable device name.  All format 1 images would
share the same pool (which would need to be assigned a
reserved pool id different from the empty string).

Which brings up another thing...
It would be good to provide an efficient tool to allow
a customer to convert a format 1 rbd image into format 2.
It seems like it would mostly involve object renames,
however they would all have to be done transactionally.

					-Alex

>>      - rbd_add_parse_args() now returns a pointer to a dynamically-
>>        allocated copy of the pool name taken from the arguments.
>>      - rbd_add() has been changed to handle freeing the pool name,
>>        both in error cases and in the normal case after the pool id
>>        has been recorded.
>>
>> Signed-off-by: Alex Elder<elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |   74
>> +++++++++++++++++++++++++++++++-------------------
>>   1 files changed, 46 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 3aa0ca0..76e978c 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -56,7 +56,6 @@
>>   #define RBD_MINORS_PER_MAJOR    256        /* max minors per blkdev */
>>
>>   #define RBD_MAX_MD_NAME_LEN    (RBD_MAX_OBJ_NAME_LEN +
>> sizeof(RBD_SUFFIX))
>> -#define RBD_MAX_POOL_NAME_LEN    64
>>   #define RBD_MAX_SNAP_NAME_LEN    32
>>   #define RBD_MAX_OPT_LEN        1024
>>
>> @@ -166,8 +165,7 @@ struct rbd_device {
>>       char            obj[RBD_MAX_OBJ_NAME_LEN]; /* rbd image name */
>>       int            obj_len;
>>       char            obj_md_name[RBD_MAX_MD_NAME_LEN]; /* hdr nm. */
>> -    char            pool_name[RBD_MAX_POOL_NAME_LEN];
>> -    int            poolid;
>> +    int            pool_id;
>>
>>       struct ceph_osd_event   *watch_event;
>>       struct ceph_osd_request *watch_request;
>> @@ -930,7 +928,7 @@ static int rbd_do_request(struct request *rq,
>>       layout->fl_stripe_unit = cpu_to_le32(1<<  RBD_MAX_OBJ_ORDER);
>>       layout->fl_stripe_count = cpu_to_le32(1);
>>       layout->fl_object_size = cpu_to_le32(1<<  RBD_MAX_OBJ_ORDER);
>> -    layout->fl_pg_pool = cpu_to_le32(dev->poolid);
>> +    layout->fl_pg_pool = cpu_to_le32(dev->pool_id);
>>       ceph_calc_raw_layout(osdc, layout, snapid, ofs,&len,&bno,
>>                   req, ops);
>>
>> @@ -1653,7 +1651,7 @@ static int rbd_header_add_snap(struct rbd_device
>> *dev,
>>           return -EINVAL;
>>
>>       monc =&dev->rbd_client->client->monc;
>> -    ret = ceph_monc_create_snapid(monc, dev->poolid,&new_snapid);
>> +    ret = ceph_monc_create_snapid(monc, dev->pool_id,&new_snapid);
>>       dout("created snapid=%lld\n", new_snapid);
>>       if (ret<  0)
>>           return ret;
>> @@ -1851,12 +1849,12 @@ static ssize_t rbd_client_id_show(struct device
>> *dev,
>>               ceph_client_id(rbd_dev->rbd_client->client));
>>   }
>>
>> -static ssize_t rbd_pool_show(struct device *dev,
>> +static ssize_t rbd_pool_id_show(struct device *dev,
>>                    struct device_attribute *attr, char *buf)
>>   {
>>       struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>>
>> -    return sprintf(buf, "%s\n", rbd_dev->pool_name);
>> +    return sprintf(buf, "%d\n", rbd_dev->pool_id);
>>   }
>>
>>   static ssize_t rbd_name_show(struct device *dev,
>> @@ -1898,7 +1896,7 @@ static ssize_t rbd_image_refresh(struct device
>> *dev,
>>   static DEVICE_ATTR(size, S_IRUGO, rbd_size_show, NULL);
>>   static DEVICE_ATTR(major, S_IRUGO, rbd_major_show, NULL);
>>   static DEVICE_ATTR(client_id, S_IRUGO, rbd_client_id_show, NULL);
>> -static DEVICE_ATTR(pool, S_IRUGO, rbd_pool_show, NULL);
>> +static DEVICE_ATTR(pool_id, S_IRUGO, rbd_pool_id_show, NULL);
>>   static DEVICE_ATTR(name, S_IRUGO, rbd_name_show, NULL);
>>   static DEVICE_ATTR(refresh, S_IWUSR, NULL, rbd_image_refresh);
>>   static DEVICE_ATTR(current_snap, S_IRUGO, rbd_snap_show, NULL);
>> @@ -1908,7 +1906,7 @@ static struct attribute *rbd_attrs[] = {
>>       &dev_attr_size.attr,
>>       &dev_attr_major.attr,
>>       &dev_attr_client_id.attr,
>> -    &dev_attr_pool.attr,
>> +    &dev_attr_pool_id.attr,
>>       &dev_attr_name.attr,
>>       &dev_attr_current_snap.attr,
>>       &dev_attr_refresh.attr,
>> @@ -2329,25 +2327,31 @@ static inline char *dup_token(const char **buf,
>> size_t *lenp)
>>   }
>>
>>   /*
>> - * This fills in the pool_name, obj, obj_len, snap_name, obj_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.
>> + * This fills in the obj, obj_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.
>> + *
>> + * Returns a pointer to a dynamically-allocated buffer containing
>> + * the pool name provided, or a pointer-coded errno if an error
>> + * occurs.
>>    */
>> -static int rbd_add_parse_args(struct rbd_device *rbd_dev,
>> +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;
>> +    size_t len;
>> +    int ret = -EINVAL;
>> +    char *pool_name = NULL;
>>
>>       /* The first four tokens are required */
>>
>>       len = next_token(&buf);
>>       if (!len)
>> -        return -EINVAL;
>> +        goto out_err;
>> +
>>       *mon_addrs_size = len + 1;
>>       *mon_addrs = buf;
>>
>> @@ -2355,15 +2359,17 @@ 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;
>> +        goto out_err;
>>
>> -    len = copy_token(&buf, rbd_dev->pool_name, sizeof
>> (rbd_dev->pool_name));
>> -    if (!len || len>= sizeof (rbd_dev->pool_name))
>> -        return -EINVAL;
>> +    pool_name = dup_token(&buf, NULL);
>> +    if (!pool_name) {
>> +        ret = -ENOMEM;
>> +        goto out_err;
>> +    }
>>
>>       len = copy_token(&buf, rbd_dev->obj, sizeof (rbd_dev->obj));
>>       if (!len || len>= sizeof (rbd_dev->obj))
>> -        return -EINVAL;
>> +        goto out_err;
>>
>>       /* We have the object length in hand, save it. */
>>
>> @@ -2382,9 +2388,14 @@ static int rbd_add_parse_args(struct rbd_device
>> *rbd_dev,
>>           memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>>               sizeof (RBD_SNAP_HEAD_NAME));
>>       else if (len>= sizeof (rbd_dev->snap_name))
>> -        return -EINVAL;
>> +        goto out_err;
>>
>> -    return 0;
>> +    return pool_name;
>> +
>> +out_err:
>> +    kfree(pool_name);
>> +
>> +    return ERR_PTR(ret);
>>   }
>>
>>   static ssize_t rbd_add(struct bus_type *bus,
>> @@ -2396,6 +2407,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>>       size_t mon_addrs_size = 0;
>>       char *options = NULL;
>>       struct ceph_osd_client *osdc;
>> +    char *pool_name;
>>       int rc = -ENOMEM;
>>
>>       if (!try_module_get(THIS_MODULE))
>> @@ -2425,10 +2437,14 @@ static ssize_t rbd_add(struct bus_type *bus,
>>       sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->id);
>>
>>       /* parse add command */
>> -    rc = rbd_add_parse_args(rbd_dev, buf,&mon_addrs,&mon_addrs_size,
>> -                options, count);
>> -    if (rc)
>> +    pool_name = rbd_add_parse_args(rbd_dev, buf,
>> +                    &mon_addrs,&mon_addrs_size,
>> +                    options, count);
>> +    if (IS_ERR(pool_name)) {
>> +        rc = PTR_ERR(pool_name);
>> +        pool_name = NULL;
>>           goto err_put_id;
>> +    }
>>
>>       rbd_dev->rbd_client = rbd_get_client(mon_addrs, mon_addrs_size - 1,
>>                           options);
>> @@ -2439,10 +2455,10 @@ static ssize_t rbd_add(struct bus_type *bus,
>>
>>       /* pick the pool */
>>       osdc =&rbd_dev->rbd_client->client->osdc;
>> -    rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name);
>> +    rc = ceph_pg_poolid_by_name(osdc->osdmap, pool_name);
>>       if (rc<  0)
>>           goto err_out_client;
>> -    rbd_dev->poolid = rc;
>> +    rbd_dev->pool_id = rc;
>>
>>       /* register our block device */
>>       rc = register_blkdev(0, rbd_dev->name);
>> @@ -2453,6 +2469,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>>       rc = rbd_bus_add_dev(rbd_dev);
>>       if (rc)
>>           goto err_out_blkdev;
>> +    kfree(pool_name);
>>
>>       /*
>>        * At this point cleanup in the event of an error is the job
>> @@ -2482,6 +2499,7 @@ err_out_blkdev:
>>   err_out_client:
>>       rbd_put_client(rbd_dev);
>>   err_put_id:
>> +    kfree(pool_name);
>>       rbd_id_put(rbd_dev);
>>   err_nomem:
>>       kfree(options);
> 
> 
> 

Re: [PATCH 08/16] rbd: don't store pool name in struct rbd_dev

[Josh Durgin]

On 07/11/2012 07:59 PM, Alex Elder wrote:
> On 07/11/2012 02:36 PM, Josh Durgin wrote:
>> On 07/11/2012 07:02 AM, Alex Elder wrote:
>>> An rbd device's pool name is used to specify the pool to use for an
>>> rbd image when it gets mapped (via rbd_add()).  However, only the
>>> pool id can be relied on to be invariant--the name of a pool can
>>> change at any time.
>>>
>>> This means that the name of the pool is potentially stale as soon as
>>> the image is mapped, so it's a bad idea to record this information.
>>> So only store the pool id, not its name, in an rbd_dev.
>>>
>>> Here are a few specific notes about this change:
>>>       - The "pool" name device attribute (/sys/bus/rbd/devices/<N>/pool)
>>>         goes away.  In its place is a "pool_id" attribute that provide
>>>         the numeric pool id for the rbd image.
>>
>> We're using the pool name for udev to provide a predictable device name
>> (/dev/rbd/poolname/imagename[@snapname]), so we probably want to keep
>> the sysfs attribute. I don't think there's a good way for us to detect
>> pool renames right now though, so we should document that the pool
>> name reported does not reflect any potential renames.
>
> OK, now that I've thought about this...
>
> I'm going to pull this patch from the series entirely.
> In its place, I'll do a patch that makes the pool name
> get allocated dynamically (like the patches that follow
> this one).  I will also put in place a patch that exposes
> the pool id via the /sys/bus/rbd/devices/<N>/pool_id though.

Sounds good.

> Long term we could switch to using the pool id for the
> predictable device name.  All format 1 images would
> share the same pool (which would need to be assigned a
> reserved pool id different from the empty string).

We could do that. If so, we should use image id as well.
I'm not sure it would be a good idea though. All user (and
management software) interaction with rbd images is done via names,
so if you ever need to reopen an image, like with live migration,
renaming is going to cause the destination to fail to find a renamed
image anyway. In general, renaming things while they're in use
is probably going to be problematic in any case, since management
layers above rbd would need to be aware of the rename as well,
and that's not something any of them support (that I'm aware of).

> Which brings up another thing...
> It would be good to provide an efficient tool to allow
> a customer to convert a format 1 rbd image into format 2.
> It seems like it would mostly involve object renames,
> however they would all have to be done transactionally.

There's no such thing as an object rename, since objects are placed
into pgs based on a hash of their name. A conversion tool wouldn't
make the process more efficient. Instead, you can just export the
format 1 image and import it as a format 2 image.

Josh

>
> 					-Alex
>
>>>       - rbd_add_parse_args() now returns a pointer to a dynamically-
>>>         allocated copy of the pool name taken from the arguments.
>>>       - rbd_add() has been changed to handle freeing the pool name,
>>>         both in error cases and in the normal case after the pool id
>>>         has been recorded.
>>>
>>> Signed-off-by: Alex Elder<elder@inktank.com>
>>> ---
>>>    drivers/block/rbd.c |   74
>>> +++++++++++++++++++++++++++++++-------------------
>>>    1 files changed, 46 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>>> index 3aa0ca0..76e978c 100644
>>> --- a/drivers/block/rbd.c
>>> +++ b/drivers/block/rbd.c
>>> @@ -56,7 +56,6 @@
>>>    #define RBD_MINORS_PER_MAJOR    256        /* max minors per blkdev */
>>>
>>>    #define RBD_MAX_MD_NAME_LEN    (RBD_MAX_OBJ_NAME_LEN +
>>> sizeof(RBD_SUFFIX))
>>> -#define RBD_MAX_POOL_NAME_LEN    64
>>>    #define RBD_MAX_SNAP_NAME_LEN    32
>>>    #define RBD_MAX_OPT_LEN        1024
>>>
>>> @@ -166,8 +165,7 @@ struct rbd_device {
>>>        char            obj[RBD_MAX_OBJ_NAME_LEN]; /* rbd image name */
>>>        int            obj_len;
>>>        char            obj_md_name[RBD_MAX_MD_NAME_LEN]; /* hdr nm. */
>>> -    char            pool_name[RBD_MAX_POOL_NAME_LEN];
>>> -    int            poolid;
>>> +    int            pool_id;
>>>
>>>        struct ceph_osd_event   *watch_event;
>>>        struct ceph_osd_request *watch_request;
>>> @@ -930,7 +928,7 @@ static int rbd_do_request(struct request *rq,
>>>        layout->fl_stripe_unit = cpu_to_le32(1<<  RBD_MAX_OBJ_ORDER);
>>>        layout->fl_stripe_count = cpu_to_le32(1);
>>>        layout->fl_object_size = cpu_to_le32(1<<  RBD_MAX_OBJ_ORDER);
>>> -    layout->fl_pg_pool = cpu_to_le32(dev->poolid);
>>> +    layout->fl_pg_pool = cpu_to_le32(dev->pool_id);
>>>        ceph_calc_raw_layout(osdc, layout, snapid, ofs,&len,&bno,
>>>                    req, ops);
>>>
>>> @@ -1653,7 +1651,7 @@ static int rbd_header_add_snap(struct rbd_device
>>> *dev,
>>>            return -EINVAL;
>>>
>>>        monc =&dev->rbd_client->client->monc;
>>> -    ret = ceph_monc_create_snapid(monc, dev->poolid,&new_snapid);
>>> +    ret = ceph_monc_create_snapid(monc, dev->pool_id,&new_snapid);
>>>        dout("created snapid=%lld\n", new_snapid);
>>>        if (ret<  0)
>>>            return ret;
>>> @@ -1851,12 +1849,12 @@ static ssize_t rbd_client_id_show(struct device
>>> *dev,
>>>                ceph_client_id(rbd_dev->rbd_client->client));
>>>    }
>>>
>>> -static ssize_t rbd_pool_show(struct device *dev,
>>> +static ssize_t rbd_pool_id_show(struct device *dev,
>>>                     struct device_attribute *attr, char *buf)
>>>    {
>>>        struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>>>
>>> -    return sprintf(buf, "%s\n", rbd_dev->pool_name);
>>> +    return sprintf(buf, "%d\n", rbd_dev->pool_id);
>>>    }
>>>
>>>    static ssize_t rbd_name_show(struct device *dev,
>>> @@ -1898,7 +1896,7 @@ static ssize_t rbd_image_refresh(struct device
>>> *dev,
>>>    static DEVICE_ATTR(size, S_IRUGO, rbd_size_show, NULL);
>>>    static DEVICE_ATTR(major, S_IRUGO, rbd_major_show, NULL);
>>>    static DEVICE_ATTR(client_id, S_IRUGO, rbd_client_id_show, NULL);
>>> -static DEVICE_ATTR(pool, S_IRUGO, rbd_pool_show, NULL);
>>> +static DEVICE_ATTR(pool_id, S_IRUGO, rbd_pool_id_show, NULL);
>>>    static DEVICE_ATTR(name, S_IRUGO, rbd_name_show, NULL);
>>>    static DEVICE_ATTR(refresh, S_IWUSR, NULL, rbd_image_refresh);
>>>    static DEVICE_ATTR(current_snap, S_IRUGO, rbd_snap_show, NULL);
>>> @@ -1908,7 +1906,7 @@ static struct attribute *rbd_attrs[] = {
>>>        &dev_attr_size.attr,
>>>        &dev_attr_major.attr,
>>>        &dev_attr_client_id.attr,
>>> -    &dev_attr_pool.attr,
>>> +    &dev_attr_pool_id.attr,
>>>        &dev_attr_name.attr,
>>>        &dev_attr_current_snap.attr,
>>>        &dev_attr_refresh.attr,
>>> @@ -2329,25 +2327,31 @@ static inline char *dup_token(const char **buf,
>>> size_t *lenp)
>>>    }
>>>
>>>    /*
>>> - * This fills in the pool_name, obj, obj_len, snap_name, obj_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.
>>> + * This fills in the obj, obj_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.
>>> + *
>>> + * Returns a pointer to a dynamically-allocated buffer containing
>>> + * the pool name provided, or a pointer-coded errno if an error
>>> + * occurs.
>>>     */
>>> -static int rbd_add_parse_args(struct rbd_device *rbd_dev,
>>> +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;
>>> +    size_t len;
>>> +    int ret = -EINVAL;
>>> +    char *pool_name = NULL;
>>>
>>>        /* The first four tokens are required */
>>>
>>>        len = next_token(&buf);
>>>        if (!len)
>>> -        return -EINVAL;
>>> +        goto out_err;
>>> +
>>>        *mon_addrs_size = len + 1;
>>>        *mon_addrs = buf;
>>>
>>> @@ -2355,15 +2359,17 @@ 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;
>>> +        goto out_err;
>>>
>>> -    len = copy_token(&buf, rbd_dev->pool_name, sizeof
>>> (rbd_dev->pool_name));
>>> -    if (!len || len>= sizeof (rbd_dev->pool_name))
>>> -        return -EINVAL;
>>> +    pool_name = dup_token(&buf, NULL);
>>> +    if (!pool_name) {
>>> +        ret = -ENOMEM;
>>> +        goto out_err;
>>> +    }
>>>
>>>        len = copy_token(&buf, rbd_dev->obj, sizeof (rbd_dev->obj));
>>>        if (!len || len>= sizeof (rbd_dev->obj))
>>> -        return -EINVAL;
>>> +        goto out_err;
>>>
>>>        /* We have the object length in hand, save it. */
>>>
>>> @@ -2382,9 +2388,14 @@ static int rbd_add_parse_args(struct rbd_device
>>> *rbd_dev,
>>>            memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>>>                sizeof (RBD_SNAP_HEAD_NAME));
>>>        else if (len>= sizeof (rbd_dev->snap_name))
>>> -        return -EINVAL;
>>> +        goto out_err;
>>>
>>> -    return 0;
>>> +    return pool_name;
>>> +
>>> +out_err:
>>> +    kfree(pool_name);
>>> +
>>> +    return ERR_PTR(ret);
>>>    }
>>>
>>>    static ssize_t rbd_add(struct bus_type *bus,
>>> @@ -2396,6 +2407,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>>>        size_t mon_addrs_size = 0;
>>>        char *options = NULL;
>>>        struct ceph_osd_client *osdc;
>>> +    char *pool_name;
>>>        int rc = -ENOMEM;
>>>
>>>        if (!try_module_get(THIS_MODULE))
>>> @@ -2425,10 +2437,14 @@ static ssize_t rbd_add(struct bus_type *bus,
>>>        sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->id);
>>>
>>>        /* parse add command */
>>> -    rc = rbd_add_parse_args(rbd_dev, buf,&mon_addrs,&mon_addrs_size,
>>> -                options, count);
>>> -    if (rc)
>>> +    pool_name = rbd_add_parse_args(rbd_dev, buf,
>>> +                    &mon_addrs,&mon_addrs_size,
>>> +                    options, count);
>>> +    if (IS_ERR(pool_name)) {
>>> +        rc = PTR_ERR(pool_name);
>>> +        pool_name = NULL;
>>>            goto err_put_id;
>>> +    }
>>>
>>>        rbd_dev->rbd_client = rbd_get_client(mon_addrs, mon_addrs_size - 1,
>>>                            options);
>>> @@ -2439,10 +2455,10 @@ static ssize_t rbd_add(struct bus_type *bus,
>>>
>>>        /* pick the pool */
>>>        osdc =&rbd_dev->rbd_client->client->osdc;
>>> -    rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name);
>>> +    rc = ceph_pg_poolid_by_name(osdc->osdmap, pool_name);
>>>        if (rc<  0)
>>>            goto err_out_client;
>>> -    rbd_dev->poolid = rc;
>>> +    rbd_dev->pool_id = rc;
>>>
>>>        /* register our block device */
>>>        rc = register_blkdev(0, rbd_dev->name);
>>> @@ -2453,6 +2469,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>>>        rc = rbd_bus_add_dev(rbd_dev);
>>>        if (rc)
>>>            goto err_out_blkdev;
>>> +    kfree(pool_name);
>>>
>>>        /*
>>>         * At this point cleanup in the event of an error is the job
>>> @@ -2482,6 +2499,7 @@ err_out_blkdev:
>>>    err_out_client:
>>>        rbd_put_client(rbd_dev);
>>>    err_put_id:
>>> +    kfree(pool_name);
>>>        rbd_id_put(rbd_dev);
>>>    err_nomem:
>>>        kfree(options);
>>
>>
>>
>
>

Re: [PATCH 10/16] rbd: dynamically allocate image name

[Alex Elder]

On 07/11/2012 03:52 PM, Josh Durgin wrote:
>>> - int obj_len;
>>> + char *obj; /* rbd image name */
>>> + size_t obj_len;
> 
> On second look, obj_len can be a local variable. It's not used
> outside of option parsing.

I use it in a patch later--in some changes I have not yet posted.

					-Alex

Re: [PATCH 13/16] rbd: rename some fields in struct rbd_dev

[Alex Elder]

On 07/11/2012 04:01 PM, Josh Durgin wrote:
>> Also rename a related symbol name:
>>      RBD_MAX_MD_NAME_LEN -->  RBD_MAX_HEADER_NAME_LEN
> 
> Looks like this got removed in patch 9.

Yes you're right.  I forgot to update the comment when I
reordered the patches.  Thanks.

					-Alex

Re: [PATCH 14/16] rbd: more symbol renames

[Alex Elder]

On 07/11/2012 04:03 PM, Josh Durgin wrote:
> On 07/11/2012 07:02 AM, Alex Elder wrote:
>> Rename variables named "obj" which represent object names so they're
>> consistenly named "object_name".
> 
> typo
> 
> 
Fixed.

Re: [PATCH 08/16] rbd: don't store pool name in struct rbd_dev

[Alex Elder]

I said I would pull this patch and replace it with two others--one
that just creates an entry in /sys/bus/rbd/devices/<N>/pool_id,
and one that makes the pool name get allocated dynamically.  I'm
about to post those for review.  They fit in about the same place
in the original series but I'm not going to pollute the list by
re-posting everything again.

					-Alex

[PATCH] rbd: create pool_id device attribute

[Alex Elder]

Add an entry under /sys/bus/rbd/devices/<N>/ named "pool_id" that
provides the id for the pool the rbd image is assocatied with.  This
is in addition to the pool name already provided.

Rename the "poolid" field in struct rbd_device  to be "pool_id".

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

Index: b/drivers/block/rbd.c
===================================================================
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -167,7 +167,7 @@ struct rbd_device {
 	int			obj_len;
 	char			obj_md_name[RBD_MAX_MD_NAME_LEN]; /* hdr nm. */
 	char			pool_name[RBD_MAX_POOL_NAME_LEN];
-	int			poolid;
+	int			pool_id;

 	struct ceph_osd_event   *watch_event;
 	struct ceph_osd_request *watch_request;
@@ -920,7 +920,7 @@ static int rbd_do_request(struct request
 	layout->fl_stripe_unit = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
 	layout->fl_stripe_count = cpu_to_le32(1);
 	layout->fl_object_size = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
-	layout->fl_pg_pool = cpu_to_le32(dev->poolid);
+	layout->fl_pg_pool = cpu_to_le32(dev->pool_id);
 	ceph_calc_raw_layout(osdc, layout, snapid, ofs, &len, &bno,
 				req, ops);

@@ -1643,7 +1643,7 @@ static int rbd_header_add_snap(struct rb
 		return -EINVAL;

 	monc = &dev->rbd_client->client->monc;
-	ret = ceph_monc_create_snapid(monc, dev->poolid, &new_snapid);
+	ret = ceph_monc_create_snapid(monc, dev->pool_id, &new_snapid);
 	dout("created snapid=%lld\n", new_snapid);
 	if (ret < 0)
 		return ret;
@@ -1847,6 +1847,14 @@ static ssize_t rbd_pool_show(struct devi
 	return sprintf(buf, "%s\n", rbd_dev->pool_name);
 }

+static ssize_t rbd_pool_id_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
+
+	return sprintf(buf, "%d\n", rbd_dev->pool_id);
+}
+
 static ssize_t rbd_name_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
@@ -1887,6 +1895,7 @@ static DEVICE_ATTR(size, S_IRUGO, rbd_si
 static DEVICE_ATTR(major, S_IRUGO, rbd_major_show, NULL);
 static DEVICE_ATTR(client_id, S_IRUGO, rbd_client_id_show, NULL);
 static DEVICE_ATTR(pool, S_IRUGO, rbd_pool_show, NULL);
+static DEVICE_ATTR(pool_id, S_IRUGO, rbd_pool_id_show, NULL);
 static DEVICE_ATTR(name, S_IRUGO, rbd_name_show, NULL);
 static DEVICE_ATTR(refresh, S_IWUSR, NULL, rbd_image_refresh);
 static DEVICE_ATTR(current_snap, S_IRUGO, rbd_snap_show, NULL);
@@ -1897,6 +1906,7 @@ static struct attribute *rbd_attrs[] = {
 	&dev_attr_major.attr,
 	&dev_attr_client_id.attr,
 	&dev_attr_pool.attr,
+	&dev_attr_pool_id.attr,
 	&dev_attr_name.attr,
 	&dev_attr_current_snap.attr,
 	&dev_attr_refresh.attr,
@@ -2430,7 +2440,7 @@ static ssize_t rbd_add(struct bus_type *
 	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name);
 	if (rc < 0)
 		goto err_out_client;
-	rbd_dev->poolid = rc;
+	rbd_dev->pool_id = rc;

 	/* register our block device */
 	rc = register_blkdev(0, rbd_dev->name);

[PATCH] rbd: dynamically allocate pool name

[Alex Elder]

There is no need to impose a small limit the length of the pool name
recorded for an rbd image in a struct rbd_device.  Remove the
limitation by allocating space for the pool name ynamically.

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

Index: b/drivers/block/rbd.c
===================================================================
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -56,7 +56,6 @@
 #define RBD_MINORS_PER_MAJOR	256		/* max minors per blkdev */

 #define RBD_MAX_MD_NAME_LEN	(RBD_MAX_OBJ_NAME_LEN + sizeof(RBD_SUFFIX))
-#define RBD_MAX_POOL_NAME_LEN	64
 #define RBD_MAX_SNAP_NAME_LEN	32
 #define RBD_MAX_OPT_LEN		1024

@@ -166,7 +165,7 @@ struct rbd_device {
 	char			obj[RBD_MAX_OBJ_NAME_LEN]; /* rbd image name */
 	int			obj_len;
 	char			obj_md_name[RBD_MAX_MD_NAME_LEN]; /* hdr nm. */
-	char			pool_name[RBD_MAX_POOL_NAME_LEN];
+	char			*pool_name;
 	int			pool_id;

 	struct ceph_osd_event   *watch_event;
@@ -2331,6 +2330,8 @@ static inline char *dup_token(const char
  * 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.
+ *
+ * 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,
@@ -2339,7 +2340,8 @@ static int rbd_add_parse_args(struct rbd
 			      char *options,
 			      size_t options_size)
 {
-	size_t	len;
+	size_t len;
+	int ret;

 	/* The first four tokens are required */

@@ -2355,13 +2357,14 @@ static int rbd_add_parse_args(struct rbd
 	if (!len || len >= options_size)
 		return -EINVAL;

-	len = copy_token(&buf, rbd_dev->pool_name, sizeof (rbd_dev->pool_name));
-	if (!len || len >= sizeof (rbd_dev->pool_name))
-		return -EINVAL;
+	rbd_dev->pool_name = dup_token(&buf, NULL);
+	if (!rbd_dev->pool_name)
+		return -ENOMEM;

+	ret = -EINVAL;
 	len = copy_token(&buf, rbd_dev->obj, sizeof (rbd_dev->obj));
 	if (!len || len >= sizeof (rbd_dev->obj))
-		return -EINVAL;
+		goto out_err;

 	/* We have the object length in hand, save it. */

@@ -2380,9 +2383,15 @@ static int rbd_add_parse_args(struct rbd
 		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
 			sizeof (RBD_SNAP_HEAD_NAME));
 	else if (len >= sizeof (rbd_dev->snap_name))
-		return -EINVAL;
+		goto out_err;

 	return 0;
+
+out_err:
+	kfree(rbd_dev->pool_name);
+	rbd_dev->pool_name = NULL;
+
+	return ret;
 }

 static ssize_t rbd_add(struct bus_type *bus,
@@ -2480,6 +2489,7 @@ err_out_blkdev:
 err_out_client:
 	rbd_put_client(rbd_dev);
 err_put_id:
+	kfree(rbd_dev->pool_name);
 	rbd_id_put(rbd_dev);
 err_nomem:
 	kfree(options);
@@ -2528,6 +2538,7 @@ static void rbd_dev_release(struct devic
 	unregister_blkdev(rbd_dev->major, rbd_dev->name);

 	/* done with the id, and with the rbd_dev */
+	kfree(rbd_dev->pool_name);
 	rbd_id_put(rbd_dev);
 	kfree(rbd_dev);

Re: [PATCH v2 03/16] libceph: define ceph_decode_string()

[Alex Elder]

OK, in a side discussion, Sage convinced me that since the only
user of this function would be ceph_extract_encoded_string()
(introduced in the next patch), and that under normal circumstances
that function most likely implements the most likely legitimate
use of this function, there is really no need for this function
to stand by itself.

So I'm retracting this patch, and am re-posting the next one,
which open-codes the functionality of ceph_decode_string()
directly inside ceph_extract_encoded_string().

					-Alex

[PATCH v3 04/16] libceph: define ceph_extract_encoded_string()

[Alex Elder]

This adds a new utility routine which will return a dynamically-
allocated buffer containing a string that has been decoded from ceph
over-the-wire format.  It also returns the length of the string
if the address of a size variable is supplied to receive it.

Signed-off-by: Alex Elder <elder@inktank.com>
---
v3: Final version.  Sage convinced me that there was no need for
    ceph_decode_string() other than its use in this function, so
    now this implements what that function had been doing directly.

 include/linux/ceph/decode.h |   44
++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

Index: b/include/linux/ceph/decode.h
===================================================================
--- a/include/linux/ceph/decode.h
+++ b/include/linux/ceph/decode.h
@@ -85,6 +85,50 @@ static inline int ceph_has_room(void **p
 	} while (0)

 /*
+ * Allocate a buffer big enough to hold the wire-encoded string, and
+ * decode the string into it.  The resulting string will always be
+ * terminated with '\0'.  If successful, *p will be advanced
+ * past the decoded data.  Also, if lenp is not a null pointer, the
+ * length (not including the terminating '\0') will be recorded in
+ * *lenp.  Note that a zero-length string is a valid return value.
+ *
+ * Returns a pointer to the newly-allocated string buffer, or a null
+ * pointer if an error occurs.  Neither *p nor *lenp will have been
+ * updated if NULL is returned.
+ *
+ * There are two possible failures:
+ *   - converting the string would require accessing memory at or
+ *     beyond the "end" pointer provided
+ *   - memory could not be allocated for the result
+ */
+static inline char *ceph_extract_encoded_string(void **p, void *end,
+						size_t *lenp, gfp_t gfp)
+{
+	u32 len;
+	void *sp = *p;
+	char *buf = NULL;
+
+	ceph_decode_32_safe(&sp, end, len, out);
+	if (!ceph_has_room(&sp, end, len))
+		return NULL;
+
+	buf = kmalloc(len + 1, gfp);
+	if (!buf)
+		return NULL;
+
+	if (len)
+		memcpy(buf, sp, len);
+	buf[len] = '\0';
+
+	*p = (char *) *p + sizeof (u32) + len;
+
+	if (lenp)
+		*lenp = (size_t) len;
+out:
+	return buf;
+}
+
+/*
  * struct ceph_timespec <-> struct timespec
  */
 static inline void ceph_decode_timespec(struct timespec *ts,

Re: [PATCH] rbd: create pool_id device attribute

[Josh Durgin]

Looks good.

I forgot to mention that this should also update the sysfs
documentation at Documentation/ABI/testing/sysfs-bus-rbd.

On 07/12/2012 10:05 AM, Alex Elder wrote:
> Add an entry under /sys/bus/rbd/devices/<N>/ named "pool_id" that
> provides the id for the pool the rbd image is assocatied with.  This
> is in addition to the pool name already provided.
>
> Rename the "poolid" field in struct rbd_device  to be "pool_id".
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c |   18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
>
> Index: b/drivers/block/rbd.c
> ===================================================================
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -167,7 +167,7 @@ struct rbd_device {
>   	int			obj_len;
>   	char			obj_md_name[RBD_MAX_MD_NAME_LEN]; /* hdr nm. */
>   	char			pool_name[RBD_MAX_POOL_NAME_LEN];
> -	int			poolid;
> +	int			pool_id;
>
>   	struct ceph_osd_event   *watch_event;
>   	struct ceph_osd_request *watch_request;
> @@ -920,7 +920,7 @@ static int rbd_do_request(struct request
>   	layout->fl_stripe_unit = cpu_to_le32(1<<  RBD_MAX_OBJ_ORDER);
>   	layout->fl_stripe_count = cpu_to_le32(1);
>   	layout->fl_object_size = cpu_to_le32(1<<  RBD_MAX_OBJ_ORDER);
> -	layout->fl_pg_pool = cpu_to_le32(dev->poolid);
> +	layout->fl_pg_pool = cpu_to_le32(dev->pool_id);
>   	ceph_calc_raw_layout(osdc, layout, snapid, ofs,&len,&bno,
>   				req, ops);
>
> @@ -1643,7 +1643,7 @@ static int rbd_header_add_snap(struct rb
>   		return -EINVAL;
>
>   	monc =&dev->rbd_client->client->monc;
> -	ret = ceph_monc_create_snapid(monc, dev->poolid,&new_snapid);
> +	ret = ceph_monc_create_snapid(monc, dev->pool_id,&new_snapid);
>   	dout("created snapid=%lld\n", new_snapid);
>   	if (ret<  0)
>   		return ret;
> @@ -1847,6 +1847,14 @@ static ssize_t rbd_pool_show(struct devi
>   	return sprintf(buf, "%s\n", rbd_dev->pool_name);
>   }
>
> +static ssize_t rbd_pool_id_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
> +
> +	return sprintf(buf, "%d\n", rbd_dev->pool_id);
> +}
> +
>   static ssize_t rbd_name_show(struct device *dev,
>   			     struct device_attribute *attr, char *buf)
>   {
> @@ -1887,6 +1895,7 @@ static DEVICE_ATTR(size, S_IRUGO, rbd_si
>   static DEVICE_ATTR(major, S_IRUGO, rbd_major_show, NULL);
>   static DEVICE_ATTR(client_id, S_IRUGO, rbd_client_id_show, NULL);
>   static DEVICE_ATTR(pool, S_IRUGO, rbd_pool_show, NULL);
> +static DEVICE_ATTR(pool_id, S_IRUGO, rbd_pool_id_show, NULL);
>   static DEVICE_ATTR(name, S_IRUGO, rbd_name_show, NULL);
>   static DEVICE_ATTR(refresh, S_IWUSR, NULL, rbd_image_refresh);
>   static DEVICE_ATTR(current_snap, S_IRUGO, rbd_snap_show, NULL);
> @@ -1897,6 +1906,7 @@ static struct attribute *rbd_attrs[] = {
>   	&dev_attr_major.attr,
>   	&dev_attr_client_id.attr,
>   	&dev_attr_pool.attr,
> +	&dev_attr_pool_id.attr,
>   	&dev_attr_name.attr,
>   	&dev_attr_current_snap.attr,
>   	&dev_attr_refresh.attr,
> @@ -2430,7 +2440,7 @@ static ssize_t rbd_add(struct bus_type *
>   	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name);
>   	if (rc<  0)
>   		goto err_out_client;
> -	rbd_dev->poolid = rc;
> +	rbd_dev->pool_id = rc;
>
>   	/* register our block device */
>   	rc = register_blkdev(0, rbd_dev->name);
> --
> 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] rbd: dynamically allocate pool name

[Josh Durgin]

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

On 07/12/2012 10:05 AM, Alex Elder wrote:
> There is no need to impose a small limit the length of the pool name
> recorded for an rbd image in a struct rbd_device.  Remove the
> limitation by allocating space for the pool name ynamically.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> ---
>   drivers/block/rbd.c |   27 +++++++++++++++++++--------
>   1 file changed, 19 insertions(+), 8 deletions(-)
>
> Index: b/drivers/block/rbd.c
> ===================================================================
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -56,7 +56,6 @@
>   #define RBD_MINORS_PER_MAJOR	256		/* max minors per blkdev */
>
>   #define RBD_MAX_MD_NAME_LEN	(RBD_MAX_OBJ_NAME_LEN + sizeof(RBD_SUFFIX))
> -#define RBD_MAX_POOL_NAME_LEN	64
>   #define RBD_MAX_SNAP_NAME_LEN	32
>   #define RBD_MAX_OPT_LEN		1024
>
> @@ -166,7 +165,7 @@ struct rbd_device {
>   	char			obj[RBD_MAX_OBJ_NAME_LEN]; /* rbd image name */
>   	int			obj_len;
>   	char			obj_md_name[RBD_MAX_MD_NAME_LEN]; /* hdr nm. */
> -	char			pool_name[RBD_MAX_POOL_NAME_LEN];
> +	char			*pool_name;
>   	int			pool_id;
>
>   	struct ceph_osd_event   *watch_event;
> @@ -2331,6 +2330,8 @@ static inline char *dup_token(const char
>    * 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.
> + *
> + * 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,
> @@ -2339,7 +2340,8 @@ static int rbd_add_parse_args(struct rbd
>   			      char *options,
>   			      size_t options_size)
>   {
> -	size_t	len;
> +	size_t len;
> +	int ret;
>
>   	/* The first four tokens are required */
>
> @@ -2355,13 +2357,14 @@ static int rbd_add_parse_args(struct rbd
>   	if (!len || len>= options_size)
>   		return -EINVAL;
>
> -	len = copy_token(&buf, rbd_dev->pool_name, sizeof (rbd_dev->pool_name));
> -	if (!len || len>= sizeof (rbd_dev->pool_name))
> -		return -EINVAL;
> +	rbd_dev->pool_name = dup_token(&buf, NULL);
> +	if (!rbd_dev->pool_name)
> +		return -ENOMEM;
>
> +	ret = -EINVAL;
>   	len = copy_token(&buf, rbd_dev->obj, sizeof (rbd_dev->obj));
>   	if (!len || len>= sizeof (rbd_dev->obj))
> -		return -EINVAL;
> +		goto out_err;
>
>   	/* We have the object length in hand, save it. */
>
> @@ -2380,9 +2383,15 @@ static int rbd_add_parse_args(struct rbd
>   		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>   			sizeof (RBD_SNAP_HEAD_NAME));
>   	else if (len>= sizeof (rbd_dev->snap_name))
> -		return -EINVAL;
> +		goto out_err;
>
>   	return 0;
> +
> +out_err:
> +	kfree(rbd_dev->pool_name);
> +	rbd_dev->pool_name = NULL;
> +
> +	return ret;
>   }
>
>   static ssize_t rbd_add(struct bus_type *bus,
> @@ -2480,6 +2489,7 @@ err_out_blkdev:
>   err_out_client:
>   	rbd_put_client(rbd_dev);
>   err_put_id:
> +	kfree(rbd_dev->pool_name);
>   	rbd_id_put(rbd_dev);
>   err_nomem:
>   	kfree(options);
> @@ -2528,6 +2538,7 @@ static void rbd_dev_release(struct devic
>   	unregister_blkdev(rbd_dev->major, rbd_dev->name);
>
>   	/* done with the id, and with the rbd_dev */
> +	kfree(rbd_dev->pool_name);
>   	rbd_id_put(rbd_dev);
>   	kfree(rbd_dev);
>
> --
> 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

[PATCH v2 07/16] rbd: dynamically allocate object prefix

[Alex Elder]

There is no need to impose a small limit the length of the object
prefix recorded for an rbd image in a struct rbd_image_header.
Remove the limitation by allocating space for the object prefix
dynamically.

Signed-off-by: Alex Elder <elder@inktank.com>
v2: The object prefix shouldn't change when the header is refreshed.
    Verify that (but still free the extra copy that gets allocated
    when the header is re-read).
---
 drivers/block/rbd.c |   34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

Index: b/drivers/block/rbd.c
===================================================================
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -77,7 +77,7 @@
  */
 struct rbd_image_header {
 	u64 image_size;
-	char object_prefix[32];
+	char *object_prefix;
 	__u8 obj_order;
 	__u8 crypt_type;
 	__u8 comp_type;
@@ -517,8 +517,15 @@ static int rbd_header_from_disk(struct r
 		header->snap_names = NULL;
 		header->snap_sizes = NULL;
 	}
+
+	header->object_prefix = kmalloc(sizeof (ondisk->block_name) + 1,
+					gfp_flags);
+	if (!header->object_prefix)
+		goto err_sizes;
+
 	memcpy(header->object_prefix, ondisk->block_name,
 	       sizeof(ondisk->block_name));
+	header->object_prefix[sizeof (ondisk->block_name)] = '\0';

 	header->image_size = le64_to_cpu(ondisk->image_size);
 	header->obj_order = ondisk->options.order;
@@ -545,6 +552,8 @@ static int rbd_header_from_disk(struct r

 	return 0;

+err_sizes:
+	kfree(header->snap_sizes);
 err_names:
 	kfree(header->snap_names);
 err_snapc:
@@ -610,9 +619,10 @@ done:

 static void rbd_header_free(struct rbd_image_header *header)
 {
-	kfree(header->snapc);
-	kfree(header->snap_names);
+	kfree(header->object_prefix);
 	kfree(header->snap_sizes);
+	kfree(header->snap_names);
+	kfree(header->snapc);
 }

 /*
@@ -1710,15 +1720,20 @@ static int __rbd_refresh_header(struct r
 		   if head moves */
 		follow_seq = 1;

-	kfree(rbd_dev->header.snapc);
-	kfree(rbd_dev->header.snap_names);
+	/* rbd_dev->header.object_prefix shouldn't change */
 	kfree(rbd_dev->header.snap_sizes);
+	kfree(rbd_dev->header.snap_names);
+	kfree(rbd_dev->header.snapc);

 	rbd_dev->header.total_snaps = h.total_snaps;
 	rbd_dev->header.snapc = h.snapc;
 	rbd_dev->header.snap_names = h.snap_names;
 	rbd_dev->header.snap_names_len = h.snap_names_len;
 	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));
+	kfree(h.object_prefix);
+
 	if (follow_seq)
 		rbd_dev->header.snapc->seq = rbd_dev->header.snapc->snaps[0];
 	else
@@ -2361,10 +2376,11 @@ static int rbd_add_parse_args(struct rbd
 	if (!rbd_dev->pool_name)
 		return -ENOMEM;

-	ret = -EINVAL;
 	len = copy_token(&buf, rbd_dev->obj, sizeof (rbd_dev->obj));
-	if (!len || len >= sizeof (rbd_dev->obj))
+	if (!len || len >= sizeof (rbd_dev->obj)) {
+		ret = -EINVAL;
 		goto out_err;
+	}

 	/* We have the object length in hand, save it. */

@@ -2382,8 +2398,10 @@ static int rbd_add_parse_args(struct rbd
 	if (!len)
 		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
 			sizeof (RBD_SNAP_HEAD_NAME));
-	else if (len >= sizeof (rbd_dev->snap_name))
+	else if (len >= sizeof (rbd_dev->snap_name)) {
+		ret = -EINVAL;
 		goto out_err;
+	}

 	return 0;

Re: [PATCH] rbd: create pool_id device attribute

[Alex Elder]

On 07/12/2012 12:16 PM, Josh Durgin wrote:
> Looks good.
> 
> I forgot to mention that this should also update the sysfs
> documentation at Documentation/ABI/testing/sysfs-bus-rbd.

Thanks.  I'll do that as a separate patch, but will get it
posted for review later this afternoon and will commit it
to the testing branch along with these changes.

					-Alex

Re: [PATCH v2 07/16] rbd: dynamically allocate object prefix

[Josh Durgin]

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

On 07/12/2012 10:24 AM, Alex Elder wrote:
> There is no need to impose a small limit the length of the object
> prefix recorded for an rbd image in a struct rbd_image_header.
> Remove the limitation by allocating space for the object prefix
> dynamically.
>
> Signed-off-by: Alex Elder<elder@inktank.com>
> v2: The object prefix shouldn't change when the header is refreshed.
>      Verify that (but still free the extra copy that gets allocated
>      when the header is re-read).
> ---
>   drivers/block/rbd.c |   34 ++++++++++++++++++++++++++--------
>   1 file changed, 26 insertions(+), 8 deletions(-)
>
> Index: b/drivers/block/rbd.c
> ===================================================================
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -77,7 +77,7 @@
>    */
>   struct rbd_image_header {
>   	u64 image_size;
> -	char object_prefix[32];
> +	char *object_prefix;
>   	__u8 obj_order;
>   	__u8 crypt_type;
>   	__u8 comp_type;
> @@ -517,8 +517,15 @@ static int rbd_header_from_disk(struct r
>   		header->snap_names = NULL;
>   		header->snap_sizes = NULL;
>   	}
> +
> +	header->object_prefix = kmalloc(sizeof (ondisk->block_name) + 1,
> +					gfp_flags);
> +	if (!header->object_prefix)
> +		goto err_sizes;
> +
>   	memcpy(header->object_prefix, ondisk->block_name,
>   	       sizeof(ondisk->block_name));
> +	header->object_prefix[sizeof (ondisk->block_name)] = '\0';
>
>   	header->image_size = le64_to_cpu(ondisk->image_size);
>   	header->obj_order = ondisk->options.order;
> @@ -545,6 +552,8 @@ static int rbd_header_from_disk(struct r
>
>   	return 0;
>
> +err_sizes:
> +	kfree(header->snap_sizes);
>   err_names:
>   	kfree(header->snap_names);
>   err_snapc:
> @@ -610,9 +619,10 @@ done:
>
>   static void rbd_header_free(struct rbd_image_header *header)
>   {
> -	kfree(header->snapc);
> -	kfree(header->snap_names);
> +	kfree(header->object_prefix);
>   	kfree(header->snap_sizes);
> +	kfree(header->snap_names);
> +	kfree(header->snapc);
>   }
>
>   /*
> @@ -1710,15 +1720,20 @@ static int __rbd_refresh_header(struct r
>   		   if head moves */
>   		follow_seq = 1;
>
> -	kfree(rbd_dev->header.snapc);
> -	kfree(rbd_dev->header.snap_names);
> +	/* rbd_dev->header.object_prefix shouldn't change */
>   	kfree(rbd_dev->header.snap_sizes);
> +	kfree(rbd_dev->header.snap_names);
> +	kfree(rbd_dev->header.snapc);
>
>   	rbd_dev->header.total_snaps = h.total_snaps;
>   	rbd_dev->header.snapc = h.snapc;
>   	rbd_dev->header.snap_names = h.snap_names;
>   	rbd_dev->header.snap_names_len = h.snap_names_len;
>   	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));
> +	kfree(h.object_prefix);
> +
>   	if (follow_seq)
>   		rbd_dev->header.snapc->seq = rbd_dev->header.snapc->snaps[0];
>   	else
> @@ -2361,10 +2376,11 @@ static int rbd_add_parse_args(struct rbd
>   	if (!rbd_dev->pool_name)
>   		return -ENOMEM;
>
> -	ret = -EINVAL;
>   	len = copy_token(&buf, rbd_dev->obj, sizeof (rbd_dev->obj));
> -	if (!len || len>= sizeof (rbd_dev->obj))
> +	if (!len || len>= sizeof (rbd_dev->obj)) {
> +		ret = -EINVAL;
>   		goto out_err;
> +	}
>
>   	/* We have the object length in hand, save it. */
>
> @@ -2382,8 +2398,10 @@ static int rbd_add_parse_args(struct rbd
>   	if (!len)
>   		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>   			sizeof (RBD_SNAP_HEAD_NAME));
> -	else if (len>= sizeof (rbd_dev->snap_name))
> +	else if (len>= sizeof (rbd_dev->snap_name)) {
> +		ret = -EINVAL;
>   		goto out_err;
> +	}
>
>   	return 0;
>
> --
> 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 v3 04/16] libceph: define ceph_extract_encoded_string()

[Sage Weil]

On Thu, 12 Jul 2012, Alex Elder wrote:
> This adds a new utility routine which will return a dynamically-
> allocated buffer containing a string that has been decoded from ceph
> over-the-wire format.  It also returns the length of the string
> if the address of a size variable is supplied to receive it.
> 
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
> v3: Final version.  Sage convinced me that there was no need for
>     ceph_decode_string() other than its use in this function, so
>     now this implements what that function had been doing directly.
> 
>  include/linux/ceph/decode.h |   44
> ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> Index: b/include/linux/ceph/decode.h
> ===================================================================
> --- a/include/linux/ceph/decode.h
> +++ b/include/linux/ceph/decode.h
> @@ -85,6 +85,50 @@ static inline int ceph_has_room(void **p
>  	} while (0)
> 
>  /*
> + * Allocate a buffer big enough to hold the wire-encoded string, and
> + * decode the string into it.  The resulting string will always be
> + * terminated with '\0'.  If successful, *p will be advanced
> + * past the decoded data.  Also, if lenp is not a null pointer, the
> + * length (not including the terminating '\0') will be recorded in
> + * *lenp.  Note that a zero-length string is a valid return value.
> + *
> + * Returns a pointer to the newly-allocated string buffer, or a null
> + * pointer if an error occurs.  Neither *p nor *lenp will have been
> + * updated if NULL is returned.
> + *
> + * There are two possible failures:
> + *   - converting the string would require accessing memory at or
> + *     beyond the "end" pointer provided
> + *   - memory could not be allocated for the result

I think the caller should be able to distinguish between these two cases 
in the return value.  What about:

> + */
> +static inline char *ceph_extract_encoded_string(void **p, void *end,
> +						size_t *lenp, gfp_t gfp)


int ceph_extract_encoded_string(void **p, void *end, char **str, gfp_t gfp)

and return the length, or an error code?  That avoids futzing with ERR_PTR 
and gives you the len a bit less awkwardly...

sage


> +{
> +	u32 len;
> +	void *sp = *p;
> +	char *buf = NULL;
> +
> +	ceph_decode_32_safe(&sp, end, len, out);
> +	if (!ceph_has_room(&sp, end, len))
> +		return NULL;
> +
> +	buf = kmalloc(len + 1, gfp);
> +	if (!buf)
> +		return NULL;
> +
> +	if (len)
> +		memcpy(buf, sp, len);
> +	buf[len] = '\0';
> +
> +	*p = (char *) *p + sizeof (u32) + len;
> +
> +	if (lenp)
> +		*lenp = (size_t) len;
> +out:
> +	return buf;
> +}
> +
> +/*
>   * struct ceph_timespec <-> struct timespec
>   */
>  static inline void ceph_decode_timespec(struct timespec *ts,
> --
> 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 v3 04/16] libceph: define ceph_extract_encoded_string()

[Alex Elder]

On 07/12/2012 01:20 PM, Sage Weil wrote:
> I think the caller should be able to distinguish between these two cases 
> in the return value. 

OK.  I'll adjust and repost.	-Alex

Re: [PATCH v3 04/16] libceph: define ceph_extract_encoded_string()

[Alex Elder]

On 07/12/2012 01:20 PM, Sage Weil wrote:
> I think the caller should be able to distinguish between these two cases 
> in the return value.  What about:
> 
>> > + */
>> > +static inline char *ceph_extract_encoded_string(void **p, void *end,
>> > +						size_t *lenp, gfp_t gfp)
> 
> int ceph_extract_encoded_string(void **p, void *end, char **str, gfp_t gfp)
> 
> and return the length, or an error code?  That avoids futzing with ERR_PTR 
> and gives you the len a bit less awkwardly...
> 

I opted to stick with my original prototype--returning the string--and
using the ERR_PTR() etc.  I return the string because I *always* want
that.  Only sometimes is the length needed.

					-Alex

[PATCH v4 04/16] libceph: define ceph_extract_encoded_string()

[Alex Elder]

This adds a new utility routine which will return a dynamically-
allocated buffer containing a string that has been decoded from ceph
over-the-wire format.  It also returns the length of the string
if the address of a size variable is supplied to receive it.

Signed-off-by: Alex Elder <elder@inktank.com>
---
v4: Last final version.  Caller can now distinguish between the two
    error conditions.
v3: Final version.  Sage convinced me that there was no need for
    ceph_decode_string() other than its use in this function, so
    now this implements what that function had been doing directly.
v2: Made the function safe from overrunning the source memory, and
    and added a gfp argument to pass to kmalloc().

 include/linux/ceph/decode.h |   47
+++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
index bcbd66c..4bbf2db 100644
--- a/include/linux/ceph/decode.h
+++ b/include/linux/ceph/decode.h
@@ -1,6 +1,7 @@
 #ifndef __CEPH_DECODE_H
 #define __CEPH_DECODE_H

+#include <linux/err.h>
 #include <linux/bug.h>
 #include <linux/time.h>
 #include <asm/unaligned.h>
@@ -85,6 +86,52 @@ static inline int ceph_has_room(void **p, void *end,
size_t n)
 	} while (0)

 /*
+ * Allocate a buffer big enough to hold the wire-encoded string, and
+ * decode the string into it.  The resulting string will always be
+ * terminated with '\0'.  If successful, *p will be advanced
+ * past the decoded data.  Also, if lenp is not a null pointer, the
+ * length (not including the terminating '\0') will be recorded in
+ * *lenp.  Note that a zero-length string is a valid return value.
+ *
+ * Returns a pointer to the newly-allocated string buffer, or a
+ * pointer-coded errno if an error occurs.  Neither *p nor *lenp
+ * will have been updated if an error is returned.
+ *
+ * There are two possible failures:
+ *   - converting the string would require accessing memory at or
+ *     beyond the "end" pointer provided (-E
+ *   - memory could not be allocated for the result
+ */
+static inline char *ceph_extract_encoded_string(void **p, void *end,
+						size_t *lenp, gfp_t gfp)
+{
+	u32 len;
+	void *sp = *p;
+	char *buf;
+
+	ceph_decode_32_safe(&sp, end, len, bad);
+	if (!ceph_has_room(&sp, end, len))
+		goto bad;
+
+	buf = kmalloc(len + 1, gfp);
+	if (!buf)
+		return ERR_PTR(-ENOMEM);
+
+	if (len)
+		memcpy(buf, sp, len);
+	buf[len] = '\0';
+
+	*p = (char *) *p + sizeof (u32) + len;
+	if (lenp)
+		*lenp = (size_t) len;
+
+	return buf;
+
+bad:
+	return ERR_PTR(-ERANGE);
+}
+
+/*
  * struct ceph_timespec <-> struct timespec
  */
 static inline void ceph_decode_timespec(struct timespec *ts,
-- 
1.7.5.4