[PATCH 0/5] rbd: eliminate object version tracking

[PATCH 0/5] rbd: eliminate object version tracking

[Alex Elder]

As of this patch:
    rbd: set up watch in rbd_dev_image_probe()
there is no longer any need to keep track of the
object version for an rbd header object.  Getting
rid of it allows quite a bit of code to get
simplified, and the patches after the first in
this series do just that.

					-Alex

[PATCH 1/5] rbd: stop tracking header object version
[PATCH 2/5] rbd: get rid of some version parameters
[PATCH 3/5] rbd: more version parameter removal
[PATCH 4/5] rbd: drop rbd_obj_method_sync() version parameter
[PATCH 5/5] rbd: drop obj_request->version

[PATCH 1/5] rbd: stop tracking header object version

[Alex Elder]

The rbd code takes care to maintain the version of the header
object.  This was done in hopes of using it to detect a change in
the object between reading it and setting up a watch request to
be notified of changes.

The mechanism was never fully implemented, however.  And we now
avoid the original problem by setting up the watch request before
ever reading the content of the header.

The osd doesn't interpret the object version supplied with a WATCH
osd op, nor does it use the version supplied with a NOTIFY_ACK op
(we can just supply 0 for both).  There is therefore no need to
maintain the header's object version any more, so stop doing so.

We'll be able to simplify some more rbd code in the next few patches
as a result of this.

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

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index ab5c901..c4707dc 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -110,8 +110,6 @@ struct rbd_image_header {

 	u64 stripe_unit;
 	u64 stripe_count;
-
-	u64 obj_version;
 };

 /*
@@ -2554,8 +2552,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,
-				rbd_dev->header.obj_version, start);
+				rbd_dev->watch_event->cookie, 0, start);
 	rbd_osd_req_format_write(obj_request);

 	ret = rbd_obj_request_submit(osdc, obj_request);
@@ -2987,8 +2984,6 @@ static int rbd_read_header(struct rbd_device *rbd_dev,
 	if (IS_ERR(ondisk))
 		return PTR_ERR(ondisk);
 	ret = rbd_header_from_disk(header, ondisk);
-	if (ret >= 0)
-		header->obj_version = ver;
 	kfree(ondisk);

 	return ret;
@@ -3044,9 +3039,6 @@ static int rbd_dev_v1_refresh(struct rbd_device
*rbd_dev, u64 *hver)
 	/* osd requests may still refer to snapc */
 	ceph_put_snap_context(rbd_dev->header.snapc);

-	if (hver)
-		*hver = h.obj_version;
-	rbd_dev->header.obj_version = h.obj_version;
 	rbd_dev->header.image_size = h.image_size;
 	rbd_dev->header.snapc = h.snapc;
 	rbd_dev->header.snap_names = h.snap_names;
@@ -4656,7 +4648,6 @@ static int rbd_dev_v2_probe(struct rbd_device
*rbd_dev)
 	ret = rbd_dev_v2_snap_context(rbd_dev, &ver);
 	if (ret)
 		goto out_err;
-	rbd_dev->header.obj_version = ver;

 	dout("discovered version 2 image, header name is %s\n",
 		rbd_dev->header_name);
-- 
1.7.9.5

[PATCH 2/5] rbd: get rid of some version parameters

[Alex Elder]

