rbd: keep reference to lingering object requests

Move the code that unregisters an rbd device's lingering header object watch request into rbd_dev_header_watch_sync(), so it occurs in the same function that originally sets up that request.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   10 ++++------
  1 file changed, 4 insertions(+), 6 deletions(-)

drivers/block/rbd.c +4 -0

index 47e5798..363a813 100644
@@ -1730,6 +1730,10 @@ static int rbd_dev_header_watch_sync(struct
 rbd_device *rbd_dev, int start)
  	if (start) {
  		rbd_dev->watch_request = obj_request->osd_req;
  		ceph_osdc_set_request_linger(osdc, rbd_dev->watch_request);
+	} else {
+		ceph_osdc_unregister_linger_request(osdc,
+						rbd_dev->watch_request);
+		rbd_dev->watch_request = NULL;
  	}
  	ret = rbd_obj_request_submit(osdc, obj_request);

if (ret)

drivers/block/rbd.c +0 -6

@@ -4040,12 +4044,6 @@ static void rbd_dev_release(struct device *dev)
  {
  	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
 
-	if (rbd_dev->watch_request) {
-		struct ceph_client *client = rbd_dev->rbd_client->client;
-
-		ceph_osdc_unregister_linger_request(&client->osdc,
-						    rbd_dev->watch_request);
-	}
  	if (rbd_dev->watch_event)
  		rbd_dev_header_watch_sync(rbd_dev, 0);
 

Switch to keeping track of the object request pointer rather than the osd request used to watch the rbd image header object.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |    8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

drivers/block/rbd.c +4 -4

index 363a813..4e78402 100644
@@ -270,7 +270,7 @@ struct rbd_device {
  	struct ceph_file_layout	layout;
 
  	struct ceph_osd_event   *watch_event;
-	struct ceph_osd_request *watch_request;
+	struct rbd_obj_request	*watch_request;
 
  	struct rbd_spec		*parent_spec;
  	u64			parent_overlap;

@@ -1728,11 +1728,11 @@ static int rbd_dev_header_watch_sync(struct
 rbd_device *rbd_dev, int start)
  		goto out_cancel;
 
  	if (start) {
-		rbd_dev->watch_request = obj_request->osd_req;
-		ceph_osdc_set_request_linger(osdc, rbd_dev->watch_request);
+		ceph_osdc_set_request_linger(osdc, obj_request->osd_req);
+		rbd_dev->watch_request = obj_request;
  	} else {
  		ceph_osdc_unregister_linger_request(osdc,
-						rbd_dev->watch_request);
+					rbd_dev->watch_request->osd_req);
  		rbd_dev->watch_request = NULL;
  	}

  	ret = rbd_obj_request_submit(osdc, obj_request);

Decrement the obj_request_count value when deleting an object request from its image request's list. Rearrange a few lines in the surrounding code.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   16 ++++++++++++----
  1 file changed, 12 insertions(+), 4 deletions(-)

drivers/block/rbd.c +11 -4

index 4e78402..340773f 100644
@@ -1071,22 +1071,29 @@ static void rbd_img_request_put(struct
 rbd_img_request *img_request)
  static inline void rbd_img_obj_request_add(struct rbd_img_request
 *img_request,
  					struct rbd_obj_request *obj_request)
  {
+	rbd_assert(obj_request->img_request == NULL);
+
  	rbd_obj_request_get(obj_request);
  	obj_request->img_request = img_request;
-	list_add_tail(&obj_request->links, &img_request->obj_requests);
-	obj_request->which = img_request->obj_request_count++;
+	obj_request->which = img_request->obj_request_count;
  	rbd_assert(obj_request->which != BAD_WHICH);
+	img_request->obj_request_count++;
+	list_add_tail(&obj_request->links, &img_request->obj_requests);
  }
 
  static inline void rbd_img_obj_request_del(struct rbd_img_request
 *img_request,
  					struct rbd_obj_request *obj_request)
  {
  	rbd_assert(obj_request->which != BAD_WHICH);
-	obj_request->which = BAD_WHICH;
+
  	list_del(&obj_request->links);
+	rbd_assert(img_request->obj_request_count > 0);
+	img_request->obj_request_count--;
+	rbd_assert(obj_request->which == img_request->obj_request_count);
+	obj_request->which = BAD_WHICH;
  	rbd_assert(obj_request->img_request == img_request);
-	obj_request->callback = NULL;
  	obj_request->img_request = NULL;
+	obj_request->callback = NULL;

  	rbd_obj_request_put(obj_request);
  }

drivers/block/rbd.c +1 -0

@@ -1482,6 +1489,7 @@ static void rbd_img_request_destroy(struct kref *kref)
 
  	for_each_obj_request_safe(img_request, obj_request, next_obj_request)
  		rbd_img_obj_request_del(img_request, obj_request);
+	rbd_assert(img_request->obj_request_count == 0);
 
  	if (img_request->write_request)
  		ceph_put_snap_context(img_request->snapc);

The new request code arranges to get a callback for every osd request we submit (this was not the case previously).

We register a lingering object watch request for the header object for each mapped rbd image.

If a connection problem occurs, the osd client will re-submit lingering requests. And each time such a request is re-submitted, its callback function will get called again.

We therefore need to ensure the object request associated with the lingering osd request stays valid, and the way to do that is to have an extra reference to the lingering osd request.

So when a request to initiate a watch has completed, do not drop a reference as one normally would. Instead, hold off dropping that reference until the request to tear down that watch request is done.

Also, only set the rbd device's watch_request pointer after the watch request has been completed successfully, and clear the pointer once it's been torn down.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   31 ++++++++++++++++++++++---------
  1 file changed, 22 insertions(+), 9 deletions(-)

drivers/block/rbd.c +1 -0

index 340773f..177ba0c 100644
@@ -1716,6 +1716,7 @@ static int rbd_dev_header_watch_sync(struct
 rbd_device *rbd_dev, int start)
  						&rbd_dev->watch_event);
  		if (ret < 0)
  			return ret;
