[PATCH 0/6] rbd: miscellaneous cleanups

[PATCH 0/6] rbd: miscellaneous cleanups

[Alex Elder]

This is just some odds and ends, cleaning up code mostly.
One might be considered a minor bug, but it's not serious.

					-Alex

[PATCH 1/6] rbd: update capacity in rbd_dev_refresh()
[PATCH 2/6] rbd: kill rbd_update_mapping_size()
[PATCH 3/6] rbd: don't print warning if not mapping a parent
[PATCH 4/6] rbd: don't look up snapshot id in rbd_dev_mapping_set()
[PATCH 5/6] rbd: kill rbd_dev_clear_mapping()
[PATCH 6/6] rbd: always set read-only flag in rbd_add()

[PATCH 1/6] rbd: update capacity in rbd_dev_refresh()

[Alex Elder]

When a mapped image changes size, we change the capacity recorded
for the Linux disk associated with it, in rbd_update_mapping_size().
That function is called in two places--the format 1 and format 2
refresh routines.

There is no need to set the capacity while holding the header
semaphore.  Instead, do it in the common rbd_dev_refresh(), using
the logic that's already there to initiate disk revalidation.

Add handling in the request function, just in case a request
that exceeds the capacity of the device comes in (perhaps one
that was started before a refresh shrunk the device).

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 5d5e3f0..a0f1fe5 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2874,6 +2874,13 @@ static void rbd_request_fn(struct request_queue *q)
 			goto end_request;	/* Shouldn't happen */
 		}

+		result = -ENOSPC;
+		if (offset + length > rbd_dev->mapping.size) {
+			rbd_warn(rbd_dev, "beyond EOD (%llu~%llu > %llu)\n",
+				offset, length, rbd_dev->mapping.size);
+			goto end_request;
+		}
+
 		result = -ENOMEM;
 		img_request = rbd_img_request_create(rbd_dev, offset, length,
 							write_request, false);
@@ -3116,14 +3123,8 @@ static void rbd_update_mapping_size(struct
rbd_device *rbd_dev)
 	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
 		return;

-	if (rbd_dev->mapping.size != rbd_dev->header.image_size) {
-		sector_t size;
-
+	if (rbd_dev->mapping.size != rbd_dev->header.image_size)
 		rbd_dev->mapping.size = rbd_dev->header.image_size;
-		size = (sector_t)rbd_dev->mapping.size / SECTOR_SIZE;
-		dout("setting size to %llu sectors", (unsigned long long)size);
-		set_capacity(rbd_dev->disk, size);
-	}
 }

 /*
@@ -3200,8 +3201,14 @@ static int rbd_dev_refresh(struct rbd_device
*rbd_dev)

 	rbd_exists_validate(rbd_dev);
 	mutex_unlock(&ctl_mutex);
-	if (mapping_size != rbd_dev->mapping.size)
+	if (mapping_size != rbd_dev->mapping.size) {
+		sector_t size;
+
+		size = (sector_t)rbd_dev->mapping.size / SECTOR_SIZE;
+		dout("setting size to %llu sectors", (unsigned long long)size);
+		set_capacity(rbd_dev->disk, size);
 		revalidate_disk(rbd_dev->disk);
+	}

 	return ret;
 }
-- 
1.7.9.5

Re: [PATCH 1/6] rbd: update capacity in rbd_dev_refresh()

[Josh Durgin]

On 05/06/2013 06:37 PM, Alex Elder wrote:
> When a mapped image changes size, we change the capacity recorded
> for the Linux disk associated with it, in rbd_update_mapping_size().
> That function is called in two places--the format 1 and format 2
> refresh routines.
>
> There is no need to set the capacity while holding the header
> semaphore.  Instead, do it in the common rbd_dev_refresh(), using
> the logic that's already there to initiate disk revalidation.
>
> Add handling in the request function, just in case a request
> that exceeds the capacity of the device comes in (perhaps one
> that was started before a refresh shrunk the device).
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 5d5e3f0..a0f1fe5 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2874,6 +2874,13 @@ static void rbd_request_fn(struct request_queue *q)
>   			goto end_request;	/* Shouldn't happen */
>   		}
>
> +		result = -ENOSPC;

