[PATCH 0/7] rbd: more miscellaneous refactoring

[PATCH 0/7] rbd: more miscellaneous refactoring

[Alex Elder]

These patches consist of various refactoring that didn't fit
into any particular category.  Several involve changing function
prototypes to return a pointer, or return some other bit of
information, in order to simplify or constrain things.

Read their individual descriptions for better detail.

					-Alex

[PATCH 1/7] rbd: have __rbd_add_snap_dev() return a pointer
[PATCH 2/7] rbd: make rbd_create_rw_ops() return a pointer
[PATCH 3/7] rbd: pass null version pointer in add_snap()
[PATCH 4/7] rbd: always pass ops array to rbd_req_sync_op()
[PATCH 5/7] rbd: fixes in rbd_header_from_disk()
[PATCH 6/7] rbd: return obj version in __rbd_refresh_header()
[PATCH 7/7] rbd: create rbd_refresh_header()

[PATCH 1/7] rbd: have __rbd_add_snap_dev() return a pointer

[Alex Elder]

It's not obvious whether the snapshot pointer whose address is
provided to __rbd_add_snap_dev() will be assigned by that function.
Change it to return the snapshot, or a pointer-coded errno in the
event of a failure.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index fd5f3038..fd91964 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2025,15 +2025,21 @@ static int rbd_register_snap_dev(struct rbd_snap
*snap,
 	return ret;
 }

-static int __rbd_add_snap_dev(struct rbd_device *rbd_dev,
-			      int i, const char *name,
-			      struct rbd_snap **snapp)
+static struct rbd_snap *__rbd_add_snap_dev(struct rbd_device *rbd_dev,
+					      int i, const char *name)
 {
+	struct rbd_snap *snap;
 	int ret;
-	struct rbd_snap *snap = kzalloc(sizeof(*snap), GFP_KERNEL);
+
+	snap = kzalloc(sizeof (*snap), GFP_KERNEL);
 	if (!snap)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
+
+	ret = -ENOMEM;
 	snap->name = kstrdup(name, GFP_KERNEL);
+	if (!snap->name)
+		goto err;
+
 	snap->size = rbd_dev->header.snap_sizes[i];
 	snap->id = rbd_dev->header.snapc->snaps[i];
 	if (device_is_registered(&rbd_dev->dev)) {
@@ -2041,12 +2047,14 @@ static int __rbd_add_snap_dev(struct rbd_device
*rbd_dev,
 		if (ret < 0)
 			goto err;
 	}
-	*snapp = snap;
-	return 0;
+
+	return snap;
+
 err:
 	kfree(snap->name);
 	kfree(snap);
-	return ret;
+
+	return ERR_PTR(ret);
 }

 /*
@@ -2079,7 +2087,6 @@ static int __rbd_init_snaps_header(struct
rbd_device *rbd_dev)
 	const char *name, *first_name;
 	int i = rbd_dev->header.total_snaps;
 	struct rbd_snap *snap, *old_snap = NULL;
-	int ret;
 	struct list_head *p, *n;

 	first_name = rbd_dev->header.snap_names;
@@ -2122,9 +2129,9 @@ static int __rbd_init_snaps_header(struct
rbd_device *rbd_dev)
 			if (cur_id >= old_snap->id)
 				break;
 			/* a new snapshot */
-			ret = __rbd_add_snap_dev(rbd_dev, i - 1, name, &snap);
-			if (ret < 0)
-				return ret;
+			snap = __rbd_add_snap_dev(rbd_dev, i - 1, name);
+			if (IS_ERR(snap))
+				return PTR_ERR(snap);

 			/* note that we add it backward so using n and not p */
 			list_add(&snap->node, n);
@@ -2138,9 +2145,9 @@ static int __rbd_init_snaps_header(struct
rbd_device *rbd_dev)
 			WARN_ON(1);
 			return -EINVAL;
 		}
-		ret = __rbd_add_snap_dev(rbd_dev, i - 1, name, &snap);
-		if (ret < 0)
-			return ret;
+		snap = __rbd_add_snap_dev(rbd_dev, i - 1, name);
+		if (IS_ERR(snap))
+			return PTR_ERR(snap);
 		list_add(&snap->node, &rbd_dev->snaps);
 	}

-- 
1.7.9.5

[PATCH 2/7] rbd: make rbd_create_rw_ops() return a pointer

[Alex Elder]

Either rbd_create_rw_ops() will succeed, or it will fail because a
memory allocation failed.  Have it just return a valid pointer or
null rather than stuffing a pointer into a provided address and
returning an errno.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index fd91964..4d8b52c 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -789,22 +789,24 @@ err_out:
 /*
  * helpers for osd request op vectors.
  */