+		rbd_assert(rbd_dev->watch_event != NULL);
  	}
 

  	ret = -ENOMEM;

drivers/block/rbd.c +21 -9

@@ -1735,32 +1736,44 @@ static int rbd_dev_header_watch_sync(struct
 rbd_device *rbd_dev, int start)
  	if (!obj_request->osd_req)
  		goto out_cancel;
 
-	if (start) {
+	if (start)
  		ceph_osdc_set_request_linger(osdc, obj_request->osd_req);
-		rbd_dev->watch_request = obj_request;
-	} else {
+	else
  		ceph_osdc_unregister_linger_request(osdc,
  					rbd_dev->watch_request->osd_req);
-		rbd_dev->watch_request = NULL;
-	}
  	ret = rbd_obj_request_submit(osdc, obj_request);
  	if (ret)
  		goto out_cancel;
  	ret = rbd_obj_request_wait(obj_request);
  	if (ret)
  		goto out_cancel;
-
  	ret = obj_request->result;
  	if (ret)
  		goto out_cancel;
 
-	if (start)
-		goto done;	/* Done if setting up the watch request */
+	/*
+	 * Since a watch request is set to linger the osd client
+	 * will hang onto it in case it needs to be re-sent in the
+	 * event of connection loss.  If we're initiating the watch
+	 * we therefore do *not* want to drop our reference to the
+	 * object request now; we'll effectively transfer ownership
+	 * of it to the osd client instead.  Instead, we'll drop
+	 * that reference when the watch request gets torn down.
+	 */
+	if (start) {
+		rbd_dev->watch_request = obj_request;
+
+		return 0;
+	}
+
+	/* We have successfully torn down the watch request */
+
+	rbd_obj_request_put(rbd_dev->watch_request);
+	rbd_dev->watch_request = NULL;
  out_cancel:
  	/* Cancel the event if we're tearing down, or on error */
  	ceph_osdc_cancel_event(rbd_dev->watch_event);
  	rbd_dev->watch_event = NULL;
-done:
  	if (obj_request)
  		rbd_obj_request_put(obj_request);

Quoted 6 lines · Alex Elder

The new request code arranges to get a callback for every osd request we submit (this was not the case previously).

We register a lingering object watch request for the header object for each mapped rbd image.

Quoted 3 lines

If a connection problem occurs, the osd client will re-submit lingering requests. And each time such a request is re-submitted, its callback function will get called again.

I think this should be fixed in the osd_client - rbd should only get the callback once, when the watch is first registered. Later we could add a separate callback to handle re-registration if we need to.

Quoted 85 lines

We therefore need to ensure the object request associated with the lingering osd request stays valid, and the way to do that is to have an extra reference to the lingering osd request.

