[PATCH 0/2] Can't grow size twice for a super1.0 array

[PATCH 0/2] Can't grow size twice for a super1.0 array

[Xiao Ni]

Hi all

This are the test steps:
[root@storageqe-54 ~]# mdadm -CR /dev/md0 -l1 -n2 /dev/loop0 /dev/loop1 -e 1.0 --size=1G
[root@storageqe-54 ~]# mdadm --wait /dev/md0
[root@storageqe-54 ~]# mdadm -G /dev/md0 --size=5G
mdadm: component size of /dev/md0 has been set to 5242880K
[root@storageqe-54 ~]# mdadm --wait /dev/md0
[root@storageqe-54 ~]# mdadm -G /dev/md0 --size=6G
mdadm: Cannot set device size for /dev/md0: No space left on device

loop0                                    7:0    0   7.8G  0 loop  
└─md0                                    9:0    0     5G  0 raid1 
loop1                                    7:1    0   7.8G  0 loop  
└─md0                                    9:0    0     5G  0 raid1 

The reason is that it calcluates the max usable space in a wrong way for super1.0 array.
It uses rdev->sectors to calcuate the max usable space rather than the whole disk size.
At first it grow size and it's successful. Because rdev->sectors is set to the whole disk
size when creating raid device. super_1_load sets rdev->sectors to sb->data_size. In mdadm
it sets sb->data_size to the whole disk size rathan than the raid device size. I think it's
a wrong place too. Because of this error, it can grow size successfully at the first time.

It re-sets rdev->sectors to real raid device after reshaping to the new size. So it can't
grow size anymore.

Xiao Ni (2):
  super1.0 calculates max sectors in a wrong way
  Don't need to reset superblock start address for super1.0

 drivers/md/md.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

-- 
2.7.5

[PATCH 1/2] super1.0 calculates max sectors in a wrong way

[Xiao Ni]

One super1.0 raid array wants to grow size, it needs to check the device max usable
size. If the requested size is bigger than max usable size, it will return an error.

Now it uses rdev->sectors for max usable size. If one disk is 500G and the raid device
only uses the 100GB of this disk. rdev->sectors can't tell the real max usable size.
The max usable size should be dev_size-(superblock_size+bitmap_size+badblock_size).

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 drivers/md/md.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index f567f53..d2c5984 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2183,10 +2183,30 @@ super_1_rdev_size_change(struct md_rdev *rdev, sector_t num_sectors)
 		return 0;
 	} else {
 		/* minor version 0; superblock after data */
-		sector_t sb_start;
-		sb_start = (i_size_read(rdev->bdev->bd_inode) >> 9) - 8*2;
+		sector_t sb_start, bm_space;
+		sector_t dev_size = i_size_read(rdev->bdev->bd_inode) >> 9;
+
+		/* 8K is for superblock */
+		sb_start = dev_size - 8*2;
 		sb_start &= ~(sector_t)(4*2 - 1);
-		max_sectors = rdev->sectors + sb_start - rdev->sb_start;
+
+		/* if the device is bigger than 8Gig, save 64k for bitmap usage,
+		 * if bigger than 200Gig, save 128k
+		 */
+		if (dev_size < 64*2)
+			bm_space = 0;
+		else if (dev_size - 64*2 >= 200*1024*1024*2)
+			bm_space = 128*2;
+		else if (dev_size - 4*2 > 8*1024*1024*2)
+			bm_space = 64*2;
+		else
+			bm_space = 4*2;
+
+		/* Space that can be used to store date needs to decrease superblock
+		 * bitmap space and bad block space(4K)
+		 */
+		max_sectors = sb_start - bm_space - 4*2;
+
 		if (!num_sectors || num_sectors > max_sectors)
 			num_sectors = max_sectors;
 		rdev->sb_start = sb_start;
-- 
2.7.5

[PATCH 2/2] Don't need to reset superblock start address for super1.0

[Xiao Ni]

The superblock start address doesn't change after creating raid device. It always at the end
of the device(end_pos-8*2).

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 drivers/md/md.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index d2c5984..9de9c0e 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2209,7 +2209,6 @@ super_1_rdev_size_change(struct md_rdev *rdev, sector_t num_sectors)
 
 		if (!num_sectors || num_sectors > max_sectors)
 			num_sectors = max_sectors;
-		rdev->sb_start = sb_start;
 	}
 	sb = page_address(rdev->sb_page);
 	sb->data_size = cpu_to_le64(num_sectors);
-- 
2.7.5

Re: [PATCH 1/2] super1.0 calculates max sectors in a wrong way

[Song Liu]

