[PATCH 00/12] rbd: cleanup series

[PATCH 00/12] rbd: cleanup series

[Alex Elder]

This series includes a bunch of relatively small cleanups.
They're grouped a bit below, but they apply together in
this sequence and the later ones may have dependencies on
those earlier in the series.

Summaries:
[PATCH 01/12] rbd: drop extra header_rwsem init
[PATCH 02/12] rbd: simplify __rbd_remove_all_snaps()
[PATCH 03/12] rbd: clean up a few dout() calls
[PATCH 04/12] ceph: define snap counts as u32 everywhere
    These four are very simple and straightforward cleanups.

[PATCH 05/12] rbd: snapc is unused in rbd_req_sync_read()
[PATCH 06/12] rbd: drop rbd_header_from_disk() gfp_flags parameter
[PATCH 07/12] rbd: drop rbd_dev parameter in snap functions
[PATCH 08/12] rbd: drop rbd_req_sync_exec() "ver" parameter
    These four each drop an unused argument from a function.

[PATCH 09/12] rbd: have __rbd_add_snap_dev() return a pointer
[PATCH 10/12] rbd: make rbd_create_rw_ops() return a pointer
    These two each change a function so it returns a pointer
    rather than filling in the address of a provided pointer.

[PATCH 11/12] rbd: always pass ops array to rbd_req_sync_op()
[PATCH 12/12] rbd: fixes in rbd_header_from_disk()
    These comprise slightly more involved refactoring of the
    code, but as with the rest of the patches in this series,
    there should be no functional difference as a result.

					-Alex

[PATCH 01/12] rbd: drop extra header_rwsem init

[Alex Elder]

In commit c666601a there was inadvertently added an extra
initialization of rbd_dev->header_rwsem.  This gets rid of the
duplicate.

(Guangliang Zhao also offered up the same fix.)

Reported-by: Guangliang Zhao <gzhao@suse.com>
Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 6df8c62..b9895fe 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2458,8 +2458,6 @@ static ssize_t rbd_add(struct bus_type *bus,
 	INIT_LIST_HEAD(&rbd_dev->snaps);
 	init_rwsem(&rbd_dev->header_rwsem);

-	init_rwsem(&rbd_dev->header_rwsem);
-
 	/* generate unique id: find highest unique id, add one */
 	rbd_id_get(rbd_dev);

-- 
1.7.5.4

[PATCH 02/12] rbd: simplify __rbd_remove_all_snaps()

[Alex Elder]

This just replaces a while loop with list_for_each_entry_safe()
in __rbd_remove_all_snaps().

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index b9895fe..74e6a33 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1692,11 +1692,10 @@ bad:
 static void __rbd_remove_all_snaps(struct rbd_device *rbd_dev)
 {
 	struct rbd_snap *snap;
+	struct rbd_snap *next;

-	while (!list_empty(&rbd_dev->snaps)) {
-		snap = list_first_entry(&rbd_dev->snaps, struct rbd_snap, node);
+	list_for_each_entry_safe(snap, next, &rbd_dev->snaps, node)
 		__rbd_remove_snap_dev(rbd_dev, snap);
-	}
 }

 /*
-- 
1.7.5.4

[PATCH 03/12] rbd: clean up a few dout() calls

[Alex Elder]

There was a dout() call in rbd_do_request() that was reporting
the reporting the offset as the length and vice versa.  While
fixing that I did a quick scan of other dout() calls and fixed
a couple of other minor things.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 74e6a33..93b2447 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -895,7 +895,7 @@ static int rbd_do_request(struct request *rq,
 	}

 	dout("rbd_do_request object_name=%s ofs=%lld len=%lld\n",
-		object_name, len, ofs);
+		object_name, ofs, len);

 	osdc = &rbd_dev->rbd_client->client->osdc;
 	req = ceph_osdc_alloc_request(osdc, flags, snapc, ops,
@@ -1315,8 +1315,7 @@ static void rbd_notify_cb(u64 ver, u64 notify_id,
u8 opcode, void *data)
 		return;

 	dout("rbd_notify_cb %s notify_id=%lld opcode=%d\n",
-				rbd_dev->header_name,
-		notify_id, (int)opcode);
+			rbd_dev->header_name, notify_id, (int) opcode);
 }

 /*
@@ -1664,7 +1663,7 @@ static int rbd_header_add_snap(struct rbd_device
*rbd_dev,

 	monc = &rbd_dev->rbd_client->client->monc;
 	ret = ceph_monc_create_snapid(monc, rbd_dev->pool_id, &new_snapid);
-	dout("created snapid=%lld\n", new_snapid);
+	dout("created snapid=%llu\n", new_snapid);
 	if (ret < 0)
 		return ret;

-- 
1.7.5.4

[PATCH 04/12] ceph: define snap counts as u32 everywhere

[Alex Elder]

There are two structures in which a count of snapshots are
maintained:

    struct ceph_snap_context {
	...
        u32 num_snaps;
	...
    }
and
    struct ceph_snap_realm {
	...
        u32 num_prior_parent_snaps;   /*  had prior to parent_since */
	...
        u32 num_snaps;
	...
    }

