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 }
...
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
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
