| Message ID | 50FF0B8A.6080007@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; >> > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; >>> >> -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-) @@ -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;