Another pile of patches

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 <[email protected]>

---
  drivers/block/rbd.c |  231
+++++++++++++++++++++++++++++++++------------------
  1 file changed, 152 insertions(+), 79 deletions(-)

drivers/block/rbd.c +151 -78

index c800047..cc06c55 100644
@@ -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)

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 <[email protected]>

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 <[email protected]>

---

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

drivers/block/rbd.c +7 -5

index cc06c55..147c8df 100644
@@ -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:

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 <[email protected]>

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

drivers/block/rbd.c +4 -4

index 147c8df..2a523a1 100644
@@ -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)

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 <[email protected]>

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

drivers/block/rbd.c +5 -4

index 2a523a1..05525b2 100644
@@ -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;

drivers/block/rbd.c +1 -1

@@ -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;

  }

drivers/block/rbd.c +3 -3

@@ -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;

drivers/block/rbd.c +3 -4

@@ -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 */

drivers/block/rbd.c +2 -2

@@ -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);

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 <[email protected]>

---
  drivers/block/rbd.c |   11 +++++------
  1 file changed, 5 insertions(+), 6 deletions(-)

drivers/block/rbd.c +2 -3

index 05525b2..fb44d4d 100644
@@ -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;

  }

drivers/block/rbd.c +3 -3

@@ -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 " : "",

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

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   48 +++++++++++++++++++++++++++---------------------
  1 file changed, 27 insertions(+), 21 deletions(-)

drivers/block/rbd.c +2 -18

index fb44d4d..6e36c63 100644
@@ -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 */

drivers/block/rbd.c +25 -3

@@ -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);

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 <[email protected]>

---
  drivers/block/rbd.c |   77
+++++++++++++++++++++++++--------------------------
  1 file changed, 37 insertions(+), 40 deletions(-)

drivers/block/rbd.c +31 -15

index 6e36c63..4fe14ff 100644
@@ -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;

drivers/block/rbd.c +6 -25

@@ -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);

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 <[email protected]>

---
  drivers/block/rbd.c |   13 ++++---------
  1 file changed, 4 insertions(+), 9 deletions(-)

drivers/block/rbd.c +0 -1

index 4fe14ff..cae7423 100644
@@ -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;

drivers/block/rbd.c +1 -3

@@ -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;

drivers/block/rbd.c +1 -2

@@ -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;

drivers/block/rbd.c +1 -1

@@ -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;

drivers/block/rbd.c +1 -2

@@ -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;

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 <[email protected]>

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

drivers/block/rbd.c +28 -21

index cae7423..f900f3b 100644
@@ -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;

  }

drivers/block/rbd.c +1 -8

@@ -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);

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 <[email protected]>

---
  drivers/block/rbd.c |   23 ++++++++++-------------
  1 file changed, 10 insertions(+), 13 deletions(-)

drivers/block/rbd.c +3 -4

index f900f3b..948a084 100644
@@ -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;

drivers/block/rbd.c +1 -2

@@ -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;

drivers/block/rbd.c +6 -7

@@ -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;
 

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 <[email protected]>

---
  drivers/block/rbd.c |   30 +++++++++++++++++++-----------
  1 file changed, 19 insertions(+), 11 deletions(-)

drivers/block/rbd.c +10 -9

index 948a084..392dded 100644
@@ -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:

drivers/block/rbd.c +9 -2

@@ -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);
 

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 <[email protected]>

---
  drivers/block/rbd.c |   37 ++++++++++++++++++++-----------------
  1 file changed, 20 insertions(+), 17 deletions(-)

drivers/block/rbd.c +9 -8

index 392dded..388dd47 100644
@@ -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);

drivers/block/rbd.c +7 -3

@@ -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;

drivers/block/rbd.c +4 -6

@@ -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);

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 <[email protected]>

---
  drivers/block/rbd.c |  158
+++++++++++++++++++++++++++++----------------------
  1 file changed, 90 insertions(+), 68 deletions(-)

drivers/block/rbd.c +0 -0

index 388dd47..c39e238 100644

drivers/block/rbd.c +7 -2

+
+	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;

drivers/block/rbd.c +4 -4

  	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;

drivers/block/rbd.c +6 -6

@@ -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;

drivers/block/rbd.c +9 -7

@@ -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 */

drivers/block/rbd.c +7 -6

@@ -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 */
 

/*

drivers/block/rbd.c +9 -9

@@ -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;

drivers/block/rbd.c +2 -2

@@ -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 */

drivers/block/rbd.c +6 -6

@@ -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);

drivers/block/rbd.c +8 -7

@@ -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;

  }

drivers/block/rbd.c +16 -12

@@ -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);
 

A couple notes below, but looks good.

Reviewed-by Josh Durgin <[email protected]>

Quoted 21 lines · 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 <[email protected]>

---
  drivers/block/rbd.c |  158
+++++++++++++++++++++++++++++----------------------
  1 file changed, 90 insertions(+), 68 deletions(-)

drivers/block/rbd.c +0 -0

index 388dd47..c39e238 100644
@@ -112,6 +112,27 @@ struct rbd_image_header {
      u64 obj_version;
  };
 

drivers/block/rbd.c struct rbd_image_header { · line 120

Quoted 6 lines

drivers/block/rbd.c +6 -0

+/*
+ * 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.

Quoted 4 lines

drivers/block/rbd.c +4 -0

+struct rbd_spec {
+    u64        pool_id;
+    char        *pool_name;
+

drivers/block/rbd.c struct rbd_image_header { · line 128

Quoted 4 lines

drivers/block/rbd.c +4 -0

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

Quoted 23 lines

drivers/block/rbd.c +8 -7

+
+    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;
 

drivers/block/rbd.c struct rbd_device { · line 215

Quoted 3 lines

drivers/block/rbd.c +1 -2

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

Quoted 6 lines

drivers/block/rbd.c +0 -0

      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)
 

drivers/block/rbd.c static int snap_by_name(struct rbd_device *rbd_dev, · line 674

Quoted 7 lines

drivers/block/rbd.c +1 -1

      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;

Quoted 1 line · Josh Durgin

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

Quoted 24 lines

Reviewed-by Josh Durgin <[email protected]>

Quoted 21 lines · 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 <[email protected]>

---
   drivers/block/rbd.c |  158
+++++++++++++++++++++++++++++----------------------
   1 file changed, 90 insertions(+), 68 deletions(-)

drivers/block/rbd.c +0 -0

index 388dd47..c39e238 100644
@@ -112,6 +112,27 @@ struct rbd_image_header {
       u64 obj_version;
   };
 

Quoted 9 lines

drivers/block/rbd.c struct rbd_image_header { · line 120

Quoted 6 lines

drivers/block/rbd.c +6 -0

+/*
+ * 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.