blk-core.c usually uses -EIO for this case. That's more expected from a
read anyway, so I think -EIO would be better.

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

> +		if (offset + length > rbd_dev->mapping.size) {
> +			rbd_warn(rbd_dev, "beyond EOD (%llu~%llu > %llu)\n",
> +				offset, length, rbd_dev->mapping.size);
> +			goto end_request;
> +		}
> +
>   		result = -ENOMEM;
>   		img_request = rbd_img_request_create(rbd_dev, offset, length,
>   							write_request, false);
> @@ -3116,14 +3123,8 @@ static void rbd_update_mapping_size(struct
> rbd_device *rbd_dev)
>   	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
>   		return;
>
> -	if (rbd_dev->mapping.size != rbd_dev->header.image_size) {
> -		sector_t size;
> -
> +	if (rbd_dev->mapping.size != rbd_dev->header.image_size)
>   		rbd_dev->mapping.size = rbd_dev->header.image_size;
> -		size = (sector_t)rbd_dev->mapping.size / SECTOR_SIZE;
> -		dout("setting size to %llu sectors", (unsigned long long)size);
> -		set_capacity(rbd_dev->disk, size);
> -	}
>   }
>
>   /*
> @@ -3200,8 +3201,14 @@ static int rbd_dev_refresh(struct rbd_device
> *rbd_dev)
>
>   	rbd_exists_validate(rbd_dev);
>   	mutex_unlock(&ctl_mutex);
> -	if (mapping_size != rbd_dev->mapping.size)
> +	if (mapping_size != rbd_dev->mapping.size) {
> +		sector_t size;
> +
> +		size = (sector_t)rbd_dev->mapping.size / SECTOR_SIZE;
> +		dout("setting size to %llu sectors", (unsigned long long)size);
> +		set_capacity(rbd_dev->disk, size);
>   		revalidate_disk(rbd_dev->disk);
> +	}
>
>   	return ret;
>   }
>

[PATCH 2/6] rbd: kill rbd_update_mapping_size()

[Alex Elder]

Since rbd_update_mapping_size() is now a trivial wrapper, just open
code it in its two callers.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index a0f1fe5..0a24cdf 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3118,15 +3118,6 @@ static int rbd_read_header(struct rbd_device
*rbd_dev,
 	return ret;
 }

-static void rbd_update_mapping_size(struct rbd_device *rbd_dev)
-{
-	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
-		return;
-
-	if (rbd_dev->mapping.size != rbd_dev->header.image_size)
-		rbd_dev->mapping.size = rbd_dev->header.image_size;
-}
-
 /*
  * only read the first part of the ondisk header, without the snaps info
  */
@@ -3143,7 +3134,9 @@ static int rbd_dev_v1_refresh(struct rbd_device
*rbd_dev)

 	/* Update image size, and check for resize of mapped image */
 	rbd_dev->header.image_size = h.image_size;
-	rbd_update_mapping_size(rbd_dev);
+	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);
@@ -4074,7 +4067,9 @@ static int rbd_dev_v2_refresh(struct rbd_device
*rbd_dev)
 	ret = rbd_dev_v2_image_size(rbd_dev);
 	if (ret)
 		goto out;
-	rbd_update_mapping_size(rbd_dev);
+	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;

 	ret = rbd_dev_v2_snap_context(rbd_dev);
 	dout("rbd_dev_v2_snap_context returned %d\n", ret);
-- 
1.7.9.5

Re: [PATCH 2/6] rbd: kill rbd_update_mapping_size()

[Josh Durgin]

Alex Elder <elder@inktank.com> wrote:

