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

[NeilBrown <neilb@suse.de>]

[-- 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 --]