Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked

[Zhilong Liu <zlliu@suse.com>]

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
>