[PATCH 0/7] rbd: use common code for probe and refresh

[PATCH 0/7] rbd: use common code for probe and refresh

[Alex Elder]

This is some work I had nearly done a long time ago.  It didn't even
have a bug associated with it.  I resurrected it over the weekend
and ported it to the new code.

It's basically cleanup though.  For format 1 rbd images, when
probing an image, header information for it is read in and
translated directly into the rbd_dev->header structure.

For an image refresh, instead, we use a stack structure to
hold the translated header, and then in a second step we
copy that into rbd_dev->header.

This series gets rid of the local variable, and always just
puts things directly into rbd_dev->header.  It also simplifies
probe and refresh for both format 1 and format 2, using a
common rbd_dev_vX_header_info() function for both purposes.

This set of patches, as well as the two single patches
and series of six I just posted, are available in the
"review/wip-rbd-cleanup-1" branch of the ceph-client git
repository.

					-Alex

[PATCH 1/7] rbd: set the mapping size and features later
[PATCH 2/7] rbd: zero format 1 header structure earlier
[PATCH 3/7] rbd: refactor rbd_header_from_disk()
[PATCH 4/7] rbd: update in-core header directly
[PATCH 5/7] rbd: simplify rbd_dev_v1_probe()
[PATCH 6/7] rbd: get rid of trivial v1 header wrappers
[PATCH 7/7] rbd: define rbd_dev_v1_header_info()

[PATCH 1/7] rbd: set the mapping size and features later

[Alex Elder]

Defer setting the size and features fields of a mapped image until
after the Linux disk structure is set up.  Set the capacity of the
disk after that.

Rearrange the definition of rbd_image_header, separating the fields
that are set only once from those that can be updated.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 0c72643..357a11f 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -100,21 +100,20 @@
  * block device image metadata (in-memory version)
  */
 struct rbd_image_header {
-	/* These four fields never change for a given rbd image */
+	/* These six fields never change for a given rbd image */
 	char *object_prefix;
-	u64 features;
 	__u8 obj_order;
 	__u8 crypt_type;
 	__u8 comp_type;
+	u64 stripe_unit;
+	u64 stripe_count;
+	u64 features;		/* Might be changeable someday? */

 	/* The remaining fields need to be updated occasionally */
 	u64 image_size;
 	struct ceph_snap_context *snapc;
-	char *snap_names;
-	u64 *snap_sizes;
-
-	u64 stripe_unit;
-	u64 stripe_count;
+	char *snap_names;	/* format 1 only */
+	u64 *snap_sizes;	/* format 1 only */
 };

 /*
@@ -4637,10 +4636,6 @@ static int rbd_dev_device_setup(struct rbd_device
*rbd_dev)
 {
 	int ret;

-	ret = rbd_dev_mapping_set(rbd_dev);
-	if (ret)
-		return ret;
-
 	/* generate unique id: find highest unique id, add one */
 	rbd_dev_id_get(rbd_dev);

@@ -4662,13 +4657,17 @@ static int rbd_dev_device_setup(struct
rbd_device *rbd_dev)
 	if (ret)
 		goto err_out_blkdev;

-	ret = rbd_bus_add_dev(rbd_dev);
+	ret = rbd_dev_mapping_set(rbd_dev);
 	if (ret)
 		goto err_out_disk;
+	set_capacity(rbd_dev->disk, rbd_dev->mapping.size / SECTOR_SIZE);
+
+	ret = rbd_bus_add_dev(rbd_dev);
+	if (ret)
+		goto err_out_mapping;

 	/* Everything's ready.  Announce the disk to the world. */

-	set_capacity(rbd_dev->disk, rbd_dev->mapping.size / SECTOR_SIZE);
 	set_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags);
 	add_disk(rbd_dev->disk);

@@ -4677,6 +4676,8 @@ static int rbd_dev_device_setup(struct rbd_device
*rbd_dev)

 	return ret;

+err_out_mapping:
+	rbd_dev_mapping_clear(rbd_dev);
 err_out_disk:
 	rbd_free_disk(rbd_dev);
 err_out_blkdev:
-- 
1.7.9.5

[PATCH 2/7] rbd: zero format 1 header structure earlier