On Tue, Jun 30, 2020 at 12:55 AM Xiao Ni <xni@redhat.com> wrote:
>
> One super1.0 raid array wants to grow size, it needs to check the device max usable
> size. If the requested size is bigger than max usable size, it will return an error.
>
> Now it uses rdev->sectors for max usable size. If one disk is 500G and the raid device
> only uses the 100GB of this disk. rdev->sectors can't tell the real max usable size.
> The max usable size should be dev_size-(superblock_size+bitmap_size+badblock_size).
>
> Signed-off-by: Xiao Ni <xni@redhat.com>

Thanks for the fix!

> ---
>  drivers/md/md.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index f567f53..d2c5984 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -2183,10 +2183,30 @@ super_1_rdev_size_change(struct md_rdev *rdev, sector_t num_sectors)
>                 return 0;
>         } else {
>                 /* minor version 0; superblock after data */
> -               sector_t sb_start;
> -               sb_start = (i_size_read(rdev->bdev->bd_inode) >> 9) - 8*2;
> +               sector_t sb_start, bm_space;
> +               sector_t dev_size = i_size_read(rdev->bdev->bd_inode) >> 9;
> +
> +               /* 8K is for superblock */
> +               sb_start = dev_size - 8*2;
>                 sb_start &= ~(sector_t)(4*2 - 1);
> -               max_sectors = rdev->sectors + sb_start - rdev->sb_start;
> +
> +               /* if the device is bigger than 8Gig, save 64k for bitmap usage,
> +                * if bigger than 200Gig, save 128k
> +                */
> +               if (dev_size < 64*2)
> +                       bm_space = 0;
> +               else if (dev_size - 64*2 >= 200*1024*1024*2)
> +                       bm_space = 128*2;
> +               else if (dev_size - 4*2 > 8*1024*1024*2)
> +                       bm_space = 64*2;
> +               else
> +                       bm_space = 4*2;

Could you explain the handling of bitmap space?

Thanks,
Song

Re: [PATCH 1/2] super1.0 calculates max sectors in a wrong way

[Xiao Ni]

On 07/28/2020 01:33 AM, Song Liu wrote:
> On Tue, Jun 30, 2020 at 12:55 AM Xiao Ni <xni@redhat.com> wrote:
>> One super1.0 raid array wants to grow size, it needs to check the device max usable
>> size. If the requested size is bigger than max usable size, it will return an error.
>>
>> Now it uses rdev->sectors for max usable size. If one disk is 500G and the raid device
>> only uses the 100GB of this disk. rdev->sectors can't tell the real max usable size.
>> The max usable size should be dev_size-(superblock_size+bitmap_size+badblock_size).
>>
>> Signed-off-by: Xiao Ni <xni@redhat.com>
> Thanks for the fix!
>
>> ---
>>   drivers/md/md.c | 26 +++++++++++++++++++++++---
>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index f567f53..d2c5984 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -2183,10 +2183,30 @@ super_1_rdev_size_change(struct md_rdev *rdev, sector_t num_sectors)
>>                  return 0;
>>          } else {
>>                  /* minor version 0; superblock after data */
>> -               sector_t sb_start;
>> -               sb_start = (i_size_read(rdev->bdev->bd_inode) >> 9) - 8*2;
>> +               sector_t sb_start, bm_space;
>> +               sector_t dev_size = i_size_read(rdev->bdev->bd_inode) >> 9;
>> +
>> +               /* 8K is for superblock */
>> +               sb_start = dev_size - 8*2;
>>                  sb_start &= ~(sector_t)(4*2 - 1);
>> -               max_sectors = rdev->sectors + sb_start - rdev->sb_start;
>> +
>> +               /* if the device is bigger than 8Gig, save 64k for bitmap usage,
>> +                * if bigger than 200Gig, save 128k
>> +                */
>> +               if (dev_size < 64*2)
>> +                       bm_space = 0;
>> +               else if (dev_size - 64*2 >= 200*1024*1024*2)
>> +                       bm_space = 128*2;
>> +               else if (dev_size - 4*2 > 8*1024*1024*2)
>> +                       bm_space = 64*2;
>> +               else
>> +                       bm_space = 4*2;
> Could you explain the handling of bitmap space?
Hi Song

This calculation of bitmap is decided by mdadm. In mdadm super1.c 
choose_bm_space function,
it uses this way to calculate bitmap space. Do I need to add comments 
here to describe this?
Something like "mdadm function choose_bm_space decides the bitmap space"?

Regards
Xiao

Re: [PATCH 1/2] super1.0 calculates max sectors in a wrong way

[Song Liu]

On Mon, Jul 27, 2020 at 6:18 PM Xiao Ni <xni@redhat.com> wrote:
>
>
>
> On 07/28/2020 01:33 AM, Song Liu wrote:
> > On Tue, Jun 30, 2020 at 12:55 AM Xiao Ni <xni@redhat.com> wrote:
> >> One super1.0 raid array wants to grow size, it needs to check the device max usable
> >> size. If the requested size is bigger than max usable size, it will return an error.
> >>
> >> Now it uses rdev->sectors for max usable size. If one disk is 500G and the raid device
> >> only uses the 100GB of this disk. rdev->sectors can't tell the real max usable size.
> >> The max usable size should be dev_size-(superblock_size+bitmap_size+badblock_size).
> >>
> >> Signed-off-by: Xiao Ni <xni@redhat.com>
> > Thanks for the fix!
> >
> >> ---
> >>   drivers/md/md.c | 26 +++++++++++++++++++++++---
> >>   1 file changed, 23 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >> index f567f53..d2c5984 100644
> >> --- a/drivers/md/md.c
> >> +++ b/drivers/md/md.c
> >> @@ -2183,10 +2183,30 @@ super_1_rdev_size_change(struct md_rdev *rdev, sector_t num_sectors)
> >>                  return 0;
> >>          } else {
> >>                  /* minor version 0; superblock after data */
> >> -               sector_t sb_start;
> >> -               sb_start = (i_size_read(rdev->bdev->bd_inode) >> 9) - 8*2;
> >> +               sector_t sb_start, bm_space;
> >> +               sector_t dev_size = i_size_read(rdev->bdev->bd_inode) >> 9;
> >> +
> >> +               /* 8K is for superblock */
> >> +               sb_start = dev_size - 8*2;
> >>                  sb_start &= ~(sector_t)(4*2 - 1);
> >> -               max_sectors = rdev->sectors + sb_start - rdev->sb_start;
> >> +
> >> +               /* if the device is bigger than 8Gig, save 64k for bitmap usage,
> >> +                * if bigger than 200Gig, save 128k
> >> +                */
> >> +               if (dev_size < 64*2)
> >> +                       bm_space = 0;
> >> +               else if (dev_size - 64*2 >= 200*1024*1024*2)
> >> +                       bm_space = 128*2;
> >> +               else if (dev_size - 4*2 > 8*1024*1024*2)
> >> +                       bm_space = 64*2;
> >> +               else
> >> +                       bm_space = 4*2;
> > Could you explain the handling of bitmap space?
> Hi Song
>
> This calculation of bitmap is decided by mdadm. In mdadm super1.c
> choose_bm_space function,
> it uses this way to calculate bitmap space. Do I need to add comments
> here to describe this?
> Something like "mdadm function choose_bm_space decides the bitmap space"?

Thanks for the explanation.

I merged the two patches, made some changes, and applied it to md-next. Please
let me know whether it looks good.

Song

Re: [PATCH 1/2] super1.0 calculates max sectors in a wrong way

[Xiao Ni]

On 07/29/2020 08:43 AM, Song Liu wrote:
>> Hi Song
>>
>> This calculation of bitmap is decided by mdadm. In mdadm super1.c
>> choose_bm_space function,
>> it uses this way to calculate bitmap space. Do I need to add comments
>> here to describe this?
>> Something like "mdadm function choose_bm_space decides the bitmap space"?
> Thanks for the explanation.
>
> I merged the two patches, made some changes, and applied it to md-next. Please
> let me know whether it looks good.
>
The function super_10_choose_bm_space can make people confused. All 
types of super1 calculate
bitmap space in the same way (super1.0, 1.1 and 1.2). Could you change 
the function name to
super_1_choose_bm_space?

Thanks
Xiao

Re: [PATCH 1/2] super1.0 calculates max sectors in a wrong way

[Song Liu]

On Tue, Jul 28, 2020 at 6:15 PM Xiao Ni <xni@redhat.com> wrote:
>
>
>
> On 07/29/2020 08:43 AM, Song Liu wrote:
> >> Hi Song
> >>
> >> This calculation of bitmap is decided by mdadm. In mdadm super1.c
> >> choose_bm_space function,
> >> it uses this way to calculate bitmap space. Do I need to add comments
> >> here to describe this?
> >> Something like "mdadm function choose_bm_space decides the bitmap space"?
> > Thanks for the explanation.
> >
> > I merged the two patches, made some changes, and applied it to md-next. Please
> > let me know whether it looks good.
> >
> The function super_10_choose_bm_space can make people confused. All
> types of super1 calculate
> bitmap space in the same way (super1.0, 1.1 and 1.2). Could you change
> the function name to
> super_1_choose_bm_space?

Updated. Thanks!

Song