[PATCH 0/5] rbd: code cleanup

[PATCH 0/5] rbd: code cleanup

[Alex Elder]

This second series consists of fairly simple bits of code cleanup.

It is available as branch "wip-rbd-review-2" on the ceph-client git
repository, and is based on the branch "wip-rbd-review-1".

    https://github.com/ceph/ceph-client/tree/wip-rbd-review-2

					-Alex

[PATCH 1/5]  a0f6877 rbd: define rbd_assert()
    This defines a new macro rbd_assert() that's used to express
    invariant properties assumed by the code.  If the property does
    not hold it triggers a BUG().  Everywhere that BUG_ON() had been
    used is replaced with an equivalent call to rbd_assert().
[PATCH 2/5]  a8aac82 rbd: rename rbd_id_get()
[PATCH 3/5]  12c5aa2 rbd: rename __rbd_init_snaps_header()
    These rename some symbols to align better with how their used.
[PATCH 4/5]  e8cb272 rbd: kill rbd_dev->q
[PATCH 5/5]  1557afc rbd: kill rbd_image_header->total_snaps
    These eliminate two struct fields, one which is unused and the
    other which is redundant and therefore not necessary.

[PATCH 1/5] rbd: define rbd_assert()

[Alex Elder]

Define rbd_assert() and use it in place of various BUG_ON() calls
now present in the code.  By default assertion checking is enabled;
we want to do this differently at some point.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 7ba70c4..d84b534 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -41,6 +41,8 @@

 #include "rbd_types.h"

+#define RBD_DEBUG	/* Activate rbd_assert() calls */
+
 /*
  * The basic unit of block I/O is a sector.  It is interpreted in a
  * number of contexts in Linux (blk, bio, genhd), but the default is
@@ -232,6 +234,18 @@ static struct device rbd_root_dev = {
 	.release =      rbd_root_dev_release,
 };

+#ifdef RBD_DEBUG
+#define rbd_assert(expr)						\
+		if (unlikely(!(expr))) {				\
+			printk(KERN_ERR "\nAssertion failure in %s() "	\
+						"at line %d:\n\n"	\
+					"\trbd_assert(%s);\n\n",	\
+					__func__, __LINE__, #expr);	\
+			BUG();						\
+		}
+#else /* !RBD_DEBUG */
+#  define rbd_assert(expr)	((void) 0)
+#endif /* !RBD_DEBUG */

 static struct device *rbd_get_dev(struct rbd_device *rbd_dev)
 {
@@ -406,7 +420,8 @@ static int parse_rbd_opts_token(char *c, void *private)
 		rbd_opts->read_only = false;
 		break;
 	default:
-		BUG_ON(token);
+		rbd_assert(false);
+		break;
 	}
 	return 0;
 }