[Alex Elder]

The passed-in header structure is zeroed in rbd_header_from_disk().
Instead, have the caller do it.  Note that there are two callers,
rbd_dev_v1_refresh() and rbd_dev_v1_probe().  The latter already has
a zeroed header structure so zeroing it isn't necessary there.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 357a11f..cdba93b 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -738,8 +738,6 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 	size_t size;
 	u32 i;

-	memset(header, 0, sizeof (*header));
-
 	snap_count = le32_to_cpu(ondisk->snap_count);

 	len = strnlen(ondisk->object_prefix, sizeof (ondisk->object_prefix));
@@ -3103,6 +3101,7 @@ static int rbd_dev_v1_refresh(struct rbd_device
*rbd_dev)
 	int ret;
 	struct rbd_image_header h;

+	memset(&h, 0, sizeof (h));
 	ret = rbd_read_header(rbd_dev, &h);
 	if (ret < 0)
 		return ret;
-- 
1.7.9.5

[PATCH 3/7] rbd: refactor rbd_header_from_disk()

[Alex Elder]

This rearranges rbd_header_from_disk so that it:
    - allocates the snapshot context right away
    - keeps results in local variables, not changing the passed-in
      header until it's known we'll succeed
    - does initialization of set-once fields in a header only if
      they have not already been set

