[PATCH] rbd: fix a bug in resizing a mapping

[PATCH] rbd: fix a bug in resizing a mapping

[Alex Elder]

When a snapshot context update occurs, rbd_update_mapping_size() is
called to set the capacity of the disk to record the updated
size of the image in case it has changed.

There's a bug though.  The mapping size is in units of *bytes*.  The
code that updates the mapping size field is assigning a value that
has been scaled down to *sectors*.

Fix that.  Also, check to see if the size has actually changed, and
don't bother updating things (specifically, calling set_capacity())
if it has not.

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

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 5918e0b..37d9349 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3034,15 +3034,17 @@ static void rbd_remove_all_snaps(struct
rbd_device *rbd_dev)

 static void rbd_update_mapping_size(struct rbd_device *rbd_dev)
 {
-	sector_t size;
-
 	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
 		return;

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

 /*
-- 
1.7.9.5

Re: [PATCH] rbd: fix a bug in resizing a mapping

[Alex Elder]

On 04/27/2013 07:18 AM, Alex Elder wrote:
> When a snapshot context update occurs, rbd_update_mapping_size() is
> called to set the capacity of the disk to record the updated
> size of the image in case it has changed.

This patch, as well as the series of 4 and the series of 6 that
I posted after it, are avaialble in the "review/wip-rbd-cleanup-3"
branch of the ceph-client git respository.

					-Alex


> There's a bug though.  The mapping size is in units of *bytes*.  The
> code that updates the mapping size field is assigning a value that
> has been scaled down to *sectors*.
> 
> Fix that.  Also, check to see if the size has actually changed, and
> don't bother updating things (specifically, calling set_capacity())
> if it has not.
> 
> This resolves:
>     http://tracker.ceph.com/issues/4833
> 
> 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 5918e0b..37d9349 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -3034,15 +3034,17 @@ static void rbd_remove_all_snaps(struct
> rbd_device *rbd_dev)
> 
>  static void rbd_update_mapping_size(struct rbd_device *rbd_dev)
>  {
> -	sector_t size;
> -
>  	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
>  		return;
> 
> -	size = (sector_t) rbd_dev->header.image_size / SECTOR_SIZE;
> -	dout("setting size to %llu sectors", (unsigned long long) size);
> -	rbd_dev->mapping.size = (u64) size;
> -	set_capacity(rbd_dev->disk, size);
> +	if (rbd_dev->mapping.size != rbd_dev->header.image_size) {
> +		sector_t 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);
> +	}
>  }
> 
>  /*
> 

Re: [PATCH] rbd: fix a bug in resizing a mapping

[Alex Elder]

On 04/27/2013 07:39 AM, Alex Elder wrote:
> On 04/27/2013 07:18 AM, Alex Elder wrote:
>> When a snapshot context update occurs, rbd_update_mapping_size() is
>> called to set the capacity of the disk to record the updated
>> size of the image in case it has changed.

I have updated a few patches in this series, based on a problem
or two I saw while working with some follow-on patches.  Since
they have not yet been reviewed, I'm no going to bother describing
what has changed, I've just updated the "review/wip-rbd-cleanup-3"
branch to contain the new code.

					-Alex

> This patch, as well as the series of 4 and the series of 6 that
> I posted after it, are avaialble in the "review/wip-rbd-cleanup-3"
> branch of the ceph-client git respository.
> 
> 					-Alex
> 
> 
>> There's a bug though.  The mapping size is in units of *bytes*.  The
>> code that updates the mapping size field is assigning a value that
>> has been scaled down to *sectors*.
>>
>> Fix that.  Also, check to see if the size has actually changed, and
>> don't bother updating things (specifically, calling set_capacity())
>> if it has not.
>>
>> This resolves:
>>     http://tracker.ceph.com/issues/4833
>>
>> 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 5918e0b..37d9349 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -3034,15 +3034,17 @@ static void rbd_remove_all_snaps(struct
>> rbd_device *rbd_dev)
>>
>>  static void rbd_update_mapping_size(struct rbd_device *rbd_dev)
>>  {
>> -	sector_t size;
>> -
>>  	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
>>  		return;
>>
>> -	size = (sector_t) rbd_dev->header.image_size / SECTOR_SIZE;
>> -	dout("setting size to %llu sectors", (unsigned long long) size);
>> -	rbd_dev->mapping.size = (u64) size;
>> -	set_capacity(rbd_dev->disk, size);
>> +	if (rbd_dev->mapping.size != rbd_dev->header.image_size) {
>> +		sector_t 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);
>> +	}
>>  }
>>
>>  /*
>>
> 

Re: [PATCH] rbd: fix a bug in resizing a mapping

[Josh Durgin]

On 04/29/2013 10:51 AM, Alex Elder wrote:
> On 04/27/2013 07:39 AM, Alex Elder wrote:
>> On 04/27/2013 07:18 AM, Alex Elder wrote:
>>> When a snapshot context update occurs, rbd_update_mapping_size() is
>>> called to set the capacity of the disk to record the updated
>>> size of the image in case it has changed.

This patch looks good.
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

>>> There's a bug though.  The mapping size is in units of *bytes*.  The
>>> code that updates the mapping size field is assigning a value that
>>> has been scaled down to *sectors*.
>>>
>>> Fix that.  Also, check to see if the size has actually changed, and
>>> don't bother updating things (specifically, calling set_capacity())
>>> if it has not.
>>>
>>> This resolves:
>>>      http://tracker.ceph.com/issues/4833
>>>
>>> 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 5918e0b..37d9349 100644
>>> --- a/drivers/block/rbd.c
>>> +++ b/drivers/block/rbd.c
>>> @@ -3034,15 +3034,17 @@ static void rbd_remove_all_snaps(struct
>>> rbd_device *rbd_dev)
>>>
>>>   static void rbd_update_mapping_size(struct rbd_device *rbd_dev)
>>>   {
>>> -	sector_t size;
>>> -
>>>   	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
>>>   		return;
>>>
>>> -	size = (sector_t) rbd_dev->header.image_size / SECTOR_SIZE;
>>> -	dout("setting size to %llu sectors", (unsigned long long) size);
>>> -	rbd_dev->mapping.size = (u64) size;
>>> -	set_capacity(rbd_dev->disk, size);
>>> +	if (rbd_dev->mapping.size != rbd_dev->header.image_size) {
>>> +		sector_t 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);
>>> +	}
>>>   }
>>>
>>>   /*
>>>
>>
>