-static int rbd_create_rw_ops(struct ceph_osd_req_op **ops,
-			    int num_ops,
-			    int opcode,
-			    u32 payload_len)
-{
-	*ops = kzalloc(sizeof(struct ceph_osd_req_op) * (num_ops + 1),
-		       GFP_NOIO);
-	if (!*ops)
-		return -ENOMEM;
-	(*ops)[0].op = opcode;
+static struct ceph_osd_req_op *rbd_create_rw_ops(int num_ops,
+					int opcode, u32 payload_len)
+{
+	struct ceph_osd_req_op *ops;
+
+	ops = kzalloc(sizeof (*ops) * (num_ops + 1), GFP_NOIO);
+	if (!ops)
+		return NULL;
+
+	ops[0].op = opcode;
+
 	/*
 	 * op extent offset and length will be set later on
 	 * in calc_raw_layout()
 	 */
-	(*ops)[0].payload_len = payload_len;
-	return 0;
+	ops[0].payload_len = payload_len;
+
+	return ops;
 }

 static void rbd_destroy_ops(struct ceph_osd_req_op *ops)
@@ -1039,8 +1041,9 @@ static int rbd_req_sync_op(struct rbd_device *rbd_dev,

 	if (!orig_ops) {
 		payload_len = (flags & CEPH_OSD_FLAG_WRITE ? len : 0);
-		ret = rbd_create_rw_ops(&ops, 1, opcode, payload_len);
-		if (ret < 0)
+		ret = -ENOMEM;
+		ops = rbd_create_rw_ops(1, opcode, payload_len);
+		if (!ops)
 			goto done;

 		if ((flags & CEPH_OSD_FLAG_WRITE) && buf) {
@@ -1103,8 +1106,9 @@ static int rbd_do_op(struct request *rq,

 	payload_len = (flags & CEPH_OSD_FLAG_WRITE ? seg_len : 0);

-	ret = rbd_create_rw_ops(&ops, 1, opcode, payload_len);
-	if (ret < 0)
+	ret = -ENOMEM;
+	ops = rbd_create_rw_ops(1, opcode, payload_len);
+	if (!ops)
 		goto done;

 	/* we've taken care of segment sizes earlier when we
@@ -1190,9 +1194,9 @@ static int rbd_req_sync_notify_ack(struct
rbd_device *rbd_dev,
 	struct ceph_osd_req_op *ops;
 	int ret;

-	ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_NOTIFY_ACK, 0);
-	if (ret < 0)
-		return ret;
+	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_NOTIFY_ACK, 0);
+	if (!ops)
+		return -ENOMEM;

 	ops[0].watch.ver = cpu_to_le64(ver);
 	ops[0].watch.cookie = notify_id;
@@ -1239,10 +1243,11 @@ static int rbd_req_sync_watch(struct rbd_device
*rbd_dev)
 {
 	struct ceph_osd_req_op *ops;
 	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
+	int ret;

-	int ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_WATCH, 0);
-	if (ret < 0)
-		return ret;
+	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_WATCH, 0);
+	if (!ops)
+		return -ENOMEM;

 	ret = ceph_osdc_create_event(osdc, rbd_watch_cb, 0,
 				     (void *)rbd_dev, &rbd_dev->watch_event);
@@ -1282,10 +1287,11 @@ fail:
 static int rbd_req_sync_unwatch(struct rbd_device *rbd_dev)
 {
 	struct ceph_osd_req_op *ops;
+	int ret;

-	int ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_WATCH, 0);
-	if (ret < 0)
-		return ret;
+	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_WATCH, 0);
+	if (!ops)
+		return -ENOMEM;

 	ops[0].watch.ver = 0;
 	ops[0].watch.cookie = cpu_to_le64(rbd_dev->watch_event->cookie);
@@ -1332,9 +1338,9 @@ static int rbd_req_sync_notify(struct rbd_device
*rbd_dev)
 	int payload_len = sizeof(u32) + sizeof(u32);
 	int ret;

-	ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_NOTIFY, payload_len);
-	if (ret < 0)
-		return ret;
+	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_NOTIFY, payload_len);
+	if (!ops)
+		return -ENOMEM;

 	info.rbd_dev = rbd_dev;

@@ -1385,10 +1391,12 @@ static int rbd_req_sync_exec(struct rbd_device
*rbd_dev,
 	struct ceph_osd_req_op *ops;
 	int class_name_len = strlen(class_name);
 	int method_name_len = strlen(method_name);
-	int ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_CALL,
+	int ret;
+
+	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_CALL,
 				    class_name_len + method_name_len + len);
-	if (ret < 0)
-		return ret;
+	if (!ops)
+		return -ENOMEM;

 	ops[0].cls.class_name = class_name;
 	ops[0].cls.class_len = (__u8) class_name_len;
-- 
1.7.9.5

[PATCH 3/7] rbd: pass null version pointer in add_snap()

[Alex Elder]

rbd_header_add_snap() passes the address of a version variable to
rbd_req_sync_exec(), but it ignores the result.  Just pass a null
pointer instead.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 4d8b52c..eacf255 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1664,7 +1664,6 @@ static int rbd_header_add_snap(struct rbd_device
*rbd_dev,
 	u64 new_snapid;
 	int ret;
 	void *data, *p, *e;
-	u64 ver;
 	struct ceph_mon_client *monc;

 	/* we should create a snapshot only if we're pointing at the head */
@@ -1689,7 +1688,7 @@ static int rbd_header_add_snap(struct rbd_device
*rbd_dev,

 	ret = rbd_req_sync_exec(rbd_dev, rbd_dev->header_name,
 				"rbd", "snap_add",
-				data, p - data, &ver);
+				data, p - data, NULL);

 	kfree(data);

-- 
1.7.9.5

[PATCH 4/7] rbd: always pass ops array to rbd_req_sync_op()

[Alex Elder]

All of the callers of rbd_req_sync_op() except one pass a non-null
"ops" pointer.  The only one that does not is rbd_req_sync_read(),
which passes CEPH_OSD_OP_READ as its "opcode" and, CEPH_OSD_FLAG_READ
for "flags".

By allocating the ops array in rbd_req_sync_read() and moving the
special case code for the null ops pointer into it, it becomes
clear that much of that code is not even necessary.

In addition, the "opcode" argument to rbd_req_sync_op() is never
actually used, so get rid of that.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index eacf255..4584500 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1019,9 +1019,8 @@ static void rbd_simple_req_cb(struct
ceph_osd_request *req, struct ceph_msg *msg
 static int rbd_req_sync_op(struct rbd_device *rbd_dev,
 			   struct ceph_snap_context *snapc,
 			   u64 snapid,
-			   int opcode,
 			   int flags,
-			   struct ceph_osd_req_op *orig_ops,
+			   struct ceph_osd_req_op *ops,
 			   const char *object_name,
 			   u64 ofs, u64 len,
 			   char *buf,
@@ -1031,28 +1030,14 @@ static int rbd_req_sync_op(struct rbd_device
*rbd_dev,
 	int ret;
 	struct page **pages;
 	int num_pages;
-	struct ceph_osd_req_op *ops = orig_ops;
-	u32 payload_len;
+
+	BUG_ON(ops == NULL);

 	num_pages = calc_pages_for(ofs , len);
 	pages = ceph_alloc_page_vector(num_pages, GFP_KERNEL);
 	if (IS_ERR(pages))
 		return PTR_ERR(pages);

-	if (!orig_ops) {
-		payload_len = (flags & CEPH_OSD_FLAG_WRITE ? len : 0);
-		ret = -ENOMEM;
-		ops = rbd_create_rw_ops(1, opcode, payload_len);
-		if (!ops)
-			goto done;
-
-		if ((flags & CEPH_OSD_FLAG_WRITE) && buf) {
-			ret = ceph_copy_to_page_vector(pages, buf, ofs, len);
-			if (ret < 0)
-				goto done_ops;
-		}
-	}
-
 	ret = rbd_do_request(NULL, rbd_dev, snapc, snapid,
 			  object_name, ofs, len, NULL,
 			  pages, num_pages,
@@ -1062,14 +1047,11 @@ static int rbd_req_sync_op(struct rbd_device
*rbd_dev,
 			  NULL,
 			  linger_req, ver);
 	if (ret < 0)
-		goto done_ops;
+		goto done;

 	if ((flags & CEPH_OSD_FLAG_READ) && buf)
 		ret = ceph_copy_from_page_vector(pages, buf, ofs, ret);

-done_ops:
-	if (!orig_ops)
-		rbd_destroy_ops(ops);
 done:
 	ceph_release_page_vector(pages, num_pages);
 	return ret;
@@ -1176,12 +1158,20 @@ static int rbd_req_sync_read(struct rbd_device
*rbd_dev,
 			  char *buf,
 			  u64 *ver)
 {
-	return rbd_req_sync_op(rbd_dev, NULL,
+	struct ceph_osd_req_op *ops;
+	int ret;
+
+	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_READ, 0);
+	if (!ops)
+		return -ENOMEM;
+
+	ret = rbd_req_sync_op(rbd_dev, NULL,
 			       snapid,
-			       CEPH_OSD_OP_READ,
 			       CEPH_OSD_FLAG_READ,
-			       NULL,
-			       object_name, ofs, len, buf, NULL, ver);
+			       ops, object_name, ofs, len, buf, NULL, ver);
+	rbd_destroy_ops(ops);
+
+	return ret;
 }

 /*
@@ -1260,7 +1250,6 @@ static int rbd_req_sync_watch(struct rbd_device
*rbd_dev)

 	ret = rbd_req_sync_op(rbd_dev, NULL,
 			      CEPH_NOSNAP,
-			      0,
 			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			      ops,
 			      rbd_dev->header_name,
@@ -1299,7 +1288,6 @@ static int rbd_req_sync_unwatch(struct rbd_device
*rbd_dev)

 	ret = rbd_req_sync_op(rbd_dev, NULL,
 			      CEPH_NOSNAP,
-			      0,
 			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			      ops,
 			      rbd_dev->header_name,
@@ -1357,7 +1345,6 @@ static int rbd_req_sync_notify(struct rbd_device
*rbd_dev)

 	ret = rbd_req_sync_op(rbd_dev, NULL,
 			       CEPH_NOSNAP,
-			       0,
 			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			       ops,
 			       rbd_dev->header_name,
@@ -1408,7 +1395,6 @@ static int rbd_req_sync_exec(struct rbd_device
*rbd_dev,

 	ret = rbd_req_sync_op(rbd_dev, NULL,
 			       CEPH_NOSNAP,
-			       0,
 			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			       ops,
 			       object_name, 0, 0, NULL, NULL, ver);
-- 
1.7.9.5

[PATCH 5/7] rbd: fixes in rbd_header_from_disk()

[Alex Elder]

This fixes a few issues in rbd_header_from_disk():
    - There is a check intended to catch overflow, but it's wrong in
      two ways.
	- First, the type we don't want to overflow is size_t, not
	  unsigned int, and there is now a SIZE_MAX we can use for
	  use with that type.
	- Second, we're allocating the snapshot ids and snapshot
	  image sizes separately (each has type u64; on disk they
          grouped together as a rbd_image_header_ondisk structure).
	  So we can use the size of u64 in this overflow check.
    - If there are no snapshots, then there should be no snapshot
      names.  Enforce this, and issue a warning if we encounter a
      header with no snapshots but a non-zero snap_names_len.
    - When saving the snapshot names into the header, be more direct
      in defining the offset in the on-disk structure from which
      they're being copied by using "snap_count" rather than "i"
      in the array index.
    - If an error occurs, the "snapc" and "snap_names" fields are
      freed at the end of the function.  Make those fields be null
      pointers after they're freed, to be explicit that they are
      no longer valid.
    - Finally, move the definition of the local variable "i" to the
      innermost scope in which it's needed.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 4584500..3daf8fb 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -494,14 +494,14 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 				 struct rbd_image_header_ondisk *ondisk,
 				 u32 allocated_snaps)
 {
-	u32 i, snap_count;
+	u32 snap_count;

 	if (!rbd_dev_ondisk_valid(ondisk))
 		return -ENXIO;

 	snap_count = le32_to_cpu(ondisk->snap_count);
-	if (snap_count > (UINT_MAX - sizeof(struct ceph_snap_context))
-			 / sizeof (*ondisk))
+	if (snap_count > (SIZE_MAX - sizeof(struct ceph_snap_context))
+				 / sizeof (u64))
 		return -EINVAL;
 	header->snapc = kmalloc(sizeof(struct ceph_snap_context) +
 				snap_count * sizeof(u64),
@@ -509,8 +509,8 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 	if (!header->snapc)
 		return -ENOMEM;

-	header->snap_names_len = le64_to_cpu(ondisk->snap_names_len);
 	if (snap_count) {
+		header->snap_names_len = le64_to_cpu(ondisk->snap_names_len);
 		header->snap_names = kmalloc(header->snap_names_len,
 					     GFP_KERNEL);
 		if (!header->snap_names)
@@ -520,6 +520,8 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 		if (!header->snap_sizes)
 			goto err_names;
 	} else {
+		WARN_ON(ondisk->snap_names_len);
+		header->snap_names_len = 0;
 		header->snap_names = NULL;
 		header->snap_sizes = NULL;
 	}
@@ -544,6 +546,8 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 	header->total_snaps = snap_count;

 	if (snap_count && allocated_snaps == snap_count) {
+		int i;
+
 		for (i = 0; i < snap_count; i++) {
 			header->snapc->snaps[i] =
 				le64_to_cpu(ondisk->snaps[i].id);
@@ -552,7 +556,7 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 		}

 		/* copy snapshot names */
-		memcpy(header->snap_names, &ondisk->snaps[i],
+		memcpy(header->snap_names, &ondisk->snaps[snap_count],
 			header->snap_names_len);
 	}

@@ -562,8 +566,11 @@ err_sizes:
 	kfree(header->snap_sizes);
 err_names:
 	kfree(header->snap_names);
+	header->snap_names = NULL;
 err_snapc:
 	kfree(header->snapc);
+	header->snapc = NULL;
+
 	return -ENOMEM;
 }

-- 
1.7.9.5

[PATCH 6/7] rbd: return obj version in __rbd_refresh_header()

[Alex Elder]

Add a new parameter to __rbd_refresh_header() through which the
version of the header object is passed back to the caller.  In most
cases this isn't needed.  The main motivation is to normalize
(almost) all calls to __rbd_refresh_header() so they are all
wrapped immediately by mutex_lock()/mutex_unlock().

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 3daf8fb..31be4ca 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -240,7 +240,7 @@ static void rbd_put_dev(struct rbd_device *rbd_dev)
 	put_device(&rbd_dev->dev);
 }

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

 static int rbd_open(struct block_device *bdev, fmode_t mode)
 {
@@ -1223,8 +1223,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
u8 opcode, void *data)
 	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n",
 		rbd_dev->header_name, notify_id, (int) opcode);
 	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-	rc = __rbd_refresh_header(rbd_dev);
-	hver = rbd_dev->header.obj_version;
+	rc = __rbd_refresh_header(rbd_dev, &hver);
 	mutex_unlock(&ctl_mutex);
 	if (rc)
 		pr_warning(RBD_DRV_NAME "%d got notification but failed to "
@@ -1702,7 +1701,7 @@ static void __rbd_remove_all_snaps(struct
rbd_device *rbd_dev)
 /*
  * only read the first part of the ondisk header, without the snaps info
  */
-static int __rbd_refresh_header(struct rbd_device *rbd_dev)
+static int __rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver)
 {
 	int ret;
 	struct rbd_image_header h;
@@ -1727,6 +1726,8 @@ static int __rbd_refresh_header(struct rbd_device
*rbd_dev)
 	/* 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.total_snaps = h.total_snaps;
@@ -1896,17 +1897,13 @@ static ssize_t rbd_image_refresh(struct device *dev,
 				 size_t size)
 {
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
-	int rc;
-	int ret = size;
+	int ret;

 	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-
-	rc = __rbd_refresh_header(rbd_dev);
-	if (rc < 0)
-		ret = rc;
-
+	ret = __rbd_refresh_header(rbd_dev, NULL);
 	mutex_unlock(&ctl_mutex);
-	return ret;
+
+	return ret < 0 ? ret : size;
 }

 static DEVICE_ATTR(size, S_IRUGO, rbd_size_show, NULL);
@@ -2195,7 +2192,7 @@ static int rbd_init_watch_dev(struct rbd_device
*rbd_dev)
 		ret = rbd_req_sync_watch(rbd_dev);
 		if (ret == -ERANGE) {
 			mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-			rc = __rbd_refresh_header(rbd_dev);
+			rc = __rbd_refresh_header(rbd_dev, NULL);
 			mutex_unlock(&ctl_mutex);
 			if (rc < 0)
 				return rc;
@@ -2645,7 +2642,7 @@ static ssize_t rbd_snap_add(struct device *dev,
 	if (ret < 0)
 		goto err_unlock;

-	ret = __rbd_refresh_header(rbd_dev);
+	ret = __rbd_refresh_header(rbd_dev, NULL);
 	if (ret < 0)
 		goto err_unlock;

-- 
1.7.9.5

[PATCH 7/7] rbd: create rbd_refresh_header()

[Alex Elder]

Create a simple helper that handles the common case of calling
__rbd_refresh_header() while holding the ctl_mutex.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 31be4ca..94d0745 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -240,7 +240,7 @@ static void rbd_put_dev(struct rbd_device *rbd_dev)
 	put_device(&rbd_dev->dev);
 }

-static int __rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver);
+static int rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver);

 static int rbd_open(struct block_device *bdev, fmode_t mode)
 {
@@ -1222,9 +1222,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
u8 opcode, void *data)

 	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n",
 		rbd_dev->header_name, notify_id, (int) opcode);
-	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-	rc = __rbd_refresh_header(rbd_dev, &hver);
-	mutex_unlock(&ctl_mutex);
+	rc = rbd_refresh_header(rbd_dev, &hver);
 	if (rc)
 		pr_warning(RBD_DRV_NAME "%d got notification but failed to "
 			   " update snaps: %d\n", rbd_dev->major, rc);
@@ -1746,6 +1744,17 @@ static int __rbd_refresh_header(struct rbd_device
*rbd_dev, u64 *hver)
 	return ret;
 }

+static int rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver)
+{
+	int ret;
+
+	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
+	ret = __rbd_refresh_header(rbd_dev, hver);
+	mutex_unlock(&ctl_mutex);
+
+	return ret;
+}
+
 static int rbd_init_disk(struct rbd_device *rbd_dev)
 {
 	struct gendisk *disk;
@@ -1899,9 +1908,7 @@ static ssize_t rbd_image_refresh(struct device *dev,
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
 	int ret;

-	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-	ret = __rbd_refresh_header(rbd_dev, NULL);
-	mutex_unlock(&ctl_mutex);
+	ret = rbd_refresh_header(rbd_dev, NULL);

 	return ret < 0 ? ret : size;
 }
@@ -2191,9 +2198,7 @@ static int rbd_init_watch_dev(struct rbd_device
*rbd_dev)
 	do {
 		ret = rbd_req_sync_watch(rbd_dev);
 		if (ret == -ERANGE) {
-			mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-			rc = __rbd_refresh_header(rbd_dev, NULL);
-			mutex_unlock(&ctl_mutex);
+			rc = rbd_refresh_header(rbd_dev, NULL);
 			if (rc < 0)
 				return rc;
 		}
-- 
1.7.9.5

Re: [PATCH 1/7] rbd: have __rbd_add_snap_dev() return a pointer

[Josh Durgin]

On 07/26/2012 12:08 PM, Alex Elder wrote:
> It's not obvious whether the snapshot pointer whose address is
> provided to __rbd_add_snap_dev() will be assigned by that function.
> Change it to return the snapshot, or a pointer-coded errno in the
> event of a failure.
>
> Signed-off-by: Alex Elder <elder@inktank.com>

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

> ---
>   drivers/block/rbd.c |   37 ++++++++++++++++++++++---------------
>   1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index fd5f3038..fd91964 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2025,15 +2025,21 @@ static int rbd_register_snap_dev(struct rbd_snap
> *snap,
>   	return ret;
>   }
>
> -static int __rbd_add_snap_dev(struct rbd_device *rbd_dev,
> -			      int i, const char *name,
> -			      struct rbd_snap **snapp)
> +static struct rbd_snap *__rbd_add_snap_dev(struct rbd_device *rbd_dev,
> +					      int i, const char *name)
>   {
> +	struct rbd_snap *snap;
>   	int ret;
> -	struct rbd_snap *snap = kzalloc(sizeof(*snap), GFP_KERNEL);
> +
> +	snap = kzalloc(sizeof (*snap), GFP_KERNEL);
>   	if (!snap)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
> +
> +	ret = -ENOMEM;
>   	snap->name = kstrdup(name, GFP_KERNEL);
> +	if (!snap->name)
> +		goto err;
> +
>   	snap->size = rbd_dev->header.snap_sizes[i];
>   	snap->id = rbd_dev->header.snapc->snaps[i];
>   	if (device_is_registered(&rbd_dev->dev)) {
> @@ -2041,12 +2047,14 @@ static int __rbd_add_snap_dev(struct rbd_device
> *rbd_dev,
>   		if (ret < 0)
>   			goto err;
>   	}
> -	*snapp = snap;
> -	return 0;
> +
> +	return snap;
> +
>   err:
>   	kfree(snap->name);
>   	kfree(snap);
> -	return ret;
> +
> +	return ERR_PTR(ret);
>   }
>
>   /*
> @@ -2079,7 +2087,6 @@ static int __rbd_init_snaps_header(struct
> rbd_device *rbd_dev)
>   	const char *name, *first_name;
>   	int i = rbd_dev->header.total_snaps;
>   	struct rbd_snap *snap, *old_snap = NULL;
> -	int ret;
>   	struct list_head *p, *n;
>
>   	first_name = rbd_dev->header.snap_names;
> @@ -2122,9 +2129,9 @@ static int __rbd_init_snaps_header(struct
> rbd_device *rbd_dev)
>   			if (cur_id >= old_snap->id)
>   				break;
>   			/* a new snapshot */
> -			ret = __rbd_add_snap_dev(rbd_dev, i - 1, name, &snap);
> -			if (ret < 0)
> -				return ret;
> +			snap = __rbd_add_snap_dev(rbd_dev, i - 1, name);
> +			if (IS_ERR(snap))
> +				return PTR_ERR(snap);
>
>   			/* note that we add it backward so using n and not p */
>   			list_add(&snap->node, n);
> @@ -2138,9 +2145,9 @@ static int __rbd_init_snaps_header(struct
> rbd_device *rbd_dev)
>   			WARN_ON(1);
>   			return -EINVAL;
>   		}
> -		ret = __rbd_add_snap_dev(rbd_dev, i - 1, name, &snap);
> -		if (ret < 0)
> -			return ret;
> +		snap = __rbd_add_snap_dev(rbd_dev, i - 1, name);
> +		if (IS_ERR(snap))
> +			return PTR_ERR(snap);
>   		list_add(&snap->node, &rbd_dev->snaps);
>   	}
>

Re: [PATCH 2/7] rbd: make rbd_create_rw_ops() return a pointer

[Josh Durgin]

On 07/26/2012 12:08 PM, Alex Elder wrote:
> Either rbd_create_rw_ops() will succeed, or it will fail because a
> memory allocation failed.  Have it just return a valid pointer or
> null rather than stuffing a pointer into a provided address and
> returning an errno.
>
> Signed-off-by: Alex Elder <elder@inktank.com>

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

> ---
>   drivers/block/rbd.c |   70
> ++++++++++++++++++++++++++++-----------------------
>   1 file changed, 39 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index fd91964..4d8b52c 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -789,22 +789,24 @@ err_out:
>   /*
>    * helpers for osd request op vectors.
>    */
> -static int rbd_create_rw_ops(struct ceph_osd_req_op **ops,
> -			    int num_ops,
> -			    int opcode,
> -			    u32 payload_len)
> -{
> -	*ops = kzalloc(sizeof(struct ceph_osd_req_op) * (num_ops + 1),
> -		       GFP_NOIO);
> -	if (!*ops)
> -		return -ENOMEM;
> -	(*ops)[0].op = opcode;
> +static struct ceph_osd_req_op *rbd_create_rw_ops(int num_ops,
> +					int opcode, u32 payload_len)
> +{
> +	struct ceph_osd_req_op *ops;
> +
> +	ops = kzalloc(sizeof (*ops) * (num_ops + 1), GFP_NOIO);
> +	if (!ops)
> +		return NULL;
> +
> +	ops[0].op = opcode;
> +
>   	/*
>   	 * op extent offset and length will be set later on
>   	 * in calc_raw_layout()
>   	 */
> -	(*ops)[0].payload_len = payload_len;
> -	return 0;
> +	ops[0].payload_len = payload_len;
> +
> +	return ops;
>   }
>
>   static void rbd_destroy_ops(struct ceph_osd_req_op *ops)
> @@ -1039,8 +1041,9 @@ static int rbd_req_sync_op(struct rbd_device *rbd_dev,
>
>   	if (!orig_ops) {
>   		payload_len = (flags & CEPH_OSD_FLAG_WRITE ? len : 0);
> -		ret = rbd_create_rw_ops(&ops, 1, opcode, payload_len);
> -		if (ret < 0)
> +		ret = -ENOMEM;
> +		ops = rbd_create_rw_ops(1, opcode, payload_len);
> +		if (!ops)
>   			goto done;
>
>   		if ((flags & CEPH_OSD_FLAG_WRITE) && buf) {
> @@ -1103,8 +1106,9 @@ static int rbd_do_op(struct request *rq,
>
>   	payload_len = (flags & CEPH_OSD_FLAG_WRITE ? seg_len : 0);
>
> -	ret = rbd_create_rw_ops(&ops, 1, opcode, payload_len);
> -	if (ret < 0)
> +	ret = -ENOMEM;
> +	ops = rbd_create_rw_ops(1, opcode, payload_len);
> +	if (!ops)
>   		goto done;
>
>   	/* we've taken care of segment sizes earlier when we
> @@ -1190,9 +1194,9 @@ static int rbd_req_sync_notify_ack(struct
> rbd_device *rbd_dev,
>   	struct ceph_osd_req_op *ops;
>   	int ret;
>
> -	ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_NOTIFY_ACK, 0);
> -	if (ret < 0)
> -		return ret;
> +	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_NOTIFY_ACK, 0);
> +	if (!ops)
> +		return -ENOMEM;
>
>   	ops[0].watch.ver = cpu_to_le64(ver);
>   	ops[0].watch.cookie = notify_id;
> @@ -1239,10 +1243,11 @@ static int rbd_req_sync_watch(struct rbd_device
> *rbd_dev)
>   {
>   	struct ceph_osd_req_op *ops;
>   	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
> +	int ret;
>
> -	int ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_WATCH, 0);
> -	if (ret < 0)
> -		return ret;
> +	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_WATCH, 0);
> +	if (!ops)
> +		return -ENOMEM;
>
>   	ret = ceph_osdc_create_event(osdc, rbd_watch_cb, 0,
>   				     (void *)rbd_dev, &rbd_dev->watch_event);
> @@ -1282,10 +1287,11 @@ fail:
>   static int rbd_req_sync_unwatch(struct rbd_device *rbd_dev)
>   {
>   	struct ceph_osd_req_op *ops;
> +	int ret;
>
> -	int ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_WATCH, 0);
> -	if (ret < 0)
> -		return ret;
> +	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_WATCH, 0);
> +	if (!ops)
> +		return -ENOMEM;
>
>   	ops[0].watch.ver = 0;
>   	ops[0].watch.cookie = cpu_to_le64(rbd_dev->watch_event->cookie);
> @@ -1332,9 +1338,9 @@ static int rbd_req_sync_notify(struct rbd_device
> *rbd_dev)
>   	int payload_len = sizeof(u32) + sizeof(u32);
>   	int ret;
>
> -	ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_NOTIFY, payload_len);
> -	if (ret < 0)
> -		return ret;
> +	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_NOTIFY, payload_len);
> +	if (!ops)
> +		return -ENOMEM;
>
>   	info.rbd_dev = rbd_dev;
>
> @@ -1385,10 +1391,12 @@ static int rbd_req_sync_exec(struct rbd_device
> *rbd_dev,
>   	struct ceph_osd_req_op *ops;
>   	int class_name_len = strlen(class_name);
>   	int method_name_len = strlen(method_name);
> -	int ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_CALL,
> +	int ret;
> +
> +	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_CALL,
>   				    class_name_len + method_name_len + len);
> -	if (ret < 0)
> -		return ret;
> +	if (!ops)
> +		return -ENOMEM;
>
>   	ops[0].cls.class_name = class_name;
>   	ops[0].cls.class_len = (__u8) class_name_len;
>

Re: [PATCH 3/7] rbd: pass null version pointer in add_snap()

[Josh Durgin]

On 07/26/2012 12:08 PM, Alex Elder wrote:
> rbd_header_add_snap() passes the address of a version variable to
> rbd_req_sync_exec(), but it ignores the result.  Just pass a null
> pointer instead.
>
> Signed-off-by: Alex Elder <elder@inktank.com>

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

> ---
>   drivers/block/rbd.c |    3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 4d8b52c..eacf255 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -1664,7 +1664,6 @@ static int rbd_header_add_snap(struct rbd_device
> *rbd_dev,
>   	u64 new_snapid;
>   	int ret;
>   	void *data, *p, *e;
> -	u64 ver;
>   	struct ceph_mon_client *monc;
>
>   	/* we should create a snapshot only if we're pointing at the head */
> @@ -1689,7 +1688,7 @@ static int rbd_header_add_snap(struct rbd_device
> *rbd_dev,
>
>   	ret = rbd_req_sync_exec(rbd_dev, rbd_dev->header_name,
>   				"rbd", "snap_add",
> -				data, p - data, &ver);
> +				data, p - data, NULL);
>
>   	kfree(data);
>

Re: [PATCH 4/7] rbd: always pass ops array to rbd_req_sync_op()

[Josh Durgin]

On 07/26/2012 12:08 PM, Alex Elder wrote:
> All of the callers of rbd_req_sync_op() except one pass a non-null
> "ops" pointer.  The only one that does not is rbd_req_sync_read(),
> which passes CEPH_OSD_OP_READ as its "opcode" and, CEPH_OSD_FLAG_READ
> for "flags".
>
> By allocating the ops array in rbd_req_sync_read() and moving the
> special case code for the null ops pointer into it, it becomes
> clear that much of that code is not even necessary.
>
> In addition, the "opcode" argument to rbd_req_sync_op() is never
> actually used, so get rid of that.
>
> Signed-off-by: Alex Elder <elder@inktank.com>

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

> ---
>   drivers/block/rbd.c |   46 ++++++++++++++++------------------------------
>   1 file changed, 16 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index eacf255..4584500 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -1019,9 +1019,8 @@ static void rbd_simple_req_cb(struct
> ceph_osd_request *req, struct ceph_msg *msg
>   static int rbd_req_sync_op(struct rbd_device *rbd_dev,
>   			   struct ceph_snap_context *snapc,
>   			   u64 snapid,
> -			   int opcode,
>   			   int flags,
> -			   struct ceph_osd_req_op *orig_ops,
> +			   struct ceph_osd_req_op *ops,
>   			   const char *object_name,
>   			   u64 ofs, u64 len,
>   			   char *buf,
> @@ -1031,28 +1030,14 @@ static int rbd_req_sync_op(struct rbd_device
> *rbd_dev,
>   	int ret;
>   	struct page **pages;
>   	int num_pages;
> -	struct ceph_osd_req_op *ops = orig_ops;
> -	u32 payload_len;
> +
> +	BUG_ON(ops == NULL);
>
>   	num_pages = calc_pages_for(ofs , len);
>   	pages = ceph_alloc_page_vector(num_pages, GFP_KERNEL);
>   	if (IS_ERR(pages))
>   		return PTR_ERR(pages);
>
> -	if (!orig_ops) {
> -		payload_len = (flags & CEPH_OSD_FLAG_WRITE ? len : 0);
> -		ret = -ENOMEM;
> -		ops = rbd_create_rw_ops(1, opcode, payload_len);
> -		if (!ops)
> -			goto done;
> -
> -		if ((flags & CEPH_OSD_FLAG_WRITE) && buf) {
> -			ret = ceph_copy_to_page_vector(pages, buf, ofs, len);
> -			if (ret < 0)
> -				goto done_ops;
> -		}
> -	}
> -
>   	ret = rbd_do_request(NULL, rbd_dev, snapc, snapid,
>   			  object_name, ofs, len, NULL,
>   			  pages, num_pages,
> @@ -1062,14 +1047,11 @@ static int rbd_req_sync_op(struct rbd_device
> *rbd_dev,
>   			  NULL,
>   			  linger_req, ver);
>   	if (ret < 0)
> -		goto done_ops;
> +		goto done;
>
>   	if ((flags & CEPH_OSD_FLAG_READ) && buf)
>   		ret = ceph_copy_from_page_vector(pages, buf, ofs, ret);
>
> -done_ops:
> -	if (!orig_ops)
> -		rbd_destroy_ops(ops);
>   done:
>   	ceph_release_page_vector(pages, num_pages);
>   	return ret;
> @@ -1176,12 +1158,20 @@ static int rbd_req_sync_read(struct rbd_device
> *rbd_dev,
>   			  char *buf,
>   			  u64 *ver)
>   {
> -	return rbd_req_sync_op(rbd_dev, NULL,
> +	struct ceph_osd_req_op *ops;
> +	int ret;
> +
> +	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_READ, 0);
> +	if (!ops)
> +		return -ENOMEM;
> +
> +	ret = rbd_req_sync_op(rbd_dev, NULL,
>   			       snapid,
> -			       CEPH_OSD_OP_READ,
>   			       CEPH_OSD_FLAG_READ,
> -			       NULL,
> -			       object_name, ofs, len, buf, NULL, ver);
> +			       ops, object_name, ofs, len, buf, NULL, ver);
> +	rbd_destroy_ops(ops);
> +
> +	return ret;
>   }
>
>   /*
> @@ -1260,7 +1250,6 @@ static int rbd_req_sync_watch(struct rbd_device
> *rbd_dev)
>
>   	ret = rbd_req_sync_op(rbd_dev, NULL,
>   			      CEPH_NOSNAP,
> -			      0,
>   			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
>   			      ops,
>   			      rbd_dev->header_name,
> @@ -1299,7 +1288,6 @@ static int rbd_req_sync_unwatch(struct rbd_device
> *rbd_dev)
>
>   	ret = rbd_req_sync_op(rbd_dev, NULL,
>   			      CEPH_NOSNAP,
> -			      0,
>   			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
>   			      ops,
>   			      rbd_dev->header_name,
> @@ -1357,7 +1345,6 @@ static int rbd_req_sync_notify(struct rbd_device
> *rbd_dev)
>
>   	ret = rbd_req_sync_op(rbd_dev, NULL,
>   			       CEPH_NOSNAP,
> -			       0,
>   			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
>   			       ops,
>   			       rbd_dev->header_name,
> @@ -1408,7 +1395,6 @@ static int rbd_req_sync_exec(struct rbd_device
> *rbd_dev,
>
>   	ret = rbd_req_sync_op(rbd_dev, NULL,
>   			       CEPH_NOSNAP,
> -			       0,
>   			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
>   			       ops,
>   			       object_name, 0, 0, NULL, NULL, ver);
>

Re: [PATCH 5/7] rbd: fixes in rbd_header_from_disk()

[Josh Durgin]

On 07/26/2012 12:08 PM, Alex Elder wrote:
> This fixes a few issues in rbd_header_from_disk():
>      - There is a check intended to catch overflow, but it's wrong in
>        two ways.
> 	- First, the type we don't want to overflow is size_t, not
> 	  unsigned int, and there is now a SIZE_MAX we can use for
> 	  use with that type.
> 	- Second, we're allocating the snapshot ids and snapshot
> 	  image sizes separately (each has type u64; on disk they
>            grouped together as a rbd_image_header_ondisk structure).
> 	  So we can use the size of u64 in this overflow check.
>      - If there are no snapshots, then there should be no snapshot
>        names.  Enforce this, and issue a warning if we encounter a
>        header with no snapshots but a non-zero snap_names_len.
>      - When saving the snapshot names into the header, be more direct
>        in defining the offset in the on-disk structure from which
>        they're being copied by using "snap_count" rather than "i"
>        in the array index.
>      - If an error occurs, the "snapc" and "snap_names" fields are
>        freed at the end of the function.  Make those fields be null
>        pointers after they're freed, to be explicit that they are
>        no longer valid.

Why not do this for snap_sizes too?

>      - Finally, move the definition of the local variable "i" to the
>        innermost scope in which it's needed.
>
> Signed-off-by: Alex Elder <elder@inktank.com>

Looks good.

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

> ---
>   drivers/block/rbd.c |   17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 4584500..3daf8fb 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -494,14 +494,14 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>   				 struct rbd_image_header_ondisk *ondisk,
>   				 u32 allocated_snaps)
>   {
> -	u32 i, snap_count;
> +	u32 snap_count;
>
>   	if (!rbd_dev_ondisk_valid(ondisk))
>   		return -ENXIO;
>
>   	snap_count = le32_to_cpu(ondisk->snap_count);
> -	if (snap_count > (UINT_MAX - sizeof(struct ceph_snap_context))
> -			 / sizeof (*ondisk))
> +	if (snap_count > (SIZE_MAX - sizeof(struct ceph_snap_context))
> +				 / sizeof (u64))
>   		return -EINVAL;
>   	header->snapc = kmalloc(sizeof(struct ceph_snap_context) +
>   				snap_count * sizeof(u64),
> @@ -509,8 +509,8 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>   	if (!header->snapc)
>   		return -ENOMEM;
>
> -	header->snap_names_len = le64_to_cpu(ondisk->snap_names_len);
>   	if (snap_count) {
> +		header->snap_names_len = le64_to_cpu(ondisk->snap_names_len);
>   		header->snap_names = kmalloc(header->snap_names_len,
>   					     GFP_KERNEL);
>   		if (!header->snap_names)
> @@ -520,6 +520,8 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>   		if (!header->snap_sizes)
>   			goto err_names;
>   	} else {
> +		WARN_ON(ondisk->snap_names_len);
> +		header->snap_names_len = 0;
>   		header->snap_names = NULL;
>   		header->snap_sizes = NULL;
>   	}
> @@ -544,6 +546,8 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>   	header->total_snaps = snap_count;
>
>   	if (snap_count && allocated_snaps == snap_count) {
> +		int i;
> +
>   		for (i = 0; i < snap_count; i++) {
>   			header->snapc->snaps[i] =
>   				le64_to_cpu(ondisk->snaps[i].id);
> @@ -552,7 +556,7 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>   		}
>
>   		/* copy snapshot names */
> -		memcpy(header->snap_names, &ondisk->snaps[i],
> +		memcpy(header->snap_names, &ondisk->snaps[snap_count],
>   			header->snap_names_len);
>   	}
>
> @@ -562,8 +566,11 @@ err_sizes:
>   	kfree(header->snap_sizes);
>   err_names:
>   	kfree(header->snap_names);
> +	header->snap_names = NULL;
>   err_snapc:
>   	kfree(header->snapc);
> +	header->snapc = NULL;
> +
>   	return -ENOMEM;
>   }
>

Re: [PATCH 6/7] rbd: return obj version in __rbd_refresh_header()

[Josh Durgin]

On 07/26/2012 12:09 PM, Alex Elder wrote:
> Add a new parameter to __rbd_refresh_header() through which the
> version of the header object is passed back to the caller.  In most
> cases this isn't needed.  The main motivation is to normalize
> (almost) all calls to __rbd_refresh_header() so they are all
> wrapped immediately by mutex_lock()/mutex_unlock().
>
> Signed-off-by: Alex Elder <elder@inktank.com>

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

> ---
>   drivers/block/rbd.c |   25 +++++++++++--------------
>   1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 3daf8fb..31be4ca 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -240,7 +240,7 @@ static void rbd_put_dev(struct rbd_device *rbd_dev)
>   	put_device(&rbd_dev->dev);
>   }
>
> -static int __rbd_refresh_header(struct rbd_device *rbd_dev);
> +static int __rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver);
>
>   static int rbd_open(struct block_device *bdev, fmode_t mode)
>   {
> @@ -1223,8 +1223,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
> u8 opcode, void *data)
>   	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n",
>   		rbd_dev->header_name, notify_id, (int) opcode);
>   	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> -	rc = __rbd_refresh_header(rbd_dev);
> -	hver = rbd_dev->header.obj_version;
> +	rc = __rbd_refresh_header(rbd_dev, &hver);
>   	mutex_unlock(&ctl_mutex);
>   	if (rc)
>   		pr_warning(RBD_DRV_NAME "%d got notification but failed to "
> @@ -1702,7 +1701,7 @@ static void __rbd_remove_all_snaps(struct
> rbd_device *rbd_dev)
>   /*
>    * only read the first part of the ondisk header, without the snaps info
>    */
> -static int __rbd_refresh_header(struct rbd_device *rbd_dev)
> +static int __rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver)
>   {
>   	int ret;
>   	struct rbd_image_header h;
> @@ -1727,6 +1726,8 @@ static int __rbd_refresh_header(struct rbd_device
> *rbd_dev)
>   	/* 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.total_snaps = h.total_snaps;
> @@ -1896,17 +1897,13 @@ static ssize_t rbd_image_refresh(struct device *dev,
>   				 size_t size)
>   {
>   	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
> -	int rc;
> -	int ret = size;
> +	int ret;
>
>   	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> -
> -	rc = __rbd_refresh_header(rbd_dev);
> -	if (rc < 0)
> -		ret = rc;
> -
> +	ret = __rbd_refresh_header(rbd_dev, NULL);
>   	mutex_unlock(&ctl_mutex);
> -	return ret;
> +
> +	return ret < 0 ? ret : size;
>   }
>
>   static DEVICE_ATTR(size, S_IRUGO, rbd_size_show, NULL);
> @@ -2195,7 +2192,7 @@ static int rbd_init_watch_dev(struct rbd_device
> *rbd_dev)
>   		ret = rbd_req_sync_watch(rbd_dev);
>   		if (ret == -ERANGE) {
>   			mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> -			rc = __rbd_refresh_header(rbd_dev);
> +			rc = __rbd_refresh_header(rbd_dev, NULL);
>   			mutex_unlock(&ctl_mutex);
>   			if (rc < 0)
>   				return rc;
> @@ -2645,7 +2642,7 @@ static ssize_t rbd_snap_add(struct device *dev,
>   	if (ret < 0)
>   		goto err_unlock;
>
> -	ret = __rbd_refresh_header(rbd_dev);
> +	ret = __rbd_refresh_header(rbd_dev, NULL);
>   	if (ret < 0)
>   		goto err_unlock;
>

Re: [PATCH 7/7] rbd: create rbd_refresh_header()

[Josh Durgin]

On 07/26/2012 12:09 PM, Alex Elder wrote:
> Create a simple helper that handles the common case of calling
> __rbd_refresh_header() while holding the ctl_mutex.
>
> Signed-off-by: Alex Elder <elder@inktank.com>

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

> ---
>   drivers/block/rbd.c |   25 +++++++++++++++----------
>   1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 31be4ca..94d0745 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -240,7 +240,7 @@ static void rbd_put_dev(struct rbd_device *rbd_dev)
>   	put_device(&rbd_dev->dev);
>   }
>
> -static int __rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver);
> +static int rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver);
>
>   static int rbd_open(struct block_device *bdev, fmode_t mode)
>   {
> @@ -1222,9 +1222,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
> u8 opcode, void *data)
>
>   	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n",
>   		rbd_dev->header_name, notify_id, (int) opcode);
> -	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> -	rc = __rbd_refresh_header(rbd_dev, &hver);
> -	mutex_unlock(&ctl_mutex);
> +	rc = rbd_refresh_header(rbd_dev, &hver);
>   	if (rc)
>   		pr_warning(RBD_DRV_NAME "%d got notification but failed to "
>   			   " update snaps: %d\n", rbd_dev->major, rc);
> @@ -1746,6 +1744,17 @@ static int __rbd_refresh_header(struct rbd_device
> *rbd_dev, u64 *hver)
>   	return ret;
>   }
>
> +static int rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver)
> +{
> +	int ret;
> +
> +	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> +	ret = __rbd_refresh_header(rbd_dev, hver);
> +	mutex_unlock(&ctl_mutex);
> +
> +	return ret;
> +}
> +
>   static int rbd_init_disk(struct rbd_device *rbd_dev)
>   {
>   	struct gendisk *disk;
> @@ -1899,9 +1908,7 @@ static ssize_t rbd_image_refresh(struct device *dev,
>   	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>   	int ret;
>
> -	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> -	ret = __rbd_refresh_header(rbd_dev, NULL);
> -	mutex_unlock(&ctl_mutex);
> +	ret = rbd_refresh_header(rbd_dev, NULL);
>
>   	return ret < 0 ? ret : size;
>   }
> @@ -2191,9 +2198,7 @@ static int rbd_init_watch_dev(struct rbd_device
> *rbd_dev)
>   	do {
>   		ret = rbd_req_sync_watch(rbd_dev);
>   		if (ret == -ERANGE) {
> -			mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> -			rc = __rbd_refresh_header(rbd_dev, NULL);
> -			mutex_unlock(&ctl_mutex);
> +			rc = rbd_refresh_header(rbd_dev, NULL);
>   			if (rc < 0)
>   				return rc;
>   		}
>

Re: [PATCH 5/7] rbd: fixes in rbd_header_from_disk()

[Alex Elder]

On 07/30/2012 04:23 PM, Josh Durgin wrote:
> Why not do this for snap_sizes too?

Because I seemed to have missed that one for some reason.

I'll add it before I commit.  Thanks for catching it.

					-Alex