The last point is moot at the moment, because rbd_read_header()
(the only caller) always supplies a zero-filled header buffer.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index cdba93b..2403098 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -727,86 +727,109 @@ static bool rbd_dev_ondisk_valid(struct
rbd_image_header_ondisk *ondisk)
 }

 /*
- * Create a new header structure, translate header format from the on-disk
- * header.
+ * Fill an rbd image header with information from the given format 1
+ * on-disk header.
  */
 static int rbd_header_from_disk(struct rbd_image_header *header,
 				 struct rbd_image_header_ondisk *ondisk)
 {
+	bool first_time = header->object_prefix == NULL;
+	struct ceph_snap_context *snapc;
+	char *object_prefix = NULL;
+	char *snap_names = NULL;
+	u64 *snap_sizes = NULL;
 	u32 snap_count;
-	size_t len;
 	size_t size;
+	int ret = -ENOMEM;
 	u32 i;

-	snap_count = le32_to_cpu(ondisk->snap_count);
+	/* Allocate this now to avoid having to handle failure below */

-	len = strnlen(ondisk->object_prefix, sizeof (ondisk->object_prefix));
-	header->object_prefix = kmalloc(len + 1, GFP_KERNEL);
-	if (!header->object_prefix)
-		return -ENOMEM;
-	memcpy(header->object_prefix, ondisk->object_prefix, len);
-	header->object_prefix[len] = '\0';
+	if (first_time) {
+		size_t len;

+		len = strnlen(ondisk->object_prefix,
+				sizeof (ondisk->object_prefix));
+		object_prefix = kmalloc(len + 1, GFP_KERNEL);
+		if (!object_prefix)
+			return -ENOMEM;
+		memcpy(object_prefix, ondisk->object_prefix, len);
+		object_prefix[len] = '\0';
+	}
+
+	/* Allocate the snapshot context and fill it in */
+
+	snap_count = le32_to_cpu(ondisk->snap_count);
+	snapc = ceph_create_snap_context(snap_count, GFP_KERNEL);
+	if (!snapc)
+		goto out_err;
+	snapc->seq = le64_to_cpu(ondisk->snap_seq);
 	if (snap_count) {
+		struct rbd_image_snap_ondisk *snaps;
 		u64 snap_names_len = le64_to_cpu(ondisk->snap_names_len);

-		/* Save a copy of the snapshot names */
+		/* We'll keep a copy of the snapshot names... */

-		if (snap_names_len > (u64) SIZE_MAX)
-			return -EIO;
-		header->snap_names = kmalloc(snap_names_len, GFP_KERNEL);
-		if (!header->snap_names)
+		if (snap_names_len > (u64)SIZE_MAX)
+			goto out_2big;
+		snap_names = kmalloc(snap_names_len, GFP_KERNEL);
+		if (!snap_names)
 			goto out_err;
-		/*
-		 * Note that rbd_dev_v1_header_read() guarantees
-		 * the ondisk buffer we're working with has
-		 * snap_names_len bytes beyond the end of the
-		 * snapshot id array, this memcpy() is safe.
-		 */
-		memcpy(header->snap_names, &ondisk->snaps[snap_count],
-			snap_names_len);

-		/* Record each snapshot's size */
+		/* ...as well as the array of their sizes. */

 		size = snap_count * sizeof (*header->snap_sizes);
-		header->snap_sizes = kmalloc(size, GFP_KERNEL);
-		if (!header->snap_sizes)
+		snap_sizes = kmalloc(size, GFP_KERNEL);
+		if (!snap_sizes)
 			goto out_err;
-		for (i = 0; i < snap_count; i++)
-			header->snap_sizes[i] =
-				le64_to_cpu(ondisk->snaps[i].image_size);
-	} else {
-		header->snap_names = NULL;
-		header->snap_sizes = NULL;
+
+		/*
+		 * Copy the names, and fill in each snapshot's id
+		 * and size.
+		 *
+		 * Note that rbd_dev_v1_header_read() guarantees the
+		 * ondisk buffer we're working with has
+		 * snap_names_len bytes beyond the end of the
+		 * snapshot id array, this memcpy() is safe.
+		 */
+		memcpy(snap_names, &ondisk->snaps[snap_count], snap_names_len);
+		snaps = ondisk->snaps;
+		for (i = 0; i < snap_count; i++) {
+			snapc->snaps[i] = le64_to_cpu(snaps[i].id);
+			snap_sizes[i] = le64_to_cpu(snaps[i].image_size);
+		}
 	}

-	header->features = 0;	/* No features support in v1 images */
-	header->obj_order = ondisk->options.order;
-	header->crypt_type = ondisk->options.crypt_type;
-	header->comp_type = ondisk->options.comp_type;
+	/* We won't fail any more, fill in the header */
+
+	if (first_time) {
+		header->object_prefix = object_prefix;
+		header->obj_order = ondisk->options.order;
+		header->crypt_type = ondisk->options.crypt_type;
+		header->comp_type = ondisk->options.comp_type;
+		/* The rest aren't used for format 1 images */
+		header->stripe_unit = 0;
+		header->stripe_count = 0;
+		header->features = 0;
+	}

-	/* Allocate and fill in the snapshot context */
+	/* The remaining fields always get updated (when we refresh) */

 	header->image_size = le64_to_cpu(ondisk->image_size);
-
-	header->snapc = ceph_create_snap_context(snap_count, GFP_KERNEL);
-	if (!header->snapc)
-		goto out_err;
-	header->snapc->seq = le64_to_cpu(ondisk->snap_seq);
-	for (i = 0; i < snap_count; i++)
-		header->snapc->snaps[i] = le64_to_cpu(ondisk->snaps[i].id);
+	header->snapc = snapc;
+	header->snap_names = snap_names;
+	header->snap_sizes = snap_sizes;

 	return 0;
-
+out_2big:
+	ret = -EIO;
 out_err:
-	kfree(header->snap_sizes);
-	header->snap_sizes = NULL;
-	kfree(header->snap_names);
-	header->snap_names = NULL;
-	kfree(header->object_prefix);
-	header->object_prefix = NULL;
+	kfree(snap_sizes);
+	kfree(snap_names);
+	ceph_put_snap_context(snapc);
+	kfree(object_prefix);

-	return -ENOMEM;
+	return ret;
 }

 static const char *_rbd_dev_v1_snap_name(struct rbd_device *rbd_dev,
u32 which)
-- 
1.7.9.5

[PATCH 4/7] rbd: update in-core header directly

[Alex Elder]

Now that rbd_header_from_disk() only fills in one-time fields once,
we can extend it slightly so it releases the other fields before
replacing their values.  This way there's no need to pass a
temporary buffer and then copy all the results in.  Just use the rbd
device header structure in rbd_header_from_disk() so its values get
updated directly.

Note that this means we need to take the header semaphore at the
point we update things.  So pass the rbd_dev rather than the address
of its header as its first argument to rbd_header_from_disk(), and
have it return an error code.

