Skip to contentSkip to conversation

rbd: kill rbd_init_watch_dev()

PATCH In review Alex Elder 2 participants 3 messages Jul 26, 2012 – Jul 30, 2012

Subsystems (2)
lore
Share & tools
lore

Alex Elder

author
#0 lore raw

PATCH 1/1 rbd: kill rbd_init_watch_dev()

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.

---
 drivers/block/rbd.c |   18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)
drivers/block/rbd.c +0 -0
index 94d0745..71e3f3b 100644
@@ -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;

Signature · 1 line
1.7.9.5
#1 lore raw
Quoted 11 lines · Alex Elder

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.

Quoted 3 lines

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

Quoted 42 lines
---
  drivers/block/rbd.c |   18 +-----------------
  1 file changed, 1 insertion(+), 17 deletions(-)
drivers/block/rbd.c +0 -16
index 94d0745..71e3f3b 100644
@@ -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);
 

/*

drivers/block/rbd.c +1 -1
@@ -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;
 
author
#2 lore raw
Quoted 16 lines · Josh Durgin
Quoted 11 lines · Alex Elder

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.

Quoted 3 lines

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.

Quoted 5 lines

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

Quoted 47 lines

Josh

Quoted 42 lines
---
  drivers/block/rbd.c |   18 +-----------------
  1 file changed, 1 insertion(+), 17 deletions(-)
drivers/block/rbd.c +0 -16
index 94d0745..71e3f3b 100644
@@ -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);
 

/*

drivers/block/rbd.c +1 -1
@@ -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;