>Since rbd_update_mapping_size() is now a trivial wrapper, just open
>code it in its two callers.
>
>Signed-off-by: Alex Elder <elder@inktank.com>
>---
> drivers/block/rbd.c |   17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>index a0f1fe5..0a24cdf 100644
>--- a/drivers/block/rbd.c
>+++ b/drivers/block/rbd.c
>@@ -3118,15 +3118,6 @@ static int rbd_read_header(struct rbd_device
>*rbd_dev,
> 	return ret;
> }
>
>-static void rbd_update_mapping_size(struct rbd_device *rbd_dev)
>-{
>-	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
>-		return;
>-
>-	if (rbd_dev->mapping.size != rbd_dev->header.image_size)
>-		rbd_dev->mapping.size = rbd_dev->header.image_size;
>-}
>-
> /*
>* only read the first part of the ondisk header, without the snaps info
>  */
>@@ -3143,7 +3134,9 @@ static int rbd_dev_v1_refresh(struct rbd_device
>*rbd_dev)
>
> 	/* Update image size, and check for resize of mapped image */
> 	rbd_dev->header.image_size = h.image_size;
>-	rbd_update_mapping_size(rbd_dev);
>+	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;

Using && instead of two conditions would be a bit cleaner. Maybe you
have later patches that depend on this though. Open coding looks fine to
me in any case.

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

>
> 	/* rbd_dev->header.object_prefix shouldn't change */
> 	kfree(rbd_dev->header.snap_sizes);
>@@ -4074,7 +4067,9 @@ static int rbd_dev_v2_refresh(struct rbd_device
>*rbd_dev)
> 	ret = rbd_dev_v2_image_size(rbd_dev);
> 	if (ret)
> 		goto out;
>-	rbd_update_mapping_size(rbd_dev);
>+	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;
>
> 	ret = rbd_dev_v2_snap_context(rbd_dev);
> 	dout("rbd_dev_v2_snap_context returned %d\n", ret);

Re: [PATCH 2/6] rbd: kill rbd_update_mapping_size()

[Alex Elder]

On 05/07/2013 09:56 AM, Josh Durgin wrote:
> Alex Elder <elder@inktank.com> wrote:
> 
>> Since rbd_update_mapping_size() is now a trivial wrapper, just open
>> code it in its two callers.
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>> drivers/block/rbd.c |   17 ++++++-----------
>> 1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index a0f1fe5..0a24cdf 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -3118,15 +3118,6 @@ static int rbd_read_header(struct rbd_device
>> *rbd_dev,
>>     return ret;
>> }
>>
>> -static void rbd_update_mapping_size(struct rbd_device *rbd_dev)
>> -{
>> -    if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
>> -        return;
>> -
>> -    if (rbd_dev->mapping.size != rbd_dev->header.image_size)
>> -        rbd_dev->mapping.size = rbd_dev->header.image_size;
>> -}
>> -
>> /*
>> * only read the first part of the ondisk header, without the snaps info
>>  */
>> @@ -3143,7 +3134,9 @@ static int rbd_dev_v1_refresh(struct rbd_device
>> *rbd_dev)
>>
>>     /* Update image size, and check for resize of mapped image */
>>     rbd_dev->header.image_size = h.image_size;
>> -    rbd_update_mapping_size(rbd_dev);
>> +    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;
> 
> Using && instead of two conditions would be a bit cleaner. Maybe you
> have later patches that depend on this though. Open coding looks fine to
> me in any case.

Honestly, that's the way I had it, but I looked at both
and liked this better, I think because the lines ended up
long or something.

I'll take another look before I commit it.

					-Alex


> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
> 
>>
>>     /* rbd_dev->header.object_prefix shouldn't change */
>>     kfree(rbd_dev->header.snap_sizes);
>> @@ -4074,7 +4067,9 @@ static int rbd_dev_v2_refresh(struct rbd_device
>> *rbd_dev)
>>     ret = rbd_dev_v2_image_size(rbd_dev);
>>     if (ret)
>>         goto out;
>> -    rbd_update_mapping_size(rbd_dev);
>> +    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;
>>
>>     ret = rbd_dev_v2_snap_context(rbd_dev);
>>     dout("rbd_dev_v2_snap_context returned %d\n", ret);
> 

