[mdadm PATCH 0/3] fixed broken level conversion and one strncmp issue

[mdadm PATCH 0/3] fixed broken level conversion and one strncmp issue

[Zhilong Liu]

Hi, Jes;
  The patches mainly fixed the broken raid level conversion against
the commit: 4b74a905a67e
(mdadm/grow: Component size must be larger than chunk size) and one
strncmp issue in mdstat.
  Any suggestions is very welcome.

Thanks very much,
Zhilong

Zhilong Liu (3):
  mdadm/Grow: fix the broken raid level conversion
  mdadm/mdstat: fixup a number of '==' broken formatting
  mdadm/mdstat: correct the strncmp number 4 as 6

 Grow.c   |  3 ++-
 mdstat.c | 39 ++++++++++++++++++++-------------------
 2 files changed, 22 insertions(+), 20 deletions(-)

-- 
2.6.6

[PATCH 1/3] mdadm/Grow: fix the broken raid level conversion

[Zhilong Liu]

To fix the commit: 4b74a905a67e
(mdadm/grow: Component size must be larger than chunk size)
Since cannot change component size at the same time as other
changes, ensure the 'level' is UnSet when changing component
size, and also not affect the raid level conversion.

Signed-off-by: Zhilong Liu <zlliu@suse.com>
---
 Grow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Grow.c b/Grow.c
index 1149753..180fd78 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1814,7 +1814,8 @@ int Grow_reshape(char *devname, int fd,
 	}
 
 	if (array.level > 1 &&
-	   (array.chunk_size / 1024) > (int)s->size) {
+	   (array.chunk_size / 1024) > (int)s->size &&
+	    s->level == UnSet) {
 		pr_err("component size must be larger than chunk size.\n");
 		return 1;
 	}
-- 
2.6.6

[PATCH 2/3] mdadm/mdstat: fixup a number of '==' broken formatting

[Zhilong Liu]

This commit doesn't change any codes, just tidy up the
code formatting.

Signed-off-by: Zhilong Liu <zlliu@suse.com>
---
 mdstat.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/mdstat.c b/mdstat.c
index 0d44050..6c906a7 100644
--- a/mdstat.c
+++ b/mdstat.c
@@ -158,11 +158,11 @@ struct mdstat_ent *mdstat_read(int hold, int start)
 		char devnm[32];
 		int in_devs = 0;
 
-		if (strcmp(line, "Personalities")==0)
+		if (strcmp(line, "Personalities") == 0)
 			continue;
-		if (strcmp(line, "read_ahead")==0)
+		if (strcmp(line, "read_ahead") == 0)
 			continue;
-		if (strcmp(line, "unused")==0)
+		if (strcmp(line, "unused") == 0)
 			continue;
 		insert_here = NULL;
 		/* Better be an md line.. */
