rbd: a few simple changes

A few very minor changes to the rbd code:
- RBD_MAX_OPT_LEN is unused, so get rid of it
- Consolidate rbd options definitions
- Make rbd_segment_name() return pointer to const char

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

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

drivers/block/rbd.c +7 -8

index 007b726..4ed0741 100644
@@ -69,7 +69,6 @@
  			(NAME_MAX - (sizeof (RBD_SNAP_DEV_NAME_PREFIX) - 1))
 
  #define RBD_MAX_SNAP_COUNT	510	/* allows max snapc to fit in 4KB */
-#define RBD_MAX_OPT_LEN		1024
 
  #define RBD_SNAP_HEAD_NAME	"-"
 

@@ -96,8 +95,6 @@
  #define DEV_NAME_LEN		32
  #define MAX_INT_FORMAT_WIDTH	((5 * sizeof (int)) / 2 + 1)
 
-#define RBD_READ_ONLY_DEFAULT		false
-
  /*
   * block device image metadata (in-memory version)
   */

@@ -156,10 +153,6 @@ struct rbd_spec {
  	struct kref	kref;
  };
 
-struct rbd_options {
-	bool	read_only;
-};
-
  /*
   * an instance of the client.  multiple devices may share an rbd client.
   */

@@ -475,6 +468,12 @@ static match_table_t rbd_opts_tokens = {
  	{-1, NULL}
  };
 
+struct rbd_options {
+	bool	read_only;
+};
+
+#define RBD_READ_ONLY_DEFAULT	false
+
  static int parse_rbd_opts_token(char *c, void *private)
  {
  	struct rbd_options *rbd_opts = private;

@@ -773,7 +772,7 @@ static void rbd_header_free(struct rbd_image_header
 *header)
  	header->snapc = NULL;
  }
 