[PATCH 3/6] rbd: don't print warning if not mapping a parent

[Alex Elder]

The presence of the LAYERING bit in an rbd image's feature mask does
not guarantee the image actually has a parent image.  Currently that
bit is set only when a clone (i.e., image with a parent) is created,
but it is (currently) not cleared if that clone gets flattened back
into a "normal" image.  A "parent_id" query will leave the
parent_spec for the image being mapped a null pointer, but will not
return an error.

Currently, whenever an image with the LAYERED feature gets mapped, a
warning about the use of layered images gets printed.  But we don't
want to do this for a flattened image, so print the warning only
if we find there is a parent spec after the probe.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 0a24cdf..7d93dbd 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4567,13 +4567,14 @@ static int rbd_dev_v2_probe(struct rbd_device
*rbd_dev)
 		ret = rbd_dev_v2_parent_info(rbd_dev);
 		if (ret)
 			goto out_err;
-
 		/*
-		 * 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).
+		 * Print a warning if this image has a parent.
+		 * Don't print it if the image now being probed
+		 * is itself a parent.  We can tell at this point
+		 * because we won't know its pool name yet (just its
+		 * pool id).
 		 */
-		if (rbd_dev->spec->pool_name)
+		if (rbd_dev->parent_spec && rbd_dev->spec->pool_name)
 			rbd_warn(rbd_dev, "WARNING: kernel layering "
 					"is EXPERIMENTAL!");
 	}
-- 
1.7.9.5

Re: [PATCH 3/6] rbd: don't print warning if not mapping a parent

[Josh Durgin]

Alex Elder <elder@inktank.com> wrote:

>The presence of the LAYERING bit in an rbd image's feature mask does
>not guarantee the image actually has a parent image.  Currently that
>bit is set only when a clone (i.e., image with a parent) is created,
>but it is (currently) not cleared if that clone gets flattened back
>into a "normal" image.  A "parent_id" query will leave the
>parent_spec for the image being mapped a null pointer, but will not
>return an error.
>
>Currently, whenever an image with the LAYERED feature gets mapped, a
>warning about the use of layered images gets printed.  But we don't
>want to do this for a flattened image, so print the warning only
>if we find there is a parent spec after the probe.
>
>Signed-off-by: Alex Elder <elder@inktank.com>
>---
> drivers/block/rbd.c |   11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>index 0a24cdf..7d93dbd 100644
>--- a/drivers/block/rbd.c
>+++ b/drivers/block/rbd.c
>@@ -4567,13 +4567,14 @@ static int rbd_dev_v2_probe(struct rbd_device
>*rbd_dev)
> 		ret = rbd_dev_v2_parent_info(rbd_dev);
> 		if (ret)
> 			goto out_err;
>-
> 		/*
>-		 * 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).
>+		 * Print a warning if this image has a parent.
>+		 * Don't print it if the image now being probed
>+		 * is itself a parent.  We can tell at this point
>+		 * because we won't know its pool name yet (just its
>+		 * pool id).
> 		 */
>-		if (rbd_dev->spec->pool_name)
>+		if (rbd_dev->parent_spec && rbd_dev->spec->pool_name)
> 			rbd_warn(rbd_dev, "WARNING: kernel layering "
> 					"is EXPERIMENTAL!");
> 	}

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

[PATCH 4/6] rbd: don't look up snapshot id in rbd_dev_mapping_set()

[Alex Elder]

Currently rbd_dev_mapping_set() looks up the snapshot id for the
snapshot whose name is found in the rbd device's spec structure.

That function gets called by rbd_dev_device_setup(), which is
called by rbd_add() *after* rbd_dev_image_probe().  If the
image probe succeeds, the rbd device's spec will already have
been updated to include names and ids for all fields.

