| Message ID | 50119656.9080709@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/26/2012 12:11 PM, Alex Elder wrote: > When an rbd image is mapped, a watch request is issued so that the > client will get notified in the event the image header object > changes. This is done using rbd_init_watch_dev(), which calls > rbd_req_sync_watch(). > > rbd_init_watch_dev() is organized as a loop, arranging to re-issue > the request after refreshing the header if -ERANGE ever gets > returned. But the only way rbd_req_sync_watch() will return -ERANGE > is if the CEPH_OSD_OP_WATCH operation returns that, which it will > not. > > As a result, the whole looping structure and in fact the whole > function becomes excessive. So get rid of rbd_init_watch_dev(), > and call rbd_req_sync_watch() directly in the one place it's used. Instead of getting rid of this loop, we should make it send a compound operation consisting of (CEPH_OSD_OP_ASSERT_VER, CEPH_OSD_OP_WATCH). Then we will get -ERANGE if our version of the header is out of data, so we don't lose header updates before the watch is established. Josh > Signed-off-by: Alex Elder <elder@inktank.com> > --- > drivers/block/rbd.c | 18 +----------------- > 1 file changed, 1 insertion(+), 17 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 94d0745..71e3f3b 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -2191,22 +2191,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); > - if (ret == -ERANGE) { > - rc = rbd_refresh_header(rbd_dev, NULL); > - if (rc < 0) > - return rc; > - } > - } while (ret == -ERANGE); > - > - return ret; > -} > - > static atomic64_t rbd_id_max = ATOMIC64_INIT(0); > > /* > @@ -2510,7 +2494,7 @@ static ssize_t rbd_add(struct bus_type *bus, > if (rc) > goto err_out_bus; > > - rc = rbd_init_watch_dev(rbd_dev); > + rc = rbd_req_sync_watch(rbd_dev); > if (rc) > 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 07/30/2012 04:46 PM, Josh Durgin wrote: > On 07/26/2012 12:11 PM, Alex Elder wrote: >> When an rbd image is mapped, a watch request is issued so that the >> client will get notified in the event the image header object >> changes. This is done using rbd_init_watch_dev(), which calls >> rbd_req_sync_watch(). >> >> rbd_init_watch_dev() is organized as a loop, arranging to re-issue >> the request after refreshing the header if -ERANGE ever gets >> returned. But the only way rbd_req_sync_watch() will return -ERANGE >> is if the CEPH_OSD_OP_WATCH operation returns that, which it will >> not. >> >> As a result, the whole looping structure and in fact the whole >> function becomes excessive. So get rid of rbd_init_watch_dev(), >> and call rbd_req_sync_watch() directly in the one place it's used. > > Instead of getting rid of this loop, we should make it > send a compound operation consisting of (CEPH_OSD_OP_ASSERT_VER, > CEPH_OSD_OP_WATCH). Then we will get -ERANGE if our version > of the header is out of data, so we don't lose header updates > before the watch is established. I don't have compound operations going yet, but now I see why it may have been written this way. So I'll drop this patch, and will make a note to re-visit this spot later. -Alex > Josh > >> Signed-off-by: Alex Elder <elder@inktank.com> >> --- >> drivers/block/rbd.c | 18 +----------------- >> 1 file changed, 1 insertion(+), 17 deletions(-) >> >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c >> index 94d0745..71e3f3b 100644 >> --- a/drivers/block/rbd.c >> +++ b/drivers/block/rbd.c >> @@ -2191,22 +2191,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); >> - if (ret == -ERANGE) { >> - rc = rbd_refresh_header(rbd_dev, NULL); >> - if (rc < 0) >> - return rc; >> - } >> - } while (ret == -ERANGE); >> - >> - return ret; >> -} >> - >> static atomic64_t rbd_id_max = ATOMIC64_INIT(0); >> >> /* >> @@ -2510,7 +2494,7 @@ static ssize_t rbd_add(struct bus_type *bus, >> if (rc) >> goto err_out_bus; >> >> - rc = rbd_init_watch_dev(rbd_dev); >> + rc = rbd_req_sync_watch(rbd_dev); >> if (rc) >> 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 94d0745..71e3f3b 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -2191,22 +2191,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); - if (ret == -ERANGE) { - rc = rbd_refresh_header(rbd_dev, NULL); - if (rc < 0) - return rc; - } - } while (ret == -ERANGE); - - return ret; -} - static atomic64_t rbd_id_max = ATOMIC64_INIT(0);
When an rbd image is mapped, a watch request is issued so that the client will get notified in the event the image header object changes. This is done using rbd_init_watch_dev(), which calls rbd_req_sync_watch(). rbd_init_watch_dev() is organized as a loop, arranging to re-issue the request after refreshing the header if -ERANGE ever gets returned. But the only way rbd_req_sync_watch() will return -ERANGE is if the CEPH_OSD_OP_WATCH operation returns that, which it will not. As a result, the whole looping structure and in fact the whole function becomes excessive. So get rid of rbd_init_watch_dev(), and call rbd_req_sync_watch() directly in the one place it's used. Signed-off-by: Alex Elder <elder@inktank.com> --- drivers/block/rbd.c | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) /* @@ -2510,7 +2494,7 @@ static ssize_t rbd_add(struct bus_type *bus, if (rc) goto err_out_bus; - rc = rbd_init_watch_dev(rbd_dev); + rc = rbd_req_sync_watch(rbd_dev); if (rc) goto err_out_bus;