rbd: updates and enhancements

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 <[email protected]>

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

include/linux/ceph/decode.h +1 -1

index d8615de..bcbd66c 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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<[email protected]>

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

include/linux/ceph/decode.h +1 -1

index d8615de..bcbd66c 100644
@@ -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);

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 <[email protected]>

---
 drivers/block/rbd.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

drivers/block/rbd.c +0 -1

index 8f428a8..2ae3bb0 100644
@@ -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);

drivers/block/rbd.c +1 -1

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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<[email protected]>

---
  drivers/block/rbd.c |    3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)

drivers/block/rbd.c +0 -1

index 8f428a8..2ae3bb0 100644
@@ -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);

drivers/block/rbd.c +1 -1

@@ -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,

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 <[email protected]>

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

include/linux/ceph/decode.h +36 -0

index bcbd66c..7ead11fc 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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<[email protected]>

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

include/linux/ceph/decode.h +36 -0

index bcbd66c..7ead11fc 100644
@@ -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)

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 <[email protected]>

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

include/linux/ceph/decode.h +19 -0

index 7ead11fc..7759164 100644
@@ -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)

include/linux/ceph/decode.h +0 -0

  * 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines · 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.

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

Quoted 40 lines

Signed-off-by Alex Elder <[email protected]>

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

include/linux/ceph/decode.h +26 -0

index 7ead11fc..7759164 100644
@@ -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;

include/linux/ceph/decode.h static inline size_t ceph_decode_string(void **p, · line 112

Quoted 3 lines

include/linux/ceph/decode.h +3 -0

+}
+
+/*

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.

Quoted 11 lines

include/linux/ceph/decode.h +0 -0

  * 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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines · 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.

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

Quoted 40 lines

Signed-off-by Alex Elder <[email protected]>

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

include/linux/ceph/decode.h +26 -0

index 7ead11fc..7759164 100644
@@ -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;

include/linux/ceph/decode.h static inline size_t ceph_decode_string(void **p, · line 112

Quoted 3 lines

include/linux/ceph/decode.h +3 -0

+}
+
+/*

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.

Quoted 10 lines

include/linux/ceph/decode.h +0 -0

  * 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 11 lines · Yehuda Sadeh

Quoted 8 lines · 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.

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.

Quoted 40 lines

Quoted 40 lines

Signed-off-by Alex Elder <[email protected]>

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

include/linux/ceph/decode.h +26 -0

index 7ead11fc..7759164 100644
@@ -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;

Quoted 8 lines

include/linux/ceph/decode.h static inline size_t ceph_decode_string(void **p, · line 112

Quoted 3 lines

include/linux/ceph/decode.h +3 -0

+}
+
+/*

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?

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 <[email protected]>

---
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
===================================================================

include/linux/ceph/decode.h +5 -0

@@ -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

include/linux/ceph/decode.h +26 -1

+{
+	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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

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

Quoted 3 lines

include/linux/ceph/decode.h static inline int ceph_has_room(void **p · line 105

Quoted 3 lines

+ */

include/linux/ceph/decode.h +2 -0

+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...

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 <[email protected]>

---
 drivers/block/rbd.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

drivers/block/rbd.c +36 -1

index 2ae3bb0..63c132f 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

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<[email protected]>

---
  drivers/block/rbd.c |   36 ++++++++++++++++++++++++++++++++++++
  1 files changed, 36 insertions(+), 0 deletions(-)

drivers/block/rbd.c +36 -0

index 2ae3bb0..63c132f 100644
@@ -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

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 <[email protected]>

---
 drivers/block/rbd.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

drivers/block/rbd.c +2 -2

index 63c132f..57d264c 100644
@@ -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);

drivers/block/rbd.c +1 -1

@@ -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)

{

drivers/block/rbd.c +1 -1

@@ -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);

drivers/block/rbd.c +2 -3

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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<[email protected]>

---
  drivers/block/rbd.c |   12 ++++++------
  1 files changed, 6 insertions(+), 6 deletions(-)

drivers/block/rbd.c +2 -2

index 63c132f..57d264c 100644
@@ -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);

drivers/block/rbd.c +1 -1

@@ -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)

  {

drivers/block/rbd.c +1 -1

@@ -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);

drivers/block/rbd.c +2 -2

@@ -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);

Might want to rename block_name in struct rbd_image_header_ondisk too.

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<[email protected]>