-static char *rbd_segment_name(struct rbd_device *rbd_dev, u64 offset)
+static const char *rbd_segment_name(struct rbd_device *rbd_dev, u64 offset)
  {
  	char *name;

u64 segment;

drivers/block/rbd.c +1 -1

@@ -1338,7 +1337,7 @@ static int rbd_do_op(struct request *rq,
  		     struct rbd_req_coll *coll,
  		     int coll_index)
  {
-	char *seg_name;
+	const char *seg_name;
  	u64 seg_ofs;
  	u64 seg_len;
  	int ret;

The return type of rbd_get_num_segments() is int, but the values it operates on are u64. Although it's not likely, there's no guarantee the result won't exceed what can be respresented in an int. The function is already designed to return -ERANGE on error, so just add this possible overflow as another reason to return that.

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

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

drivers/block/rbd.c +1 -0

index 4ed0741..58d01e3 100644
@@ -820,6 +820,7 @@ static int rbd_get_num_segments(struct
 rbd_image_header *header,
  {
  	u64 start_seg;
  	u64 end_seg;
+	u64 result;
 
  	if (!len)

return 0;

drivers/block/rbd.c +5 -1

@@ -829,7 +830,11 @@ static int rbd_get_num_segments(struct
 rbd_image_header *header,
  	start_seg = ofs >> header->obj_order;
  	end_seg = (ofs + len - 1) >> header->obj_order;
 
-	return end_seg - start_seg + 1;
+	result = end_seg - start_seg + 1;
+	if (result > (u64) INT_MAX)
+		return -ERANGE;
+
+	return (int) result;
  }
 

/*

A few very minor changes to the rbd code:
- RBD_MAX_OPT_LEN is unused, so get rid of it
- Consolidate rbd options definitions
- Make rbd_segment_name() return pointer to const char

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

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

drivers/block/rbd.c +7 -8

index 007b726..4ed0741 100644
@@ -69,7 +69,6 @@
  			(NAME_MAX - (sizeof (RBD_SNAP_DEV_NAME_PREFIX) - 1))
 
  #define RBD_MAX_SNAP_COUNT	510	/* allows max snapc to fit in 4KB */
-#define RBD_MAX_OPT_LEN		1024
 
  #define RBD_SNAP_HEAD_NAME	"-"
 

@@ -96,8 +95,6 @@
  #define DEV_NAME_LEN		32
  #define MAX_INT_FORMAT_WIDTH	((5 * sizeof (int)) / 2 + 1)
 
-#define RBD_READ_ONLY_DEFAULT		false
-
  /*
   * block device image metadata (in-memory version)
   */

@@ -156,10 +153,6 @@ struct rbd_spec {
  	struct kref	kref;
  };
 
-struct rbd_options {
-	bool	read_only;
-};
-
  /*
   * an instance of the client.  multiple devices may share an rbd client.
   */

@@ -475,6 +468,12 @@ static match_table_t rbd_opts_tokens = {
  	{-1, NULL}
  };
 
+struct rbd_options {
+	bool	read_only;
+};
+
+#define RBD_READ_ONLY_DEFAULT	false
+
  static int parse_rbd_opts_token(char *c, void *private)
  {
  	struct rbd_options *rbd_opts = private;

@@ -773,7 +772,7 @@ static void rbd_header_free(struct rbd_image_header
 *header)
  	header->snapc = NULL;
  }
 
-static char *rbd_segment_name(struct rbd_device *rbd_dev, u64 offset)
+static const char *rbd_segment_name(struct rbd_device *rbd_dev, u64 offset)
  {
  	char *name;

u64 segment;

drivers/block/rbd.c +1 -1

@@ -1338,7 +1337,7 @@ static int rbd_do_op(struct request *rq,
  		     struct rbd_req_coll *coll,
  		     int coll_index)
  {
-	char *seg_name;
+	const char *seg_name;
  	u64 seg_ofs;
  	u64 seg_len;
  	int ret;

The return type of rbd_get_num_segments() is int, but the values it operates on are u64. Although it's not likely, there's no guarantee the result won't exceed what can be respresented in an int. The function is already designed to return -ERANGE on error, so just add this possible overflow as another reason to return that.

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

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

drivers/block/rbd.c +1 -0

index 4ed0741..58d01e3 100644
@@ -820,6 +820,7 @@ static int rbd_get_num_segments(struct
 rbd_image_header *header,
  {
  	u64 start_seg;
  	u64 end_seg;
+	u64 result;
 
  	if (!len)

return 0;

drivers/block/rbd.c +5 -1

@@ -829,7 +830,11 @@ static int rbd_get_num_segments(struct
 rbd_image_header *header,
  	start_seg = ofs >> header->obj_order;
  	end_seg = (ofs + len - 1) >> header->obj_order;
 
-	return end_seg - start_seg + 1;
+	result = end_seg - start_seg + 1;
+	if (result > (u64) INT_MAX)
+		return -ERANGE;
+
+	return (int) result;
  }
 

/*

When an rbd image is initially mapped a watch event is registered so we can do something if the header object changes. Right now if that returns ERANGE we loop back and try to initiate it again. However the code that sets up the watch event doesn't clean up after itself very well, and doing that properly will require some work.

For now, just stop retrying, and if ERANGE gets returned, fail the map request.

This resolves (for the near term):

     http://tracker.newdream.net/issues/3860

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

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

drivers/block/rbd.c +3 -2

index 58d01e3..6689363 100644
@@ -1474,6 +1474,9 @@ static int rbd_req_sync_watch(struct rbd_device
 *rbd_dev, int start)
  	struct ceph_osd_req_op *op;
  	int ret = 0;
 
+	rbd_assert(start ^ !!rbd_dev->watch_event);
+	rbd_assert(start ^ !!rbd_dev->watch_request);
+
  	if (start) {
  		struct ceph_osd_client *osdc;

@@ -1482,8 +1485,6 @@ static int rbd_req_sync_watch(struct rbd_device
 *rbd_dev, int start)
  						&rbd_dev->watch_event);
  		if (ret < 0)
  			return ret;
-	} else {
-		rbd_assert(rbd_dev->watch_request != NULL);
  	}
 

op = rbd_osd_req_op_create(CEPH_OSD_OP_WATCH,

drivers/block/rbd.c +0 -16

@@ -3023,22 +3024,6 @@ static void rbd_bus_del_dev(struct rbd_device
 *rbd_dev)
  	device_unregister(&rbd_dev->dev);
  }
 
-static int rbd_init_watch_dev(struct rbd_device *rbd_dev)
-{
-	int ret, rc;
-
-	do {
-		ret = rbd_req_sync_watch(rbd_dev, 1);
-		if (ret == -ERANGE) {
-			rc = rbd_dev_refresh(rbd_dev, NULL);
-			if (rc < 0)
-				return rc;
-		}
-	} while (ret == -ERANGE);
-
-	return ret;
-}
-
  static atomic64_t rbd_dev_id_max = ATOMIC64_INIT(0);
 

/*

drivers/block/rbd.c +1 -1

@@ -3584,7 +3569,7 @@ static int rbd_dev_probe_finish(struct rbd_device
 *rbd_dev)
  	if (ret)
  		goto err_out_bus;
 
-	ret = rbd_init_watch_dev(rbd_dev);
+	ret = rbd_req_sync_watch(rbd_dev, 1);
  	if (ret)
  		goto err_out_bus;

Quoted 5 lines · Alex Elder

When an rbd image is initially mapped a watch event is registered so we can do something if the header object changes. Right now if that returns ERANGE we loop back and try to initiate it again. However the code that sets up the watch event doesn't clean up after itself very well, and doing that properly will require some work.

The osds will never return ERANGE. That part of a watch operation was never implemented.

Quoted 3 lines

For now, just stop retrying, and if ERANGE gets returned, fail the map request.

Quoted 2 lines

This resolves (for the near term):

     http://tracker.newdream.net/issues/3860

The code change is fine, but the commit message should be changed. The retry is currently useless because ERANGE will never happen there, so removing it makes sense, but the real fix is
http://www.tracker.newdream.net/issues/3871

With the commit message updated,

Reviewed-by Josh Durgin <[email protected]>

Quoted 64 lines

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

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

drivers/block/rbd.c +3 -2

index 58d01e3..6689363 100644
@@ -1474,6 +1474,9 @@ static int rbd_req_sync_watch(struct rbd_device
 *rbd_dev, int start)
      struct ceph_osd_req_op *op;
      int ret = 0;
 
+    rbd_assert(start ^ !!rbd_dev->watch_event);
+    rbd_assert(start ^ !!rbd_dev->watch_request);
+
      if (start) {
          struct ceph_osd_client *osdc;

@@ -1482,8 +1485,6 @@ static int rbd_req_sync_watch(struct rbd_device
 *rbd_dev, int start)
                          &rbd_dev->watch_event);
          if (ret < 0)
              return ret;
-    } else {
-        rbd_assert(rbd_dev->watch_request != NULL);
      }
 

op = rbd_osd_req_op_create(CEPH_OSD_OP_WATCH,

drivers/block/rbd.c +0 -16

@@ -3023,22 +3024,6 @@ static void rbd_bus_del_dev(struct rbd_device
 *rbd_dev)
      device_unregister(&rbd_dev->dev);
  }
 
-static int rbd_init_watch_dev(struct rbd_device *rbd_dev)
-{
-    int ret, rc;
-
-    do {
-        ret = rbd_req_sync_watch(rbd_dev, 1);
-        if (ret == -ERANGE) {
-            rc = rbd_dev_refresh(rbd_dev, NULL);
-            if (rc < 0)
-                return rc;
-        }
-    } while (ret == -ERANGE);
-
-    return ret;
-}
-
  static atomic64_t rbd_dev_id_max = ATOMIC64_INIT(0);
 

/*

drivers/block/rbd.c +1 -1

@@ -3584,7 +3569,7 @@ static int rbd_dev_probe_finish(struct rbd_device
 *rbd_dev)
      if (ret)
          goto err_out_bus;
 
-    ret = rbd_init_watch_dev(rbd_dev);
+    ret = rbd_req_sync_watch(rbd_dev, 1);
      if (ret)
          goto err_out_bus;

Quoted 16 lines · Josh Durgin

Quoted 5 lines · Alex Elder

When an rbd image is initially mapped a watch event is registered so we can do something if the header object changes. Right now if that returns ERANGE we loop back and try to initiate it again. However the code that sets up the watch event doesn't clean up after itself very well, and doing that properly will require some work.

The osds will never return ERANGE. That part of a watch operation was never implemented.

Quoted 5 lines

For now, just stop retrying, and if ERANGE gets returned, fail the map request.

This resolves (for the near term):

      http://tracker.newdream.net/issues/3860

Quoted 4 lines

The code change is fine, but the commit message should be changed. The retry is currently useless because ERANGE will never happen there, so removing it makes sense, but the real fix is
http://www.tracker.newdream.net/issues/3871

So the fix is to post a watch *before* the initial read of the header/snapshot context?

I think, as it turns out, the new request code cleans up after itself just fine. And I also think that it wouldn't be too hard to set up the watch earlier. The main issue is making sure the watch callback doesn't assume everything is known about the image.

Quoted 68 lines

With the commit message updated,

Reviewed-by Josh Durgin <[email protected]>

Quoted 64 lines

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

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

drivers/block/rbd.c +3 -2

index 58d01e3..6689363 100644
@@ -1474,6 +1474,9 @@ static int rbd_req_sync_watch(struct rbd_device
 *rbd_dev, int start)
       struct ceph_osd_req_op *op;
       int ret = 0;
 
+    rbd_assert(start ^ !!rbd_dev->watch_event);
+    rbd_assert(start ^ !!rbd_dev->watch_request);
+
       if (start) {
           struct ceph_osd_client *osdc;

@@ -1482,8 +1485,6 @@ static int rbd_req_sync_watch(struct rbd_device
 *rbd_dev, int start)
                           &rbd_dev->watch_event);
           if (ret < 0)
               return ret;
-    } else {
-        rbd_assert(rbd_dev->watch_request != NULL);
       }
 

op = rbd_osd_req_op_create(CEPH_OSD_OP_WATCH,

drivers/block/rbd.c +0 -16

@@ -3023,22 +3024,6 @@ static void rbd_bus_del_dev(struct rbd_device
 *rbd_dev)
       device_unregister(&rbd_dev->dev);
   }
 
-static int rbd_init_watch_dev(struct rbd_device *rbd_dev)
-{
-    int ret, rc;
-
-    do {
-        ret = rbd_req_sync_watch(rbd_dev, 1);
-        if (ret == -ERANGE) {
-            rc = rbd_dev_refresh(rbd_dev, NULL);
-            if (rc < 0)
-                return rc;
-        }
-    } while (ret == -ERANGE);
-
-    return ret;
-}
-
   static atomic64_t rbd_dev_id_max = ATOMIC64_INIT(0);
 

   /*

drivers/block/rbd.c +1 -1

@@ -3584,7 +3569,7 @@ static int rbd_dev_probe_finish(struct rbd_device
 *rbd_dev)
       if (ret)
           goto err_out_bus;
 
-    ret = rbd_init_watch_dev(rbd_dev);
+    ret = rbd_req_sync_watch(rbd_dev, 1);
       if (ret)
           goto err_out_bus;