[PATCH 0/2] rbd: tear down watch request if rbd_dev_device_setup() fails

[PATCH 0/2] rbd: tear down watch request if rbd_dev_device_setup() fails

[Ilya Dryomov]

Hello,

Currently, if rbd_dev_device_setup() fails, we don't tear down our
watch request.  Looks like this bug was introduced by 1f3ef78861ac,
which moved a tear down call while fixing something else.  Fix it.

These patches can be pulled from wip-rbd-unwatch branch of
ceph-client.git.

Thanks,

                Ilya


Ilya Dryomov (2):
  rbd: introduce rbd_dev_header_unwatch_sync() and switch to it
  rbd: tear down watch request if rbd_dev_device_setup() fails

 drivers/block/rbd.c |   41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] rbd: introduce rbd_dev_header_unwatch_sync() and switch to it

[Ilya Dryomov]

Rename rbd_dev_header_watch_sync() to __rbd_dev_header_watch_sync() and
introduce two helpers: rbd_dev_header_{,un}watch_sync() to make it more
clear what is going on.

Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com>
---
 drivers/block/rbd.c |   35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index cb1db2979d3d..c91108b760cf 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2866,7 +2866,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id, u8 opcode, void *data)
  * Request sync osd watch/unwatch.  The value of "start" determines
  * whether a watch request is being initiated or torn down.
  */
-static int rbd_dev_header_watch_sync(struct rbd_device *rbd_dev, bool start)
+static int __rbd_dev_header_watch_sync(struct rbd_device *rbd_dev, bool start)
 {
 	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
 	struct rbd_obj_request *obj_request;
@@ -2941,6 +2941,22 @@ out_cancel:
 	return ret;
 }
 
+static int rbd_dev_header_watch_sync(struct rbd_device *rbd_dev)
+{
+	return __rbd_dev_header_watch_sync(rbd_dev, true);
+}
+
+static void rbd_dev_header_unwatch_sync(struct rbd_device *rbd_dev)
+{
+	int ret;
+
+	ret = __rbd_dev_header_watch_sync(rbd_dev, false);
+	if (ret) {
+		rbd_warn(rbd_dev, "unable to tear down watch request: %d\n",
+			 ret);
+	}
+}
+
 /*
  * Synchronous osd object method call.  Returns the number of bytes
  * returned in the outbound buffer, or a negative error code.
@@ -4961,7 +4977,6 @@ static void rbd_dev_image_release(struct rbd_device *rbd_dev)
 static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool mapping)
 {
 	int ret;
-	int tmp;
 
 	/*
 	 * Get the id from the image id object.  Unless there's an
@@ -4980,7 +4995,7 @@ static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool mapping)
 		goto err_out_format;
 
 	if (mapping) {
-		ret = rbd_dev_header_watch_sync(rbd_dev, true);
+		ret = rbd_dev_header_watch_sync(rbd_dev);
 		if (ret)
 			goto out_header_name;
 	}
@@ -5007,12 +5022,8 @@ static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool mapping)
 err_out_probe:
 	rbd_dev_unprobe(rbd_dev);
 err_out_watch:
-	if (mapping) {
-		tmp = rbd_dev_header_watch_sync(rbd_dev, false);
-		if (tmp)
-			rbd_warn(rbd_dev, "unable to tear down "
-					"watch request (%d)\n", tmp);
-	}
+	if (mapping)
+		rbd_dev_header_unwatch_sync(rbd_dev);
 out_header_name:
 	kfree(rbd_dev->header_name);
 	rbd_dev->header_name = NULL;
@@ -5191,16 +5202,14 @@ static ssize_t rbd_remove(struct bus_type *bus,
 	if (ret < 0 || already)
 		return ret;
 
-	ret = rbd_dev_header_watch_sync(rbd_dev, false);
-	if (ret)
-		rbd_warn(rbd_dev, "failed to cancel watch event (%d)\n", ret);
-
+	rbd_dev_header_unwatch_sync(rbd_dev);
 	/*
 	 * flush remaining watch callbacks - these must be complete
 	 * before the osd_client is shutdown
 	 */
 	dout("%s: flushing notifies", __func__);
 	ceph_osdc_flush_notifies(&rbd_dev->rbd_client->client->osdc);
+
 	/*
 	 * Don't free anything from rbd_dev->disk until after all
 	 * notifies are completely processed. Otherwise
-- 
1.7.10.4

[PATCH 2/2] rbd: tear down watch request if rbd_dev_device_setup() fails

[Ilya Dryomov]

Tear down watch request if rbd_dev_device_setup() fails.

Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com>
---
 drivers/block/rbd.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index c91108b760cf..e709f4ae117f 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -5101,6 +5101,12 @@ static ssize_t rbd_add(struct bus_type *bus,
 
 	rc = rbd_dev_device_setup(rbd_dev);
 	if (rc) {
+		/*
+		 * rbd_dev_header_unwatch_sync() can't be moved into
+		 * rbd_dev_image_release() without refactoring, see
+		 * commit 1f3ef78861ac.
+		 */
+		rbd_dev_header_unwatch_sync(rbd_dev);
 		rbd_dev_image_release(rbd_dev);
 		goto err_out_module;
 	}
-- 
1.7.10.4

Re: [PATCH 0/2] rbd: tear down watch request if rbd_dev_device_setup() fails

[Josh Durgin]

On 12/16/2013 12:05 PM, Ilya Dryomov wrote:
> Hello,
>
> Currently, if rbd_dev_device_setup() fails, we don't tear down our
> watch request.  Looks like this bug was introduced by 1f3ef78861ac,
> which moved a tear down call while fixing something else.  Fix it.
>
> These patches can be pulled from wip-rbd-unwatch branch of
> ceph-client.git.
>
> Thanks,
>
>                  Ilya
>
>
> Ilya Dryomov (2):
>    rbd: introduce rbd_dev_header_unwatch_sync() and switch to it
>    rbd: tear down watch request if rbd_dev_device_setup() fails
>
>   drivers/block/rbd.c |   41 ++++++++++++++++++++++++++++-------------
>   1 file changed, 28 insertions(+), 13 deletions(-)
>

These look good to me.

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>