On Thu, 19 Mar 2015 11:50:05 +0800 Guoqing Jiang <GQJiang@xxxxxxxx> wrote:
> 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);
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:
pgpvpzGHkGRzh.pgp
Description: OpenPGP digital signature