These fields never take on negative values (e.g., to hold special
meaning), and so are really inherently unsigned.  Furthermore they
take their value from over-the-wire or on-disk formatted 32-bit
values.

So change their definition to have type u32, and change some spots
elsewhere in the code to account for this change.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 fs/ceph/snap.c               |   18 ++++++++++--------
 fs/ceph/super.h              |    4 ++--
 include/linux/ceph/libceph.h |    2 +-
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index e5206fc..cbb2f54 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -296,8 +296,7 @@ static int build_snap_context(struct ceph_snap_realm
*realm)
 	struct ceph_snap_realm *parent = realm->parent;
 	struct ceph_snap_context *snapc;
 	int err = 0;
-	int i;
-	int num = realm->num_prior_parent_snaps + realm->num_snaps;
+	u32 num = realm->num_prior_parent_snaps + realm->num_snaps;

 	/*
 	 * build parent context, if it hasn't been built.
@@ -321,11 +320,11 @@ static int build_snap_context(struct
ceph_snap_realm *realm)
 	    realm->cached_context->seq == realm->seq &&
 	    (!parent ||
 	     realm->cached_context->seq >= parent->cached_context->seq)) {
-		dout("build_snap_context %llx %p: %p seq %lld (%d snaps)"
+		dout("build_snap_context %llx %p: %p seq %lld (%u snaps)"
 		     " (unchanged)\n",
 		     realm->ino, realm, realm->cached_context,
 		     realm->cached_context->seq,
-		     realm->cached_context->num_snaps);
+		     (unsigned int) realm->cached_context->num_snaps);
 		return 0;
 	}

@@ -342,6 +341,8 @@ static int build_snap_context(struct ceph_snap_realm
*realm)
 	num = 0;
 	snapc->seq = realm->seq;
 	if (parent) {
+		u32 i;
+
 		/* include any of parent's snaps occurring _after_ my
 		   parent became my parent */
 		for (i = 0; i < parent->cached_context->num_snaps; i++)
@@ -361,8 +362,9 @@ static int build_snap_context(struct ceph_snap_realm
*realm)

 	sort(snapc->snaps, num, sizeof(u64), cmpu64_rev, NULL);
 	snapc->num_snaps = num;
-	dout("build_snap_context %llx %p: %p seq %lld (%d snaps)\n",
-	     realm->ino, realm, snapc, snapc->seq, snapc->num_snaps);
+	dout("build_snap_context %llx %p: %p seq %lld (%u snaps)\n",
+	     realm->ino, realm, snapc, snapc->seq,
+	     (unsigned int) snapc->num_snaps);

 	if (realm->cached_context)
 		ceph_put_snap_context(realm->cached_context);
@@ -402,9 +404,9 @@ static void rebuild_snap_realms(struct
ceph_snap_realm *realm)
  * helper to allocate and decode an array of snapids.  free prior
  * instance, if any.
  */