@@ -705,7 +720,7 @@ static u64 rbd_segment_length(struct rbd_device
*rbd_dev,

 	offset &= segment_size - 1;

-	BUG_ON(length > U64_MAX - offset);
+	rbd_assert(length <= U64_MAX - offset);
 	if (offset + length > segment_size)
 		length = segment_size - offset;

@@ -842,7 +857,7 @@ static struct bio *bio_chain_clone(struct bio **old,
struct bio **next,
 		total += tmp->bi_size;
 	}

-	BUG_ON(total < len);
+	rbd_assert(total == len);

 	*old = old_chain;

@@ -1101,7 +1116,7 @@ static int rbd_req_sync_op(struct rbd_device *rbd_dev,
 	struct page **pages;
 	int num_pages;

-	BUG_ON(ops == NULL);
+	rbd_assert(ops != NULL);

 	num_pages = calc_pages_for(ofs , len);
 	pages = ceph_alloc_page_vector(num_pages, GFP_KERNEL);
@@ -1163,7 +1178,7 @@ static int rbd_do_op(struct request *rq,
 	/* we've taken care of segment sizes earlier when we
 	   cloned the bios. We should never have a segment
 	   truncated at this point */
-	BUG_ON(seg_len < len);
+	rbd_assert(seg_len == len);

 	ret = rbd_do_request(rq, rbd_dev, snapc, snapid,
 			     seg_name, seg_ofs, seg_len,
@@ -2186,7 +2201,7 @@ static int __rbd_init_snaps_header(struct
rbd_device *rbd_dev)
 					     : CEPH_NOSNAP;
 		snap = links != head ? list_entry(links, struct rbd_snap, node)
 				     : NULL;
-		BUG_ON(snap && snap->id == CEPH_NOSNAP);
+		rbd_assert(!snap || snap->id != CEPH_NOSNAP);

 		if (snap_id == CEPH_NOSNAP || (snap && snap->id > snap_id)) {
 			struct list_head *next = links->next;
@@ -2222,8 +2237,9 @@ static int __rbd_init_snaps_header(struct
rbd_device *rbd_dev)
 		} else {
 			/* Already have this one */

-			BUG_ON(snap->size != rbd_dev->header.snap_sizes[index]);
-			BUG_ON(strcmp(snap->name, snap_name));
+			rbd_assert(snap->size ==
+					rbd_dev->header.snap_sizes[index]);
+			rbd_assert(!strcmp(snap->name, snap_name));

 			/* Done with this list entry; advance */

@@ -2313,7 +2329,7 @@ static void rbd_id_put(struct rbd_device *rbd_dev)
 	int rbd_id = rbd_dev->dev_id;
 	int max_id;

-	BUG_ON(rbd_id < 1);
+	rbd_assert(rbd_id > 0);

 	spin_lock(&rbd_dev_list_lock);
 	list_del_init(&rbd_dev->node);
@@ -2705,6 +2721,7 @@ static ssize_t rbd_remove(struct bus_type *bus,

 done:
 	mutex_unlock(&ctl_mutex);
+
 	return ret;
 }

-- 
1.7.9.5

Re: [PATCH 1/5] rbd: define rbd_assert()

[Josh Durgin]

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

On 09/07/2012 06:39 AM, Alex Elder wrote:
> Define rbd_assert() and use it in place of various BUG_ON() calls
> now present in the code.  By default assertion checking is enabled;
> we want to do this differently at some point.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   35 ++++++++++++++++++++++++++---------
>   1 file changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 7ba70c4..d84b534 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -41,6 +41,8 @@
>
>   #include "rbd_types.h"
>
> +#define RBD_DEBUG	/* Activate rbd_assert() calls */
> +
>   /*
>    * The basic unit of block I/O is a sector.  It is interpreted in a
>    * number of contexts in Linux (blk, bio, genhd), but the default is
> @@ -232,6 +234,18 @@ static struct device rbd_root_dev = {
>   	.release =      rbd_root_dev_release,
>   };
>
> +#ifdef RBD_DEBUG
> +#define rbd_assert(expr)						\
> +		if (unlikely(!(expr))) {				\
> +			printk(KERN_ERR "\nAssertion failure in %s() "	\
> +						"at line %d:\n\n"	\
> +					"\trbd_assert(%s);\n\n",	\
> +					__func__, __LINE__, #expr);	\
> +			BUG();						\
> +		}
> +#else /* !RBD_DEBUG */
> +#  define rbd_assert(expr)	((void) 0)
> +#endif /* !RBD_DEBUG */
>
>   static struct device *rbd_get_dev(struct rbd_device *rbd_dev)
>   {
> @@ -406,7 +420,8 @@ static int parse_rbd_opts_token(char *c, void *private)
>   		rbd_opts->read_only = false;
>   		break;
>   	default:
> -		BUG_ON(token);
> +		rbd_assert(false);
> +		break;
>   	}
>   	return 0;
>   }
> @@ -705,7 +720,7 @@ static u64 rbd_segment_length(struct rbd_device
> *rbd_dev,
>
>   	offset &= segment_size - 1;
>
> -	BUG_ON(length > U64_MAX - offset);
> +	rbd_assert(length <= U64_MAX - offset);
>   	if (offset + length > segment_size)
>   		length = segment_size - offset;
>
> @@ -842,7 +857,7 @@ static struct bio *bio_chain_clone(struct bio **old,
> struct bio **next,
>   		total += tmp->bi_size;
>   	}
>
> -	BUG_ON(total < len);
> +	rbd_assert(total == len);
>
>   	*old = old_chain;
>
> @@ -1101,7 +1116,7 @@ static int rbd_req_sync_op(struct rbd_device *rbd_dev,
>   	struct page **pages;
>   	int num_pages;
>
> -	BUG_ON(ops == NULL);
> +	rbd_assert(ops != NULL);
>
>   	num_pages = calc_pages_for(ofs , len);
>   	pages = ceph_alloc_page_vector(num_pages, GFP_KERNEL);
> @@ -1163,7 +1178,7 @@ static int rbd_do_op(struct request *rq,
>   	/* we've taken care of segment sizes earlier when we
>   	   cloned the bios. We should never have a segment
>   	   truncated at this point */
> -	BUG_ON(seg_len < len);
> +	rbd_assert(seg_len == len);
>
>   	ret = rbd_do_request(rq, rbd_dev, snapc, snapid,
>   			     seg_name, seg_ofs, seg_len,
> @@ -2186,7 +2201,7 @@ static int __rbd_init_snaps_header(struct
> rbd_device *rbd_dev)
>   					     : CEPH_NOSNAP;
>   		snap = links != head ? list_entry(links, struct rbd_snap, node)
>   				     : NULL;
> -		BUG_ON(snap && snap->id == CEPH_NOSNAP);
> +		rbd_assert(!snap || snap->id != CEPH_NOSNAP);
>
>   		if (snap_id == CEPH_NOSNAP || (snap && snap->id > snap_id)) {
>   			struct list_head *next = links->next;
> @@ -2222,8 +2237,9 @@ static int __rbd_init_snaps_header(struct
> rbd_device *rbd_dev)
>   		} else {
>   			/* Already have this one */
>
> -			BUG_ON(snap->size != rbd_dev->header.snap_sizes[index]);
> -			BUG_ON(strcmp(snap->name, snap_name));
> +			rbd_assert(snap->size ==
> +					rbd_dev->header.snap_sizes[index]);
> +			rbd_assert(!strcmp(snap->name, snap_name));
>
>   			/* Done with this list entry; advance */
>
> @@ -2313,7 +2329,7 @@ static void rbd_id_put(struct rbd_device *rbd_dev)
>   	int rbd_id = rbd_dev->dev_id;
>   	int max_id;
>
> -	BUG_ON(rbd_id < 1);
> +	rbd_assert(rbd_id > 0);
>
>   	spin_lock(&rbd_dev_list_lock);
>   	list_del_init(&rbd_dev->node);
> @@ -2705,6 +2721,7 @@ static ssize_t rbd_remove(struct bus_type *bus,
>
>   done:
>   	mutex_unlock(&ctl_mutex);
> +
>   	return ret;
>   }
>

[PATCH 2/5] rbd: rename rbd_id_get()

[Alex Elder]

This should have been done as part of this commit:

    commit de71a2970d57463d3d965025e33ec3adcf391248
    Author: Alex Elder <elder@inktank.com>
    Date:   Tue Jul 3 16:01:19 2012 -0500
    rbd: rename rbd_device->id

rbd_id_get() is assigning the rbd_dev->dev_id field.  Change the
name of that function as well as rbd_id_put() and rbd_id_max
to reflect what they are affecting.

Add some dynamic debug statements related to rbd device id activity.

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 d84b534..8cb8e0a 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2304,26 +2304,28 @@ static int rbd_init_watch_dev(struct rbd_device
*rbd_dev)
 	return ret;
 }

-static atomic64_t rbd_id_max = ATOMIC64_INIT(0);
+static atomic64_t rbd_dev_id_max = ATOMIC64_INIT(0);

 /*
  * Get a unique rbd identifier for the given new rbd_dev, and add
  * the rbd_dev to the global list.  The minimum rbd id is 1.
  */
-static void rbd_id_get(struct rbd_device *rbd_dev)
+static void rbd_dev_id_get(struct rbd_device *rbd_dev)
 {
-	rbd_dev->dev_id = atomic64_inc_return(&rbd_id_max);
+	rbd_dev->dev_id = atomic64_inc_return(&rbd_dev_id_max);

 	spin_lock(&rbd_dev_list_lock);
 	list_add_tail(&rbd_dev->node, &rbd_dev_list);
 	spin_unlock(&rbd_dev_list_lock);
+	dout("rbd_dev %p given dev id %llu\n", rbd_dev,
+		(unsigned long long) rbd_dev->dev_id);
 }

 /*
  * Remove an rbd_dev from the global list, and record that its
  * identifier is no longer in use.
  */
-static void rbd_id_put(struct rbd_device *rbd_dev)
+static void rbd_dev_id_put(struct rbd_device *rbd_dev)
 {
 	struct list_head *tmp;
 	int rbd_id = rbd_dev->dev_id;
@@ -2331,6 +2333,8 @@ static void rbd_id_put(struct rbd_device *rbd_dev)

 	rbd_assert(rbd_id > 0);

+	dout("rbd_dev %p released dev id %llu\n", rbd_dev,
+		(unsigned long long) rbd_dev->dev_id);
 	spin_lock(&rbd_dev_list_lock);
 	list_del_init(&rbd_dev->node);

@@ -2338,7 +2342,7 @@ static void rbd_id_put(struct rbd_device *rbd_dev)
 	 * If the id being "put" is not the current maximum, there
 	 * is nothing special we need to do.
 	 */
-	if (rbd_id != atomic64_read(&rbd_id_max)) {
+	if (rbd_id != atomic64_read(&rbd_dev_id_max)) {
 		spin_unlock(&rbd_dev_list_lock);
 		return;
 	}
@@ -2359,12 +2363,13 @@ static void rbd_id_put(struct rbd_device *rbd_dev)
 	spin_unlock(&rbd_dev_list_lock);

 	/*
-	 * The max id could have been updated by rbd_id_get(), in
+	 * The max id could have been updated by rbd_dev_id_get(), in
 	 * which case it now accurately reflects the new maximum.
 	 * Be careful not to overwrite the maximum value in that
 	 * case.
 	 */
-	atomic64_cmpxchg(&rbd_id_max, rbd_id, max_id);
+	atomic64_cmpxchg(&rbd_dev_id_max, rbd_id, max_id);
+	dout("  max dev id has been reset\n");
 }

 /*
@@ -2563,7 +2568,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	init_rwsem(&rbd_dev->header_rwsem);

 	/* generate unique id: find highest unique id, add one */
-	rbd_id_get(rbd_dev);
+	rbd_dev_id_get(rbd_dev);

 	/* Fill in the device name, now that we have its id. */
 	BUILD_BUG_ON(DEV_NAME_LEN
@@ -2631,7 +2636,7 @@ err_put_id:
 		kfree(rbd_dev->image_name);
 		kfree(rbd_dev->pool_name);
 	}
-	rbd_id_put(rbd_dev);
+	rbd_dev_id_put(rbd_dev);
 err_nomem:
 	kfree(rbd_dev);
 	kfree(options);
@@ -2683,7 +2688,7 @@ static void rbd_dev_release(struct device *dev)
 	kfree(rbd_dev->header_name);
 	kfree(rbd_dev->pool_name);
 	kfree(rbd_dev->image_name);
-	rbd_id_put(rbd_dev);
+	rbd_dev_id_put(rbd_dev);
 	kfree(rbd_dev);

 	/* release module ref */
-- 
1.7.9.5

Re: [PATCH 2/5] rbd: rename rbd_id_get()

[Josh Durgin]

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

On 09/07/2012 06:39 AM, Alex Elder wrote:
> This should have been done as part of this commit:
>
>      commit de71a2970d57463d3d965025e33ec3adcf391248
>      Author: Alex Elder <elder@inktank.com>
>      Date:   Tue Jul 3 16:01:19 2012 -0500
>      rbd: rename rbd_device->id
>
> rbd_id_get() is assigning the rbd_dev->dev_id field.  Change the
> name of that function as well as rbd_id_put() and rbd_id_max
> to reflect what they are affecting.
>
> Add some dynamic debug statements related to rbd device id activity.
>
> 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 d84b534..8cb8e0a 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2304,26 +2304,28 @@ static int rbd_init_watch_dev(struct rbd_device
> *rbd_dev)
>   	return ret;
>   }
>
> -static atomic64_t rbd_id_max = ATOMIC64_INIT(0);
> +static atomic64_t rbd_dev_id_max = ATOMIC64_INIT(0);
>
>   /*
>    * Get a unique rbd identifier for the given new rbd_dev, and add
>    * the rbd_dev to the global list.  The minimum rbd id is 1.
>    */
> -static void rbd_id_get(struct rbd_device *rbd_dev)
> +static void rbd_dev_id_get(struct rbd_device *rbd_dev)
>   {
> -	rbd_dev->dev_id = atomic64_inc_return(&rbd_id_max);
> +	rbd_dev->dev_id = atomic64_inc_return(&rbd_dev_id_max);
>
>   	spin_lock(&rbd_dev_list_lock);
>   	list_add_tail(&rbd_dev->node, &rbd_dev_list);
>   	spin_unlock(&rbd_dev_list_lock);
> +	dout("rbd_dev %p given dev id %llu\n", rbd_dev,
> +		(unsigned long long) rbd_dev->dev_id);
>   }
>
>   /*
>    * Remove an rbd_dev from the global list, and record that its
>    * identifier is no longer in use.
>    */
> -static void rbd_id_put(struct rbd_device *rbd_dev)
> +static void rbd_dev_id_put(struct rbd_device *rbd_dev)
>   {
>   	struct list_head *tmp;
>   	int rbd_id = rbd_dev->dev_id;
> @@ -2331,6 +2333,8 @@ static void rbd_id_put(struct rbd_device *rbd_dev)
>
>   	rbd_assert(rbd_id > 0);
>
> +	dout("rbd_dev %p released dev id %llu\n", rbd_dev,
> +		(unsigned long long) rbd_dev->dev_id);
>   	spin_lock(&rbd_dev_list_lock);
>   	list_del_init(&rbd_dev->node);
>
> @@ -2338,7 +2342,7 @@ static void rbd_id_put(struct rbd_device *rbd_dev)
>   	 * If the id being "put" is not the current maximum, there
>   	 * is nothing special we need to do.
>   	 */
> -	if (rbd_id != atomic64_read(&rbd_id_max)) {
> +	if (rbd_id != atomic64_read(&rbd_dev_id_max)) {
>   		spin_unlock(&rbd_dev_list_lock);
>   		return;
>   	}
> @@ -2359,12 +2363,13 @@ static void rbd_id_put(struct rbd_device *rbd_dev)
>   	spin_unlock(&rbd_dev_list_lock);
>
>   	/*
> -	 * The max id could have been updated by rbd_id_get(), in
> +	 * The max id could have been updated by rbd_dev_id_get(), in
>   	 * which case it now accurately reflects the new maximum.
>   	 * Be careful not to overwrite the maximum value in that
>   	 * case.
>   	 */
> -	atomic64_cmpxchg(&rbd_id_max, rbd_id, max_id);
> +	atomic64_cmpxchg(&rbd_dev_id_max, rbd_id, max_id);
> +	dout("  max dev id has been reset\n");
>   }
>
>   /*
> @@ -2563,7 +2568,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	init_rwsem(&rbd_dev->header_rwsem);
>
>   	/* generate unique id: find highest unique id, add one */
> -	rbd_id_get(rbd_dev);
> +	rbd_dev_id_get(rbd_dev);
>
>   	/* Fill in the device name, now that we have its id. */
>   	BUILD_BUG_ON(DEV_NAME_LEN
> @@ -2631,7 +2636,7 @@ err_put_id:
>   		kfree(rbd_dev->image_name);
>   		kfree(rbd_dev->pool_name);
>   	}
> -	rbd_id_put(rbd_dev);
> +	rbd_dev_id_put(rbd_dev);
>   err_nomem:
>   	kfree(rbd_dev);
>   	kfree(options);
> @@ -2683,7 +2688,7 @@ static void rbd_dev_release(struct device *dev)
>   	kfree(rbd_dev->header_name);
>   	kfree(rbd_dev->pool_name);
>   	kfree(rbd_dev->image_name);
> -	rbd_id_put(rbd_dev);
> +	rbd_dev_id_put(rbd_dev);
>   	kfree(rbd_dev);
>
>   	/* release module ref */
>

[PATCH 3/5] rbd: rename __rbd_init_snaps_header()

[Alex Elder]

The name __rbd_init_snaps_header() doesn't really convey what that
function does very well.  Its purpose is to scan a new snapshot
context and either create or destroy snapshot device entries so
that local host's view is consistent with the reality maintained
on the OSDs.  This patch just changes the name of this function,
to be rbd_dev_snap_devs_update().  Still not perfect, but I think
better.

Also add some dynamic debug statements to this function.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 8cb8e0a..7726368 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -201,7 +201,7 @@ static DEFINE_SPINLOCK(rbd_dev_list_lock);
 static LIST_HEAD(rbd_client_list);		/* clients */
 static DEFINE_SPINLOCK(rbd_client_list_lock);

-static int __rbd_init_snaps_header(struct rbd_device *rbd_dev);
+static int rbd_dev_snap_devs_update(struct rbd_device *rbd_dev);
 static void rbd_dev_release(struct device *dev);
 static ssize_t rbd_snap_add(struct device *dev,
 			    struct device_attribute *attr,
@@ -1848,7 +1848,7 @@ static int __rbd_refresh_header(struct rbd_device
*rbd_dev, u64 *hver)
 	WARN_ON(strcmp(rbd_dev->header.object_prefix, h.object_prefix));
 	kfree(h.object_prefix);

-	ret = __rbd_init_snaps_header(rbd_dev);
+	ret = rbd_dev_snap_devs_update(rbd_dev);

 	up_write(&rbd_dev->header_rwsem);

@@ -1880,7 +1880,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 		return rc;

 	/* no need to lock here, as rbd_dev is not registered yet */
-	rc = __rbd_init_snaps_header(rbd_dev);
+	rc = rbd_dev_snap_devs_update(rbd_dev);
 	if (rc)
 		return rc;

@@ -2184,7 +2184,7 @@ err:
  * snapshot id, highest id first.  (Snapshots in the rbd_dev's list
  * are also maintained in that order.)
  */
-static int __rbd_init_snaps_header(struct rbd_device *rbd_dev)
+static int rbd_dev_snap_devs_update(struct rbd_device *rbd_dev)
 {
 	struct ceph_snap_context *snapc = rbd_dev->header.snapc;
 	const u32 snap_count = snapc->num_snaps;
@@ -2193,6 +2193,7 @@ static int __rbd_init_snaps_header(struct
rbd_device *rbd_dev)
 	struct list_head *links = head->next;
 	u32 index = 0;

+	dout("%s: snap count is %u\n", __func__, (unsigned int) snap_count);
 	while (index < snap_count || links != head) {
 		u64 snap_id;
 		struct rbd_snap *snap;
@@ -2211,6 +2212,9 @@ static int __rbd_init_snaps_header(struct
rbd_device *rbd_dev)
 			if (rbd_dev->snap_id == snap->id)
 				rbd_dev->snap_exists = false;
 			__rbd_remove_snap_dev(snap);
+			dout("%ssnap id %llu has been removed\n",
+				rbd_dev->snap_id == snap->id ? "mapped " : "",
+				(unsigned long long) snap->id);

 			/* Done with this list entry; advance */

@@ -2218,6 +2222,8 @@ static int __rbd_init_snaps_header(struct
rbd_device *rbd_dev)
 			continue;
 		}

+		dout("entry %u: snap_id = %llu\n", (unsigned int) snap_count,
+			(unsigned long long) snap_id);
 		if (!snap || (snap_id != CEPH_NOSNAP && snap->id < snap_id)) {
 			struct rbd_snap *new_snap;

@@ -2225,11 +2231,17 @@ static int __rbd_init_snaps_header(struct
rbd_device *rbd_dev)

 			new_snap = __rbd_add_snap_dev(rbd_dev, index,
 							snap_name);
-			if (IS_ERR(new_snap))
-				return PTR_ERR(new_snap);
+			if (IS_ERR(new_snap)) {
+				int err = PTR_ERR(new_snap);
+
+				dout("  failed to add dev, error %d\n", err);
+
+				return err;
+			}

 			/* New goes before existing, or at end of list */

+			dout("  added dev%s\n", snap ? "" : " at end\n");
 			if (snap)
 				list_add_tail(&new_snap->node, &snap->node);
 			else
@@ -2237,6 +2249,8 @@ static int __rbd_init_snaps_header(struct
rbd_device *rbd_dev)
 		} else {
 			/* Already have this one */

+			dout("  already present\n");
+
 			rbd_assert(snap->size ==
 					rbd_dev->header.snap_sizes[index]);
 			rbd_assert(!strcmp(snap->name, snap_name));
@@ -2251,6 +2265,7 @@ static int __rbd_init_snaps_header(struct
rbd_device *rbd_dev)
 		index++;
 		snap_name += strlen(snap_name) + 1;
 	}
+	dout("%s: done\n", __func__);

 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH 3/5] rbd: rename __rbd_init_snaps_header()

[Josh Durgin]

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

On 09/07/2012 06:39 AM, Alex Elder wrote:
> The name __rbd_init_snaps_header() doesn't really convey what that
> function does very well.  Its purpose is to scan a new snapshot
> context and either create or destroy snapshot device entries so
> that local host's view is consistent with the reality maintained
> on the OSDs.  This patch just changes the name of this function,
> to be rbd_dev_snap_devs_update().  Still not perfect, but I think
> better.
>
> Also add some dynamic debug statements to this function.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   27 +++++++++++++++++++++------
>   1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 8cb8e0a..7726368 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -201,7 +201,7 @@ static DEFINE_SPINLOCK(rbd_dev_list_lock);
>   static LIST_HEAD(rbd_client_list);		/* clients */
>   static DEFINE_SPINLOCK(rbd_client_list_lock);
>
> -static int __rbd_init_snaps_header(struct rbd_device *rbd_dev);
> +static int rbd_dev_snap_devs_update(struct rbd_device *rbd_dev);
>   static void rbd_dev_release(struct device *dev);
>   static ssize_t rbd_snap_add(struct device *dev
>   			    struct device_attribute *attr,
> @@ -1848,7 +1848,7 @@ static int __rbd_refresh_header(struct rbd_device
> *rbd_dev, u64 *hver)
>   	WARN_ON(strcmp(rbd_dev->header.object_prefix, h.object_prefix));
>   	kfree(h.object_prefix);
>
> -	ret = __rbd_init_snaps_header(rbd_dev);
> +	ret = rbd_dev_snap_devs_update(rbd_dev);
>
>   	up_write(&rbd_dev->header_rwsem);
>
> @@ -1880,7 +1880,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>   		return rc;
>
>   	/* no need to lock here, as rbd_dev is not registered yet */
> -	rc = __rbd_init_snaps_header(rbd_dev);
> +	rc = rbd_dev_snap_devs_update(rbd_dev);
>   	if (rc)
>   		return rc;
>
> @@ -2184,7 +2184,7 @@ err:
>    * snapshot id, highest id first.  (Snapshots in the rbd_dev's list
>    * are also maintained in that order.)
>    */
> -static int __rbd_init_snaps_header(struct rbd_device *rbd_dev)
> +static int rbd_dev_snap_devs_update(struct rbd_device *rbd_dev)
>   {
>   	struct ceph_snap_context *snapc = rbd_dev->header.snapc;
>   	const u32 snap_count = snapc->num_snaps;
> @@ -2193,6 +2193,7 @@ static int __rbd_init_snaps_header(struct
> rbd_device *rbd_dev)
>   	struct list_head *links = head->next;
>   	u32 index = 0;
>
> +	dout("%s: snap count is %u\n", __func__, (unsigned int) snap_count);
>   	while (index < snap_count || links != head) {
>   		u64 snap_id;
>   		struct rbd_snap *snap;
> @@ -2211,6 +2212,9 @@ static int __rbd_init_snaps_header(struct
> rbd_device *rbd_dev)
>   			if (rbd_dev->snap_id == snap->id)
>   				rbd_dev->snap_exists = false;
>   			__rbd_remove_snap_dev(snap);
> +			dout("%ssnap id %llu has been removed\n",
> +				rbd_dev->snap_id == snap->id ? "mapped " : "",
> +				(unsigned long long) snap->id);
>
>   			/* Done with this list entry; advance */
>
> @@ -2218,6 +2222,8 @@ static int __rbd_init_snaps_header(struct
> rbd_device *rbd_dev)
>   			continue;
>   		}
>
> +		dout("entry %u: snap_id = %llu\n", (unsigned int) snap_count,
> +			(unsigned long long) snap_id);
>   		if (!snap || (snap_id != CEPH_NOSNAP && snap->id < snap_id)) {
>   			struct rbd_snap *new_snap;
>
> @@ -2225,11 +2231,17 @@ static int __rbd_init_snaps_header(struct
> rbd_device *rbd_dev)
>
>   			new_snap = __rbd_add_snap_dev(rbd_dev, index,
>   							snap_name);
> -			if (IS_ERR(new_snap))
> -				return PTR_ERR(new_snap);
> +			if (IS_ERR(new_snap)) {
> +				int err = PTR_ERR(new_snap);
> +
> +				dout("  failed to add dev, error %d\n", err);
> +
> +				return err;
> +			}
>
>   			/* New goes before existing, or at end of list */
>
> +			dout("  added dev%s\n", snap ? "" : " at end\n");
>   			if (snap)
>   				list_add_tail(&new_snap->node, &snap->node);
>   			else
> @@ -2237,6 +2249,8 @@ static int __rbd_init_snaps_header(struct
> rbd_device *rbd_dev)
>   		} else {
>   			/* Already have this one */
>
> +			dout("  already present\n");
> +
>   			rbd_assert(snap->size ==
>   					rbd_dev->header.snap_sizes[index]);
>   			rbd_assert(!strcmp(snap->name, snap_name));
> @@ -2251,6 +2265,7 @@ static int __rbd_init_snaps_header(struct
> rbd_device *rbd_dev)
>   		index++;
>   		snap_name += strlen(snap_name) + 1;
>   	}
> +	dout("%s: done\n", __func__);
>
>   	return 0;
>   }
>

[PATCH 4/5] rbd: kill rbd_dev->q

[Alex Elder]

A copy of rbd_dev->disk->queue is held in rbd_dev->q, but it's
never actually used.  So get just get rid of the field.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 7726368..774a36b 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -155,7 +155,6 @@ struct rbd_device {

 	int			major;		/* blkdev assigned major */
 	struct gendisk		*disk;		/* blkdev's gendisk and rq */
-	struct request_queue	*q;

 	struct rbd_options	rbd_opts;
 	struct rbd_client	*rbd_client;
@@ -1923,7 +1922,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 	q->queuedata = rbd_dev;

 	rbd_dev->disk = disk;
-	rbd_dev->q = q;

 	/* finally, announce the disk to the world */
 	set_capacity(disk, total_size / SECTOR_SIZE);
-- 
1.7.9.5

Re: [PATCH 4/5] rbd: kill rbd_dev->q

[Josh Durgin]

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

On 09/07/2012 06:39 AM, Alex Elder wrote:
> A copy of rbd_dev->disk->queue is held in rbd_dev->q, but it's
> never actually used.  So get just get rid of the field.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 7726368..774a36b 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -155,7 +155,6 @@ struct rbd_device {
>
>   	int			major;		/* blkdev assigned major */
>   	struct gendisk		*disk;		/* blkdev's gendisk and rq */
> -	struct request_queue	*q;
>
>   	struct rbd_options	rbd_opts;
>   	struct rbd_client	*rbd_client;
> @@ -1923,7 +1922,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>   	q->queuedata = rbd_dev;
>
>   	rbd_dev->disk = disk;
> -	rbd_dev->q = q;
>
>   	/* finally, announce the disk to the world */
>   	set_capacity(disk, total_size / SECTOR_SIZE);
>

[PATCH 5/5] rbd: kill rbd_image_header->total_snaps

[Alex Elder]

The "total_snaps" field in an rbd header structure is never any
different from the value of "num_snaps" stored within a snapshot
context.  Avoid any confusion by just using the value held within
the snapshot context, and get rid of the "total_snaps" field.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 774a36b..eb6b772 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -87,7 +87,6 @@ struct rbd_image_header {
 	__u8 crypt_type;
 	__u8 comp_type;
 	struct ceph_snap_context *snapc;
-	u32 total_snaps;

 	char *snap_names;
 	u64 *snap_sizes;
@@ -588,7 +587,6 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 	header->obj_order = ondisk->options.order;
 	header->crypt_type = ondisk->options.crypt_type;
 	header->comp_type = ondisk->options.comp_type;
-	header->total_snaps = snap_count;

 	/* Allocate and fill in the snapshot context */

@@ -624,7 +622,8 @@ static int snap_by_name(struct rbd_image_header
*header, const char *snap_name,
 	int i;
 	char *p = header->snap_names;

-	for (i = 0; i < header->total_snaps; i++) {
+	rbd_assert(header->snapc != NULL);
+	for (i = 0; i < header->snapc->num_snaps; i++) {
 		if (!strcmp(snap_name, p)) {

 			/* Found it.  Pass back its id and/or size */
@@ -1839,7 +1838,6 @@ static int __rbd_refresh_header(struct rbd_device
*rbd_dev, u64 *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;
 	rbd_dev->header.snapc = h.snapc;
 	rbd_dev->header.snap_names = h.snap_names;
 	rbd_dev->header.snap_sizes = h.snap_sizes;
-- 
1.7.9.5

Re: [PATCH 5/5] rbd: kill rbd_image_header->total_snaps

[Josh Durgin]

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

On 09/07/2012 06:39 AM, Alex Elder wrote:
> The "total_snaps" field in an rbd header structure is never any
> different from the value of "num_snaps" stored within a snapshot
> context.  Avoid any confusion by just using the value held within
> the snapshot context, and get rid of the "total_snaps" field.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 774a36b..eb6b772 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -87,7 +87,6 @@ struct rbd_image_header {
>   	__u8 crypt_type;
>   	__u8 comp_type;
>   	struct ceph_snap_context *snapc;
> -	u32 total_snaps;
>
>   	char *snap_names;
>   	u64 *snap_sizes;
> @@ -588,7 +587,6 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>   	header->obj_order = ondisk->options.order;
>   	header->crypt_type = ondisk->options.crypt_type;
>   	header->comp_type = ondisk->options.comp_type;
> -	header->total_snaps = snap_count;
>
>   	/* Allocate and fill in the snapshot context */
>
> @@ -624,7 +622,8 @@ static int snap_by_name(struct rbd_image_header
> *header, const char *snap_name,
>   	int i;
>   	char *p = header->snap_names;
>
> -	for (i = 0; i < header->total_snaps; i++) {
> +	rbd_assert(header->snapc != NULL);
> +	for (i = 0; i < header->snapc->num_snaps; i++) {
>   		if (!strcmp(snap_name, p)) {
>
>   			/* Found it.  Pass back its id and/or size */
> @@ -1839,7 +1838,6 @@ static int __rbd_refresh_header(struct rbd_device
> *rbd_dev, u64 *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;
>   	rbd_dev->header.snapc = h.snapc;
>   	rbd_dev->header.snap_names = h.snap_names;
>   	rbd_dev->header.snap_sizes = h.snap_sizes;
>