[PATCH 0/3] rbd: hide some snapshot info details

[PATCH 0/3] rbd: hide some snapshot info details

[Alex Elder]

Just three odds-and-ends patches in this seventh series before the
next series (which does all the real work of adding support for rbd
image format 2).

The patches rearrange the code that gets information about particular
snapshots so it hides the details of which format the underlying image
uses.

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

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

					-Alex

[PATCH 1/3]  6a763d5 rbd: don't use index in __rbd_add_snap_dev()
[PATCH 2/3]  5652941 rbd: add an rbd features field
[PATCH 3/3]  79ae7ee rbd: encapsulate code that gets snapshot info

[PATCH 1/3] rbd: don't use index in __rbd_add_snap_dev()

[Alex Elder]

Pass the snapshot id and snapshot size rather than an index
to __rbd_add_snap_dev() to specify values for a new snapshot.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index d73edb1..2b048e3 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2031,7 +2031,8 @@ static int rbd_register_snap_dev(struct rbd_snap
*snap,
 }

 static struct rbd_snap *__rbd_add_snap_dev(struct rbd_device *rbd_dev,
-					      int i, const char *name)
+						const char *snap_name,
+						u64 snap_id, u64 snap_size)
 {
 	struct rbd_snap *snap;
 	int ret;
@@ -2041,12 +2042,12 @@ static struct rbd_snap
*__rbd_add_snap_dev(struct rbd_device *rbd_dev,
 		return ERR_PTR(-ENOMEM);

 	ret = -ENOMEM;
-	snap->name = kstrdup(name, GFP_KERNEL);
+	snap->name = kstrdup(snap_name, GFP_KERNEL);
 	if (!snap->name)
 		goto err;

-	snap->size = rbd_dev->header.snap_sizes[i];
-	snap->id = rbd_dev->header.snapc->snaps[i];
+	snap->id = snap_id;
+	snap->size = snap_size;

 	return snap;

@@ -2111,12 +2112,13 @@ static int rbd_dev_snaps_update(struct
rbd_device *rbd_dev)
 		dout("entry %u: snap_id = %llu\n", (unsigned int) snap_count,
 			(unsigned long long) snap_id);
 		if (!snap || (snap_id != CEPH_NOSNAP && snap->id < snap_id)) {
+			struct rbd_image_header	*header = &rbd_dev->header;
 			struct rbd_snap *new_snap;

 			/* We haven't seen this snapshot before */

-			new_snap = __rbd_add_snap_dev(rbd_dev, index,
-							snap_name);
+			new_snap = __rbd_add_snap_dev(rbd_dev, snap_name,
+					snap_id, header->snap_sizes[index]);
 			if (IS_ERR(new_snap)) {
 				int err = PTR_ERR(new_snap);

-- 
1.7.9.5

Re: [PATCH 1/3] rbd: don't use index in __rbd_add_snap_dev()

[Josh Durgin]

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

On 09/07/2012 12:24 PM, Alex Elder wrote:
> Pass the snapshot id and snapshot size rather than an index
> to __rbd_add_snap_dev() to specify values for a new snapshot.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index d73edb1..2b048e3 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2031,7 +2031,8 @@ static int rbd_register_snap_dev(struct rbd_snap
> *snap,
>   }
>
>   static struct rbd_snap *__rbd_add_snap_dev(struct rbd_device *rbd_dev,
> -					      int i, const char *name)
> +						const char *snap_name,
> +						u64 snap_id, u64 snap_size)
>   {
>   	struct rbd_snap *snap;
>   	int ret;
> @@ -2041,12 +2042,12 @@ static struct rbd_snap
> *__rbd_add_snap_dev(struct rbd_device *rbd_dev,
>   		return ERR_PTR(-ENOMEM);
>
>   	ret = -ENOMEM;
> -	snap->name = kstrdup(name, GFP_KERNEL);
> +	snap->name = kstrdup(snap_name, GFP_KERNEL);
>   	if (!snap->name)
>   		goto err;
>
> -	snap->size = rbd_dev->header.snap_sizes[i];
> -	snap->id = rbd_dev->header.snapc->snaps[i];
> +	snap->id = snap_id;
> +	snap->size = snap_size;
>
>   	return snap;
>
> @@ -2111,12 +2112,13 @@ static int rbd_dev_snaps_update(struct
> rbd_device *rbd_dev)
>   		dout("entry %u: snap_id = %llu\n", (unsigned int) snap_count,
>   			(unsigned long long) snap_id);
>   		if (!snap || (snap_id != CEPH_NOSNAP && snap->id < snap_id)) {
> +			struct rbd_image_header	*header = &rbd_dev->header;
>   			struct rbd_snap *new_snap;
>
>   			/* We haven't seen this snapshot before */
>
> -			new_snap = __rbd_add_snap_dev(rbd_dev, index,
> -							snap_name);
> +			new_snap = __rbd_add_snap_dev(rbd_dev, snap_name,
> +					snap_id, header->snap_sizes[index]);
>   			if (IS_ERR(new_snap)) {
>   				int err = PTR_ERR(new_snap);
>

[PATCH 2/3] rbd: add an rbd features field

[Alex Elder]

Record the features values for each rbd image and each of its
snapshots.  This is really something that only becomes meaningful
for version 2 images, so this is just putting in place code
that will form common infrastructure.

It may be useful to expand the sysfs entries--and therefore the
information we maintain--for the image and for each snapshot.
But I'm going to hold off doing that until we start making
active use of the feature bits.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 Documentation/ABI/testing/sysfs-bus-rbd |    7 +++++
 drivers/block/rbd.c                     |   43
+++++++++++++++++++++++++++++--
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-rbd
b/Documentation/ABI/testing/sysfs-bus-rbd
index 6fe4224..1cf2adf 100644
--- a/Documentation/ABI/testing/sysfs-bus-rbd
+++ b/Documentation/ABI/testing/sysfs-bus-rbd
@@ -25,6 +25,10 @@ client_id

 	The ceph unique client id that was assigned for this specific session.

+features
+
+	A hexadecimal encoding of the feature bits for this image.
+
 major

 	The block device major number.
@@ -78,4 +82,7 @@ snap_size

 	The size of the image when this snapshot was taken.

+snap_features
+
+	A hexadecimal encoding of the feature bits for this snapshot.

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 2b048e3..c62d3f4 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -85,6 +85,7 @@
 struct rbd_image_header {
 	/* These four fields never change for a given rbd image */
 	char *object_prefix;
+	u64 features;
 	__u8 obj_order;
 	__u8 crypt_type;
 	__u8 comp_type;
@@ -148,12 +149,14 @@ struct rbd_snap {
 	u64			size;
 	struct list_head	node;
 	u64			id;
+	u64			features;
 };

 struct rbd_mapping {
 	char                    *snap_name;
 	u64                     snap_id;
 	u64                     size;
+	u64                     features;
 	bool                    snap_exists;
 	bool			read_only;
 };
@@ -590,6 +593,7 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 		header->snap_sizes = NULL;
 	}

+	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;
@@ -632,6 +636,7 @@ static int snap_by_name(struct rbd_device *rbd_dev,
const char *snap_name)
 		if (!strcmp(snap_name, snap->name)) {
 			rbd_dev->mapping.snap_id = snap->id;
 			rbd_dev->mapping.size = snap->size;
+			rbd_dev->mapping.features = snap->features;

 			return 0;
 		}
@@ -648,6 +653,7 @@ static int rbd_dev_set_mapping(struct rbd_device
*rbd_dev, char *snap_name)
 		    sizeof (RBD_SNAP_HEAD_NAME))) {
 		rbd_dev->mapping.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;
 		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
 		ret = 0;
@@ -1835,6 +1841,19 @@ static ssize_t rbd_size_show(struct device *dev,
 	return sprintf(buf, "%llu\n", (unsigned long long) size * SECTOR_SIZE);
 }

+/*
+ * Note this shows the features for whatever's mapped, which is not
+ * necessarily the base image.
+ */
+static ssize_t rbd_features_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
+
+	return sprintf(buf, "0x%016llx\n",
+			(unsigned long long) rbd_dev->mapping.features);
+}
+
 static ssize_t rbd_major_show(struct device *dev,
 			      struct device_attribute *attr, char *buf)
 {
@@ -1884,6 +1903,10 @@ static ssize_t rbd_image_id_show(struct device *dev,
 	return sprintf(buf, "%s\n", rbd_dev->image_id);
 }

+/*
+ * Shows the name of the currently-mapped snapshot (or
+ * RBD_SNAP_HEAD_NAME for the base image).
+ */
 static ssize_t rbd_snap_show(struct device *dev,
 			     struct device_attribute *attr,
 			     char *buf)
@@ -1907,6 +1930,7 @@ static ssize_t rbd_image_refresh(struct device *dev,
 }

 static DEVICE_ATTR(size, S_IRUGO, rbd_size_show, NULL);
+static DEVICE_ATTR(features, S_IRUGO, rbd_features_show, NULL);
 static DEVICE_ATTR(major, S_IRUGO, rbd_major_show, NULL);
 static DEVICE_ATTR(client_id, S_IRUGO, rbd_client_id_show, NULL);
 static DEVICE_ATTR(pool, S_IRUGO, rbd_pool_show, NULL);
@@ -1918,6 +1942,7 @@ static DEVICE_ATTR(current_snap, S_IRUGO,
rbd_snap_show, NULL);

 static struct attribute *rbd_attrs[] = {
 	&dev_attr_size.attr,
+	&dev_attr_features.attr,
 	&dev_attr_major.attr,
 	&dev_attr_client_id.attr,
 	&dev_attr_pool.attr,
@@ -1971,12 +1996,24 @@ static ssize_t rbd_snap_id_show(struct device *dev,
 	return sprintf(buf, "%llu\n", (unsigned long long)snap->id);
 }

+static ssize_t rbd_snap_features_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev);
+
+	return sprintf(buf, "0x%016llx\n",
+			(unsigned long long) snap->features);
+}
+
 static DEVICE_ATTR(snap_size, S_IRUGO, rbd_snap_size_show, NULL);
 static DEVICE_ATTR(snap_id, S_IRUGO, rbd_snap_id_show, NULL);
+static DEVICE_ATTR(snap_features, S_IRUGO, rbd_snap_features_show, NULL);

 static struct attribute *rbd_snap_attrs[] = {
 	&dev_attr_snap_size.attr,
 	&dev_attr_snap_id.attr,
+	&dev_attr_snap_features.attr,
 	NULL,
 };

@@ -2032,7 +2069,8 @@ static int rbd_register_snap_dev(struct rbd_snap
*snap,

 static struct rbd_snap *__rbd_add_snap_dev(struct rbd_device *rbd_dev,
 						const char *snap_name,
-						u64 snap_id, u64 snap_size)
+						u64 snap_id, u64 snap_size,
+						u64 snap_features)
 {
 	struct rbd_snap *snap;
 	int ret;
@@ -2048,6 +2086,7 @@ static struct rbd_snap *__rbd_add_snap_dev(struct
rbd_device *rbd_dev,

 	snap->id = snap_id;
 	snap->size = snap_size;
+	snap->features = snap_features;

 	return snap;

@@ -2118,7 +2157,7 @@ static int rbd_dev_snaps_update(struct rbd_device
*rbd_dev)
 			/* We haven't seen this snapshot before */

 			new_snap = __rbd_add_snap_dev(rbd_dev, snap_name,
-					snap_id, header->snap_sizes[index]);
+					snap_id, header->snap_sizes[index], 0);
 			if (IS_ERR(new_snap)) {
 				int err = PTR_ERR(new_snap);

-- 
1.7.9.5

Re: [PATCH 2/3] rbd: add an rbd features field

[Josh Durgin]

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

On 09/07/2012 12:25 PM, Alex Elder wrote:
> Record the features values for each rbd image and each of its
> snapshots.  This is really something that only becomes meaningful
> for version 2 images, so this is just putting in place code
> that will form common infrastructure.
>
> It may be useful to expand the sysfs entries--and therefore the
> information we maintain--for the image and for each snapshot.
> But I'm going to hold off doing that until we start making
> active use of the feature bits.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   Documentation/ABI/testing/sysfs-bus-rbd |    7 +++++
>   drivers/block/rbd.c                     |   43
> +++++++++++++++++++++++++++++--
>   2 files changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-rbd
> b/Documentation/ABI/testing/sysfs-bus-rbd
> index 6fe4224..1cf2adf 100644
> --- a/Documentation/ABI/testing/sysfs-bus-rbd
> +++ b/Documentation/ABI/testing/sysfs-bus-rbd
> @@ -25,6 +25,10 @@ client_id
>
>   	The ceph unique client id that was assigned for this specific session.
>
> +features
> +
> +	A hexadecimal encoding of the feature bits for this image.
> +
>   major
>
>   	The block device major number.
> @@ -78,4 +82,7 @@ snap_size
>
>   	The size of the image when this snapshot was taken.
>
> +snap_features
> +
> +	A hexadecimal encoding of the feature bits for this snapshot.
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 2b048e3..c62d3f4 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -85,6 +85,7 @@
>   struct rbd_image_header {
>   	/* These four fields never change for a given rbd image */
>   	char *object_prefix;
> +	u64 features;
>   	__u8 obj_order;
>   	__u8 crypt_type;
>   	__u8 comp_type;
> @@ -148,12 +149,14 @@ struct rbd_snap {
>   	u64			size;
>   	struct list_head	node;
>   	u64			id;
> +	u64			features;
>   };
>
>   struct rbd_mapping {
>   	char                    *snap_name;
>   	u64                     snap_id;
>   	u64                     size;
> +	u64                     features;
>   	bool                    snap_exists;
>   	bool			read_only;
>   };
> @@ -590,6 +593,7 @@ static int rbd_header_from_disk(struct
> rbd_image_header *header,
>   		header->snap_sizes = NULL;
>   	}
>
> +	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;
> @@ -632,6 +636,7 @@ static int snap_by_name(struct rbd_device *rbd_dev,
> const char *snap_name)
>   		if (!strcmp(snap_name, snap->name)) {
>   			rbd_dev->mapping.snap_id = snap->id;
>   			rbd_dev->mapping.size = snap->size;
> +			rbd_dev->mapping.features = snap->features;
>
>   			return 0;
>   		}
> @@ -648,6 +653,7 @@ static int rbd_dev_set_mapping(struct rbd_device
> *rbd_dev, char *snap_name)
>   		    sizeof (RBD_SNAP_HEAD_NAME))) {
>   		rbd_dev->mapping.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;
>   		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
>   		ret = 0;
> @@ -1835,6 +1841,19 @@ static ssize_t rbd_size_show(struct device *dev,
>   	return sprintf(buf, "%llu\n", (unsigned long long) size * SECTOR_SIZE);
>   }
>
> +/*
> + * Note this shows the features for whatever's mapped, which is not
> + * necessarily the base image.
> + */
> +static ssize_t rbd_features_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
> +
> +	return sprintf(buf, "0x%016llx\n",
> +			(unsigned long long) rbd_dev->mapping.features);
> +}
> +
>   static ssize_t rbd_major_show(struct device *dev,
>   			      struct device_attribute *attr, char *buf)
>   {
> @@ -1884,6 +1903,10 @@ static ssize_t rbd_image_id_show(struct device *dev,
>   	return sprintf(buf, "%s\n", rbd_dev->image_id);
>   }
>
> +/*
> + * Shows the name of the currently-mapped snapshot (or
> + * RBD_SNAP_HEAD_NAME for the base image).
> + */
>   static ssize_t rbd_snap_show(struct device *dev,
>   			     struct device_attribute *attr,
>   			     char *buf)
> @@ -1907,6 +1930,7 @@ static ssize_t rbd_image_refresh(struct device *dev,
>   }
>
>   static DEVICE_ATTR(size, S_IRUGO, rbd_size_show, NULL);
> +static DEVICE_ATTR(features, S_IRUGO, rbd_features_show, NULL);
>   static DEVICE_ATTR(major, S_IRUGO, rbd_major_show, NULL);
>   static DEVICE_ATTR(client_id, S_IRUGO, rbd_client_id_show, NULL);
>   static DEVICE_ATTR(pool, S_IRUGO, rbd_pool_show, NULL);
> @@ -1918,6 +1942,7 @@ static DEVICE_ATTR(current_snap, S_IRUGO,
> rbd_snap_show, NULL);
>
>   static struct attribute *rbd_attrs[] = {
>   	&dev_attr_size.attr,
> +	&dev_attr_features.attr,
>   	&dev_attr_major.attr,
>   	&dev_attr_client_id.attr,
>   	&dev_attr_pool.attr,
> @@ -1971,12 +1996,24 @@ static ssize_t rbd_snap_id_show(struct device *dev,
>   	return sprintf(buf, "%llu\n", (unsigned long long)snap->id);
>   }
>
> +static ssize_t rbd_snap_features_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev);
> +
> +	return sprintf(buf, "0x%016llx\n",
> +			(unsigned long long) snap->features);
> +}
> +
>   static DEVICE_ATTR(snap_size, S_IRUGO, rbd_snap_size_show, NULL);
>   static DEVICE_ATTR(snap_id, S_IRUGO, rbd_snap_id_show, NULL);
> +static DEVICE_ATTR(snap_features, S_IRUGO, rbd_snap_features_show, NULL);
>
>   static struct attribute *rbd_snap_attrs[] = {
>   	&dev_attr_snap_size.attr,
>   	&dev_attr_snap_id.attr,
> +	&dev_attr_snap_features.attr,
>   	NULL,
>   };
>
> @@ -2032,7 +2069,8 @@ static int rbd_register_snap_dev(struct rbd_snap
> *snap,
>
>   static struct rbd_snap *__rbd_add_snap_dev(struct rbd_device *rbd_dev,
>   						const char *snap_name,
> -						u64 snap_id, u64 snap_size)
> +						u64 snap_id, u64 snap_size,
> +						u64 snap_features)
>   {
>   	struct rbd_snap *snap;
>   	int ret;
> @@ -2048,6 +2086,7 @@ static struct rbd_snap *__rbd_add_snap_dev(struct
> rbd_device *rbd_dev,
>
>   	snap->id = snap_id;
>   	snap->size = snap_size;
> +	snap->features = snap_features;
>
>   	return snap;
>
> @@ -2118,7 +2157,7 @@ static int rbd_dev_snaps_update(struct rbd_device
> *rbd_dev)
>   			/* We haven't seen this snapshot before */
>
>   			new_snap = __rbd_add_snap_dev(rbd_dev, snap_name,
> -					snap_id, header->snap_sizes[index]);
> +					snap_id, header->snap_sizes[index], 0);
>   			if (IS_ERR(new_snap)) {
>   				int err = PTR_ERR(new_snap);
>

[PATCH 3/3] rbd: encapsulate code that gets snapshot info

[Alex Elder]

Create a function that encapsulates looking up the name, size and
features related to a given snapshot, which is indicated by its
index in an rbd device's snapshot context array of snapshot ids.

This interface will be used to hide differences between the format 1
and format 2 images.

At the moment this (looking up the name anyway) is slightly less
efficient than what's done currently, but we may be able to optimize
this a bit later on by cacheing the last lookup if it proves to be a
problem.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index c62d3f4..e6b8fa6 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2097,6 +2097,25 @@ err:
 	return ERR_PTR(ret);
 }

+static char *rbd_dev_v1_snap_info(struct rbd_device *rbd_dev, u32 which,
+		u64 *snap_size, u64 *snap_features)
+{
+	char *snap_name;
+
+	rbd_assert(which < rbd_dev->header.snapc->num_snaps);
+
+	*snap_size = rbd_dev->header.snap_sizes[which];
+	*snap_features = 0;	/* No features for v1 */
+
+	/* Skip over names until we find the one we are looking for */
+
+	snap_name = rbd_dev->header.snap_names;
+	while (which--)
+		snap_name += strlen(snap_name) + 1;
+
+	return snap_name;
+}
+
 /*
  * Scan the rbd device's current snapshot list and compare it to the
  * newly-received snapshot context.  Remove any existing snapshots
@@ -2113,7 +2132,6 @@ static int rbd_dev_snaps_update(struct rbd_device
*rbd_dev)
 {
 	struct ceph_snap_context *snapc = rbd_dev->header.snapc;
 	const u32 snap_count = snapc->num_snaps;
-	char *snap_name = rbd_dev->header.snap_names;
 	struct list_head *head = &rbd_dev->snaps;
 	struct list_head *links = head->next;
 	u32 index = 0;
@@ -2122,6 +2140,9 @@ static int rbd_dev_snaps_update(struct rbd_device
*rbd_dev)
 	while (index < snap_count || links != head) {
 		u64 snap_id;
 		struct rbd_snap *snap;
+		char *snap_name;
+		u64 snap_size = 0;
+		u64 snap_features = 0;

 		snap_id = index < snap_count ? snapc->snaps[index]
 					     : CEPH_NOSNAP;
@@ -2148,16 +2169,20 @@ static int rbd_dev_snaps_update(struct
rbd_device *rbd_dev)
 			continue;
 		}

+		snap_name = rbd_dev_v1_snap_info(rbd_dev, index,
+						&snap_size, &snap_features);
+		if (IS_ERR(snap_name))
+			return PTR_ERR(snap_name);
+
 		dout("entry %u: snap_id = %llu\n", (unsigned int) snap_count,
 			(unsigned long long) snap_id);
 		if (!snap || (snap_id != CEPH_NOSNAP && snap->id < snap_id)) {
-			struct rbd_image_header	*header = &rbd_dev->header;
 			struct rbd_snap *new_snap;

 			/* We haven't seen this snapshot before */

 			new_snap = __rbd_add_snap_dev(rbd_dev, snap_name,
-					snap_id, header->snap_sizes[index], 0);
+					snap_id, snap_size, snap_features);
 			if (IS_ERR(new_snap)) {
 				int err = PTR_ERR(new_snap);

@@ -2178,9 +2203,9 @@ static int rbd_dev_snaps_update(struct rbd_device
*rbd_dev)

 			dout("  already present\n");

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

 			/* Done with this list entry; advance */

@@ -2190,7 +2215,6 @@ static int rbd_dev_snaps_update(struct rbd_device
*rbd_dev)
 		/* Advance to the next entry in the snapshot context */

 		index++;
-		snap_name += strlen(snap_name) + 1;
 	}
 	dout("%s: done\n", __func__);

-- 
1.7.9.5

Re: [PATCH 3/3] rbd: encapsulate code that gets snapshot info

[Josh Durgin]

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

On 09/07/2012 12:25 PM, Alex Elder wrote:
> Create a function that encapsulates looking up the name, size and
> features related to a given snapshot, which is indicated by its
> index in an rbd device's snapshot context array of snapshot ids.
>
> This interface will be used to hide differences between the format 1
> and format 2 images.
>
> At the moment this (looking up the name anyway) is slightly less
> efficient than what's done currently, but we may be able to optimize
> this a bit later on by cacheing the last lookup if it proves to be a
> problem.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   36 ++++++++++++++++++++++++++++++------
>   1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index c62d3f4..e6b8fa6 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2097,6 +2097,25 @@ err:
>   	return ERR_PTR(ret);
>   }
>
> +static char *rbd_dev_v1_snap_info(struct rbd_device *rbd_dev, u32 which,
> +		u64 *snap_size, u64 *snap_features)
> +{
> +	char *snap_name;
> +
> +	rbd_assert(which < rbd_dev->header.snapc->num_snaps);
> +
> +	*snap_size = rbd_dev->header.snap_sizes[which];
> +	*snap_features = 0;	/* No features for v1 */
> +
> +	/* Skip over names until we find the one we are looking for */
> +
> +	snap_name = rbd_dev->header.snap_names;
> +	while (which--)
> +		snap_name += strlen(snap_name) + 1;
> +
> +	return snap_name;
> +}
> +
>   /*
>    * Scan the rbd device's current snapshot list and compare it to the
>    * newly-received snapshot context.  Remove any existing snapshots
> @@ -2113,7 +2132,6 @@ static int rbd_dev_snaps_update(struct rbd_device
> *rbd_dev)
>   {
>   	struct ceph_snap_context *snapc = rbd_dev->header.snapc;
>   	const u32 snap_count = snapc->num_snaps;
> -	char *snap_name = rbd_dev->header.snap_names;
>   	struct list_head *head = &rbd_dev->snaps;
>   	struct list_head *links = head->next;
>   	u32 index = 0;
> @@ -2122,6 +2140,9 @@ static int rbd_dev_snaps_update(struct rbd_device
> *rbd_dev)
>   	while (index < snap_count || links != head) {
>   		u64 snap_id;
>   		struct rbd_snap *snap;
> +		char *snap_name;
> +		u64 snap_size = 0;
> +		u64 snap_features = 0;
>
>   		snap_id = index < snap_count ? snapc->snaps[index]
>   					     : CEPH_NOSNAP;
> @@ -2148,16 +2169,20 @@ static int rbd_dev_snaps_update(struct
> rbd_device *rbd_dev)
>   			continue;
>   		}
>
> +		snap_name = rbd_dev_v1_snap_info(rbd_dev, index,
> +						&snap_size, &snap_features);
> +		if (IS_ERR(snap_name))
> +			return PTR_ERR(snap_name);
> +
>   		dout("entry %u: snap_id = %llu\n", (unsigned int) snap_count,
>   			(unsigned long long) snap_id);
>   		if (!snap || (snap_id != CEPH_NOSNAP && snap->id < snap_id)) {
> -			struct rbd_image_header	*header = &rbd_dev->header;
>   			struct rbd_snap *new_snap;
>
>   			/* We haven't seen this snapshot before */
>
>   			new_snap = __rbd_add_snap_dev(rbd_dev, snap_name,
> -					snap_id, header->snap_sizes[index], 0);
> +					snap_id, snap_size, snap_features);
>   			if (IS_ERR(new_snap)) {
>   				int err = PTR_ERR(new_snap);
>
> @@ -2178,9 +2203,9 @@ static int rbd_dev_snaps_update(struct rbd_device
> *rbd_dev)
>
>   			dout("  already present\n");
>
> -			rbd_assert(snap->size ==
> -					rbd_dev->header.snap_sizes[index]);
> +			rbd_assert(snap->size == snap_size);
>   			rbd_assert(!strcmp(snap->name, snap_name));
> +			rbd_assert(snap->features == snap_features);
>
>   			/* Done with this list entry; advance */
>
> @@ -2190,7 +2215,6 @@ static int rbd_dev_snaps_update(struct rbd_device
> *rbd_dev)
>   		/* Advance to the next entry in the snapshot context */
>
>   		index++;
> -		snap_name += strlen(snap_name) + 1;
>   	}
>   	dout("%s: done\n", __func__);
>