Another pile of patches

Another pile of patches

[Alex Elder]

I'm about to send out several series of patches.  They are in
order--each series based on the previous, so I'm summarizing
them here.

					-Alex

[PATCH, resend] rbd: simplify rbd_rq_fn()
   Resending this one

[PATCH] rbd: remove snapshots on error in rbd_add()
    Bug

[PATCH] rbd: make pool_id a 64 bit value
    Prepares for upcoming changes

[PATCH 1/2] rbd: move snap info out of rbd_mapping struct
[PATCH 2/2] rbd: rename snap_exists field
    A few changes related to mapping structure (which I'm going
    to be removing entirely soon)

[PATCH 1/2] rbd: move ceph_parse_options() call up
[PATCH 2/2] rbd: do all argument parsing in one place
    Consolidate argument parsing

[PATCH 1/8] rbd: get rid of snap_name_len
[PATCH 2/8] rbd: remove options args from rbd_add_parse_args()
[PATCH 3/8] rbd: remove snap_name arg from rbd_add_parse_args()
[PATCH 4/8] rbd: pass and populate rbd_options structure
[PATCH 5/8] rbd: have rbd_add_parse_args() return error
[PATCH 6/8] rbd: define image specification structure
[PATCH 7/8] rbd: add reference counting to rbd_spec
[PATCH 8/8] rbd: fill rbd_spec in rbd_add_parse_args()
    This a series that arranges for the argument parsing routine
    for rbd_add() to do nothing more than parse arguments.  Right
    now it's a mix of parsing and initialization, and this lays
    some solid ground work for a cleaner implementation of the
    upcoming layering features.

[PATCH, resend] rbd: simplify rbd_rq_fn()

[Alex Elder]

When processing a request, rbd_rq_fn() makes clones of the bio's in
the request's bio chain and submits the results to osd's to be
satisfied.  If a request bio straddles the boundary between objects
backing the rbd image, it must be represented by two cloned bio's,
one for the first part (at the end of one object) and one for the
second (at the beginning of the next object).

This has been handled by a function bio_chain_clone(), which
includes an interface only a mother could love, and which has
been found to have other problems.

This patch defines two new fairly generic bio functions (one which
replaces bio_chain_clone()) to help out the situation, and then
revises rbd_rq_fn() to make use of them.

First, bio_clone_range() clones a portion of a single bio, starting
at a given offset within the bio and including only as many bytes
as requested.  As a convenience, a request to clone the entire bio
is passed directly to bio_clone().

Second, bio_chain_clone_range() performs a similar function,
producing a chain of cloned bio's covering a sub-range of the
source chain.  No bio_pair structures are used, and if successful
the result will represent exactly the specified range.

Using bio_chain_clone_range() makes bio_rq_fn() a little easier
to understand, because it avoids the need to pass very much
state information between consecutive calls.  By avoiding the need
to track a bio_pair structure, it also eliminates the problem
described here:  http://tracker.newdream.net/issues/2933

Note that a block request (and therefore the complete length of
a bio chain processed in rbd_rq_fn()) is an unsigned int, while
the result of rbd_segment_length() is u64.  This change makes
this range trunctation explicit, and trips a bug if the the
segment boundary is too far off.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index c800047..cc06c55 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -826,77 +826,144 @@ static void zero_bio_chain(struct bio *chain, int
start_ofs)
 }

 /*
- * bio_chain_clone - clone a chain of bios up to a certain length.
- * might return a bio_pair that will need to be released.
+ * Clone a portion of a bio, starting at the given byte offset
+ * and continuing for the number of bytes indicated.
  */