Therefore there's no need to look up the snapshot id in
rbd_dev_mapping_set().

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 7d93dbd..5eebf6d 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -936,20 +936,11 @@ static int rbd_snap_features(struct rbd_device
*rbd_dev, u64 snap_id,

 static int rbd_dev_mapping_set(struct rbd_device *rbd_dev)
 {
-	const char *snap_name = rbd_dev->spec->snap_name;
-	u64 snap_id;
+	u64 snap_id = rbd_dev->spec->snap_id;
 	u64 size = 0;
 	u64 features = 0;
 	int ret;

-	if (strcmp(snap_name, RBD_SNAP_HEAD_NAME)) {
-		snap_id = rbd_snap_id_by_name(rbd_dev, snap_name);
-		if (snap_id == CEPH_NOSNAP)
-			return -ENOENT;
-	} else {
-		snap_id = CEPH_NOSNAP;
-	}
-
 	ret = rbd_snap_size(rbd_dev, snap_id, &size);
 	if (ret)
 		return ret;
-- 
1.7.9.5

Re: [PATCH 4/6] rbd: don't look up snapshot id in rbd_dev_mapping_set()

[Josh Durgin]

Alex Elder <elder@inktank.com> wrote:

>Currently rbd_dev_mapping_set() looks up the snapshot id for the
>snapshot whose name is found in the rbd device's spec structure.
>
>That function gets called by rbd_dev_device_setup(), which is
>called by rbd_add() *after* rbd_dev_image_probe().  If the
>image probe succeeds, the rbd device's spec will already have
>been updated to include names and ids for all fields.
>
>Therefore there's no need to look up the snapshot id in
>rbd_dev_mapping_set().
>
>Signed-off-by: Alex Elder <elder@inktank.com>
>---
> drivers/block/rbd.c |   11 +----------
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
>diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>index 7d93dbd..5eebf6d 100644
>--- a/drivers/block/rbd.c
>+++ b/drivers/block/rbd.c
>@@ -936,20 +936,11 @@ static int rbd_snap_features(struct rbd_device
>*rbd_dev, u64 snap_id,
>
> static int rbd_dev_mapping_set(struct rbd_device *rbd_dev)
> {
>-	const char *snap_name = rbd_dev->spec->snap_name;
>-	u64 snap_id;
>+	u64 snap_id = rbd_dev->spec->snap_id;
> 	u64 size = 0;
> 	u64 features = 0;
> 	int ret;
>
>-	if (strcmp(snap_name, RBD_SNAP_HEAD_NAME)) {
>-		snap_id = rbd_snap_id_by_name(rbd_dev, snap_name);
>-		if (snap_id == CEPH_NOSNAP)
>-			return -ENOENT;
>-	} else {
>-		snap_id = CEPH_NOSNAP;
>-	}
>-
> 	ret = rbd_snap_size(rbd_dev, snap_id, &size);
> 	if (ret)
> 		return ret;

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

[PATCH 5/6] rbd: kill rbd_dev_clear_mapping()

[Alex Elder]

This function is a duplicate of rbd_dev_mapping_clear(), and was
added by mistake.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 5eebf6d..530793a 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -966,13 +966,6 @@ static void rbd_dev_mapping_clear(struct rbd_device
*rbd_dev)
 	rbd_dev->mapping.read_only = true;
 }

-static void rbd_dev_clear_mapping(struct rbd_device *rbd_dev)
-{
-	rbd_dev->mapping.size = 0;
-	rbd_dev->mapping.features = 0;
-	rbd_dev->mapping.read_only = true;
-}
-
 static const char *rbd_segment_name(struct rbd_device *rbd_dev, u64 offset)
 {
 	char *name;
@@ -4910,7 +4903,7 @@ static void rbd_dev_device_release(struct device *dev)

 	rbd_free_disk(rbd_dev);
 	clear_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags);
-	rbd_dev_clear_mapping(rbd_dev);
+	rbd_dev_mapping_clear(rbd_dev);
 	unregister_blkdev(rbd_dev->major, rbd_dev->name);
 	rbd_dev->major = 0;
 	rbd_dev_id_put(rbd_dev);
-- 
1.7.9.5

Re: [PATCH 5/6] rbd: kill rbd_dev_clear_mapping()

[Josh Durgin]

Alex Elder <elder@inktank.com> wrote:

>This function is a duplicate of rbd_dev_mapping_clear(), and was
>added by mistake.
>
>Signed-off-by: Alex Elder <elder@inktank.com>
>---
> drivers/block/rbd.c |    9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
>diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>index 5eebf6d..530793a 100644
>--- a/drivers/block/rbd.c
>+++ b/drivers/block/rbd.c
>@@ -966,13 +966,6 @@ static void rbd_dev_mapping_clear(struct
>rbd_device
>*rbd_dev)
> 	rbd_dev->mapping.read_only = true;
> }
>
>-static void rbd_dev_clear_mapping(struct rbd_device *rbd_dev)
>-{
>-	rbd_dev->mapping.size = 0;
>-	rbd_dev->mapping.features = 0;
>-	rbd_dev->mapping.read_only = true;
>-}
>-
>static const char *rbd_segment_name(struct rbd_device *rbd_dev, u64
>offset)
> {
> 	char *name;
>@@ -4910,7 +4903,7 @@ static void rbd_dev_device_release(struct device
>*dev)
>
> 	rbd_free_disk(rbd_dev);
> 	clear_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags);
>-	rbd_dev_clear_mapping(rbd_dev);
>+	rbd_dev_mapping_clear(rbd_dev);
> 	unregister_blkdev(rbd_dev->major, rbd_dev->name);
> 	rbd_dev->major = 0;
> 	rbd_dev_id_put(rbd_dev);

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

[PATCH 6/6] rbd: always set read-only flag in rbd_add()

[Alex Elder]

Hold off setting the read-only flag in rbd_add() for an image being
mapped until we have successfully probed the image.  At that point
we know whether it's a snapshot mapping or not, so we can set the
read-only flag in that one place rather than doing so (for
snapshots) in rbd_dev_mapping_set().  To do this, pass a flag to the
image probe routine indicating whether we want a read-only mapping.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 530793a..0c72643 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -359,7 +359,7 @@ static ssize_t rbd_add(struct bus_type *bus, const
char *buf,
 		       size_t count);
 static ssize_t rbd_remove(struct bus_type *bus, const char *buf,
 			  size_t count);
-static int rbd_dev_image_probe(struct rbd_device *rbd_dev);
+static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool read_only);

 static struct bus_attribute rbd_bus_attrs[] = {
 	__ATTR(add, S_IWUSR, NULL, rbd_add),
@@ -951,11 +951,6 @@ static int rbd_dev_mapping_set(struct rbd_device
*rbd_dev)
 	rbd_dev->mapping.size = size;
 	rbd_dev->mapping.features = features;

-	/* If we are mapping a snapshot it must be marked read-only */
-
-	if (snap_id != CEPH_NOSNAP)
-		rbd_dev->mapping.read_only = true;
-
 	return 0;
 }

@@ -963,7 +958,6 @@ static void rbd_dev_mapping_clear(struct rbd_device
*rbd_dev)
 {
 	rbd_dev->mapping.size = 0;
 	rbd_dev->mapping.features = 0;
-	rbd_dev->mapping.read_only = true;
 }

 static const char *rbd_segment_name(struct rbd_device *rbd_dev, u64 offset)
@@ -4620,7 +4614,7 @@ static int rbd_dev_probe_parent(struct rbd_device
*rbd_dev)
 	if (!parent)
 		goto out_err;

-	ret = rbd_dev_image_probe(parent);
+	ret = rbd_dev_image_probe(parent, true);
 	if (ret < 0)
 		goto out_err;
 	rbd_dev->parent = parent;
@@ -4743,7 +4737,7 @@ static void rbd_dev_image_release(struct
rbd_device *rbd_dev)
  * device.  For format 2 images this includes determining the image
  * id.
  */
-static int rbd_dev_image_probe(struct rbd_device *rbd_dev)
+static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool read_only)
 {
 	int ret;
 	int tmp;
@@ -4778,6 +4772,12 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev)
 	if (ret)
 		goto err_out_probe;