@@ -187,9 +187,9 @@ struct mdstat_ent *mdstat_read(int hold, int start)
 		for (w=dl_next(line); w!= line ; w=dl_next(w)) {
 			int l = strlen(w);
 			char *eq;
-			if (strcmp(w, "active")==0)
+			if (strcmp(w, "active") == 0)
 				ent->active = 1;
-			else if (strcmp(w, "inactive")==0) {
+			else if (strcmp(w, "inactive") == 0) {
 				ent->active = 0;
 				in_devs = 1;
 			} else if (ent->active > 0 &&
@@ -197,13 +197,13 @@ struct mdstat_ent *mdstat_read(int hold, int start)
 				 w[0] != '(' /*readonly*/) {
 				ent->level = xstrdup(w);
 				in_devs = 1;
-			} else if (in_devs && strcmp(w, "blocks")==0)
+			} else if (in_devs && strcmp(w, "blocks") == 0)
 				in_devs = 0;
 			else if (in_devs) {
 				char *ep = strchr(w, '[');
 				ent->devcnt +=
 					add_member_devname(&ent->members, w);
-				if (ep && strncmp(w, "md", 2)==0) {
+				if (ep && strncmp(w, "md", 2) == 0) {
 					/* This has an md device as a component.
 					 * If that device is already in the
 					 * list, make sure we insert before
@@ -226,31 +226,31 @@ struct mdstat_ent *mdstat_read(int hold, int start)
 			} else if (w[0] == '[' && isdigit(w[1])) {
 				ent->raid_disks = atoi(w+1);
 			} else if (!ent->pattern &&
-				 w[0] == '[' &&
-				 (w[1] == 'U' || w[1] == '_')) {
+				   w[0] == '[' &&
+				   (w[1] == 'U' || w[1] == '_')) {
 				ent->pattern = xstrdup(w+1);
-				if (ent->pattern[l-2]==']')
+				if (ent->pattern[l-2] == ']')
 					ent->pattern[l-2] = '\0';
 			} else if (ent->percent == RESYNC_NONE &&
-				   strncmp(w, "re", 2)== 0 &&
+				   strncmp(w, "re", 2) == 0 &&
 				   w[l-1] == '%' &&
-				   (eq=strchr(w, '=')) != NULL ) {
+				   (eq = strchr(w, '=')) != NULL ) {
 				ent->percent = atoi(eq+1);
-				if (strncmp(w,"resync", 6)==0)
+				if (strncmp(w,"resync", 6) == 0)
 					ent->resync = 1;
-				else if (strncmp(w, "reshape", 7)==0)
+				else if (strncmp(w, "reshape", 7) == 0)
 					ent->resync = 2;
 				else
 					ent->resync = 0;
 			} else if (ent->percent == RESYNC_NONE &&
 				   (w[0] == 'r' || w[0] == 'c')) {
-				if (strncmp(w, "resync", 4)==0)
+				if (strncmp(w, "resync", 4) == 0)
 					ent->resync = 1;
-				if (strncmp(w, "reshape", 7)==0)
+				if (strncmp(w, "reshape", 7) == 0)
 					ent->resync = 2;
-				if (strncmp(w, "recovery", 8)==0)
+				if (strncmp(w, "recovery", 8) == 0)
 					ent->resync = 0;
-				if (strncmp(w, "check", 5)==0)
+				if (strncmp(w, "check", 5) == 0)
 					ent->resync = 3;
 
 				if (l > 8 && strcmp(w+l-8, "=DELAYED") == 0)
@@ -289,7 +289,8 @@ struct mdstat_ent *mdstat_read(int hold, int start)
 			e->next = rv;
 			rv = e;
 		}
-	} else rv = all;
+	} else
+		rv = all;
 	return rv;
 }
 
-- 
2.6.6

[PATCH 3/3] mdadm/mdstat: correct the strncmp number 4 as 6

[Zhilong Liu]

mdstat: it should be corrected as 6 when strncmp 'resync'.

Signed-off-by: Zhilong Liu <zlliu@suse.com>
---
 mdstat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mdstat.c b/mdstat.c
index 6c906a7..7e600d0 100644
--- a/mdstat.c
+++ b/mdstat.c
@@ -244,7 +244,7 @@ struct mdstat_ent *mdstat_read(int hold, int start)
 					ent->resync = 0;
 			} else if (ent->percent == RESYNC_NONE &&
 				   (w[0] == 'r' || w[0] == 'c')) {
-				if (strncmp(w, "resync", 4) == 0)
+				if (strncmp(w, "resync", 6) == 0)
 					ent->resync = 1;
 				if (strncmp(w, "reshape", 7) == 0)
 					ent->resync = 2;
-- 
2.6.6

Re: [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

On Mon, Oct 09 2017, Zhilong Liu wrote:

> To fix the commit: 4b74a905a67e
> (mdadm/grow: Component size must be larger than chunk size)
> Since cannot change component size at the same time as other
> changes, ensure the 'level' is UnSet when changing component
> size, and also not affect the raid level conversion.
>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> ---
>  Grow.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Grow.c b/Grow.c
> index 1149753..180fd78 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1814,7 +1814,8 @@ int Grow_reshape(char *devname, int fd,
>  	}
>  
>  	if (array.level > 1 &&
> -	   (array.chunk_size / 1024) > (int)s->size) {
> +	   (array.chunk_size / 1024) > (int)s->size &&
> +	    s->level == UnSet) {
>  		pr_err("component size must be larger than chunk size.\n");
>  		return 1;
>  	}

This patch doesn't make any sense to me.
If the chunk_size is meaningful, then is must be less than the new
s->size.
This is true whether the level is being changed or not.

Why do you think that the component size cannot be changed at the same
time as other changes?

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion

[Zhilong Liu]

On 10/09/2017 06:49 PM, NeilBrown wrote:
> On Mon, Oct 09 2017, Zhilong Liu wrote:
>
>> To fix the commit: 4b74a905a67e
>> (mdadm/grow: Component size must be larger than chunk size)
>> Since cannot change component size at the same time as other
>> changes, ensure the 'level' is UnSet when changing component
>> size, and also not affect the raid level conversion.
>>
>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>> ---
>>   Grow.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Grow.c b/Grow.c
>> index 1149753..180fd78 100644
>> --- a/Grow.c
>> +++ b/Grow.c
>> @@ -1814,7 +1814,8 @@ int Grow_reshape(char *devname, int fd,
>>   	}
>>   
>>   	if (array.level > 1 &&
>> -	   (array.chunk_size / 1024) > (int)s->size) {
>> +	   (array.chunk_size / 1024) > (int)s->size &&
>> +	    s->level == UnSet) {
>>   		pr_err("component size must be larger than chunk size.\n");
>>   		return 1;
>>   	}
> This patch doesn't make any sense to me.

Hi, Neil;
   Sorry for the later reply due to meetings.

I do agree your suggestion that adding test "s->size > 0" is more 
comfortable than
adding "s->level == UnSet".

This patch mainly wanna ensure that changing new component size must be 
">=" current
chunk size, otherwise the mddev->pers->resize would set the 
mddev->dev_sectors as '0'.

array.level > 1       ---> only against the raids which the chunk size 
is meaningful.
(array.chunk_size / 1024) > (int)s->size     ----> ensure the 
explanation above.
s->size > 0    ----> to ensure that valid re-size has required.

> If the chunk_size is meaningful, then is must be less than the new
> s->size.

Yes here.
> This is true whether the level is being changed or not.
>
> Why do you think that the component size cannot be changed at the same
> time as other changes?

Both user space and kernel space have codes to verify only one change 
happens.

in mdadm/Grow.c:   Grow_reshape()
...
1801         if (s->size > 0 &&
1802             (s->chunk || s->level!= UnSet || s->layout_str || 
s->raiddisks)) {
1803                 pr_err("cannot change component size at the same 
time as other changes.\n"
1804                         "   Change size first, then check data is 
intact before making other changes.\n");
1805                 return 1;
1806         }
...

in md.c:  update_array_info()
...
6833         /* Check there is only one change */
6834         if (info->size >= 0 && mddev->dev_sectors / 2 != info->size)
6835                 cnt++;
6836         if (mddev->raid_disks != info->raid_disks)
6837                 cnt++;
6838         if (mddev->layout != info->layout)
6839                 cnt++;
6840         if ((state ^ info->state) & (1<<MD_SB_BITMAP_PRESENT))
6841                 cnt++;
6842         if (cnt == 0)
6843                 return 0;
6844         if (cnt > 1)
6845                 return -EINVAL;
...

Here I give a test to explain more details.

Steps:
1. create a raid5 array.
./mdadm -CR /dev/md0 -l5 -b internal -n2 -x1 /dev/loop10 /dev/loop11 
/dev/loop12
2. changing component size:
./mdadm -G /dev/md0 --size 511

Currently, the /sys/block/md0/md/chunk_size is default by 512*1024, and 
we required to
set new component size as 511K.

in mdadm:
it goes Grow_reshape() and sent a ioctl command of SET_ARRAY_INFO.

in md:
ioctl parses the command and goes to update_array_info  --> update_size 
--> mddev->pers->resize;
in raid5.c:
cut piece of code from raid5_resize().
...
7719         sectors &= ~((sector_t)conf->chunk_sectors - 1);
7720         newsize = raid5_size(mddev, sectors, mddev->raid_disks);
...

the line 7719 shows the valid sectors should be the integer times of 
conf->chunk_sectors,
but sectors would be '0' when sectors <= conf->chunk_sectors.
At this time, the raid5 array is still on-line, but it's not meaningful 
any more due to the new
component size has set as '0'.

Thus, firstly I expose this patch to user space and require some ideas, 
would you suggest
that add something in kernel space? such as ensure "sectors > 
conf->chunk_sectors"?

Thanks for your patience to correct me if any wrong here.

Best regards,
-Zhilong

> NeilBrown

Re: [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4901 bytes --]

On Tue, Oct 10 2017, Zhilong Liu wrote:

> On 10/09/2017 06:49 PM, NeilBrown wrote:
>> On Mon, Oct 09 2017, Zhilong Liu wrote:
>>
>>> To fix the commit: 4b74a905a67e
>>> (mdadm/grow: Component size must be larger than chunk size)
>>> Since cannot change component size at the same time as other
>>> changes, ensure the 'level' is UnSet when changing component
>>> size, and also not affect the raid level conversion.
>>>
>>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>>> ---
>>>   Grow.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Grow.c b/Grow.c
>>> index 1149753..180fd78 100644
>>> --- a/Grow.c
>>> +++ b/Grow.c
>>> @@ -1814,7 +1814,8 @@ int Grow_reshape(char *devname, int fd,
>>>   	}
>>>   
>>>   	if (array.level > 1 &&
>>> -	   (array.chunk_size / 1024) > (int)s->size) {
>>> +	   (array.chunk_size / 1024) > (int)s->size &&
>>> +	    s->level == UnSet) {
>>>   		pr_err("component size must be larger than chunk size.\n");
>>>   		return 1;
>>>   	}
>> This patch doesn't make any sense to me.
>
> Hi, Neil;
>    Sorry for the later reply due to meetings.
>
> I do agree your suggestion that adding test "s->size > 0" is more 
> comfortable than
> adding "s->level == UnSet".
>
> This patch mainly wanna ensure that changing new component size must be 
> ">=" current
> chunk size, otherwise the mddev->pers->resize would set the 
> mddev->dev_sectors as '0'.
>
> array.level > 1       ---> only against the raids which the chunk size 
> is meaningful.
> (array.chunk_size / 1024) > (int)s->size     ----> ensure the 
> explanation above.
> s->size > 0    ----> to ensure that valid re-size has required.
>
>> If the chunk_size is meaningful, then is must be less than the new
>> s->size.
>
> Yes here.
>> This is true whether the level is being changed or not.
>>
>> Why do you think that the component size cannot be changed at the same
>> time as other changes?
>
> Both user space and kernel space have codes to verify only one change 
> happens.
>
> in mdadm/Grow.c:   Grow_reshape()
> ...
> 1801         if (s->size > 0 &&
> 1802             (s->chunk || s->level!= UnSet || s->layout_str || 
> s->raiddisks)) {
> 1803                 pr_err("cannot change component size at the same 
> time as other changes.\n"
> 1804                         "   Change size first, then check data is 
> intact before making other changes.\n");
> 1805                 return 1;
> 1806         }
> ...

Ahhh, yes.  I had forgotten about that.  There was probably a good
reason.
Thanks.

>
> in md.c:  update_array_info()
> ...
> 6833         /* Check there is only one change */

This isn't directly relevant.  mdadm could send multiple change
commands, one after the other.


> 6834         if (info->size >= 0 && mddev->dev_sectors / 2 != info->size)
> 6835                 cnt++;
> 6836         if (mddev->raid_disks != info->raid_disks)
> 6837                 cnt++;
> 6838         if (mddev->layout != info->layout)
> 6839                 cnt++;
> 6840         if ((state ^ info->state) & (1<<MD_SB_BITMAP_PRESENT))
> 6841                 cnt++;
> 6842         if (cnt == 0)
> 6843                 return 0;
> 6844         if (cnt > 1)
> 6845                 return -EINVAL;
> ...
>
> Here I give a test to explain more details.
>
> Steps:
> 1. create a raid5 array.
> ./mdadm -CR /dev/md0 -l5 -b internal -n2 -x1 /dev/loop10 /dev/loop11 
> /dev/loop12
> 2. changing component size:
> ./mdadm -G /dev/md0 --size 511
>
> Currently, the /sys/block/md0/md/chunk_size is default by 512*1024, and 
> we required to
> set new component size as 511K.
>
> in mdadm:
> it goes Grow_reshape() and sent a ioctl command of SET_ARRAY_INFO.
>
> in md:
> ioctl parses the command and goes to update_array_info  --> update_size 
> --> mddev->pers->resize;
> in raid5.c:
> cut piece of code from raid5_resize().
> ...
> 7719         sectors &= ~((sector_t)conf->chunk_sectors - 1);
> 7720         newsize = raid5_size(mddev, sectors, mddev->raid_disks);
> ...
>
> the line 7719 shows the valid sectors should be the integer times of 
> conf->chunk_sectors,
> but sectors would be '0' when sectors <= conf->chunk_sectors.
> At this time, the raid5 array is still on-line, but it's not meaningful 
> any more due to the new
> component size has set as '0'.
>
> Thus, firstly I expose this patch to user space and require some ideas, 
> would you suggest
> that add something in kernel space? such as ensure "sectors > 
> conf->chunk_sectors"?

I would suggest
   if (sectors == 0)
       return -EINVAL;

between lines 7719 abd 7720.

raid10.c needs a similar change.  Probably raid1.c too.

Thanks,
NeilBrown

>
> Thanks for your patience to correct me if any wrong here.
>
> Best regards,
> -Zhilong
>
>> NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 2/3] mdadm/mdstat: fixup a number of '==' broken formatting

[Jes Sorensen]

On 10/09/2017 04:21 AM, Zhilong Liu wrote:
> This commit doesn't change any codes, just tidy up the
> code formatting.
> 
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> ---
>   mdstat.c | 39 ++++++++++++++++++++-------------------
>   1 file changed, 20 insertions(+), 19 deletions(-)

Applied!

Thanks,
Jes

Re: [PATCH 3/3] mdadm/mdstat: correct the strncmp number 4 as 6

[Jes Sorensen]

On 10/09/2017 04:21 AM, Zhilong Liu wrote:
> mdstat: it should be corrected as 6 when strncmp 'resync'.
> 
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> ---
>   mdstat.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Applied!

Thanks,
Jes

[PATCH v2] mdadm/grow: adding a test to ensure resize was required

[Zhilong Liu]

To fix the commit: 4b74a905a67e
(mdadm/grow: Component size must be larger than chunk size)

array.level > 1 : against the raids which chunk_size is meaningful.
s->size > 0 : ensure that changing component size has required.
array.chunk_size / 1024 > s->size : ensure component size should
be always >= current chunk_size when requires resize, otherwise,
mddev->pers->resize would be set mddev->dev_sectors as '0'.

Reported-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
Suggested-by: NeilBrown <neilb@suse.com>
Signed-off-by: Zhilong Liu <zlliu@suse.com>
---

v1: [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion

changes:
  correct the test 's->level == UnSet' as 's->size > 0'

 Grow.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Grow.c b/Grow.c
index 4d79d83..8c2d50c 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1810,6 +1810,7 @@ int Grow_reshape(char *devname, int fd,
 	}
 
 	if (array.level > 1 &&
+	    s->size > 0 &&
 	   (array.chunk_size / 1024) > (int)s->size) {
 		pr_err("component size must be larger than chunk size.\n");
 		return 1;
-- 
2.6.6

Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required

[Jes Sorensen]

On 10/11/2017 04:53 AM, Zhilong Liu wrote:
> To fix the commit: 4b74a905a67e
> (mdadm/grow: Component size must be larger than chunk size)
> 
> array.level > 1 : against the raids which chunk_size is meaningful.
> s->size > 0 : ensure that changing component size has required.
> array.chunk_size / 1024 > s->size : ensure component size should
> be always >= current chunk_size when requires resize, otherwise,
> mddev->pers->resize would be set mddev->dev_sectors as '0'.
> 
> Reported-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> Suggested-by: NeilBrown <neilb@suse.com>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> ---
> 
> v1: [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion
> 
> changes:
>    correct the test 's->level == UnSet' as 's->size > 0'
> 
>   Grow.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/Grow.c b/Grow.c
> index 4d79d83..8c2d50c 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1810,6 +1810,7 @@ int Grow_reshape(char *devname, int fd,
>   	}
>   
>   	if (array.level > 1 &&
> +	    s->size > 0 &&
>   	   (array.chunk_size / 1024) > (int)s->size) {
>   		pr_err("component size must be larger than chunk size.\n");
>   		return 1;

Applied, however I fixed the broken formatting in the process.

Thanks,
Jes

Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required

[John Stoffel]

>>>>> "Zhilong" == Zhilong Liu <zlliu@suse.com> writes:

Zhilong> To fix the commit: 4b74a905a67e
Zhilong> (mdadm/grow: Component size must be larger than chunk size)

Zhilong> array.level > 1 : against the raids which chunk_size is meaningful.
s-> size > 0 : ensure that changing component size has required.
Zhilong> array.chunk_size / 1024 > s->size : ensure component size should
Zhilong> be always >= current chunk_size when requires resize, otherwise,
mddev-> pers->resize would be set mddev->dev_sectors as '0'.

Is there any possibility of setting up a test harness that could be
used to check for problems like this automatically?  Sorta like the
xfstests that the filesystem people run?

I don't know if you've seen the discussion (I know Jes has) about
mdadm --grow allowing you to reduce an array size enough to nuke the
superblocks, so I wonder if this fix will also apply to that problem?



Zhilong> Reported-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
Zhilong> Suggested-by: NeilBrown <neilb@suse.com>
Zhilong> Signed-off-by: Zhilong Liu <zlliu@suse.com>
Zhilong> ---

Zhilong> v1: [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion

Zhilong> changes:
Zhilong>   correct the test 's->level == UnSet' as 's->size > 0'

Zhilong>  Grow.c | 1 +
Zhilong>  1 file changed, 1 insertion(+)

Zhilong> diff --git a/Grow.c b/Grow.c
Zhilong> index 4d79d83..8c2d50c 100644
Zhilong> --- a/Grow.c
Zhilong> +++ b/Grow.c
Zhilong> @@ -1810,6 +1810,7 @@ int Grow_reshape(char *devname, int fd,
Zhilong>  	}
 
Zhilong>  	if (array.level > 1 &&
Zhilong> +	    s->size > 0 &&
Zhilong>  	   (array.chunk_size / 1024) > (int)s->size) {
Zhilong>  		pr_err("component size must be larger than chunk size.\n");
Zhilong>  		return 1;
Zhilong> -- 
Zhilong> 2.6.6

Zhilong> --
Zhilong> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
Zhilong> the body of a message to majordomo@vger.kernel.org
Zhilong> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required

[Zhilong Liu]

On 10/12/2017 03:44 AM, John Stoffel wrote:
>>>>>> "Zhilong" == Zhilong Liu <zlliu@suse.com> writes:
> Zhilong> To fix the commit: 4b74a905a67e
> Zhilong> (mdadm/grow: Component size must be larger than chunk size)
>
> Zhilong> array.level > 1 : against the raids which chunk_size is meaningful.
> s-> size > 0 : ensure that changing component size has required.
> Zhilong> array.chunk_size / 1024 > s->size : ensure component size should
> Zhilong> be always >= current chunk_size when requires resize, otherwise,
> mddev-> pers->resize would be set mddev->dev_sectors as '0'.
>
> Is there any possibility of setting up a test harness that could be
> used to check for problems like this automatically?  Sorta like the
> xfstests that the filesystem people run?

Agree your suggestions, it's still many works to do in our test part, I have
done a little bit, but daily-work always take more time.
mdadm needs many reliable 'unit test cases' to support, I would continue
to do this once I have time.
Against this patch, I would re-draft the test case of 'tests/02r5grow', 
it can
cover to test this scenario.

>
> I don't know if you've seen the discussion (I know Jes has) about
> mdadm --grow allowing you to reduce an array size enough to nuke the
> superblocks, so I wonder if this fix will also apply to that problem?

Sorry for that, I have no memory with this issue. I do appreciate that if
have detail example here.

Thanks,
-Zhilong
>
>
> Zhilong> Reported-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> Zhilong> Suggested-by: NeilBrown <neilb@suse.com>
> Zhilong> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> Zhilong> ---
>
> Zhilong> v1: [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion
>
> Zhilong> changes:
> Zhilong>   correct the test 's->level == UnSet' as 's->size > 0'
>
> Zhilong>  Grow.c | 1 +
> Zhilong>  1 file changed, 1 insertion(+)
>
> Zhilong> diff --git a/Grow.c b/Grow.c
> Zhilong> index 4d79d83..8c2d50c 100644
> Zhilong> --- a/Grow.c
> Zhilong> +++ b/Grow.c
> Zhilong> @@ -1810,6 +1810,7 @@ int Grow_reshape(char *devname, int fd,
> Zhilong>  	}
>   
> Zhilong>  	if (array.level > 1 &&
> Zhilong> +	    s->size > 0 &&
> Zhilong>  	   (array.chunk_size / 1024) > (int)s->size) {
> Zhilong>  		pr_err("component size must be larger than chunk size.\n");
> Zhilong>  		return 1;
> Zhilong> --
> Zhilong> 2.6.6
>
> Zhilong> --
> Zhilong> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> Zhilong> the body of a message to majordomo@vger.kernel.org
> Zhilong> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

RE: [PATCH v2] mdadm/grow: adding a test to ensure resize was required

[Majchrzak, Tomasz]

Hi Jes,

I wonder if it would be a lot of inconvenience for you to push commits on more regular basis (optimally once accepted). We run daily regression tests on upstream mdadm and frequent pushes would allow us to see problems sooner.

Tomek

-----Original Message-----
From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-owner@vger.kernel.org] On Behalf Of Jes Sorensen
Sent: Wednesday, October 11, 2017 7:32 PM
To: Zhilong Liu <zlliu@suse.com>
Cc: linux-raid@vger.kernel.org
Subject: Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required

On 10/11/2017 04:53 AM, Zhilong Liu wrote:
> To fix the commit: 4b74a905a67e
> (mdadm/grow: Component size must be larger than chunk size)
> 
> array.level > 1 : against the raids which chunk_size is meaningful.
> s->size > 0 : ensure that changing component size has required.
> array.chunk_size / 1024 > s->size : ensure component size should
> be always >= current chunk_size when requires resize, otherwise,
> mddev->pers->resize would be set mddev->dev_sectors as '0'.
> 
> Reported-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> Suggested-by: NeilBrown <neilb@suse.com>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> ---
> 
> v1: [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion
> 
> changes:
>    correct the test 's->level == UnSet' as 's->size > 0'
> 
>   Grow.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/Grow.c b/Grow.c
> index 4d79d83..8c2d50c 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1810,6 +1810,7 @@ int Grow_reshape(char *devname, int fd,
>   	}
>   
>   	if (array.level > 1 &&
> +	    s->size > 0 &&
>   	   (array.chunk_size / 1024) > (int)s->size) {
>   		pr_err("component size must be larger than chunk size.\n");
>   		return 1;

Applied, however I fixed the broken formatting in the process.

Thanks,
Jes


--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required

[Zhilong Liu]

On 10/12/2017 01:31 AM, Jes Sorensen wrote:
> On 10/11/2017 04:53 AM, Zhilong Liu wrote:
>> To fix the commit: 4b74a905a67e
>> (mdadm/grow: Component size must be larger than chunk size)
>>
>> array.level > 1 : against the raids which chunk_size is meaningful.
>> s->size > 0 : ensure that changing component size has required.
>> array.chunk_size / 1024 > s->size : ensure component size should
>> be always >= current chunk_size when requires resize, otherwise,
>> mddev->pers->resize would be set mddev->dev_sectors as '0'.
>>
>> Reported-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
>> Suggested-by: NeilBrown <neilb@suse.com>
>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>> ---
>>
>> v1: [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion
>>
>> changes:
>>    correct the test 's->level == UnSet' as 's->size > 0'
>>
>>   Grow.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/Grow.c b/Grow.c
>> index 4d79d83..8c2d50c 100644
>> --- a/Grow.c
>> +++ b/Grow.c
>> @@ -1810,6 +1810,7 @@ int Grow_reshape(char *devname, int fd,
>>       }
>>         if (array.level > 1 &&
>> +        s->size > 0 &&
>>          (array.chunk_size / 1024) > (int)s->size) {
>>           pr_err("component size must be larger than chunk size.\n");
>>           return 1;
>
> Applied, however I fixed the broken formatting in the process.
>

I'm sorry that I have re-checked the code, this patch is buggy.
The correct fix should be "s->size > 1" or "s->level == UnSet",
I forgot the parameter of "--size max".

Thanks,
-Zhilong

> Thanks,
> Jes
>
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH v3] mdadm/grow: adding a test to ensure resize was required

[Zhilong Liu]

To fix the commit: 4b74a905a67e
(mdadm/grow: Component size must be larger than chunk size)

array.level > 1 : against the raids which chunk_size is meaningful.
s->size > 1 : ensure changing component size has required, s->size
is '1' when '--grow --size max' parameter is specified.
array.chunk_size / 1024 > s->size : ensure component size should
be always >= current chunk_size when requires resize, otherwise,
mddev->pers->resize would be set mddev->dev_sectors as '0'.

Reported-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
Suggested-by: NeilBrown <neilb@suse.com>
Signed-off-by: Zhilong Liu <zlliu@suse.com>
---

v1: [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion
v2: [PATCH v2] mdadm/grow: adding a test to ensure resize was required

changes:
  v2: correct the test 's->level == UnSet' as 's->size > 0' against v1
  v3: correct 's->size > 0' as 's->size > 1' against v2, s->size is '1'
      when '--grow --size max' parameter is specified.

 Grow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Grow.c b/Grow.c
index 4d79d83..e14c921 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1810,7 +1810,8 @@ int Grow_reshape(char *devname, int fd,
 	}
 
 	if (array.level > 1 &&
-	   (array.chunk_size / 1024) > (int)s->size) {
+	    s->size > 1 &&
+	    (array.chunk_size / 1024) > (int)s->size) {
 		pr_err("component size must be larger than chunk size.\n");
 		return 1;
 	}
-- 
2.6.6

Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required

[Jes Sorensen]

On 10/12/2017 06:55 AM, Majchrzak, Tomasz wrote:
> Hi Jes,
> 
> I wonder if it would be a lot of inconvenience for you to push commits on more regular basis (optimally once accepted). We run daily regression tests on upstream mdadm and frequent pushes would allow us to see problems sooner.

Hi Tomek,

I generally try to push when I apply patches. It happens I miss it, 
especially if it's just a single smaller patch.

I just pushed the tree now.

Cheers,
Jes

Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required

[Zhilong Liu]

On 10/24/2017 12:43 AM, Jes Sorensen wrote:
> On 10/12/2017 06:55 AM, Majchrzak, Tomasz wrote:
>> Hi Jes,
>>
>> I wonder if it would be a lot of inconvenience for you to push 
>> commits on more regular basis (optimally once accepted). We run daily 
>> regression tests on upstream mdadm and frequent pushes would allow us 
>> to see problems sooner.
>
> Hi Tomek,
>
> I generally try to push when I apply patches. It happens I miss it, 
> especially if it's just a single smaller patch.
>
> I just pushed the tree now.
>

Hi Jes,
   Yesterday I sent a v3 version in-reply-to v2 in this mail tree, maybe 
I shall re-send another commit
based on current head-commit?

Thanks,
-Zhilong

> Cheers,
> Jes
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required

[Tomasz Majchrzak]

On Tue, Oct 24, 2017 at 02:21:09PM +0800, Zhilong Liu wrote:
> 
> Hi Jes,
>   Yesterday I sent a v3 version in-reply-to v2 in this mail tree,
> maybe I shall re-send another commit
> based on current head-commit?
> 
> Thanks,
> -Zhilong

Hi Zhilong,

I see that version 2 of the patch has been applied to the repository but
changes from v3 have never made it there. Please send the rebased patch
again as grow doesn't work "--max" parameter.

Thanks,

Tomek