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@xxxxxxxx>
>>> ---
>>> 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:
signature.asc
Description: PGP signature