Several functions in rbd have parameters meant to allow the version
of an object to be passed in or out.  The purpose of those was to
allow the version of a header object to be maintained, but we no
longer do that.  As a result, these parameters are never actually
needed or used, so get rid of them.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index c4707dc..0bfc3e9 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2838,8 +2838,7 @@ static void rbd_free_disk(struct rbd_device *rbd_dev)

 static int rbd_obj_read_sync(struct rbd_device *rbd_dev,
 				const char *object_name,
-				u64 offset, u64 length,
-				void *buf, u64 *version)
+				u64 offset, u64 length, void *buf)

 {
 	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
@@ -2890,10 +2889,8 @@ static int rbd_obj_read_sync(struct rbd_device
*rbd_dev,
 	rbd_assert(obj_request->xferred <= (u64) SIZE_MAX);
 	size = (size_t) obj_request->xferred;
 	ceph_copy_from_page_vector(pages, buf, 0, size);
-	rbd_assert(size <= (size_t) INT_MAX);
-	ret = (int) size;
-	if (version)
-		*version = obj_request->version;
+	rbd_assert(size <= (size_t)INT_MAX);
+	ret = (int)size;
 out:
 	if (obj_request)
 		rbd_obj_request_put(obj_request);
@@ -2914,7 +2911,7 @@ out:
  * Returns a pointer-coded errno if a failure occurs.
  */
 static struct rbd_image_header_ondisk *
-rbd_dev_v1_header_read(struct rbd_device *rbd_dev, u64 *version)
+rbd_dev_v1_header_read(struct rbd_device *rbd_dev)
 {
 	struct rbd_image_header_ondisk *ondisk = NULL;
 	u32 snap_count = 0;
@@ -2942,7 +2939,7 @@ rbd_dev_v1_header_read(struct rbd_device *rbd_dev,
u64 *version)
 			return ERR_PTR(-ENOMEM);

 		ret = rbd_obj_read_sync(rbd_dev, rbd_dev->header_name,
-				       0, size, ondisk, version);
+				       0, size, ondisk);
 		if (ret < 0)
 			goto out_err;
 		if ((size_t)ret < size) {
@@ -2977,10 +2974,9 @@ static int rbd_read_header(struct rbd_device
*rbd_dev,
 			   struct rbd_image_header *header)
 {
 	struct rbd_image_header_ondisk *ondisk;
-	u64 ver = 0;
 	int ret;

-	ondisk = rbd_dev_v1_header_read(rbd_dev, &ver);
+	ondisk = rbd_dev_v1_header_read(rbd_dev);
 	if (IS_ERR(ondisk))
 		return PTR_ERR(ondisk);
 	ret = rbd_header_from_disk(header, ondisk);
-- 
1.7.9.5

[PATCH 3/5] rbd: more version parameter removal

[Alex Elder]

Continued from the last patch, more parameters that can go away
because we no longer have a need to track object versions.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 0bfc3e9..e405c61 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -429,8 +429,8 @@ static int rbd_img_obj_request_submit(struct
rbd_obj_request *obj_request);
 static void rbd_img_parent_read(struct rbd_obj_request *obj_request);
 static void rbd_dev_remove_parent(struct rbd_device *rbd_dev);

-static int rbd_dev_refresh(struct rbd_device *rbd_dev, u64 *hver);
-static int rbd_dev_v2_refresh(struct rbd_device *rbd_dev, u64 *hver);
+static int rbd_dev_refresh(struct rbd_device *rbd_dev);
+static int rbd_dev_v2_refresh(struct rbd_device *rbd_dev);

 static int rbd_open(struct block_device *bdev, fmode_t mode)
 {
@@ -2468,8 +2468,7 @@ out_err:
 	obj_request_done_set(obj_request);
 }

-static int rbd_obj_notify_ack(struct rbd_device *rbd_dev,
-				   u64 ver, u64 notify_id)
+static int rbd_obj_notify_ack(struct rbd_device *rbd_dev, u64 notify_id)
 {
 	struct rbd_obj_request *obj_request;
 	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
@@ -2487,7 +2486,7 @@ static int rbd_obj_notify_ack(struct rbd_device
*rbd_dev,
 	obj_request->callback = rbd_obj_request_put;

 	osd_req_op_watch_init(obj_request->osd_req, 0, CEPH_OSD_OP_NOTIFY_ACK,
-					notify_id, ver, 0);
+					notify_id, 0, 0);
 	rbd_osd_req_format_read(obj_request);

 	ret = rbd_obj_request_submit(osdc, obj_request);
@@ -2501,17 +2500,16 @@ out:
 static void rbd_watch_cb(u64 ver, u64 notify_id, u8 opcode, void *data)
 {
 	struct rbd_device *rbd_dev = (struct rbd_device *)data;
-	u64 hver;

 	if (!rbd_dev)
 		return;

 	dout("%s: \"%s\" notify_id %llu opcode %u\n", __func__,
-		rbd_dev->header_name, (unsigned long long) notify_id,
-		(unsigned int) opcode);
-	(void)rbd_dev_refresh(rbd_dev, &hver);
+		rbd_dev->header_name, (unsigned long long)notify_id,
+		(unsigned int)opcode);
+	(void)rbd_dev_refresh(rbd_dev);

-	rbd_obj_notify_ack(rbd_dev, hver, notify_id);
+	rbd_obj_notify_ack(rbd_dev, notify_id);
 }

 /*
@@ -3014,7 +3012,7 @@ static void rbd_update_mapping_size(struct
rbd_device *rbd_dev)
 /*
  * only read the first part of the ondisk header, without the snaps info
  */
-static int rbd_dev_v1_refresh(struct rbd_device *rbd_dev, u64 *hver)
+static int rbd_dev_v1_refresh(struct rbd_device *rbd_dev)
 {
 	int ret;
 	struct rbd_image_header h;
@@ -3051,7 +3049,7 @@ static int rbd_dev_v1_refresh(struct rbd_device
*rbd_dev, u64 *hver)
 	return ret;
 }

-static int rbd_dev_refresh(struct rbd_device *rbd_dev, u64 *hver)
+static int rbd_dev_refresh(struct rbd_device *rbd_dev)
 {
 	u64 image_size;
 	int ret;
@@ -3060,9 +3058,9 @@ static int rbd_dev_refresh(struct rbd_device
*rbd_dev, u64 *hver)
 	image_size = rbd_dev->header.image_size;
 	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
 	if (rbd_dev->image_format == 1)
-		ret = rbd_dev_v1_refresh(rbd_dev, hver);
+		ret = rbd_dev_v1_refresh(rbd_dev);
 	else
-		ret = rbd_dev_v2_refresh(rbd_dev, hver);
+		ret = rbd_dev_v2_refresh(rbd_dev);
 	mutex_unlock(&ctl_mutex);
 	if (ret)
 		rbd_warn(rbd_dev, "got notification but failed to "
@@ -3271,7 +3269,7 @@ static ssize_t rbd_image_refresh(struct device *dev,
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
 	int ret;

-	ret = rbd_dev_refresh(rbd_dev, NULL);
+	ret = rbd_dev_refresh(rbd_dev);

 	return ret < 0 ? ret : size;
 }
@@ -3824,7 +3822,7 @@ out_err:
 	return ret;
 }

-static int rbd_dev_v2_snap_context(struct rbd_device *rbd_dev, u64 *ver)
+static int rbd_dev_v2_snap_context(struct rbd_device *rbd_dev)
 {
 	size_t size;
 	int ret;
@@ -3850,7 +3848,7 @@ static int rbd_dev_v2_snap_context(struct
rbd_device *rbd_dev, u64 *ver)

 	ret = rbd_obj_method_sync(rbd_dev, rbd_dev->header_name,
 				"rbd", "get_snapcontext", NULL, 0,
-				reply_buf, size, ver);
+				reply_buf, size, NULL);
 	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
 	if (ret < 0)
 		goto out;
@@ -3978,7 +3976,7 @@ static const char *rbd_dev_snap_info(struct
rbd_device *rbd_dev, u32 which,
 	return ERR_PTR(-EINVAL);
 }

-static int rbd_dev_v2_refresh(struct rbd_device *rbd_dev, u64 *hver)
+static int rbd_dev_v2_refresh(struct rbd_device *rbd_dev)
 {
 	int ret;

@@ -3989,7 +3987,7 @@ static int rbd_dev_v2_refresh(struct rbd_device
*rbd_dev, u64 *hver)
 		goto out;
 	rbd_update_mapping_size(rbd_dev);

-	ret = rbd_dev_v2_snap_context(rbd_dev, hver);
+	ret = rbd_dev_v2_snap_context(rbd_dev);
 	dout("rbd_dev_v2_snap_context returned %d\n", ret);
 	if (ret)
 		goto out;
@@ -4591,7 +4589,6 @@ out_err:
 static int rbd_dev_v2_probe(struct rbd_device *rbd_dev)
 {
 	int ret;
-	u64 ver = 0;

 	ret = rbd_dev_v2_image_size(rbd_dev);
 	if (ret)
@@ -4641,7 +4638,7 @@ static int rbd_dev_v2_probe(struct rbd_device
*rbd_dev)

 	/* Get the snapshot context, plus the header version */

-	ret = rbd_dev_v2_snap_context(rbd_dev, &ver);
+	ret = rbd_dev_v2_snap_context(rbd_dev);
 	if (ret)
 		goto out_err;

-- 
1.7.9.5

[PATCH 4/5] rbd: drop rbd_obj_method_sync() version parameter

[Alex Elder]

Only NULL is passed as the version argument to rbd_obj_method_sync(),
so get rid of it.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index e405c61..ddf4305 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1,3 +1,4 @@
+
 /*
    rbd.c -- Export ceph rados objects as a Linux block device

@@ -2602,8 +2603,7 @@ static int rbd_obj_method_sync(struct rbd_device
*rbd_dev,
 			     const void *outbound,
 			     size_t outbound_size,
 			     void *inbound,
-			     size_t inbound_size,
-			     u64 *version)
+			     size_t inbound_size)
 {
 	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
 	struct rbd_obj_request *obj_request;
@@ -2669,8 +2669,6 @@ static int rbd_obj_method_sync(struct rbd_device
*rbd_dev,
 	rbd_assert(obj_request->xferred < (u64)INT_MAX);
 	ret = (int)obj_request->xferred;
 	ceph_copy_from_page_vector(pages, inbound, 0, obj_request->xferred);
-	if (version)
-		*version = obj_request->version;
 out:
 	if (obj_request)
 		rbd_obj_request_put(obj_request);
@@ -3463,7 +3461,7 @@ static int _rbd_dev_v2_snap_size(struct rbd_device
*rbd_dev, u64 snap_id,
 	ret = rbd_obj_method_sync(rbd_dev, rbd_dev->header_name,
 				"rbd", "get_size",
 				&snapid, sizeof (snapid),
-				&size_buf, sizeof (size_buf), NULL);
+				&size_buf, sizeof (size_buf));
 	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
 	if (ret < 0)
 		return ret;
@@ -3500,7 +3498,7 @@ static int rbd_dev_v2_object_prefix(struct
rbd_device *rbd_dev)

 	ret = rbd_obj_method_sync(rbd_dev, rbd_dev->header_name,
 				"rbd", "get_object_prefix", NULL, 0,
-				reply_buf, RBD_OBJ_PREFIX_LEN_MAX, NULL);
+				reply_buf, RBD_OBJ_PREFIX_LEN_MAX);
 	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
 	if (ret < 0)
 		goto out;
@@ -3536,7 +3534,7 @@ static int _rbd_dev_v2_snap_features(struct
rbd_device *rbd_dev, u64 snap_id,
 	ret = rbd_obj_method_sync(rbd_dev, rbd_dev->header_name,
 				"rbd", "get_features",
 				&snapid, sizeof (snapid),
-				&features_buf, sizeof (features_buf), NULL);
+				&features_buf, sizeof (features_buf));
 	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
 	if (ret < 0)
 		return ret;
@@ -3593,7 +3591,7 @@ static int rbd_dev_v2_parent_info(struct
rbd_device *rbd_dev)
 	ret = rbd_obj_method_sync(rbd_dev, rbd_dev->header_name,
 				"rbd", "get_parent",
 				&snapid, sizeof (snapid),
-				reply_buf, size, NULL);
+				reply_buf, size);
 	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
 	if (ret < 0)
 		goto out_err;
@@ -3650,7 +3648,7 @@ static int rbd_dev_v2_striping_info(struct
rbd_device *rbd_dev)

 	ret = rbd_obj_method_sync(rbd_dev, rbd_dev->header_name,
 				"rbd", "get_stripe_unit_count", NULL, 0,
-				(char *)&striping_info_buf, size, NULL);
+				(char *)&striping_info_buf, size);
 	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
 	if (ret < 0)
 		return ret;
@@ -3717,7 +3715,7 @@ static char *rbd_dev_image_name(struct rbd_device
*rbd_dev)
 	ret = rbd_obj_method_sync(rbd_dev, RBD_DIRECTORY,
 				"rbd", "dir_get_name",
 				image_id, image_id_size,
-				reply_buf, size, NULL);
+				reply_buf, size);
 	if (ret < 0)
 		goto out;
 	p = reply_buf;
@@ -3848,7 +3846,7 @@ static int rbd_dev_v2_snap_context(struct
rbd_device *rbd_dev)

 	ret = rbd_obj_method_sync(rbd_dev, rbd_dev->header_name,
 				"rbd", "get_snapcontext", NULL, 0,
-				reply_buf, size, NULL);
+				reply_buf, size);
 	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
 	if (ret < 0)
 		goto out;
@@ -3913,7 +3911,7 @@ static const char *rbd_dev_v2_snap_name(struct
rbd_device *rbd_dev, u32 which)
 	ret = rbd_obj_method_sync(rbd_dev, rbd_dev->header_name,
 				"rbd", "get_snapshot_name",
 				&snap_id, sizeof (snap_id),
-				reply_buf, size, NULL);
+				reply_buf, size);
 	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
 	if (ret < 0) {
 		snap_name = ERR_PTR(ret);
@@ -4506,7 +4504,7 @@ static int rbd_dev_image_id(struct rbd_device
*rbd_dev)

 	ret = rbd_obj_method_sync(rbd_dev, object_name,
 				"rbd", "get_id", NULL, 0,
-				response, RBD_IMAGE_ID_LEN_MAX, NULL);
+				response, RBD_IMAGE_ID_LEN_MAX);
 	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
 	if (ret == -ENOENT) {
 		image_id = kstrdup("", GFP_KERNEL);
-- 
1.7.9.5

[PATCH 5/5] rbd: drop obj_request->version

[Alex Elder]

Nothing ever uses the version field maintained in the object request
structure any more, so get rid of it.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index ddf4305..15e84d8 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -225,7 +225,6 @@ struct rbd_obj_request {
 	struct ceph_osd_request	*osd_req;

 	u64			xferred;	/* bytes transferred */
-	u64			version;
 	int			result;

 	rbd_obj_callback_t	callback;
@@ -1486,7 +1485,6 @@ static void rbd_osd_req_callback(struct
ceph_osd_request *osd_req,

 	if (osd_req->r_result < 0)
 		obj_request->result = osd_req->r_result;
-	obj_request->version = le64_to_cpu(osd_req->r_reassert_version.version);

 	BUG_ON(osd_req->r_num_ops > 2);

-- 
1.7.9.5

Re: [PATCH 0/5] rbd: eliminate object version tracking

[Josh Durgin]

On 04/30/2013 05:28 AM, Alex Elder wrote:
> As of this patch:
>      rbd: set up watch in rbd_dev_image_probe()
> there is no longer any need to keep track of the
> object version for an rbd header object.  Getting
> rid of it allows quite a bit of code to get
> simplified, and the patches after the first in
> this series do just that.
>
> 					-Alex
>
> [PATCH 1/5] rbd: stop tracking header object version
> [PATCH 2/5] rbd: get rid of some version parameters
> [PATCH 3/5] rbd: more version parameter removal
> [PATCH 4/5] rbd: drop rbd_obj_method_sync() version parameter
> [PATCH 5/5] rbd: drop obj_request->version

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