As a result, rbd_dev_v1_header_read() does all the work,
rbd_read_header() becomes unnecessary, and rbd_dev_v1_refresh()
becomes a very simple wrapper.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 2403098..a7985d9 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -730,9 +730,10 @@ static bool rbd_dev_ondisk_valid(struct
rbd_image_header_ondisk *ondisk)
  * Fill an rbd image header with information from the given format 1
  * on-disk header.
  */
-static int rbd_header_from_disk(struct rbd_image_header *header,
+static int rbd_header_from_disk(struct rbd_device *rbd_dev,
 				 struct rbd_image_header_ondisk *ondisk)
 {
+	struct rbd_image_header *header = &rbd_dev->header;
 	bool first_time = header->object_prefix == NULL;
 	struct ceph_snap_context *snapc;
 	char *object_prefix = NULL;
@@ -802,6 +803,7 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,

 	/* We won't fail any more, fill in the header */

+	down_write(&rbd_dev->header_rwsem);
 	if (first_time) {
 		header->object_prefix = object_prefix;
 		header->obj_order = ondisk->options.order;
@@ -811,6 +813,10 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 		header->stripe_unit = 0;
 		header->stripe_count = 0;
 		header->features = 0;
+	} else {
+		ceph_put_snap_context(header->snapc);
+		kfree(header->snap_names);
+		kfree(header->snap_sizes);
 	}

 	/* The remaining fields always get updated (when we refresh) */
@@ -820,6 +826,14 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 	header->snap_names = snap_names;
 	header->snap_sizes = snap_sizes;

+	/* Make sure mapping size is consistent with header info */
+
+	if (rbd_dev->spec->snap_id == CEPH_NOSNAP || first_time)
+		if (rbd_dev->mapping.size != header->image_size)
+			rbd_dev->mapping.size = header->image_size;
+
+	up_write(&rbd_dev->header_rwsem);
+
 	return 0;
 out_2big:
 	ret = -EIO;
@@ -3032,17 +3046,11 @@ out:
 }

 /*
- * Read the complete header for the given rbd device.
- *
- * Returns a pointer to a dynamically-allocated buffer containing
- * the complete and validated header.  Caller can pass the address
- * of a variable that will be filled in with the version of the
- * header object at the time it was read.
- *
- * Returns a pointer-coded errno if a failure occurs.
+ * Read the complete header for the given rbd device.  On successful
+ * return, the rbd_dev->header field will contain up-to-date
+ * information about the image.
  */
-static struct rbd_image_header_ondisk *
-rbd_dev_v1_header_read(struct rbd_device *rbd_dev)
+static int rbd_dev_v1_header_read(struct rbd_device *rbd_dev)
 {
 	struct rbd_image_header_ondisk *ondisk = NULL;
 	u32 snap_count = 0;
@@ -3067,22 +3075,22 @@ rbd_dev_v1_header_read(struct rbd_device *rbd_dev)
 		size += names_size;
 		ondisk = kmalloc(size, GFP_KERNEL);
 		if (!ondisk)
-			return ERR_PTR(-ENOMEM);
+			return -ENOMEM;

 		ret = rbd_obj_read_sync(rbd_dev, rbd_dev->header_name,
 				       0, size, ondisk);
 		if (ret < 0)
-			goto out_err;
+			goto out;
 		if ((size_t)ret < size) {
 			ret = -ENXIO;
 			rbd_warn(rbd_dev, "short header read (want %zd got %d)",
 				size, ret);
-			goto out_err;
+			goto out;
 		}
 		if (!rbd_dev_ondisk_valid(ondisk)) {
 			ret = -ENXIO;
 			rbd_warn(rbd_dev, "invalid header");
-			goto out_err;
+			goto out;
 		}

 		names_size = le64_to_cpu(ondisk->snap_names_len);
@@ -3090,27 +3098,8 @@ rbd_dev_v1_header_read(struct rbd_device *rbd_dev)
 		snap_count = le32_to_cpu(ondisk->snap_count);
 	} while (snap_count != want_count);

-	return ondisk;
-
-out_err:
-	kfree(ondisk);
-
-	return ERR_PTR(ret);
-}
-
-/*
- * reload the ondisk the header
- */
-static int rbd_read_header(struct rbd_device *rbd_dev,
-			   struct rbd_image_header *header)
-{
-	struct rbd_image_header_ondisk *ondisk;
-	int ret;
-
-	ondisk = rbd_dev_v1_header_read(rbd_dev);
-	if (IS_ERR(ondisk))
-		return PTR_ERR(ondisk);
-	ret = rbd_header_from_disk(header, ondisk);
+	ret = rbd_header_from_disk(rbd_dev, ondisk);
+out:
 	kfree(ondisk);

 	return ret;
@@ -3121,40 +3110,7 @@ static int rbd_read_header(struct rbd_device
*rbd_dev,
  */
 static int rbd_dev_v1_refresh(struct rbd_device *rbd_dev)
 {
-	int ret;
-	struct rbd_image_header h;
-
-	memset(&h, 0, sizeof (h));
-	ret = rbd_read_header(rbd_dev, &h);
-	if (ret < 0)
-		return ret;
-
-	down_write(&rbd_dev->header_rwsem);
-
-	/* Update image size, and check for resize of mapped image */
-	rbd_dev->header.image_size = h.image_size;
-	if (rbd_dev->spec->snap_id == CEPH_NOSNAP)
-		if (rbd_dev->mapping.size != rbd_dev->header.image_size)
-			rbd_dev->mapping.size = rbd_dev->header.image_size;
-
-	/* rbd_dev->header.object_prefix shouldn't change */
-	kfree(rbd_dev->header.snap_sizes);
-	kfree(rbd_dev->header.snap_names);
-	/* osd requests may still refer to snapc */
-	ceph_put_snap_context(rbd_dev->header.snapc);
-
-	rbd_dev->header.image_size = h.image_size;
-	rbd_dev->header.snapc = h.snapc;
-	rbd_dev->header.snap_names = h.snap_names;
-	rbd_dev->header.snap_sizes = h.snap_sizes;
-	/* Free the extra copy of the object prefix */
-	if (strcmp(rbd_dev->header.object_prefix, h.object_prefix))
-		rbd_warn(rbd_dev, "object prefix changed (ignoring)");
-	kfree(h.object_prefix);
-
-	up_write(&rbd_dev->header_rwsem);
-
-	return ret;
+	return rbd_dev_v1_header_read(rbd_dev);
 }

 /*
@@ -4517,7 +4473,7 @@ static int rbd_dev_v1_probe(struct rbd_device
*rbd_dev)

 	/* Populate rbd image metadata */

-	ret = rbd_read_header(rbd_dev, &rbd_dev->header);
+	ret = rbd_dev_v1_header_read(rbd_dev);
 	if (ret < 0)
 		goto out_err;

-- 
1.7.9.5

[PATCH 5/7] rbd: simplify rbd_dev_v1_probe()

[Alex Elder]

An rbd_dev structure's fields are all zero-filled for an initial
probe, so there's no need to explicitly zero the parent_spec
and parent_overlap fields in rbd_dev_v1_probe().  Removing these
assignments makes rbd_dev_v1_probe() *almost* trivial.

Move the dout() message that announces discovery of an image into
rbd_dev_image_probe(), generalize to support images in either format
and only show it if an image is fully discovered.

This highlights that are some unnecessary cleanups in the error
path for rbd_dev_v1_probe(), so they can be removed.

Now rbd_dev_v1_probe() *is* a trivial wrapper function.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index a7985d9..ed18888 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4469,31 +4469,7 @@ static void rbd_dev_unprobe(struct rbd_device
*rbd_dev)

 static int rbd_dev_v1_probe(struct rbd_device *rbd_dev)
 {
-	int ret;
-
-	/* Populate rbd image metadata */
-
-	ret = rbd_dev_v1_header_read(rbd_dev);
-	if (ret < 0)
-		goto out_err;
-
-	/* Version 1 images have no parent (no layering) */
-
-	rbd_dev->parent_spec = NULL;
-	rbd_dev->parent_overlap = 0;
-
-	dout("discovered version 1 image, header name is %s\n",
-		rbd_dev->header_name);
-
-	return 0;
-
-out_err:
-	kfree(rbd_dev->header_name);
-	rbd_dev->header_name = NULL;
-	kfree(rbd_dev->spec->image_id);
-	rbd_dev->spec->image_id = NULL;
-
-	return ret;
+	return rbd_dev_v1_header_read(rbd_dev);
 }

 static int rbd_dev_v2_probe(struct rbd_device *rbd_dev)
@@ -4553,9 +4529,6 @@ static int rbd_dev_v2_probe(struct rbd_device
*rbd_dev)
 	if (ret)
 		goto out_err;

-	dout("discovered version 2 image, header name is %s\n",
-		rbd_dev->header_name);
-
 	return 0;
 out_err:
 	rbd_dev->parent_overlap = 0;
@@ -4758,9 +4731,13 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev, bool read_only)
 	rbd_dev->mapping.read_only = read_only;

 	ret = rbd_dev_probe_parent(rbd_dev);
-	if (!ret)
-		return 0;
+	if (ret)
+		goto err_out_probe;
+
+	dout("discovered format %u image, header name is %s\n",
+		rbd_dev->image_format, rbd_dev->header_name);

+	return 0;
 err_out_probe:
 	rbd_dev_unprobe(rbd_dev);
 err_out_watch:
-- 
1.7.9.5

[PATCH 6/7] rbd: get rid of trivial v1 header wrappers

[Alex Elder]

Get rid of the trivial wrapper functions rbd_dev_v1_refresh() and
rbd_dev_v1_probe(), substituting rbd_dev_v1_header_read() calls
in their place.

Rename rbd_dev_v1_header_read() to be rbd_dev_v1_header_info(), to
be more generic (it will better reflect what happens with format 2
images).

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index ed18888..9e667c5 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -788,7 +788,7 @@ static int rbd_header_from_disk(struct rbd_device
*rbd_dev,
 		 * Copy the names, and fill in each snapshot's id
 		 * and size.
 		 *
-		 * Note that rbd_dev_v1_header_read() guarantees the
+		 * Note that rbd_dev_v1_header_info() guarantees the
 		 * ondisk buffer we're working with has
 		 * snap_names_len bytes beyond the end of the
 		 * snapshot id array, this memcpy() is safe.
@@ -3050,7 +3050,7 @@ out:
  * return, the rbd_dev->header field will contain up-to-date
  * information about the image.
  */
-static int rbd_dev_v1_header_read(struct rbd_device *rbd_dev)
+static int rbd_dev_v1_header_info(struct rbd_device *rbd_dev)
 {
 	struct rbd_image_header_ondisk *ondisk = NULL;
 	u32 snap_count = 0;
@@ -3106,14 +3106,6 @@ out:
 }

 /*
- * only read the first part of the ondisk header, without the snaps info
- */
-static int rbd_dev_v1_refresh(struct rbd_device *rbd_dev)
-{
-	return rbd_dev_v1_header_read(rbd_dev);
-}
-
-/*
  * Clear the rbd device's EXISTS flag if the snapshot it's mapped to
  * has disappeared from the (just updated) snapshot context.
  */
@@ -3141,7 +3133,7 @@ static int rbd_dev_refresh(struct rbd_device *rbd_dev)
 	mapping_size = rbd_dev->mapping.size;
 	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
 	if (rbd_dev->image_format == 1)
-		ret = rbd_dev_v1_refresh(rbd_dev);
+		ret = rbd_dev_v1_header_info(rbd_dev);
 	else
 		ret = rbd_dev_v2_refresh(rbd_dev);

@@ -4467,11 +4459,6 @@ static void rbd_dev_unprobe(struct rbd_device
*rbd_dev)
 	memset(header, 0, sizeof (*header));
 }

-static int rbd_dev_v1_probe(struct rbd_device *rbd_dev)
-{
-	return rbd_dev_v1_header_read(rbd_dev);
-}
-
 static int rbd_dev_v2_probe(struct rbd_device *rbd_dev)
 {
 	int ret;
@@ -4714,7 +4701,7 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev, bool read_only)
 		goto out_header_name;

 	if (rbd_dev->image_format == 1)
-		ret = rbd_dev_v1_probe(rbd_dev);
+		ret = rbd_dev_v1_header_info(rbd_dev);
 	else
 		ret = rbd_dev_v2_probe(rbd_dev);
 	if (ret)
-- 
1.7.9.5

[PATCH 7/7] rbd: define rbd_dev_v1_header_info()

[Alex Elder]

This rearranges rbd_dev_v2_refresh() so it works more like
rbd_dev_v1_header_info().  While format 1 images need to read the
whole header object to get any information, format 2 can collect
almost all information selectively.  So the one-time initialization
will remain in a separate function--based on rbd_dev_v2_probe().

Rename rbd_dev_v2_refresh() to be rbd_dev_v2_header_info(), and have
it call rbd_dev_v2_header_onetime() if it's being called for the
first time for the given rbd device.

Rename rbd_dev_v2_probe() to be rbd_dev_v2_header_onetime() and
remove the image size and snapshot context calls it held in
common with the refresh function.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 9e667c5..a300c03 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -425,7 +425,8 @@ static void rbd_img_parent_read(struct
rbd_obj_request *obj_request);
 static void rbd_dev_remove_parent(struct rbd_device *rbd_dev);

 static int rbd_dev_refresh(struct rbd_device *rbd_dev);
-static int rbd_dev_v2_refresh(struct rbd_device *rbd_dev);
+static int rbd_dev_v2_header_onetime(struct rbd_device *rbd_dev);
+static int rbd_dev_v2_header_info(struct rbd_device *rbd_dev);
 static const char *rbd_dev_v2_snap_name(struct rbd_device *rbd_dev,
 					u64 snap_id);
 static int _rbd_dev_v2_snap_size(struct rbd_device *rbd_dev, u64 snap_id,
@@ -3135,7 +3136,7 @@ static int rbd_dev_refresh(struct rbd_device *rbd_dev)
 	if (rbd_dev->image_format == 1)
 		ret = rbd_dev_v1_header_info(rbd_dev);
 	else
-		ret = rbd_dev_v2_refresh(rbd_dev);
+		ret = rbd_dev_v2_header_info(rbd_dev);

 	/* If it's a mapped snapshot, validate its EXISTS flag */

@@ -4005,12 +4006,19 @@ out:
 	return snap_name;
 }

-static int rbd_dev_v2_refresh(struct rbd_device *rbd_dev)
+static int rbd_dev_v2_header_info(struct rbd_device *rbd_dev)
 {
+	bool first_time = rbd_dev->header.object_prefix == NULL;
 	int ret;

 	down_write(&rbd_dev->header_rwsem);

+	if (first_time) {
+		ret = rbd_dev_v2_header_onetime(rbd_dev);
+		if (ret)
+			goto out;
+	}
+
 	ret = rbd_dev_v2_image_size(rbd_dev);
 	if (ret)
 		goto out;
@@ -4459,22 +4467,18 @@ static void rbd_dev_unprobe(struct rbd_device
*rbd_dev)
 	memset(header, 0, sizeof (*header));
 }

-static int rbd_dev_v2_probe(struct rbd_device *rbd_dev)
+static int rbd_dev_v2_header_onetime(struct rbd_device *rbd_dev)
 {
 	int ret;

-	ret = rbd_dev_v2_image_size(rbd_dev);
-	if (ret)
-		goto out_err;
-
-	/* Get the object prefix (a.k.a. block_name) for the image */
-
 	ret = rbd_dev_v2_object_prefix(rbd_dev);
 	if (ret)
 		goto out_err;

-	/* Get the and check features for the image */
-
+	/*
+	 * Get the and check features for the image.  Currently the
+	 * features are assumed to never change.
+	 */
 	ret = rbd_dev_v2_features(rbd_dev);
 	if (ret)
 		goto out_err;
@@ -4504,17 +4508,7 @@ static int rbd_dev_v2_probe(struct rbd_device
*rbd_dev)
 		if (ret < 0)
 			goto out_err;
 	}
-
-	/* crypto and compression type aren't (yet) supported for v2 images */
-
-	rbd_dev->header.crypt_type = 0;
-	rbd_dev->header.comp_type = 0;
-
-	/* Get the snapshot context, plus the header version */
-
-	ret = rbd_dev_v2_snap_context(rbd_dev);
-	if (ret)
-		goto out_err;
+	/* No support for crypto and compression type format 2 images */

 	return 0;
 out_err:
@@ -4703,7 +4697,7 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev, bool read_only)
 	if (rbd_dev->image_format == 1)
 		ret = rbd_dev_v1_header_info(rbd_dev);
 	else
