[PATCH 0/2] rbd: two bugs

[PATCH 0/2] rbd: two bugs

[Alex Elder]

This series fixes two bugs, one that affects rbd format 2
images only, and one that fixes an inadvertent change in
an earlier commit.

					-Alex

[PATCH 1/2] rbd: zero return code in rbd_dev_image_id()
[PATCH 2/2] rbd: fix read-only option name

[PATCH 1/2] rbd: zero return code in rbd_dev_image_id()

[Alex Elder]

There is a call in rbd_dev_image_id() to rbd_req_sync_exec()
to get the image id for an image.  Despite the "get_id" class
method only returning 0 on success, I am getting back a positive
value (I think the number of bytes returned with the call).

That may or may not be how rbd_req_sync_exec() is supposed to
behave, but zeroing the return value if successful makes it moot
and makes this path through the code work as desired.

Do the same in rbd_dev_v2_object_prefix().

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index cf5d109..65e9f1f 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2169,6 +2169,7 @@ static int rbd_dev_v2_object_prefix(struct
rbd_device *rbd_dev)
 	dout("%s: rbd_req_sync_exec returned %d\n", __func__, ret);
 	if (ret < 0)
 		goto out;
+	ret = 0;    /* rbd_req_sync_exec() can return positive */

 	p = reply_buf;
 	rbd_dev->header.object_prefix = ceph_extract_encoded_string(&p,
@@ -2862,6 +2863,7 @@ static int rbd_dev_image_id(struct rbd_device
*rbd_dev)
 	dout("%s: rbd_req_sync_exec returned %d\n", __func__, ret);
 	if (ret < 0)
 		goto out;
+	ret = 0;    /* rbd_req_sync_exec() can return positive */

 	p = response;
 	rbd_dev->image_id = ceph_extract_encoded_string(&p,
-- 
1.7.9.5

[PATCH 2/2] rbd: fix read-only option name

[Alex Elder]

The name of the "read-only" mapping option was inadvertently changed
in this commit:

    f84344f3 rbd: separate mapping info in rbd_dev

Revert that hunk to return it to what it should be.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 65e9f1f..d032883 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -397,7 +397,7 @@ enum {
 static match_table_t rbd_opts_tokens = {
 	/* int args above */
 	/* string args above */
-	{Opt_read_only, "mapping.read_only"},
+	{Opt_read_only, "read_only"},
 	{Opt_read_only, "ro"},		/* Alternate spelling */
 	{Opt_read_write, "read_write"},
 	{Opt_read_write, "rw"},		/* Alternate spelling */
-- 
1.7.9.5

Re: [PATCH 1/2] rbd: zero return code in rbd_dev_image_id()

[Dan Mick]

I really feel like we ought to root-cause this before we patch the 
kernel client.  Something isn't working the way we expect.

On 10/22/2012 09:44 AM, Alex Elder wrote:
> There is a call in rbd_dev_image_id() to rbd_req_sync_exec()
> to get the image id for an image.  Despite the "get_id" class
> method only returning 0 on success, I am getting back a positive
> value (I think the number of bytes returned with the call).
>
> That may or may not be how rbd_req_sync_exec() is supposed to
> behave, but zeroing the return value if successful makes it moot
> and makes this path through the code work as desired.
>
> Do the same in rbd_dev_v2_object_prefix().
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index cf5d109..65e9f1f 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2169,6 +2169,7 @@ static int rbd_dev_v2_object_prefix(struct
> rbd_device *rbd_dev)
>   	dout("%s: rbd_req_sync_exec returned %d\n", __func__, ret);
>   	if (ret < 0)
>   		goto out;
> +	ret = 0;    /* rbd_req_sync_exec() can return positive */
>
>   	p = reply_buf;
>   	rbd_dev->header.object_prefix = ceph_extract_encoded_string(&p,
> @@ -2862,6 +2863,7 @@ static int rbd_dev_image_id(struct rbd_device
> *rbd_dev)
>   	dout("%s: rbd_req_sync_exec returned %d\n", __func__, ret);
>   	if (ret < 0)
>   		goto out;
> +	ret = 0;    /* rbd_req_sync_exec() can return positive */
>
>   	p = response;
>   	rbd_dev->image_id = ceph_extract_encoded_string(&p,
>

Re: [PATCH 1/2] rbd: zero return code in rbd_dev_image_id()

[Dan Mick]

OK, assuming the comment is changed to reflect the "we expect a data 
length back here" indication, Alex explained this offline and I agree.


On 10/22/2012 10:18 AM, Dan Mick wrote:
> I really feel like we ought to root-cause this before we patch the
> kernel client.  Something isn't working the way we expect.
>
> On 10/22/2012 09:44 AM, Alex Elder wrote:
>> There is a call in rbd_dev_image_id() to rbd_req_sync_exec()
>> to get the image id for an image.  Despite the "get_id" class
>> method only returning 0 on success, I am getting back a positive
>> value (I think the number of bytes returned with the call).
>>
>> That may or may not be how rbd_req_sync_exec() is supposed to
>> behave, but zeroing the return value if successful makes it moot
>> and makes this path through the code work as desired.
>>
>> Do the same in rbd_dev_v2_object_prefix().
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index cf5d109..65e9f1f 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -2169,6 +2169,7 @@ static int rbd_dev_v2_object_prefix(struct
>> rbd_device *rbd_dev)
>>       dout("%s: rbd_req_sync_exec returned %d\n", __func__, ret);
>>       if (ret < 0)
>>           goto out;
>> +    ret = 0;    /* rbd_req_sync_exec() can return positive */
>>
>>       p = reply_buf;
>>       rbd_dev->header.object_prefix = ceph_extract_encoded_string(&p,
>> @@ -2862,6 +2863,7 @@ static int rbd_dev_image_id(struct rbd_device
>> *rbd_dev)
>>       dout("%s: rbd_req_sync_exec returned %d\n", __func__, ret);
>>       if (ret < 0)
>>           goto out;
>> +    ret = 0;    /* rbd_req_sync_exec() can return positive */
>>
>>       p = response;
>>       rbd_dev->image_id = ceph_extract_encoded_string(&p,
>>

Re: [PATCH 1/2] rbd: zero return code in rbd_dev_image_id()

[Josh Durgin]

Now that we tracked down the cause of the positive return value to the
client side the commit message could be updated.

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

On 10/22/2012 09:44 AM, Alex Elder wrote:
> There is a call in rbd_dev_image_id() to rbd_req_sync_exec()
> to get the image id for an image.  Despite the "get_id" class
> method only returning 0 on success, I am getting back a positive
> value (I think the number of bytes returned with the call).
>
> That may or may not be how rbd_req_sync_exec() is supposed to
> behave, but zeroing the return value if successful makes it moot
> and makes this path through the code work as desired.
>
> Do the same in rbd_dev_v2_object_prefix().
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index cf5d109..65e9f1f 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2169,6 +2169,7 @@ static int rbd_dev_v2_object_prefix(struct
> rbd_device *rbd_dev)
>   	dout("%s: rbd_req_sync_exec returned %d\n", __func__, ret);
>   	if (ret < 0)
>   		goto out;
> +	ret = 0;    /* rbd_req_sync_exec() can return positive */
>
>   	p = reply_buf;
>   	rbd_dev->header.object_prefix = ceph_extract_encoded_string(&p,
> @@ -2862,6 +2863,7 @@ static int rbd_dev_image_id(struct rbd_device
> *rbd_dev)
>   	dout("%s: rbd_req_sync_exec returned %d\n", __func__, ret);
>   	if (ret < 0)
>   		goto out;
> +	ret = 0;    /* rbd_req_sync_exec() can return positive */
>
>   	p = response;
>   	rbd_dev->image_id = ceph_extract_encoded_string(&p,
>

Re: [PATCH 2/2] rbd: fix read-only option name

[Josh Durgin]

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

On 10/22/2012 09:44 AM, Alex Elder wrote:
> The name of the "read-only" mapping option was inadvertently changed
> in this commit:
>
>      f84344f3 rbd: separate mapping info in rbd_dev
>
> Revert that hunk to return it to what it should be.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 65e9f1f..d032883 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -397,7 +397,7 @@ enum {
>   static match_table_t rbd_opts_tokens = {
>   	/* int args above */
>   	/* string args above */
> -	{Opt_read_only, "mapping.read_only"},
> +	{Opt_read_only, "read_only"},
>   	{Opt_read_only, "ro"},		/* Alternate spelling */
>   	{Opt_read_write, "read_write"},
>   	{Opt_read_write, "rw"},		/* Alternate spelling */
>