* [mdadm PATCH] Create: move STOP_ARRAY to abort_locked @ 2017-04-26 7:03 Zhilong Liu 2017-05-04 12:20 ` Zhilong Liu 0 siblings, 1 reply; 9+ messages in thread From: Zhilong Liu @ 2017-04-26 7:03 UTC (permalink / raw) To: Jes.Sorensen; +Cc: neilb, linux-raid, Zhilong Liu The sysfs entry and devnm would be created once create_mddev() performed successfully, but the creating isn't completed here, move STOP_ARRAY to abort_locked, the purpose is to cleanup the partially created array. Signed-off-by: Zhilong Liu <zlliu@suse.com> --- Create.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/Create.c b/Create.c index 6ca0924..fe0ab7e 100644 --- a/Create.c +++ b/Create.c @@ -904,10 +904,8 @@ int Create(struct supertype *st, char *mddev, remove_partitions(fd); if (st->ss->add_to_super(st, &inf->disk, fd, dv->devname, - dv->data_offset)) { - ioctl(mdfd, STOP_ARRAY, NULL); + dv->data_offset)) goto abort_locked; - } st->ss->getinfo_super(st, inf, NULL); safe_mode_delay = inf->safe_mode_delay; @@ -1008,7 +1006,6 @@ int Create(struct supertype *st, char *mddev, sysfs_set_safemode(&info, safe_mode_delay); if (err) { pr_err("failed to activate array.\n"); - ioctl(mdfd, STOP_ARRAY, NULL); goto abort; } } else if (c->readonly && @@ -1018,7 +1015,6 @@ int Create(struct supertype *st, char *mddev, "array_state", "readonly") < 0) { pr_err("Failed to start array: %s\n", strerror(errno)); - ioctl(mdfd, STOP_ARRAY, NULL); goto abort; } } else { @@ -1030,7 +1026,6 @@ int Create(struct supertype *st, char *mddev, if (info.array.chunk_size & (info.array.chunk_size-1)) { cont_err("Problem may be that chunk size is not a power of 2\n"); } - ioctl(mdfd, STOP_ARRAY, NULL); goto abort; } /* if start_ro module parameter is set, array is @@ -1061,7 +1056,9 @@ int Create(struct supertype *st, char *mddev, map_remove(&map, fd2devnm(mdfd)); map_unlock(&map); - if (mdfd >= 0) + if (mdfd >= 0) { + ioctl(mdfd, STOP_ARRAY, NULL); close(mdfd); + } return 1; } -- 2.6.6 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked 2017-04-26 7:03 [mdadm PATCH] Create: move STOP_ARRAY to abort_locked Zhilong Liu @ 2017-05-04 12:20 ` Zhilong Liu 2017-05-04 14:54 ` Jes Sorensen 0 siblings, 1 reply; 9+ messages in thread From: Zhilong Liu @ 2017-05-04 12:20 UTC (permalink / raw) To: Jes.Sorensen; +Cc: linux-raid Hi Jes, apply for review, this is a bug I ever encountered. On 04/26/2017 03:03 PM, Zhilong Liu wrote: > The sysfs entry and devnm would be created once create_mddev() > performed successfully, but the creating isn't completed here, > move STOP_ARRAY to abort_locked, the purpose is to cleanup the > partially created array. > > Signed-off-by: Zhilong Liu <zlliu@suse.com> > --- > Create.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/Create.c b/Create.c > index 6ca0924..fe0ab7e 100644 > --- a/Create.c > +++ b/Create.c > @@ -904,10 +904,8 @@ int Create(struct supertype *st, char *mddev, > remove_partitions(fd); > if (st->ss->add_to_super(st, &inf->disk, > fd, dv->devname, > - dv->data_offset)) { > - ioctl(mdfd, STOP_ARRAY, NULL); > + dv->data_offset)) > goto abort_locked; > - } > st->ss->getinfo_super(st, inf, NULL); > safe_mode_delay = inf->safe_mode_delay; > > @@ -1008,7 +1006,6 @@ int Create(struct supertype *st, char *mddev, > sysfs_set_safemode(&info, safe_mode_delay); > if (err) { > pr_err("failed to activate array.\n"); > - ioctl(mdfd, STOP_ARRAY, NULL); > goto abort; > } > } else if (c->readonly && > @@ -1018,7 +1015,6 @@ int Create(struct supertype *st, char *mddev, > "array_state", "readonly") < 0) { > pr_err("Failed to start array: %s\n", > strerror(errno)); > - ioctl(mdfd, STOP_ARRAY, NULL); > goto abort; > } > } else { > @@ -1030,7 +1026,6 @@ int Create(struct supertype *st, char *mddev, > if (info.array.chunk_size & (info.array.chunk_size-1)) { > cont_err("Problem may be that chunk size is not a power of 2\n"); > } > - ioctl(mdfd, STOP_ARRAY, NULL); > goto abort; > } > /* if start_ro module parameter is set, array is > @@ -1061,7 +1056,9 @@ int Create(struct supertype *st, char *mddev, > map_remove(&map, fd2devnm(mdfd)); > map_unlock(&map); > > - if (mdfd >= 0) > + if (mdfd >= 0) { > + ioctl(mdfd, STOP_ARRAY, NULL); > close(mdfd); > + } > return 1; > } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked 2017-05-04 12:20 ` Zhilong Liu @ 2017-05-04 14:54 ` Jes Sorensen 2017-05-04 17:42 ` Zhilong 2017-05-05 3:31 ` Liu Zhilong 0 siblings, 2 replies; 9+ messages in thread From: Jes Sorensen @ 2017-05-04 14:54 UTC (permalink / raw) To: Zhilong Liu; +Cc: linux-raid On 05/04/2017 08:20 AM, Zhilong Liu wrote: > Hi Jes, > > apply for review, this is a bug I ever encountered. Zhilong, Under what circumstances do you see this? Thanks, Jes > > On 04/26/2017 03:03 PM, Zhilong Liu wrote: >> The sysfs entry and devnm would be created once create_mddev() >> performed successfully, but the creating isn't completed here, >> move STOP_ARRAY to abort_locked, the purpose is to cleanup the >> partially created array. >> >> Signed-off-by: Zhilong Liu <zlliu@suse.com> >> --- >> Create.c | 11 ++++------- >> 1 file changed, 4 insertions(+), 7 deletions(-) >> >> diff --git a/Create.c b/Create.c >> index 6ca0924..fe0ab7e 100644 >> --- a/Create.c >> +++ b/Create.c >> @@ -904,10 +904,8 @@ int Create(struct supertype *st, char *mddev, >> remove_partitions(fd); >> if (st->ss->add_to_super(st, &inf->disk, >> fd, dv->devname, >> - dv->data_offset)) { >> - ioctl(mdfd, STOP_ARRAY, NULL); >> + dv->data_offset)) >> goto abort_locked; >> - } >> st->ss->getinfo_super(st, inf, NULL); >> safe_mode_delay = inf->safe_mode_delay; >> @@ -1008,7 +1006,6 @@ int Create(struct supertype *st, char *mddev, >> sysfs_set_safemode(&info, safe_mode_delay); >> if (err) { >> pr_err("failed to activate array.\n"); >> - ioctl(mdfd, STOP_ARRAY, NULL); >> goto abort; >> } >> } else if (c->readonly && >> @@ -1018,7 +1015,6 @@ int Create(struct supertype *st, char *mddev, >> "array_state", "readonly") < 0) { >> pr_err("Failed to start array: %s\n", >> strerror(errno)); >> - ioctl(mdfd, STOP_ARRAY, NULL); >> goto abort; >> } >> } else { >> @@ -1030,7 +1026,6 @@ int Create(struct supertype *st, char *mddev, >> if (info.array.chunk_size & >> (info.array.chunk_size-1)) { >> cont_err("Problem may be that chunk size is not >> a power of 2\n"); >> } >> - ioctl(mdfd, STOP_ARRAY, NULL); >> goto abort; >> } >> /* if start_ro module parameter is set, array is >> @@ -1061,7 +1056,9 @@ int Create(struct supertype *st, char *mddev, >> map_remove(&map, fd2devnm(mdfd)); >> map_unlock(&map); >> - if (mdfd >= 0) >> + if (mdfd >= 0) { >> + ioctl(mdfd, STOP_ARRAY, NULL); >> close(mdfd); >> + } >> return 1; >> } > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked 2017-05-04 14:54 ` Jes Sorensen @ 2017-05-04 17:42 ` Zhilong 2017-05-05 3:31 ` Liu Zhilong 1 sibling, 0 replies; 9+ messages in thread From: Zhilong @ 2017-05-04 17:42 UTC (permalink / raw) To: Jes Sorensen; +Cc: linux-raid 发自我的 iPhone > 在 2017年5月4日，22:54，Jes Sorensen <jes.sorensen@gmail.com> 写道： > >> On 05/04/2017 08:20 AM, Zhilong Liu wrote: >> Hi Jes, >> >> apply for review, this is a bug I ever encountered. > > Zhilong, > > Under what circumstances do you see this? > I have tested this path after invoking create_mddev, such as issue the command: ./mdadm -CR /dev/md0 -b internal -l1 -n2 /dev/loop[0-1] --size 63 It would fail and print unsupported chunk size. But the device /dev/md0 and sysfs didn't cleanup after abort creating. For raid1, the chunk size is not meaningful, but mdadm doesn't allow component size is less than 64k when creating bitmap, thus I choose this command to test. Thanks, Zhilong > Thanks, > Jes > >> >>> On 04/26/2017 03:03 PM, Zhilong Liu wrote: >>> The sysfs entry and devnm would be created once create_mddev() >>> performed successfully, but the creating isn't completed here, >>> move STOP_ARRAY to abort_locked, the purpose is to cleanup the >>> partially created array. >>> >>> Signed-off-by: Zhilong Liu <zlliu@suse.com> >>> --- >>> Create.c | 11 ++++------- >>> 1 file changed, 4 insertions(+), 7 deletions(-) >>> >>> diff --git a/Create.c b/Create.c >>> index 6ca0924..fe0ab7e 100644 >>> --- a/Create.c >>> +++ b/Create.c >>> @@ -904,10 +904,8 @@ int Create(struct supertype *st, char *mddev, >>> remove_partitions(fd); >>> if (st->ss->add_to_super(st, &inf->disk, >>> fd, dv->devname, >>> - dv->data_offset)) { >>> - ioctl(mdfd, STOP_ARRAY, NULL); >>> + dv->data_offset)) >>> goto abort_locked; >>> - } >>> st->ss->getinfo_super(st, inf, NULL); >>> safe_mode_delay = inf->safe_mode_delay; >>> @@ -1008,7 +1006,6 @@ int Create(struct supertype *st, char *mddev, >>> sysfs_set_safemode(&info, safe_mode_delay); >>> if (err) { >>> pr_err("failed to activate array.\n"); >>> - ioctl(mdfd, STOP_ARRAY, NULL); >>> goto abort; >>> } >>> } else if (c->readonly && >>> @@ -1018,7 +1015,6 @@ int Create(struct supertype *st, char *mddev, >>> "array_state", "readonly") < 0) { >>> pr_err("Failed to start array: %s\n", >>> strerror(errno)); >>> - ioctl(mdfd, STOP_ARRAY, NULL); >>> goto abort; >>> } >>> } else { >>> @@ -1030,7 +1026,6 @@ int Create(struct supertype *st, char *mddev, >>> if (info.array.chunk_size & >>> (info.array.chunk_size-1)) { >>> cont_err("Problem may be that chunk size is not >>> a power of 2\n"); >>> } >>> - ioctl(mdfd, STOP_ARRAY, NULL); >>> goto abort; >>> } >>> /* if start_ro module parameter is set, array is >>> @@ -1061,7 +1056,9 @@ int Create(struct supertype *st, char *mddev, >>> map_remove(&map, fd2devnm(mdfd)); >>> map_unlock(&map); >>> - if (mdfd >= 0) >>> + if (mdfd >= 0) { >>> + ioctl(mdfd, STOP_ARRAY, NULL); >>> close(mdfd); >>> + } >>> return 1; >>> } >> > > -- > 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] 9+ messages in thread
* Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked 2017-05-04 14:54 ` Jes Sorensen 2017-05-04 17:42 ` Zhilong @ 2017-05-05 3:31 ` Liu Zhilong 2017-05-08 1:50 ` Zhilong Liu 1 sibling, 1 reply; 9+ messages in thread From: Liu Zhilong @ 2017-05-05 3:31 UTC (permalink / raw) To: Jes Sorensen; +Cc: linux-raid On 05/04/2017 10:54 PM, Jes Sorensen wrote: > On 05/04/2017 08:20 AM, Zhilong Liu wrote: >> Hi Jes, >> >> apply for review, this is a bug I ever encountered. > > Zhilong, > > Under what circumstances do you see this? > Issued the command: linux-g0sr:/home/test # ./mdadm -CR /dev/md0 -l1 -n2 -b internal /dev/loop[0-1] --size 63 ... ... ... mdadm: Given bitmap chunk size not supported. linux-g0sr:/home/test # ls /dev/md0 /dev/md0 linux-g0sr:/home/test # ls /sys/block/md0/md/ array_size bitmap component_size level metadata_version raid_disks reshape_position safe_mode_delay array_state chunk_size layout max_read_errors new_dev reshape_direction resync_start create_mddev() writes the devnm to /sys/module/md_mod/parameter/new_array, then in md.c, module_param_call() called the 'set' function of add_named_array(), md_alloc() init_and_add the kobject for devm, finally the devnm device has created and sysfs has registered after create_mddev executed successfully. Thus it's better to STOP_ARRAY in any case after create_mddev() invoked. Thanks, -Zhilong > Thanks, > Jes > >> >> On 04/26/2017 03:03 PM, Zhilong Liu wrote: >>> The sysfs entry and devnm would be created once create_mddev() >>> performed successfully, but the creating isn't completed here, >>> move STOP_ARRAY to abort_locked, the purpose is to cleanup the >>> partially created array. >>> >>> Signed-off-by: Zhilong Liu <zlliu@suse.com> >>> --- >>> Create.c | 11 ++++------- >>> 1 file changed, 4 insertions(+), 7 deletions(-) >>> >>> diff --git a/Create.c b/Create.c >>> index 6ca0924..fe0ab7e 100644 >>> --- a/Create.c >>> +++ b/Create.c >>> @@ -904,10 +904,8 @@ int Create(struct supertype *st, char *mddev, >>> remove_partitions(fd); >>> if (st->ss->add_to_super(st, &inf->disk, >>> fd, dv->devname, >>> - dv->data_offset)) { >>> - ioctl(mdfd, STOP_ARRAY, NULL); >>> + dv->data_offset)) >>> goto abort_locked; >>> - } >>> st->ss->getinfo_super(st, inf, NULL); >>> safe_mode_delay = inf->safe_mode_delay; >>> @@ -1008,7 +1006,6 @@ int Create(struct supertype *st, char *mddev, >>> sysfs_set_safemode(&info, safe_mode_delay); >>> if (err) { >>> pr_err("failed to activate array.\n"); >>> - ioctl(mdfd, STOP_ARRAY, NULL); >>> goto abort; >>> } >>> } else if (c->readonly && >>> @@ -1018,7 +1015,6 @@ int Create(struct supertype *st, char *mddev, >>> "array_state", "readonly") < 0) { >>> pr_err("Failed to start array: %s\n", >>> strerror(errno)); >>> - ioctl(mdfd, STOP_ARRAY, NULL); >>> goto abort; >>> } >>> } else { >>> @@ -1030,7 +1026,6 @@ int Create(struct supertype *st, char *mddev, >>> if (info.array.chunk_size & >>> (info.array.chunk_size-1)) { >>> cont_err("Problem may be that chunk size is not >>> a power of 2\n"); >>> } >>> - ioctl(mdfd, STOP_ARRAY, NULL); >>> goto abort; >>> } >>> /* if start_ro module parameter is set, array is >>> @@ -1061,7 +1056,9 @@ int Create(struct supertype *st, char *mddev, >>> map_remove(&map, fd2devnm(mdfd)); >>> map_unlock(&map); >>> - if (mdfd >= 0) >>> + if (mdfd >= 0) { >>> + ioctl(mdfd, STOP_ARRAY, NULL); >>> close(mdfd); >>> + } >>> return 1; >>> } >> > > -- > 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] 9+ messages in thread
* Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked 2017-05-05 3:31 ` Liu Zhilong @ 2017-05-08 1:50 ` Zhilong Liu 2017-05-08 17:54 ` Jes Sorensen 0 siblings, 1 reply; 9+ messages in thread From: Zhilong Liu @ 2017-05-08 1:50 UTC (permalink / raw) To: Jes Sorensen; +Cc: linux-raid On 05/05/2017 11:31 AM, Liu Zhilong wrote: > > > On 05/04/2017 10:54 PM, Jes Sorensen wrote: >> On 05/04/2017 08:20 AM, Zhilong Liu wrote: >>> Hi Jes, >>> >>> apply for review, this is a bug I ever encountered. >> >> Zhilong, >> >> Under what circumstances do you see this? >> > > Issued the command: > linux-g0sr:/home/test # ./mdadm -CR /dev/md0 -l1 -n2 -b internal > /dev/loop[0-1] --size 63 > ... ... ... > mdadm: Given bitmap chunk size not supported. > linux-g0sr:/home/test # ls /dev/md0 > /dev/md0 > linux-g0sr:/home/test # ls /sys/block/md0/md/ > array_size bitmap component_size level metadata_version > raid_disks reshape_position safe_mode_delay > array_state chunk_size layout max_read_errors > new_dev reshape_direction resync_start > > create_mddev() writes the devnm to > /sys/module/md_mod/parameter/new_array, > then in md.c, module_param_call() called the 'set' function of > add_named_array(), > md_alloc() init_and_add the kobject for devm, finally the devnm device > has created > and sysfs has registered after create_mddev executed successfully. > Thus it's better > to STOP_ARRAY in any case after create_mddev() invoked. > this patch depends on the kernel commit: 039b7225e6e9 ("md: allow creation of mdNNN arrays via md_mod/parameters/new_array") Neil's patch has set "mddev->hold_active = UNTIL_STOP", thus the STOP_ARRAY can work well on this situation. Thanks, -Zhilong > Thanks, > -Zhilong > >> Thanks, >> Jes >> >>> >>> On 04/26/2017 03:03 PM, Zhilong Liu wrote: >>>> The sysfs entry and devnm would be created once create_mddev() >>>> performed successfully, but the creating isn't completed here, >>>> move STOP_ARRAY to abort_locked, the purpose is to cleanup the >>>> partially created array. >>>> >>>> Signed-off-by: Zhilong Liu <zlliu@suse.com> >>>> --- >>>> Create.c | 11 ++++------- >>>> 1 file changed, 4 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/Create.c b/Create.c >>>> index 6ca0924..fe0ab7e 100644 >>>> --- a/Create.c >>>> +++ b/Create.c >>>> @@ -904,10 +904,8 @@ int Create(struct supertype *st, char *mddev, >>>> remove_partitions(fd); >>>> if (st->ss->add_to_super(st, &inf->disk, >>>> fd, dv->devname, >>>> - dv->data_offset)) { >>>> - ioctl(mdfd, STOP_ARRAY, NULL); >>>> + dv->data_offset)) >>>> goto abort_locked; >>>> - } >>>> st->ss->getinfo_super(st, inf, NULL); >>>> safe_mode_delay = inf->safe_mode_delay; >>>> @@ -1008,7 +1006,6 @@ int Create(struct supertype *st, char *mddev, >>>> sysfs_set_safemode(&info, safe_mode_delay); >>>> if (err) { >>>> pr_err("failed to activate array.\n"); >>>> - ioctl(mdfd, STOP_ARRAY, NULL); >>>> goto abort; >>>> } >>>> } else if (c->readonly && >>>> @@ -1018,7 +1015,6 @@ int Create(struct supertype *st, char *mddev, >>>> "array_state", "readonly") < 0) { >>>> pr_err("Failed to start array: %s\n", >>>> strerror(errno)); >>>> - ioctl(mdfd, STOP_ARRAY, NULL); >>>> goto abort; >>>> } >>>> } else { >>>> @@ -1030,7 +1026,6 @@ int Create(struct supertype *st, char *mddev, >>>> if (info.array.chunk_size & >>>> (info.array.chunk_size-1)) { >>>> cont_err("Problem may be that chunk size is not >>>> a power of 2\n"); >>>> } >>>> - ioctl(mdfd, STOP_ARRAY, NULL); >>>> goto abort; >>>> } >>>> /* if start_ro module parameter is set, array is >>>> @@ -1061,7 +1056,9 @@ int Create(struct supertype *st, char *mddev, >>>> map_remove(&map, fd2devnm(mdfd)); >>>> map_unlock(&map); >>>> - if (mdfd >= 0) >>>> + if (mdfd >= 0) { >>>> + ioctl(mdfd, STOP_ARRAY, NULL); >>>> close(mdfd); >>>> + } >>>> return 1; >>>> } >>> >> >> -- >> 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 >> > > -- > 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] 9+ messages in thread
* Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked 2017-05-08 1:50 ` Zhilong Liu @ 2017-05-08 17:54 ` Jes Sorensen 2017-05-11 13:01 ` Zhilong Liu 2017-05-31 10:25 ` Zhilong Liu 0 siblings, 2 replies; 9+ messages in thread From: Jes Sorensen @ 2017-05-08 17:54 UTC (permalink / raw) To: Zhilong Liu; +Cc: linux-raid On 05/07/2017 09:50 PM, Zhilong Liu wrote: > > > On 05/05/2017 11:31 AM, Liu Zhilong wrote: >> >> >> On 05/04/2017 10:54 PM, Jes Sorensen wrote: >>> On 05/04/2017 08:20 AM, Zhilong Liu wrote: >>>> Hi Jes, >>>> >>>> apply for review, this is a bug I ever encountered. >>> >>> Zhilong, >>> >>> Under what circumstances do you see this? >>> >> >> Issued the command: >> linux-g0sr:/home/test # ./mdadm -CR /dev/md0 -l1 -n2 -b internal >> /dev/loop[0-1] --size 63 >> ... ... ... >> mdadm: Given bitmap chunk size not supported. >> linux-g0sr:/home/test # ls /dev/md0 >> /dev/md0 >> linux-g0sr:/home/test # ls /sys/block/md0/md/ >> array_size bitmap component_size level metadata_version >> raid_disks reshape_position safe_mode_delay >> array_state chunk_size layout max_read_errors >> new_dev reshape_direction resync_start >> >> create_mddev() writes the devnm to >> /sys/module/md_mod/parameter/new_array, >> then in md.c, module_param_call() called the 'set' function of >> add_named_array(), >> md_alloc() init_and_add the kobject for devm, finally the devnm device >> has created >> and sysfs has registered after create_mddev executed successfully. >> Thus it's better >> to STOP_ARRAY in any case after create_mddev() invoked. >> > > this patch depends on the kernel commit: > 039b7225e6e9 ("md: allow creation of mdNNN arrays via > md_mod/parameters/new_array") > Neil's patch has set "mddev->hold_active = UNTIL_STOP", thus the > STOP_ARRAY can work > well on this situation. OK now I am confused - are you saying this change will only work after Neil's kernel patch has been applied? That would be no good, mdadm needs to work on older kernels too. Jes ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked 2017-05-08 17:54 ` Jes Sorensen @ 2017-05-11 13:01 ` Zhilong Liu 2017-05-31 10:25 ` Zhilong Liu 1 sibling, 0 replies; 9+ messages in thread From: Zhilong Liu @ 2017-05-11 13:01 UTC (permalink / raw) To: Jes Sorensen; +Cc: linux-raid On 05/09/2017 01:54 AM, Jes Sorensen wrote: > On 05/07/2017 09:50 PM, Zhilong Liu wrote: >> >> >> On 05/05/2017 11:31 AM, Liu Zhilong wrote: >>> >>> >>> On 05/04/2017 10:54 PM, Jes Sorensen wrote: >>>> On 05/04/2017 08:20 AM, Zhilong Liu wrote: >>>>> Hi Jes, >>>>> >>>>> apply for review, this is a bug I ever encountered. >>>> >>>> Zhilong, >>>> >>>> Under what circumstances do you see this? >>>> >>> >>> Issued the command: >>> linux-g0sr:/home/test # ./mdadm -CR /dev/md0 -l1 -n2 -b internal >>> /dev/loop[0-1] --size 63 >>> ... ... ... >>> mdadm: Given bitmap chunk size not supported. >>> linux-g0sr:/home/test # ls /dev/md0 >>> /dev/md0 >>> linux-g0sr:/home/test # ls /sys/block/md0/md/ >>> array_size bitmap component_size level metadata_version >>> raid_disks reshape_position safe_mode_delay >>> array_state chunk_size layout max_read_errors >>> new_dev reshape_direction resync_start >>> >>> create_mddev() writes the devnm to >>> /sys/module/md_mod/parameter/new_array, >>> then in md.c, module_param_call() called the 'set' function of >>> add_named_array(), >>> md_alloc() init_and_add the kobject for devm, finally the devnm device >>> has created >>> and sysfs has registered after create_mddev executed successfully. >>> Thus it's better >>> to STOP_ARRAY in any case after create_mddev() invoked. >>> >> >> this patch depends on the kernel commit: >> 039b7225e6e9 ("md: allow creation of mdNNN arrays via >> md_mod/parameters/new_array") >> Neil's patch has set "mddev->hold_active = UNTIL_STOP", thus the >> STOP_ARRAY can work >> well on this situation. > > OK now I am confused - are you saying this change will only work after > Neil's kernel patch has been applied? That would be no good, mdadm > needs to work on older kernels too. > Yes, I do agree your kindly reminding. In order to fix this scenario that exit the process abnormally but create_mddev() has invoked, so how about declaring one utility function to do STOP_ARRAY like following, it's a better way to stop the array if it opened by O_EXCL flag. cut the piece code from Manage_stop(): ... ... /* As we have an O_EXCL open, any use of the device * which blocks STOP_ARRAY is probably a transient use, * so it is reasonable to retry for a while - 5 seconds. */ count = 25; err = 0; while (count && fd >= 0 && (err = ioctl(fd, STOP_ARRAY, NULL)) < 0 && errno == EBUSY) { usleep(200000); count --; } ... ... Thanks, -Zhilong > Jes > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked 2017-05-08 17:54 ` Jes Sorensen 2017-05-11 13:01 ` Zhilong Liu @ 2017-05-31 10:25 ` Zhilong Liu 1 sibling, 0 replies; 9+ messages in thread From: Zhilong Liu @ 2017-05-31 10:25 UTC (permalink / raw) To: Jes Sorensen; +Cc: linux-raid On 05/09/2017 01:54 AM, Jes Sorensen wrote: > On 05/07/2017 09:50 PM, Zhilong Liu wrote: >> >> >> On 05/05/2017 11:31 AM, Liu Zhilong wrote: >>> >>> >>> On 05/04/2017 10:54 PM, Jes Sorensen wrote: >>>> On 05/04/2017 08:20 AM, Zhilong Liu wrote: >>>>> Hi Jes, >>>>> >>>>> apply for review, this is a bug I ever encountered. >>>> >>>> Zhilong, >>>> >>>> Under what circumstances do you see this? >>>> >>> >>> Issued the command: >>> linux-g0sr:/home/test # ./mdadm -CR /dev/md0 -l1 -n2 -b internal >>> /dev/loop[0-1] --size 63 >>> ... ... ... >>> mdadm: Given bitmap chunk size not supported. >>> linux-g0sr:/home/test # ls /dev/md0 >>> /dev/md0 >>> linux-g0sr:/home/test # ls /sys/block/md0/md/ >>> array_size bitmap component_size level metadata_version >>> raid_disks reshape_position safe_mode_delay >>> array_state chunk_size layout max_read_errors >>> new_dev reshape_direction resync_start >>> >>> create_mddev() writes the devnm to >>> /sys/module/md_mod/parameter/new_array, >>> then in md.c, module_param_call() called the 'set' function of >>> add_named_array(), >>> md_alloc() init_and_add the kobject for devm, finally the devnm device >>> has created >>> and sysfs has registered after create_mddev executed successfully. >>> Thus it's better >>> to STOP_ARRAY in any case after create_mddev() invoked. >>> >> >> this patch depends on the kernel commit: >> 039b7225e6e9 ("md: allow creation of mdNNN arrays via >> md_mod/parameters/new_array") >> Neil's patch has set "mddev->hold_active = UNTIL_STOP", thus the >> STOP_ARRAY can work >> well on this situation. > > OK now I am confused - are you saying this change will only work after > Neil's kernel patch has been applied? That would be no good, mdadm > needs to work on older kernels too. > Sorry for the late reply for this. Currently, the creating array method for later kernel(newer than v4.11), it can avoid the race problem via to set 'create_on_open' as N and write the mddev -> /sys/module/md_mod/parameters/new_array, then mdadm can send the ioctl to stop_array directly if creating array fail, it won't happen the race. refer to commit: 78b6350dcaad ("md: support disabling of create-on-open semantics.") And for older kernel, it's difficult to avoid the problem of the race when use 'change', 'add' and 'remove' udev rules in a short period of time via to ioctl commands. For example, this case tested on my latest Tumbleweed(20170524) with latest mdadm source. Steps: First part: - open one terminal to monitor the udev: Terminal 1: # udevadm monitor monitor will print the received events for: UDEV - the event which udev sends out after rule processing KERNEL - the kernel uevent - type the command in another terminal: Terminal 2: # ./mdadm -CR /dev/md1 --bitmap=internal -l1 -n2 /dev/loop1 /dev/loop2 --size 63 add_internal_bitmap received the abnormal chunk_size( < 64k), and return a failure. but it left the partially created array and didn't cleanup. For this test, the udev monitor prints: Terminal 1: # udevadm monitor ... ... KERNEL[146.077168] add /devices/virtual/bdi/9:1 (bdi) KERNEL[146.077211] add /devices/virtual/block/md1 (block) UDEV [146.078112] add /devices/virtual/bdi/9:1 (bdi) UDEV [146.084163] add /devices/virtual/block/md1 (block) Terminal 2: # ./mdadm -S /dev/md1 Terminal 1: # udevadm monitor KERNEL[153.276209] remove /devices/virtual/bdi/9:1 (bdi) KERNEL[153.276317] remove /devices/virtual/block/md1 (block) UDEV [153.277034] remove /devices/virtual/bdi/9:1 (bdi) UDEV [153.280801] remove /devices/virtual/block/md1 (block) Second part: add the ioctl(stop_array) and compiled to monitor the udevs. # git diff diff --git a/Create.c b/Create.c index 239545f..21568ca 100644 --- a/Create.c +++ b/Create.c @@ -1065,7 +1065,9 @@ int Create(struct supertype *st, char *mddev, map_remove(&map, fd2devnm(mdfd)); map_unlock(&map); - if (mdfd >= 0) + if (mdfd >= 0) { + ioctl(mdfd, STOP_ARRAY, NULL); close(mdfd); + } return 1; } Terminal 2: # ./mdadm -CR /dev/md1 --bitmap=internal -l1 -n2 /dev/loop1 /dev/loop2 --size 63 Terminal 1: # udevadm monitor ... ... KERNEL[171.964597] add /devices/virtual/bdi/9:1 (bdi) KERNEL[171.965346] add /devices/virtual/block/md1 (block) UDEV [171.965565] add /devices/virtual/bdi/9:1 (bdi) KERNEL[171.984195] remove /devices/virtual/bdi/9:1 (bdi) KERNEL[171.984555] remove /devices/virtual/block/md1 (block) UDEV [171.984590] remove /devices/virtual/bdi/9:1 (bdi) KERNEL[172.004504] add /devices/virtual/bdi/9:1 (bdi) KERNEL[172.004890] add /devices/virtual/block/md1 (block) UDEV [172.004923] add /devices/virtual/bdi/9:1 (bdi) UDEV [172.005787] add /devices/virtual/block/md1 (block) UDEV [172.009648] remove /devices/virtual/block/md1 (block) UDEV [172.013232] add /devices/virtual/block/md1 (block) So shall give udev a moment to process 'add' events? mdadm can work as expect when adding usleep(100 * 1000) before perform ioctl(STOP_ARRAY). this is the udev monitor result tested by the following sample. Terminal 1: # udevadm monitor KERNEL[5476.780692] add /devices/virtual/bdi/9:1 (bdi) KERNEL[5476.780976] add /devices/virtual/block/md1 (block) UDEV [5476.782355] add /devices/virtual/bdi/9:1 (bdi) UDEV [5476.786056] add /devices/virtual/block/md1 (block) KERNEL[5476.896255] remove /devices/virtual/bdi/9:1 (bdi) KERNEL[5476.896367] remove /devices/virtual/block/md1 (block) UDEV [5476.896895] remove /devices/virtual/bdi/9:1 (bdi) UDEV [5476.900752] remove /devices/virtual/block/md1 (block) Such as like this: diff --git a/Create.c b/Create.c index 239545f..a07ace8 100644 --- a/Create.c +++ b/Create.c @@ -902,10 +902,8 @@ int Create(struct supertype *st, char *mddev, remove_partitions(fd); if (st->ss->add_to_super(st, &inf->disk, fd, dv->devname, - dv->data_offset)) { - ioctl(mdfd, STOP_ARRAY, NULL); + dv->data_offset)) goto abort_locked; - } st->ss->getinfo_super(st, inf, NULL); safe_mode_delay = inf->safe_mode_delay; @@ -1006,7 +1004,6 @@ int Create(struct supertype *st, char *mddev, sysfs_set_safemode(&info, safe_mode_delay); if (err) { pr_err("failed to activate array.\n"); - ioctl(mdfd, STOP_ARRAY, NULL); goto abort; } } else if (c->readonly && @@ -1016,7 +1013,6 @@ int Create(struct supertype *st, char *mddev, "array_state", "readonly") < 0) { pr_err("Failed to start array: %s\n", strerror(errno)); - ioctl(mdfd, STOP_ARRAY, NULL); goto abort; } } else { @@ -1028,7 +1024,6 @@ int Create(struct supertype *st, char *mddev, if (info.array.chunk_size & (info.array.chunk_size-1)) { cont_err("Problem may be that chunk size is not a power of 2\n"); } - ioctl(mdfd, STOP_ARRAY, NULL); goto abort; } /* if start_ro module parameter is set, array is @@ -1065,7 +1060,11 @@ int Create(struct supertype *st, char *mddev, map_remove(&map, fd2devnm(mdfd)); map_unlock(&map); - if (mdfd >= 0) + if (mdfd >= 0) { + /* Give udev a moment to finish 'add' events. */ + usleep(100*1000); + ioctl(mdfd, STOP_ARRAY, NULL); close(mdfd); + } return 1; } Thanks, -Zhilong > Jes > > > ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-05-31 10:25 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-04-26 7:03 [mdadm PATCH] Create: move STOP_ARRAY to abort_locked Zhilong Liu 2017-05-04 12:20 ` Zhilong Liu 2017-05-04 14:54 ` Jes Sorensen 2017-05-04 17:42 ` Zhilong 2017-05-05 3:31 ` Liu Zhilong 2017-05-08 1:50 ` Zhilong Liu 2017-05-08 17:54 ` Jes Sorensen 2017-05-11 13:01 ` Zhilong Liu 2017-05-31 10:25 ` Zhilong Liu
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).