-static struct bio *bio_chain_clone(struct bio **old, struct bio **next,
-				   struct bio_pair **bp,
-				   int len, gfp_t gfpmask)
-{
-	struct bio *old_chain = *old;
-	struct bio *new_chain = NULL;
-	struct bio *tail;
-	int total = 0;
-
-	if (*bp) {
-		bio_pair_release(*bp);
-		*bp = NULL;
-	}
+static struct bio *bio_clone_range(struct bio *bio_src,
+					unsigned int offset,
+					unsigned int len,
+					gfp_t gfpmask)
+{
+	struct bio_vec *bv;
+	unsigned int resid;
+	unsigned short idx;
+	unsigned int voff;
+	unsigned short end_idx;
+	unsigned short vcnt;
+	struct bio *bio;

-	while (old_chain && (total < len)) {
-		struct bio *tmp;
+	/* Handle the easy case for the caller */

-		tmp = bio_kmalloc(gfpmask, old_chain->bi_max_vecs);
-		if (!tmp)
-			goto err_out;
-		gfpmask &= ~__GFP_WAIT;	/* can't wait after the first */
+	if (!offset && len == bio_src->bi_size)
+		return bio_clone(bio_src, gfpmask);

-		if (total + old_chain->bi_size > len) {
-			struct bio_pair *bp;
+	if (WARN_ON_ONCE(!len))
+		return NULL;
+	if (WARN_ON_ONCE(len > bio_src->bi_size))
+		return NULL;
+	if (WARN_ON_ONCE(offset > bio_src->bi_size - len))
+		return NULL;

-			/*
-			 * this split can only happen with a single paged bio,
-			 * split_bio will BUG_ON if this is not the case
-			 */
-			dout("bio_chain_clone split! total=%d remaining=%d"
-			     "bi_size=%u\n",
-			     total, len - total, old_chain->bi_size);
+	/* Find first affected segment... */

-			/* split the bio. We'll release it either in the next
-			   call, or it will have to be released outside */
-			bp = bio_split(old_chain, (len - total) / SECTOR_SIZE);
-			if (!bp)
-				goto err_out;
+	resid = offset;
+	__bio_for_each_segment(bv, bio_src, idx, 0) {
+		if (resid < bv->bv_len)
+			break;
+		resid -= bv->bv_len;
+	}
+	voff = resid;

-			__bio_clone(tmp, &bp->bio1);
+	/* ...and the last affected segment */

-			*next = &bp->bio2;
-		} else {
-			__bio_clone(tmp, old_chain);
-			*next = old_chain->bi_next;
-		}
+	resid += len;
+	__bio_for_each_segment(bv, bio_src, end_idx, idx) {
+		if (resid <= bv->bv_len)
+			break;
+		resid -= bv->bv_len;
+	}
+	vcnt = end_idx - idx + 1;
+
+	/* Build the clone */
+
+	bio = bio_alloc(gfpmask, (unsigned int) vcnt);
+	if (!bio)
+		return NULL;	/* ENOMEM */

-		tmp->bi_bdev = NULL;
-		tmp->bi_next = NULL;
-		if (new_chain)
-			tail->bi_next = tmp;
-		else
-			new_chain = tmp;
-		tail = tmp;
-		old_chain = old_chain->bi_next;
+	bio->bi_bdev = bio_src->bi_bdev;
+	bio->bi_sector = bio_src->bi_sector + (offset >> SECTOR_SHIFT);
+	bio->bi_rw = bio_src->bi_rw;
+	bio->bi_flags |= 1 << BIO_CLONED;

-		total += tmp->bi_size;
+	/*
+	 * Copy over our part of the bio_vec, then update the first
+	 * and last (or only) entries.
+	 */
+	memcpy(&bio->bi_io_vec[0], &bio_src->bi_io_vec[idx],
+			vcnt * sizeof (struct bio_vec));
+	bio->bi_io_vec[0].bv_offset += voff;
+	if (vcnt > 1) {
+		bio->bi_io_vec[0].bv_len -= voff;
+		bio->bi_io_vec[vcnt - 1].bv_len = resid;
+	} else {
+		bio->bi_io_vec[0].bv_len = len;
 	}

-	rbd_assert(total == len);
+	bio->bi_vcnt = vcnt;
+	bio->bi_size = len;
+	bio->bi_idx = 0;
+
+	return bio;
+}
+
+/*
+ * Clone a portion of a bio chain, starting at the given byte offset
+ * into the first bio in the source chain and continuing for the
+ * number of bytes indicated.  The result is another bio chain of
+ * exactly the given length, or a null pointer on error.
+ *
+ * The bio_src and offset parameters are both in-out.  On entry they
+ * refer to the first source bio and the offset into that bio where
+ * the start of data to be cloned is located.
+ *
+ * On return, bio_src is updated to refer to the bio in the source
+ * chain that contains first un-cloned byte, and *offset will
+ * contain the offset of that byte within that bio.
+ */
+static struct bio *bio_chain_clone_range(struct bio **bio_src,
+					unsigned int *offset,
+					unsigned int len,
+					gfp_t gfpmask)
+{
+	struct bio *bi = *bio_src;
+	unsigned int off = *offset;
+	struct bio *chain = NULL;
+	struct bio **end;
+
+	/* Build up a chain of clone bios up to the limit */
+
+	if (!bi || off >= bi->bi_size || !len)
+		return NULL;		/* Nothing to clone */

-	*old = old_chain;
+	end = &chain;
+	while (len) {
+		unsigned int bi_size;
+		struct bio *bio;
+
+		if (!bi)
+			goto out_err;	/* EINVAL; ran out of bio's */
+		bi_size = min_t(unsigned int, bi->bi_size - off, len);
+		bio = bio_clone_range(bi, off, bi_size, gfpmask);
+		if (!bio)
+			goto out_err;	/* ENOMEM */
+
+		*end = bio;
+		end = &bio->bi_next;
+
+		off += bi_size;
+		if (off == bi->bi_size) {
+			bi = bi->bi_next;
+			off = 0;
+		}
+		len -= bi_size;
+	}
+	*bio_src = bi;
+	*offset = off;

-	return new_chain;
+	return chain;
+out_err:
+	bio_chain_put(chain);

-err_out:
-	dout("bio_chain_clone with err\n");
-	bio_chain_put(new_chain);
 	return NULL;
 }

@@ -1014,8 +1081,9 @@ static int rbd_do_request(struct request *rq,
 		req_data->coll_index = coll_index;
 	}

-	dout("rbd_do_request object_name=%s ofs=%llu len=%llu\n", object_name,
-		(unsigned long long) ofs, (unsigned long long) len);
+	dout("rbd_do_request object_name=%s ofs=%llu len=%llu coll=%p[%d]\n",
+		object_name, (unsigned long long) ofs,
+		(unsigned long long) len, coll, coll_index);

 	osdc = &rbd_dev->rbd_client->client->osdc;
 	req = ceph_osdc_alloc_request(osdc, flags, snapc, ops,
@@ -1463,18 +1531,16 @@ static void rbd_rq_fn(struct request_queue *q)
 {
 	struct rbd_device *rbd_dev = q->queuedata;
 	struct request *rq;
-	struct bio_pair *bp = NULL;

 	while ((rq = blk_fetch_request(q))) {
 		struct bio *bio;
-		struct bio *rq_bio, *next_bio = NULL;
 		bool do_write;
 		unsigned int size;
-		u64 op_size = 0;
 		u64 ofs;
 		int num_segs, cur_seg = 0;
 		struct rbd_req_coll *coll;
 		struct ceph_snap_context *snapc;
+		unsigned int bio_offset;

 		dout("fetched request\n");

@@ -1486,10 +1552,6 @@ static void rbd_rq_fn(struct request_queue *q)

 		/* deduce our operation (read, write) */
 		do_write = (rq_data_dir(rq) == WRITE);
-
-		size = blk_rq_bytes(rq);
-		ofs = blk_rq_pos(rq) * SECTOR_SIZE;
-		rq_bio = rq->bio;
 		if (do_write && rbd_dev->mapping.read_only) {
 			__blk_end_request_all(rq, -EROFS);
 			continue;
@@ -1512,6 +1574,10 @@ static void rbd_rq_fn(struct request_queue *q)

 		up_read(&rbd_dev->header_rwsem);

+		size = blk_rq_bytes(rq);
+		ofs = blk_rq_pos(rq) * SECTOR_SIZE;
+		bio = rq->bio;
+
 		dout("%s 0x%x bytes at 0x%llx\n",
 		     do_write ? "write" : "read",
 		     size, (unsigned long long) blk_rq_pos(rq) * SECTOR_SIZE);
@@ -1531,30 +1597,37 @@ static void rbd_rq_fn(struct request_queue *q)
 			continue;
 		}

+		bio_offset = 0;
 		do {
-			/* a bio clone to be passed down to OSD req */
+			u64 limit = rbd_segment_length(rbd_dev, ofs, size);
+			unsigned int chain_size;
+			struct bio *bio_chain;
+
+			BUG_ON(limit > (u64) UINT_MAX);
+			chain_size = (unsigned int) limit;
 			dout("rq->bio->bi_vcnt=%hu\n", rq->bio->bi_vcnt);
-			op_size = rbd_segment_length(rbd_dev, ofs, size);
+
 			kref_get(&coll->kref);
-			bio = bio_chain_clone(&rq_bio, &next_bio, &bp,
-					      op_size, GFP_ATOMIC);
-			if (bio)
+
+			/* Pass a cloned bio chain via an osd request */
+
+			bio_chain = bio_chain_clone_range(&bio,
+						&bio_offset, chain_size,
+						GFP_ATOMIC);
+			if (bio_chain)
 				(void) rbd_do_op(rq, rbd_dev, snapc,
-						ofs, op_size,
-						bio, coll, cur_seg);
+						ofs, chain_size,
+						bio_chain, coll, cur_seg);
 			else
 				rbd_coll_end_req_index(rq, coll, cur_seg,
-						       -ENOMEM, op_size);
-			size -= op_size;
-			ofs += op_size;
+						       -ENOMEM, chain_size);
+			size -= chain_size;
+			ofs += chain_size;

 			cur_seg++;
-			rq_bio = next_bio;
 		} while (size > 0);
 		kref_put(&coll->kref, rbd_coll_release);

-		if (bp)
-			bio_pair_release(bp);
 		spin_lock_irq(q->queue_lock);

 		ceph_put_snap_context(snapc);
@@ -1564,7 +1637,7 @@ static void rbd_rq_fn(struct request_queue *q)
 /*
  * a queue callback. Makes sure that we don't create a bio that spans
across
  * multiple osd objects. One exception would be with a single page bios,
- * which we handle later at bio_chain_clone
+ * which we handle later at bio_chain_clone_range()
  */
 static int rbd_merge_bvec(struct request_queue *q, struct
bvec_merge_data *bmd,
 			  struct bio_vec *bvec)
-- 
1.7.9.5

Re: [PATCH, resend] rbd: simplify rbd_rq_fn()

[Josh Durgin]

This is much easier to read now. It might be useful
to add messages for the different failure cases in
bio_chain_clone_range later.

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

On 10/26/2012 03:44 PM, Alex Elder wrote:
> When processing a request, rbd_rq_fn() makes clones of the bio's in
> the request's bio chain and submits the results to osd's to be
> satisfied.  If a request bio straddles the boundary between objects
> backing the rbd image, it must be represented by two cloned bio's,
> one for the first part (at the end of one object) and one for the
> second (at the beginning of the next object).
>
> This has been handled by a function bio_chain_clone(), which
> includes an interface only a mother could love, and which has
> been found to have other problems.
>
> This patch defines two new fairly generic bio functions (one which
> replaces bio_chain_clone()) to help out the situation, and then
> revises rbd_rq_fn() to make use of them.
>
> First, bio_clone_range() clones a portion of a single bio, starting
> at a given offset within the bio and including only as many bytes
> as requested.  As a convenience, a request to clone the entire bio
> is passed directly to bio_clone().
>
> Second, bio_chain_clone_range() performs a similar function,
> producing a chain of cloned bio's covering a sub-range of the
> source chain.  No bio_pair structures are used, and if successful
> the result will represent exactly the specified range.
>
> Using bio_chain_clone_range() makes bio_rq_fn() a little easier
> to understand, because it avoids the need to pass very much
> state information between consecutive calls.  By avoiding the need
> to track a bio_pair structure, it also eliminates the problem
> described here:  http://tracker.newdream.net/issues/2933
>
> Note that a block request (and therefore the complete length of
> a bio chain processed in rbd_rq_fn()) is an unsigned int, while
> the result of rbd_segment_length() is u64.  This change makes
> this range trunctation explicit, and trips a bug if the the
> segment boundary is too far off.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |  231
> +++++++++++++++++++++++++++++++++------------------
>   1 file changed, 152 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index c800047..cc06c55 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -826,77 +826,144 @@ static void zero_bio_chain(struct bio *chain, int
> start_ofs)
>   }
>
>   /*
> - * bio_chain_clone - clone a chain of bios up to a certain length.
> - * might return a bio_pair that will need to be released.
> + * Clone a portion of a bio, starting at the given byte offset
> + * and continuing for the number of bytes indicated.
>    */
> -static struct bio *bio_chain_clone(struct bio **old, struct bio **next,
> -				   struct bio_pair **bp,
> -				   int len, gfp_t gfpmask)
> -{
> -	struct bio *old_chain = *old;
> -	struct bio *new_chain = NULL;
> -	struct bio *tail;
> -	int total = 0;
> -
> -	if (*bp) {
> -		bio_pair_release(*bp);
> -		*bp = NULL;
> -	}
> +static struct bio *bio_clone_range(struct bio *bio_src,
> +					unsigned int offset,
> +					unsigned int len,
> +					gfp_t gfpmask)
> +{
> +	struct bio_vec *bv;
> +	unsigned int resid;
> +	unsigned short idx;
> +	unsigned int voff;
> +	unsigned short end_idx;
> +	unsigned short vcnt;
> +	struct bio *bio;
>
> -	while (old_chain && (total < len)) {
> -		struct bio *tmp;
> +	/* Handle the easy case for the caller */
>
> -		tmp = bio_kmalloc(gfpmask, old_chain->bi_max_vecs);
> -		if (!tmp)
> -			goto err_out;
> -		gfpmask &= ~__GFP_WAIT;	/* can't wait after the first */
> +	if (!offset && len == bio_src->bi_size)
> +		return bio_clone(bio_src, gfpmask);
>
> -		if (total + old_chain->bi_size > len) {
> -			struct bio_pair *bp;
> +	if (WARN_ON_ONCE(!len))
> +		return NULL;
> +	if (WARN_ON_ONCE(len > bio_src->bi_size))
> +		return NULL;
> +	if (WARN_ON_ONCE(offset > bio_src->bi_size - len))
> +		return NULL;
>
> -			/*
> -			 * this split can only happen with a single paged bio,
> -			 * split_bio will BUG_ON if this is not the case
> -			 */
> -			dout("bio_chain_clone split! total=%d remaining=%d"
> -			     "bi_size=%u\n",
> -			     total, len - total, old_chain->bi_size);
> +	/* Find first affected segment... */
>
> -			/* split the bio. We'll release it either in the next
> -			   call, or it will have to be released outside */
> -			bp = bio_split(old_chain, (len - total) / SECTOR_SIZE);
> -			if (!bp)
> -				goto err_out;
> +	resid = offset;
> +	__bio_for_each_segment(bv, bio_src, idx, 0) {
> +		if (resid < bv->bv_len)
> +			break;
> +		resid -= bv->bv_len;
> +	}
> +	voff = resid;
>
> -			__bio_clone(tmp, &bp->bio1);
> +	/* ...and the last affected segment */
>
> -			*next = &bp->bio2;
> -		} else {
> -			__bio_clone(tmp, old_chain);
> -			*next = old_chain->bi_next;
> -		}
> +	resid += len;
> +	__bio_for_each_segment(bv, bio_src, end_idx, idx) {
> +		if (resid <= bv->bv_len)
> +			break;
> +		resid -= bv->bv_len;
> +	}
> +	vcnt = end_idx - idx + 1;
> +
> +	/* Build the clone */
> +
> +	bio = bio_alloc(gfpmask, (unsigned int) vcnt);
> +	if (!bio)
> +		return NULL;	/* ENOMEM */
>
> -		tmp->bi_bdev = NULL;
> -		tmp->bi_next = NULL;
> -		if (new_chain)
> -			tail->bi_next = tmp;
> -		else
> -			new_chain = tmp;
> -		tail = tmp;
> -		old_chain = old_chain->bi_next;
> +	bio->bi_bdev = bio_src->bi_bdev;
> +	bio->bi_sector = bio_src->bi_sector + (offset >> SECTOR_SHIFT);
> +	bio->bi_rw = bio_src->bi_rw;
> +	bio->bi_flags |= 1 << BIO_CLONED;
>
> -		total += tmp->bi_size;
> +	/*
> +	 * Copy over our part of the bio_vec, then update the first
> +	 * and last (or only) entries.
> +	 */
> +	memcpy(&bio->bi_io_vec[0], &bio_src->bi_io_vec[idx],
> +			vcnt * sizeof (struct bio_vec));
> +	bio->bi_io_vec[0].bv_offset += voff;
> +	if (vcnt > 1) {
> +		bio->bi_io_vec[0].bv_len -= voff;
> +		bio->bi_io_vec[vcnt - 1].bv_len = resid;
> +	} else {
> +		bio->bi_io_vec[0].bv_len = len;
>   	}
>
> -	rbd_assert(total == len);
> +	bio->bi_vcnt = vcnt;
> +	bio->bi_size = len;
> +	bio->bi_idx = 0;
> +
> +	return bio;
> +}
> +
> +/*
> + * Clone a portion of a bio chain, starting at the given byte offset
> + * into the first bio in the source chain and continuing for the
> + * number of bytes indicated.  The result is another bio chain of
> + * exactly the given length, or a null pointer on error.
> + *
> + * The bio_src and offset parameters are both in-out.  On entry they
> + * refer to the first source bio and the offset into that bio where
> + * the start of data to be cloned is located.
> + *
> + * On return, bio_src is updated to refer to the bio in the source
> + * chain that contains first un-cloned byte, and *offset will
> + * contain the offset of that byte within that bio.
> + */
> +static struct bio *bio_chain_clone_range(struct bio **bio_src,
> +					unsigned int *offset,
> +					unsigned int len,
> +					gfp_t gfpmask)
> +{
> +	struct bio *bi = *bio_src;
> +	unsigned int off = *offset;
> +	struct bio *chain = NULL;
> +	struct bio **end;
> +
> +	/* Build up a chain of clone bios up to the limit */
> +
> +	if (!bi || off >= bi->bi_size || !len)
> +		return NULL;		/* Nothing to clone */
>
> -	*old = old_chain;
> +	end = &chain;
> +	while (len) {
> +		unsigned int bi_size;
> +		struct bio *bio;
> +
> +		if (!bi)
> +			goto out_err;	/* EINVAL; ran out of bio's */
> +		bi_size = min_t(unsigned int, bi->bi_size - off, len);
> +		bio = bio_clone_range(bi, off, bi_size, gfpmask);
> +		if (!bio)
> +			goto out_err;	/* ENOMEM */
> +
> +		*end = bio;
> +		end = &bio->bi_next;
> +
> +		off += bi_size;
> +		if (off == bi->bi_size) {
> +			bi = bi->bi_next;
> +			off = 0;
> +		}
> +		len -= bi_size;
> +	}
> +	*bio_src = bi;
> +	*offset = off;
>
> -	return new_chain;
> +	return chain;
> +out_err:
> +	bio_chain_put(chain);
>
> -err_out:
> -	dout("bio_chain_clone with err\n");
> -	bio_chain_put(new_chain);
>   	return NULL;
>   }
>
> @@ -1014,8 +1081,9 @@ static int rbd_do_request(struct request *rq,
>   		req_data->coll_index = coll_index;
>   	}
>
> -	dout("rbd_do_request object_name=%s ofs=%llu len=%llu\n", object_name,
> -		(unsigned long long) ofs, (unsigned long long) len);
> +	dout("rbd_do_request object_name=%s ofs=%llu len=%llu coll=%p[%d]\n",
> +		object_name, (unsigned long long) ofs,
> +		(unsigned long long) len, coll, coll_index);
>
>   	osdc = &rbd_dev->rbd_client->client->osdc;
>   	req = ceph_osdc_alloc_request(osdc, flags, snapc, ops,
> @@ -1463,18 +1531,16 @@ static void rbd_rq_fn(struct request_queue *q)
>   {
>   	struct rbd_device *rbd_dev = q->queuedata;
>   	struct request *rq;
> -	struct bio_pair *bp = NULL;
>
>   	while ((rq = blk_fetch_request(q))) {
>   		struct bio *bio;
> -		struct bio *rq_bio, *next_bio = NULL;
>   		bool do_write;
>   		unsigned int size;
> -		u64 op_size = 0;
>   		u64 ofs;
>   		int num_segs, cur_seg = 0;
>   		struct rbd_req_coll *coll;
>   		struct ceph_snap_context *snapc;
> +		unsigned int bio_offset;
>
>   		dout("fetched request\n");
>
> @@ -1486,10 +1552,6 @@ static void rbd_rq_fn(struct request_queue *q)
>
>   		/* deduce our operation (read, write) */
>   		do_write = (rq_data_dir(rq) == WRITE);
> -
> -		size = blk_rq_bytes(rq);
> -		ofs = blk_rq_pos(rq) * SECTOR_SIZE;
> -		rq_bio = rq->bio;
>   		if (do_write && rbd_dev->mapping.read_only) {
>   			__blk_end_request_all(rq, -EROFS);
>   			continue;
> @@ -1512,6 +1574,10 @@ static void rbd_rq_fn(struct request_queue *q)
>
>   		up_read(&rbd_dev->header_rwsem);
>
> +		size = blk_rq_bytes(rq);
> +		ofs = blk_rq_pos(rq) * SECTOR_SIZE;
> +		bio = rq->bio;
> +
>   		dout("%s 0x%x bytes at 0x%llx\n",
>   		     do_write ? "write" : "read",
>   		     size, (unsigned long long) blk_rq_pos(rq) * SECTOR_SIZE);
> @@ -1531,30 +1597,37 @@ static void rbd_rq_fn(struct request_queue *q)
>   			continue;
>   		}
>
> +		bio_offset = 0;
>   		do {
> -			/* a bio clone to be passed down to OSD req */
> +			u64 limit = rbd_segment_length(rbd_dev, ofs, size);
> +			unsigned int chain_size;
> +			struct bio *bio_chain;
> +
> +			BUG_ON(limit > (u64) UINT_MAX);
> +			chain_size = (unsigned int) limit;
>   			dout("rq->bio->bi_vcnt=%hu\n", rq->bio->bi_vcnt);
> -			op_size = rbd_segment_length(rbd_dev, ofs, size);
> +
>   			kref_get(&coll->kref);
> -			bio = bio_chain_clone(&rq_bio, &next_bio, &bp,
> -					      op_size, GFP_ATOMIC);
> -			if (bio)
> +
> +			/* Pass a cloned bio chain via an osd request */
> +
> +			bio_chain = bio_chain_clone_range(&bio,
> +						&bio_offset, chain_size,
> +						GFP_ATOMIC);
> +			if (bio_chain)
>   				(void) rbd_do_op(rq, rbd_dev, snapc,
> -						ofs, op_size,
> -						bio, coll, cur_seg);
> +						ofs, chain_size,
> +						bio_chain, coll, cur_seg);
>   			else
>   				rbd_coll_end_req_index(rq, coll, cur_seg,
> -						       -ENOMEM, op_size);
> -			size -= op_size;
> -			ofs += op_size;
> +						       -ENOMEM, chain_size);
> +			size -= chain_size;
> +			ofs += chain_size;
>
>   			cur_seg++;
> -			rq_bio = next_bio;
>   		} while (size > 0);
>   		kref_put(&coll->kref, rbd_coll_release);
>
> -		if (bp)
> -			bio_pair_release(bp);
>   		spin_lock_irq(q->queue_lock);
>
>   		ceph_put_snap_context(snapc);
> @@ -1564,7 +1637,7 @@ static void rbd_rq_fn(struct request_queue *q)
>   /*
>    * a queue callback. Makes sure that we don't create a bio that spans
> across
>    * multiple osd objects. One exception would be with a single page bios,
> - * which we handle later at bio_chain_clone
> + * which we handle later at bio_chain_clone_range()
>    */
>   static int rbd_merge_bvec(struct request_queue *q, struct
> bvec_merge_data *bmd,
>   			  struct bio_vec *bvec)
>

Re: [PATCH, resend] rbd: simplify rbd_rq_fn()

[Alex Elder]

On 10/29/2012 03:29 PM, Josh Durgin wrote:
> This is much easier to read now. It might be useful
> to add messages for the different failure cases in
> bio_chain_clone_range later.

I've added this to my growing list of cleanup tasks.

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

[PATCH] rbd: remove snapshots on error in rbd_add()

[Alex Elder]

If rbd_dev_snaps_update() has ever been called for an rbd device
structure there could be snapshot structures on its snaps list.
In rbd_add(), this function is called but a subsequent error
path neglected to clean up any of these snapshots.

Add a call to rbd_remove_all_snaps() in the appropriate spot to
remedy this.  Change a couple of error labels to be a little
clearer while there.

And drop the leading underscorse from the function name; there's
nothing special about that function that they might signify.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index cc06c55..147c8df 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1787,7 +1787,7 @@ static int rbd_read_header(struct rbd_device *rbd_dev,
 	return ret;
 }

-static void __rbd_remove_all_snaps(struct rbd_device *rbd_dev)
+static void rbd_remove_all_snaps(struct rbd_device *rbd_dev)
 {
 	struct rbd_snap *snap;
 	struct rbd_snap *next;
@@ -3179,11 +3179,11 @@ static ssize_t rbd_add(struct bus_type *bus,
 	/* no need to lock here, as rbd_dev is not registered yet */
 	rc = rbd_dev_snaps_update(rbd_dev);
 	if (rc)
-		goto err_out_header;
+		goto err_out_probe;

 	rc = rbd_dev_set_mapping(rbd_dev, snap_name);
 	if (rc)
-		goto err_out_header;
+		goto err_out_snaps;

 	/* generate unique id: find highest unique id, add one */
 	rbd_dev_id_get(rbd_dev);
@@ -3247,7 +3247,9 @@ err_out_blkdev:
 	unregister_blkdev(rbd_dev->major, rbd_dev->name);
 err_out_id:
 	rbd_dev_id_put(rbd_dev);
-err_out_header:
+err_out_snaps:
+	rbd_remove_all_snaps(rbd_dev);
+err_out_probe:
 	rbd_header_free(&rbd_dev->header);
 err_out_client:
 	kfree(rbd_dev->header_name);
@@ -3345,7 +3347,7 @@ static ssize_t rbd_remove(struct bus_type *bus,
 		goto done;
 	}

-	__rbd_remove_all_snaps(rbd_dev);
+	rbd_remove_all_snaps(rbd_dev);
 	rbd_bus_del_dev(rbd_dev);

 done:
-- 
1.7.9.5

Re: [PATCH] rbd: remove snapshots on error in rbd_add()

[Josh Durgin]

On 10/26/2012 03:45 PM, Alex Elder wrote:
> If rbd_dev_snaps_update() has ever been called for an rbd device
> structure there could be snapshot structures on its snaps list.
> In rbd_add(), this function is called but a subsequent error
> path neglected to clean up any of these snapshots.
>
> Add a call to rbd_remove_all_snaps() in the appropriate spot to
> remedy this.  Change a couple of error labels to be a little
> clearer while there.
>
> And drop the leading underscorse from the function name; there's

underscores

> nothing special about that function that they might signify.

These could be removed for __rbd_remove_snap_dev() too.

> Signed-off-by: Alex Elder <elder@inktank.com>
> ---

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

>   drivers/block/rbd.c |   12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index cc06c55..147c8df 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -1787,7 +1787,7 @@ static int rbd_read_header(struct rbd_device *rbd_dev,
>   	return ret;
>   }
>
> -static void __rbd_remove_all_snaps(struct rbd_device *rbd_dev)
> +static void rbd_remove_all_snaps(struct rbd_device *rbd_dev)
>   {
>   	struct rbd_snap *snap;
>   	struct rbd_snap *next;
> @@ -3179,11 +3179,11 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	/* no need to lock here, as rbd_dev is not registered yet */
>   	rc = rbd_dev_snaps_update(rbd_dev);
>   	if (rc)
> -		goto err_out_header;
> +		goto err_out_probe;
>
>   	rc = rbd_dev_set_mapping(rbd_dev, snap_name);
>   	if (rc)
> -		goto err_out_header;
> +		goto err_out_snaps;
>
>   	/* generate unique id: find highest unique id, add one */
>   	rbd_dev_id_get(rbd_dev);
> @@ -3247,7 +3247,9 @@ err_out_blkdev:
>   	unregister_blkdev(rbd_dev->major, rbd_dev->name);
>   err_out_id:
>   	rbd_dev_id_put(rbd_dev);
> -err_out_header:
> +err_out_snaps:
> +	rbd_remove_all_snaps(rbd_dev);
> +err_out_probe:
>   	rbd_header_free(&rbd_dev->header);
>   err_out_client:
>   	kfree(rbd_dev->header_name);
> @@ -3345,7 +3347,7 @@ static ssize_t rbd_remove(struct bus_type *bus,
>   		goto done;
>   	}
>
> -	__rbd_remove_all_snaps(rbd_dev);
> +	rbd_remove_all_snaps(rbd_dev);
>   	rbd_bus_del_dev(rbd_dev);
>
>   done:
>

[PATCH] rbd: make pool_id a 64 bit value

[Alex Elder]

If a format 2 image has a parent, its pool id will be specified
using a 64-bit value.  Change the pool id we save for an image to
match that.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 147c8df..2a523a1 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -197,7 +197,7 @@ struct rbd_device {
 	size_t			image_name_len;
 	char			*header_name;
 	char			*pool_name;
-	int			pool_id;
+	u64			pool_id;

 	struct ceph_osd_event   *watch_event;
 	struct ceph_osd_request *watch_request;
@@ -1113,7 +1113,7 @@ static int rbd_do_request(struct request *rq,
 	layout->fl_stripe_unit = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
 	layout->fl_stripe_count = cpu_to_le32(1);
 	layout->fl_object_size = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
-	layout->fl_pg_pool = cpu_to_le32(rbd_dev->pool_id);
+	layout->fl_pg_pool = cpu_to_le32((int) rbd_dev->pool_id);
 	ret = ceph_calc_raw_layout(osdc, layout, snapid, ofs, &len, &bno,
 				   req, ops);
 	rbd_assert(ret == 0);
@@ -1982,7 +1982,7 @@ static ssize_t rbd_pool_id_show(struct device *dev,
 {
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

-	return sprintf(buf, "%d\n", rbd_dev->pool_id);
+	return sprintf(buf, "%llu\n", (unsigned long long) rbd_dev->pool_id);
 }

 static ssize_t rbd_name_show(struct device *dev,
@@ -3170,7 +3170,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name);
 	if (rc < 0)
 		goto err_out_client;
-	rbd_dev->pool_id = rc;
+	rbd_dev->pool_id = (u64) rc;

 	rc = rbd_dev_probe(rbd_dev);
 	if (rc < 0)
-- 
1.7.9.5

Re: [PATCH] rbd: make pool_id a 64 bit value

[Josh Durgin]

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

On 10/26/2012 03:45 PM, Alex Elder wrote:
> If a format 2 image has a parent, its pool id will be specified
> using a 64-bit value.  Change the pool id we save for an image to
> match that.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 147c8df..2a523a1 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -197,7 +197,7 @@ struct rbd_device {
>   	size_t			image_name_len;
>   	char			*header_name;
>   	char			*pool_name;
> -	int			pool_id;
> +	u64			pool_id;
>
>   	struct ceph_osd_event   *watch_event;
>   	struct ceph_osd_request *watch_request;
> @@ -1113,7 +1113,7 @@ static int rbd_do_request(struct request *rq,
>   	layout->fl_stripe_unit = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
>   	layout->fl_stripe_count = cpu_to_le32(1);
>   	layout->fl_object_size = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
> -	layout->fl_pg_pool = cpu_to_le32(rbd_dev->pool_id);
> +	layout->fl_pg_pool = cpu_to_le32((int) rbd_dev->pool_id);
>   	ret = ceph_calc_raw_layout(osdc, layout, snapid, ofs, &len, &bno,
>   				   req, ops);
>   	rbd_assert(ret == 0);
> @@ -1982,7 +1982,7 @@ static ssize_t rbd_pool_id_show(struct device *dev,
>   {
>   	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>
> -	return sprintf(buf, "%d\n", rbd_dev->pool_id);
> +	return sprintf(buf, "%llu\n", (unsigned long long) rbd_dev->pool_id);
>   }
>
>   static ssize_t rbd_name_show(struct device *dev,
> @@ -3170,7 +3170,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name);
>   	if (rc < 0)
>   		goto err_out_client;
> -	rbd_dev->pool_id = rc;
> +	rbd_dev->pool_id = (u64) rc;
>
>   	rc = rbd_dev_probe(rbd_dev);
>   	if (rc < 0)
>

[PATCH 0/2] rbd: mapping structure changes

[Alex Elder]

This small series just affects the rbd_mapping structure.  It
moves a few fields out of it and back into the rbd_device
structure.  The second one also modifies the interpretation
of the "exists" flag for a mapped image.

					-Alex

[PATCH 1/2] rbd: move snap info out of rbd_mapping struct
[PATCH 2/2] rbd: rename snap_exists field

[PATCH 1/2] rbd: move snap info out of rbd_mapping struct

[Alex Elder]

Moving the snap_id and snap_name fields into the separate
rbd_mapping structure was misguided.  (And in time, perhaps
we'll do away with that structure altogether...)

Move these fields back into struct rbd_device.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 2a523a1..05525b2 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -166,8 +166,6 @@ struct rbd_snap {
 };

 struct rbd_mapping {
-	char                    *snap_name;
-	u64                     snap_id;
 	u64                     size;
 	u64                     features;
 	bool                    snap_exists;
@@ -199,6 +197,9 @@ struct rbd_device {
 	char			*pool_name;
 	u64			pool_id;

+	char                    *snap_name;
+	u64                     snap_id;
+
 	struct ceph_osd_event   *watch_event;
 	struct ceph_osd_request *watch_request;

@@ -669,7 +670,7 @@ static int snap_by_name(struct rbd_device *rbd_dev,
const char *snap_name)

 	list_for_each_entry(snap, &rbd_dev->snaps, node) {
 		if (!strcmp(snap_name, snap->name)) {
-			rbd_dev->mapping.snap_id = snap->id;
+			rbd_dev->snap_id = snap->id;
 			rbd_dev->mapping.size = snap->size;
 			rbd_dev->mapping.features = snap->features;

@@ -686,7 +687,7 @@ static int rbd_dev_set_mapping(struct rbd_device
*rbd_dev, char *snap_name)

 	if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME,
 		    sizeof (RBD_SNAP_HEAD_NAME))) {
-		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
+		rbd_dev->snap_id = CEPH_NOSNAP;
 		rbd_dev->mapping.size = rbd_dev->header.image_size;
 		rbd_dev->mapping.features = rbd_dev->header.features;
 		rbd_dev->mapping.snap_exists = false;
@@ -698,7 +699,7 @@ static int rbd_dev_set_mapping(struct rbd_device
*rbd_dev, char *snap_name)
 		rbd_dev->mapping.snap_exists = true;
 		rbd_dev->mapping.read_only = true;
 	}
-	rbd_dev->mapping.snap_name = snap_name;
+	rbd_dev->snap_name = snap_name;
 done:
 	return ret;
 }
@@ -1278,7 +1279,7 @@ static int rbd_do_op(struct request *rq,
 		opcode = CEPH_OSD_OP_READ;
 		flags = CEPH_OSD_FLAG_READ;
 		snapc = NULL;
-		snapid = rbd_dev->mapping.snap_id;
+		snapid = rbd_dev->snap_id;
 		payload_len = 0;
 	}

@@ -1561,7 +1562,7 @@ static void rbd_rq_fn(struct request_queue *q)

 		down_read(&rbd_dev->header_rwsem);

-		if (rbd_dev->mapping.snap_id != CEPH_NOSNAP &&
+		if (rbd_dev->snap_id != CEPH_NOSNAP &&
 				!rbd_dev->mapping.snap_exists) {
 			up_read(&rbd_dev->header_rwsem);
 			dout("request for non-existent snapshot");
@@ -1800,7 +1801,7 @@ static void rbd_update_mapping_size(struct
rbd_device *rbd_dev)
 {
 	sector_t size;

-	if (rbd_dev->mapping.snap_id != CEPH_NOSNAP)
+	if (rbd_dev->snap_id != CEPH_NOSNAP)
 		return;

 	size = (sector_t) rbd_dev->header.image_size / SECTOR_SIZE;
@@ -2011,7 +2012,7 @@ static ssize_t rbd_snap_show(struct device *dev,
 {
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

-	return sprintf(buf, "%s\n", rbd_dev->mapping.snap_name);
+	return sprintf(buf, "%s\n", rbd_dev->snap_name);
 }

 static ssize_t rbd_image_refresh(struct device *dev,
@@ -2567,12 +2568,11 @@ static int rbd_dev_snaps_update(struct
rbd_device *rbd_dev)

 			/* Existing snapshot not in the new snap context */

-			if (rbd_dev->mapping.snap_id == snap->id)
+			if (rbd_dev->snap_id == snap->id)
 				rbd_dev->mapping.snap_exists = false;
 			__rbd_remove_snap_dev(snap);
 			dout("%ssnap id %llu has been removed\n",
-				rbd_dev->mapping.snap_id == snap->id ?
-								"mapped " : "",
+				rbd_dev->snap_id == snap->id ?  "mapped " : "",
 				(unsigned long long) snap->id);

 			/* Done with this list entry; advance */
@@ -3256,7 +3256,7 @@ err_out_client:
 	rbd_put_client(rbd_dev);
 	kfree(rbd_dev->image_id);
 err_out_args:
-	kfree(rbd_dev->mapping.snap_name);
+	kfree(rbd_dev->snap_name);
 	kfree(rbd_dev->image_name);
 	kfree(rbd_dev->pool_name);
 err_out_mem:
@@ -3309,7 +3309,7 @@ static void rbd_dev_release(struct device *dev)
 	rbd_header_free(&rbd_dev->header);

 	/* done with the id, and with the rbd_dev */
-	kfree(rbd_dev->mapping.snap_name);
+	kfree(rbd_dev->snap_name);
 	kfree(rbd_dev->image_id);
 	kfree(rbd_dev->header_name);
 	kfree(rbd_dev->pool_name);
-- 
1.7.9.5

Re: [PATCH 1/2] rbd: move snap info out of rbd_mapping struct

[Josh Durgin]

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

On 10/26/2012 03:51 PM, Alex Elder wrote:
> Moving the snap_id and snap_name fields into the separate
> rbd_mapping structure was misguided.  (And in time, perhaps
> we'll do away with that structure altogether...)
>
> Move these fields back into struct rbd_device.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 2a523a1..05525b2 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -166,8 +166,6 @@ struct rbd_snap {
>   };
>
>   struct rbd_mapping {
> -	char                    *snap_name;
> -	u64                     snap_id;
>   	u64                     size;
>   	u64                     features;
>   	bool                    snap_exists;
> @@ -199,6 +197,9 @@ struct rbd_device {
>   	char			*pool_name;
>   	u64			pool_id;
>
> +	char                    *snap_name;
> +	u64                     snap_id;
> +
>   	struct ceph_osd_event   *watch_event;
>   	struct ceph_osd_request *watch_request;
>
> @@ -669,7 +670,7 @@ static int snap_by_name(struct rbd_device *rbd_dev,
> const char *snap_name)
>
>   	list_for_each_entry(snap, &rbd_dev->snaps, node) {
>   		if (!strcmp(snap_name, snap->name)) {
> -			rbd_dev->mapping.snap_id = snap->id;
> +			rbd_dev->snap_id = snap->id;
>   			rbd_dev->mapping.size = snap->size;
>   			rbd_dev->mapping.features = snap->features;
>
> @@ -686,7 +687,7 @@ static int rbd_dev_set_mapping(struct rbd_device
> *rbd_dev, char *snap_name)
>
>   	if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME,
>   		    sizeof (RBD_SNAP_HEAD_NAME))) {
> -		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
> +		rbd_dev->snap_id = CEPH_NOSNAP;
>   		rbd_dev->mapping.size = rbd_dev->header.image_size;
>   		rbd_dev->mapping.features = rbd_dev->header.features;
>   		rbd_dev->mapping.snap_exists = false;
> @@ -698,7 +699,7 @@ static int rbd_dev_set_mapping(struct rbd_device
> *rbd_dev, char *snap_name)
>   		rbd_dev->mapping.snap_exists = true;
>   		rbd_dev->mapping.read_only = true;
>   	}
> -	rbd_dev->mapping.snap_name = snap_name;
> +	rbd_dev->snap_name = snap_name;
>   done:
>   	return ret;
>   }
> @@ -1278,7 +1279,7 @@ static int rbd_do_op(struct request *rq,
>   		opcode = CEPH_OSD_OP_READ;
>   		flags = CEPH_OSD_FLAG_READ;
>   		snapc = NULL;
> -		snapid = rbd_dev->mapping.snap_id;
> +		snapid = rbd_dev->snap_id;
>   		payload_len = 0;
>   	}
>
> @@ -1561,7 +1562,7 @@ static void rbd_rq_fn(struct request_queue *q)
>
>   		down_read(&rbd_dev->header_rwsem);
>
> -		if (rbd_dev->mapping.snap_id != CEPH_NOSNAP &&
> +		if (rbd_dev->snap_id != CEPH_NOSNAP &&
>   				!rbd_dev->mapping.snap_exists) {
>   			up_read(&rbd_dev->header_rwsem);
>   			dout("request for non-existent snapshot");
> @@ -1800,7 +1801,7 @@ static void rbd_update_mapping_size(struct
> rbd_device *rbd_dev)
>   {
>   	sector_t size;
>
> -	if (rbd_dev->mapping.snap_id != CEPH_NOSNAP)
> +	if (rbd_dev->snap_id != CEPH_NOSNAP)
>   		return;
>
>   	size = (sector_t) rbd_dev->header.image_size / SECTOR_SIZE;
> @@ -2011,7 +2012,7 @@ static ssize_t rbd_snap_show(struct device *dev,
>   {
>   	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>
> -	return sprintf(buf, "%s\n", rbd_dev->mapping.snap_name);
> +	return sprintf(buf, "%s\n", rbd_dev->snap_name);
>   }
>
>   static ssize_t rbd_image_refresh(struct device *dev,
> @@ -2567,12 +2568,11 @@ static int rbd_dev_snaps_update(struct
> rbd_device *rbd_dev)
>
>   			/* Existing snapshot not in the new snap context */
>
> -			if (rbd_dev->mapping.snap_id == snap->id)
> +			if (rbd_dev->snap_id == snap->id)
>   				rbd_dev->mapping.snap_exists = false;
>   			__rbd_remove_snap_dev(snap);
>   			dout("%ssnap id %llu has been removed\n",
> -				rbd_dev->mapping.snap_id == snap->id ?
> -								"mapped " : "",
> +				rbd_dev->snap_id == snap->id ?  "mapped " : "",
>   				(unsigned long long) snap->id);
>
>   			/* Done with this list entry; advance */
> @@ -3256,7 +3256,7 @@ err_out_client:
>   	rbd_put_client(rbd_dev);
>   	kfree(rbd_dev->image_id);
>   err_out_args:
> -	kfree(rbd_dev->mapping.snap_name);
> +	kfree(rbd_dev->snap_name);
>   	kfree(rbd_dev->image_name);
>   	kfree(rbd_dev->pool_name);
>   err_out_mem:
> @@ -3309,7 +3309,7 @@ static void rbd_dev_release(struct device *dev)
>   	rbd_header_free(&rbd_dev->header);
>
>   	/* done with the id, and with the rbd_dev */
> -	kfree(rbd_dev->mapping.snap_name);
> +	kfree(rbd_dev->snap_name);
>   	kfree(rbd_dev->image_id);
>   	kfree(rbd_dev->header_name);
>   	kfree(rbd_dev->pool_name);
>

[PATCH 2/2] rbd: rename snap_exists field

[Alex Elder]

A Boolean field "snap_exists" in an rbd mapping is used to indicate
whether a mapped snapshot has been removed from an image's snapshot
context, to stop sending requests for that snapshot as soon as we
know it's gone.

Generalize the interpretation of this field so it applies to
non-snapshot (i.e. "head") mappings.  That is, define its value
to be false until the mapping has been set, and then define it to be
true for both snapshot mappings or head mappings.

Rename the field "exists" to reflect the broader interpretation.
The rbd_mapping structure is on its way out, so move the field
back into the rbd_device structure.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 05525b2..fb44d4d 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -168,7 +168,6 @@ struct rbd_snap {
 struct rbd_mapping {
 	u64                     size;
 	u64                     features;
-	bool                    snap_exists;
 	bool			read_only;
 };

@@ -189,6 +188,7 @@ struct rbd_device {
 	spinlock_t		lock;		/* queue lock */

 	struct rbd_image_header	header;
+	bool                    exists;
 	char			*image_id;
 	size_t			image_id_len;
 	char			*image_name;
@@ -690,16 +690,15 @@ static int rbd_dev_set_mapping(struct rbd_device
*rbd_dev, char *snap_name)
 		rbd_dev->snap_id = CEPH_NOSNAP;
 		rbd_dev->mapping.size = rbd_dev->header.image_size;
 		rbd_dev->mapping.features = rbd_dev->header.features;
-		rbd_dev->mapping.snap_exists = false;
 		ret = 0;
 	} else {
 		ret = snap_by_name(rbd_dev, snap_name);
 		if (ret < 0)
 			goto done;
-		rbd_dev->mapping.snap_exists = true;
 		rbd_dev->mapping.read_only = true;
 	}
 	rbd_dev->snap_name = snap_name;
+	rbd_dev->exists = true;
 done:
 	return ret;
 }
@@ -1562,8 +1561,8 @@ static void rbd_rq_fn(struct request_queue *q)

 		down_read(&rbd_dev->header_rwsem);

-		if (rbd_dev->snap_id != CEPH_NOSNAP &&
-				!rbd_dev->mapping.snap_exists) {
+		if (!rbd_dev->exists) {
+			rbd_assert(rbd_dev->snap_id != CEPH_NOSNAP);
 			up_read(&rbd_dev->header_rwsem);
 			dout("request for non-existent snapshot");
 			spin_lock_irq(q->queue_lock);
@@ -2569,7 +2568,7 @@ static int rbd_dev_snaps_update(struct rbd_device
*rbd_dev)
 			/* Existing snapshot not in the new snap context */

 			if (rbd_dev->snap_id == snap->id)
-				rbd_dev->mapping.snap_exists = false;
+				rbd_dev->exists = false;
 			__rbd_remove_snap_dev(snap);
 			dout("%ssnap id %llu has been removed\n",
 				rbd_dev->snap_id == snap->id ?  "mapped " : "",
-- 
1.7.9.5

Re: [PATCH 2/2] rbd: rename snap_exists field

[Josh Durgin]

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

On 10/26/2012 03:51 PM, Alex Elder wrote:
> A Boolean field "snap_exists" in an rbd mapping is used to indicate
> whether a mapped snapshot has been removed from an image's snapshot
> context, to stop sending requests for that snapshot as soon as we
> know it's gone.
>
> Generalize the interpretation of this field so it applies to
> non-snapshot (i.e. "head") mappings.  That is, define its value
> to be false until the mapping has been set, and then define it to be
> true for both snapshot mappings or head mappings.
>
> Rename the field "exists" to reflect the broader interpretation.
> The rbd_mapping structure is on its way out, so move the field
> back into the rbd_device structure.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 05525b2..fb44d4d 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -168,7 +168,6 @@ struct rbd_snap {
>   struct rbd_mapping {
>   	u64                     size;
>   	u64                     features;
> -	bool                    snap_exists;
>   	bool			read_only;
>   };
>
> @@ -189,6 +188,7 @@ struct rbd_device {
>   	spinlock_t		lock;		/* queue lock */
>
>   	struct rbd_image_header	header;
> +	bool                    exists;
>   	char			*image_id;
>   	size_t			image_id_len;
>   	char			*image_name;
> @@ -690,16 +690,15 @@ static int rbd_dev_set_mapping(struct rbd_device
> *rbd_dev, char *snap_name)
>   		rbd_dev->snap_id = CEPH_NOSNAP;
>   		rbd_dev->mapping.size = rbd_dev->header.image_size;
>   		rbd_dev->mapping.features = rbd_dev->header.features;
> -		rbd_dev->mapping.snap_exists = false;
>   		ret = 0;
>   	} else {
>   		ret = snap_by_name(rbd_dev, snap_name);
>   		if (ret < 0)
>   			goto done;
> -		rbd_dev->mapping.snap_exists = true;
>   		rbd_dev->mapping.read_only = true;
>   	}
>   	rbd_dev->snap_name = snap_name;
> +	rbd_dev->exists = true;
>   done:
>   	return ret;
>   }
> @@ -1562,8 +1561,8 @@ static void rbd_rq_fn(struct request_queue *q)
>
>   		down_read(&rbd_dev->header_rwsem);
>
> -		if (rbd_dev->snap_id != CEPH_NOSNAP &&
> -				!rbd_dev->mapping.snap_exists) {
> +		if (!rbd_dev->exists) {
> +			rbd_assert(rbd_dev->snap_id != CEPH_NOSNAP);
>   			up_read(&rbd_dev->header_rwsem);
>   			dout("request for non-existent snapshot");
>   			spin_lock_irq(q->queue_lock);
> @@ -2569,7 +2568,7 @@ static int rbd_dev_snaps_update(struct rbd_device
> *rbd_dev)
>   			/* Existing snapshot not in the new snap context */
>
>   			if (rbd_dev->snap_id == snap->id)
> -				rbd_dev->mapping.snap_exists = false;
> +				rbd_dev->exists = false;
>   			__rbd_remove_snap_dev(snap);
>   			dout("%ssnap id %llu has been removed\n",
>   				rbd_dev->snap_id == snap->id ?  "mapped " : "",
>

[PATCH 0/2] rbd: consolidate argument parsing

[Alex Elder]

This series moves some code that handles argument parsing
out of rbd_get_client() and into rbd_add_parse_args().

					-Alex

[PATCH 1/2] rbd: move ceph_parse_options() call up
[PATCH 2/2] rbd: do all argument parsing in one place

[PATCH 1/2] rbd: move ceph_parse_options() call up

[Alex Elder]

Move option parsing out of rbd_get_client() and into its caller.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index fb44d4d..6e36c63 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -453,27 +453,11 @@ static int parse_rbd_opts_token(char *c, void
*private)
  * Get a ceph client with specific addr and configuration, if one does
  * not exist create it.
  */
-static int rbd_get_client(struct rbd_device *rbd_dev, const char *mon_addr,
-				size_t mon_addr_len, char *options)
+static int rbd_get_client(struct rbd_device *rbd_dev,
+				struct ceph_options *ceph_opts)
 {
-	struct rbd_options rbd_opts;
-	struct ceph_options *ceph_opts;
 	struct rbd_client *rbdc;

-	/* Initialize all rbd options to the defaults */
-
-	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;
-
-	ceph_opts = ceph_parse_options(options, mon_addr,
-					mon_addr + mon_addr_len,
-					parse_rbd_opts_token, &rbd_opts);
-	if (IS_ERR(ceph_opts))
-		return PTR_ERR(ceph_opts);
-
-	/* Record the parsed rbd options */
-
-	rbd_dev->mapping.read_only = rbd_opts.read_only;
-
 	rbdc = rbd_client_find(ceph_opts);
 	if (rbdc) {
 		/* using an existing client */
@@ -3132,9 +3116,11 @@ static ssize_t rbd_add(struct bus_type *bus,
 	struct rbd_device *rbd_dev = NULL;
 	const char *mon_addrs = NULL;
 	size_t mon_addrs_size = 0;
+	char *snap_name;
+	struct rbd_options rbd_opts;
+	struct ceph_options *ceph_opts;
 	struct ceph_osd_client *osdc;
 	int rc = -ENOMEM;
-	char *snap_name;

 	if (!try_module_get(THIS_MODULE))
 		return -ENODEV;
@@ -3160,9 +3146,26 @@ static ssize_t rbd_add(struct bus_type *bus,
 		goto err_out_mem;
 	}

-	rc = rbd_get_client(rbd_dev, mon_addrs, mon_addrs_size - 1, options);
-	if (rc < 0)
+	/* Initialize all rbd options to the defaults */
+
+	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;
+
+	ceph_opts = ceph_parse_options(options, mon_addrs,
+					mon_addrs + mon_addrs_size - 1,
+					parse_rbd_opts_token, &rbd_opts);
+	if (IS_ERR(ceph_opts)) {
+		rc = PTR_ERR(ceph_opts);
 		goto err_out_args;
+	}
+
+	/* Record the parsed rbd options */
+
+	rbd_dev->mapping.read_only = rbd_opts.read_only;
+
+	rc = rbd_get_client(rbd_dev, ceph_opts);
+	if (rc < 0)
+		goto err_out_opts;
+	ceph_opts = NULL;	/* ceph_opts now owned by rbd_dev client */

 	/* pick the pool */
 	osdc = &rbd_dev->rbd_client->client->osdc;
@@ -3254,6 +3257,9 @@ err_out_client:
 	kfree(rbd_dev->header_name);
 	rbd_put_client(rbd_dev);
 	kfree(rbd_dev->image_id);
+err_out_opts:
+	if (ceph_opts)
+		ceph_destroy_options(ceph_opts);
 err_out_args:
 	kfree(rbd_dev->snap_name);
 	kfree(rbd_dev->image_name);
-- 
1.7.9.5

Re: [PATCH 1/2] rbd: move ceph_parse_options() call up

[Josh Durgin]

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

On 10/26/2012 03:55 PM, Alex Elder wrote:
> Move option parsing out of rbd_get_client() and into its caller.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   48 +++++++++++++++++++++++++++---------------------
>   1 file changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index fb44d4d..6e36c63 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -453,27 +453,11 @@ static int parse_rbd_opts_token(char *c, void
> *private)
>    * Get a ceph client with specific addr and configuration, if one does
>    * not exist create it.
>    */
> -static int rbd_get_client(struct rbd_device *rbd_dev, const char *mon_addr,
> -				size_t mon_addr_len, char *options)
> +static int rbd_get_client(struct rbd_device *rbd_dev,
> +				struct ceph_options *ceph_opts)
>   {
> -	struct rbd_options rbd_opts;
> -	struct ceph_options *ceph_opts;
>   	struct rbd_client *rbdc;
>
> -	/* Initialize all rbd options to the defaults */
> -
> -	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;
> -
> -	ceph_opts = ceph_parse_options(options, mon_addr,
> -					mon_addr + mon_addr_len,
> -					parse_rbd_opts_token, &rbd_opts);
> -	if (IS_ERR(ceph_opts))
> -		return PTR_ERR(ceph_opts);
> -
> -	/* Record the parsed rbd options */
> -
> -	rbd_dev->mapping.read_only = rbd_opts.read_only;
> -
>   	rbdc = rbd_client_find(ceph_opts);
>   	if (rbdc) {
>   		/* using an existing client */
> @@ -3132,9 +3116,11 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	struct rbd_device *rbd_dev = NULL;
>   	const char *mon_addrs = NULL;
>   	size_t mon_addrs_size = 0;
> +	char *snap_name;
> +	struct rbd_options rbd_opts;
> +	struct ceph_options *ceph_opts;
>   	struct ceph_osd_client *osdc;
>   	int rc = -ENOMEM;
> -	char *snap_name;
>
>   	if (!try_module_get(THIS_MODULE))
>   		return -ENODEV;
> @@ -3160,9 +3146,26 @@ static ssize_t rbd_add(struct bus_type *bus,
>   		goto err_out_mem;
>   	}
>
> -	rc = rbd_get_client(rbd_dev, mon_addrs, mon_addrs_size - 1, options);
> -	if (rc < 0)
> +	/* Initialize all rbd options to the defaults */
> +
> +	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;
> +
> +	ceph_opts = ceph_parse_options(options, mon_addrs,
> +					mon_addrs + mon_addrs_size - 1,
> +					parse_rbd_opts_token, &rbd_opts);
> +	if (IS_ERR(ceph_opts)) {
> +		rc = PTR_ERR(ceph_opts);
>   		goto err_out_args;
> +	}
> +
> +	/* Record the parsed rbd options */
> +
> +	rbd_dev->mapping.read_only = rbd_opts.read_only;
> +
> +	rc = rbd_get_client(rbd_dev, ceph_opts);
> +	if (rc < 0)
> +		goto err_out_opts;
> +	ceph_opts = NULL;	/* ceph_opts now owned by rbd_dev client */
>
>   	/* pick the pool */
>   	osdc = &rbd_dev->rbd_client->client->osdc;
> @@ -3254,6 +3257,9 @@ err_out_client:
>   	kfree(rbd_dev->header_name);
>   	rbd_put_client(rbd_dev);
>   	kfree(rbd_dev->image_id);
> +err_out_opts:
> +	if (ceph_opts)
> +		ceph_destroy_options(ceph_opts);
>   err_out_args:
>   	kfree(rbd_dev->snap_name);
>   	kfree(rbd_dev->image_name);
>

[PATCH 2/2] rbd: do all argument parsing in one place

[Alex Elder]

This patch makes rbd_add_parse_args() be the single place all
argument parsing occurs for an image map request:
    - Move the ceph_parse_options() call into that function
    - Use local variables rather than parameters to hold the list
      of monitor addresses supplied
    - Rather than returning it, pass the snapshot name (and its
      length) back via parameters
    - Have the function return a ceph_options structure pointer

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 6e36c63..4fe14ff 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2845,24 +2845,27 @@ static inline char *dup_token(const char **buf,
size_t *lenp)
  *
  * Note: rbd_dev is assumed to have been initially zero-filled.
  */
-static char *rbd_add_parse_args(struct rbd_device *rbd_dev,
-				const char *buf,
-				const char **mon_addrs,
-				size_t *mon_addrs_size,
-				char *options,
-				size_t options_size)
+static struct ceph_options *rbd_add_parse_args(struct rbd_device *rbd_dev,
+						const char *buf,
+						char *options,
+						size_t options_size,
+						char **snap_name,
+						size_t *snap_name_len)
 {
 	size_t len;
-	char *err_ptr = ERR_PTR(-EINVAL);
-	char *snap_name;
+	const char *mon_addrs;
+	size_t mon_addrs_size;
+	struct rbd_options rbd_opts;
+	struct ceph_options *ceph_opts;
+	struct ceph_options *err_ptr = ERR_PTR(-EINVAL);

 	/* The first four tokens are required */

 	len = next_token(&buf);
 	if (!len)
 		return err_ptr;
-	*mon_addrs_size = len + 1;
-	*mon_addrs = buf;
+	mon_addrs_size = len + 1;
+	mon_addrs = buf;

 	buf += len;

@@ -2890,14 +2893,27 @@ static char *rbd_add_parse_args(struct
rbd_device *rbd_dev,
 		buf = RBD_SNAP_HEAD_NAME; /* No snapshot supplied */
 		len = sizeof (RBD_SNAP_HEAD_NAME) - 1;
 	}
-	snap_name = kmalloc(len + 1, GFP_KERNEL);
-	if (!snap_name)
+	*snap_name = kmalloc(len + 1, GFP_KERNEL);
+	if (!*snap_name)
 		goto out_err;
-	memcpy(snap_name, buf, len);
-	*(snap_name + len) = '\0';
+	memcpy(*snap_name, buf, len);
+	*(*snap_name + len) = '\0';
+	*snap_name_len = len;
+	/* Initialize all rbd options to the defaults */

-	return snap_name;
+	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;

+	ceph_opts = ceph_parse_options(options, mon_addrs,
+					mon_addrs + mon_addrs_size - 1,
+					parse_rbd_opts_token, &rbd_opts);
+
+	/* Record the parsed rbd options */
+
+	if (!IS_ERR(ceph_opts)) {
+		rbd_dev->mapping.read_only = rbd_opts.read_only;
+	}
+
+	return ceph_opts;
 out_err:
 	kfree(rbd_dev->image_name);
 	rbd_dev->image_name = NULL;
@@ -3114,10 +3130,8 @@ static ssize_t rbd_add(struct bus_type *bus,
 {
 	char *options;
 	struct rbd_device *rbd_dev = NULL;
-	const char *mon_addrs = NULL;
-	size_t mon_addrs_size = 0;
 	char *snap_name;
-	struct rbd_options rbd_opts;
+	size_t snap_name_len = 0;
 	struct ceph_options *ceph_opts;
 	struct ceph_osd_client *osdc;
 	int rc = -ENOMEM;
@@ -3139,32 +3153,16 @@ static ssize_t rbd_add(struct bus_type *bus,
 	init_rwsem(&rbd_dev->header_rwsem);

 	/* parse add command */
-	snap_name = rbd_add_parse_args(rbd_dev, buf,
-				&mon_addrs, &mon_addrs_size, options, count);
-	if (IS_ERR(snap_name)) {
-		rc = PTR_ERR(snap_name);
-		goto err_out_mem;
-	}
-
-	/* Initialize all rbd options to the defaults */
-
-	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;
-
-	ceph_opts = ceph_parse_options(options, mon_addrs,
-					mon_addrs + mon_addrs_size - 1,
-					parse_rbd_opts_token, &rbd_opts);
+	ceph_opts = rbd_add_parse_args(rbd_dev, buf, options, count,
+				&snap_name, &snap_name_len);
 	if (IS_ERR(ceph_opts)) {
 		rc = PTR_ERR(ceph_opts);
-		goto err_out_args;
+		goto err_out_mem;
 	}

-	/* Record the parsed rbd options */
-
-	rbd_dev->mapping.read_only = rbd_opts.read_only;
-
 	rc = rbd_get_client(rbd_dev, ceph_opts);
 	if (rc < 0)
-		goto err_out_opts;
+		goto err_out_args;
 	ceph_opts = NULL;	/* ceph_opts now owned by rbd_dev client */

 	/* pick the pool */
@@ -3257,10 +3255,9 @@ err_out_client:
 	kfree(rbd_dev->header_name);
 	rbd_put_client(rbd_dev);
 	kfree(rbd_dev->image_id);
-err_out_opts:
+err_out_args:
 	if (ceph_opts)
 		ceph_destroy_options(ceph_opts);
-err_out_args:
 	kfree(rbd_dev->snap_name);
 	kfree(rbd_dev->image_name);
 	kfree(rbd_dev->pool_name);
-- 
1.7.9.5

Re: [PATCH 2/2] rbd: do all argument parsing in one place

[Josh Durgin]

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

On 10/26/2012 03:55 PM, Alex Elder wrote:
> This patch makes rbd_add_parse_args() be the single place all
> argument parsing occurs for an image map request:
>      - Move the ceph_parse_options() call into that function
>      - Use local variables rather than parameters to hold the list
>        of monitor addresses supplied
>      - Rather than returning it, pass the snapshot name (and its
>        length) back via parameters
>      - Have the function return a ceph_options structure pointer
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   77
> +++++++++++++++++++++++++--------------------------
>   1 file changed, 37 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 6e36c63..4fe14ff 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2845,24 +2845,27 @@ static inline char *dup_token(const char **buf,
> size_t *lenp)
>    *
>    * Note: rbd_dev is assumed to have been initially zero-filled.
>    */
> -static char *rbd_add_parse_args(struct rbd_device *rbd_dev,
> -				const char *buf,
> -				const char **mon_addrs,
> -				size_t *mon_addrs_size,
> -				char *options,
> -				size_t options_size)
> +static struct ceph_options *rbd_add_parse_args(struct rbd_device *rbd_dev,
> +						const char *buf,
> +						char *options,
> +						size_t options_size,
> +						char **snap_name,
> +						size_t *snap_name_len)
>   {
>   	size_t len;
> -	char *err_ptr = ERR_PTR(-EINVAL);
> -	char *snap_name;
> +	const char *mon_addrs;
> +	size_t mon_addrs_size;
> +	struct rbd_options rbd_opts;
> +	struct ceph_options *ceph_opts;
> +	struct ceph_options *err_ptr = ERR_PTR(-EINVAL);
>
>   	/* The first four tokens are required */
>
>   	len = next_token(&buf);
>   	if (!len)
>   		return err_ptr;
> -	*mon_addrs_size = len + 1;
> -	*mon_addrs = buf;
> +	mon_addrs_size = len + 1;
> +	mon_addrs = buf;
>
>   	buf += len;
>
> @@ -2890,14 +2893,27 @@ static char *rbd_add_parse_args(struct
> rbd_device *rbd_dev,
>   		buf = RBD_SNAP_HEAD_NAME; /* No snapshot supplied */
>   		len = sizeof (RBD_SNAP_HEAD_NAME) - 1;
>   	}
> -	snap_name = kmalloc(len + 1, GFP_KERNEL);
> -	if (!snap_name)
> +	*snap_name = kmalloc(len + 1, GFP_KERNEL);
> +	if (!*snap_name)
>   		goto out_err;
> -	memcpy(snap_name, buf, len);
> -	*(snap_name + len) = '\0';
> +	memcpy(*snap_name, buf, len);
> +	*(*snap_name + len) = '\0';
> +	*snap_name_len = len;
> +	/* Initialize all rbd options to the defaults */
>
> -	return snap_name;
> +	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;
>
> +	ceph_opts = ceph_parse_options(options, mon_addrs,
> +					mon_addrs + mon_addrs_size - 1,
> +					parse_rbd_opts_token, &rbd_opts);
> +
> +	/* Record the parsed rbd options */
> +
> +	if (!IS_ERR(ceph_opts)) {
> +		rbd_dev->mapping.read_only = rbd_opts.read_only;
> +	}
> +
> +	return ceph_opts;
>   out_err:
>   	kfree(rbd_dev->image_name);
>   	rbd_dev->image_name = NULL;
> @@ -3114,10 +3130,8 @@ static ssize_t rbd_add(struct bus_type *bus,
>   {
>   	char *options;
>   	struct rbd_device *rbd_dev = NULL;
> -	const char *mon_addrs = NULL;
> -	size_t mon_addrs_size = 0;
>   	char *snap_name;
> -	struct rbd_options rbd_opts;
> +	size_t snap_name_len = 0;
>   	struct ceph_options *ceph_opts;
>   	struct ceph_osd_client *osdc;
>   	int rc = -ENOMEM;
> @@ -3139,32 +3153,16 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	init_rwsem(&rbd_dev->header_rwsem);
>
>   	/* parse add command */
> -	snap_name = rbd_add_parse_args(rbd_dev, buf,
> -				&mon_addrs, &mon_addrs_size, options, count);
> -	if (IS_ERR(snap_name)) {
> -		rc = PTR_ERR(snap_name);
> -		goto err_out_mem;
> -	}
> -
> -	/* Initialize all rbd options to the defaults */
> -
> -	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;
> -
> -	ceph_opts = ceph_parse_options(options, mon_addrs,
> -					mon_addrs + mon_addrs_size - 1,
> -					parse_rbd_opts_token, &rbd_opts);
> +	ceph_opts = rbd_add_parse_args(rbd_dev, buf, options, count,
> +				&snap_name, &snap_name_len);
>   	if (IS_ERR(ceph_opts)) {
>   		rc = PTR_ERR(ceph_opts);
> -		goto err_out_args;
> +		goto err_out_mem;
>   	}
>
> -	/* Record the parsed rbd options */
> -
> -	rbd_dev->mapping.read_only = rbd_opts.read_only;
> -
>   	rc = rbd_get_client(rbd_dev, ceph_opts);
>   	if (rc < 0)
> -		goto err_out_opts;
> +		goto err_out_args;
>   	ceph_opts = NULL;	/* ceph_opts now owned by rbd_dev client */
>
>   	/* pick the pool */
> @@ -3257,10 +3255,9 @@ err_out_client:
>   	kfree(rbd_dev->header_name);
>   	rbd_put_client(rbd_dev);
>   	kfree(rbd_dev->image_id);
> -err_out_opts:
> +err_out_args:
>   	if (ceph_opts)
>   		ceph_destroy_options(ceph_opts);
> -err_out_args:
>   	kfree(rbd_dev->snap_name);
>   	kfree(rbd_dev->image_name);
>   	kfree(rbd_dev->pool_name);
>

[PATCH 0/8] rbd: have rbd_add_parse_args() only parse args

[Alex Elder]

rbd_add_parse_args() parses options written to the "add" entry in
/sys/bus/rbd/, but it does a mix of other things as well.  This
series rearranges things such that the only thing that function
does is parse arguments, and encode the information found into
some data structures that are used later for configuration and
setting up new mapped rbd images.

					-Alex

[PATCH 1/8] rbd: get rid of snap_name_len
[PATCH 2/8] rbd: remove options args from rbd_add_parse_args()
[PATCH 3/8] rbd: remove snap_name arg from rbd_add_parse_args()
[PATCH 4/8] rbd: pass and populate rbd_options structure
[PATCH 5/8] rbd: have rbd_add_parse_args() return error
[PATCH 6/8] rbd: define image specification structure
[PATCH 7/8] rbd: add reference counting to rbd_spec
[PATCH 8/8] rbd: fill rbd_spec in rbd_add_parse_args()

[PATCH 1/8] rbd: get rid of snap_name_len

[Alex Elder]

The value returned in the "snap_name_len" argument to
rbd_add_parse_args() is never actually used, so get rid of it.

The snap_name_len recorded in *rbd_dev_v2_snap_name() is not
useful either, so get rid of that too.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 4fe14ff..cae7423 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2408,7 +2408,6 @@ static char *rbd_dev_v2_snap_name(struct
rbd_device *rbd_dev, u32 which)
 	int ret;
 	void *p;
 	void *end;
-	size_t snap_name_len;
 	char *snap_name;

 	size = sizeof (__le32) + RBD_MAX_SNAP_NAME_LEN;
@@ -2428,9 +2427,7 @@ static char *rbd_dev_v2_snap_name(struct
rbd_device *rbd_dev, u32 which)

 	p = reply_buf;
 	end = (char *) reply_buf + size;
-	snap_name_len = 0;
-	snap_name = ceph_extract_encoded_string(&p, end, &snap_name_len,
-				GFP_KERNEL);
+	snap_name = ceph_extract_encoded_string(&p, end, NULL, GFP_KERNEL);
 	if (IS_ERR(snap_name)) {
 		ret = PTR_ERR(snap_name);
 		goto out;
@@ -2849,8 +2846,7 @@ static struct ceph_options
*rbd_add_parse_args(struct rbd_device *rbd_dev,
 						const char *buf,
 						char *options,
 						size_t options_size,
-						char **snap_name,
-						size_t *snap_name_len)
+						char **snap_name)
 {
 	size_t len;
 	const char *mon_addrs;
@@ -2898,7 +2894,7 @@ static struct ceph_options
*rbd_add_parse_args(struct rbd_device *rbd_dev,
 		goto out_err;
 	memcpy(*snap_name, buf, len);
 	*(*snap_name + len) = '\0';
-	*snap_name_len = len;
+
 	/* Initialize all rbd options to the defaults */

 	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;
@@ -3131,7 +3127,6 @@ static ssize_t rbd_add(struct bus_type *bus,
 	char *options;
 	struct rbd_device *rbd_dev = NULL;
 	char *snap_name;
-	size_t snap_name_len = 0;
 	struct ceph_options *ceph_opts;
 	struct ceph_osd_client *osdc;
 	int rc = -ENOMEM;
@@ -3154,7 +3149,7 @@ static ssize_t rbd_add(struct bus_type *bus,

 	/* parse add command */
 	ceph_opts = rbd_add_parse_args(rbd_dev, buf, options, count,
-				&snap_name, &snap_name_len);
+				&snap_name);
 	if (IS_ERR(ceph_opts)) {
 		rc = PTR_ERR(ceph_opts);
 		goto err_out_mem;
-- 
1.7.9.5

Re: [PATCH 1/8] rbd: get rid of snap_name_len

[Josh Durgin]

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

On 10/26/2012 04:00 PM, Alex Elder wrote:
> The value returned in the "snap_name_len" argument to
> rbd_add_parse_args() is never actually used, so get rid of it.
>
> The snap_name_len recorded in *rbd_dev_v2_snap_name() is not
> useful either, so get rid of that too.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   13 ++++---------
>   1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 4fe14ff..cae7423 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2408,7 +2408,6 @@ static char *rbd_dev_v2_snap_name(struct
> rbd_device *rbd_dev, u32 which)
>   	int ret;
>   	void *p;
>   	void *end;
> -	size_t snap_name_len;
>   	char *snap_name;
>
>   	size = sizeof (__le32) + RBD_MAX_SNAP_NAME_LEN;
> @@ -2428,9 +2427,7 @@ static char *rbd_dev_v2_snap_name(struct
> rbd_device *rbd_dev, u32 which)
>
>   	p = reply_buf;
>   	end = (char *) reply_buf + size;
> -	snap_name_len = 0;
> -	snap_name = ceph_extract_encoded_string(&p, end, &snap_name_len,
> -				GFP_KERNEL);
> +	snap_name = ceph_extract_encoded_string(&p, end, NULL, GFP_KERNEL);
>   	if (IS_ERR(snap_name)) {
>   		ret = PTR_ERR(snap_name);
>   		goto out;
> @@ -2849,8 +2846,7 @@ static struct ceph_options
> *rbd_add_parse_args(struct rbd_device *rbd_dev,
>   						const char *buf,
>   						char *options,
>   						size_t options_size,
> -						char **snap_name,
> -						size_t *snap_name_len)
> +						char **snap_name)
>   {
>   	size_t len;
>   	const char *mon_addrs;
> @@ -2898,7 +2894,7 @@ static struct ceph_options
> *rbd_add_parse_args(struct rbd_device *rbd_dev,
>   		goto out_err;
>   	memcpy(*snap_name, buf, len);
>   	*(*snap_name + len) = '\0';
> -	*snap_name_len = len;
> +
>   	/* Initialize all rbd options to the defaults */
>
>   	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;
> @@ -3131,7 +3127,6 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	char *options;
>   	struct rbd_device *rbd_dev = NULL;
>   	char *snap_name;
> -	size_t snap_name_len = 0;
>   	struct ceph_options *ceph_opts;
>   	struct ceph_osd_client *osdc;
>   	int rc = -ENOMEM;
> @@ -3154,7 +3149,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>
>   	/* parse add command */
>   	ceph_opts = rbd_add_parse_args(rbd_dev, buf, options, count,
> -				&snap_name, &snap_name_len);
> +				&snap_name);
>   	if (IS_ERR(ceph_opts)) {
>   		rc = PTR_ERR(ceph_opts);
>   		goto err_out_mem;
>

[PATCH 2/8] rbd: remove options args from rbd_add_parse_args()

[Alex Elder]

They "options" argument to rbd_add_parse_args() (and it's partner
options_size) is now only needed within the function, so there's no
need to have the caller allocate and pass the options buffer.  Just
allocate the options buffer within the function using dup_token().

Also distinguish between failures due to failed memory allocation
and failing because a required argument was missing.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index cae7423..f900f3b 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2844,54 +2844,58 @@ static inline char *dup_token(const char **buf,
size_t *lenp)
  */
 static struct ceph_options *rbd_add_parse_args(struct rbd_device *rbd_dev,
 						const char *buf,
-						char *options,
-						size_t options_size,
 						char **snap_name)
 {
 	size_t len;
 	const char *mon_addrs;
 	size_t mon_addrs_size;
+	char *options;
+	struct ceph_options *err_ptr = ERR_PTR(-EINVAL);
 	struct rbd_options rbd_opts;
 	struct ceph_options *ceph_opts;
-	struct ceph_options *err_ptr = ERR_PTR(-EINVAL);

 	/* The first four tokens are required */

 	len = next_token(&buf);
 	if (!len)
-		return err_ptr;
-	mon_addrs_size = len + 1;
+		return err_ptr;	/* Missing monitor address(es) */
 	mon_addrs = buf;
-
+	mon_addrs_size = len + 1;
 	buf += len;

-	len = copy_token(&buf, options, options_size);
-	if (!len || len >= options_size)
-		return err_ptr;
+	options = dup_token(&buf, NULL);
+	if (!options)
+		goto out_mem;
+	if (!*options)
+		goto out_err;	/* Missing options */

-	err_ptr = ERR_PTR(-ENOMEM);
 	rbd_dev->pool_name = dup_token(&buf, NULL);
 	if (!rbd_dev->pool_name)
-		goto out_err;
+		goto out_mem;
+	if (!*rbd_dev->pool_name)
+		goto out_err;	/* Missing pool name */

 	rbd_dev->image_name = dup_token(&buf, &rbd_dev->image_name_len);
 	if (!rbd_dev->image_name)
-		goto out_err;
-
-	/* Snapshot name is optional; default is to use "head" */
+		goto out_mem;
+	if (!*rbd_dev->image_name)
+		goto out_err;	/* Missing image name */

+	/*
+	 * Snapshot name is optional; default is to use "-"
+	 * (indicating the head/no snapshot).
+	 */
 	len = next_token(&buf);
-	if (len > RBD_MAX_SNAP_NAME_LEN) {
-		err_ptr = ERR_PTR(-ENAMETOOLONG);
-		goto out_err;
-	}
 	if (!len) {
 		buf = RBD_SNAP_HEAD_NAME; /* No snapshot supplied */
 		len = sizeof (RBD_SNAP_HEAD_NAME) - 1;
+	} else if (len > RBD_MAX_SNAP_NAME_LEN) {
+		err_ptr = ERR_PTR(-ENAMETOOLONG);
+		goto out_err;
 	}
 	*snap_name = kmalloc(len + 1, GFP_KERNEL);
 	if (!*snap_name)
-		goto out_err;
+		goto out_mem;
 	memcpy(*snap_name, buf, len);
 	*(*snap_name + len) = '\0';

@@ -2902,20 +2906,23 @@ static struct ceph_options
*rbd_add_parse_args(struct rbd_device *rbd_dev,
 	ceph_opts = ceph_parse_options(options, mon_addrs,
 					mon_addrs + mon_addrs_size - 1,
 					parse_rbd_opts_token, &rbd_opts);
+	kfree(options);

 	/* Record the parsed rbd options */

-	if (!IS_ERR(ceph_opts)) {
+	if (!IS_ERR(ceph_opts))
 		rbd_dev->mapping.read_only = rbd_opts.read_only;
-	}

 	return ceph_opts;
+out_mem:
+	err_ptr = ERR_PTR(-ENOMEM);
 out_err:
 	kfree(rbd_dev->image_name);
 	rbd_dev->image_name = NULL;
 	rbd_dev->image_name_len = 0;
 	kfree(rbd_dev->pool_name);
 	rbd_dev->pool_name = NULL;
+	kfree(options);

 	return err_ptr;
 }
@@ -3124,7 +3131,6 @@ static ssize_t rbd_add(struct bus_type *bus,
 		       const char *buf,
 		       size_t count)
 {
-	char *options;
 	struct rbd_device *rbd_dev = NULL;
 	char *snap_name;
 	struct ceph_options *ceph_opts;
@@ -3134,9 +3140,6 @@ static ssize_t rbd_add(struct bus_type *bus,
 	if (!try_module_get(THIS_MODULE))
 		return -ENODEV;

-	options = kmalloc(count, GFP_KERNEL);
-	if (!options)
-		goto err_out_mem;
 	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
 	if (!rbd_dev)
 		goto err_out_mem;
@@ -3148,8 +3151,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	init_rwsem(&rbd_dev->header_rwsem);

 	/* parse add command */
-	ceph_opts = rbd_add_parse_args(rbd_dev, buf, options, count,
-				&snap_name);
+	ceph_opts = rbd_add_parse_args(rbd_dev, buf, &snap_name);
 	if (IS_ERR(ceph_opts)) {
 		rc = PTR_ERR(ceph_opts);
 		goto err_out_mem;
@@ -3233,7 +3235,6 @@ err_out_bus:
 	/* this will also clean up rest of rbd_dev stuff */

 	rbd_bus_del_dev(rbd_dev);
-	kfree(options);
 	return rc;

 err_out_disk:
@@ -3258,7 +3259,6 @@ err_out_args:
 	kfree(rbd_dev->pool_name);
 err_out_mem:
 	kfree(rbd_dev);
-	kfree(options);

 	dout("Error adding device %s\n", buf);
 	module_put(THIS_MODULE);
-- 
1.7.9.5

Re: [PATCH 2/8] rbd: remove options args from rbd_add_parse_args()

[Josh Durgin]

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

On 10/26/2012 04:00 PM, Alex Elder wrote:
> They "options" argument to rbd_add_parse_args() (and it's partner
> options_size) is now only needed within the function, so there's no
> need to have the caller allocate and pass the options buffer.  Just
> allocate the options buffer within the function using dup_token().
>
> Also distinguish between failures due to failed memory allocation
> and failing because a required argument was missing.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   58
> +++++++++++++++++++++++++--------------------------
>   1 file changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index cae7423..f900f3b 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2844,54 +2844,58 @@ static inline char *dup_token(const char **buf,
> size_t *lenp)
>    */
>   static struct ceph_options *rbd_add_parse_args(struct rbd_device *rbd_dev,
>   						const char *buf,
> -						char *options,
> -						size_t options_size,
>   						char **snap_name)
>   {
>   	size_t len;
>   	const char *mon_addrs;
>   	size_t mon_addrs_size;
> +	char *options;
> +	struct ceph_options *err_ptr = ERR_PTR(-EINVAL);
>   	struct rbd_options rbd_opts;
>   	struct ceph_options *ceph_opts;
> -	struct ceph_options *err_ptr = ERR_PTR(-EINVAL);
>
>   	/* The first four tokens are required */
>
>   	len = next_token(&buf);
>   	if (!len)
> -		return err_ptr;
> -	mon_addrs_size = len + 1;
> +		return err_ptr;	/* Missing monitor address(es) */
>   	mon_addrs = buf;
> -
> +	mon_addrs_size = len + 1;
>   	buf += len;
>
> -	len = copy_token(&buf, options, options_size);
> -	if (!len || len >= options_size)
> -		return err_ptr;
> +	options = dup_token(&buf, NULL);
> +	if (!options)
> +		goto out_mem;
> +	if (!*options)
> +		goto out_err;	/* Missing options */
>
> -	err_ptr = ERR_PTR(-ENOMEM);
>   	rbd_dev->pool_name = dup_token(&buf, NULL);
>   	if (!rbd_dev->pool_name)
> -		goto out_err;
> +		goto out_mem;
> +	if (!*rbd_dev->pool_name)
> +		goto out_err;	/* Missing pool name */
>
>   	rbd_dev->image_name = dup_token(&buf, &rbd_dev->image_name_len);
>   	if (!rbd_dev->image_name)
> -		goto out_err;
> -
> -	/* Snapshot name is optional; default is to use "head" */
> +		goto out_mem;
> +	if (!*rbd_dev->image_name)
> +		goto out_err;	/* Missing image name */
>
> +	/*
> +	 * Snapshot name is optional; default is to use "-"
> +	 * (indicating the head/no snapshot).
> +	 */
>   	len = next_token(&buf);
> -	if (len > RBD_MAX_SNAP_NAME_LEN) {
> -		err_ptr = ERR_PTR(-ENAMETOOLONG);
> -		goto out_err;
> -	}
>   	if (!len) {
>   		buf = RBD_SNAP_HEAD_NAME; /* No snapshot supplied */
>   		len = sizeof (RBD_SNAP_HEAD_NAME) - 1;
> +	} else if (len > RBD_MAX_SNAP_NAME_LEN) {
> +		err_ptr = ERR_PTR(-ENAMETOOLONG);
> +		goto out_err;
>   	}
>   	*snap_name = kmalloc(len + 1, GFP_KERNEL);
>   	if (!*snap_name)
> -		goto out_err;
> +		goto out_mem;
>   	memcpy(*snap_name, buf, len);
>   	*(*snap_name + len) = '\0';
>
> @@ -2902,20 +2906,23 @@ static struct ceph_options
> *rbd_add_parse_args(struct rbd_device *rbd_dev,
>   	ceph_opts = ceph_parse_options(options, mon_addrs,
>   					mon_addrs + mon_addrs_size - 1,
>   					parse_rbd_opts_token, &rbd_opts);
> +	kfree(options);
>
>   	/* Record the parsed rbd options */
>
> -	if (!IS_ERR(ceph_opts)) {
> +	if (!IS_ERR(ceph_opts))
>   		rbd_dev->mapping.read_only = rbd_opts.read_only;
> -	}
>
>   	return ceph_opts;
> +out_mem:
> +	err_ptr = ERR_PTR(-ENOMEM);
>   out_err:
>   	kfree(rbd_dev->image_name);
>   	rbd_dev->image_name = NULL;
>   	rbd_dev->image_name_len = 0;
>   	kfree(rbd_dev->pool_name);
>   	rbd_dev->pool_name = NULL;
> +	kfree(options);
>
>   	return err_ptr;
>   }
> @@ -3124,7 +3131,6 @@ static ssize_t rbd_add(struct bus_type *bus,
>   		       const char *buf,
>   		       size_t count)
>   {
> -	char *options;
>   	struct rbd_device *rbd_dev = NULL;
>   	char *snap_name;
>   	struct ceph_options *ceph_opts;
> @@ -3134,9 +3140,6 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	if (!try_module_get(THIS_MODULE))
>   		return -ENODEV;
>
> -	options = kmalloc(count, GFP_KERNEL);
> -	if (!options)
> -		goto err_out_mem;
>   	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
>   	if (!rbd_dev)
>   		goto err_out_mem;
> @@ -3148,8 +3151,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	init_rwsem(&rbd_dev->header_rwsem);
>
>   	/* parse add command */
> -	ceph_opts = rbd_add_parse_args(rbd_dev, buf, options, count,
> -				&snap_name);
> +	ceph_opts = rbd_add_parse_args(rbd_dev, buf, &snap_name);
>   	if (IS_ERR(ceph_opts)) {
>   		rc = PTR_ERR(ceph_opts);
>   		goto err_out_mem;
> @@ -3233,7 +3235,6 @@ err_out_bus:
>   	/* this will also clean up rest of rbd_dev stuff */
>
>   	rbd_bus_del_dev(rbd_dev);
> -	kfree(options);
>   	return rc;
>
>   err_out_disk:
> @@ -3258,7 +3259,6 @@ err_out_args:
>   	kfree(rbd_dev->pool_name);
>   err_out_mem:
>   	kfree(rbd_dev);
> -	kfree(options);
>
>   	dout("Error adding device %s\n", buf);
>   	module_put(THIS_MODULE);
>

[PATCH 3/8] rbd: remove snap_name arg from rbd_add_parse_args()

[Alex Elder]

The snapshot name returned by rbd_add_parse_args() just gets saved
in the rbd_dev eventually.  So just do that inside that function and
do away with the snap_name argument, both in rbd_add_parse_args()
and rbd_dev_set_mapping().

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index f900f3b..948a084 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -665,23 +665,22 @@ static int snap_by_name(struct rbd_device
*rbd_dev, const char *snap_name)
 	return -ENOENT;
 }

-static int rbd_dev_set_mapping(struct rbd_device *rbd_dev, char *snap_name)
+static int rbd_dev_set_mapping(struct rbd_device *rbd_dev)
 {
 	int ret;

-	if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME,
+	if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
 		    sizeof (RBD_SNAP_HEAD_NAME))) {
 		rbd_dev->snap_id = CEPH_NOSNAP;
 		rbd_dev->mapping.size = rbd_dev->header.image_size;
 		rbd_dev->mapping.features = rbd_dev->header.features;
 		ret = 0;
 	} else {
-		ret = snap_by_name(rbd_dev, snap_name);
+		ret = snap_by_name(rbd_dev, rbd_dev->snap_name);
 		if (ret < 0)
 			goto done;
 		rbd_dev->mapping.read_only = true;
 	}
-	rbd_dev->snap_name = snap_name;
 	rbd_dev->exists = true;
 done:
 	return ret;
@@ -2843,8 +2842,7 @@ static inline char *dup_token(const char **buf,
size_t *lenp)
  * Note: rbd_dev is assumed to have been initially zero-filled.
  */
 static struct ceph_options *rbd_add_parse_args(struct rbd_device *rbd_dev,
-						const char *buf,
-						char **snap_name)
+						const char *buf)
 {
 	size_t len;
 	const char *mon_addrs;
@@ -2893,11 +2891,11 @@ static struct ceph_options
*rbd_add_parse_args(struct rbd_device *rbd_dev,
 		err_ptr = ERR_PTR(-ENAMETOOLONG);
 		goto out_err;
 	}
-	*snap_name = kmalloc(len + 1, GFP_KERNEL);
-	if (!*snap_name)
+	rbd_dev->snap_name = kmalloc(len + 1, GFP_KERNEL);
+	if (!rbd_dev->snap_name)
 		goto out_mem;
-	memcpy(*snap_name, buf, len);
-	*(*snap_name + len) = '\0';
+	memcpy(rbd_dev->snap_name, buf, len);
+	*(rbd_dev->snap_name + len) = '\0';

 	/* Initialize all rbd options to the defaults */

@@ -3132,7 +3130,6 @@ static ssize_t rbd_add(struct bus_type *bus,
 		       size_t count)
 {
 	struct rbd_device *rbd_dev = NULL;
-	char *snap_name;
 	struct ceph_options *ceph_opts;
 	struct ceph_osd_client *osdc;
 	int rc = -ENOMEM;
@@ -3151,7 +3148,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	init_rwsem(&rbd_dev->header_rwsem);

 	/* parse add command */
-	ceph_opts = rbd_add_parse_args(rbd_dev, buf, &snap_name);
+	ceph_opts = rbd_add_parse_args(rbd_dev, buf);
 	if (IS_ERR(ceph_opts)) {
 		rc = PTR_ERR(ceph_opts);
 		goto err_out_mem;
@@ -3178,7 +3175,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	if (rc)
 		goto err_out_probe;

-	rc = rbd_dev_set_mapping(rbd_dev, snap_name);
+	rc = rbd_dev_set_mapping(rbd_dev);
 	if (rc)
 		goto err_out_snaps;

-- 
1.7.9.5

Re: [PATCH 3/8] rbd: remove snap_name arg from rbd_add_parse_args()

[Josh Durgin]

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

On 10/26/2012 04:01 PM, Alex Elder wrote:
> The snapshot name returned by rbd_add_parse_args() just gets saved
> in the rbd_dev eventually.  So just do that inside that function and
> do away with the snap_name argument, both in rbd_add_parse_args()
> and rbd_dev_set_mapping().
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   23 ++++++++++-------------
>   1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index f900f3b..948a084 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -665,23 +665,22 @@ static int snap_by_name(struct rbd_device
> *rbd_dev, const char *snap_name)
>   	return -ENOENT;
>   }
>
> -static int rbd_dev_set_mapping(struct rbd_device *rbd_dev, char *snap_name)
> +static int rbd_dev_set_mapping(struct rbd_device *rbd_dev)
>   {
>   	int ret;
>
> -	if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME,
> +	if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>   		    sizeof (RBD_SNAP_HEAD_NAME))) {
>   		rbd_dev->snap_id = CEPH_NOSNAP;
>   		rbd_dev->mapping.size = rbd_dev->header.image_size;
>   		rbd_dev->mapping.features = rbd_dev->header.features;
>   		ret = 0;
>   	} else {
> -		ret = snap_by_name(rbd_dev, snap_name);
> +		ret = snap_by_name(rbd_dev, rbd_dev->snap_name);
>   		if (ret < 0)
>   			goto done;
>   		rbd_dev->mapping.read_only = true;
>   	}
> -	rbd_dev->snap_name = snap_name;
>   	rbd_dev->exists = true;
>   done:
>   	return ret;
> @@ -2843,8 +2842,7 @@ static inline char *dup_token(const char **buf,
> size_t *lenp)
>    * Note: rbd_dev is assumed to have been initially zero-filled.
>    */
>   static struct ceph_options *rbd_add_parse_args(struct rbd_device *rbd_dev,
> -						const char *buf,
> -						char **snap_name)
> +						const char *buf)
>   {
>   	size_t len;
>   	const char *mon_addrs;
> @@ -2893,11 +2891,11 @@ static struct ceph_options
> *rbd_add_parse_args(struct rbd_device *rbd_dev,
>   		err_ptr = ERR_PTR(-ENAMETOOLONG);
>   		goto out_err;
>   	}
> -	*snap_name = kmalloc(len + 1, GFP_KERNEL);
> -	if (!*snap_name)
> +	rbd_dev->snap_name = kmalloc(len + 1, GFP_KERNEL);
> +	if (!rbd_dev->snap_name)
>   		goto out_mem;
> -	memcpy(*snap_name, buf, len);
> -	*(*snap_name + len) = '\0';
> +	memcpy(rbd_dev->snap_name, buf, len);
> +	*(rbd_dev->snap_name + len) = '\0';
>
>   	/* Initialize all rbd options to the defaults */
>
> @@ -3132,7 +3130,6 @@ static ssize_t rbd_add(struct bus_type *bus,
>   		       size_t count)
>   {
>   	struct rbd_device *rbd_dev = NULL;
> -	char *snap_name;
>   	struct ceph_options *ceph_opts;
>   	struct ceph_osd_client *osdc;
>   	int rc = -ENOMEM;
> @@ -3151,7 +3148,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	init_rwsem(&rbd_dev->header_rwsem);
>
>   	/* parse add command */
> -	ceph_opts = rbd_add_parse_args(rbd_dev, buf, &snap_name);
> +	ceph_opts = rbd_add_parse_args(rbd_dev, buf);
>   	if (IS_ERR(ceph_opts)) {
>   		rc = PTR_ERR(ceph_opts);
>   		goto err_out_mem;
> @@ -3178,7 +3175,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	if (rc)
>   		goto err_out_probe;
>
> -	rc = rbd_dev_set_mapping(rbd_dev, snap_name);
> +	rc = rbd_dev_set_mapping(rbd_dev);
>   	if (rc)
>   		goto err_out_snaps;
>

[PATCH 4/8] rbd: pass and populate rbd_options structure

[Alex Elder]

Have the caller pass the address of an rbd_options structure to
rbd_add_parse_args(), to be initialized with the information
gleaned as a result of the parse.

I know, this is another near-reversal of a recent change...

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 948a084..392dded 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2842,15 +2842,16 @@ static inline char *dup_token(const char **buf,
size_t *lenp)
  * Note: rbd_dev is assumed to have been initially zero-filled.
  */
 static struct ceph_options *rbd_add_parse_args(struct rbd_device *rbd_dev,
-						const char *buf)
+						const char *buf,
+						struct rbd_options **opts)
 {
 	size_t len;
 	const char *mon_addrs;
 	size_t mon_addrs_size;
 	char *options;
 	struct ceph_options *err_ptr = ERR_PTR(-EINVAL);
-	struct rbd_options rbd_opts;
 	struct ceph_options *ceph_opts;
+	struct rbd_options *rbd_opts = NULL;

 	/* The first four tokens are required */

@@ -2899,17 +2900,17 @@ static struct ceph_options
*rbd_add_parse_args(struct rbd_device *rbd_dev,

 	/* Initialize all rbd options to the defaults */

-	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;
+	rbd_opts = kzalloc(sizeof (*rbd_opts), GFP_KERNEL);
+	if (!rbd_opts)
+		goto out_mem;
+
+	rbd_opts->read_only = RBD_READ_ONLY_DEFAULT;

 	ceph_opts = ceph_parse_options(options, mon_addrs,
 					mon_addrs + mon_addrs_size - 1,
-					parse_rbd_opts_token, &rbd_opts);
+					parse_rbd_opts_token, rbd_opts);
 	kfree(options);
-
-	/* Record the parsed rbd options */
-
-	if (!IS_ERR(ceph_opts))
-		rbd_dev->mapping.read_only = rbd_opts.read_only;
+	*opts = rbd_opts;

 	return ceph_opts;
 out_mem:
@@ -3131,6 +3132,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 {
 	struct rbd_device *rbd_dev = NULL;
 	struct ceph_options *ceph_opts;
+	struct rbd_options *rbd_opts = NULL;
 	struct ceph_osd_client *osdc;
 	int rc = -ENOMEM;

@@ -3139,7 +3141,7 @@ static ssize_t rbd_add(struct bus_type *bus,

 	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
 	if (!rbd_dev)
-		goto err_out_mem;
+		return -ENOMEM;

 	/* static rbd_device initialization */
 	spin_lock_init(&rbd_dev->lock);
@@ -3148,11 +3150,12 @@ static ssize_t rbd_add(struct bus_type *bus,
 	init_rwsem(&rbd_dev->header_rwsem);

 	/* parse add command */
-	ceph_opts = rbd_add_parse_args(rbd_dev, buf);
+	ceph_opts = rbd_add_parse_args(rbd_dev, buf, &rbd_opts);
 	if (IS_ERR(ceph_opts)) {
 		rc = PTR_ERR(ceph_opts);
 		goto err_out_mem;
 	}
+	rbd_dev->mapping.read_only = rbd_opts->read_only;

 	rc = rbd_get_client(rbd_dev, ceph_opts);
 	if (rc < 0)
@@ -3219,6 +3222,8 @@ static ssize_t rbd_add(struct bus_type *bus,
 	if (rc)
 		goto err_out_bus;

+	kfree(rbd_opts);
+
 	/* Everything's ready.  Announce the disk to the world. */

 	add_disk(rbd_dev->disk);
@@ -3232,6 +3237,8 @@ err_out_bus:
 	/* this will also clean up rest of rbd_dev stuff */

 	rbd_bus_del_dev(rbd_dev);
+	kfree(rbd_opts);
+
 	return rc;

 err_out_disk:
@@ -3254,6 +3261,7 @@ err_out_args:
 	kfree(rbd_dev->snap_name);
 	kfree(rbd_dev->image_name);
 	kfree(rbd_dev->pool_name);
+	kfree(rbd_opts);
 err_out_mem:
 	kfree(rbd_dev);

-- 
1.7.9.5

Re: [PATCH 4/8] rbd: pass and populate rbd_options structure

[Josh Durgin]

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

On 10/26/2012 04:02 PM, Alex Elder wrote:
> Have the caller pass the address of an rbd_options structure to
> rbd_add_parse_args(), to be initialized with the information
> gleaned as a result of the parse.
>
> I know, this is another near-reversal of a recent change...
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   30 +++++++++++++++++++-----------
>   1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 948a084..392dded 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2842,15 +2842,16 @@ static inline char *dup_token(const char **buf,
> size_t *lenp)
>    * Note: rbd_dev is assumed to have been initially zero-filled.
>    */
>   static struct ceph_options *rbd_add_parse_args(struct rbd_device *rbd_dev,
> -						const char *buf)
> +						const char *buf,
> +						struct rbd_options **opts)
>   {
>   	size_t len;
>   	const char *mon_addrs;
>   	size_t mon_addrs_size;
>   	char *options;
>   	struct ceph_options *err_ptr = ERR_PTR(-EINVAL);
> -	struct rbd_options rbd_opts;
>   	struct ceph_options *ceph_opts;
> +	struct rbd_options *rbd_opts = NULL;
>
>   	/* The first four tokens are required */
>
> @@ -2899,17 +2900,17 @@ static struct ceph_options
> *rbd_add_parse_args(struct rbd_device *rbd_dev,
>
>   	/* Initialize all rbd options to the defaults */
>
> -	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;
> +	rbd_opts = kzalloc(sizeof (*rbd_opts), GFP_KERNEL);
> +	if (!rbd_opts)
> +		goto out_mem;
> +
> +	rbd_opts->read_only = RBD_READ_ONLY_DEFAULT;
>
>   	ceph_opts = ceph_parse_options(options, mon_addrs,
>   					mon_addrs + mon_addrs_size - 1,
> -					parse_rbd_opts_token, &rbd_opts);
> +					parse_rbd_opts_token, rbd_opts);
>   	kfree(options);
> -
> -	/* Record the parsed rbd options */
> -
> -	if (!IS_ERR(ceph_opts))
> -		rbd_dev->mapping.read_only = rbd_opts.read_only;
> +	*opts = rbd_opts;
>
>   	return ceph_opts;
>   out_mem:
> @@ -3131,6 +3132,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   {
>   	struct rbd_device *rbd_dev = NULL;
>   	struct ceph_options *ceph_opts;
> +	struct rbd_options *rbd_opts = NULL;
>   	struct ceph_osd_client *osdc;
>   	int rc = -ENOMEM;
>
> @@ -3139,7 +3141,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>
>   	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
>   	if (!rbd_dev)
> -		goto err_out_mem;
> +		return -ENOMEM;
>
>   	/* static rbd_device initialization */
>   	spin_lock_init(&rbd_dev->lock);
> @@ -3148,11 +3150,12 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	init_rwsem(&rbd_dev->header_rwsem);
>
>   	/* parse add command */
> -	ceph_opts = rbd_add_parse_args(rbd_dev, buf);
> +	ceph_opts = rbd_add_parse_args(rbd_dev, buf, &rbd_opts);
>   	if (IS_ERR(ceph_opts)) {
>   		rc = PTR_ERR(ceph_opts);
>   		goto err_out_mem;
>   	}
> +	rbd_dev->mapping.read_only = rbd_opts->read_only;
>
>   	rc = rbd_get_client(rbd_dev, ceph_opts);
>   	if (rc < 0)
> @@ -3219,6 +3222,8 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	if (rc)
>   		goto err_out_bus;
>
> +	kfree(rbd_opts);
> +
>   	/* Everything's ready.  Announce the disk to the world. */
>
>   	add_disk(rbd_dev->disk);
> @@ -3232,6 +3237,8 @@ err_out_bus:
>   	/* this will also clean up rest of rbd_dev stuff */
>
>   	rbd_bus_del_dev(rbd_dev);
> +	kfree(rbd_opts);
> +
>   	return rc;
>
>   err_out_disk:
> @@ -3254,6 +3261,7 @@ err_out_args:
>   	kfree(rbd_dev->snap_name);
>   	kfree(rbd_dev->image_name);
>   	kfree(rbd_dev->pool_name);
> +	kfree(rbd_opts);
>   err_out_mem:
>   	kfree(rbd_dev);
>

[PATCH 5/8] rbd: have rbd_add_parse_args() return error

[Alex Elder]

Change the interface to rbd_add_parse_args() so it returns an
error code rather than a pointer.  Return the ceph_options result
via a pointer whose address is passed as an argument.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 392dded..388dd47 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2841,30 +2841,31 @@ static inline char *dup_token(const char **buf,
size_t *lenp)
  *
  * Note: rbd_dev is assumed to have been initially zero-filled.
  */
-static struct ceph_options *rbd_add_parse_args(struct rbd_device *rbd_dev,
-						const char *buf,
-						struct rbd_options **opts)
+static int rbd_add_parse_args(struct rbd_device *rbd_dev,
+				const char *buf,
+				struct ceph_options **ceph_opts,
+				struct rbd_options **opts)
 {
 	size_t len;
 	const char *mon_addrs;
 	size_t mon_addrs_size;
 	char *options;
-	struct ceph_options *err_ptr = ERR_PTR(-EINVAL);
-	struct ceph_options *ceph_opts;
 	struct rbd_options *rbd_opts = NULL;
+	int ret;

 	/* The first four tokens are required */

 	len = next_token(&buf);
 	if (!len)
-		return err_ptr;	/* Missing monitor address(es) */
+		return -EINVAL;	/* Missing monitor address(es) */
 	mon_addrs = buf;
 	mon_addrs_size = len + 1;
 	buf += len;

+	ret = -EINVAL;
 	options = dup_token(&buf, NULL);
 	if (!options)
-		goto out_mem;
+		return -ENOMEM;
 	if (!*options)
 		goto out_err;	/* Missing options */

@@ -2889,7 +2890,7 @@ static struct ceph_options
*rbd_add_parse_args(struct rbd_device *rbd_dev,
 		buf = RBD_SNAP_HEAD_NAME; /* No snapshot supplied */
 		len = sizeof (RBD_SNAP_HEAD_NAME) - 1;
 	} else if (len > RBD_MAX_SNAP_NAME_LEN) {
-		err_ptr = ERR_PTR(-ENAMETOOLONG);
+		ret = -ENAMETOOLONG;
 		goto out_err;
 	}
 	rbd_dev->snap_name = kmalloc(len + 1, GFP_KERNEL);
@@ -2906,15 +2907,19 @@ static struct ceph_options
*rbd_add_parse_args(struct rbd_device *rbd_dev,

 	rbd_opts->read_only = RBD_READ_ONLY_DEFAULT;

-	ceph_opts = ceph_parse_options(options, mon_addrs,
+	*ceph_opts = ceph_parse_options(options, mon_addrs,
 					mon_addrs + mon_addrs_size - 1,
 					parse_rbd_opts_token, rbd_opts);
 	kfree(options);
+	if (IS_ERR(*ceph_opts)) {
+		ret = PTR_ERR(*ceph_opts);
+		goto out_err;
+	}
 	*opts = rbd_opts;

-	return ceph_opts;
+	return 0;
 out_mem:
-	err_ptr = ERR_PTR(-ENOMEM);
+	ret = -ENOMEM;
 out_err:
 	kfree(rbd_dev->image_name);
 	rbd_dev->image_name = NULL;
@@ -2923,7 +2928,7 @@ out_err:
 	rbd_dev->pool_name = NULL;
 	kfree(options);

-	return err_ptr;
+	return ret;
 }

 /*
@@ -3131,7 +3136,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 		       size_t count)
 {
 	struct rbd_device *rbd_dev = NULL;
-	struct ceph_options *ceph_opts;
+	struct ceph_options *ceph_opts = NULL;
 	struct rbd_options *rbd_opts = NULL;
 	struct ceph_osd_client *osdc;
 	int rc = -ENOMEM;
@@ -3150,11 +3155,9 @@ static ssize_t rbd_add(struct bus_type *bus,
 	init_rwsem(&rbd_dev->header_rwsem);

 	/* parse add command */
-	ceph_opts = rbd_add_parse_args(rbd_dev, buf, &rbd_opts);
-	if (IS_ERR(ceph_opts)) {
-		rc = PTR_ERR(ceph_opts);
+	rc = rbd_add_parse_args(rbd_dev, buf, &ceph_opts, &rbd_opts);
+	if (rc < 0)
 		goto err_out_mem;
-	}
 	rbd_dev->mapping.read_only = rbd_opts->read_only;

 	rc = rbd_get_client(rbd_dev, ceph_opts);
-- 
1.7.9.5

Re: [PATCH 5/8] rbd: have rbd_add_parse_args() return error

[Josh Durgin]

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

On 10/26/2012 04:02 PM, Alex Elder wrote:
> Change the interface to rbd_add_parse_args() so it returns an
> error code rather than a pointer.  Return the ceph_options result
> via a pointer whose address is passed as an argument.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   37 ++++++++++++++++++++-----------------
>   1 file changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 392dded..388dd47 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2841,30 +2841,31 @@ static inline char *dup_token(const char **buf,
> size_t *lenp)
>    *
>    * Note: rbd_dev is assumed to have been initially zero-filled.
>    */
> -static struct ceph_options *rbd_add_parse_args(struct rbd_device *rbd_dev,
> -						const char *buf,
> -						struct rbd_options **opts)
> +static int rbd_add_parse_args(struct rbd_device *rbd_dev,
> +				const char *buf,
> +				struct ceph_options **ceph_opts,
> +				struct rbd_options **opts)
>   {
>   	size_t len;
>   	const char *mon_addrs;
>   	size_t mon_addrs_size;
>   	char *options;
> -	struct ceph_options *err_ptr = ERR_PTR(-EINVAL);
> -	struct ceph_options *ceph_opts;
>   	struct rbd_options *rbd_opts = NULL;
> +	int ret;
>
>   	/* The first four tokens are required */
>
>   	len = next_token(&buf);
>   	if (!len)
> -		return err_ptr;	/* Missing monitor address(es) */
> +		return -EINVAL;	/* Missing monitor address(es) */
>   	mon_addrs = buf;
>   	mon_addrs_size = len + 1;
>   	buf += len;
>
> +	ret = -EINVAL;
>   	options = dup_token(&buf, NULL);
>   	if (!options)
> -		goto out_mem;
> +		return -ENOMEM;
>   	if (!*options)
>   		goto out_err;	/* Missing options */
>
> @@ -2889,7 +2890,7 @@ static struct ceph_options
> *rbd_add_parse_args(struct rbd_device *rbd_dev,
>   		buf = RBD_SNAP_HEAD_NAME; /* No snapshot supplied */
>   		len = sizeof (RBD_SNAP_HEAD_NAME) - 1;
>   	} else if (len > RBD_MAX_SNAP_NAME_LEN) {
> -		err_ptr = ERR_PTR(-ENAMETOOLONG);
> +		ret = -ENAMETOOLONG;
>   		goto out_err;
>   	}
>   	rbd_dev->snap_name = kmalloc(len + 1, GFP_KERNEL);
> @@ -2906,15 +2907,19 @@ static struct ceph_options
> *rbd_add_parse_args(struct rbd_device *rbd_dev,
>
>   	rbd_opts->read_only = RBD_READ_ONLY_DEFAULT;
>
> -	ceph_opts = ceph_parse_options(options, mon_addrs,
> +	*ceph_opts = ceph_parse_options(options, mon_addrs,
>   					mon_addrs + mon_addrs_size - 1,
>   					parse_rbd_opts_token, rbd_opts);
>   	kfree(options);
> +	if (IS_ERR(*ceph_opts)) {
> +		ret = PTR_ERR(*ceph_opts);
> +		goto out_err;
> +	}
>   	*opts = rbd_opts;
>
> -	return ceph_opts;
> +	return 0;
>   out_mem:
> -	err_ptr = ERR_PTR(-ENOMEM);
> +	ret = -ENOMEM;
>   out_err:
>   	kfree(rbd_dev->image_name);
>   	rbd_dev->image_name = NULL;
> @@ -2923,7 +2928,7 @@ out_err:
>   	rbd_dev->pool_name = NULL;
>   	kfree(options);
>
> -	return err_ptr;
> +	return ret;
>   }
>
>   /*
> @@ -3131,7 +3136,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   		       size_t count)
>   {
>   	struct rbd_device *rbd_dev = NULL;
> -	struct ceph_options *ceph_opts;
> +	struct ceph_options *ceph_opts = NULL;
>   	struct rbd_options *rbd_opts = NULL;
>   	struct ceph_osd_client *osdc;
>   	int rc = -ENOMEM;
> @@ -3150,11 +3155,9 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	init_rwsem(&rbd_dev->header_rwsem);
>
>   	/* parse add command */
> -	ceph_opts = rbd_add_parse_args(rbd_dev, buf, &rbd_opts);
> -	if (IS_ERR(ceph_opts)) {
> -		rc = PTR_ERR(ceph_opts);
> +	rc = rbd_add_parse_args(rbd_dev, buf, &ceph_opts, &rbd_opts);
> +	if (rc < 0)
>   		goto err_out_mem;
> -	}
>   	rbd_dev->mapping.read_only = rbd_opts->read_only;
>
>   	rc = rbd_get_client(rbd_dev, ceph_opts);
>

[PATCH 6/8] rbd: define image specification structure

[Alex Elder]

Group the fields that uniquely specify an rbd image into a new
reference-counted rbd_spec structure.  This structure will be used
to describe the desired image when mapping an image, and when
probing parent images in layered rbd devices.  Replace the set of
fields in the rbd device structure with a pointer to a dynamically
allocated rbd_spec.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 388dd47..c39e238 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -112,6 +112,27 @@ struct rbd_image_header {
 	u64 obj_version;
 };

+/*
+ * An rbd image specification.
+ *
+ * The tuple (pool_id, image_id, snap_id) is sufficient to uniquely
+ * identify an image.
+ */
+struct rbd_spec {
+	u64		pool_id;
+	char		*pool_name;
+
+	char		*image_id;
+	size_t		image_id_len;
+	char		*image_name;
+	size_t		image_name_len;
+
+	u64		snap_id;
+	char		*snap_name;
+
+	struct kref	kref;
+};
+
 struct rbd_options {
 	bool	read_only;
 };
@@ -189,16 +210,9 @@ struct rbd_device {

 	struct rbd_image_header	header;
 	bool                    exists;
-	char			*image_id;
-	size_t			image_id_len;
-	char			*image_name;
-	size_t			image_name_len;
-	char			*header_name;
-	char			*pool_name;
-	u64			pool_id;
+	struct rbd_spec		*spec;

-	char                    *snap_name;
-	u64                     snap_id;
+	char			*header_name;

 	struct ceph_osd_event   *watch_event;
 	struct ceph_osd_request *watch_request;
@@ -654,7 +668,7 @@ static int snap_by_name(struct rbd_device *rbd_dev,
const char *snap_name)

 	list_for_each_entry(snap, &rbd_dev->snaps, node) {
 		if (!strcmp(snap_name, snap->name)) {
-			rbd_dev->snap_id = snap->id;
+			rbd_dev->spec->snap_id = snap->id;
 			rbd_dev->mapping.size = snap->size;
 			rbd_dev->mapping.features = snap->features;

@@ -669,14 +683,14 @@ static int rbd_dev_set_mapping(struct rbd_device
*rbd_dev)
 {
 	int ret;

-	if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
+	if (!memcmp(rbd_dev->spec->snap_name, RBD_SNAP_HEAD_NAME,
 		    sizeof (RBD_SNAP_HEAD_NAME))) {
-		rbd_dev->snap_id = CEPH_NOSNAP;
+		rbd_dev->spec->snap_id = CEPH_NOSNAP;
 		rbd_dev->mapping.size = rbd_dev->header.image_size;
 		rbd_dev->mapping.features = rbd_dev->header.features;
 		ret = 0;
 	} else {
-		ret = snap_by_name(rbd_dev, rbd_dev->snap_name);
+		ret = snap_by_name(rbd_dev, rbd_dev->spec->snap_name);
 		if (ret < 0)
 			goto done;
 		rbd_dev->mapping.read_only = true;
@@ -1096,7 +1110,7 @@ static int rbd_do_request(struct request *rq,
 	layout->fl_stripe_unit = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
 	layout->fl_stripe_count = cpu_to_le32(1);
 	layout->fl_object_size = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
-	layout->fl_pg_pool = cpu_to_le32((int) rbd_dev->pool_id);
+	layout->fl_pg_pool = cpu_to_le32((int) rbd_dev->spec->pool_id);
 	ret = ceph_calc_raw_layout(osdc, layout, snapid, ofs, &len, &bno,
 				   req, ops);
 	rbd_assert(ret == 0);
@@ -1261,7 +1275,7 @@ static int rbd_do_op(struct request *rq,
 		opcode = CEPH_OSD_OP_READ;
 		flags = CEPH_OSD_FLAG_READ;
 		snapc = NULL;
-		snapid = rbd_dev->snap_id;
+		snapid = rbd_dev->spec->snap_id;
 		payload_len = 0;
 	}

@@ -1545,7 +1559,7 @@ static void rbd_rq_fn(struct request_queue *q)
 		down_read(&rbd_dev->header_rwsem);

 		if (!rbd_dev->exists) {
-			rbd_assert(rbd_dev->snap_id != CEPH_NOSNAP);
+			rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP);
 			up_read(&rbd_dev->header_rwsem);
 			dout("request for non-existent snapshot");
 			spin_lock_irq(q->queue_lock);
@@ -1726,13 +1740,13 @@ rbd_dev_v1_header_read(struct rbd_device
*rbd_dev, u64 *version)
 			ret = -ENXIO;
 			pr_warning("short header read for image %s"
 					" (want %zd got %d)\n",
-				rbd_dev->image_name, size, ret);
+				rbd_dev->spec->image_name, size, ret);
 			goto out_err;
 		}
 		if (!rbd_dev_ondisk_valid(ondisk)) {
 			ret = -ENXIO;
 			pr_warning("invalid header for image %s\n",
-				rbd_dev->image_name);
+				rbd_dev->spec->image_name);
 			goto out_err;
 		}

@@ -1783,7 +1797,7 @@ static void rbd_update_mapping_size(struct
rbd_device *rbd_dev)
 {
 	sector_t size;

-	if (rbd_dev->snap_id != CEPH_NOSNAP)
+	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
 		return;

 	size = (sector_t) rbd_dev->header.image_size / SECTOR_SIZE;
@@ -1957,7 +1971,7 @@ static ssize_t rbd_pool_show(struct device *dev,
 {
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

-	return sprintf(buf, "%s\n", rbd_dev->pool_name);
+	return sprintf(buf, "%s\n", rbd_dev->spec->pool_name);
 }

 static ssize_t rbd_pool_id_show(struct device *dev,
@@ -1965,7 +1979,8 @@ static ssize_t rbd_pool_id_show(struct device *dev,
 {
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

-	return sprintf(buf, "%llu\n", (unsigned long long) rbd_dev->pool_id);
+	return sprintf(buf, "%llu\n",
+		(unsigned long long) rbd_dev->spec->pool_id);
 }

 static ssize_t rbd_name_show(struct device *dev,
@@ -1973,7 +1988,7 @@ static ssize_t rbd_name_show(struct device *dev,
 {
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

-	return sprintf(buf, "%s\n", rbd_dev->image_name);
+	return sprintf(buf, "%s\n", rbd_dev->spec->image_name);
 }

 static ssize_t rbd_image_id_show(struct device *dev,
@@ -1981,7 +1996,7 @@ static ssize_t rbd_image_id_show(struct device *dev,
 {
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

-	return sprintf(buf, "%s\n", rbd_dev->image_id);
+	return sprintf(buf, "%s\n", rbd_dev->spec->image_id);
 }

 /*
@@ -1994,7 +2009,7 @@ static ssize_t rbd_snap_show(struct device *dev,
 {
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

-	return sprintf(buf, "%s\n", rbd_dev->snap_name);
+	return sprintf(buf, "%s\n", rbd_dev->spec->snap_name);
 }

 static ssize_t rbd_image_refresh(struct device *dev,
@@ -2547,11 +2562,12 @@ static int rbd_dev_snaps_update(struct
rbd_device *rbd_dev)

 			/* Existing snapshot not in the new snap context */

-			if (rbd_dev->snap_id == snap->id)
+			if (rbd_dev->spec->snap_id == snap->id)
 				rbd_dev->exists = false;
 			__rbd_remove_snap_dev(snap);
 			dout("%ssnap id %llu has been removed\n",
-				rbd_dev->snap_id == snap->id ?  "mapped " : "",
+				rbd_dev->spec->snap_id == snap->id ?
+							"mapped " : "",
 				(unsigned long long) snap->id);

 			/* Done with this list entry; advance */
@@ -2869,16 +2885,17 @@ static int rbd_add_parse_args(struct rbd_device
*rbd_dev,
 	if (!*options)
 		goto out_err;	/* Missing options */

-	rbd_dev->pool_name = dup_token(&buf, NULL);
-	if (!rbd_dev->pool_name)
+	rbd_dev->spec->pool_name = dup_token(&buf, NULL);
+	if (!rbd_dev->spec->pool_name)
 		goto out_mem;
-	if (!*rbd_dev->pool_name)
+	if (!*rbd_dev->spec->pool_name)
 		goto out_err;	/* Missing pool name */

-	rbd_dev->image_name = dup_token(&buf, &rbd_dev->image_name_len);
-	if (!rbd_dev->image_name)
+	rbd_dev->spec->image_name =
+		dup_token(&buf, &rbd_dev->spec->image_name_len);
+	if (!rbd_dev->spec->image_name)
 		goto out_mem;
-	if (!*rbd_dev->image_name)
+	if (!*rbd_dev->spec->image_name)
 		goto out_err;	/* Missing image name */

 	/*
@@ -2893,11 +2910,11 @@ static int rbd_add_parse_args(struct rbd_device
*rbd_dev,
 		ret = -ENAMETOOLONG;
 		goto out_err;
 	}
-	rbd_dev->snap_name = kmalloc(len + 1, GFP_KERNEL);
-	if (!rbd_dev->snap_name)
+	rbd_dev->spec->snap_name = kmalloc(len + 1, GFP_KERNEL);
+	if (!rbd_dev->spec->snap_name)
 		goto out_mem;
-	memcpy(rbd_dev->snap_name, buf, len);
-	*(rbd_dev->snap_name + len) = '\0';
+	memcpy(rbd_dev->spec->snap_name, buf, len);
+	*(rbd_dev->spec->snap_name + len) = '\0';

 	/* Initialize all rbd options to the defaults */

@@ -2921,11 +2938,11 @@ static int rbd_add_parse_args(struct rbd_device
*rbd_dev,
 out_mem:
 	ret = -ENOMEM;
 out_err:
-	kfree(rbd_dev->image_name);
-	rbd_dev->image_name = NULL;
-	rbd_dev->image_name_len = 0;
-	kfree(rbd_dev->pool_name);
-	rbd_dev->pool_name = NULL;
+	kfree(rbd_dev->spec->image_name);
+	rbd_dev->spec->image_name = NULL;
+	rbd_dev->spec->image_name_len = 0;
+	kfree(rbd_dev->spec->pool_name);
+	rbd_dev->spec->pool_name = NULL;
 	kfree(options);

 	return ret;
@@ -2957,11 +2974,11 @@ static int rbd_dev_image_id(struct rbd_device
*rbd_dev)
 	 * First, see if the format 2 image id file exists, and if
 	 * so, get the image's persistent id from it.
 	 */
-	size = sizeof (RBD_ID_PREFIX) + rbd_dev->image_name_len;
+	size = sizeof (RBD_ID_PREFIX) + rbd_dev->spec->image_name_len;
 	object_name = kmalloc(size, GFP_NOIO);
 	if (!object_name)
 		return -ENOMEM;
-	sprintf(object_name, "%s%s", RBD_ID_PREFIX, rbd_dev->image_name);
+	sprintf(object_name, "%s%s", RBD_ID_PREFIX, rbd_dev->spec->image_name);
 	dout("rbd id object name is %s\n", object_name);

 	/* Response will be an encoded string, which includes a length */
@@ -2984,15 +3001,15 @@ static int rbd_dev_image_id(struct rbd_device
*rbd_dev)
 	ret = 0;    /* rbd_req_sync_exec() can return positive */

 	p = response;
-	rbd_dev->image_id = ceph_extract_encoded_string(&p,
+	rbd_dev->spec->image_id = ceph_extract_encoded_string(&p,
 						p + RBD_IMAGE_ID_LEN_MAX,
-						&rbd_dev->image_id_len,
+						&rbd_dev->spec->image_id_len,
 						GFP_NOIO);
-	if (IS_ERR(rbd_dev->image_id)) {
-		ret = PTR_ERR(rbd_dev->image_id);
-		rbd_dev->image_id = NULL;
+	if (IS_ERR(rbd_dev->spec->image_id)) {
+		ret = PTR_ERR(rbd_dev->spec->image_id);
+		rbd_dev->spec->image_id = NULL;
 	} else {
-		dout("image_id is %s\n", rbd_dev->image_id);
+		dout("image_id is %s\n", rbd_dev->spec->image_id);
 	}
 out:
 	kfree(response);
@@ -3008,20 +3025,21 @@ static int rbd_dev_v1_probe(struct rbd_device
*rbd_dev)

 	/* Version 1 images have no id; empty string is used */

-	rbd_dev->image_id = kstrdup("", GFP_KERNEL);
-	if (!rbd_dev->image_id)
+	rbd_dev->spec->image_id = kstrdup("", GFP_KERNEL);
+	if (!rbd_dev->spec->image_id)
 		return -ENOMEM;
-	rbd_dev->image_id_len = 0;
+	rbd_dev->spec->image_id_len = 0;

 	/* Record the header object name for this rbd image. */

-	size = rbd_dev->image_name_len + sizeof (RBD_SUFFIX);
+	size = rbd_dev->spec->image_name_len + sizeof (RBD_SUFFIX);
 	rbd_dev->header_name = kmalloc(size, GFP_KERNEL);
 	if (!rbd_dev->header_name) {
 		ret = -ENOMEM;
 		goto out_err;
 	}
-	sprintf(rbd_dev->header_name, "%s%s", rbd_dev->image_name, RBD_SUFFIX);
+	sprintf(rbd_dev->header_name, "%s%s",
+		rbd_dev->spec->image_name, RBD_SUFFIX);

 	/* Populate rbd image metadata */

@@ -3038,8 +3056,8 @@ static int rbd_dev_v1_probe(struct rbd_device
*rbd_dev)
 out_err:
 	kfree(rbd_dev->header_name);
 	rbd_dev->header_name = NULL;
-	kfree(rbd_dev->image_id);
-	rbd_dev->image_id = NULL;
+	kfree(rbd_dev->spec->image_id);
+	rbd_dev->spec->image_id = NULL;

 	return ret;
 }
@@ -3054,12 +3072,12 @@ static int rbd_dev_v2_probe(struct rbd_device
*rbd_dev)
 	 * Image id was filled in by the caller.  Record the header
 	 * object name for this rbd image.
 	 */
-	size = sizeof (RBD_HEADER_PREFIX) + rbd_dev->image_id_len;
+	size = sizeof (RBD_HEADER_PREFIX) + rbd_dev->spec->image_id_len;
 	rbd_dev->header_name = kmalloc(size, GFP_KERNEL);
 	if (!rbd_dev->header_name)
 		return -ENOMEM;
 	sprintf(rbd_dev->header_name, "%s%s",
-			RBD_HEADER_PREFIX, rbd_dev->image_id);
+			RBD_HEADER_PREFIX, rbd_dev->spec->image_id);

 	/* Get the size and object order for the image */

@@ -3147,6 +3165,9 @@ static ssize_t rbd_add(struct bus_type *bus,
 	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
 	if (!rbd_dev)
 		return -ENOMEM;
+	rbd_dev->spec = kzalloc(sizeof (*rbd_dev->spec), GFP_KERNEL);
+	if (!rbd_dev->spec)
+		goto err_out_mem;

 	/* static rbd_device initialization */
 	spin_lock_init(&rbd_dev->lock);
@@ -3167,10 +3188,10 @@ static ssize_t rbd_add(struct bus_type *bus,

 	/* pick the pool */
 	osdc = &rbd_dev->rbd_client->client->osdc;
-	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name);
+	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->spec->pool_name);
 	if (rc < 0)
 		goto err_out_client;
-	rbd_dev->pool_id = (u64) rc;
+	rbd_dev->spec->pool_id = (u64) rc;

 	rc = rbd_dev_probe(rbd_dev);
 	if (rc < 0)
@@ -3257,15 +3278,16 @@ err_out_probe:
 err_out_client:
 	kfree(rbd_dev->header_name);
 	rbd_put_client(rbd_dev);
-	kfree(rbd_dev->image_id);
+	kfree(rbd_dev->spec->image_id);
 err_out_args:
 	if (ceph_opts)
 		ceph_destroy_options(ceph_opts);
-	kfree(rbd_dev->snap_name);
-	kfree(rbd_dev->image_name);
-	kfree(rbd_dev->pool_name);
+	kfree(rbd_dev->spec->snap_name);
+	kfree(rbd_dev->spec->image_name);
+	kfree(rbd_dev->spec->pool_name);
 	kfree(rbd_opts);
 err_out_mem:
+	kfree(rbd_dev->spec);
 	kfree(rbd_dev);

 	dout("Error adding device %s\n", buf);
@@ -3314,11 +3336,11 @@ static void rbd_dev_release(struct device *dev)
 	rbd_header_free(&rbd_dev->header);

 	/* done with the id, and with the rbd_dev */
-	kfree(rbd_dev->snap_name);
-	kfree(rbd_dev->image_id);
+	kfree(rbd_dev->spec->snap_name);
+	kfree(rbd_dev->spec->image_id);
 	kfree(rbd_dev->header_name);
-	kfree(rbd_dev->pool_name);
-	kfree(rbd_dev->image_name);
+	kfree(rbd_dev->spec->pool_name);
+	kfree(rbd_dev->spec->image_name);
 	rbd_dev_id_put(rbd_dev);
 	kfree(rbd_dev);

-- 
1.7.9.5

Re: [PATCH 6/8] rbd: define image specification structure

[Josh Durgin]

A couple notes below, but looks good.

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

On 10/26/2012 04:03 PM, Alex Elder wrote:
> Group the fields that uniquely specify an rbd image into a new
> reference-counted rbd_spec structure.  This structure will be used
> to describe the desired image when mapping an image, and when
> probing parent images in layered rbd devices.  Replace the set of
> fields in the rbd device structure with a pointer to a dynamically
> allocated rbd_spec.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |  158
> +++++++++++++++++++++++++++++----------------------
>   1 file changed, 90 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 388dd47..c39e238 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -112,6 +112,27 @@ struct rbd_image_header {
>   	u64 obj_version;
>   };
>
> +/*
> + * An rbd image specification.
> + *
> + * The tuple (pool_id, image_id, snap_id) is sufficient to uniquely
> + * identify an image.
> + */

This looks like it's meant to be immutable. If so, it'd be nice
to have that in the comment.

> +struct rbd_spec {
> +	u64		pool_id;
> +	char		*pool_name;
> +
> +	char		*image_id;
> +	size_t		image_id_len;
> +	char		*image_name;
> +	size_t		image_name_len;

I realize you're just rearranging things in this patch, but it's a bit
odd that only image_id and image_name have corresponding lengths stored.
Those fields don't seem necessary.

> +
> +	u64		snap_id;
> +	char		*snap_name;
> +
> +	struct kref	kref;
> +};
> +
>   struct rbd_options {
>   	bool	read_only;
>   };
> @@ -189,16 +210,9 @@ struct rbd_device {
>
>   	struct rbd_image_header	header;
>   	bool                    exists;
> -	char			*image_id;
> -	size_t			image_id_len;
> -	char			*image_name;
> -	size_t			image_name_len;
> -	char			*header_name;
> -	char			*pool_name;
> -	u64			pool_id;
> +	struct rbd_spec		*spec;
>
> -	char                    *snap_name;
> -	u64                     snap_id;
> +	char			*header_name;

Are you planning to split out more stuff into a common
struct rbd_image, like header_name, exists, etc?
There's a bunch of stuff that's not specific to a particular rbd_device
in here.

>
>   	struct ceph_osd_event   *watch_event;
>   	struct ceph_osd_request *watch_request;
> @@ -654,7 +668,7 @@ static int snap_by_name(struct rbd_device *rbd_dev,
> const char *snap_name)
>
>   	list_for_each_entry(snap, &rbd_dev->snaps, node) {
>   		if (!strcmp(snap_name, snap->name)) {
> -			rbd_dev->snap_id = snap->id;
> +			rbd_dev->spec->snap_id = snap->id;
>   			rbd_dev->mapping.size = snap->size;
>   			rbd_dev->mapping.features = snap->features;
>
> @@ -669,14 +683,14 @@ static int rbd_dev_set_mapping(struct rbd_device
> *rbd_dev)
>   {
>   	int ret;
>
> -	if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
> +	if (!memcmp(rbd_dev->spec->snap_name, RBD_SNAP_HEAD_NAME,
>   		    sizeof (RBD_SNAP_HEAD_NAME))) {
> -		rbd_dev->snap_id = CEPH_NOSNAP;
> +		rbd_dev->spec->snap_id = CEPH_NOSNAP;
>   		rbd_dev->mapping.size = rbd_dev->header.image_size;
>   		rbd_dev->mapping.features = rbd_dev->header.features;
>   		ret = 0;
>   	} else {
> -		ret = snap_by_name(rbd_dev, rbd_dev->snap_name);
> +		ret = snap_by_name(rbd_dev, rbd_dev->spec->snap_name);
>   		if (ret < 0)
>   			goto done;
>   		rbd_dev->mapping.read_only = true;
> @@ -1096,7 +1110,7 @@ static int rbd_do_request(struct request *rq,
>   	layout->fl_stripe_unit = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
>   	layout->fl_stripe_count = cpu_to_le32(1);
>   	layout->fl_object_size = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
> -	layout->fl_pg_pool = cpu_to_le32((int) rbd_dev->pool_id);
> +	layout->fl_pg_pool = cpu_to_le32((int) rbd_dev->spec->pool_id);
>   	ret = ceph_calc_raw_layout(osdc, layout, snapid, ofs, &len, &bno,
>   				   req, ops);
>   	rbd_assert(ret == 0);
> @@ -1261,7 +1275,7 @@ static int rbd_do_op(struct request *rq,
>   		opcode = CEPH_OSD_OP_READ;
>   		flags = CEPH_OSD_FLAG_READ;
>   		snapc = NULL;
> -		snapid = rbd_dev->snap_id;
> +		snapid = rbd_dev->spec->snap_id;
>   		payload_len = 0;
>   	}
>
> @@ -1545,7 +1559,7 @@ static void rbd_rq_fn(struct request_queue *q)
>   		down_read(&rbd_dev->header_rwsem);
>
>   		if (!rbd_dev->exists) {
> -			rbd_assert(rbd_dev->snap_id != CEPH_NOSNAP);
> +			rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP);
>   			up_read(&rbd_dev->header_rwsem);
>   			dout("request for non-existent snapshot");
>   			spin_lock_irq(q->queue_lock);
> @@ -1726,13 +1740,13 @@ rbd_dev_v1_header_read(struct rbd_device
> *rbd_dev, u64 *version)
>   			ret = -ENXIO;
>   			pr_warning("short header read for image %s"
>   					" (want %zd got %d)\n",
> -				rbd_dev->image_name, size, ret);
> +				rbd_dev->spec->image_name, size, ret);
>   			goto out_err;
>   		}
>   		if (!rbd_dev_ondisk_valid(ondisk)) {
>   			ret = -ENXIO;
>   			pr_warning("invalid header for image %s\n",
> -				rbd_dev->image_name);
> +				rbd_dev->spec->image_name);
>   			goto out_err;
>   		}
>
> @@ -1783,7 +1797,7 @@ static void rbd_update_mapping_size(struct
> rbd_device *rbd_dev)
>   {
>   	sector_t size;
>
> -	if (rbd_dev->snap_id != CEPH_NOSNAP)
> +	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
>   		return;
>
>   	size = (sector_t) rbd_dev->header.image_size / SECTOR_SIZE;
> @@ -1957,7 +1971,7 @@ static ssize_t rbd_pool_show(struct device *dev,
>   {
>   	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>
> -	return sprintf(buf, "%s\n", rbd_dev->pool_name);
> +	return sprintf(buf, "%s\n", rbd_dev->spec->pool_name);
>   }
>
>   static ssize_t rbd_pool_id_show(struct device *dev,
> @@ -1965,7 +1979,8 @@ static ssize_t rbd_pool_id_show(struct device *dev,
>   {
>   	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>
> -	return sprintf(buf, "%llu\n", (unsigned long long) rbd_dev->pool_id);
> +	return sprintf(buf, "%llu\n",
> +		(unsigned long long) rbd_dev->spec->pool_id);
>   }
>
>   static ssize_t rbd_name_show(struct device *dev,
> @@ -1973,7 +1988,7 @@ static ssize_t rbd_name_show(struct device *dev,
>   {
>   	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>
> -	return sprintf(buf, "%s\n", rbd_dev->image_name);
> +	return sprintf(buf, "%s\n", rbd_dev->spec->image_name);
>   }
>
>   static ssize_t rbd_image_id_show(struct device *dev,
> @@ -1981,7 +1996,7 @@ static ssize_t rbd_image_id_show(struct device *dev,
>   {
>   	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>
> -	return sprintf(buf, "%s\n", rbd_dev->image_id);
> +	return sprintf(buf, "%s\n", rbd_dev->spec->image_id);
>   }
>
>   /*
> @@ -1994,7 +2009,7 @@ static ssize_t rbd_snap_show(struct device *dev,
>   {
>   	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>
> -	return sprintf(buf, "%s\n", rbd_dev->snap_name);
> +	return sprintf(buf, "%s\n", rbd_dev->spec->snap_name);
>   }
>
>   static ssize_t rbd_image_refresh(struct device *dev,
> @@ -2547,11 +2562,12 @@ static int rbd_dev_snaps_update(struct
> rbd_device *rbd_dev)
>
>   			/* Existing snapshot not in the new snap context */
>
> -			if (rbd_dev->snap_id == snap->id)
> +			if (rbd_dev->spec->snap_id == snap->id)
>   				rbd_dev->exists = false;
>   			__rbd_remove_snap_dev(snap);
>   			dout("%ssnap id %llu has been removed\n",
> -				rbd_dev->snap_id == snap->id ?  "mapped " : "",
> +				rbd_dev->spec->snap_id == snap->id ?
> +							"mapped " : "",
>   				(unsigned long long) snap->id);
>
>   			/* Done with this list entry; advance */
> @@ -2869,16 +2885,17 @@ static int rbd_add_parse_args(struct rbd_device
> *rbd_dev,
>   	if (!*options)
>   		goto out_err;	/* Missing options */
>
> -	rbd_dev->pool_name = dup_token(&buf, NULL);
> -	if (!rbd_dev->pool_name)
> +	rbd_dev->spec->pool_name = dup_token(&buf, NULL);
> +	if (!rbd_dev->spec->pool_name)
>   		goto out_mem;
> -	if (!*rbd_dev->pool_name)
> +	if (!*rbd_dev->spec->pool_name)
>   		goto out_err;	/* Missing pool name */
>
> -	rbd_dev->image_name = dup_token(&buf, &rbd_dev->image_name_len);
> -	if (!rbd_dev->image_name)
> +	rbd_dev->spec->image_name =
> +		dup_token(&buf, &rbd_dev->spec->image_name_len);
> +	if (!rbd_dev->spec->image_name)
>   		goto out_mem;
> -	if (!*rbd_dev->image_name)
> +	if (!*rbd_dev->spec->image_name)
>   		goto out_err;	/* Missing image name */
>
>   	/*
> @@ -2893,11 +2910,11 @@ static int rbd_add_parse_args(struct rbd_device
> *rbd_dev,
>   		ret = -ENAMETOOLONG;
>   		goto out_err;
>   	}
> -	rbd_dev->snap_name = kmalloc(len + 1, GFP_KERNEL);
> -	if (!rbd_dev->snap_name)
> +	rbd_dev->spec->snap_name = kmalloc(len + 1, GFP_KERNEL);
> +	if (!rbd_dev->spec->snap_name)
>   		goto out_mem;
> -	memcpy(rbd_dev->snap_name, buf, len);
> -	*(rbd_dev->snap_name + len) = '\0';
> +	memcpy(rbd_dev->spec->snap_name, buf, len);
> +	*(rbd_dev->spec->snap_name + len) = '\0';
>
>   	/* Initialize all rbd options to the defaults */
>
> @@ -2921,11 +2938,11 @@ static int rbd_add_parse_args(struct rbd_device
> *rbd_dev,
>   out_mem:
>   	ret = -ENOMEM;
>   out_err:
> -	kfree(rbd_dev->image_name);
> -	rbd_dev->image_name = NULL;
> -	rbd_dev->image_name_len = 0;
> -	kfree(rbd_dev->pool_name);
> -	rbd_dev->pool_name = NULL;
> +	kfree(rbd_dev->spec->image_name);
> +	rbd_dev->spec->image_name = NULL;
> +	rbd_dev->spec->image_name_len = 0;
> +	kfree(rbd_dev->spec->pool_name);
> +	rbd_dev->spec->pool_name = NULL;
>   	kfree(options);
>
>   	return ret;
> @@ -2957,11 +2974,11 @@ static int rbd_dev_image_id(struct rbd_device
> *rbd_dev)
>   	 * First, see if the format 2 image id file exists, and if
>   	 * so, get the image's persistent id from it.
>   	 */
> -	size = sizeof (RBD_ID_PREFIX) + rbd_dev->image_name_len;
> +	size = sizeof (RBD_ID_PREFIX) + rbd_dev->spec->image_name_len;
>   	object_name = kmalloc(size, GFP_NOIO);
>   	if (!object_name)
>   		return -ENOMEM;
> -	sprintf(object_name, "%s%s", RBD_ID_PREFIX, rbd_dev->image_name);
> +	sprintf(object_name, "%s%s", RBD_ID_PREFIX, rbd_dev->spec->image_name);
>   	dout("rbd id object name is %s\n", object_name);
>
>   	/* Response will be an encoded string, which includes a length */
> @@ -2984,15 +3001,15 @@ static int rbd_dev_image_id(struct rbd_device
> *rbd_dev)
>   	ret = 0;    /* rbd_req_sync_exec() can return positive */
>
>   	p = response;
> -	rbd_dev->image_id = ceph_extract_encoded_string(&p,
> +	rbd_dev->spec->image_id = ceph_extract_encoded_string(&p,
>   						p + RBD_IMAGE_ID_LEN_MAX,
> -						&rbd_dev->image_id_len,
> +						&rbd_dev->spec->image_id_len,
>   						GFP_NOIO);
> -	if (IS_ERR(rbd_dev->image_id)) {
> -		ret = PTR_ERR(rbd_dev->image_id);
> -		rbd_dev->image_id = NULL;
> +	if (IS_ERR(rbd_dev->spec->image_id)) {
> +		ret = PTR_ERR(rbd_dev->spec->image_id);
> +		rbd_dev->spec->image_id = NULL;
>   	} else {
> -		dout("image_id is %s\n", rbd_dev->image_id);
> +		dout("image_id is %s\n", rbd_dev->spec->image_id);
>   	}
>   out:
>   	kfree(response);
> @@ -3008,20 +3025,21 @@ static int rbd_dev_v1_probe(struct rbd_device
> *rbd_dev)
>
>   	/* Version 1 images have no id; empty string is used */
>
> -	rbd_dev->image_id = kstrdup("", GFP_KERNEL);
> -	if (!rbd_dev->image_id)
> +	rbd_dev->spec->image_id = kstrdup("", GFP_KERNEL);
> +	if (!rbd_dev->spec->image_id)
>   		return -ENOMEM;
> -	rbd_dev->image_id_len = 0;
> +	rbd_dev->spec->image_id_len = 0;
>
>   	/* Record the header object name for this rbd image. */
>
> -	size = rbd_dev->image_name_len + sizeof (RBD_SUFFIX);
> +	size = rbd_dev->spec->image_name_len + sizeof (RBD_SUFFIX);
>   	rbd_dev->header_name = kmalloc(size, GFP_KERNEL);
>   	if (!rbd_dev->header_name) {
>   		ret = -ENOMEM;
>   		goto out_err;
>   	}
> -	sprintf(rbd_dev->header_name, "%s%s", rbd_dev->image_name, RBD_SUFFIX);
> +	sprintf(rbd_dev->header_name, "%s%s",
> +		rbd_dev->spec->image_name, RBD_SUFFIX);
>
>   	/* Populate rbd image metadata */
>
> @@ -3038,8 +3056,8 @@ static int rbd_dev_v1_probe(struct rbd_device
> *rbd_dev)
>   out_err:
>   	kfree(rbd_dev->header_name);
>   	rbd_dev->header_name = NULL;
> -	kfree(rbd_dev->image_id);
> -	rbd_dev->image_id = NULL;
> +	kfree(rbd_dev->spec->image_id);
> +	rbd_dev->spec->image_id = NULL;
>
>   	return ret;
>   }
> @@ -3054,12 +3072,12 @@ static int rbd_dev_v2_probe(struct rbd_device
> *rbd_dev)
>   	 * Image id was filled in by the caller.  Record the header
>   	 * object name for this rbd image.
>   	 */
> -	size = sizeof (RBD_HEADER_PREFIX) + rbd_dev->image_id_len;
> +	size = sizeof (RBD_HEADER_PREFIX) + rbd_dev->spec->image_id_len;
>   	rbd_dev->header_name = kmalloc(size, GFP_KERNEL);
>   	if (!rbd_dev->header_name)
>   		return -ENOMEM;
>   	sprintf(rbd_dev->header_name, "%s%s",
> -			RBD_HEADER_PREFIX, rbd_dev->image_id);
> +			RBD_HEADER_PREFIX, rbd_dev->spec->image_id);
>
>   	/* Get the size and object order for the image */
>
> @@ -3147,6 +3165,9 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
>   	if (!rbd_dev)
>   		return -ENOMEM;
> +	rbd_dev->spec = kzalloc(sizeof (*rbd_dev->spec), GFP_KERNEL);
> +	if (!rbd_dev->spec)
> +		goto err_out_mem;
>
>   	/* static rbd_device initialization */
>   	spin_lock_init(&rbd_dev->lock);
> @@ -3167,10 +3188,10 @@ static ssize_t rbd_add(struct bus_type *bus,
>
>   	/* pick the pool */
>   	osdc = &rbd_dev->rbd_client->client->osdc;
> -	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name);
> +	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->spec->pool_name);
>   	if (rc < 0)
>   		goto err_out_client;
> -	rbd_dev->pool_id = (u64) rc;
> +	rbd_dev->spec->pool_id = (u64) rc;
>
>   	rc = rbd_dev_probe(rbd_dev);
>   	if (rc < 0)
> @@ -3257,15 +3278,16 @@ err_out_probe:
>   err_out_client:
>   	kfree(rbd_dev->header_name);
>   	rbd_put_client(rbd_dev);
> -	kfree(rbd_dev->image_id);
> +	kfree(rbd_dev->spec->image_id);
>   err_out_args:
>   	if (ceph_opts)
>   		ceph_destroy_options(ceph_opts);
> -	kfree(rbd_dev->snap_name);
> -	kfree(rbd_dev->image_name);
> -	kfree(rbd_dev->pool_name);
> +	kfree(rbd_dev->spec->snap_name);
> +	kfree(rbd_dev->spec->image_name);
> +	kfree(rbd_dev->spec->pool_name);
>   	kfree(rbd_opts);
>   err_out_mem:
> +	kfree(rbd_dev->spec);
>   	kfree(rbd_dev);
>
>   	dout("Error adding device %s\n", buf);
> @@ -3314,11 +3336,11 @@ static void rbd_dev_release(struct device *dev)
>   	rbd_header_free(&rbd_dev->header);
>
>   	/* done with the id, and with the rbd_dev */
> -	kfree(rbd_dev->snap_name);
> -	kfree(rbd_dev->image_id);
> +	kfree(rbd_dev->spec->snap_name);
> +	kfree(rbd_dev->spec->image_id);
>   	kfree(rbd_dev->header_name);
> -	kfree(rbd_dev->pool_name);
> -	kfree(rbd_dev->image_name);
> +	kfree(rbd_dev->spec->pool_name);
> +	kfree(rbd_dev->spec->image_name);
>   	rbd_dev_id_put(rbd_dev);
>   	kfree(rbd_dev);
>

Re: [PATCH 6/8] rbd: define image specification structure

[Alex Elder]

On 10/29/2012 05:13 PM, Josh Durgin wrote:
> A couple notes below, but looks good.

I responded to all of your notes below.  And I will update
the code/comments as appropriate after we have a chance
to talk about it but for the time being I'm going to
commit what I posted as-is.

					-Alex

> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
> 
> On 10/26/2012 04:03 PM, Alex Elder wrote:
>> Group the fields that uniquely specify an rbd image into a new
>> reference-counted rbd_spec structure.  This structure will be used
>> to describe the desired image when mapping an image, and when
>> probing parent images in layered rbd devices.  Replace the set of
>> fields in the rbd device structure with a pointer to a dynamically
>> allocated rbd_spec.
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |  158
>> +++++++++++++++++++++++++++++----------------------
>>   1 file changed, 90 insertions(+), 68 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 388dd47..c39e238 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -112,6 +112,27 @@ struct rbd_image_header {
>>       u64 obj_version;
>>   };
>>
>> +/*
>> + * An rbd image specification.
>> + *
>> + * The tuple (pool_id, image_id, snap_id) is sufficient to uniquely
>> + * identify an image.
>> + */
> 
> This looks like it's meant to be immutable. If so, it'd be nice
> to have that in the comment.

I want to be sure we agree on what that term means before saying
either way.

My aim in encapsulating this was to have a single thing represent
the identity of an image.  I started with just the pool, image,
and snapshot id's, but after a bit decided the names really
belonged there too.  I think the name<->id association can in
some cases change.

Also, for a given image, the id never changes, but a parent
will be specified using this, and that can change.

>> +struct rbd_spec {
>> +    u64        pool_id;
>> +    char        *pool_name;
>> +
>> +    char        *image_id;
>> +    size_t        image_id_len;
>> +    char        *image_name;
>> +    size_t        image_name_len;
> 
> I realize you're just rearranging things in this patch, but it's a bit
> odd that only image_id and image_name have corresponding lengths stored.
> Those fields don't seem necessary.

That's somewhat of an artifact, carrying along what was
already there, and grouping them like this makes it more
obvious those two were different.

Both the image id and image name lengths are now used one
place and it's basically one time only--in the probe routine.
So it is not valuable to keep them around.  I will remove
them in a future patch.

>> +
>> +    u64        snap_id;
>> +    char        *snap_name;
>> +
>> +    struct kref    kref;
>> +};
>> +
>>   struct rbd_options {
>>       bool    read_only;
>>   };
>> @@ -189,16 +210,9 @@ struct rbd_device {
>>
>>       struct rbd_image_header    header;
>>       bool                    exists;
>> -    char            *image_id;
>> -    size_t            image_id_len;
>> -    char            *image_name;
>> -    size_t            image_name_len;
>> -    char            *header_name;
>> -    char            *pool_name;
>> -    u64            pool_id;
>> +    struct rbd_spec        *spec;
>>
>> -    char                    *snap_name;
>> -    u64                     snap_id;
>> +    char            *header_name;
> 
> Are you planning to split out more stuff into a common
> struct rbd_image, like header_name, exists, etc?
> There's a bunch of stuff that's not specific to a particular rbd_device
> in here.

Possibly.  Do you mean to distinguish the Linux device side
from the rados storage side?  Let's talk about this today.

>>
>>       struct ceph_osd_event   *watch_event;
>>       struct ceph_osd_request *watch_request;
>> @@ -654,7 +668,7 @@ static int snap_by_name(struct rbd_device *rbd_dev,
>> const char *snap_name)
>>
>>       list_for_each_entry(snap, &rbd_dev->snaps, node) {
>>           if (!strcmp(snap_name, snap->name)) {
>> -            rbd_dev->snap_id = snap->id;
>> +            rbd_dev->spec->snap_id = snap->id;
>>               rbd_dev->mapping.size = snap->size;
>>               rbd_dev->mapping.features = snap->features;
>>

. . .

Re: [PATCH 6/8] rbd: define image specification structure

[Josh Durgin]

On 10/30/2012 05:40 AM, Alex Elder wrote:
> On 10/29/2012 05:13 PM, Josh Durgin wrote:
>> A couple notes below, but looks good.
>
> I responded to all of your notes below.  And I will update
> the code/comments as appropriate after we have a chance
> to talk about it but for the time being I'm going to
> commit what I posted as-is.
>
> 					-Alex
>
>> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
>>
>> On 10/26/2012 04:03 PM, Alex Elder wrote:
>>> Group the fields that uniquely specify an rbd image into a new
>>> reference-counted rbd_spec structure.  This structure will be used
>>> to describe the desired image when mapping an image, and when
>>> probing parent images in layered rbd devices.  Replace the set of
>>> fields in the rbd device structure with a pointer to a dynamically
>>> allocated rbd_spec.
>>>
>>> Signed-off-by: Alex Elder <elder@inktank.com>
>>> ---
>>>    drivers/block/rbd.c |  158
>>> +++++++++++++++++++++++++++++----------------------
>>>    1 file changed, 90 insertions(+), 68 deletions(-)
>>>
>>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>>> index 388dd47..c39e238 100644
>>> --- a/drivers/block/rbd.c
>>> +++ b/drivers/block/rbd.c
>>> @@ -112,6 +112,27 @@ struct rbd_image_header {
>>>        u64 obj_version;
>>>    };
>>>
>>> +/*
>>> + * An rbd image specification.
>>> + *
>>> + * The tuple (pool_id, image_id, snap_id) is sufficient to uniquely
>>> + * identify an image.
>>> + */
>>
>> This looks like it's meant to be immutable. If so, it'd be nice
>> to have that in the comment.
>
> I want to be sure we agree on what that term means before saying
> either way.
>
> My aim in encapsulating this was to have a single thing represent
> the identity of an image.  I started with just the pool, image,
> and snapshot id's, but after a bit decided the names really
> belonged there too.  I think the name<->id association can in
> some cases change.
>
> Also, for a given image, the id never changes, but a parent
> will be specified using this, and that can change.

When I was asking about immutability, I was wondering whether access to
the fields inside an rbd_spec would need to be synchronized. If those
fields won't change, it'd be good to document that fact. Otherwise
document how they'll be accessed.

>>> +struct rbd_spec {
>>> +    u64        pool_id;
>>> +    char        *pool_name;
>>> +
>>> +    char        *image_id;
>>> +    size_t        image_id_len;
>>> +    char        *image_name;
>>> +    size_t        image_name_len;
>>
>> I realize you're just rearranging things in this patch, but it's a bit
>> odd that only image_id and image_name have corresponding lengths stored.
>> Those fields don't seem necessary.
>
> That's somewhat of an artifact, carrying along what was
> already there, and grouping them like this makes it more
> obvious those two were different.
>
> Both the image id and image name lengths are now used one
> place and it's basically one time only--in the probe routine.
> So it is not valuable to keep them around.  I will remove
> them in a future patch.

Sounds good.

>>> +
>>> +    u64        snap_id;
>>> +    char        *snap_name;
>>> +
>>> +    struct kref    kref;
>>> +};
>>> +
>>>    struct rbd_options {
>>>        bool    read_only;
>>>    };
>>> @@ -189,16 +210,9 @@ struct rbd_device {
>>>
>>>        struct rbd_image_header    header;
>>>        bool                    exists;
>>> -    char            *image_id;
>>> -    size_t            image_id_len;
>>> -    char            *image_name;
>>> -    size_t            image_name_len;
>>> -    char            *header_name;
>>> -    char            *pool_name;
>>> -    u64            pool_id;
>>> +    struct rbd_spec        *spec;
>>>
>>> -    char                    *snap_name;
>>> -    u64                     snap_id;
>>> +    char            *header_name;
>>
>> Are you planning to split out more stuff into a common
>> struct rbd_image, like header_name, exists, etc?
>> There's a bunch of stuff that's not specific to a particular rbd_device
>> in here.
>
> Possibly.  Do you mean to distinguish the Linux device side
> from the rados storage side?  Let's talk about this today.

Yes that's what I meant. We talked about this, and decided
this separation isn't needed yet, and may not be needed to
implement layering.

>>>
>>>        struct ceph_osd_event   *watch_event;
>>>        struct ceph_osd_request *watch_request;
>>> @@ -654,7 +668,7 @@ static int snap_by_name(struct rbd_device *rbd_dev,
>>> const char *snap_name)
>>>
>>>        list_for_each_entry(snap, &rbd_dev->snaps, node) {
>>>            if (!strcmp(snap_name, snap->name)) {
>>> -            rbd_dev->snap_id = snap->id;
>>> +            rbd_dev->spec->snap_id = snap->id;
>>>                rbd_dev->mapping.size = snap->size;
>>>                rbd_dev->mapping.features = snap->features;
>>>
>
> . . .
>

[PATCH 7/8] rbd: add reference counting to rbd_spec

[Alex Elder]

With layered images we'll share rbd_spec structures, so add a
reference count to it.  It neatens up some code also.

A silly get/put pair is added to the alloc routine just to avoid
"defined but not used" warnings.  It will go away soon.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index c39e238..19c7c6b 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2134,6 +2134,45 @@ static struct device_type rbd_snap_device_type = {
 	.release	= rbd_snap_dev_release,
 };

+static struct rbd_spec *rbd_spec_get(struct rbd_spec *spec)
+{
+	kref_get(&spec->kref);
+
+	return spec;
+}
+
+static void rbd_spec_free(struct kref *kref);
+static void rbd_spec_put(struct rbd_spec *spec)
+{
+	if (spec)
+		kref_put(&spec->kref, rbd_spec_free);
+}
+
+static struct rbd_spec *rbd_spec_alloc(void)
+{
+	struct rbd_spec *spec;
+
+	spec = kzalloc(sizeof (*spec), GFP_KERNEL);
+	if (!spec)
+		return NULL;
+	kref_init(&spec->kref);
+
+	rbd_spec_put(rbd_spec_get(spec));	/* TEMPORARY */
+
+	return spec;
+}
+
+static void rbd_spec_free(struct kref *kref)
+{
+	struct rbd_spec *spec = container_of(kref, struct rbd_spec, kref);
+
+	kfree(spec->pool_name);
+	kfree(spec->image_id);
+	kfree(spec->image_name);
+	kfree(spec->snap_name);
+	kfree(spec);
+}
+
 static bool rbd_snap_registered(struct rbd_snap *snap)
 {
 	bool ret = snap->dev.type == &rbd_snap_device_type;
@@ -3165,7 +3204,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
 	if (!rbd_dev)
 		return -ENOMEM;
-	rbd_dev->spec = kzalloc(sizeof (*rbd_dev->spec), GFP_KERNEL);
+	rbd_dev->spec = rbd_spec_alloc();
 	if (!rbd_dev->spec)
 		goto err_out_mem;

@@ -3278,16 +3317,12 @@ err_out_probe:
 err_out_client:
 	kfree(rbd_dev->header_name);
 	rbd_put_client(rbd_dev);
-	kfree(rbd_dev->spec->image_id);
 err_out_args:
 	if (ceph_opts)
 		ceph_destroy_options(ceph_opts);
-	kfree(rbd_dev->spec->snap_name);
-	kfree(rbd_dev->spec->image_name);
-	kfree(rbd_dev->spec->pool_name);
 	kfree(rbd_opts);
 err_out_mem:
-	kfree(rbd_dev->spec);
+	rbd_spec_put(rbd_dev->spec);
 	kfree(rbd_dev);

 	dout("Error adding device %s\n", buf);
@@ -3336,12 +3371,9 @@ static void rbd_dev_release(struct device *dev)
 	rbd_header_free(&rbd_dev->header);

 	/* done with the id, and with the rbd_dev */
-	kfree(rbd_dev->spec->snap_name);
-	kfree(rbd_dev->spec->image_id);
 	kfree(rbd_dev->header_name);
-	kfree(rbd_dev->spec->pool_name);
-	kfree(rbd_dev->spec->image_name);
 	rbd_dev_id_put(rbd_dev);
+	rbd_spec_put(rbd_dev->spec);
 	kfree(rbd_dev);

 	/* release module ref */
-- 
1.7.9.5

Re: [PATCH 7/8] rbd: add reference counting to rbd_spec

[Josh Durgin]

On 10/26/2012 04:03 PM, Alex Elder wrote:
> With layered images we'll share rbd_spec structures, so add a
> reference count to it.  It neatens up some code also.

Could you explain your plan for these data structures? What
will the structs and their relationships look like with
clones mapped?

> A silly get/put pair is added to the alloc routine just to avoid
> "defined but not used" warnings.  It will go away soon.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   52
> +++++++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index c39e238..19c7c6b 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2134,6 +2134,45 @@ static struct device_type rbd_snap_device_type = {
>   	.release	= rbd_snap_dev_release,
>   };
>
> +static struct rbd_spec *rbd_spec_get(struct rbd_spec *spec)
> +{
> +	kref_get(&spec->kref);
> +
> +	return spec;
> +}
> +
> +static void rbd_spec_free(struct kref *kref);
> +static void rbd_spec_put(struct rbd_spec *spec)
> +{
> +	if (spec)
> +		kref_put(&spec->kref, rbd_spec_free);
> +}
> +
> +static struct rbd_spec *rbd_spec_alloc(void)
> +{
> +	struct rbd_spec *spec;
> +
> +	spec = kzalloc(sizeof (*spec), GFP_KERNEL);
> +	if (!spec)
> +		return NULL;
> +	kref_init(&spec->kref);
> +
> +	rbd_spec_put(rbd_spec_get(spec));	/* TEMPORARY */
> +
> +	return spec;
> +}
> +
> +static void rbd_spec_free(struct kref *kref)
> +{
> +	struct rbd_spec *spec = container_of(kref, struct rbd_spec, kref);
> +
> +	kfree(spec->pool_name);
> +	kfree(spec->image_id);
> +	kfree(spec->image_name);
> +	kfree(spec->snap_name);
> +	kfree(spec);
> +}
> +
>   static bool rbd_snap_registered(struct rbd_snap *snap)
>   {
>   	bool ret = snap->dev.type == &rbd_snap_device_type;
> @@ -3165,7 +3204,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
>   	if (!rbd_dev)
>   		return -ENOMEM;
> -	rbd_dev->spec = kzalloc(sizeof (*rbd_dev->spec), GFP_KERNEL);
> +	rbd_dev->spec = rbd_spec_alloc();
>   	if (!rbd_dev->spec)
>   		goto err_out_mem;
>
> @@ -3278,16 +3317,12 @@ err_out_probe:
>   err_out_client:
>   	kfree(rbd_dev->header_name);
>   	rbd_put_client(rbd_dev);
> -	kfree(rbd_dev->spec->image_id);
>   err_out_args:
>   	if (ceph_opts)
>   		ceph_destroy_options(ceph_opts);
> -	kfree(rbd_dev->spec->snap_name);
> -	kfree(rbd_dev->spec->image_name);
> -	kfree(rbd_dev->spec->pool_name);
>   	kfree(rbd_opts);
>   err_out_mem:
> -	kfree(rbd_dev->spec);
> +	rbd_spec_put(rbd_dev->spec);
>   	kfree(rbd_dev);
>
>   	dout("Error adding device %s\n", buf);
> @@ -3336,12 +3371,9 @@ static void rbd_dev_release(struct device *dev)
>   	rbd_header_free(&rbd_dev->header);
>
>   	/* done with the id, and with the rbd_dev */
> -	kfree(rbd_dev->spec->snap_name);
> -	kfree(rbd_dev->spec->image_id);
>   	kfree(rbd_dev->header_name);
> -	kfree(rbd_dev->spec->pool_name);
> -	kfree(rbd_dev->spec->image_name);
>   	rbd_dev_id_put(rbd_dev);
> +	rbd_spec_put(rbd_dev->spec);
>   	kfree(rbd_dev);
>
>   	/* release module ref */
>

Re: [PATCH 7/8] rbd: add reference counting to rbd_spec

[Alex Elder]

On 10/29/2012 05:20 PM, Josh Durgin wrote:
> On 10/26/2012 04:03 PM, Alex Elder wrote:
>> With layered images we'll share rbd_spec structures, so add a
>> reference count to it.  It neatens up some code also.
> 
> Could you explain your plan for these data structures? What
> will the structs and their relationships look like with
> clones mapped?

Like I mentioned in the previous message, this structure
encapsulates the information that identifies an rbd image.

In the next patch, the information defining the image to
be mapped is extracted from "command line" arguments to
populate the content of one of these in the argument
parsing routine.

In an upcoming patch, some create/destroy helper routines
will be defined for an rbd_device structure. To create an
rbd device, you provide an initialized (activated?)
rbd_client (which includes a ceph client) and the identity
of an image you want that rbd_device to map using that client.

So in response to a map request via /sys/bus/rbd/add, a
new rbd_client will be created in that way.  Later, a
mapped rbd image (v2) will continue its probing activity
by probing its parent, and if it has one, an rbd_device
structure will be created for that purpose, using the
same client as the original/child image and the image
specification fetched from the child for the parent.

I suppose it's not obvious at this point why reference
counting is needed.  It's one of those things that is
needed later on (in work that's well underway but which
I have not posted yet), and this turns out to be a
good time to put that in place so it's there when it's
needed, shortly.

In any case, the reference counting here arises because
the image spec for a parent will be created and filled in
while probing the child.  But I want to provide that image
spec for creating the parent rbd_device structure, and
keeping track of who owned what got a little confusing
so I decided that just sharing a reference counted
structure was the best way to go.

					-Alex

>> A silly get/put pair is added to the alloc routine just to avoid
>> "defined but not used" warnings.  It will go away soon.
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |   52
>> +++++++++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 42 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index c39e238..19c7c6b 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -2134,6 +2134,45 @@ static struct device_type rbd_snap_device_type = {
>>       .release    = rbd_snap_dev_release,
>>   };
>>
>> +static struct rbd_spec *rbd_spec_get(struct rbd_spec *spec)
>> +{
>> +    kref_get(&spec->kref);
>> +
>> +    return spec;
>> +}
>> +
>> +static void rbd_spec_free(struct kref *kref);
>> +static void rbd_spec_put(struct rbd_spec *spec)
>> +{
>> +    if (spec)
>> +        kref_put(&spec->kref, rbd_spec_free);
>> +}
>> +
>> +static struct rbd_spec *rbd_spec_alloc(void)
>> +{
>> +    struct rbd_spec *spec;
>> +
>> +    spec = kzalloc(sizeof (*spec), GFP_KERNEL);
>> +    if (!spec)
>> +        return NULL;
>> +    kref_init(&spec->kref);
>> +
>> +    rbd_spec_put(rbd_spec_get(spec));    /* TEMPORARY */
>> +
>> +    return spec;
>> +}
>> +
>> +static void rbd_spec_free(struct kref *kref)
>> +{
>> +    struct rbd_spec *spec = container_of(kref, struct rbd_spec, kref);
>> +
>> +    kfree(spec->pool_name);
>> +    kfree(spec->image_id);
>> +    kfree(spec->image_name);
>> +    kfree(spec->snap_name);
>> +    kfree(spec);
>> +}
>> +
>>   static bool rbd_snap_registered(struct rbd_snap *snap)
>>   {
>>       bool ret = snap->dev.type == &rbd_snap_device_type;
>> @@ -3165,7 +3204,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>>       rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
>>       if (!rbd_dev)
>>           return -ENOMEM;
>> -    rbd_dev->spec = kzalloc(sizeof (*rbd_dev->spec), GFP_KERNEL);
>> +    rbd_dev->spec = rbd_spec_alloc();
>>       if (!rbd_dev->spec)
>>           goto err_out_mem;
>>
>> @@ -3278,16 +3317,12 @@ err_out_probe:
>>   err_out_client:
>>       kfree(rbd_dev->header_name);
>>       rbd_put_client(rbd_dev);
>> -    kfree(rbd_dev->spec->image_id);
>>   err_out_args:
>>       if (ceph_opts)
>>           ceph_destroy_options(ceph_opts);
>> -    kfree(rbd_dev->spec->snap_name);
>> -    kfree(rbd_dev->spec->image_name);
>> -    kfree(rbd_dev->spec->pool_name);
>>       kfree(rbd_opts);
>>   err_out_mem:
>> -    kfree(rbd_dev->spec);
>> +    rbd_spec_put(rbd_dev->spec);
>>       kfree(rbd_dev);
>>
>>       dout("Error adding device %s\n", buf);
>> @@ -3336,12 +3371,9 @@ static void rbd_dev_release(struct device *dev)
>>       rbd_header_free(&rbd_dev->header);
>>
>>       /* done with the id, and with the rbd_dev */
>> -    kfree(rbd_dev->spec->snap_name);
>> -    kfree(rbd_dev->spec->image_id);
>>       kfree(rbd_dev->header_name);
>> -    kfree(rbd_dev->spec->pool_name);
>> -    kfree(rbd_dev->spec->image_name);
>>       rbd_dev_id_put(rbd_dev);
>> +    rbd_spec_put(rbd_dev->spec);
>>       kfree(rbd_dev);
>>
>>       /* release module ref */
>>
> 

Re: [PATCH 7/8] rbd: add reference counting to rbd_spec

[Josh Durgin]

On 10/30/2012 05:59 AM, Alex Elder wrote:
> On 10/29/2012 05:20 PM, Josh Durgin wrote:
>> On 10/26/2012 04:03 PM, Alex Elder wrote:
>>> With layered images we'll share rbd_spec structures, so add a
>>> reference count to it.  It neatens up some code also.
>>
>> Could you explain your plan for these data structures? What
>> will the structs and their relationships look like with
>> clones mapped?
>
> Like I mentioned in the previous message, this structure
> encapsulates the information that identifies an rbd image.
>
> In the next patch, the information defining the image to
> be mapped is extracted from "command line" arguments to
> populate the content of one of these in the argument
> parsing routine.
>
> In an upcoming patch, some create/destroy helper routines
> will be defined for an rbd_device structure. To create an
> rbd device, you provide an initialized (activated?)
> rbd_client (which includes a ceph client) and the identity
> of an image you want that rbd_device to map using that client.
>
> So in response to a map request via /sys/bus/rbd/add, a
> new rbd_client will be created in that way.  Later, a
> mapped rbd image (v2) will continue its probing activity
> by probing its parent, and if it has one, an rbd_device
> structure will be created for that purpose, using the
> same client as the original/child image and the image
> specification fetched from the child for the parent.
>
> I suppose it's not obvious at this point why reference
> counting is needed.  It's one of those things that is
> needed later on (in work that's well underway but which
> I have not posted yet), and this turns out to be a
> good time to put that in place so it's there when it's
> needed, shortly.
>
> In any case, the reference counting here arises because
> the image spec for a parent will be created and filled in
> while probing the child.  But I want to provide that image
> spec for creating the parent rbd_device structure, and
> keeping track of who owned what got a little confusing
> so I decided that just sharing a reference counted
> structure was the best way to go.

This makes sense now, thanks.

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

> 					-Alex
>
>>> A silly get/put pair is added to the alloc routine just to avoid
>>> "defined but not used" warnings.  It will go away soon.
>>>
>>> Signed-off-by: Alex Elder <elder@inktank.com>
>>> ---
>>>    drivers/block/rbd.c |   52
>>> +++++++++++++++++++++++++++++++++++++++++----------
>>>    1 file changed, 42 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>>> index c39e238..19c7c6b 100644
>>> --- a/drivers/block/rbd.c
>>> +++ b/drivers/block/rbd.c
>>> @@ -2134,6 +2134,45 @@ static struct device_type rbd_snap_device_type = {
>>>        .release    = rbd_snap_dev_release,
>>>    };
>>>
>>> +static struct rbd_spec *rbd_spec_get(struct rbd_spec *spec)
>>> +{
>>> +    kref_get(&spec->kref);
>>> +
>>> +    return spec;
>>> +}
>>> +
>>> +static void rbd_spec_free(struct kref *kref);
>>> +static void rbd_spec_put(struct rbd_spec *spec)
>>> +{
>>> +    if (spec)
>>> +        kref_put(&spec->kref, rbd_spec_free);
>>> +}
>>> +
>>> +static struct rbd_spec *rbd_spec_alloc(void)
>>> +{
>>> +    struct rbd_spec *spec;
>>> +
>>> +    spec = kzalloc(sizeof (*spec), GFP_KERNEL);
>>> +    if (!spec)
>>> +        return NULL;
>>> +    kref_init(&spec->kref);
>>> +
>>> +    rbd_spec_put(rbd_spec_get(spec));    /* TEMPORARY */
>>> +
>>> +    return spec;
>>> +}
>>> +
>>> +static void rbd_spec_free(struct kref *kref)
>>> +{
>>> +    struct rbd_spec *spec = container_of(kref, struct rbd_spec, kref);
>>> +
>>> +    kfree(spec->pool_name);
>>> +    kfree(spec->image_id);
>>> +    kfree(spec->image_name);
>>> +    kfree(spec->snap_name);
>>> +    kfree(spec);
>>> +}
>>> +
>>>    static bool rbd_snap_registered(struct rbd_snap *snap)
>>>    {
>>>        bool ret = snap->dev.type == &rbd_snap_device_type;
>>> @@ -3165,7 +3204,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>>>        rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
>>>        if (!rbd_dev)
>>>            return -ENOMEM;
>>> -    rbd_dev->spec = kzalloc(sizeof (*rbd_dev->spec), GFP_KERNEL);
>>> +    rbd_dev->spec = rbd_spec_alloc();
>>>        if (!rbd_dev->spec)
>>>            goto err_out_mem;
>>>
>>> @@ -3278,16 +3317,12 @@ err_out_probe:
>>>    err_out_client:
>>>        kfree(rbd_dev->header_name);
>>>        rbd_put_client(rbd_dev);
>>> -    kfree(rbd_dev->spec->image_id);
>>>    err_out_args:
>>>        if (ceph_opts)
>>>            ceph_destroy_options(ceph_opts);
>>> -    kfree(rbd_dev->spec->snap_name);
>>> -    kfree(rbd_dev->spec->image_name);
>>> -    kfree(rbd_dev->spec->pool_name);
>>>        kfree(rbd_opts);
>>>    err_out_mem:
>>> -    kfree(rbd_dev->spec);
>>> +    rbd_spec_put(rbd_dev->spec);
>>>        kfree(rbd_dev);
>>>
>>>        dout("Error adding device %s\n", buf);
>>> @@ -3336,12 +3371,9 @@ static void rbd_dev_release(struct device *dev)
>>>        rbd_header_free(&rbd_dev->header);
>>>
>>>        /* done with the id, and with the rbd_dev */
>>> -    kfree(rbd_dev->spec->snap_name);
>>> -    kfree(rbd_dev->spec->image_id);
>>>        kfree(rbd_dev->header_name);
>>> -    kfree(rbd_dev->spec->pool_name);
>>> -    kfree(rbd_dev->spec->image_name);
>>>        rbd_dev_id_put(rbd_dev);
>>> +    rbd_spec_put(rbd_dev->spec);
>>>        kfree(rbd_dev);
>>>
>>>        /* release module ref */
>>>
>>

[PATCH 8/8] rbd: fill rbd_spec in rbd_add_parse_args()

[Alex Elder]

Pass the address of an rbd_spec structure to rbd_add_parse_args().
Use it to hold the information defining the rbd image to be mapped
in an rbd_add() call.

Use the result in the caller to initialize the rbd_dev->id field.

This means rbd_dev is no longer needed in rbd_add_parse_args(),
so get rid of it.

Now that this transformation of rbd_add_parse_args() is complete,
correct and expand on the its header documentation to reflect the
new reality.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 19c7c6b..28abd31 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2887,21 +2887,53 @@ static inline char *dup_token(const char **buf,
size_t *lenp)
 }

 /*
- * This fills in the pool_name, image_name, image_name_len, rbd_dev,
- * rbd_md_name, and name fields of the given rbd_dev, based on the
- * list of monitor addresses and other options provided via
- * /sys/bus/rbd/add.  Returns a pointer to a dynamically-allocated
- * copy of the snapshot name to map if successful, or a
- * pointer-coded error otherwise.
+ * Parse the options provided for an "rbd add" (i.e., rbd image
+ * mapping) request.  These arrive via a write to /sys/bus/rbd/add,
+ * and the data written is passed here via a NUL-terminated buffer.
+ * Returns 0 if successful or an error code otherwise.
  *
- * Note: rbd_dev is assumed to have been initially zero-filled.
+ * The information extracted from these options is recorded in
+ * the other parameters which return dynamically-allocated
+ * structures:
+ *  ceph_opts
+ *      The address of a pointer that will refer to a ceph options
+ *      structure.  Caller must release the returned pointer using
+ *      ceph_destroy_options() when it is no longer needed.
+ *  rbd_opts
+ *  	Address of an rbd options pointer.  Fully initialized by
+ *  	this function; caller must release with kfree().
+ *  spec
+ *  	Address of an rbd image specification pointer.  Fully
+ *  	initialized by this function based on parsed options.
+ *	Caller must release with kfree().
+ *
+ * The options passed take this form:
+ *  <mon_addrs> <options> <pool_name> <image_name> [<snap_id>]
+ * where:
+ *  <mon_addrs>
+ *      A comma-separated list of one or more monitor addresses.
+ *      A monitor address is an ip address, optionally followed
+ *      by a port number (separated by a colon).
+ *        I.e.:  ip1[:port1][,ip2[:port2]...]
+ *  <options>
+ *      A comma-separated list of ceph and/or rbd options.
+ *  <pool_name>
+ *      The name of the rados pool containing the rbd image.
+ *  <image_name>
+ *      The name of the image in that pool to map.
+ *  <snap_id>
+ *      An optional snapshot id.  If provided, the mapping will
+ *      present data from the image at the time that snapshot was
+ *      created.  The image head is used if no snapshot id is
+ *      provided.  Snapshot mappings are always read-only.
  */
-static int rbd_add_parse_args(struct rbd_device *rbd_dev,
-				const char *buf,
+static int rbd_add_parse_args(const char *buf,
 				struct ceph_options **ceph_opts,
-				struct rbd_options **opts)
+				struct rbd_options **opts,
+				struct rbd_spec **rbd_spec)
 {
 	size_t len;
+	struct rbd_spec *spec;
 	const char *mon_addrs;
 	size_t mon_addrs_size;
 	char *options;
@@ -2917,24 +2949,27 @@ static int rbd_add_parse_args(struct rbd_device
*rbd_dev,
 	mon_addrs_size = len + 1;
 	buf += len;

+	spec = rbd_spec_alloc();
+	if (!spec)
+		return -ENOMEM;
+
 	ret = -EINVAL;
 	options = dup_token(&buf, NULL);
 	if (!options)
-		return -ENOMEM;
+		goto out_mem;
 	if (!*options)
 		goto out_err;	/* Missing options */

-	rbd_dev->spec->pool_name = dup_token(&buf, NULL);
-	if (!rbd_dev->spec->pool_name)
+	spec->pool_name = dup_token(&buf, NULL);
+	if (!spec->pool_name)
 		goto out_mem;
-	if (!*rbd_dev->spec->pool_name)
+	if (!*spec->pool_name)
 		goto out_err;	/* Missing pool name */

-	rbd_dev->spec->image_name =
-		dup_token(&buf, &rbd_dev->spec->image_name_len);
-	if (!rbd_dev->spec->image_name)
+	spec->image_name = dup_token(&buf, &spec->image_name_len);
+	if (!spec->image_name)
 		goto out_mem;
-	if (!*rbd_dev->spec->image_name)
+	if (!*spec->image_name)
 		goto out_err;	/* Missing image name */

 	/*
@@ -2949,11 +2984,11 @@ static int rbd_add_parse_args(struct rbd_device
*rbd_dev,
 		ret = -ENAMETOOLONG;
 		goto out_err;
 	}
-	rbd_dev->spec->snap_name = kmalloc(len + 1, GFP_KERNEL);
-	if (!rbd_dev->spec->snap_name)
+	spec->snap_name = kmalloc(len + 1, GFP_KERNEL);
+	if (!spec->snap_name)
 		goto out_mem;
-	memcpy(rbd_dev->spec->snap_name, buf, len);
-	*(rbd_dev->spec->snap_name + len) = '\0';
+	memcpy(spec->snap_name, buf, len);
+	*(spec->snap_name + len) = '\0';

 	/* Initialize all rbd options to the defaults */

@@ -2973,15 +3008,15 @@ static int rbd_add_parse_args(struct rbd_device
*rbd_dev,
 	}
 	*opts = rbd_opts;

+	*rbd_spec = spec;
+
 	return 0;
 out_mem:
 	ret = -ENOMEM;
+	kfree(spec->image_name);
+	kfree(spec->pool_name);
+	kfree(spec);
 out_err:
-	kfree(rbd_dev->spec->image_name);
-	rbd_dev->spec->image_name = NULL;
-	rbd_dev->spec->image_name_len = 0;
-	kfree(rbd_dev->spec->pool_name);
-	rbd_dev->spec->pool_name = NULL;
 	kfree(options);

 	return ret;
@@ -3195,6 +3230,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	struct rbd_device *rbd_dev = NULL;
 	struct ceph_options *ceph_opts = NULL;
 	struct rbd_options *rbd_opts = NULL;
+	struct rbd_spec *spec = NULL;
 	struct ceph_osd_client *osdc;
 	int rc = -ENOMEM;

@@ -3204,9 +3240,6 @@ static ssize_t rbd_add(struct bus_type *bus,
 	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
 	if (!rbd_dev)
 		return -ENOMEM;
-	rbd_dev->spec = rbd_spec_alloc();
-	if (!rbd_dev->spec)
-		goto err_out_mem;

 	/* static rbd_device initialization */
 	spin_lock_init(&rbd_dev->lock);
@@ -3215,9 +3248,10 @@ static ssize_t rbd_add(struct bus_type *bus,
 	init_rwsem(&rbd_dev->header_rwsem);

 	/* parse add command */
-	rc = rbd_add_parse_args(rbd_dev, buf, &ceph_opts, &rbd_opts);
+	rc = rbd_add_parse_args(buf, &ceph_opts, &rbd_opts, &spec);
 	if (rc < 0)
 		goto err_out_mem;
+
 	rbd_dev->mapping.read_only = rbd_opts->read_only;

 	rc = rbd_get_client(rbd_dev, ceph_opts);
@@ -3227,10 +3261,12 @@ static ssize_t rbd_add(struct bus_type *bus,

 	/* pick the pool */
 	osdc = &rbd_dev->rbd_client->client->osdc;
-	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->spec->pool_name);
+	rc = ceph_pg_poolid_by_name(osdc->osdmap, spec->pool_name);
 	if (rc < 0)
 		goto err_out_client;
-	rbd_dev->spec->pool_id = (u64) rc;
+	spec->pool_id = (u64) rc;
+
+	rbd_dev->spec = spec;

 	rc = rbd_dev_probe(rbd_dev);
 	if (rc < 0)
@@ -3321,8 +3357,8 @@ err_out_args:
 	if (ceph_opts)
 		ceph_destroy_options(ceph_opts);
 	kfree(rbd_opts);
+	rbd_spec_put(spec);
 err_out_mem:
-	rbd_spec_put(rbd_dev->spec);
 	kfree(rbd_dev);

 	dout("Error adding device %s\n", buf);
-- 
1.7.9.5

Re: [PATCH 8/8] rbd: fill rbd_spec in rbd_add_parse_args()

[Josh Durgin]

On 10/26/2012 04:03 PM, Alex Elder wrote:
> Pass the address of an rbd_spec structure to rbd_add_parse_args().
> Use it to hold the information defining the rbd image to be mapped
> in an rbd_add() call.
>
> Use the result in the caller to initialize the rbd_dev->id field.
>
> This means rbd_dev is no longer needed in rbd_add_parse_args(),
> so get rid of it.
>
> Now that this transformation of rbd_add_parse_args() is complete,
> correct and expand on the its header documentation to reflect the
> new reality.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |  104
> ++++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 70 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 19c7c6b..28abd31 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2887,21 +2887,53 @@ static inline char *dup_token(const char **buf,
> size_t *lenp)
>   }
>
>   /*
> - * This fills in the pool_name, image_name, image_name_len, rbd_dev,
> - * rbd_md_name, and name fields of the given rbd_dev, based on the
> - * list of monitor addresses and other options provided via
> - * /sys/bus/rbd/add.  Returns a pointer to a dynamically-allocated
> - * copy of the snapshot name to map if successful, or a
> - * pointer-coded error otherwise.
> + * Parse the options provided for an "rbd add" (i.e., rbd image
> + * mapping) request.  These arrive via a write to /sys/bus/rbd/add,
> + * and the data written is passed here via a NUL-terminated buffer.
> + * Returns 0 if successful or an error code otherwise.
>    *
> - * Note: rbd_dev is assumed to have been initially zero-filled.
> + * The information extracted from these options is recorded in
> + * the other parameters which return dynamically-allocated
> + * structures:
> + *  ceph_opts
> + *      The address of a pointer that will refer to a ceph options
> + *      structure.  Caller must release the returned pointer using
> + *      ceph_destroy_options() when it is no longer needed.
> + *  rbd_opts
> + *  	Address of an rbd options pointer.  Fully initialized by
> + *  	this function; caller must release with kfree().
> + *  spec
> + *  	Address of an rbd image specification pointer.  Fully
> + *  	initialized by this function based on parsed options.
> + *	Caller must release with kfree().
> + *
> + * The options passed take this form:
> + *  <mon_addrs> <options> <pool_name> <image_name> [<snap_id>]
> + * where:
> + *  <mon_addrs>
> + *      A comma-separated list of one or more monitor addresses.
> + *      A monitor address is an ip address, optionally followed
> + *      by a port number (separated by a colon).
> + *        I.e.:  ip1[:port1][,ip2[:port2]...]
> + *  <options>
> + *      A comma-separated list of ceph and/or rbd options.
> + *  <pool_name>
> + *      The name of the rados pool containing the rbd image.
> + *  <image_name>
> + *      The name of the image in that pool to map.
> + *  <snap_id>
> + *      An optional snapshot id.  If provided, the mapping will
> + *      present data from the image at the time that snapshot was
> + *      created.  The image head is used if no snapshot id is
> + *      provided.  Snapshot mappings are always read-only.
>    */
> -static int rbd_add_parse_args(struct rbd_device *rbd_dev,
> -				const char *buf,
> +static int rbd_add_parse_args(const char *buf,
>   				struct ceph_options **ceph_opts,
> -				struct rbd_options **opts)
> +				struct rbd_options **opts,
> +				struct rbd_spec **rbd_spec)
>   {
>   	size_t len;
> +	struct rbd_spec *spec;
>   	const char *mon_addrs;
>   	size_t mon_addrs_size;
>   	char *options;
> @@ -2917,24 +2949,27 @@ static int rbd_add_parse_args(struct rbd_device
> *rbd_dev,
>   	mon_addrs_size = len + 1;
>   	buf += len;
>
> +	spec = rbd_spec_alloc();
> +	if (!spec)
> +		return -ENOMEM;
> +
>   	ret = -EINVAL;
>   	options = dup_token(&buf, NULL);
>   	if (!options)
> -		return -ENOMEM;
> +		goto out_mem;
>   	if (!*options)
>   		goto out_err;	/* Missing options */
>
> -	rbd_dev->spec->pool_name = dup_token(&buf, NULL);
> -	if (!rbd_dev->spec->pool_name)
> +	spec->pool_name = dup_token(&buf, NULL);
> +	if (!spec->pool_name)
>   		goto out_mem;
> -	if (!*rbd_dev->spec->pool_name)
> +	if (!*spec->pool_name)
>   		goto out_err;	/* Missing pool name */
>
> -	rbd_dev->spec->image_name =
> -		dup_token(&buf, &rbd_dev->spec->image_name_len);
> -	if (!rbd_dev->spec->image_name)
> +	spec->image_name = dup_token(&buf, &spec->image_name_len);
> +	if (!spec->image_name)
>   		goto out_mem;
> -	if (!*rbd_dev->spec->image_name)
> +	if (!*spec->image_name)
>   		goto out_err;	/* Missing image name */
>
>   	/*
> @@ -2949,11 +2984,11 @@ static int rbd_add_parse_args(struct rbd_device
> *rbd_dev,
>   		ret = -ENAMETOOLONG;
>   		goto out_err;
>   	}
> -	rbd_dev->spec->snap_name = kmalloc(len + 1, GFP_KERNEL);
> -	if (!rbd_dev->spec->snap_name)
> +	spec->snap_name = kmalloc(len + 1, GFP_KERNEL);
> +	if (!spec->snap_name)
>   		goto out_mem;
> -	memcpy(rbd_dev->spec->snap_name, buf, len);
> -	*(rbd_dev->spec->snap_name + len) = '\0';
> +	memcpy(spec->snap_name, buf, len);
> +	*(spec->snap_name + len) = '\0';
>
>   	/* Initialize all rbd options to the defaults */
>
> @@ -2973,15 +3008,15 @@ static int rbd_add_parse_args(struct rbd_device
> *rbd_dev,
>   	}
>   	*opts = rbd_opts;
>
> +	*rbd_spec = spec;
> +
>   	return 0;
>   out_mem:
>   	ret = -ENOMEM;
> +	kfree(spec->image_name);
> +	kfree(spec->pool_name);
> +	kfree(spec);

This is missing spec->snap_name. Why not use rbd_spec_put()?

>   out_err:
> -	kfree(rbd_dev->spec->image_name);
> -	rbd_dev->spec->image_name = NULL;
> -	rbd_dev->spec->image_name_len = 0;
> -	kfree(rbd_dev->spec->pool_name);
> -	rbd_dev->spec->pool_name = NULL;
>   	kfree(options);
>
>   	return ret;
> @@ -3195,6 +3230,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	struct rbd_device *rbd_dev = NULL;
>   	struct ceph_options *ceph_opts = NULL;
>   	struct rbd_options *rbd_opts = NULL;
> +	struct rbd_spec *spec = NULL;
>   	struct ceph_osd_client *osdc;
>   	int rc = -ENOMEM;
>
> @@ -3204,9 +3240,6 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
>   	if (!rbd_dev)
>   		return -ENOMEM;
> -	rbd_dev->spec = rbd_spec_alloc();
> -	if (!rbd_dev->spec)
> -		goto err_out_mem;
>
>   	/* static rbd_device initialization */
>   	spin_lock_init(&rbd_dev->lock);
> @@ -3215,9 +3248,10 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	init_rwsem(&rbd_dev->header_rwsem);
>
>   	/* parse add command */
> -	rc = rbd_add_parse_args(rbd_dev, buf, &ceph_opts, &rbd_opts);
> +	rc = rbd_add_parse_args(buf, &ceph_opts, &rbd_opts, &spec);
>   	if (rc < 0)
>   		goto err_out_mem;
> +
>   	rbd_dev->mapping.read_only = rbd_opts->read_only;
>
>   	rc = rbd_get_client(rbd_dev, ceph_opts);
> @@ -3227,10 +3261,12 @@ static ssize_t rbd_add(struct bus_type *bus,
>
>   	/* pick the pool */
>   	osdc = &rbd_dev->rbd_client->client->osdc;
> -	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->spec->pool_name);
> +	rc = ceph_pg_poolid_by_name(osdc->osdmap, spec->pool_name);
>   	if (rc < 0)
>   		goto err_out_client;
> -	rbd_dev->spec->pool_id = (u64) rc;
> +	spec->pool_id = (u64) rc;
> +
> +	rbd_dev->spec = spec;
>
>   	rc = rbd_dev_probe(rbd_dev);
>   	if (rc < 0)
> @@ -3321,8 +3357,8 @@ err_out_args:
>   	if (ceph_opts)
>   		ceph_destroy_options(ceph_opts);
>   	kfree(rbd_opts);
> +	rbd_spec_put(spec);
>   err_out_mem:
> -	rbd_spec_put(rbd_dev->spec);
>   	kfree(rbd_dev);
>
>   	dout("Error adding device %s\n", buf);
>

Re: [PATCH 8/8] rbd: fill rbd_spec in rbd_add_parse_args()

[Alex Elder]

On 10/29/2012 05:30 PM, Josh Durgin wrote:
> On 10/26/2012 04:03 PM, Alex Elder wrote:
>> Pass the address of an rbd_spec structure to rbd_add_parse_args().
>> Use it to hold the information defining the rbd image to be mapped
>> in an rbd_add() call.
>>
>> Use the result in the caller to initialize the rbd_dev->id field.
>>
>> This means rbd_dev is no longer needed in rbd_add_parse_args(),
>> so get rid of it.
>>
>> Now that this transformation of rbd_add_parse_args() is complete,
>> correct and expand on the its header documentation to reflect the
>> new reality.
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |  104
>> ++++++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 70 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 19c7c6b..28abd31 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c

. . .

>> @@ -2973,15 +3008,15 @@ static int rbd_add_parse_args(struct rbd_device
>> *rbd_dev,
>>       }
>>       *opts = rbd_opts;
>>
>> +    *rbd_spec = spec;
>> +
>>       return 0;
>>   out_mem:
>>       ret = -ENOMEM;
>> +    kfree(spec->image_name);
>> +    kfree(spec->pool_name);
>> +    kfree(spec);
> 
> This is missing spec->snap_name. Why not use rbd_spec_put()?

You're right, and you're right.  It was an oversight.  I'll fix
that.  Do you want me to repost?

In keeping with some of your previous comments I have also added
a note to include in a future patch messages about why a map
request failed in rbd_add() via a log message.

					-Alex

>>   out_err:
>> -    kfree(rbd_dev->spec->image_name);
>> -    rbd_dev->spec->image_name = NULL;
>> -    rbd_dev->spec->image_name_len = 0;
>> -    kfree(rbd_dev->spec->pool_name);
>> -    rbd_dev->spec->pool_name = NULL;
>>       kfree(options);
>>
. . .

Re: [PATCH 8/8] rbd: fill rbd_spec in rbd_add_parse_args()

[Josh Durgin]

On 10/30/2012 06:09 AM, Alex Elder wrote:
> On 10/29/2012 05:30 PM, Josh Durgin wrote:
>> On 10/26/2012 04:03 PM, Alex Elder wrote:
>>> Pass the address of an rbd_spec structure to rbd_add_parse_args().
>>> Use it to hold the information defining the rbd image to be mapped
>>> in an rbd_add() call.
>>>
>>> Use the result in the caller to initialize the rbd_dev->id field.
>>>
>>> This means rbd_dev is no longer needed in rbd_add_parse_args(),
>>> so get rid of it.
>>>
>>> Now that this transformation of rbd_add_parse_args() is complete,
>>> correct and expand on the its header documentation to reflect the
>>> new reality.
>>>
>>> Signed-off-by: Alex Elder <elder@inktank.com>
>>> ---
>>>    drivers/block/rbd.c |  104
>>> ++++++++++++++++++++++++++++++++++-----------------
>>>    1 file changed, 70 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>>> index 19c7c6b..28abd31 100644
>>> --- a/drivers/block/rbd.c
>>> +++ b/drivers/block/rbd.c
>
> . . .
>
>>> @@ -2973,15 +3008,15 @@ static int rbd_add_parse_args(struct rbd_device
>>> *rbd_dev,
>>>        }
>>>        *opts = rbd_opts;
>>>
>>> +    *rbd_spec = spec;
>>> +
>>>        return 0;
>>>    out_mem:
>>>        ret = -ENOMEM;
>>> +    kfree(spec->image_name);
>>> +    kfree(spec->pool_name);
>>> +    kfree(spec);
>>
>> This is missing spec->snap_name. Why not use rbd_spec_put()?
>
> You're right, and you're right.  It was an oversight.  I'll fix
> that.  Do you want me to repost?

No, the change is trivial enough. You can add my reviewed-by with that.

> In keeping with some of your previous comments I have also added
> a note to include in a future patch messages about why a map
> request failed in rbd_add() via a log message.

Sounds good.

>
> 					-Alex
>
>>>    out_err:
>>> -    kfree(rbd_dev->spec->image_name);
>>> -    rbd_dev->spec->image_name = NULL;
>>> -    rbd_dev->spec->image_name_len = 0;
>>> -    kfree(rbd_dev->spec->pool_name);
>>> -    rbd_dev->spec->pool_name = NULL;
>>>        kfree(options);
>>>
> . . .