[PATCH 1/1] Make bm_blocks to match previous semantic

[PATCH 1/1] Make bm_blocks to match previous semantic

[jgq516]

From: Guoqing Jiang <gqjiang@suse.com>

The bm_blocks is modified by commit fe60ce (md/bitmap: use
sector_div for sector_t divisions), but it makes bm_blocks
has different value which is changed from like "a/b" to "a%b",
need to correct this to make sure cluster-md still works.

Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
---
This is against for-next branch.

 drivers/md/bitmap.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index 501f83f..ea9c685 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -571,11 +571,10 @@ static int bitmap_read_sb(struct bitmap *bitmap)
 re_read:
 	/* If cluster_slot is set, the cluster is setup */
 	if (bitmap->cluster_slot >= 0) {
-		sector_t bm_blocks;
-		sector_t resync_sectors = bitmap->mddev->resync_max_sectors;
+		sector_t bm_blocks = bitmap->mddev->resync_max_sectors;
 
-		bm_blocks = sector_div(resync_sectors,
-				       bitmap->mddev->bitmap_info.chunksize >> 9);
+		sector_div(bm_blocks,
+				bitmap->mddev->bitmap_info.chunksize >> 9);
 		bm_blocks = bm_blocks << 3;
 		bm_blocks = DIV_ROUND_UP_SECTOR_T(bm_blocks, 4096);
 		bitmap->mddev->bitmap_info.offset += bitmap->cluster_slot * (bm_blocks << 3);
-- 
1.7.12.4

Re: [PATCH 1/1] Make bm_blocks to match previous semantic

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2954 bytes --]

On Tue, 17 Mar 2015 10:40:30 +0800 jgq516@gmail.com wrote:

> From: Guoqing Jiang <gqjiang@suse.com>
> 
> The bm_blocks is modified by commit fe60ce (md/bitmap: use
> sector_div for sector_t divisions), but it makes bm_blocks
> has different value which is changed from like "a/b" to "a%b",
> need to correct this to make sure cluster-md still works.

One of us is confused here.

This code is trying to find the start of the bitmap relevant to this host in
a table of multiple bitmaps.  So it first needs to find out the size of each
bitmap.  It then multiples the size by the index number of this host to get
an offset.

So it take the total number of sectors (resync_max_sectors), divides by the
chunksize (in sectors) to get a number of chunks.  This is the number of bits.

Then it should div-round-up by 8 to get a number of bytes.
Then div-round-up by 4096 to get number of 4-K blocks, because the bitmaps
are always 4K aligned.

Then this number is multiplied by 8 (or shifted by 3) to get a number of
sectors to add to the start of the table.

So the original code in commit b97e92574c0bf335db1cd2ec491d8ff5cd5d0b49
is wrong because it uses sector_div in a way which destroys
resync_max_sectors.
And is wrong because it multiplies by 8 (<<3) instead of divides by 8 to
convert from bits to bytes.

commit f9209a323547f054c7439a3bf67c45e64a054bd
removes the abuse of sector_div, which is good, but uses a simple "a/b"
division, which isn't allowed in the kernel.

commit fe60ce80488a2a481ac175c4ff98f90df22e1e46
then does the right thing with sector_div, but the "<< 3" is still the wrong
way around.

If you still think your code is correct, please explain in detail why.

Goldwyn: if you agree that "<< 3" should be ">> 3" or even
DIV_ROUND_UP_SECTOR_T( , 8);
please send a patch.  If you don't think so, please explain why.

Thanks,
NeilBrown



