[PATCH] md/bitmap: preserve whole sb_page when initializing bitmap

[PATCH] md/bitmap: preserve whole sb_page when initializing bitmap

[Song Liu]

When bitmap_resize() is used to initialize the bitmap, it is
necessary to preserve the whole page of sb_page instead of just
the header (bitmap_super_t). This is because the sb_page may
contain bitmap read from the disks (initialized by mdadm).

Note that, this code path is only triggered with certain
combinations of parameters. One example is when raid456 array
is created with write journal

Signed-off-by: Song Liu <songliubraving@fb.com>
---
 drivers/md/bitmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index 40f3cd7..93dd809 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -2118,7 +2118,7 @@ int bitmap_resize(struct bitmap *bitmap, sector_t blocks,
 	if (store.sb_page && bitmap->storage.sb_page)
 		memcpy(page_address(store.sb_page),
 		       page_address(bitmap->storage.sb_page),
-		       sizeof(bitmap_super_t));
+		       init ? PAGE_SIZE : sizeof(bitmap_super_t));
 	bitmap_file_unmap(&bitmap->storage);
 	bitmap->storage = store;
 
-- 
2.9.3

Re: [PATCH] md/bitmap: preserve whole sb_page when initializing bitmap

[Shaohua Li]

On Tue, Aug 01, 2017 at 11:11:37PM -0700, Song Liu wrote:
> When bitmap_resize() is used to initialize the bitmap, it is
> necessary to preserve the whole page of sb_page instead of just
> the header (bitmap_super_t). This is because the sb_page may
> contain bitmap read from the disks (initialized by mdadm).
> 
> Note that, this code path is only triggered with certain
> combinations of parameters. One example is when raid456 array
> is created with write journal

Good catch, this issue probably only exists with journal, because we write
superblock there.
 
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>  drivers/md/bitmap.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> index 40f3cd7..93dd809 100644
> --- a/drivers/md/bitmap.c
> +++ b/drivers/md/bitmap.c
> @@ -2118,7 +2118,7 @@ int bitmap_resize(struct bitmap *bitmap, sector_t blocks,
>  	if (store.sb_page && bitmap->storage.sb_page)
>  		memcpy(page_address(store.sb_page),
>  		       page_address(bitmap->storage.sb_page),
> -		       sizeof(bitmap_super_t));
> +		       init ? PAGE_SIZE : sizeof(bitmap_super_t));

I think this should be 'roundup(sizeof(bitmap_super_t),
bdev_logical_block_size(rdev->bdev))', we not always read one page.

The init check is unnecessary too, because if it isn't init, we will initialize
the bitmaps later in bitmap_resize, so the copy doesn't matter.

Thanks,
Shaohua


>  	bitmap_file_unmap(&bitmap->storage);
>  	bitmap->storage = store;
>  
> -- 
> 2.9.3
> 

Re: [PATCH] md/bitmap: preserve whole sb_page when initializing bitmap

[Song Liu]

>> On 8/8/17, 8:30 AM, "Shaohua Li" <shli@kernel.org> wrote:

    On Tue, Aug 01, 2017 at 11:11:37PM -0700, Song Liu wrote:
    > When bitmap_resize() is used to initialize the bitmap, it is
    > necessary to preserve the whole page of sb_page instead of just
    > the header (bitmap_super_t). This is because the sb_page may
    > contain bitmap read from the disks (initialized by mdadm).
    > 
    > Note that, this code path is only triggered with certain
    > combinations of parameters. One example is when raid456 array
    > is created with write journal
    
    Good catch, this issue probably only exists with journal, because we write
    superblock there.
     
    > Signed-off-by: Song Liu <songliubraving@fb.com>
    > ---
    >  drivers/md/bitmap.c | 3 +--
    >  1 file changed, 1 insertion(+), 2 deletions(-)
    > 
    > diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
    > index 40f3cd7..93dd809 100644
    > --- a/drivers/md/bitmap.c
    > +++ b/drivers/md/bitmap.c
    > @@ -2118,7 +2118,7 @@ int bitmap_resize(struct bitmap *bitmap, sector_t blocks,
    >  	if (store.sb_page && bitmap->storage.sb_page)
    >  		memcpy(page_address(store.sb_page),
    >  		       page_address(bitmap->storage.sb_page),
    > -		       sizeof(bitmap_super_t));
    > +		       init ? PAGE_SIZE : sizeof(bitmap_super_t));
    
    I think this should be 'roundup(sizeof(bitmap_super_t),
    bdev_logical_block_size(rdev->bdev))', we not always read one page.
    
The sb_page is read from from one device, but updated in all devices. So when 
different devices have different block sizes, we may still write wrong bitmap
to the devices. How about we make read_sb_page() and write_sb_page() both access
full page instead?

Thanks,
Song