All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] rbd: a few simple changes
@ 2013-01-22 21:56 Alex Elder
  2013-01-22 21:57 ` [PATCH 1/3] rbd: small changes Alex Elder
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Alex Elder @ 2013-01-22 21:56 UTC (permalink / raw)
  To: ceph-devel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] rbd: small changes
  2013-01-22 21:56 [PATCH 0/3] rbd: a few simple changes Alex Elder
@ 2013-01-22 21:57 ` Alex Elder
  2013-01-22 22:40   ` Dan Mick
  2013-01-24 23:03   ` Josh Durgin
  2013-01-22 21:58 ` [PATCH 2/3] rbd: check for overflow in rbd_get_num_segments() Alex Elder
  2013-01-22 21:58 ` [PATCH 3/3] rbd: don't retry setting up header watch Alex Elder
  2 siblings, 2 replies; 11+ messages in thread
From: Alex Elder @ 2013-01-22 21:57 UTC (permalink / raw)
  To: ceph-devel

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 <elder@inktank.com>
---
 drivers/block/rbd.c |   17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 007b726..4ed0741 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -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;
@@ -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;
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] rbd: check for overflow in rbd_get_num_segments()
  2013-01-22 21:56 [PATCH 0/3] rbd: a few simple changes Alex Elder
  2013-01-22 21:57 ` [PATCH 1/3] rbd: small changes Alex Elder
@ 2013-01-22 21:58 ` Alex Elder
  2013-01-22 22:40   ` Dan Mick
  2013-01-24 23:03   ` Josh Durgin
  2013-01-22 21:58 ` [PATCH 3/3] rbd: don't retry setting up header watch Alex Elder
  2 siblings, 2 replies; 11+ messages in thread
From: Alex Elder @ 2013-01-22 21:58 UTC (permalink / raw)
  To: ceph-devel

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 <elder@inktank.com>
---
 drivers/block/rbd.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 4ed0741..58d01e3 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -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;
 }

 /*
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] rbd: don't retry setting up header watch
  2013-01-22 21:56 [PATCH 0/3] rbd: a few simple changes Alex Elder
  2013-01-22 21:57 ` [PATCH 1/3] rbd: small changes Alex Elder
  2013-01-22 21:58 ` [PATCH 2/3] rbd: check for overflow in rbd_get_num_segments() Alex Elder
@ 2013-01-22 21:58 ` Alex Elder
  2013-01-24 23:09   ` Josh Durgin
  2 siblings, 1 reply; 11+ messages in thread
From: Alex Elder @ 2013-01-22 21:58 UTC (permalink / raw)
  To: ceph-devel

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 <elder@inktank.com>
---
 drivers/block/rbd.c |   23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 58d01e3..6689363 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -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;

-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] rbd: small changes
  2013-01-22 21:57 ` [PATCH 1/3] rbd: small changes Alex Elder
@ 2013-01-22 22:40   ` Dan Mick
  2013-01-24 23:03   ` Josh Durgin
  1 sibling, 0 replies; 11+ messages in thread
From: Dan Mick @ 2013-01-22 22:40 UTC (permalink / raw)
  To: Alex Elder; +Cc: ceph-devel

Reviewed-by: Dan Mick <dan.mick@inktank.com>

On 01/22/2013 01:57 PM, Alex Elder wrote:
> 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 <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 007b726..4ed0741 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -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;
> @@ -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;
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] rbd: check for overflow in rbd_get_num_segments()
  2013-01-22 21:58 ` [PATCH 2/3] rbd: check for overflow in rbd_get_num_segments() Alex Elder
@ 2013-01-22 22:40   ` Dan Mick
  2013-01-24 23:03   ` Josh Durgin
  1 sibling, 0 replies; 11+ messages in thread
From: Dan Mick @ 2013-01-22 22:40 UTC (permalink / raw)
  To: Alex Elder; +Cc: ceph-devel

Reviewed-by: Dan Mick <dan.mick@inktank.com>

On 01/22/2013 01:58 PM, Alex Elder wrote:
> 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 <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 4ed0741..58d01e3 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -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;
>   }
>
>   /*
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] rbd: small changes
  2013-01-22 21:57 ` [PATCH 1/3] rbd: small changes Alex Elder
  2013-01-22 22:40   ` Dan Mick
@ 2013-01-24 23:03   ` Josh Durgin
  1 sibling, 0 replies; 11+ messages in thread
