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