[PATCH 0/3] rbd: a few small issues

[PATCH 0/3] rbd: a few small issues

[Alex Elder]

This series has two patches that fix two annoyances
and a third that makes snapshot names be treated
consistently as constant data.

					-Alex

[PATCH 1/3] rbd: fix up the layering warning message
[PATCH 2/3] rbd: don't revalidate so much
[PATCH 3/3] rbd: snap names are pointer to constant data

[PATCH 1/3] rbd: fix up the layering warning message

[Alex Elder]

A warning gets spewed for any image being probed, including parent
images.  Set up a condition such that the warning message only gets
printed for the image being mapped, not any of its parents.

Also, I didn't like the way the warning ended up being so long.
Make it a terse warning instead.  People experimenting with layering
will know what the message means.

This is part of:
    http://tracker.ceph.com/issues/4867

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 8b30d83..176a3d7 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4624,8 +4624,15 @@ static int rbd_dev_v2_probe(struct rbd_device
*rbd_dev)
 		ret = rbd_dev_v2_parent_info(rbd_dev);
 		if (ret)
 			goto out_err;
-		rbd_warn(rbd_dev, "WARNING: kernel support for "
-					"layered rbd images is EXPERIMENTAL!");
+
+		/*
+		 * Don't print a warning for parent images.  We can
+		 * tell this point because we won't know its pool
+		 * name yet (just its pool id).
+		 */
+		if (rbd_dev->spec->pool_name)
+			rbd_warn(rbd_dev, "WARNING: kernel layering "
+					"is EXPERIMENTAL!");
 	}

 	/* If the image supports fancy striping, get its parameters */
-- 
1.7.9.5

[PATCH 2/3] rbd: don't revalidate so much

[Alex Elder]

Whenever a header object event causes a mapped rbd image to refresh
its header information, revalidate_disk() is being called.  This was
done in rbd_dev_refresh() outside the control mutex in order to
avoid a lock inversion.  Although a an event like this *might*
indicate the image has changed size, most of the time it does not.

Record the image size before and after the refresh, and only
call revalidate_disk() if it changes.

This resolves:
    http://tracker.ceph.com/issues/4867

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 176a3d7..dbc61e3 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3065,19 +3065,22 @@ static int rbd_dev_v1_refresh(struct rbd_device
*rbd_dev, u64 *hver)

 static int rbd_dev_refresh(struct rbd_device *rbd_dev, u64 *hver)
 {
+	u64 image_size;
 	int ret;

 	rbd_assert(rbd_image_format_valid(rbd_dev->image_format));
+	image_size = rbd_dev->header.image_size;
 	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
 	if (rbd_dev->image_format == 1)
 		ret = rbd_dev_v1_refresh(rbd_dev, hver);
 	else
 		ret = rbd_dev_v2_refresh(rbd_dev, hver);
 	mutex_unlock(&ctl_mutex);
-	revalidate_disk(rbd_dev->disk);
 	if (ret)
 		rbd_warn(rbd_dev, "got notification but failed to "
 			   " update snaps: %d\n", ret);
+	if (image_size != rbd_dev->header.image_size)
+		revalidate_disk(rbd_dev->disk);

 	return ret;
 }
-- 
1.7.9.5

[PATCH 3/3] rbd: snap names are pointer to constant data

[Alex Elder]

Make explicit that snapshot names don't change by making functions
return and take parameters that that point to const qualified data.

This resolves:
    http://tracker.ceph.com/issues/4857

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index dbc61e3..ab5c901 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3435,10 +3435,10 @@ static struct rbd_snap *rbd_snap_create(struct
rbd_device *rbd_dev,
  * Returns a dynamically-allocated snapshot name if successful, or a
  * pointer-coded error otherwise.
  */
-static char *rbd_dev_v1_snap_info(struct rbd_device *rbd_dev, u32 which,
+static const char *rbd_dev_v1_snap_info(struct rbd_device *rbd_dev, u32
which,
 		u64 *snap_size, u64 *snap_features)
 {
-	char *snap_name;
+	const char *snap_name;
 	int i;

 	rbd_assert(which < rbd_dev->header.snapc->num_snaps);
@@ -3907,7 +3907,7 @@ out:
 	return ret;
 }

-static char *rbd_dev_v2_snap_name(struct rbd_device *rbd_dev, u32 which)
+static const char *rbd_dev_v2_snap_name(struct rbd_device *rbd_dev, u32
which)
 {
 	size_t size;
 	void *reply_buf;
@@ -3948,13 +3948,13 @@ out:
 	return snap_name;
 }

-static char *rbd_dev_v2_snap_info(struct rbd_device *rbd_dev, u32 which,
+static const char *rbd_dev_v2_snap_info(struct rbd_device *rbd_dev, u32
which,
 		u64 *snap_size, u64 *snap_features)
 {
 	u64 snap_id;
 	u64 size;
 	u64 features;
-	char *snap_name;
+	const char *snap_name;
 	int ret;

 	rbd_assert(which < rbd_dev->header.snapc->num_snaps);
@@ -3978,7 +3978,7 @@ out_err:
 	return ERR_PTR(ret);
 }

-static char *rbd_dev_snap_info(struct rbd_device *rbd_dev, u32 which,
+static const char *rbd_dev_snap_info(struct rbd_device *rbd_dev, u32 which,
 		u64 *snap_size, u64 *snap_features)
 {
 	if (rbd_dev->image_format == 1)
@@ -4045,7 +4045,7 @@ 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;
+		const char *snap_name;
 		u64 snap_size = 0;
 		u64 snap_features = 0;

-- 
1.7.9.5

Re: [PATCH 0/3] rbd: a few small issues

[Josh Durgin]

On 04/30/2013 05:22 AM, Alex Elder wrote:
> This series has two patches that fix two annoyances
> and a third that makes snapshot names be treated
> consistently as constant data.
>
> 					-Alex
>
> [PATCH 1/3] rbd: fix up the layering warning message
> [PATCH 2/3] rbd: don't revalidate so much
> [PATCH 3/3] rbd: snap names are pointer to constant data

These all look good.

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