From: Josh Durgin @ 2013-01-24 23:03 UTC (permalink / raw)
  To: Alex Elder; +Cc: ceph-devel

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

On 01/22/2013 01:57 PM, Alex Elder wrote:
> 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 <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 007b726..4ed0741 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -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;
> @@ -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;
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] rbd: check for overflow in rbd_get_num_segments()
  2013-01-22 21:58 ` [PATCH 2/3] rbd: check for overflow in rbd_get_num_segments() Alex Elder
  2013-01-22 22:40   ` Dan Mick
@ 2013-01-24 23:03   ` Josh Durgin
  1 sibling, 0 replies; 11+ messages in thread
From: Josh Durgin @ 2013-01-24 23:03 UTC (permalink / raw)
  To: Alex Elder; +Cc: ceph-devel

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

On 01/22/2013 01:58 PM, Alex Elder wrote:
> 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 <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 4ed0741..58d01e3 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -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;
>   }
>
>   /*
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] rbd: don't retry setting up header watch
  2013-01-22 21:58 ` [PATCH 3/3] rbd: don't retry setting up header watch Alex Elder
@ 2013-01-24 23:09   ` Josh Durgin
  2013-01-24 23:30     ` Alex Elder
  0 siblings, 1 reply; 11+ messages in thread
From: Josh Durgin @ 2013-01-24 23:09 UTC (permalink / raw)
  To: Alex Elder; +Cc: ceph-devel

On 01/22/2013 01:58 PM, Alex Elder wrote:
> 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.

> 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 <josh.durgin@inktank.com>

> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   23 ++++-------------------
>   1 file changed, 4 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 58d01e3..6689363 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -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;
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] rbd: don't retry setting up header watch
  2013-01-24 23:09   ` Josh Durgin
@ 2013-01-24 23:30     ` Alex Elder
  2013-01-24 23:36       ` Josh Durgin
  0 siblings, 1 reply; 11+ messages in thread
From: Alex Elder @ 2013-01-24 23:30 UTC (permalink / raw)
  To: Josh Durgin; +Cc: ceph-devel

On 01/24/2013 05:09 PM, Josh Durgin wrote:
> On 01/22/2013 01:58 PM, Alex Elder wrote:
>> 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.
> 
>> 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

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

> With the commit message updated,
> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
> 
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |   23 ++++-------------------
>>   1 file changed, 4 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 58d01e3..6689363 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -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;
>>
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] rbd: don't retry setting up header watch
  2013-01-24 23:30     ` Alex Elder
@ 2013-01-24 23:36       ` Josh Durgin
  0 siblings, 0 replies; 11+ messages in thread
From: Josh Durgin @ 2013-01-24 23:36 UTC (permalink / raw)
  To: Alex Elder; +Cc: ceph-devel

On 01/24/2013 03:30 PM, Alex Elder wrote:
> On 01/24/2013 05:09 PM, Josh Durgin wrote:
>> On 01/22/2013 01:58 PM, Alex Elder wrote:
>>> 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.
>>
>>> 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
>
> So the fix is to post a watch *before* the initial
> read of the header/snapshot context?

Yes.

> 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

>> With the commit message updated,
>> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
>>
>>> Signed-off-by: Alex Elder <elder@inktank.com>
>>> ---
>>>    drivers/block/rbd.c |   23 ++++-------------------
>>>    1 file changed, 4 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>>> index 58d01e3..6689363 100644
>>> --- a/drivers/block/rbd.c
>>> +++ b/drivers/block/rbd.c
>>> @@ -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;
>>>
>>


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-01-24 23:36 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-22 21:56 [PATCH 0/3] rbd: a few simple changes Alex Elder
2013-01-22 21:57 ` [PATCH 1/3] rbd: small changes Alex Elder
2013-01-22 22:40   ` Dan Mick
2013-01-24 23:03   ` Josh Durgin
2013-01-22 21:58 ` [PATCH 2/3] rbd: check for overflow in rbd_get_num_segments() Alex Elder
2013-01-22 22:40   ` Dan Mick
2013-01-24 23:03   ` Josh Durgin
2013-01-22 21:58 ` [PATCH 3/3] rbd: don't retry setting up header watch Alex Elder
2013-01-24 23:09   ` Josh Durgin
2013-01-24 23:30     ` Alex Elder
2013-01-24 23:36       ` Josh Durgin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.