[PATCH 0/5] rbd: fix some bugs

[PATCH 0/5] rbd: fix some bugs

[Alex Elder]

This series is mostly bug fixes of varying degrees of importance.
The last one is a simple cleanup.  These patches are available in
the "review/wip-rbd-cleanup-2" branch of the ceph-client git
git repository.

					-Alex

[PATCH 1/5] rbd: fix an incorrect assertion condition
[PATCH 2/5] rbd: support reading parent page data
[PATCH 3/5] rbd: set mapping read-only flag in rbd_add()
[PATCH 4/5] rbd: only set up watch for mapped images
[PATCH 5/5] rbd: kill rbd_img_request_get()

[PATCH 1/5] rbd: fix an incorrect assertion condition

[Alex Elder]

In rbd_img_obj_parent_read_full_callback() there is an assertion
intended to verify the size of the image request for a full parent
read was the size of the original request's target object.  But
assertion was looking at the parent image order rather than the
original one, and these values can differ.

Fix that.

This resolves:
    http://tracker.ceph.com/issues/4938

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 0d874a5..15ac2a5 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2186,13 +2186,13 @@ rbd_img_obj_parent_read_full_callback(struct
rbd_img_request *img_request)
 	result = img_request->result;
 	obj_size = img_request->length;
 	xferred = img_request->xferred;
+	rbd_img_request_put(img_request);

-	rbd_dev = img_request->rbd_dev;
+	rbd_assert(orig_request->img_request);
+	rbd_dev = orig_request->img_request->rbd_dev;
 	rbd_assert(rbd_dev);
 	rbd_assert(obj_size == (u64)1 << rbd_dev->header.obj_order);

-	rbd_img_request_put(img_request);
-
 	if (result)
 		goto out_err;

-- 
1.7.9.5

[PATCH 2/5] rbd: support reading parent page data

[Alex Elder]

Currently, rbd_img_parent_read() assumes the incoming object request
contains bio data.  But if a layered image is part of a multi-layer
stack of images will result in read requests of page data to parent
images.

Fortunately, it's not hard to add support for page data.

This resolves:
    http://tracker.ceph.com/issues/4939

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 15ac2a5..2a0e9b8 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2574,7 +2574,7 @@ static void rbd_img_parent_read(struct
rbd_obj_request *obj_request)
 	rbd_assert(obj_request_img_data_test(obj_request));
 	rbd_assert(obj_request->img_request != NULL);
 	rbd_assert(obj_request->result == (s32) -ENOENT);
-	rbd_assert(obj_request->type == OBJ_REQUEST_BIO);
+	rbd_assert(obj_request_type_valid(obj_request->type));

 	rbd_dev = obj_request->img_request->rbd_dev;
 	rbd_assert(rbd_dev->parent != NULL);
@@ -2590,8 +2590,12 @@ static void rbd_img_parent_read(struct
rbd_obj_request *obj_request)
 	rbd_obj_request_get(obj_request);
 	img_request->obj_request = obj_request;

-	result = rbd_img_request_fill(img_request, OBJ_REQUEST_BIO,
-					obj_request->bio_list);
+	if (obj_request->type == OBJ_REQUEST_BIO)
+		result = rbd_img_request_fill(img_request, OBJ_REQUEST_BIO,
+						obj_request->bio_list);
+	else
+		result = rbd_img_request_fill(img_request, OBJ_REQUEST_PAGES,
+						obj_request->pages);
 	if (result)
 		goto out_err;

-- 
1.7.9.5

[PATCH 3/5] rbd: set mapping read-only flag in rbd_add()

[Alex Elder]

The rbd_dev->mapping field for a parent image is not meaningful.
Since rbd_image_probe() is used both for images being mapped and
their parents, it doesn't make sense to set that flag in that
function.

So move the setting of the mapping.read_only flag out of
rbd_dev_image_probe() and into rbd_add() instead.

This resolves:
    http://tracker.ceph.com/issues/4940

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 2a0e9b8..dbfc44a 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -358,7 +358,7 @@ static ssize_t rbd_add(struct bus_type *bus, const
char *buf,
 		       size_t count);
 static ssize_t rbd_remove(struct bus_type *bus, const char *buf,
 			  size_t count);
