* [PATCH v4] super1: fix sb->max_dev when adding a new disk in linear array
@ 2017-05-22 6:16 Lidong Zhong
2017-05-22 11:07 ` NeilBrown
0 siblings, 1 reply; 6+ messages in thread
From: Lidong Zhong @ 2017-05-22 6:16 UTC (permalink / raw)
To: linux-raid, neilb; +Cc: colyli, Jes.Sorensen
The value of sb->max_dev will always be increased by 1 when adding
a new disk in linear array. It causes an inconsistence between each
disk in the array and the "Array State" value of "mdadm --examine DISK"
is wrong. For example, when adding the first new disk into linear array
it will be:
Array State : RAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
('A' == active, '.' == missing, 'R' == replacing)
Adding the second disk into linear array it will be
Array State : .AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
('A' == active, '.' == missing, 'R' == replacing)
Signed-off-by: Lidong Zhong <lzhong@suse.com>
---
super1.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/super1.c b/super1.c
index 2fcb814..03cea72 100644
--- a/super1.c
+++ b/super1.c
@@ -1267,8 +1267,10 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
break;
sb->dev_number = __cpu_to_le32(i);
info->disk.number = i;
- if (max >= __le32_to_cpu(sb->max_dev))
+ if (i >= max) {
+ sb->dev_roles[max] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
sb->max_dev = __cpu_to_le32(max+1);
+ }
random_uuid(sb->device_uuid);
@@ -1293,9 +1295,14 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
}
}
} else if (strcmp(update, "linear-grow-update") == 0) {
+ unsigned int max = __le32_to_cpu(sb->max_dev);
sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
sb->dev_roles[info->disk.number] =
__cpu_to_le16(info->disk.raid_disk);
+ if (info->array.raid_disks >= max) {
+ sb->dev_roles[max] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
+ sb->max_dev = __cpu_to_le32(max+1);
+ }
} else if (strcmp(update, "resync") == 0) {
/* make sure resync happens */
sb->resync_offset = 0ULL;
--
2.12.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] super1: fix sb->max_dev when adding a new disk in linear array
2017-05-22 6:16 [PATCH v4] super1: fix sb->max_dev when adding a new disk in linear array Lidong Zhong
@ 2017-05-22 11:07 ` NeilBrown
2017-05-23 3:55 ` Lidong Zhong
0 siblings, 1 reply; 6+ messages in thread
From: NeilBrown @ 2017-05-22 11:07 UTC (permalink / raw)
To: Lidong Zhong, linux-raid; +Cc: colyli, Jes.Sorensen
[-- Attachment #1: Type: text/plain, Size: 2708 bytes --]
On Mon, May 22 2017, Lidong Zhong wrote:
> The value of sb->max_dev will always be increased by 1 when adding
> a new disk in linear array. It causes an inconsistence between each
> disk in the array and the "Array State" value of "mdadm --examine DISK"
> is wrong. For example, when adding the first new disk into linear array
> it will be:
>
> Array State : RAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
> ('A' == active, '.' == missing, 'R' == replacing)
>
> Adding the second disk into linear array it will be
>
> Array State : .AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
> ('A' == active, '.' == missing, 'R' == replacing)
>
> Signed-off-by: Lidong Zhong <lzhong@suse.com>
> ---
> super1.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/super1.c b/super1.c
> index 2fcb814..03cea72 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -1267,8 +1267,10 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
> break;
> sb->dev_number = __cpu_to_le32(i);
> info->disk.number = i;
> - if (max >= __le32_to_cpu(sb->max_dev))
> + if (i >= max) {
> + sb->dev_roles[max] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
Why do you assign to dev_roles[max]?
max must equal i here, and a few lines later:
sb->dev_roles[i] = __cpu_to_le16(info->disk.raid_disk);
your assignment is over-written. So it is pointless.
If i was greater than max (which should be impossible), you assignment
here would corrupt the dev_roles table.
Please drop this assignment.
> sb->max_dev = __cpu_to_le32(max+1);
> + }
>
> random_uuid(sb->device_uuid);
>
> @@ -1293,9 +1295,14 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
> }
> }
> } else if (strcmp(update, "linear-grow-update") == 0) {
> + unsigned int max = __le32_to_cpu(sb->max_dev);
> sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
> sb->dev_roles[info->disk.number] =
> __cpu_to_le16(info->disk.raid_disk);
> + if (info->array.raid_disks >= max) {
if raid_disks == max there is no need to change anything.
It is only when raid_disks > max that you need to increase max.
> + sb->dev_roles[max] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
When you increase max, you do need to assign MD_DISK_ROLE_SPARE to the
new element, but you need to do that *before* disk.raid_disk is
assigned, in case info->disk.number == max (as it could be for the
recently added device).
NeilBrown
> + sb->max_dev = __cpu_to_le32(max+1);
> + }
> } else if (strcmp(update, "resync") == 0) {
> /* make sure resync happens */
> sb->resync_offset = 0ULL;
> --
> 2.12.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] super1: fix sb->max_dev when adding a new disk in linear array
2017-05-22 11:07 ` NeilBrown
@ 2017-05-23 3:55 ` Lidong Zhong
2017-05-24 1:57 ` NeilBrown
0 siblings, 1 reply; 6+ messages in thread
From: Lidong Zhong @ 2017-05-23 3:55 UTC (permalink / raw)
To: NeilBrown, linux-raid; +Cc: colyli, Jes.Sorensen
On 05/22/2017 07:07 PM, NeilBrown wrote:
> On Mon, May 22 2017, Lidong Zhong wrote:
>
>> The value of sb->max_dev will always be increased by 1 when adding
>> a new disk in linear array. It causes an inconsistence between each
>> disk in the array and the "Array State" value of "mdadm --examine DISK"
>> is wrong. For example, when adding the first new disk into linear array
>> it will be:
>>
>> Array State : RAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
>> ('A' == active, '.' == missing, 'R' == replacing)
>>
>> Adding the second disk into linear array it will be
>>
>> Array State : .AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
>> ('A' == active, '.' == missing, 'R' == replacing)
>>
>> Signed-off-by: Lidong Zhong <lzhong@suse.com>
>> ---
>> super1.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/super1.c b/super1.c
>> index 2fcb814..03cea72 100644
>> --- a/super1.c
>> +++ b/super1.c
>> @@ -1267,8 +1267,10 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
>> break;
>> sb->dev_number = __cpu_to_le32(i);
>> info->disk.number = i;
>> - if (max >= __le32_to_cpu(sb->max_dev))
>> + if (i >= max) {
>> + sb->dev_roles[max] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
>
Hi Neil,
> Why do you assign to dev_roles[max]?
I meant to assure there will always be a spare spot in dev_roles[],
that is sb->max_dev at least is at lease 1 more than raid_disks.
Now I see what you mean in your reply to my last version patch.
> max must equal i here, and a few lines later:
> sb->dev_roles[i] = __cpu_to_le16(info->disk.raid_disk);
>
> your assignment is over-written. So it is pointless.
> If i was greater than max (which should be impossible), you assignment
> here would corrupt the dev_roles table.
>
> Please drop this assignment.
Yes, just increase the max_dev value is enough.
>
>> sb->max_dev = __cpu_to_le32(max+1);
>> + }
>>
>> random_uuid(sb->device_uuid);
>>
>> @@ -1293,9 +1295,14 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
>> }
>> }
>> } else if (strcmp(update, "linear-grow-update") == 0) {
>> + unsigned int max = __le32_to_cpu(sb->max_dev);
>> sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
>> sb->dev_roles[info->disk.number] =
>> __cpu_to_le16(info->disk.raid_disk);
>> + if (info->array.raid_disks >= max) {
>
> if raid_disks == max there is no need to change anything.
> It is only when raid_disks > max that you need to increase max.
>
Yes, the max_dev should only be updated when raid_disks > max.
>> + sb->dev_roles[max] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
>
> When you increase max, you do need to assign MD_DISK_ROLE_SPARE to the
> new element, but you need to do that *before* disk.raid_disk is
> assigned, in case info->disk.number == max (as it could be for the
> recently added device).
>
I think it's also pointless to assign MD_DISK_ROLE_SPARE
since there is no SPARE in dev_roles when we need to update
sb->max_dev. The newly added device will not meet the condition
as max_dev has already been updated, that's saying, we only
need to update the max_dev value for original disks.
The following code should work
1297 } else if (strcmp(update, "linear-grow-update") == 0) {
1298 unsigned int max = __le32_to_cpu(sb->max_dev);
1299 sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
1300 sb->dev_roles[info->disk.number] =
1301 __cpu_to_le16(info->disk.raid_disk);
1302 if (info->array.raid_disks > max) {
1303 sb->max_dev = __cpu_to_le32(max+1);
1304 }
Thank you for your patient review.
Lidong
> NeilBrown
>
>
>> + sb->max_dev = __cpu_to_le32(max+1);
>> + }
>> } else if (strcmp(update, "resync") == 0) {
>> /* make sure resync happens */
>> sb->resync_offset = 0ULL;
>> --
>> 2.12.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] super1: fix sb->max_dev when adding a new disk in linear array
2017-05-23 3:55 ` Lidong Zhong
@ 2017-05-24 1:57 ` NeilBrown
2017-05-24 9:24 ` Lidong Zhong
0 siblings, 1 reply; 6+ messages in thread
From: NeilBrown @ 2017-05-24 1:57 UTC (permalink / raw)
To: Lidong Zhong, linux-raid; +Cc: colyli, Jes.Sorensen
[-- Attachment #1: Type: text/plain, Size: 4558 bytes --]
On Tue, May 23 2017, Lidong Zhong wrote:
> On 05/22/2017 07:07 PM, NeilBrown wrote:
>> On Mon, May 22 2017, Lidong Zhong wrote:
>>
>>> The value of sb->max_dev will always be increased by 1 when adding
>>> a new disk in linear array. It causes an inconsistence between each
>>> disk in the array and the "Array State" value of "mdadm --examine DISK"
>>> is wrong. For example, when adding the first new disk into linear array
>>> it will be:
>>>
>>> Array State : RAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
>>> ('A' == active, '.' == missing, 'R' == replacing)
>>>
>>> Adding the second disk into linear array it will be
>>>
>>> Array State : .AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
>>> ('A' == active, '.' == missing, 'R' == replacing)
>>>
>>> Signed-off-by: Lidong Zhong <lzhong@suse.com>
>>> ---
>>> super1.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/super1.c b/super1.c
>>> index 2fcb814..03cea72 100644
>>> --- a/super1.c
>>> +++ b/super1.c
>>> @@ -1267,8 +1267,10 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
>>> break;
>>> sb->dev_number = __cpu_to_le32(i);
>>> info->disk.number = i;
>>> - if (max >= __le32_to_cpu(sb->max_dev))
>>> + if (i >= max) {
>>> + sb->dev_roles[max] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
>>
>
> Hi Neil,
>
>> Why do you assign to dev_roles[max]?
>
> I meant to assure there will always be a spare spot in dev_roles[],
> that is sb->max_dev at least is at lease 1 more than raid_disks.
> Now I see what you mean in your reply to my last version patch.
>
>> max must equal i here, and a few lines later:
>> sb->dev_roles[i] = __cpu_to_le16(info->disk.raid_disk);
>>
>> your assignment is over-written. So it is pointless.
>> If i was greater than max (which should be impossible), you assignment
>> here would corrupt the dev_roles table.
>>
>> Please drop this assignment.
>
> Yes, just increase the max_dev value is enough.
>
>>
>>> sb->max_dev = __cpu_to_le32(max+1);
>>> + }
>>>
>>> random_uuid(sb->device_uuid);
>>>
>>> @@ -1293,9 +1295,14 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
>>> }
>>> }
>>> } else if (strcmp(update, "linear-grow-update") == 0) {
>>> + unsigned int max = __le32_to_cpu(sb->max_dev);
>>> sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
>>> sb->dev_roles[info->disk.number] =
>>> __cpu_to_le16(info->disk.raid_disk);
>>> + if (info->array.raid_disks >= max) {
>>
>> if raid_disks == max there is no need to change anything.
>> It is only when raid_disks > max that you need to increase max.
>>
>
> Yes, the max_dev should only be updated when raid_disks > max.
>
>>> + sb->dev_roles[max] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
>>
>> When you increase max, you do need to assign MD_DISK_ROLE_SPARE to the
>> new element, but you need to do that *before* disk.raid_disk is
>> assigned, in case info->disk.number == max (as it could be for the
>> recently added device).
>>
> I think it's also pointless to assign MD_DISK_ROLE_SPARE
> since there is no SPARE in dev_roles when we need to update
> sb->max_dev. The newly added device will not meet the condition
> as max_dev has already been updated, that's saying, we only
> need to update the max_dev value for original disks.
> The following code should work
>
> 1297 } else if (strcmp(update, "linear-grow-update") == 0) {
> 1298 unsigned int max = __le32_to_cpu(sb->max_dev);
> 1299 sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
> 1300 sb->dev_roles[info->disk.number] =
> 1301 __cpu_to_le16(info->disk.raid_disk);
> 1302 if (info->array.raid_disks > max) {
>
>
> 1303 sb->max_dev = __cpu_to_le32(max+1);
> 1304 }
Increasing max_dev and not initializing will leave the last entry in
dev_roles[] uninitialised. That isn't good.
MD_DISK_ROLE_SPARE doesn't mean there is a spare device in that slot.
It means that if there is a device in that slot, it must be spare.
If you leave it uninitialised, it will probably be zero, and then
you will get "?" in the mdadm output again.
NeilBrown
>
> Thank you for your patient review.
>
> Lidong
>
>> NeilBrown
>>
>>
>>> + sb->max_dev = __cpu_to_le32(max+1);
>>> + }
>>> } else if (strcmp(update, "resync") == 0) {
>>> /* make sure resync happens */
>>> sb->resync_offset = 0ULL;
>>> --
>>> 2.12.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] super1: fix sb->max_dev when adding a new disk in linear array
2017-05-24 1:57 ` NeilBrown
@ 2017-05-24 9:24 ` Lidong Zhong
2017-05-25 0:32 ` NeilBrown
0 siblings, 1 reply; 6+ messages in thread
From: Lidong Zhong @ 2017-05-24 9:24 UTC (permalink / raw)
To: NeilBrown, linux-raid; +Cc: colyli, Jes.Sorensen
On 05/24/2017 09:57 AM, NeilBrown wrote:
>>>
>> I think it's also pointless to assign MD_DISK_ROLE_SPARE
>> since there is no SPARE in dev_roles when we need to update
>> sb->max_dev. The newly added device will not meet the condition
>> as max_dev has already been updated, that's saying, we only
>> need to update the max_dev value for original disks.
>> The following code should work
>>
>> 1297 } else if (strcmp(update, "linear-grow-update") == 0) {
>> 1298 unsigned int max = __le32_to_cpu(sb->max_dev);
>> 1299 sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
>> 1300 sb->dev_roles[info->disk.number] =
>> 1301 __cpu_to_le16(info->disk.raid_disk);
>> 1302 if (info->array.raid_disks > max) {
>>
>>
>> 1303 sb->max_dev = __cpu_to_le32(max+1);
>> 1304 }
>
> Increasing max_dev and not initializing will leave the last entry in
> dev_roles[] uninitialised. That isn't good.
>
Hi Neil,
As I can see, the dev_roles[] value has already been set by line 1300,
because only one disk could be added to the linear array at one time.
When info->array.raid_disks is large than max,
info->disk.number should be equal to max now.
If changing the source into
1297 } else if (strcmp(update, "linear-grow-update") == 0) {
1298 unsigned int max = __le32_to_cpu(sb->max_dev);
1299 sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
1300 if (info->array.raid_disks > max) {
1301 sb->dev_roles[max] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
1302 sb->max_dev = __cpu_to_le32(max+1);
1303 }
1304 sb->dev_roles[info->disk.number] =
1305 __cpu_to_le16(info->disk.raid_disk);
it still works, but I don't see it as necessary.
> MD_DISK_ROLE_SPARE doesn't mean there is a spare device in that slot.
> It means that if there is a device in that slot, it must be spare.
> If you leave it uninitialised, it will probably be zero, and then
> you will get "?" in the mdadm output again.
>
I did a test with growing a linear array to 129 devices
/dev/dm-128:
Magic : a92b4efc
Version : 1.2
Feature Map : 0x0
Array UUID : 8bed7e5c:1acea7d9:9087c183:77f44fc0
Name : sles12sp2-clone1:0 (local to host sles12sp2-clone1)
Creation Time : Wed May 24 16:56:10 2017
Raid Level : linear
Raid Devices : 129
Avail Dev Size : 106400 (51.95 MiB 54.48 MB)
Used Dev Size : 0
Data Offset : 96 sectors
Super Offset : 8 sectors
Unused Space : before=8 sectors, after=0 sectors
State : clean
Device UUID : 47dec6cb:e84ddc52:35f7290b:7c47a1c6
Update Time : Wed May 24 16:56:10 2017
Bad Block Log : 512 entries available at offset 72 sectors
Checksum : 57c7a375 - correct
Events : 0
Rounding : 0K
Device Role : Active device 128
Array State :
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
('A' == active, '.' == missing, 'R' == replacing)
Debug Array State (raid_disks 129, delta_extra 0) :
dev_roles[0]:0 dev_roles[1]:1 dev_roles[2]:2 dev_roles[3]:3
dev_roles[4]:4 dev_roles[5]:5 dev_roles[6]:6 dev_roles[7]:7
dev_roles[8]:8 dev_roles[9]:9 dev_roles[10]:a dev_roles[11]:b
dev_roles[12]:c dev_roles[13]:d dev_roles[14]:e dev_roles[15]:f
dev_roles[16]:10 dev_roles[17]:11 dev_roles[18]:12 dev_roles[19]:13
dev_roles[20]:14 dev_roles[21]:15 dev_roles[22]:16 dev_roles[23]:17
dev_roles[24]:18 dev_roles[25]:19 dev_roles[26]:1a dev_roles[27]:1b
dev_roles[28]:1c dev_roles[29]:1d dev_roles[30]:1e dev_roles[31]:1f
dev_roles[32]:20 dev_roles[33]:21 dev_roles[34]:22 dev_roles[35]:23
dev_roles[36]:24 dev_roles[37]:25 dev_roles[38]:26 dev_roles[39]:27
dev_roles[40]:28 dev_roles[41]:29 dev_roles[42]:2a dev_roles[43]:2b
dev_roles[44]:2c dev_roles[45]:2d dev_roles[46]:2e dev_roles[47]:2f
dev_roles[48]:30 dev_roles[49]:31 dev_roles[50]:32 dev_roles[51]:33
dev_roles[52]:34 dev_roles[53]:35 dev_roles[54]:36 dev_roles[55]:37
dev_roles[56]:38 dev_roles[57]:39 dev_roles[58]:3a dev_roles[59]:3b
dev_roles[60]:3c dev_roles[61]:3d dev_roles[62]:3e dev_roles[63]:3f
dev_roles[64]:40 dev_roles[65]:41 dev_roles[66]:42 dev_roles[67]:43
dev_roles[68]:44 dev_roles[69]:45 dev_roles[70]:46 dev_roles[71]:47
dev_roles[72]:48 dev_roles[73]:49 dev_roles[74]:4a dev_roles[75]:4b
dev_roles[76]:4c dev_roles[77]:4d dev_roles[78]:4e dev_roles[79]:4f
dev_roles[80]:50 dev_roles[81]:51 dev_roles[82]:52 dev_roles[83]:53
dev_roles[84]:54 dev_roles[85]:55 dev_roles[86]:56 dev_roles[87]:57
dev_roles[88]:58 dev_roles[89]:59 dev_roles[90]:5a dev_roles[91]:5b
dev_roles[92]:5c dev_roles[93]:5d dev_roles[94]:5e dev_roles[95]:5f
dev_roles[96]:60 dev_roles[97]:61 dev_roles[98]:62 dev_roles[99]:63
dev_roles[100]:64 dev_roles[101]:65 dev_roles[102]:66 dev_roles[103]:67
dev_roles[104]:68 dev_roles[105]:69 dev_roles[106]:6a dev_roles[107]:6b
dev_roles[108]:6c dev_roles[109]:6d dev_roles[110]:6e dev_roles[111]:6f
dev_roles[112]:70 dev_roles[113]:71 dev_roles[114]:72 dev_roles[115]:73
dev_roles[116]:74 dev_roles[117]:75 dev_roles[118]:76 dev_roles[119]:77
dev_roles[120]:78 dev_roles[121]:79 dev_roles[122]:7a dev_roles[123]:7b
dev_roles[124]:7c dev_roles[125]:7d dev_roles[126]:7e dev_roles[127]:7f
dev_roles[128]:80
And there is no problem showing the status.
Thanks,
Lidong
> NeilBrown
>
>
>>
>> Thank you for your patient review.
>>
>> Lidong
>>
>>> NeilBrown
>>>
>>>
>>>> + sb->max_dev = __cpu_to_le32(max+1);
>>>> + }
>>>> } else if (strcmp(update, "resync") == 0) {
>>>> /* make sure resync happens */
>>>> sb->resync_offset = 0ULL;
>>>> --
>>>> 2.12.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] super1: fix sb->max_dev when adding a new disk in linear array
2017-05-24 9:24 ` Lidong Zhong
@ 2017-05-25 0:32 ` NeilBrown
0 siblings, 0 replies; 6+ messages in thread
From: NeilBrown @ 2017-05-25 0:32 UTC (permalink / raw)
To: Lidong Zhong, linux-raid; +Cc: colyli, Jes.Sorensen
[-- Attachment #1: Type: text/plain, Size: 6407 bytes --]
On Wed, May 24 2017, Lidong Zhong wrote:
> On 05/24/2017 09:57 AM, NeilBrown wrote:
>>>>
>>> I think it's also pointless to assign MD_DISK_ROLE_SPARE
>>> since there is no SPARE in dev_roles when we need to update
>>> sb->max_dev. The newly added device will not meet the condition
>>> as max_dev has already been updated, that's saying, we only
>>> need to update the max_dev value for original disks.
>>> The following code should work
>>>
>>> 1297 } else if (strcmp(update, "linear-grow-update") == 0) {
>>> 1298 unsigned int max = __le32_to_cpu(sb->max_dev);
>>> 1299 sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
>>> 1300 sb->dev_roles[info->disk.number] =
>>> 1301 __cpu_to_le16(info->disk.raid_disk);
>>> 1302 if (info->array.raid_disks > max) {
>>>
>>>
>>> 1303 sb->max_dev = __cpu_to_le32(max+1);
>>> 1304 }
>>
>> Increasing max_dev and not initializing will leave the last entry in
>> dev_roles[] uninitialised. That isn't good.
>>
> Hi Neil,
>
> As I can see, the dev_roles[] value has already been set by line 1300,
> because only one disk could be added to the linear array at one time.
> When info->array.raid_disks is large than max,
> info->disk.number should be equal to max now.
> If changing the source into
>
> 1297 } else if (strcmp(update, "linear-grow-update") == 0) {
> 1298 unsigned int max = __le32_to_cpu(sb->max_dev);
> 1299 sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
> 1300 if (info->array.raid_disks > max) {
> 1301 sb->dev_roles[max] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
> 1302 sb->max_dev = __cpu_to_le32(max+1);
> 1303 }
> 1304 sb->dev_roles[info->disk.number] =
>
>
> 1305 __cpu_to_le16(info->disk.raid_disk);
>
> it still works, but I don't see it as necessary.
Yes, you are right, sorry. My mistake.
No need to change dev_roles in linear-grow-update other than
info->disk.number.
Only changed needed there is to make sure max_dev is correct.
>> MD_DISK_ROLE_SPARE doesn't mean there is a spare device in that slot.
>> It means that if there is a device in that slot, it must be spare.
>> If you leave it uninitialised, it will probably be zero, and then
>> you will get "?" in the mdadm output again.
>>
>
> I did a test with growing a linear array to 129 devices
Thanks for doing that!! Testing like this helps confirm our understanding.
Thanks,
NeilBrown
>
> /dev/dm-128:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x0
> Array UUID : 8bed7e5c:1acea7d9:9087c183:77f44fc0
> Name : sles12sp2-clone1:0 (local to host sles12sp2-clone1)
> Creation Time : Wed May 24 16:56:10 2017
> Raid Level : linear
> Raid Devices : 129
>
> Avail Dev Size : 106400 (51.95 MiB 54.48 MB)
> Used Dev Size : 0
> Data Offset : 96 sectors
> Super Offset : 8 sectors
> Unused Space : before=8 sectors, after=0 sectors
> State : clean
> Device UUID : 47dec6cb:e84ddc52:35f7290b:7c47a1c6
>
> Update Time : Wed May 24 16:56:10 2017
> Bad Block Log : 512 entries available at offset 72 sectors
> Checksum : 57c7a375 - correct
> Events : 0
>
> Rounding : 0K
>
> Device Role : Active device 128
> Array State :
> AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
> ('A' == active, '.' == missing, 'R' == replacing)
> Debug Array State (raid_disks 129, delta_extra 0) :
> dev_roles[0]:0 dev_roles[1]:1 dev_roles[2]:2 dev_roles[3]:3
> dev_roles[4]:4 dev_roles[5]:5 dev_roles[6]:6 dev_roles[7]:7
> dev_roles[8]:8 dev_roles[9]:9 dev_roles[10]:a dev_roles[11]:b
> dev_roles[12]:c dev_roles[13]:d dev_roles[14]:e dev_roles[15]:f
> dev_roles[16]:10 dev_roles[17]:11 dev_roles[18]:12 dev_roles[19]:13
> dev_roles[20]:14 dev_roles[21]:15 dev_roles[22]:16 dev_roles[23]:17
> dev_roles[24]:18 dev_roles[25]:19 dev_roles[26]:1a dev_roles[27]:1b
> dev_roles[28]:1c dev_roles[29]:1d dev_roles[30]:1e dev_roles[31]:1f
> dev_roles[32]:20 dev_roles[33]:21 dev_roles[34]:22 dev_roles[35]:23
> dev_roles[36]:24 dev_roles[37]:25 dev_roles[38]:26 dev_roles[39]:27
> dev_roles[40]:28 dev_roles[41]:29 dev_roles[42]:2a dev_roles[43]:2b
> dev_roles[44]:2c dev_roles[45]:2d dev_roles[46]:2e dev_roles[47]:2f
> dev_roles[48]:30 dev_roles[49]:31 dev_roles[50]:32 dev_roles[51]:33
> dev_roles[52]:34 dev_roles[53]:35 dev_roles[54]:36 dev_roles[55]:37
> dev_roles[56]:38 dev_roles[57]:39 dev_roles[58]:3a dev_roles[59]:3b
> dev_roles[60]:3c dev_roles[61]:3d dev_roles[62]:3e dev_roles[63]:3f
> dev_roles[64]:40 dev_roles[65]:41 dev_roles[66]:42 dev_roles[67]:43
> dev_roles[68]:44 dev_roles[69]:45 dev_roles[70]:46 dev_roles[71]:47
> dev_roles[72]:48 dev_roles[73]:49 dev_roles[74]:4a dev_roles[75]:4b
> dev_roles[76]:4c dev_roles[77]:4d dev_roles[78]:4e dev_roles[79]:4f
> dev_roles[80]:50 dev_roles[81]:51 dev_roles[82]:52 dev_roles[83]:53
> dev_roles[84]:54 dev_roles[85]:55 dev_roles[86]:56 dev_roles[87]:57
> dev_roles[88]:58 dev_roles[89]:59 dev_roles[90]:5a dev_roles[91]:5b
> dev_roles[92]:5c dev_roles[93]:5d dev_roles[94]:5e dev_roles[95]:5f
> dev_roles[96]:60 dev_roles[97]:61 dev_roles[98]:62 dev_roles[99]:63
> dev_roles[100]:64 dev_roles[101]:65 dev_roles[102]:66 dev_roles[103]:67
> dev_roles[104]:68 dev_roles[105]:69 dev_roles[106]:6a dev_roles[107]:6b
> dev_roles[108]:6c dev_roles[109]:6d dev_roles[110]:6e dev_roles[111]:6f
> dev_roles[112]:70 dev_roles[113]:71 dev_roles[114]:72 dev_roles[115]:73
> dev_roles[116]:74 dev_roles[117]:75 dev_roles[118]:76 dev_roles[119]:77
> dev_roles[120]:78 dev_roles[121]:79 dev_roles[122]:7a dev_roles[123]:7b
> dev_roles[124]:7c dev_roles[125]:7d dev_roles[126]:7e dev_roles[127]:7f
> dev_roles[128]:80
>
> And there is no problem showing the status.
>
> Thanks,
> Lidong
>> NeilBrown
>>
>>
>>>
>>> Thank you for your patient review.
>>>
>>> Lidong
>>>
>>>> NeilBrown
>>>>
>>>>
>>>>> + sb->max_dev = __cpu_to_le32(max+1);
>>>>> + }
>>>>> } else if (strcmp(update, "resync") == 0) {
>>>>> /* make sure resync happens */
>>>>> sb->resync_offset = 0ULL;
>>>>> --
>>>>> 2.12.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-25 0:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-22 6:16 [PATCH v4] super1: fix sb->max_dev when adding a new disk in linear array Lidong Zhong
2017-05-22 11:07 ` NeilBrown
2017-05-23 3:55 ` Lidong Zhong
2017-05-24 1:57 ` NeilBrown
2017-05-24 9:24 ` Lidong Zhong
2017-05-25 0:32 ` NeilBrown
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.