> 
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
> ---
> This is against for-next branch.
> 
>  drivers/md/bitmap.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> index 501f83f..ea9c685 100644
> --- a/drivers/md/bitmap.c
> +++ b/drivers/md/bitmap.c
> @@ -571,11 +571,10 @@ static int bitmap_read_sb(struct bitmap *bitmap)
>  re_read:
>  	/* If cluster_slot is set, the cluster is setup */
>  	if (bitmap->cluster_slot >= 0) {
> -		sector_t bm_blocks;
> -		sector_t resync_sectors = bitmap->mddev->resync_max_sectors;
> +		sector_t bm_blocks = bitmap->mddev->resync_max_sectors;
>  
> -		bm_blocks = sector_div(resync_sectors,
> -				       bitmap->mddev->bitmap_info.chunksize >> 9);
> +		sector_div(bm_blocks,
> +				bitmap->mddev->bitmap_info.chunksize >> 9);
>  		bm_blocks = bm_blocks << 3;
>  		bm_blocks = DIV_ROUND_UP_SECTOR_T(bm_blocks, 4096);
>  		bitmap->mddev->bitmap_info.offset += bitmap->cluster_slot * (bm_blocks << 3);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 1/1] Make bm_blocks to match previous semantic

[Guoqing Jiang]

Hi Neil,

NeilBrown wrote:
> On Tue, 17 Mar 2015 10:40:30 +0800 jgq516@gmail.com wrote:
>
>   
>> From: Guoqing Jiang <gqjiang@suse.com>
>>
>> The bm_blocks is modified by commit fe60ce (md/bitmap: use
>> sector_div for sector_t divisions), but it makes bm_blocks
>> has different value which is changed from like "a/b" to "a%b",
>> need to correct this to make sure cluster-md still works.
>>     
>
> One of us is confused here.
>
> This code is trying to find the start of the bitmap relevant to this host in
> a table of multiple bitmaps.  So it first needs to find out the size of each
> bitmap.  It then multiples the size by the index number of this host to get
> an offset.
>
>   
Thanks for detailed description, it really helps. I quoted related lines
from bitmap.c.

 574                 sector_t bm_blocks;
 575                 sector_t resync_sectors =
bitmap->mddev->resync_max_sectors; 
 576
 577                 bm_blocks = sector_div(resync_sectors,
 578                                       
bitmap->mddev->bitmap_info.chunksize >> 9);   
 579                 bm_blocks = bm_blocks << 3;
 580                 bm_blocks = DIV_ROUND_UP_SECTOR_T(bm_blocks, 4096);
 581                 bitmap->mddev->bitmap_info.offset +=
bitmap->cluster_slot * (bm_blocks << 3);

> So it take the total number of sectors (resync_max_sectors), divides by the
> chunksize (in sectors) to get a number of chunks.  This is the number of bits.
>
>   
L577 is supposed to do above job.
> Then it should div-round-up by 8 to get a number of bytes.
>   
I guess what you mean is about L579, while it used "<<3" rather than
">>3" now.
> Then div-round-up by 4096 to get number of 4-K blocks, because the bitmaps
> are always 4K aligned.
>   
L580 did the job.
> Then this number is multiplied by 8 (or shifted by 3) to get a number of
> sectors to add to the start of the table.
>   
L581 is for this, right? Is the shifted by 3 is to match the bitmap
format for each
nodes? Seems the relationship between slot and the bitmap region of the node
is like n <-----> [8*nK, 8*(n+1)K]. How about the following changes?

diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index 501f83f..b2a241b 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -571,12 +571,10 @@ static int bitmap_read_sb(struct bitmap *bitmap)
 re_read:
        /* If cluster_slot is set, the cluster is setup */
        if (bitmap->cluster_slot >= 0) {
-               sector_t bm_blocks;
-               sector_t resync_sectors = bitmap->mddev->resync_max_sectors;
+               sector_t bm_blocks = bitmap->mddev->resync_max_sectors;
 
-               bm_blocks = sector_div(resync_sectors,
-                                     
bitmap->mddev->bitmap_info.chunksize >> 9);
-               bm_blocks = bm_blocks << 3;
+               sector_div(bm_blocks,
bitmap->mddev->bitmap_info.chunksize >> 9);
+               bm_blocks = bm_blocks >> 3;
                bm_blocks = DIV_ROUND_UP_SECTOR_T(bm_blocks, 4096);
> So the original code in commit b97e92574c0bf335db1cd2ec491d8ff5cd5d0b49
> is wrong because it uses sector_div in a way which destroys
> resync_max_sectors.
> And is wrong because it multiplies by 8 (<<3) instead of divides by 8 to
> convert from bits to bytes.
>
> commit f9209a323547f054c7439a3bf67c45e64a054bd
> removes the abuse of sector_div, which is good, but uses a simple "a/b"
> division, which isn't allowed in the kernel.
>
> commit fe60ce80488a2a481ac175c4ff98f90df22e1e46
> then does the right thing with sector_div, but the "<< 3" is still the wrong
> way around.
>
> If you still think your code is correct, please explain in detail why.
>
> Goldwyn: if you agree that "<< 3" should be ">> 3" or even
> DIV_ROUND_UP_SECTOR_T( , 8);
> please send a patch.  If you don't think so, please explain why.
>
>   
But anyway, it is better wait for Goldwyn's back from vacation, :)

Thanks,
Guoqing

Re: [PATCH 1/1] Make bm_blocks to match previous semantic

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4639 bytes --]

On Thu, 19 Mar 2015 11:50:05 +0800 Guoqing Jiang <GQJiang@suse.com> wrote:

