* [mdadm PATCH 0/3] fixed broken level conversion and one strncmp issue @ 2017-10-09 8:21 Zhilong Liu 2017-10-09 8:21 ` [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion Zhilong Liu ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Zhilong Liu @ 2017-10-09 8:21 UTC (permalink / raw) To: Jes.Sorensen; +Cc: linux-raid, 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion 2017-10-09 8:21 [mdadm PATCH 0/3] fixed broken level conversion and one strncmp issue Zhilong Liu @ 2017-10-09 8:21 ` Zhilong Liu 2017-10-09 10:49 ` NeilBrown 2017-10-11 8:53 ` [PATCH v2] mdadm/grow: adding a test to ensure resize was required Zhilong Liu 2017-10-09 8:21 ` [PATCH 2/3] mdadm/mdstat: fixup a number of '==' broken formatting Zhilong Liu 2017-10-09 8:21 ` [PATCH 3/3] mdadm/mdstat: correct the strncmp number 4 as 6 Zhilong Liu 2 siblings, 2 replies; 19+ messages in thread From: Zhilong Liu @ 2017-10-09 8:21 UTC (permalink / raw) To: Jes.Sorensen; +Cc: linux-raid, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion 2017-10-09 8:21 ` [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion Zhilong Liu @ 2017-10-09 10:49 ` NeilBrown 2017-10-10 8:46 ` Zhilong Liu 2017-10-11 8:53 ` [PATCH v2] mdadm/grow: adding a test to ensure resize was required Zhilong Liu 1 sibling, 1 reply; 19+ messages in thread From: NeilBrown @ 2017-10-09 10:49 UTC (permalink / raw) To: Jes.Sorensen; +Cc: linux-raid, Zhilong Liu [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion 2017-10-09 10:49 ` NeilBrown @ 2017-10-10 8:46 ` Zhilong Liu 2017-10-10 20:31 ` NeilBrown 0 siblings, 1 reply; 19+ messages in thread From: Zhilong Liu @ 2017-10-10 8:46 UTC (permalink / raw) To: NeilBrown, Jes.Sorensen; +Cc: linux-raid 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion 2017-10-10 8:46 ` Zhilong Liu @ 2017-10-10 20:31 ` NeilBrown 0 siblings, 0 replies; 19+ messages in thread From: NeilBrown @ 2017-10-10 20:31 UTC (permalink / raw) To: Zhilong Liu, Jes.Sorensen; +Cc: linux-raid [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] mdadm/grow: adding a test to ensure resize was required 2017-10-09 8:21 ` [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion Zhilong Liu 2017-10-09 10:49 ` NeilBrown @ 2017-10-11 8:53 ` Zhilong Liu 2017-10-11 17:31 ` Jes Sorensen ` (2 more replies) 1 sibling, 3 replies; 19+ messages in thread From: Zhilong Liu @ 2017-10-11 8:53 UTC (permalink / raw) To: Jes.Sorensen; +Cc: linux-raid, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required 2017-10-11 8:53 ` [PATCH v2] mdadm/grow: adding a test to ensure resize was required Zhilong Liu @ 2017-10-11 17:31 ` Jes Sorensen 2017-10-12 10:55 ` Majchrzak, Tomasz 2017-10-18 8:01 ` Zhilong Liu 2017-10-11 19:44 ` John Stoffel 2017-10-23 9:13 ` [PATCH v3] " Zhilong Liu 2 siblings, 2 replies; 19+ messages in thread From: Jes Sorensen @ 2017-10-11 17:31 UTC (permalink / raw) To: Zhilong Liu; +Cc: linux-raid 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH v2] mdadm/grow: adding a test to ensure resize was required 2017-10-11 17:31 ` Jes Sorensen @ 2017-10-12 10:55 ` Majchrzak, Tomasz 2017-10-23 16:43 ` Jes Sorensen 2017-10-18 8:01 ` Zhilong Liu 1 sibling, 1 reply; 19+ messages in thread From: Majchrzak, Tomasz @ 2017-10-12 10:55 UTC (permalink / raw) To: Jes Sorensen; +Cc: linux-raid@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required 2017-10-12 10:55 ` Majchrzak, Tomasz @ 2017-10-23 16:43 ` Jes Sorensen 2017-10-24 6:21 ` Zhilong Liu 0 siblings, 1 reply; 19+ messages in thread From: Jes Sorensen @ 2017-10-23 16:43 UTC (permalink / raw) To: Majchrzak, Tomasz; +Cc: linux-raid@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required 2017-10-23 16:43 ` Jes Sorensen @ 2017-10-24 6:21 ` Zhilong Liu 2017-11-22 10:07 ` Tomasz Majchrzak 0 siblings, 1 reply; 19+ messages in thread From: Zhilong Liu @ 2017-10-24 6:21 UTC (permalink / raw) To: Jes Sorensen; +Cc: Majchrzak, Tomasz, linux-raid@vger.kernel.org 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required 2017-10-24 6:21 ` Zhilong Liu @ 2017-11-22 10:07 ` Tomasz Majchrzak 0 siblings, 0 replies; 19+ messages in thread From: Tomasz Majchrzak @ 2017-11-22 10:07 UTC (permalink / raw) To: Zhilong Liu; +Cc: Jes Sorensen, linux-raid@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required 2017-10-11 17:31 ` Jes Sorensen 2017-10-12 10:55 ` Majchrzak, Tomasz @ 2017-10-18 8:01 ` Zhilong Liu 1 sibling, 0 replies; 19+ messages in thread From: Zhilong Liu @ 2017-10-18 8:01 UTC (permalink / raw) To: Jes Sorensen; +Cc: linux-raid 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required 2017-10-11 8:53 ` [PATCH v2] mdadm/grow: adding a test to ensure resize was required Zhilong Liu 2017-10-11 17:31 ` Jes Sorensen @ 2017-10-11 19:44 ` John Stoffel 2017-10-12 3:25 ` Zhilong Liu 2017-10-23 9:13 ` [PATCH v3] " Zhilong Liu 2 siblings, 1 reply; 19+ messages in thread From: John Stoffel @ 2017-10-11 19:44 UTC (permalink / raw) To: Zhilong Liu; +Cc: Jes.Sorensen, linux-raid >>>>> "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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] mdadm/grow: adding a test to ensure resize was required 2017-10-11 19:44 ` John Stoffel @ 2017-10-12 3:25 ` Zhilong Liu 0 siblings, 0 replies; 19+ messages in thread From: Zhilong Liu @ 2017-10-12 3:25 UTC (permalink / raw) To: John Stoffel; +Cc: Jes.Sorensen, linux-raid 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3] mdadm/grow: adding a test to ensure resize was required 2017-10-11 8:53 ` [PATCH v2] mdadm/grow: adding a test to ensure resize was required Zhilong Liu 2017-10-11 17:31 ` Jes Sorensen 2017-10-11 19:44 ` John Stoffel @ 2017-10-23 9:13 ` Zhilong Liu 2 siblings, 0 replies; 19+ messages in thread From: Zhilong Liu @ 2017-10-23 9:13 UTC (permalink / raw) To: Jes.Sorensen; +Cc: linux-raid, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/3] mdadm/mdstat: fixup a number of '==' broken formatting 2017-10-09 8:21 [mdadm PATCH 0/3] fixed broken level conversion and one strncmp issue Zhilong Liu 2017-10-09 8:21 ` [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion Zhilong Liu @ 2017-10-09 8:21 ` Zhilong Liu 2017-10-10 20:37 ` Jes Sorensen 2017-10-09 8:21 ` [PATCH 3/3] mdadm/mdstat: correct the strncmp number 4 as 6 Zhilong Liu 2 siblings, 1 reply; 19+ messages in thread From: Zhilong Liu @ 2017-10-09 8:21 UTC (permalink / raw) To: Jes.Sorensen; +Cc: linux-raid, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] mdadm/mdstat: fixup a number of '==' broken formatting 2017-10-09 8:21 ` [PATCH 2/3] mdadm/mdstat: fixup a number of '==' broken formatting Zhilong Liu @ 2017-10-10 20:37 ` Jes Sorensen 0 siblings, 0 replies; 19+ messages in thread From: Jes Sorensen @ 2017-10-10 20:37 UTC (permalink / raw) To: Zhilong Liu; +Cc: linux-raid 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] mdadm/mdstat: correct the strncmp number 4 as 6 2017-10-09 8:21 [mdadm PATCH 0/3] fixed broken level conversion and one strncmp issue Zhilong Liu 2017-10-09 8:21 ` [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion Zhilong Liu 2017-10-09 8:21 ` [PATCH 2/3] mdadm/mdstat: fixup a number of '==' broken formatting Zhilong Liu @ 2017-10-09 8:21 ` Zhilong Liu 2017-10-10 20:38 ` Jes Sorensen 2 siblings, 1 reply; 19+ messages in thread From: Zhilong Liu @ 2017-10-09 8:21 UTC (permalink / raw) To: Jes.Sorensen; +Cc: linux-raid, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] mdadm/mdstat: correct the strncmp number 4 as 6 2017-10-09 8:21 ` [PATCH 3/3] mdadm/mdstat: correct the strncmp number 4 as 6 Zhilong Liu @ 2017-10-10 20:38 ` Jes Sorensen 0 siblings, 0 replies; 19+ messages in thread From: Jes Sorensen @ 2017-10-10 20:38 UTC (permalink / raw) To: Zhilong Liu; +Cc: linux-raid 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-11-22 10:07 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-09 8:21 [mdadm PATCH 0/3] fixed broken level conversion and one strncmp issue Zhilong Liu 2017-10-09 8:21 ` [PATCH 1/3] mdadm/Grow: fix the broken raid level conversion Zhilong Liu 2017-10-09 10:49 ` NeilBrown 2017-10-10 8:46 ` Zhilong Liu 2017-10-10 20:31 ` NeilBrown 2017-10-11 8:53 ` [PATCH v2] mdadm/grow: adding a test to ensure resize was required Zhilong Liu 2017-10-11 17:31 ` Jes Sorensen 2017-10-12 10:55 ` Majchrzak, Tomasz 2017-10-23 16:43 ` Jes Sorensen 2017-10-24 6:21 ` Zhilong Liu 2017-11-22 10:07 ` Tomasz Majchrzak 2017-10-18 8:01 ` Zhilong Liu 2017-10-11 19:44 ` John Stoffel 2017-10-12 3:25 ` Zhilong Liu 2017-10-23 9:13 ` [PATCH v3] " Zhilong Liu 2017-10-09 8:21 ` [PATCH 2/3] mdadm/mdstat: fixup a number of '==' broken formatting Zhilong Liu 2017-10-10 20:37 ` Jes Sorensen 2017-10-09 8:21 ` [PATCH 3/3] mdadm/mdstat: correct the strncmp number 4 as 6 Zhilong Liu 2017-10-10 20:38 ` Jes Sorensen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).