---
  drivers/block/rbd.c |   22 +++++++++++++++++-----
  1 files changed, 17 insertions(+), 5 deletions(-)

drivers/block/rbd.c +8 -1

index 57d264c..3aa0ca0 100644
@@ -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;

drivers/block/rbd.c +2 -0

@@ -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:

drivers/block/rbd.c +3 -2

@@ -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);
  }
 
  /*

drivers/block/rbd.c +3 -0

  	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

Quoted 6 lines

/*

(quoted diff) +0 -0

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

Review static int __rbd_refresh_header(struct · line 1725

Quoted 3 lines

(quoted diff) +1 -2

-    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.

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<[email protected]>

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
===================================================================

drivers/block/rbd.c +26 -9

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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<[email protected]>

---
  drivers/block/rbd.c |   74
+++++++++++++++++++++++++++++++-------------------
  1 files changed, 46 insertions(+), 28 deletions(-)

drivers/block/rbd.c +5 -7

index 3aa0ca0..76e978c 100644
@@ -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,

drivers/block/rbd.c +29 -16

@@ -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,

drivers/block/rbd.c +12 -5

@@ -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);

Quoted 5 lines · 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.

Quoted 8 lines

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.

Quoted 221 lines

- 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<[email protected]>

---
  drivers/block/rbd.c |   74
+++++++++++++++++++++++++++++++-------------------
  1 files changed, 46 insertions(+), 28 deletions(-)

drivers/block/rbd.c +0 -0

index 3aa0ca0..76e978c 100644
@@ -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))

drivers/block/rbd.c +1 -3

-#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;

drivers/block/rbd.c +4 -4

@@ -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,

drivers/block/rbd.c +22 -14

@@ -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. */

drivers/block/rbd.c +7 -2

@@ -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,

drivers/block/rbd.c +11 -5

@@ -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

drivers/block/rbd.c err_out_blkdev: · line 2505

Quoted 7 lines

drivers/block/rbd.c +1 -0

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · 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.

Quoted 8 lines

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.

Quoted 7 lines

- 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<[email protected]>

Quoted 15 lines · Josh Durgin

Quoted 5 lines · 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.

Quoted 8 lines

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.

Quoted 5 lines

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?

Quoted 5 lines · 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.

Quoted 8 lines

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.

Quoted 227 lines

- 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<[email protected]>

---
  drivers/block/rbd.c |   74
+++++++++++++++++++++++++++++++-------------------
  1 files changed, 46 insertions(+), 28 deletions(-)

drivers/block/rbd.c +3 -5

index 3aa0ca0..76e978c 100644
@@ -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;

drivers/block/rbd.c +2 -2

@@ -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,

drivers/block/rbd.c +1 -1

@@ -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);

drivers/block/rbd.c +21 -13

@@ -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. */

drivers/block/rbd.c +7 -2

@@ -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,

drivers/block/rbd.c +12 -5

@@ -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);

Quoted 15 lines · Josh Durgin

Quoted 5 lines · 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.

Quoted 8 lines

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.

Quoted 5 lines

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

Quoted 230 lines

Quoted 227 lines

- 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<[email protected]>

---
   drivers/block/rbd.c |   74
+++++++++++++++++++++++++++++++-------------------
   1 files changed, 46 insertions(+), 28 deletions(-)

drivers/block/rbd.c +3 -5

index 3aa0ca0..76e978c 100644
@@ -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;

drivers/block/rbd.c +2 -2

@@ -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,

drivers/block/rbd.c +1 -1

@@ -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);

drivers/block/rbd.c +21 -13

@@ -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. */

drivers/block/rbd.c +7 -2

@@ -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,

drivers/block/rbd.c +12 -5

@@ -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);

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<[email protected]>

---
  drivers/block/rbd.c |   18 ++++++++++++++----
  1 file changed, 14 insertions(+), 4 deletions(-)

Index: b/drivers/block/rbd.c
===================================================================

drivers/block/rbd.c +14 -5

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Looks good.

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

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<[email protected]>

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

Index: b/drivers/block/rbd.c
===================================================================

drivers/block/rbd.c +19 -9

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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<[email protected]>

---
  drivers/block/rbd.c |   33 +++++++++++++++++++++------------
  1 files changed, 21 insertions(+), 12 deletions(-)

drivers/block/rbd.c +8 -4

index 76e978c..4d11337 100644
@@ -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);
 

/*

drivers/block/rbd.c +1 -0

@@ -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);

drivers/block/rbd.c +12 -8

@@ -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);