-static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool read_only);
+static int rbd_dev_image_probe(struct rbd_device *rbd_dev);

 static struct bus_attribute rbd_bus_attrs[] = {
 	__ATTR(add, S_IWUSR, NULL, rbd_add),
@@ -4549,7 +4549,7 @@ static int rbd_dev_probe_parent(struct rbd_device
*rbd_dev)
 	if (!parent)
 		goto out_err;

-	ret = rbd_dev_image_probe(parent, true);
+	ret = rbd_dev_image_probe(parent);
 	if (ret < 0)
 		goto out_err;
 	rbd_dev->parent = parent;
@@ -4671,10 +4671,9 @@ static void rbd_dev_image_release(struct
rbd_device *rbd_dev)

 /*
  * Probe for the existence of the header object for the given rbd
- * device.  For format 2 images this includes determining the image
- * id.
+ * device.
  */
-static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool read_only)
+static int rbd_dev_image_probe(struct rbd_device *rbd_dev)
 {
 	int ret;
 	int tmp;
@@ -4709,12 +4708,6 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev, bool read_only)
 	if (ret)
 		goto err_out_probe;

-	/* If we are mapping a snapshot it must be marked read-only */
-
-	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
-		read_only = true;
-	rbd_dev->mapping.read_only = read_only;
-
 	ret = rbd_dev_probe_parent(rbd_dev);
 	if (ret)
 		goto err_out_probe;
@@ -4795,10 +4788,16 @@ static ssize_t rbd_add(struct bus_type *bus,
 	rbdc = NULL;		/* rbd_dev now owns this */
 	spec = NULL;		/* rbd_dev now owns this */

-	rc = rbd_dev_image_probe(rbd_dev, read_only);
+	rc = rbd_dev_image_probe(rbd_dev);
 	if (rc < 0)
 		goto err_out_rbd_dev;

+	/* If we are mapping a snapshot it must be marked read-only */
+
+	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
+		read_only = true;
+	rbd_dev->mapping.read_only = read_only;
+
 	rc = rbd_dev_device_setup(rbd_dev);
 	if (!rc)
 		return count;
-- 
1.7.9.5

[PATCH 4/5] rbd: only set up watch for mapped images

[Alex Elder]

Any changes to parent images are immaterial to any mapped clone.
So there is no need to have a watch event registered on header
objects except for the header object of an image that is mapped.
In fact, a watch request is a write operation, and we may only
have read access to a parent image.

We can't set up the watch request until we know the name of the
header object though.  So pass a flag to rbd_dev_image_probe() to
indicate whether this probe is for a mapping or for a parent image.

Change the second parameter to rbd_dev_header_watch_sync() be
Boolean while we're at it.

This resolves:
    http://tracker.ceph.com/issues/4941

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index dbfc44a..e417704 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -358,7 +358,7 @@ static ssize_t rbd_add(struct bus_type *bus, const
char *buf,
 		       size_t count);
 static ssize_t rbd_remove(struct bus_type *bus, const char *buf,
 			  size_t count);
-static int rbd_dev_image_probe(struct rbd_device *rbd_dev);
+static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool mapping);

 static struct bus_attribute rbd_bus_attrs[] = {
 	__ATTR(add, S_IWUSR, NULL, rbd_add),
@@ -2664,7 +2664,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, int 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;
@@ -2698,7 +2698,7 @@ static int rbd_dev_header_watch_sync(struct
rbd_device *rbd_dev, int start)
 					rbd_dev->watch_request->osd_req);

 	osd_req_op_watch_init(obj_request->osd_req, 0, CEPH_OSD_OP_WATCH,
-				rbd_dev->watch_event->cookie, 0, start);
+				rbd_dev->watch_event->cookie, 0, start ? 1 : 0);
 	rbd_osd_req_format_write(obj_request);

 	ret = rbd_obj_request_submit(osdc, obj_request);
@@ -4549,7 +4549,7 @@ static int rbd_dev_probe_parent(struct rbd_device
*rbd_dev)
 	if (!parent)
 		goto out_err;

-	ret = rbd_dev_image_probe(parent);
+	ret = rbd_dev_image_probe(parent, false);
 	if (ret < 0)
 		goto out_err;
 	rbd_dev->parent = parent;
