rbd: a few simple changes

PATCH In review Alex Elder 3 participants 11 messages Jan 22, 2013 – Jan 24, 2013

Subsystems (2)

Block layer RADOS block device (RBD)

lore

Share & tools

lore

Alex Elder

author

2013-01-22 21:56 UTC #0 lore raw

cover rbd: a few simple changes

I'm about to post a set of large rbd patches for review. But before doing so I'm slipping in these three little patches that are unrelated to that big chunk of work.

-Alex

[PATCH 1/3] rbd: small changes
[PATCH 2/3] rbd: check for overflow in rbd_get_num_segments() [PATCH 3/3] rbd: don't retry setting up header watch

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

Signature · 1 line

1.7.9.5

Dan Mick

Reviewed-by on v1 1/3

2013-01-22 22:40 UTC #2 lore raw

Re: [PATCH 1/3] rbd: small changes

Reviewed-by Dan Mick <[email protected]>

Quoted 75 lines · Alex Elder

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;

Josh Durgin

Reviewed-by on v1 1/3

2013-01-24 23:03 UTC #3 lore raw

Re: [PATCH 1/3] rbd: small changes

In reply to Alex Elder

Reviewed-by Josh Durgin <[email protected]>

Quoted 75 lines · Alex Elder

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;

Alex Elder

author

2013-01-22 21:58 UTC #4 lore raw

PATCH 2/3 rbd: check for overflow in rbd_get_num_segments()

In reply to Alex Elder

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 +0 -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;
@@ -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;
}

Signature · 1 line

1.7.9.5

Dan Mick

Reviewed-by on v1 2/3

2013-01-22 22:40 UTC #5 lore raw

Re: [PATCH 2/3] rbd: check for overflow in rbd_get_num_segments()

Reviewed-by Dan Mick <[email protected]>

Quoted 39 lines · Alex Elder

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

Josh Durgin

Reviewed-by on v1 2/3

2013-01-24 23:03 UTC #6 lore raw

Re: [PATCH 2/3] rbd: check for overflow in rbd_get_num_segments()

In reply to Alex Elder

Reviewed-by Josh Durgin <[email protected]>

Quoted 39 lines · Alex Elder

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

Alex Elder

author

2013-01-22 21:58 UTC #7 lore raw

PATCH 3/3 rbd: don't retry setting up header watch

In reply to 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.

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

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

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

Signature · 1 line

1.7.9.5

Josh Durgin

Reviewed-by on v1 3/3

2013-01-24 23:10 UTC #8 lore raw

Re: [PATCH 3/3] rbd: don't retry setting up header watch

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

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;

Alex Elder

author

2013-01-24 23:30 UTC #9 lore raw

Re: [PATCH 3/3] rbd: don't retry setting up header watch

Quoted 13 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 3 lines
For now, just stop retrying, and if ERANGE gets returned, fail the map request.

Quoted 7 lines

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

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.

-Alex

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;

Josh Durgin

2013-01-24 23:36 UTC #10 lore raw

Re: [PATCH 3/3] rbd: don't retry setting up header watch

Quoted 17 lines · Alex Elder

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

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?

Yes.

Quoted 6 lines

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.

Agreed. It's probably not too hard to change.

Josh

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;