-		ret = rbd_dev_v2_probe(rbd_dev);
+		ret = rbd_dev_v2_header_info(rbd_dev);
 	if (ret)
 		goto err_out_watch;

-- 
1.7.9.5

Re: [PATCH 0/7] rbd: use common code for probe and refresh

[Josh Durgin]

On 05/06/2013 06:51 PM, Alex Elder wrote:
> This is some work I had nearly done a long time ago.  It didn't even
> have a bug associated with it.  I resurrected it over the weekend
> and ported it to the new code.
>
> It's basically cleanup though.  For format 1 rbd images, when
> probing an image, header information for it is read in and
> translated directly into the rbd_dev->header structure.
>
> For an image refresh, instead, we use a stack structure to
> hold the translated header, and then in a second step we
> copy that into rbd_dev->header.
>
> This series gets rid of the local variable, and always just
> puts things directly into rbd_dev->header.  It also simplifies
> probe and refresh for both format 1 and format 2, using a
> common rbd_dev_vX_header_info() function for both purposes.
>
> This set of patches, as well as the two single patches
> and series of six I just posted, are available in the
> "review/wip-rbd-cleanup-1" branch of the ceph-client git
> repository.
>
> 					-Alex
>
> [PATCH 1/7] rbd: set the mapping size and features later
> [PATCH 2/7] rbd: zero format 1 header structure earlier
> [PATCH 3/7] rbd: refactor rbd_header_from_disk()
> [PATCH 4/7] rbd: update in-core header directly
> [PATCH 5/7] rbd: simplify rbd_dev_v1_probe()
> [PATCH 6/7] rbd: get rid of trivial v1 header wrappers
> [PATCH 7/7] rbd: define rbd_dev_v1_header_info()