@@ -4654,12 +4654,7 @@ static int rbd_dev_header_name(struct rbd_device
*rbd_dev)

 static void rbd_dev_image_release(struct rbd_device *rbd_dev)
 {
-	int ret;
-
 	rbd_dev_unprobe(rbd_dev);
-	ret = rbd_dev_header_watch_sync(rbd_dev, 0);
-	if (ret)
-		rbd_warn(rbd_dev, "failed to cancel watch event (%d)\n", ret);
 	kfree(rbd_dev->header_name);
 	rbd_dev->header_name = NULL;
 	rbd_dev->image_format = 0;
@@ -4671,9 +4666,11 @@ static void rbd_dev_image_release(struct
rbd_device *rbd_dev)

 /*
  * Probe for the existence of the header object for the given rbd
- * device.
+ * device.  If this image is the one being mapped (i.e., not a
+ * parent), initiate a watch on its header object before using that
+ * object to get detailed information about the rbd image.
  */
-static int rbd_dev_image_probe(struct rbd_device *rbd_dev)
+static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool mapping)
 {
 	int ret;
 	int tmp;
@@ -4693,9 +4690,11 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev)
 	if (ret)
 		goto err_out_format;

-	ret = rbd_dev_header_watch_sync(rbd_dev, 1);
-	if (ret)
-		goto out_header_name;
+	if (mapping) {
+		ret = rbd_dev_header_watch_sync(rbd_dev, true);
+		if (ret)
+			goto out_header_name;
+	}

 	if (rbd_dev->image_format == 1)
 		ret = rbd_dev_v1_header_info(rbd_dev);
@@ -4719,9 +4718,12 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev)
 err_out_probe:
 	rbd_dev_unprobe(rbd_dev);
 err_out_watch:
-	tmp = rbd_dev_header_watch_sync(rbd_dev, 0);
-	if (tmp)
-		rbd_warn(rbd_dev, "unable to tear down watch request\n");
+	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);
+	}
 out_header_name:
 	kfree(rbd_dev->header_name);
 	rbd_dev->header_name = NULL;
@@ -4788,7 +4790,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	rbdc = NULL;		/* rbd_dev now owns this */
 	spec = NULL;		/* rbd_dev now owns this */

-	rc = rbd_dev_image_probe(rbd_dev);
+	rc = rbd_dev_image_probe(rbd_dev, true);
 	if (rc < 0)
 		goto err_out_rbd_dev;

@@ -4910,10 +4912,13 @@ static ssize_t rbd_remove(struct bus_type *bus,
 	spin_unlock_irq(&rbd_dev->lock);
 	if (ret < 0)
 		goto done;
-	ret = count;
 	rbd_bus_del_dev(rbd_dev);
+	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_image_release(rbd_dev);
 	module_put(THIS_MODULE);
+	ret = count;
 done:
 	mutex_unlock(&ctl_mutex);

-- 
1.7.9.5

[PATCH 5/5] rbd: kill rbd_img_request_get()

[Alex Elder]

Get rid of rbd_img_request_get(), because it isn't used, and maybe
won't ever be needed.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index e417704..51c45e7 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1357,13 +1357,6 @@ static void rbd_obj_request_put(struct
rbd_obj_request *obj_request)
 	kref_put(&obj_request->kref, rbd_obj_request_destroy);
 }

-static void rbd_img_request_get(struct rbd_img_request *img_request)
-{
-	dout("%s: img %p (was %d)\n", __func__, img_request,
-		atomic_read(&img_request->kref.refcount));
-	kref_get(&img_request->kref);
-}
-
 static void rbd_img_request_destroy(struct kref *kref);
 static void rbd_img_request_put(struct rbd_img_request *img_request)
 {
@@ -1888,9 +1881,6 @@ static struct rbd_img_request *rbd_img_request_create(
 	INIT_LIST_HEAD(&img_request->obj_requests);
 	kref_init(&img_request->kref);

-	rbd_img_request_get(img_request);	/* Avoid a warning */
-	rbd_img_request_put(img_request);	/* TEMPORARY */
-
 	dout("%s: rbd_dev %p %s %llu/%llu -> img %p\n", __func__, rbd_dev,
 		write_request ? "write" : "read", offset, length,
 		img_request);
-- 
1.7.9.5

Re: [PATCH 0/5] rbd: fix some bugs

[Josh Durgin]

On 05/08/2013 03:06 PM, Alex Elder wrote:
> This series is mostly bug fixes of varying degrees of importance.
> The last one is a simple cleanup.  These patches are available in
> the "review/wip-rbd-cleanup-2" branch of the ceph-client git
> git repository.
>
> 					-Alex
>
> [PATCH 1/5] rbd: fix an incorrect assertion condition
> [PATCH 2/5] rbd: support reading parent page data
> [PATCH 3/5] rbd: set mapping read-only flag in rbd_add()
> [PATCH 4/5] rbd: only set up watch for mapped images
> [PATCH 5/5] rbd: kill rbd_img_request_get()

These all look good.

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