-static int dup_array(u64 **dst, __le64 *src, int num)
+static int dup_array(u64 **dst, __le64 *src, u32 num)
 {
-	int i;
+	u32 i;

 	kfree(*dst);
 	if (num) {
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index fc35036..3ea48b7 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -612,9 +612,9 @@ struct ceph_snap_realm {
 	u64 parent_since;   /* snapid when our current parent became so */

 	u64 *prior_parent_snaps;      /* snaps inherited from any parents we */
-	int num_prior_parent_snaps;   /*  had prior to parent_since */
+	u32 num_prior_parent_snaps;   /*  had prior to parent_since */
 	u64 *snaps;                   /* snaps specific to this realm */
-	int num_snaps;
+	u32 num_snaps;

 	struct ceph_snap_realm *parent;
 	struct list_head children;       /* list of child realms */
diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
index 98ec36a..0b72295 100644
--- a/include/linux/ceph/libceph.h
+++ b/include/linux/ceph/libceph.h
@@ -160,7 +160,7 @@ struct ceph_client {
 struct ceph_snap_context {
 	atomic_t nref;
 	u64 seq;
-	int num_snaps;
+	u32 num_snaps;
 	u64 snaps[];
 };

-- 
1.7.5.4

[PATCH 05/12] rbd: snapc is unused in rbd_req_sync_read()

[Alex Elder]

The "snapc" parameter to in rbd_req_sync_read() is not used, so
get rid of it.

Reported-by: Josh Durgin <josh.durgin@inktank.com>
Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 93b2447..06e022d 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1162,7 +1162,6 @@ static int rbd_req_read(struct request *rq,
  * Request sync osd read
  */
 static int rbd_req_sync_read(struct rbd_device *rbd_dev,
-			  struct ceph_snap_context *snapc,
 			  u64 snapid,
 			  const char *object_name,
 			  u64 ofs, u64 len,
@@ -1609,7 +1608,7 @@ static int rbd_read_header(struct rbd_device *rbd_dev,
 			return -ENOMEM;

 		rc = rbd_req_sync_read(rbd_dev,
-				       NULL, CEPH_NOSNAP,
+				       CEPH_NOSNAP,
 				       rbd_dev->header_name,
 				       0, len,
 				       (char *)dh, &ver);
-- 
1.7.5.4

[PATCH 06/12] rbd: drop rbd_header_from_disk() gfp_flags parameter

[Alex Elder]

The function rbd_header_from_disk() is only called in one spot, and
it passes GFP_KERNEL as its value for the gfp_flags parameter.

Just drop that parameter and substitute GFP_KERNEL everywhere within
that function it had been used.  (If we find we need the parameter
again in the future it's easy enough to add back again.)

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 06e022d..3b4b4d2 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -487,8 +487,7 @@ static void rbd_coll_release(struct kref *kref)
  */
 static int rbd_header_from_disk(struct rbd_image_header *header,
 				 struct rbd_image_header_ondisk *ondisk,
-				 u32 allocated_snaps,
-				 gfp_t gfp_flags)
+				 u32 allocated_snaps)
 {
 	u32 i, snap_count;

@@ -501,18 +500,18 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 		return -EINVAL;
 	header->snapc = kmalloc(sizeof(struct ceph_snap_context) +
 				snap_count * sizeof(u64),
-				gfp_flags);
+				GFP_KERNEL);
 	if (!header->snapc)
 		return -ENOMEM;

 	header->snap_names_len = le64_to_cpu(ondisk->snap_names_len);
 	if (snap_count) {
 		header->snap_names = kmalloc(header->snap_names_len,
-					     gfp_flags);
+					     GFP_KERNEL);
 		if (!header->snap_names)
 			goto err_snapc;
 		header->snap_sizes = kmalloc(snap_count * sizeof(u64),
-					     gfp_flags);
+					     GFP_KERNEL);
 		if (!header->snap_sizes)
 			goto err_names;
 	} else {
@@ -521,7 +520,7 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 	}

 	header->object_prefix = kmalloc(sizeof (ondisk->block_name) + 1,
-					gfp_flags);
+					GFP_KERNEL);
 	if (!header->object_prefix)
 		goto err_sizes;

@@ -1615,7 +1614,7 @@ static int rbd_read_header(struct rbd_device *rbd_dev,
 		if (rc < 0)
 			goto out_dh;

-		rc = rbd_header_from_disk(header, dh, snap_count, GFP_KERNEL);
+		rc = rbd_header_from_disk(header, dh, snap_count);
 		if (rc < 0) {
 			if (rc == -ENXIO)
 				pr_warning("unrecognized header format"
-- 
1.7.5.4

[PATCH 07/12] rbd: drop rbd_dev parameter in snap functions

[Alex Elder]

Both rbd_register_snap_dev() and __rbd_remove_snap_dev() have
rbd_dev parameters that are unused.  Remove them.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 3b4b4d2..955d75d 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -202,8 +202,7 @@ static ssize_t rbd_snap_add(struct device *dev,
 			    struct device_attribute *attr,
 			    const char *buf,
 			    size_t count);
-static void __rbd_remove_snap_dev(struct rbd_device *rbd_dev,
-				  struct rbd_snap *snap);
+static void __rbd_remove_snap_dev(struct rbd_snap *snap);

 static ssize_t rbd_add(struct bus_type *bus, const char *buf,
 		       size_t count);
@@ -1692,7 +1691,7 @@ static void __rbd_remove_all_snaps(struct
rbd_device *rbd_dev)
 	struct rbd_snap *next;

 	list_for_each_entry_safe(snap, next, &rbd_dev->snaps, node)
-		__rbd_remove_snap_dev(rbd_dev, snap);
+		__rbd_remove_snap_dev(snap);
 }

 /*
@@ -2000,15 +1999,13 @@ static struct device_type rbd_snap_device_type = {
 	.release	= rbd_snap_dev_release,
 };

-static void __rbd_remove_snap_dev(struct rbd_device *rbd_dev,
-				  struct rbd_snap *snap)
+static void __rbd_remove_snap_dev(struct rbd_snap *snap)
 {
 	list_del(&snap->node);
 	device_unregister(&snap->dev);
 }

-static int rbd_register_snap_dev(struct rbd_device *rbd_dev,
-				  struct rbd_snap *snap,
+static int rbd_register_snap_dev(struct rbd_snap *snap,
 				  struct device *parent)
 {
 	struct device *dev = &snap->dev;
@@ -2035,8 +2032,7 @@ static int __rbd_add_snap_dev(struct rbd_device
*rbd_dev,
 	snap->size = rbd_dev->header.snap_sizes[i];
 	snap->id = rbd_dev->header.snapc->snaps[i];
 	if (device_is_registered(&rbd_dev->dev)) {
-		ret = rbd_register_snap_dev(rbd_dev, snap,
-					     &rbd_dev->dev);
+		ret = rbd_register_snap_dev(snap, &rbd_dev->dev);
 		if (ret < 0)
 			goto err;
 	}
@@ -2101,7 +2097,7 @@ static int __rbd_init_snaps_header(struct
rbd_device *rbd_dev)
 			 */
 			if (rbd_dev->snap_id == old_snap->id)
 				rbd_dev->snap_exists = false;
-			__rbd_remove_snap_dev(rbd_dev, old_snap);
+			__rbd_remove_snap_dev(old_snap);
 			continue;
 		}
 		if (old_snap->id == cur_id) {
@@ -2165,8 +2161,7 @@ static int rbd_bus_add_dev(struct rbd_device *rbd_dev)
 		goto out;

 	list_for_each_entry(snap, &rbd_dev->snaps, node) {
-		ret = rbd_register_snap_dev(rbd_dev, snap,
-					     &rbd_dev->dev);
+		ret = rbd_register_snap_dev(snap, &rbd_dev->dev);
 		if (ret < 0)
 			break;
 	}
-- 
1.7.5.4

[PATCH 08/12] rbd: drop rbd_req_sync_exec() "ver" parameter

[Alex Elder]

The only place that passes a version pointer to rbd_req_sync_exec()
is in rbd_header_add_snap(), and that spot ignores the result.

The only thing rbd_req_sync_exec() does with its "ver" parameter is
pass it directly to rbd_req_sync_op().  So we can just use a null
pointer there, and drop the "ver" parameter to rbd_req_sync_exec().

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 955d75d..d4a8d9e 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1374,8 +1374,7 @@ static int rbd_req_sync_exec(struct rbd_device
*rbd_dev,
 			     const char *class_name,
 			     const char *method_name,
 			     const char *data,
-			     int len,
-			     u64 *ver)
+			     int len)
 {
 	struct ceph_osd_req_op *ops;
 	int class_name_len = strlen(class_name);
@@ -1398,7 +1397,7 @@ static int rbd_req_sync_exec(struct rbd_device
*rbd_dev,
 			       0,
 			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
 			       ops,
-			       object_name, 0, 0, NULL, NULL, ver);
+			       object_name, 0, 0, NULL, NULL, NULL);

 	rbd_destroy_ops(ops);

@@ -1651,7 +1650,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 */
@@ -1676,7 +1674,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);

 	kfree(data);

-- 
1.7.5.4

[PATCH 09/12] 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 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index d4a8d9e..79b0762 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2018,15 +2018,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)) {
@@ -2034,12 +2040,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);
 }

 /*
@@ -2072,7 +2080,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;
@@ -2115,9 +2122,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);
@@ -2131,9 +2138,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.5.4

[PATCH 10/12] 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 |   68
++++++++++++++++++++++++++++----------------------
 1 files changed, 38 insertions(+), 30 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 79b0762..0535612 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -783,22 +783,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)
@@ -1033,8 +1035,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) {
@@ -1097,8 +1100,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
@@ -1185,9 +1189,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;
@@ -1236,10 +1240,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);
@@ -1279,10 +1284,11 @@ static int rbd_req_sync_unwatch(struct
rbd_device *rbd_dev,
 				const char *object_name)
 {
 	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);
@@ -1328,9 +1334,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;

@@ -1379,10 +1385,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.5.4

[PATCH 11/12] 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 files changed, 16 insertions(+), 30 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 0535612..6279186 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1013,9 +1013,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,
@@ -1025,28 +1024,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,
@@ -1056,14 +1041,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;
@@ -1170,12 +1152,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;
 }

 /*
@@ -1257,7 +1247,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,
 			      object_name, 0, 0, NULL,
@@ -1296,7 +1285,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,
 			      object_name, 0, 0, NULL, NULL, NULL);
@@ -1353,7 +1341,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,
 			       object_name, 0, 0, NULL, NULL, NULL);
@@ -1402,7 +1389,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, NULL);
-- 
1.7.5.4

[PATCH 12/12] rbd: fixes in rbd_header_from_disk()

[Alex Elder]

This fixes a few issues in rbd_header_from_disk():
    - The memcmp() call at the beginning of the function is really
      looking at the "text" field of struct rbd_image_header_ondisk.
      While it does lie at the beginning of the structure, the
      comparison should be done against the field, not the structure
      as a whole.
    - 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 |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 6279186..573202a 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -488,14 +488,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 (memcmp(ondisk, RBD_HEADER_TEXT, sizeof(RBD_HEADER_TEXT)))
+	if (memcmp(&ondisk->text, RBD_HEADER_TEXT, sizeof(RBD_HEADER_TEXT)))
 		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),
@@ -503,8 +503,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)
@@ -514,6 +514,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;
 	}
@@ -538,6 +540,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);
@@ -546,7 +550,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);
 	}

@@ -556,8 +560,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.5.4

Re: [PATCH 00/12] rbd: cleanup series

[Alex Elder]

On 07/19/2012 03:58 PM, Alex Elder wrote:
> This series includes a bunch of relatively small cleanups.
> They're grouped a bit below, but they apply together in
> this sequence and the later ones may have dependencies on
> those earlier in the series.

None of these were reviewed, so I'm re-submitting them.  I
have a few more to add, and may have rearranged the order
a bit since last time.  In any case, if you intend to review
anything please disregard this series and look at the patches
I'm about to post instead.

					-Alex


> Summaries:
> [PATCH 01/12] rbd: drop extra header_rwsem init
> [PATCH 02/12] rbd: simplify __rbd_remove_all_snaps()
> [PATCH 03/12] rbd: clean up a few dout() calls
> [PATCH 04/12] ceph: define snap counts as u32 everywhere
>     These four are very simple and straightforward cleanups.
> 
> [PATCH 05/12] rbd: snapc is unused in rbd_req_sync_read()
> [PATCH 06/12] rbd: drop rbd_header_from_disk() gfp_flags parameter
> [PATCH 07/12] rbd: drop rbd_dev parameter in snap functions
> [PATCH 08/12] rbd: drop rbd_req_sync_exec() "ver" parameter
>     These four each drop an unused argument from a function.
> 
> [PATCH 09/12] rbd: have __rbd_add_snap_dev() return a pointer
> [PATCH 10/12] rbd: make rbd_create_rw_ops() return a pointer
>     These two each change a function so it returns a pointer
>     rather than filling in the address of a provided pointer.
> 
> [PATCH 11/12] rbd: always pass ops array to rbd_req_sync_op()
> [PATCH 12/12] rbd: fixes in rbd_header_from_disk()
>     These comprise slightly more involved refactoring of the
>     code, but as with the rest of the patches in this series,
>     there should be no functional difference as a result.
> 
> 					-Alex
>