These all look good.
The last one leaves the only call to rbd_dev_v2_parent_info() in
rbd_dev_v2_header_onetime(), but I'm guessing you already moved it
in your upcoming flatten-handling patches.

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

Re: [PATCH 0/7] rbd: use common code for probe and refresh

[Alex Elder]

On 05/08/2013 02:29 PM, Josh Durgin wrote:
> On 05/06/2013 06:51 PM, Alex Elder wrote:
>> This is some work I had nearly done a long time ago.  It didn't even
>> have a bug associated with it.  I resurrected it over the weekend
>> and ported it to the new code.
>>
>> It's basically cleanup though.  For format 1 rbd images, when
>> probing an image, header information for it is read in and
>> translated directly into the rbd_dev->header structure.
>>
>> For an image refresh, instead, we use a stack structure to
>> hold the translated header, and then in a second step we
>> copy that into rbd_dev->header.
>>
>> This series gets rid of the local variable, and always just
>> puts things directly into rbd_dev->header.  It also simplifies
>> probe and refresh for both format 1 and format 2, using a
>> common rbd_dev_vX_header_info() function for both purposes.
>>
>> This set of patches, as well as the two single patches
>> and series of six I just posted, are available in the
>> "review/wip-rbd-cleanup-1" branch of the ceph-client git
>> repository.
>>
>>                     -Alex
>>
>> [PATCH 1/7] rbd: set the mapping size and features later
>> [PATCH 2/7] rbd: zero format 1 header structure earlier
>> [PATCH 3/7] rbd: refactor rbd_header_from_disk()
>> [PATCH 4/7] rbd: update in-core header directly
>> [PATCH 5/7] rbd: simplify rbd_dev_v1_probe()
>> [PATCH 6/7] rbd: get rid of trivial v1 header wrappers
>> [PATCH 7/7] rbd: define rbd_dev_v1_header_info()
> 
> These all look good.
> The last one leaves the only call to rbd_dev_v2_parent_info() in
> rbd_dev_v2_header_onetime(), but I'm guessing you already moved it
> in your upcoming flatten-handling patches.

It took me a minute to figure out your point, but yes, I move
that into rbd_dev_v2_header_info() in an upcoming patch.

					-Alex

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