ceph: simple cleanups (mostly rbd)

This series includes a set of fairly minor code cleanups in rbd. I won't bother summarizing them here. They are all simple nad their descriptions ought to be sufficient.

-Alex

[PATCH 1/6] rbd: drop extra header_rwsem init [PATCH 2/6] rbd: simplify __rbd_remove_all_snaps() [PATCH 3/6] rbd: clean up a few dout() calls [PATCH 4/6] ceph: define snap counts as u32 everywhere [PATCH 5/6] rbd: encapsulate header validity test [PATCH 6/6] rbd: rename rbd_device->id

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

---
 drivers/block/rbd.c |    2 --
 1 file changed, 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.9.5

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

---
 drivers/block/rbd.c |    5 ++---
 1 file 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.9.5

There was a dout() call in rbd_do_request() that was 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 file 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.9.5

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 ea072e1..4262478 100644
@@ -154,7 +154,7 @@ struct ceph_client {
 struct ceph_snap_context {
 	atomic_t nref;
 	u64 seq;
-	int num_snaps;
+	u32 num_snaps;
 	u64 snaps[];
 };
 

1.7.9.5

If an rbd image header is read and it doesn't begin with the expected magic information, a warning is displayed. This is a fairly simple test, but it could be extended at some point. Fix the comparison so it actually looks at the "text" field rather than the front of the structure.

In any case, encapsulate the validity test in its own function.

---
 drivers/block/rbd.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index 93b2447..d95d563 100644
@@ -481,6 +481,12 @@ static void rbd_coll_release(struct kref *kref)
 	kfree(coll);
 }
 
+static bool rbd_dev_ondisk_valid(struct rbd_image_header_ondisk *ondisk)
+{
+	return !memcmp(&ondisk->text,
+			RBD_HEADER_TEXT, sizeof (RBD_HEADER_TEXT));
+}
+
 /*
  * Create a new header structure, translate header format from the on-disk
  * header.

@@ -492,7 +498,7 @@ static int rbd_header_from_disk(struct

rbd_image_header *header,
{
u32 i, snap_count;

- if (memcmp(ondisk, RBD_HEADER_TEXT, sizeof(RBD_HEADER_TEXT)))
+ if (!rbd_dev_ondisk_valid(ondisk))
return -ENXIO;

snap_count = le32_to_cpu(ondisk->snap_count);

1.7.9.5

The "id" field of an rbd device structure represents the unique client-local device id mapped to the underlying rbd image. Each rbd image will have another id--the image id--and each snapshot has its own id as well. The simple name "id" no longer conveys the information one might like to have.

Rename the device "id" field in struct rbd_dev to be "dev_id" to make it a little more obvious what we're dealing with without having to think more about context.

---
 drivers/block/rbd.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

index d95d563..0fda2ed 100644
@@ -147,7 +147,7 @@ struct rbd_snap {
  * a single device
  */
 struct rbd_device {
-	int			id;		/* blkdev unique id */
+	int			dev_id;		/* blkdev unique id */
 
 	int			major;		/* blkdev assigned major */
 	struct gendisk		*disk;		/* blkdev's gendisk and rq */

@@ -1778,7 +1778,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 		goto out;
 
 	snprintf(disk->disk_name, sizeof(disk->disk_name), RBD_DRV_NAME "%d",
-		 rbd_dev->id);
+		 rbd_dev->dev_id);
 	disk->major = rbd_dev->major;
 	disk->first_minor = 0;
 	disk->fops = &rbd_bd_ops;

@@ -2167,7 +2167,7 @@ static int rbd_bus_add_dev(struct rbd_device *rbd_dev)
 	dev->type = &rbd_device_type;
 	dev->parent = &rbd_root_dev;
 	dev->release = rbd_dev_release;
-	dev_set_name(dev, "%d", rbd_dev->id);
+	dev_set_name(dev, "%d", rbd_dev->dev_id);
 	ret = device_register(dev);
 	if (ret < 0)
 		goto out;

@@ -2215,7 +2215,7 @@ static atomic64_t rbd_id_max = ATOMIC64_INIT(0);
  */
 static void rbd_id_get(struct rbd_device *rbd_dev)
 {
-	rbd_dev->id = atomic64_inc_return(&rbd_id_max);
+	rbd_dev->dev_id = atomic64_inc_return(&rbd_id_max);
 
 	spin_lock(&rbd_dev_list_lock);
 	list_add_tail(&rbd_dev->node, &rbd_dev_list);

@@ -2229,7 +2229,7 @@ static void rbd_id_get(struct rbd_device *rbd_dev)
 static void rbd_id_put(struct rbd_device *rbd_dev)
 {
 	struct list_head *tmp;
-	int rbd_id = rbd_dev->id;
+	int rbd_id = rbd_dev->dev_id;
 	int max_id;
 
 	BUG_ON(rbd_id < 1);

@@ -2468,7 +2468,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	/* Fill in the device name, now that we have its id. */
 	BUILD_BUG_ON(DEV_NAME_LEN
 			< sizeof (RBD_DRV_NAME) + MAX_INT_FORMAT_WIDTH);
-	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->id);
+	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->dev_id);
 
 	/* parse add command */
 	rc = rbd_add_parse_args(rbd_dev, buf, &mon_addrs, &mon_addrs_size,

@@ -2545,7 +2545,7 @@ err_nomem:
 	return (ssize_t) rc;
 }
 
-static struct rbd_device *__rbd_get_dev(unsigned long id)
+static struct rbd_device *__rbd_get_dev(unsigned long dev_id)
 {
 	struct list_head *tmp;
 	struct rbd_device *rbd_dev;

@@ -2553,7 +2553,7 @@ static struct rbd_device *__rbd_get_dev(unsigned

long id)
spin_lock(&rbd_dev_list_lock); list_for_each(tmp, &rbd_dev_list) {
rbd_dev = list_entry(tmp, struct rbd_device, node);
- if (rbd_dev->id == id) {
+ if (rbd_dev->dev_id == dev_id) {
spin_unlock(&rbd_dev_list_lock); return rbd_dev;
}

1.7.9.5

It looks like there are still several dout() calls using %lld when they should be %llu after these cleanups.