rbd: cleanup series

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

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.)

---
 drivers/block/rbd.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

index 6df8c62..b9895fe 100644
@@ -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

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

---
 drivers/block/rbd.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

index b9895fe..74e6a33 100644
@@ -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

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.

---
 drivers/block/rbd.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

index 74e6a33..93b2447 100644
@@ -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

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.

---
 fs/ceph/snap.c               |   18 ++++++++++--------
 fs/ceph/super.h              |    4 ++--
 include/linux/ceph/libceph.h |    2 +-
 3 files changed, 13 insertions(+), 11 deletions(-)

index e5206fc..cbb2f54 100644
@@ -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) {

index fc35036..3ea48b7 100644
@@ -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 */

index 98ec36a..0b72295 100644
@@ -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

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

---
 drivers/block/rbd.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

index 93b2447..06e022d 100644
@@ -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

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.)

---
 drivers/block/rbd.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

index 06e022d..3b4b4d2 100644
@@ -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

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

---
 drivers/block/rbd.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

index 3b4b4d2..955d75d 100644
@@ -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

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().

---
 drivers/block/rbd.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

index 955d75d..d4a8d9e 100644
@@ -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

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.

---
 drivers/block/rbd.c |   37 ++++++++++++++++++++++---------------
 1 files changed, 22 insertions(+), 15 deletions(-)

index d4a8d9e..79b0762 100644
@@ -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

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.

---
 drivers/block/rbd.c |   68
++++++++++++++++++++++++++++----------------------
 1 files changed, 38 insertions(+), 30 deletions(-)

index 79b0762..0535612 100644
@@ -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

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.

---
 drivers/block/rbd.c |   46 ++++++++++++++++------------------------------
 1 files changed, 16 insertions(+), 30 deletions(-)

index 0535612..6279186 100644
@@ -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

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.

---
 drivers/block/rbd.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

index 6279186..573202a 100644
@@ -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

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