So when a request to initiate a watch has completed, do not drop a reference as one normally would. Instead, hold off dropping that reference until the request to tear down that watch request is done.

Also, only set the rbd device's watch_request pointer after the watch request has been completed successfully, and clear the pointer once it's been torn down.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   31 ++++++++++++++++++++++---------
  1 file changed, 22 insertions(+), 9 deletions(-)

drivers/block/rbd.c +1 -0

index 340773f..177ba0c 100644
@@ -1716,6 +1716,7 @@ static int rbd_dev_header_watch_sync(struct
 rbd_device *rbd_dev, int start)
                          &rbd_dev->watch_event);
          if (ret < 0)
              return ret;
+        rbd_assert(rbd_dev->watch_event != NULL);
      }
 

      ret = -ENOMEM;

drivers/block/rbd.c +21 -9

@@ -1735,32 +1736,44 @@ static int rbd_dev_header_watch_sync(struct
 rbd_device *rbd_dev, int start)
      if (!obj_request->osd_req)
          goto out_cancel;
 
-    if (start) {
+    if (start)
          ceph_osdc_set_request_linger(osdc, obj_request->osd_req);
-        rbd_dev->watch_request = obj_request;
-    } else {
+    else
          ceph_osdc_unregister_linger_request(osdc,
                      rbd_dev->watch_request->osd_req);
-        rbd_dev->watch_request = NULL;
-    }
      ret = rbd_obj_request_submit(osdc, obj_request);
      if (ret)
          goto out_cancel;
      ret = rbd_obj_request_wait(obj_request);
      if (ret)
          goto out_cancel;
-
      ret = obj_request->result;
      if (ret)
          goto out_cancel;
 
-    if (start)
-        goto done;    /* Done if setting up the watch request */
+    /*
+     * Since a watch request is set to linger the osd client
+     * will hang onto it in case it needs to be re-sent in the
+     * event of connection loss.  If we're initiating the watch
+     * we therefore do *not* want to drop our reference to the
+     * object request now; we'll effectively transfer ownership
+     * of it to the osd client instead.  Instead, we'll drop
+     * that reference when the watch request gets torn down.
+     */
+    if (start) {
+        rbd_dev->watch_request = obj_request;
+
+        return 0;
+    }
+
+    /* We have successfully torn down the watch request */
+
+    rbd_obj_request_put(rbd_dev->watch_request);
+    rbd_dev->watch_request = NULL;
  out_cancel:
      /* Cancel the event if we're tearing down, or on error */
      ceph_osdc_cancel_event(rbd_dev->watch_event);
      rbd_dev->watch_event = NULL;
-done:
      if (obj_request)
          rbd_obj_request_put(obj_request);

Quoted 7 lines · Josh Durgin

Quoted 6 lines · Alex Elder

The new request code arranges to get a callback for every osd request we submit (this was not the case previously).

We register a lingering object watch request for the header object for each mapped rbd image.

Quoted 7 lines

Quoted 3 lines

If a connection problem occurs, the osd client will re-submit lingering requests. And each time such a request is re-submitted, its callback function will get called again.

I think this should be fixed in the osd_client - rbd should only get the callback once, when the watch is first registered. Later we could add a separate callback to handle re-registration if we need to.

I agree. Even so, I would like to maintain a reference to this lingering object request as is done in this patch. I think it makes sense even if we'll never get another callback.

I would like to therefore address the multiple callback from the osd client as a separate issue. If I update the comments here accordingly, and open a tracker issue for the other thing, would that be OK with you?

Quoted 21 lines · Alex Elder

Quoted 7 lines · Josh Durgin

Quoted 6 lines · Alex Elder

The new request code arranges to get a callback for every osd request we submit (this was not the case previously).

We register a lingering object watch request for the header object for each mapped rbd image.

Quoted 7 lines

Quoted 3 lines

If a connection problem occurs, the osd client will re-submit lingering requests. And each time such a request is re-submitted, its callback function will get called again.

I think this should be fixed in the osd_client - rbd should only get the callback once, when the watch is first registered. Later we could add a separate callback to handle re-registration if we need to.

I agree. Even so, I would like to maintain a reference to this lingering object request as is done in this patch. I think it makes sense even if we'll never get another callback.

Quoted 4 lines

I would like to therefore address the multiple callback from the osd client as a separate issue. If I update the comments here accordingly, and open a tracker issue for the other thing, would that be OK with you?

That's fine with me.