Hi Neil,
NeilBrown wrote:
On Tue, 17 Mar 2015 10:40:30 +0800 jgq516@xxxxxxxxx wrote:
From: Guoqing Jiang <gqjiang@xxxxxxxx>
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, :)