+	/* If we are mapping a snapshot it must be marked read-only */
+
+	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
+		read_only = true;
+	rbd_dev->mapping.read_only = read_only;
+
 	ret = rbd_dev_probe_parent(rbd_dev);
 	if (!ret)
 		return 0;
@@ -4811,6 +4811,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	struct rbd_spec *spec = NULL;
 	struct rbd_client *rbdc;
 	struct ceph_osd_client *osdc;
+	bool read_only;
 	int rc = -ENOMEM;

 	if (!try_module_get(THIS_MODULE))
@@ -4820,6 +4821,9 @@ static ssize_t rbd_add(struct bus_type *bus,
 	rc = rbd_add_parse_args(buf, &ceph_opts, &rbd_opts, &spec);
 	if (rc < 0)
 		goto err_out_module;
+	read_only = rbd_opts->read_only;
+	kfree(rbd_opts);
+	rbd_opts = NULL;	/* done with this */

 	rbdc = rbd_get_client(ceph_opts);
 	if (IS_ERR(rbdc)) {
@@ -4850,11 +4854,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	rbdc = NULL;		/* rbd_dev now owns this */
 	spec = NULL;		/* rbd_dev now owns this */

-	rbd_dev->mapping.read_only = rbd_opts->read_only;
-	kfree(rbd_opts);
-	rbd_opts = NULL;	/* done with this */
-
-	rc = rbd_dev_image_probe(rbd_dev);
+	rc = rbd_dev_image_probe(rbd_dev, read_only);
 	if (rc < 0)
 		goto err_out_rbd_dev;

-- 
1.7.9.5

Re: [PATCH 6/6] rbd: always set read-only flag in rbd_add()

[Josh Durgin]

On 05/06/2013 06:38 PM, Alex Elder wrote:
> Hold off setting the read-only flag in rbd_add() for an image being
> mapped until we have successfully probed the image.  At that point
> we know whether it's a snapshot mapping or not, so we can set the
> read-only flag in that one place rather than doing so (for
> snapshots) in rbd_dev_mapping_set().  To do this, pass a flag to the
> image probe routine indicating whether we want a read-only mapping.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 530793a..0c72643 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -359,7 +359,7 @@ static ssize_t rbd_add(struct bus_type *bus, const
> char *buf,
>   		       size_t count);
>   static ssize_t rbd_remove(struct bus_type *bus, const char *buf,
>   			  size_t count);
> -static int rbd_dev_image_probe(struct rbd_device *rbd_dev);
> +static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool read_only);
>
>   static struct bus_attribute rbd_bus_attrs[] = {
>   	__ATTR(add, S_IWUSR, NULL, rbd_add),
> @@ -951,11 +951,6 @@ static int rbd_dev_mapping_set(struct rbd_device
> *rbd_dev)
>   	rbd_dev->mapping.size = size;
>   	rbd_dev->mapping.features = features;
>
> -	/* If we are mapping a snapshot it must be marked read-only */
> -
> -	if (snap_id != CEPH_NOSNAP)
> -		rbd_dev->mapping.read_only = true;
> -
>   	return 0;
>   }
>
> @@ -963,7 +958,6 @@ static void rbd_dev_mapping_clear(struct rbd_device
> *rbd_dev)
>   {
>   	rbd_dev->mapping.size = 0;
>   	rbd_dev->mapping.features = 0;
> -	rbd_dev->mapping.read_only = true;
>   }
>
>   static const char *rbd_segment_name(struct rbd_device *rbd_dev, u64 offset)
> @@ -4620,7 +4614,7 @@ static int rbd_dev_probe_parent(struct rbd_device
> *rbd_dev)
>   	if (!parent)
>   		goto out_err;
>
> -	ret = rbd_dev_image_probe(parent);
> +	ret = rbd_dev_image_probe(parent, true);
>   	if (ret < 0)
>   		goto out_err;
>   	rbd_dev->parent = parent;
> @@ -4743,7 +4737,7 @@ static void rbd_dev_image_release(struct
> rbd_device *rbd_dev)
>    * device.  For format 2 images this includes determining the image
>    * id.
>    */
> -static int rbd_dev_image_probe(struct rbd_device *rbd_dev)
> +static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool read_only)
>   {
>   	int ret;
>   	int tmp;
> @@ -4778,6 +4772,12 @@ static int rbd_dev_image_probe(struct rbd_device
> *rbd_dev)
>   	if (ret)
>   		goto err_out_probe;
>
> +	/* If we are mapping a snapshot it must be marked read-only */
> +
> +	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
> +		read_only = true;
> +	rbd_dev->mapping.read_only = read_only;
> +
>   	ret = rbd_dev_probe_parent(rbd_dev);
>   	if (!ret)
>   		return 0;
> @@ -4811,6 +4811,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	struct rbd_spec *spec = NULL;
>   	struct rbd_client *rbdc;
>   	struct ceph_osd_client *osdc;
> +	bool read_only;
>   	int rc = -ENOMEM;
>
>   	if (!try_module_get(THIS_MODULE))
> @@ -4820,6 +4821,9 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	rc = rbd_add_parse_args(buf, &ceph_opts, &rbd_opts, &spec);
>   	if (rc < 0)
>   		goto err_out_module;
> +	read_only = rbd_opts->read_only;
> +	kfree(rbd_opts);
> +	rbd_opts = NULL;	/* done with this */
>
>   	rbdc = rbd_get_client(ceph_opts);
>   	if (IS_ERR(rbdc)) {
> @@ -4850,11 +4854,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	rbdc = NULL;		/* rbd_dev now owns this */
>   	spec = NULL;		/* rbd_dev now owns this */
>
> -	rbd_dev->mapping.read_only = rbd_opts->read_only;
> -	kfree(rbd_opts);
> -	rbd_opts = NULL;	/* done with this */

It looks like this was moved accidentally? Maybe needed
by a later patch?

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

> -
> -	rc = rbd_dev_image_probe(rbd_dev);
> +	rc = rbd_dev_image_probe(rbd_dev, read_only);
>   	if (rc < 0)
>   		goto err_out_rbd_dev;
>

Re: [PATCH 6/6] rbd: always set read-only flag in rbd_add()

[Alex Elder]

On 05/07/2013 05:51 PM, Josh Durgin wrote:
> On 05/06/2013 06:38 PM, Alex Elder wrote:
>> Hold off setting the read-only flag in rbd_add() for an image being
>> mapped until we have successfully probed the image.  At that point
>> we know whether it's a snapshot mapping or not, so we can set the
>> read-only flag in that one place rather than doing so (for
>> snapshots) in rbd_dev_mapping_set().  To do this, pass a flag to the
>> image probe routine indicating whether we want a read-only mapping.
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |   28 ++++++++++++++--------------
>>   1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 530793a..0c72643 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c

. . .

>> @@ -4850,11 +4854,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>>       rbdc = NULL;        /* rbd_dev now owns this */
>>       spec = NULL;        /* rbd_dev now owns this */
>>
>> -    rbd_dev->mapping.read_only = rbd_opts->read_only;
>> -    kfree(rbd_opts);
>> -    rbd_opts = NULL;    /* done with this */
> 
> It looks like this was moved accidentally? Maybe needed
> by a later patch?

No, I changed to grabbing the read-only flag in a local
variable, and as long as I was doing that I moved it
up right after argument parsing so I could free the
options structure right away.

					-Alex

> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
> 
>> -
>> -    rc = rbd_dev_image_probe(rbd_dev);
>> +    rc = rbd_dev_image_probe(rbd_dev, read_only);
>>       if (rc < 0)
>>           goto err_out_rbd_dev;
>>
>