> Hi Neil,
> 
> NeilBrown wrote:
> > On Tue, 17 Mar 2015 10:40:30 +0800 jgq516@gmail.com wrote:
> >
> >   
> >> From: Guoqing Jiang <gqjiang@suse.com>
> >>
> >> The bm_blocks is modified by commit fe60ce (md/bitmap: use
> >> sector_div for sector_t divisions), but it makes bm_blocks
> >> has different value which is changed from like "a/b" to "a%b",
> >> need to correct this to make sure cluster-md still works.
> >>     
> >
> > One of us is confused here.
> >
> > This code is trying to find the start of the bitmap relevant to this host in
> > a table of multiple bitmaps.  So it first needs to find out the size of each
> > bitmap.  It then multiples the size by the index number of this host to get
> > an offset.
> >
> >   
> Thanks for detailed description, it really helps. I quoted related lines
> from bitmap.c.
> 
>  574                 sector_t bm_blocks;
>  575                 sector_t resync_sectors =
> bitmap->mddev->resync_max_sectors; 
>  576
>  577                 bm_blocks = sector_div(resync_sectors,
>  578                                       
> bitmap->mddev->bitmap_info.chunksize >> 9);   
>  579                 bm_blocks = bm_blocks << 3;
>  580                 bm_blocks = DIV_ROUND_UP_SECTOR_T(bm_blocks, 4096);
>  581                 bitmap->mddev->bitmap_info.offset +=
> bitmap->cluster_slot * (bm_blocks << 3);
> 
> > So it take the total number of sectors (resync_max_sectors), divides by the
> > chunksize (in sectors) to get a number of chunks.  This is the number of bits.
> >
> >   
> L577 is supposed to do above job.
> > Then it should div-round-up by 8 to get a number of bytes.
> >   
> I guess what you mean is about L579, while it used "<<3" rather than
> ">>3" now.
> > Then div-round-up by 4096 to get number of 4-K blocks, because the bitmaps
> > are always 4K aligned.
> >   
> L580 did the job.
> > Then this number is multiplied by 8 (or shifted by 3) to get a number of
> > sectors to add to the start of the table.
> >   
> L581 is for this, right? Is the shifted by 3 is to match the bitmap
> format for each
> nodes? Seems the relationship between slot and the bitmap region of the node
> is like n <-----> [8*nK, 8*(n+1)K]. How about the following changes?
> 
> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> index 501f83f..b2a241b 100644
> --- a/drivers/md/bitmap.c
> +++ b/drivers/md/bitmap.c
> @@ -571,12 +571,10 @@ static int bitmap_read_sb(struct bitmap *bitmap)
>  re_read:
>         /* If cluster_slot is set, the cluster is setup */
>         if (bitmap->cluster_slot >= 0) {
> -               sector_t bm_blocks;
> -               sector_t resync_sectors = bitmap->mddev->resync_max_sectors;
> +               sector_t bm_blocks = bitmap->mddev->resync_max_sectors;
>  
> -               bm_blocks = sector_div(resync_sectors,
> -                                     
> bitmap->mddev->bitmap_info.chunksize >> 9);
> -               bm_blocks = bm_blocks << 3;
> +               sector_div(bm_blocks,
> bitmap->mddev->bitmap_info.chunksize >> 9);

Yes, of course.  sector_div returns the remainder doesn't it!
I was thinking that it returned the quotient and set the first arg to the
remainder - and wonder why you wanted the remainder :-(

I've updated the patch to do the right thing and credited you.
Thanks.

> +               bm_blocks = bm_blocks >> 3;
>                 bm_blocks = DIV_ROUND_UP_SECTOR_T(bm_blocks, 4096);
> > So the original code in commit b97e92574c0bf335db1cd2ec491d8ff5cd5d0b49
> > is wrong because it uses sector_div in a way which destroys
> > resync_max_sectors.
> > And is wrong because it multiplies by 8 (<<3) instead of divides by 8 to
> > convert from bits to bytes.
> >
> > commit f9209a323547f054c7439a3bf67c45e64a054bd
> > removes the abuse of sector_div, which is good, but uses a simple "a/b"
> > division, which isn't allowed in the kernel.
> >
> > commit fe60ce80488a2a481ac175c4ff98f90df22e1e46
> > then does the right thing with sector_div, but the "<< 3" is still the wrong
> > way around.
> >
> > If you still think your code is correct, please explain in detail why.
> >
> > Goldwyn: if you agree that "<< 3" should be ">> 3" or even
> > DIV_ROUND_UP_SECTOR_T( , 8);
> > please send a patch.  If you don't think so, please explain why.
> >
> >   
> But anyway, it is better wait for Goldwyn's back from vacation, :)

I'll leave the other change (<<3 or >>8) until then.

thanks,
NeilBrown


> 
> Thanks,
> Guoqing


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 1/1] Make bm_blocks to match previous semantic

[Goldwyn Rodrigues]

On 03/20/2015 05:37 PM, NeilBrown wrote:
> On Thu, 19 Mar 2015 11:50:05 +0800 Guoqing Jiang <GQJiang@suse.com> wrote:
>
>> Hi Neil,
>>
>> NeilBrown wrote:
>>> On Tue, 17 Mar 2015 10:40:30 +0800 jgq516@gmail.com wrote:
>>>
>>>
>>>> From: Guoqing Jiang <gqjiang@suse.com>
>>>>
>>>> The bm_blocks is modified by commit fe60ce (md/bitmap: use
>>>> sector_div for sector_t divisions), but it makes bm_blocks
>>>> has different value which is changed from like "a/b" to "a%b",
>>>> need to correct this to make sure cluster-md still works.
>>>>
>>>
>>> One of us is confused here.
>>>
>>> This code is trying to find the start of the bitmap relevant to this host in
>>> a table of multiple bitmaps.  So it first needs to find out the size of each
>>> bitmap.  It then multiples the size by the index number of this host to get
>>> an offset.
>>>
>>>
>> Thanks for detailed description, it really helps. I quoted related lines
>> from bitmap.c.
>>
>>   574                 sector_t bm_blocks;
>>   575                 sector_t resync_sectors =
>> bitmap->mddev->resync_max_sectors;
>>   576
>>   577                 bm_blocks = sector_div(resync_sectors,
>>   578
>> bitmap->mddev->bitmap_info.chunksize >> 9);
>>   579                 bm_blocks = bm_blocks << 3;
>>   580                 bm_blocks = DIV_ROUND_UP_SECTOR_T(bm_blocks, 4096);
>>   581                 bitmap->mddev->bitmap_info.offset +=
>> bitmap->cluster_slot * (bm_blocks << 3);
>>
>>> So it take the total number of sectors (resync_max_sectors), divides by the
>>> chunksize (in sectors) to get a number of chunks.  This is the number of bits.
>>>
>>>
>> L577 is supposed to do above job.
>>> Then it should div-round-up by 8 to get a number of bytes.
>>>
>> I guess what you mean is about L579, while it used "<<3" rather than
>> ">>3" now.
>>> Then div-round-up by 4096 to get number of 4-K blocks, because the bitmaps
>>> are always 4K aligned.
>>>
>> L580 did the job.
>>> Then this number is multiplied by 8 (or shifted by 3) to get a number of
>>> sectors to add to the start of the table.
>>>
>> L581 is for this, right? Is the shifted by 3 is to match the bitmap
>> format for each
>> nodes? Seems the relationship between slot and the bitmap region of the node
>> is like n <-----> [8*nK, 8*(n+1)K]. How about the following changes?
>>
>> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
>> index 501f83f..b2a241b 100644
>> --- a/drivers/md/bitmap.c
>> +++ b/drivers/md/bitmap.c
>> @@ -571,12 +571,10 @@ static int bitmap_read_sb(struct bitmap *bitmap)
>>   re_read:
>>          /* If cluster_slot is set, the cluster is setup */
>>          if (bitmap->cluster_slot >= 0) {
>> -               sector_t bm_blocks;
>> -               sector_t resync_sectors = bitmap->mddev->resync_max_sectors;
>> +               sector_t bm_blocks = bitmap->mddev->resync_max_sectors;
>>
>> -               bm_blocks = sector_div(resync_sectors,
>> -
>> bitmap->mddev->bitmap_info.chunksize >> 9);
>> -               bm_blocks = bm_blocks << 3;
>> +               sector_div(bm_blocks,
>> bitmap->mddev->bitmap_info.chunksize >> 9);
>
> Yes, of course.  sector_div returns the remainder doesn't it!
> I was thinking that it returned the quotient and set the first arg to the
> remainder - and wonder why you wanted the remainder :-(
>
> I've updated the patch to do the right thing and credited you.
> Thanks.
>
>> +               bm_blocks = bm_blocks >> 3;
>>                  bm_blocks = DIV_ROUND_UP_SECTOR_T(bm_blocks, 4096);
>>> So the original code in commit b97e92574c0bf335db1cd2ec491d8ff5cd5d0b49
>>> is wrong because it uses sector_div in a way which destroys
>>> resync_max_sectors.
>>> And is wrong because it multiplies by 8 (<<3) instead of divides by 8 to
>>> convert from bits to bytes.
>>>
>>> commit f9209a323547f054c7439a3bf67c45e64a054bd
>>> removes the abuse of sector_div, which is good, but uses a simple "a/b"
>>> division, which isn't allowed in the kernel.
>>>
>>> commit fe60ce80488a2a481ac175c4ff98f90df22e1e46
>>> then does the right thing with sector_div, but the "<< 3" is still the wrong
>>> way around.
>>>
>>> If you still think your code is correct, please explain in detail why.
>>>
>>> Goldwyn: if you agree that "<< 3" should be ">> 3" or even
>>> DIV_ROUND_UP_SECTOR_T( , 8);
>>> please send a patch.  If you don't think so, please explain why.
>>>
>>>
>> But anyway, it is better wait for Goldwyn's back from vacation, :)
>
> I'll leave the other change (<<3 or >>8) until then.


Yes, you are right. It is working for small bitmaps which were covered 
in 1 4k blocks, but was breaking for ones where the bitmap is larger.

It should be (bm_blocks+7) >> 3 or DV_ROUND_UP_SECTOR_T( ,8). Also, it 
should account for bitmap_super_t size as well. I will send a patch 
shortly to mimic how the userspace tools calculate it.



-- 
Goldwyn