* Re: A sector-of-mismatch warning patch (was Re: Fault tolerance with badblocks)
From: NeilBrown @ 2017-05-21 22:00 UTC (permalink / raw)
To: Shaohua Li, Nix
Cc: Chris Murphy, David Brown, Anthony Youngman, Phil Turmel,
Ravi (Tom) Hale, Linux-RAID
In-Reply-To: <20170519165517.257ny67pxkcbtpkq@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 3081 bytes --]
On Fri, May 19 2017, Shaohua Li wrote:
> On Fri, May 19, 2017 at 11:32:43AM +0100, Nix wrote:
>> On 19 May 2017, NeilBrown said:
>>
>> > On Tue, May 16 2017, Nix wrote:
>> >
>> >> On 16 May 2017, NeilBrown spake thusly:
>> >>
>> >>> Actually, I have another caveat. I don't think we want these messages
>> >>> during initial resync, or any resync. Only during a 'check' or
>> >>> 'repair'.
>> >>> So add a check for MD_RECOVERY_REQUESTED or maybe for
>> >>> sh->sectors >= conf->mddev->recovery_cp
>> >>
>> >> I completely agree, but it's already inside MD_RECOVERY_CHECK:
>> >>
>> >> if (test_bit(MD_RECOVERY_CHECK, &conf->mddev->recovery)) {
>> >> /* don't try to repair!! */
>> >> set_bit(STRIPE_INSYNC, &sh->state);
>> >> pr_warn_ratelimited("%s: mismatch sector in range "
>> >> "%llu-%llu\n", mdname(conf->mddev),
>> >> (unsigned long long) sh->sector,
>> >> (unsigned long long) sh->sector +
>> >> STRIPE_SECTORS);
>> >> } else {
>> >>
>> >> Doesn't that already mean that someone has explicitly triggered a check
>> >> action?
>> >
>> > Uhmm... yeah. I lose track of which flags me what exactly.
>> > You log messages aren't generated when 'repair' is used, only when
>> > 'check' is.
>> > I can see why you might have chosen that, but I wonder if it is best.
>>
>> I'm not sure what the point is of being told when repair is used: hey,
>> there was an inconsistency here but there isn't any more! I suppose you
>> could still use it to see if the repair did the right thing. My problem
>> on that front was that I'm not sure what flag should be used to catch
>> repair but not resync etc: everywhere else in the code, repair is in an
>> unadorned else branch... is it the *lack* of MD_RECOVERY_CHECK and the
>> presence of, uh, something else?
> MD_RECOVERY_SYNC && MD_RECOVERY_REQUESTED && MD_RECOVERY_CHECK == check
> MD_RECOVERY_SYNC && MD_RECOVERY_REQUESTED == repair
> MD_RECOVERY_SYNC && !MD_RECOVERY_REQUESTED == resync
>
> Don't see the poin to print the info for 'repair'. 'repair' already changes the
> data, how could we use the info?
Surprising data is can be potentially valuable.
I don't think you should *ever* get an inconsistency in a RAID6 unless
you have faulty hardware.
If you do, then any information about the nature of the inconsistency
might be valuable in understanding the hardware fault.
I don't know in advance how I would interpret the data, but I do
know that if I didn't have the data, then I wouldn't be able to
interpret it.
However .... running "repair" when you don't know exactly what has
happened and why, is probably a bad idea. So logging probably won't
provide value.
I wouldn't go out of my way to add extra logging for the 'repair' case,
but I certainly wouldn't go out of my way to avoid logging in that case.
It seems inconsistent to log for 'check' but not 'repair', but it isn't
a big deal for me.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Fwd: Re: Promise Technology FakeRAID
From: jan.weber @ 2017-05-21 22:12 UTC (permalink / raw)
To: linux-raid
As I was recomended to send my question to this address, I do:
> Re From: NeilBrown <neilb@suse.com>
>
On Mon, May 15 2017, jan.weber@volny.cz wrote:
> Sorry to interrupt you.
I'm sorry, but I don't provide private consulting for free.
Please post your question to linux-raid@vger.kernel.org
(you will need to make sure the email only contains text, no html
component).
NeilBrown
>
> I need to assemble and get data from FakeRAID 5 originaly build on Windows 7 64 OS form 4 disk.
> But the system failed, and I have to save the data. I cannot start the array neither with mdadm neither the dmraid.
> The FakeRAID is managed by AMD RAID controller:
>
> 00:11.0 RAID bus controller: Advanced Micro Devices, Inc. [AMD/ATI] SB7x0/SB8x0/SB9x0 SATA Controller [RAID5 mode] (rev 40)
> Subsystem: ASRock Incorporation SB7x0/SB8x0/SB9x0 SATA Controller [RAID5 mode]
> Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
> Status: Cap+ 66MHz+ UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
> Latency: 32
> Interrupt: pin A routed to IRQ 19
> NUMA node: 0
> Region 0: I/O ports at f040 [size=8]
> Region 1: I/O ports at f030 [size=4]
> Region 2: I/O ports at f020 [size=8]
> Region 3: I/O ports at f010 [size=4]
> Region 4: I/O ports at f000 [size=16]
> Region 5: Memory at feb0b000 (32-bit, non-prefetchable) [size=1K]
> Capabilities: [70] SATA HBA v1.0 InCfgSpace
> Capabilities: [a4] PCI Advanced Features
> AFCap: TP+ FLR+
> AFCtrl: FLR-
> AFStatus: TP-
> Kernel driver in use: ahci
> Kernel modules: ahci
>
> The mdadm --examine /dev/sda [bcd] gives nothing,
> but on the disks themselves, there is a 32256 byte area before the end of each disk
> with some kind of “metadata” saying that the system is Promise Technology:
>
> /dev/sda
> 2baa146e200 50 72 6f 6d 69 73 65 20 54 65 63 68 6e 6f 6c 6f |Promise Technolo|
> 2baa146e210 67 79 2c 20 49 6e 63 2e 00 00 02 00 87 a1 58 58 |gy, Inc.......XX|
> 2baa146e220 41 f6 00 00 87 a1 58 58 41 f6 01 01 00 d3 d2 d1 |A.....XXA.......|
> 2baa146e230 d0 cf ce cd cc cb ca c9 c8 c7 c6 c5 c4 c3 c2 c1 |................|
> 2baa146e240 c0 bf be bd bc bb ba b9 b8 b7 b6 b5 b4 b3 b2 b1 |................|
> 2baa146e250 b0 af ae ad ac ab aa a9 a8 a7 a6 a5 a4 a3 a2 a1 |................|
> /dev/sdb
> 2baa146e200 50 72 6f 6d 69 73 65 20 54 65 63 68 6e 6f 6c 6f |Promise Technolo|
> 2baa146e210 67 79 2c 20 49 6e 63 2e 00 00 02 00 87 a1 58 58 |gy, Inc.......XX|
> 2baa146e220 34 f8 01 00 87 a1 58 58 34 f8 01 01 00 d3 d2 d1 |4.....XX4.......|
> 2baa146e230 d0 cf ce cd cc cb ca c9 c8 c7 c6 c5 c4 c3 c2 c1 |................|
> 2baa146e240 c0 bf be bd bc bb ba b9 b8 b7 b6 b5 b4 b3 b2 b1 |................|
> 2baa146e250 b0 af ae ad ac ab aa a9 a8 a7 a6 a5 a4 a3 a2 a1 |................|
> /dev/sdc
> 2baa146e200 50 72 6f 6d 69 73 65 20 54 65 63 68 6e 6f 6c 6f |Promise Technolo|
> 2baa146e210 67 79 2c 20 49 6e 63 2e 00 00 02 00 88 a1 58 58 |gy, Inc.......XX|
> 2baa146e220 46 fa 02 00 88 a1 58 58 46 fa 01 01 00 d3 d2 d1 |F.....XXF.......|
> 2baa146e230 d0 cf ce cd cc cb ca c9 c8 c7 c6 c5 c4 c3 c2 c1 |................|
> 2baa146e240 c0 bf be bd bc bb ba b9 b8 b7 b6 b5 b4 b3 b2 b1 |................|
> 2baa146e250 b0 af ae ad ac ab aa a9 a8 a7 a6 a5 a4 a3 a2 a1 |................|
> /dev/sdd
> 2baa146e200 50 72 6f 6d 69 73 65 20 54 65 63 68 6e 6f 6c 6f |Promise Technolo|
> 2baa146e210 67 79 2c 20 49 6e 63 2e 00 00 02 00 88 a1 58 58 |gy, Inc.......XX|
> 2baa146e220 59 fc 03 00 88 a1 58 58 59 fc 01 01 00 d3 d2 d1 |Y.....XXY.......|
> 2baa146e230 d0 cf ce cd cc cb ca c9 c8 c7 c6 c5 c4 c3 c2 c1 |................|
> 2baa146e240 c0 bf be bd bc bb ba b9 b8 b7 b6 b5 b4 b3 b2 b1 |................|
> 2baa146e250 b0 af ae ad ac ab aa a9 a8 a7 a6 a5 a4 a3 a2 a1 |................|
>
>
> I have also found in that area the name of the array : “pole” :
>
> /dev/sda
> 2baa146e440 07 02 01 00 25 01 f8 00 00 00 01 00 07 04 02 00 |....%...........|
> 2baa146e450 25 01 f9 00 00 00 02 00 07 06 03 00 25 01 fa 00 |%...........%...|
> 2baa146e460 00 00 03 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> 2baa146e470 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 2baa146e490 00 00 00 00 70 6f 6c 65 00 00 00 00 00 00 00 00 |....pole........|
> 2baa146e4a0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 2baa146e4d0 00 00 00 00 00 00 00 03 ff ff ff ff ff ff ff ff |................|
> 2baa146e4e0 01 01 01 00 00 00 01 00 04 00 00 00 00 08 00 00 |................|
> 2baa146e4f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> /dev/sdb
> 2baa146e440 07 02 01 00 25 01 f8 00 00 00 01 00 07 04 02 00 |....%...........|
> 2baa146e450 25 01 f9 00 00 00 02 00 07 06 03 00 25 01 fa 00 |%...........%...|
> 2baa146e460 00 00 03 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> 2baa146e470 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 2baa146e490 00 00 00 00 70 6f 6c 65 00 00 00 00 00 00 00 00 |....pole........|
> 2baa146e4a0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 2baa146e4d0 00 00 00 00 00 00 00 03 ff ff ff ff ff ff ff ff |................|
> 2baa146e4e0 01 01 01 00 00 00 01 00 04 00 00 00 00 08 00 00 |................|
> 2baa146e4f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> /dev/sdc
> 2baa146e440 07 02 01 00 25 01 f8 00 00 00 01 00 07 04 02 00 |....%...........|
> 2baa146e450 25 01 f9 00 00 00 02 00 07 06 03 00 25 01 fa 00 |%...........%...|
> 2baa146e460 00 00 03 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> 2baa146e470 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 2baa146e490 00 00 00 00 70 6f 6c 65 00 00 00 00 00 00 00 00 |....pole........|
> 2baa146e4a0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 2baa146e4d0 00 00 00 00 00 00 00 03 ff ff ff ff ff ff ff ff |................|
> 2baa146e4e0 01 01 01 00 00 00 01 00 04 00 00 00 00 08 00 00 |................|
> 2baa146e4f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> /dev/sdd
> 2baa146e440 07 02 01 00 25 01 f8 00 00 00 01 00 07 04 02 00 |....%...........|
> 2baa146e450 25 01 f9 00 00 00 02 00 07 06 03 00 25 01 fa 00 |%...........%...|
> 2baa146e460 00 00 03 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> 2baa146e470 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 2baa146e490 00 00 00 00 70 6f 6c 65 00 00 00 00 00 00 00 00 |....pole........|
> 2baa146e4a0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 2baa146e4d0 00 00 00 00 00 00 00 03 ff ff ff ff ff ff ff ff |................|
> 2baa146e4e0 01 01 01 00 00 00 01 00 04 00 00 00 00 08 00 00 |................|
> 2baa146e4f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>
>
> Please, can be the mdadm forced to read the metadata form indicated area, or how can be this kind of FakeRAID accesed?
>
> I was sucessful with various kind of “unrecognized” arrays telling the mdadm the guesed configuration
> included the chunk size and raid level. But with this FakeRAID I can do nothing.
>
> Thank for any answer
>
> dr. Weber
^ permalink raw reply
* Re: [PATCH v3] super1: fix sb->max_dev when adding a new disk in linear array
From: NeilBrown @ 2017-05-21 23:31 UTC (permalink / raw)
To: Lidong Zhong, linux-raid; +Cc: colyli, Jes.Sorensen
In-Reply-To: <20170519060638.4470-1-lzhong@suse.com>
[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]
On Fri, May 19 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 | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/super1.c b/super1.c
> index 2fcb814..811923f 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -1267,8 +1267,13 @@ 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))
> - sb->max_dev = __cpu_to_le32(max+1);
> + if (i >= max) {
> + while (max <= i) {
> + sb->dev_roles[max] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
> + max += 1;
> + }
> + sb->max_dev = __cpu_to_le32(max);
> + }
This part of the patch is OK....
>
> random_uuid(sb->device_uuid);
>
> @@ -1296,6 +1301,10 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
> sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
> sb->dev_roles[info->disk.number] =
> __cpu_to_le16(info->disk.raid_disk);
> + if (sb->raid_disks+1 >= __le32_to_cpu(sb->max_dev)) {
sb->raid_disks is an le32 number, not a cpu number. So adding 1 to
it is clearly wrong.
Why do you think you need a change here at all?
NeilBrown
> + sb->dev_roles[sb->raid_disks] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
> + sb->max_dev = __cpu_to_le32(sb->raid_disks+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
* Re: [PATCH v3] super1: fix sb->max_dev when adding a new disk in linear array
From: Lidong Zhong @ 2017-05-22 3:28 UTC (permalink / raw)
To: NeilBrown, linux-raid; +Cc: colyli, Jes.Sorensen
In-Reply-To: <87lgppkcrn.fsf@notabene.neil.brown.name>
On 05/22/2017 07:31 AM, NeilBrown wrote:
> On Fri, May 19 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 | 13 +++++++++++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/super1.c b/super1.c
>> index 2fcb814..811923f 100644
>> --- a/super1.c
>> +++ b/super1.c
>> @@ -1267,8 +1267,13 @@ 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))
>> - sb->max_dev = __cpu_to_le32(max+1);
>> + if (i >= max) {
>> + while (max <= i) {
>> + sb->dev_roles[max] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
>> + max += 1;
>> + }
>> + sb->max_dev = __cpu_to_le32(max);
>> + }
>
> This part of the patch is OK....
>
>>
>> random_uuid(sb->device_uuid);
>>
>> @@ -1296,6 +1301,10 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
>> sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
>> sb->dev_roles[info->disk.number] =
>> __cpu_to_le16(info->disk.raid_disk);
>> + if (sb->raid_disks+1 >= __le32_to_cpu(sb->max_dev)) {
>
> sb->raid_disks is an le32 number, not a cpu number. So adding 1 to
> it is clearly wrong.
>
Really sorry for the careless...I mean info->array.raid_disks here.
> Why do you think you need a change here at all?
>
The first part of this patch is dealing with the newly added disk
when the disk number is greater than sb->max_dev.
While updating the superblock on the original disks of the linear
array, shouldn't I also check if the disk numbers is greater
than sb->max_dev?
254 info.array.raid_disks = nd+1;
255 info.array.nr_disks = nd+1;
256 info.array.active_disks = nd+1;
257 info.array.working_disks = nd+1;
258
259 st->ss->update_super(st, &info, "linear-grow-update", dv,
260 0, 0, NULL);
261
262 if (st->ss->store_super(st, fd2)) {
263 pr_err("Cannot store new superblock on %s\n", dv);
264 close(fd2);
265 return 1;
266 }
Thanks,
Lidong
> NeilBrown
>
>
>> + sb->dev_roles[sb->raid_disks] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
>> + sb->max_dev = __cpu_to_le32(sb->raid_disks+1);
>> + }
>> } else if (strcmp(update, "resync") == 0) {
>> /* make sure resync happens */
>> sb->resync_offset = 0ULL;
>> --
>> 2.12.0
^ permalink raw reply
* Re: [PATCH v3] super1: fix sb->max_dev when adding a new disk in linear array
From: NeilBrown @ 2017-05-22 4:35 UTC (permalink / raw)
To: Lidong Zhong, linux-raid; +Cc: colyli, Jes.Sorensen
In-Reply-To: <86430c3a-73ed-4c9b-3386-c76b0ee82e82@suse.com>
[-- Attachment #1: Type: text/plain, Size: 3800 bytes --]
On Mon, May 22 2017, Lidong Zhong wrote:
> On 05/22/2017 07:31 AM, NeilBrown wrote:
>> On Fri, May 19 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 | 13 +++++++++++--
>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/super1.c b/super1.c
>>> index 2fcb814..811923f 100644
>>> --- a/super1.c
>>> +++ b/super1.c
>>> @@ -1267,8 +1267,13 @@ 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))
>>> - sb->max_dev = __cpu_to_le32(max+1);
>>> + if (i >= max) {
>>> + while (max <= i) {
>>> + sb->dev_roles[max] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
>>> + max += 1;
>>> + }
>>> + sb->max_dev = __cpu_to_le32(max);
>>> + }
>>
>> This part of the patch is OK....
>>
>>>
>>> random_uuid(sb->device_uuid);
>>>
>>> @@ -1296,6 +1301,10 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
>>> sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
>>> sb->dev_roles[info->disk.number] =
>>> __cpu_to_le16(info->disk.raid_disk);
>>> + if (sb->raid_disks+1 >= __le32_to_cpu(sb->max_dev)) {
>>
>> sb->raid_disks is an le32 number, not a cpu number. So adding 1 to
>> it is clearly wrong.
>>
>
> Really sorry for the careless...I mean info->array.raid_disks here.
>
>> Why do you think you need a change here at all?
>>
>
> The first part of this patch is dealing with the newly added disk
> when the disk number is greater than sb->max_dev.
> While updating the superblock on the original disks of the linear
> array, shouldn't I also check if the disk numbers is greater
> than sb->max_dev?
OK, I can see that you might need to update max_dev to make sure that it
is larger than the new disk.number (or equal to the new raid_disks).
I don't see why you need to make it bigger than raid_disks, or why you
need to change ->dev_roles[] any more than it is already being changed.
i.e. the *only* bug here is with the way max_dev is being updated.
Just change that (in two places).
I don't think the "while (max <= i)" is really needed. I know I wrote
it, and it isn't wrong. But it will always do nothing (except increment
'max' once). So there really isn't any point.
NeilBrown
>
>
> 254 info.array.raid_disks = nd+1;
> 255 info.array.nr_disks = nd+1;
> 256 info.array.active_disks = nd+1;
> 257 info.array.working_disks = nd+1;
> 258
> 259 st->ss->update_super(st, &info, "linear-grow-update", dv,
> 260 0, 0, NULL);
> 261
> 262 if (st->ss->store_super(st, fd2)) {
> 263 pr_err("Cannot store new superblock on %s\n", dv);
> 264 close(fd2);
> 265 return 1;
> 266 }
>
>>
>>
>>> + sb->dev_roles[sb->raid_disks] = __cpu_to_le16(MD_DISK_ROLE_SPARE);
>>> + sb->max_dev = __cpu_to_le32(sb->raid_disks+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
* [PATCH v4] super1: fix sb->max_dev when adding a new disk in linear array
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
* 4.11.2: reshape raid5 -> raid6 atop bcache deadlocks at start on md_attr_store / raid5_make_request
From: Nix @ 2017-05-22 9:13 UTC (permalink / raw)
To: linux-raid, linux-bcache
[I think bcache is blameless here, but there may be some interaction, so
I've Cc:ed both lists. I'm fairly sure the problem is mddev_suspend()
being called from level_store() and scheduling after suspension has
happened, but I'm not sure what the fix might be.]
So I have a box with three RAID arrays on the same physical
rotating-rust devices:
- a RAID-0 /dev/md/transient atop /dev/sd*2
- a RAID-5 /dev/md/fast, a bcache backing device (with disabled cache
for now) atop /dev/sd*3
- a RAID-5 /dev/md/slow atop /dev/sd*4.
The rootfs and everything the system needs to run, including mdadm, is
on /dev/md/fast. fs-wise, everything is using xfs except for
/dev/md/transient, which houses an ext4 (unjournalled, for speed over
robustness).
I just got a new disk and grew /dev/md/transient to use it without
incident, then grew /dev/md/slow into a RAID-6:
mdadm --add /dev/md/slow /dev/sdf4
mdadm --grow /dev/md/slow --level=6 --backup-file=/.transient/backup-slow
This worked fine.
But the *exact same command* applied to /dev/md/fast hangs:
mdadm --add /dev/md/fast /dev/sdf3
mdadm --grow /dev/md/fast --level=6 --backup-file=/.transient/backup-fast
[hang here]
At this point, all disk I/O to /dev/md/fast appears suspended: because /
is on it, this leaves me rather unable to do anything but reboot.
Thankfully the deadlock seemingly happens before anything crucial is
done, and the machine reboots and brigns up /dev/md/fast as an
unreshaped raid5 array with a spare.
Is this workflow impossible? Do you have to reshape the device the
rootfs is on from early userspace or something like that? If so... this
leaves a crucial mdadm running across the early userspace transition:
what do we do about the fs the backup file is on (since the transition
usually deletes everything)? Do we even *need* a backup file for a
reshape that's growing onto a spare, as the manpage suggests we do? Do
we simply have to leave the machine unavailable for days while the
reshape completes? (I hope not.)
That hangcheck output I was able to capture by eye suggests a deadlock
in block I/O during a brief instant of suspended I/O to the device. The
first hang we see comes from bcache, but since it's in "none" mode this
is pretty clearly just a writeback (note the md_make_request at the top
there):
[ 248.877539] Workqueue: bcache cached_dev_nodata
[ 248.888590] Call Trace:
[ 248.899337] __schedule+0x290/0x810
[ 248.909745] schedule+0x36/0x80
[ 248.920113] md_make_request+0x9c/0x220
[ 248.930229] ? wake_atomic_t_function+0x60/0x60
[ 248.940063] generic_make_request+0xfc/0x260
[ 248.949803] cached_dev_nodata+0x28/0x80
[ 248.959393] ? cached_dev_nodata+0x28/0x80
[ 248.968998] process_one_work+0x1e3/0x440
[ 248.978414] worker_thread+0x48/0x4d0
[ 248.987596] kthread+0x108/0x140
[ 248.996747] ? process_one_work+0x440/0x440
[ 249.005965] ? __kthread_create_on_node+0x150/0x150
[ 249.015264] ret_from_fork+0x29/0x40
[ 249.024544] INFO: task kworker/u40:5:245 blocked for more than 120 seconds.
This is fairly obviously
/* If it's a flush, we send the flush to the backing device too */
closure_bio_submit(bio, cl);
in drivers/md/bcache/request.c, which is fine.
After that we start to see xfs writeback hangs, all hanging on
md_make_request() or I/O scheduling:
[ 249.062946] Workqueue: writeback wb_workfn (flush-252:5)
[ 249.072733] Call Trace:
[ 249.082390] __schedule+0x290/0x810
[ 249.092053] schedule+0x36/0x80
[ 249.101568] md_make_request+0x9c/0x220
[ 249.110956] ? wake_atomic_t_function+0x60/0x60
[ 249.120202] generic_make_request+0xfc/0x260
[ 249.129219] submit_bio+0x64/0x120
[ 249.137968] ? submit_bio+0x64/0x120
[ 249.146840] ? xfs_setfilesize_trans_alloc.isra.4+0x2f/0x70
[ 249.155822] xfs_submit_ioend+0x70/0x190
[ 249.164805] xfs_vm_writepages+0x65/0x70
[ 249.173620] do_writepages+0x1e/0x30
[ 249.182239] __writeback_single_inode+0x45/0x320
[ 249.190838] writeback_sb_inodes+0x191/0x490
[ 249.199459] __writeback_inodes_wb+0x92/0xc0
[ 249.207988] wb_writeback+0x243/0x2d0
[ 249.216395] wb_workfn+0x299/0x360
[ 249.224851] ? wb_workfn+0x299/0x360
[ 249.233393] process_one_work+0x1e3/0x440
[ 249.242049] worker_thread+0x48/0x4d0
[ 249.250404] kthread+0x108/0x140
[ 249.258408] ? process_one_work+0x440/0x440
[ 249.266148] ? __kthread_create_on_node+0x150/0x150
[ 249.273652] ret_from_fork+0x29/0x40
[ 249.310928] Workqueue: xfs-cil/dm-5 xlog_cil_push_work
[ 249.318453] Call Trace:
[ 249.325929] __schedule+0x290/0x810
[ 249.333456] schedule+0x36/0x80
[ 249.340940] schedule_timeout+0x207/0x360
[ 249.348461] ? cached_dev_make_request+0x2bc/0xd00
[ 249.356018] io_schedule_timeout+0x1e/0x50
[ 249.363533] ? io_schedule_timeout+0x1e/0x50
[ 249.371078] wait_for_common_io.constprop.0+0x92/0x110
[ 249.378711] ? try_to_wake_up+0x400/0x400
[ 249.386339] wait_for_completion_io+0x18/0x20
[ 249.394026] submit_bio_wait+0x59/0x70
[ 249.401685] blkdev_issue_flush+0x5c/0x90
[ 249.409321] xfs_blkdev_issue_flush+0x19/0x20
[ 249.416951] xlog_sync+0x2a1/0x3b0
[ 249.424439] xlog_state_release_iclog+0x6d/0xd0
[ 249.431857] xlog_write+0x64a/0x6e0
[ 249.439092] xlog_cil_push+0x230/0x450
[ 249.446169] xlog_cil_push_work+0x15/0x20
[ 249.453128] process_one_work+0x1e3/0x440
[ 249.460030] worker_thread+0x221/0x4d0
[ 249.466942] kthread+0x108/0x140
[ 249.473843] ? process_one_work+0x440/0x440
[ 249.480762] ? __kthread_create_on_node+0x150/0x150
[ 249.487743] ret_from_fork+0x29/0x40
[ 249.524000] Workqueue: xfs-log/dm-5 xfs_log_worker
[ 249.531490] Call Trace:
[ 249.538923] __schedule+0x290/0x810
[ 249.546366] schedule+0x36/0x80
[ 249.553756] schedule_timeout+0x207/0x360
[ 249.561132] ? check_preempt_curr+0x79/0x90
[ 249.568514] ? ttwu_do_wakeup.isra.15+0x1e/0x160
[ 249.575957] wait_for_common+0xaa/0x160
[ 249.583432] ? wait_for_common+0xaa/0x160
[ 249.590943] ? try_to_wake_up+0x400/0x400
[ 249.598447] wait_for_completion+0x1d/0x20
[ 249.605984] flush_work+0xfb/0x1b0
[ 249.613509] ? flush_workqueue_prep_pwqs+0x1a0/0x1a0
[ 249.621093] xlog_cil_force_lsn+0x75/0x1d0
[ 249.628640] ? dequeue_task_fair+0x757/0x1920
[ 249.636169] _xfs_log_force+0x76/0x280
[ 249.643524] ? xfs_log_worker+0x32/0x100
[ 249.650715] xfs_log_force+0x2c/0x80
[ 249.657719] xfs_log_worker+0x32/0x100
[ 249.664653] process_one_work+0x1e3/0x440
[ 249.671498] worker_thread+0x48/0x4d0
[ 249.678354] kthread+0x108/0x140
[ 249.685218] ? process_one_work+0x440/0x440
[ 249.692123] ? __kthread_create_on_node+0x150/0x150
[ 249.699057] ret_from_fork+0x29/0x40
[ 249.727654] syslog-ng D 0 1658 1657 0x00000000
[ 249.735066] Call Trace:
[ 249.742333] __schedule+0x290/0x810
[ 249.749624] schedule+0x36/0x80
[ 249.756915] md_write_start+0x8f/0x160
[ 249.764185] ? wake_atomic_t_function+0x60/0x60
[ 249.771469] raid5_make_request+0x7a/0xd40
[ 249.778762] ? bch_data_insert_start+0x4da/0x5e0
[ 249.786143] ? wake_atomic_t_function+0x60/0x60
[ 249.793571] md_make_request+0xe3/0x220
[ 249.800976] generic_make_request+0xfc/0x260
[ 249.808383] submit_bio+0x64/0x120
[ 249.815838] ? submit_bio+0x64/0x120
[ 249.823253] ? xfs_setfilesize_trans_alloc.isra.4+0x2f/0x70
[ 249.830757] xfs_submit_ioend+0x70/0x190
[ 249.838269] xfs_vm_writepages+0x65/0x70
[ 249.845668] do_writepages+0x1e/0x30
[ 249.852899] __filemap_fdatawrite_range+0x71/0x90
[ 249.860023] filemap_write_and_wait_range+0x2a/0x70
[ 249.867061] xfs_file_fsync+0x54/0x190
[ 249.874007] vfs_fsync_range+0x49/0xa0
[ 249.880934] ? __fdget_pos+0x17/0x50
[ 249.887793] ? vfs_writev+0x3c/0x50
[ 249.894700] do_fsync+0x3d/0x70
[ 249.901565] SyS_fsync+0x10/0x20
I think we are stuck inside the
if (mddev->suspended) {
block in md_make_request() in all these cases.
... and finally it becomes clear that mdadm is blocked and we are in
real trouble:
[ 249.965701] INFO: task mdadm:2712 blocked for more than 120 seconds.
[ 249.973003] Not tainted 4.11.2-00005-g87b2117c3309-dirty #2
[ 249.980381] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 249.987999] mdadm D 0 2712 2020 0x00000000
[ 249.995681] Call Trace:
[ 250.003273] __schedule+0x290/0x810
[ 250.010885] schedule+0x36/0x80
[ 250.018503] mddev_suspend+0xb3/0xe0
[ 250.026158] ? wake_atomic_t_function+0x60/0x60
[ 250.033856] level_store+0x1a7/0x6c0
[ 250.041478] ? md_ioctl+0xb7/0x1c10
[ 250.049075] ? putname+0x53/0x60
[ 250.056554] md_attr_store+0x83/0xc0
[ 250.063922] sysfs_kf_write+0x37/0x40
[ 250.071082] kernfs_fop_write+0x110/0x1a0
[ 250.078127] __vfs_write+0x28/0x120
[ 250.085166] ? kernfs_iop_get_link+0x172/0x1e0
[ 250.092199] ? __alloc_fd+0x3f/0x170
[ 250.099271] vfs_write+0xb6/0x1d0
[ 250.106307] SyS_write+0x46/0xb0
[ 250.113227] entry_SYSCALL_64_fastpath+0x13/0x94
I'm not sure what this is waiting on. It could be *any* of
synchronize_rcu();
wait_event(mddev->sb_wait, atomic_read(&mddev->active_io) == 0);
mddev->pers->quiesce(mddev, 1);
since they all can probably schedule and if quiescing involves talking
to the device, we have already suspended it, so we're doomed.
I'm not sure how we can quiesce before we suspend and prevent things
from trying to issue more crucial requests (paging in mdadm?) before we
resume again, but I'm fairly sure that *something* different needs to be
done.
^ permalink raw reply
* Re: [PATCH v4] super1: fix sb->max_dev when adding a new disk in linear array
From: NeilBrown @ 2017-05-22 11:07 UTC (permalink / raw)
To: Lidong Zhong, linux-raid; +Cc: colyli, Jes.Sorensen
In-Reply-To: <20170522061612.29410-1-lzhong@suse.com>
[-- 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
* Re: 4.11.2: reshape raid5 -> raid6 atop bcache deadlocks at start on md_attr_store / raid5_make_request
From: NeilBrown @ 2017-05-22 11:35 UTC (permalink / raw)
To: Nix, linux-raid, linux-bcache
In-Reply-To: <87lgppz221.fsf@esperi.org.uk>
[-- Attachment #1: Type: text/plain, Size: 11912 bytes --]
On Mon, May 22 2017, Nix wrote:
> [I think bcache is blameless here, but there may be some interaction, so
> I've Cc:ed both lists. I'm fairly sure the problem is mddev_suspend()
> being called from level_store() and scheduling after suspension has
> happened, but I'm not sure what the fix might be.]
>
> So I have a box with three RAID arrays on the same physical
> rotating-rust devices:
>
> - a RAID-0 /dev/md/transient atop /dev/sd*2
> - a RAID-5 /dev/md/fast, a bcache backing device (with disabled cache
> for now) atop /dev/sd*3
> - a RAID-5 /dev/md/slow atop /dev/sd*4.
>
> The rootfs and everything the system needs to run, including mdadm, is
> on /dev/md/fast. fs-wise, everything is using xfs except for
> /dev/md/transient, which houses an ext4 (unjournalled, for speed over
> robustness).
>
> I just got a new disk and grew /dev/md/transient to use it without
> incident, then grew /dev/md/slow into a RAID-6:
>
> mdadm --add /dev/md/slow /dev/sdf4
> mdadm --grow /dev/md/slow --level=6 --backup-file=/.transient/backup-slow
>
> This worked fine.
>
> But the *exact same command* applied to /dev/md/fast hangs:
>
> mdadm --add /dev/md/fast /dev/sdf3
> mdadm --grow /dev/md/fast --level=6 --backup-file=/.transient/backup-fast
> [hang here]
>
> At this point, all disk I/O to /dev/md/fast appears suspended: because /
> is on it, this leaves me rather unable to do anything but reboot.
> Thankfully the deadlock seemingly happens before anything crucial is
> done, and the machine reboots and brigns up /dev/md/fast as an
> unreshaped raid5 array with a spare.
>
> Is this workflow impossible? Do you have to reshape the device the
> rootfs is on from early userspace or something like that? If so... this
> leaves a crucial mdadm running across the early userspace transition:
> what do we do about the fs the backup file is on (since the transition
> usually deletes everything)? Do we even *need* a backup file for a
> reshape that's growing onto a spare, as the manpage suggests we do? Do
> we simply have to leave the machine unavailable for days while the
> reshape completes? (I hope not.)
>
> That hangcheck output I was able to capture by eye suggests a deadlock
> in block I/O during a brief instant of suspended I/O to the device. The
> first hang we see comes from bcache, but since it's in "none" mode this
> is pretty clearly just a writeback (note the md_make_request at the top
> there):
>
> [ 248.877539] Workqueue: bcache cached_dev_nodata
> [ 248.888590] Call Trace:
> [ 248.899337] __schedule+0x290/0x810
> [ 248.909745] schedule+0x36/0x80
> [ 248.920113] md_make_request+0x9c/0x220
> [ 248.930229] ? wake_atomic_t_function+0x60/0x60
> [ 248.940063] generic_make_request+0xfc/0x260
> [ 248.949803] cached_dev_nodata+0x28/0x80
> [ 248.959393] ? cached_dev_nodata+0x28/0x80
> [ 248.968998] process_one_work+0x1e3/0x440
> [ 248.978414] worker_thread+0x48/0x4d0
> [ 248.987596] kthread+0x108/0x140
> [ 248.996747] ? process_one_work+0x440/0x440
> [ 249.005965] ? __kthread_create_on_node+0x150/0x150
> [ 249.015264] ret_from_fork+0x29/0x40
> [ 249.024544] INFO: task kworker/u40:5:245 blocked for more than 120 seconds.
>
> This is fairly obviously
>
> /* If it's a flush, we send the flush to the backing device too */
> closure_bio_submit(bio, cl);
>
> in drivers/md/bcache/request.c, which is fine.
>
> After that we start to see xfs writeback hangs, all hanging on
> md_make_request() or I/O scheduling:
>
> [ 249.062946] Workqueue: writeback wb_workfn (flush-252:5)
> [ 249.072733] Call Trace:
> [ 249.082390] __schedule+0x290/0x810
> [ 249.092053] schedule+0x36/0x80
> [ 249.101568] md_make_request+0x9c/0x220
> [ 249.110956] ? wake_atomic_t_function+0x60/0x60
> [ 249.120202] generic_make_request+0xfc/0x260
> [ 249.129219] submit_bio+0x64/0x120
> [ 249.137968] ? submit_bio+0x64/0x120
> [ 249.146840] ? xfs_setfilesize_trans_alloc.isra.4+0x2f/0x70
> [ 249.155822] xfs_submit_ioend+0x70/0x190
> [ 249.164805] xfs_vm_writepages+0x65/0x70
> [ 249.173620] do_writepages+0x1e/0x30
> [ 249.182239] __writeback_single_inode+0x45/0x320
> [ 249.190838] writeback_sb_inodes+0x191/0x490
> [ 249.199459] __writeback_inodes_wb+0x92/0xc0
> [ 249.207988] wb_writeback+0x243/0x2d0
> [ 249.216395] wb_workfn+0x299/0x360
> [ 249.224851] ? wb_workfn+0x299/0x360
> [ 249.233393] process_one_work+0x1e3/0x440
> [ 249.242049] worker_thread+0x48/0x4d0
> [ 249.250404] kthread+0x108/0x140
> [ 249.258408] ? process_one_work+0x440/0x440
> [ 249.266148] ? __kthread_create_on_node+0x150/0x150
> [ 249.273652] ret_from_fork+0x29/0x40
>
> [ 249.310928] Workqueue: xfs-cil/dm-5 xlog_cil_push_work
> [ 249.318453] Call Trace:
> [ 249.325929] __schedule+0x290/0x810
> [ 249.333456] schedule+0x36/0x80
> [ 249.340940] schedule_timeout+0x207/0x360
> [ 249.348461] ? cached_dev_make_request+0x2bc/0xd00
> [ 249.356018] io_schedule_timeout+0x1e/0x50
> [ 249.363533] ? io_schedule_timeout+0x1e/0x50
> [ 249.371078] wait_for_common_io.constprop.0+0x92/0x110
> [ 249.378711] ? try_to_wake_up+0x400/0x400
> [ 249.386339] wait_for_completion_io+0x18/0x20
> [ 249.394026] submit_bio_wait+0x59/0x70
> [ 249.401685] blkdev_issue_flush+0x5c/0x90
> [ 249.409321] xfs_blkdev_issue_flush+0x19/0x20
> [ 249.416951] xlog_sync+0x2a1/0x3b0
> [ 249.424439] xlog_state_release_iclog+0x6d/0xd0
> [ 249.431857] xlog_write+0x64a/0x6e0
> [ 249.439092] xlog_cil_push+0x230/0x450
> [ 249.446169] xlog_cil_push_work+0x15/0x20
> [ 249.453128] process_one_work+0x1e3/0x440
> [ 249.460030] worker_thread+0x221/0x4d0
> [ 249.466942] kthread+0x108/0x140
> [ 249.473843] ? process_one_work+0x440/0x440
> [ 249.480762] ? __kthread_create_on_node+0x150/0x150
> [ 249.487743] ret_from_fork+0x29/0x40
>
> [ 249.524000] Workqueue: xfs-log/dm-5 xfs_log_worker
> [ 249.531490] Call Trace:
> [ 249.538923] __schedule+0x290/0x810
> [ 249.546366] schedule+0x36/0x80
> [ 249.553756] schedule_timeout+0x207/0x360
> [ 249.561132] ? check_preempt_curr+0x79/0x90
> [ 249.568514] ? ttwu_do_wakeup.isra.15+0x1e/0x160
> [ 249.575957] wait_for_common+0xaa/0x160
> [ 249.583432] ? wait_for_common+0xaa/0x160
> [ 249.590943] ? try_to_wake_up+0x400/0x400
> [ 249.598447] wait_for_completion+0x1d/0x20
> [ 249.605984] flush_work+0xfb/0x1b0
> [ 249.613509] ? flush_workqueue_prep_pwqs+0x1a0/0x1a0
> [ 249.621093] xlog_cil_force_lsn+0x75/0x1d0
> [ 249.628640] ? dequeue_task_fair+0x757/0x1920
> [ 249.636169] _xfs_log_force+0x76/0x280
> [ 249.643524] ? xfs_log_worker+0x32/0x100
> [ 249.650715] xfs_log_force+0x2c/0x80
> [ 249.657719] xfs_log_worker+0x32/0x100
> [ 249.664653] process_one_work+0x1e3/0x440
> [ 249.671498] worker_thread+0x48/0x4d0
> [ 249.678354] kthread+0x108/0x140
> [ 249.685218] ? process_one_work+0x440/0x440
> [ 249.692123] ? __kthread_create_on_node+0x150/0x150
> [ 249.699057] ret_from_fork+0x29/0x40
>
> [ 249.727654] syslog-ng D 0 1658 1657 0x00000000
> [ 249.735066] Call Trace:
> [ 249.742333] __schedule+0x290/0x810
> [ 249.749624] schedule+0x36/0x80
> [ 249.756915] md_write_start+0x8f/0x160
> [ 249.764185] ? wake_atomic_t_function+0x60/0x60
> [ 249.771469] raid5_make_request+0x7a/0xd40
> [ 249.778762] ? bch_data_insert_start+0x4da/0x5e0
> [ 249.786143] ? wake_atomic_t_function+0x60/0x60
> [ 249.793571] md_make_request+0xe3/0x220
> [ 249.800976] generic_make_request+0xfc/0x260
> [ 249.808383] submit_bio+0x64/0x120
> [ 249.815838] ? submit_bio+0x64/0x120
> [ 249.823253] ? xfs_setfilesize_trans_alloc.isra.4+0x2f/0x70
> [ 249.830757] xfs_submit_ioend+0x70/0x190
> [ 249.838269] xfs_vm_writepages+0x65/0x70
> [ 249.845668] do_writepages+0x1e/0x30
> [ 249.852899] __filemap_fdatawrite_range+0x71/0x90
> [ 249.860023] filemap_write_and_wait_range+0x2a/0x70
> [ 249.867061] xfs_file_fsync+0x54/0x190
> [ 249.874007] vfs_fsync_range+0x49/0xa0
> [ 249.880934] ? __fdget_pos+0x17/0x50
> [ 249.887793] ? vfs_writev+0x3c/0x50
> [ 249.894700] do_fsync+0x3d/0x70
> [ 249.901565] SyS_fsync+0x10/0x20
>
> I think we are stuck inside the
>
> if (mddev->suspended) {
>
> block in md_make_request() in all these cases.
>
> ... and finally it becomes clear that mdadm is blocked and we are in
> real trouble:
>
> [ 249.965701] INFO: task mdadm:2712 blocked for more than 120 seconds.
> [ 249.973003] Not tainted 4.11.2-00005-g87b2117c3309-dirty #2
> [ 249.980381] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> [ 249.987999] mdadm D 0 2712 2020 0x00000000
> [ 249.995681] Call Trace:
> [ 250.003273] __schedule+0x290/0x810
> [ 250.010885] schedule+0x36/0x80
> [ 250.018503] mddev_suspend+0xb3/0xe0
> [ 250.026158] ? wake_atomic_t_function+0x60/0x60
> [ 250.033856] level_store+0x1a7/0x6c0
> [ 250.041478] ? md_ioctl+0xb7/0x1c10
> [ 250.049075] ? putname+0x53/0x60
> [ 250.056554] md_attr_store+0x83/0xc0
> [ 250.063922] sysfs_kf_write+0x37/0x40
> [ 250.071082] kernfs_fop_write+0x110/0x1a0
> [ 250.078127] __vfs_write+0x28/0x120
> [ 250.085166] ? kernfs_iop_get_link+0x172/0x1e0
> [ 250.092199] ? __alloc_fd+0x3f/0x170
> [ 250.099271] vfs_write+0xb6/0x1d0
> [ 250.106307] SyS_write+0x46/0xb0
> [ 250.113227] entry_SYSCALL_64_fastpath+0x13/0x94
>
> I'm not sure what this is waiting on. It could be *any* of
>
> synchronize_rcu();
> wait_event(mddev->sb_wait, atomic_read(&mddev->active_io) == 0);
> mddev->pers->quiesce(mddev, 1);
It says 'schedule()' so it is in wait_event().
>
> since they all can probably schedule and if quiescing involves talking
> to the device, we have already suspended it, so we're doomed.
>
> I'm not sure how we can quiesce before we suspend and prevent things
> from trying to issue more crucial requests (paging in mdadm?) before we
> resume again, but I'm fairly sure that *something* different needs to be
> done.
Congratulations. You have found a bug that dates from 2011.
Commit: 68866e425be2 ("MD: no sync IO while suspended")
(I think).
A write request gets to raid5_make_request() before mddev_suspend() sets
mddev->suspended.
raid5_make_request() needs md_write_start() to mark the array "dirty"
which it asks the md thread to do, but before the thread gets to the
task, mddev->suspend has been set, so md_check_recovery() doesn't update
the superblock, so md_write_start() doesn't proceed, so the request
never completes, so mddev_suspend() blocks indefinitely.
I think that md_check_recovery() need to test for mddev->suspended
somewhere else.
It needs to allow superblock updates, and probably needs to reap the
recovery thread, but must not start a new recovery thread.
Probably something like this:
diff --git a/drivers/md/md.c b/drivers/md/md.c
index f6ae1d67bcd0..dbca31be22a1 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8364,8 +8364,6 @@ static void md_start_sync(struct work_struct *ws)
*/
void md_check_recovery(struct mddev *mddev)
{
- if (mddev->suspended)
- return;
if (mddev->bitmap)
bitmap_daemon_work(mddev);
@@ -8484,6 +8482,7 @@ void md_check_recovery(struct mddev *mddev)
clear_bit(MD_RECOVERY_DONE, &mddev->recovery);
if (!test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
+ mddev->suspended ||
test_bit(MD_RECOVERY_FROZEN, &mddev->recovery))
goto not_running;
/* no recovery is running.
though it's late so don't trust anything I write.
If you try again it will almost certainly succeed. I suspect this is a
hard race to hit - well done!!!
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply related
* Reducing the number of devices in a degraded RAID-5
From: Andreas Klauer @ 2017-05-22 12:53 UTC (permalink / raw)
To: linux-raid
Hi,
this is not a recovery question, no real data involved. Thanks for helping!
Suppose you have a failing drive in RAID-5 but you wanted to move to
fewer drives anyway, so one way or another you're going to reduce
the number of drives in your RAID.
Given a RAID 5 with 5 drives [UUUUU]
Reducing it by one drive results in [UUUU] + Spare
Okay.
Given a degraded RAID 5 with 5 drives [_UUUU]
Reducing it by one drive results in [_UUU] + Spare
Still okay? Rebuild must be started manually.
It seems reducing a degraded RAID is a bad idea,
since there is no redundancy for a very long time.
So what you might end up doing is a three step process:
-> [_UUUU] (Degraded)
Step 1: Add another drive (redundancy first)
-> [UUUUU]
^ added drive
Step 2: Reduce by one drive
-> [UUUU] + Spare
Step 3: --replace the previously added drive
(if the spare happened to be one of the drives you wanted to keep)
-> [UUUU]
^ former spare
This way the process is redundant but it takes a very long time,
three separate reshape/rebuilds instead of just one.
Steps to reproduce the [_UUUU] -> [_UUU] + Spare case:
(using linux 4.10, mdadm 4.0)
# truncate -s 100M 1.img 2.img 3.img 4.img
# devices=$(for f in ?.img; do losetup --find --show "$f"; done)
# mdadm --create /dev/md42 --level=5 --raid-devices=5 missing $devices
md42 : active raid5 loop4[4] loop3[3] loop2[2] loop1[1]
405504 blocks super 1.2 level 5, 512k chunk, algorithm 2 [5/4] [_UUUU]
# mdadm --grow /dev/md42 --array-size 304128
# mdadm --grow /dev/md42 --backup-file=md42.backup --raid-devices=4
md42 : active raid5 loop4[4](S) loop3[3] loop2[2] loop1[1]
304128 blocks super 1.2 level 5, 512k chunk, algorithm 2 [4/3] [_UUU]
# not rebuilding until you re-add the spare
Is it possible to do [_UUUU] -> [UUUU] in a single step?
I haven't found a way. Any ideas?
Regards
Andreas Klauer
^ permalink raw reply
* Re: 4.11.2: reshape raid5 -> raid6 atop bcache deadlocks at start on md_attr_store / raid5_make_request
From: Nix @ 2017-05-22 15:30 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <87a865jf9a.fsf@notabene.neil.brown.name>
[linux-bcache removed, not a bcache bug]
On 22 May 2017, NeilBrown said this:
> Congratulations. You have found a bug that dates from 2011.
Oh so not that old then! (My personal record, in a previous job, was a
bug in allegedly critical functionality that entirely stopped it working
and had been broken for around fourteen years. Like hell it was
critical.)
> Commit: 68866e425be2 ("MD: no sync IO while suspended")
>
> (I think).
>
> A write request gets to raid5_make_request() before mddev_suspend() sets
> mddev->suspended.
> raid5_make_request() needs md_write_start() to mark the array "dirty"
> which it asks the md thread to do, but before the thread gets to the
> task, mddev->suspend has been set, so md_check_recovery() doesn't update
> the superblock, so md_write_start() doesn't proceed, so the request
> never completes, so mddev_suspend() blocks indefinitely.
Ooof. Looks like it's fsync()ing something -- not sure what, though.
The previous time it was a metadata update...
Hm. I note that ->events is meant to be for things like device additions
and array starts. RAID5 -> RAID6 reshaping appears to push this up a
lot:
Events : 1475948
(it was at around 4000 before this -- itself rather mystifying, given
that I've only rebooted this machine 27 times, and the array's probably
been assembled less than 20 times, and was created with the same number
of devices it has now.)
The other array has only 92 events (it was at 30-something before I
tried this reshape).
> I think that md_check_recovery() need to test for mddev->suspended
> somewhere else.
> It needs to allow superblock updates, and probably needs to reap the
> recovery thread, but must not start a new recovery thread.
My question would be, is all the intricate stuff md_check_recovery() is
doing valid on an mddev that's not only suspended but detached? Because
we detach right after the suspension in level_store()...
(btw, you probably want to remove the comment above enum array_state
that says that "suspended" is "not supported yet", too.)
> Probably something like this:
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index f6ae1d67bcd0..dbca31be22a1 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8364,8 +8364,6 @@ static void md_start_sync(struct work_struct *ws)
> */
> void md_check_recovery(struct mddev *mddev)
> {
> - if (mddev->suspended)
> - return;
>
> if (mddev->bitmap)
> bitmap_daemon_work(mddev);
> @@ -8484,6 +8482,7 @@ void md_check_recovery(struct mddev *mddev)
> clear_bit(MD_RECOVERY_DONE, &mddev->recovery);
>
> if (!test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
> + mddev->suspended ||
> test_bit(MD_RECOVERY_FROZEN, &mddev->recovery))
> goto not_running;
> /* no recovery is running.
>
> though it's late so don't trust anything I write.
This may end up clearing MD_RECOVERY_INTR and MD_RECOVERY_DONE, but I
guess in this context it's safe to do so... looks OK otherwise to the
totally ignorant fool reading this patch!
Hm, being picky now, md_check_recovery() might be slightly easier to
read if that governing mutex_trylock() conditional was inverted:
if (mddev_trylock(mddev))
return;
Then you could de-indent most of the function by one indentation
level...
> If you try again it will almost certainly succeed. I suspect this is a
> hard race to hit - well done!!!
I'll give it a try -- I hit it twice in succession, once with a
--backup-file, once without. Since mdadm does not warn about the lack of
a --backup-file, I guess the statement in the manual that it is
essential to provide one when changing RAID levels is untrue: I suspect
that it's necessary *if* you're not increasing the number of disks at
the same time, but since I'm growing into a spare, adding a
--backup-file only slows it down?
I might run a backup first though. :)
^ permalink raw reply
* Re: [PATCH 10/23] afs: switch to use uuid_t and uuid_gen
From: Andy Shevchenko @ 2017-05-22 18:49 UTC (permalink / raw)
To: Christoph Hellwig, Amir Goldstein, linux-fsdevel
Cc: Shaohua Li, Dan Williams, David Howells, Steven Whitehouse,
Mimi Zohar, linux-xfs, linux-raid, linux-nvdimm, linux-kernel
In-Reply-To: <20170518062705.25902-11-hch@lst.de>
On Thu, 2017-05-18 at 08:26 +0200, Christoph Hellwig wrote:
Changelog?
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: David Howells <dhowells@redhat.com>
> @@ -453,7 +453,7 @@ static int afs_deliver_cb_probe(struct afs_call
> *call)
> static void SRXAFSCB_ProbeUuid(struct work_struct *work)
> {
> struct afs_call *call = container_of(work, struct afs_call,
> work);
> - struct uuid_v1 *r = call->request;
> + uuid_t *r = call->request;
>
> struct {
> __be32 match;
>
Just to double check that this doesn't create a union aliasing.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH 13/23] md: namespace private helper names
From: Andy Shevchenko @ 2017-05-22 18:52 UTC (permalink / raw)
To: Christoph Hellwig, Amir Goldstein,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-xfs-u79uwXL29TY76Z2rM5mHXA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Shaohua Li,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, David Howells, Mimi Zohar,
Steven Whitehouse
In-Reply-To: <20170518062705.25902-14-hch-jcswGhMUV9g@public.gmane.org>
On Thu, 2017-05-18 at 08:26 +0200, Christoph Hellwig wrote:
> From: Amir Goldstein <amir73il@gmail.com>
>
> The md private helper uuid_equal() collides with a generic helper
> of the same name.
>
> Rename the md private helper to md_uuid_equal() and do the same for
> md_sb_equal().
>
While patch is good, shouldn't it go before we introduce those helpers?
> Cc: Shaohua Li <shli@kernel.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/md/md.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 82f798be964f..65795cc4cb7d 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -825,7 +825,7 @@ static int read_disk_sb(struct md_rdev *rdev, int
> size)
> return -EINVAL;
> }
>
> -static int uuid_equal(mdp_super_t *sb1, mdp_super_t *sb2)
> +static int md_uuid_equal(mdp_super_t *sb1, mdp_super_t *sb2)
> {
> return sb1->set_uuid0 == sb2->set_uuid0 &&
> sb1->set_uuid1 == sb2->set_uuid1 &&
> @@ -833,7 +833,7 @@ static int uuid_equal(mdp_super_t *sb1,
> mdp_super_t *sb2)
> sb1->set_uuid3 == sb2->set_uuid3;
> }
>
> -static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
> +static int md_sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
> {
> int ret;
> mdp_super_t *tmp1, *tmp2;
> @@ -1025,12 +1025,12 @@ static int super_90_load(struct md_rdev *rdev,
> struct md_rdev *refdev, int minor
> } else {
> __u64 ev1, ev2;
> mdp_super_t *refsb = page_address(refdev->sb_page);
> - if (!uuid_equal(refsb, sb)) {
> + if (!md_uuid_equal(refsb, sb)) {
> pr_warn("md: %s has different UUID to %s\n",
> b, bdevname(refdev->bdev,b2));
> goto abort;
> }
> - if (!sb_equal(refsb, sb)) {
> + if (!md_sb_equal(refsb, sb)) {
> pr_warn("md: %s has same UUID but different
> superblock to %s\n",
> b, bdevname(refdev->bdev, b2));
> goto abort;
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply
* raid5 to raid6 reshape never appeared to start, how to cancel/revert
From: Roger Heflin @ 2017-05-22 18:57 UTC (permalink / raw)
To: Linux RAID
I had a 3 disk raid5 with a hot spare. I ran this:
mdadm --grow /dev/md126 --level=6 --backup-file /root/r6rebuild
I suspect I should have changed the number of devices in the above command to 4.
The reshape "started" according to /proc/mdstat but never got past
block 1, and the time to complete started going up. I did stop the
array and have tried to do a revert-reshape but it indicates it will
only revert a number of devices change.
The backup-file was created on a separate ssd.
trying assemble now gets this:
mdadm --assemble /dev/md126 /dev/sd[abe]1 /dev/sdd
--backup-file=/root/r6rebuild
mdadm: Failed to restore critical section for reshape, sorry.
examine shows this (sdd was the spare when the --grow was issues)
mdadm --examine /dev/sdd
/dev/sdd1:
Magic : a92b4efc
Version : 0.91.00
UUID : 2fb920b1:ce7407fd:dd1a1aa6:74dcda71
Creation Time : Wed May 19 19:04:03 2010
Raid Level : raid6
Used Dev Size : 488384384 (465.76 GiB 500.11 GB)
Array Size : 976768768 (931.52 GiB 1000.21 GB)
Raid Devices : 4
Total Devices : 4
Preferred Minor : 126
Reshape pos'n : 0
New Layout : left-symmetric
Update Time : Mon May 22 09:25:38 2017
State : clean
Internal Bitmap : present
Active Devices : 4
Working Devices : 4
Failed Devices : 0
Spare Devices : 0
Checksum : bc406f24 - correct
Events : 6140735
Layout : left-symmetric-6
Chunk Size : 64K
Number Major Minor RaidDevice State
this 3 8 48 3 active /dev/sdd
0 0 8 17 0 active sync /dev/sdb1
1 1 8 65 1 active sync /dev/sde1
2 2 8 1 2 active sync /dev/sda1
3 3 8 48 3 active /dev/sdd
mdadm-3.4-2.fc25.x86_64
kernel 4.10.15.200 fc25 fully updated as of 2 days ago.
Examine seems to indicate that the reshape never stopped but the
revert is unable to cancel this grow even though it did not start at
all.
The data is not super critical I don't believe I lose anything as this
one is being used primarily for backups.
It does appear that I added sdd rather than sdd1 but I don't believe
that is anything critical to the issue as it should still work fine
with the entire disk.
Ideas on how to abort the reshape that never started or how to get it
to continue? The desired final target is a 4 disk raid6. I have not
as of yet rebooted.
^ permalink raw reply
* Re: [PATCH 23/23] uuid: remove uuid_be
From: Andy Shevchenko @ 2017-05-22 18:59 UTC (permalink / raw)
To: Christoph Hellwig, Amir Goldstein
Cc: linux-xfs, linux-raid-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Shaohua Li, Hugh Dickins,
linux-kernel, David Howells, linux-fsdevel, Mimi Zohar,
Steven Whitehouse
In-Reply-To: <20170518085629.GA6149-jcswGhMUV9g@public.gmane.org>
On Thu, 2017-05-18 at 10:56 +0200, Christoph Hellwig wrote:
> On Thu, May 18, 2017 at 10:57:24AM +0300, Amir Goldstein wrote:
> > I reviewed the entire series. You may add
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> >
> > to any of the patches as you see fit.
>
> Thanks, done!
Similar from my side, FWIW:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> > Now let's talk about how this is going to be merged.
> > Do you intend to send Linus a pull request?
>
> Yes, that might be the best idea. I'm also contemplating listing
> me plus anyone volunteering (you?, Andy) as maintaines for the uuid
> code.
I would agree to be a designated reviewer for now (too many stuff to
follow as a (co-)maintainer).
> > The reason I am asking is because this last removal patch should
> > probably
> > be applied near the end of the merge window (?).
> > Because maintainers cannot apply patches with code that uses the new
> > uuid_t to linux-next branches and we don't want linux-next build to
> > fail
> > with new code that uses uuid_be...
>
> Yeah, we can probably defer that one for now.
I would also like to append some of patches from my side, though they
are WIP.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply
* Re: 4.11.2: reshape raid5 -> raid6 atop bcache deadlocks at start on md_attr_store / raid5_make_request
From: Wols Lists @ 2017-05-22 19:07 UTC (permalink / raw)
To: Nix, NeilBrown; +Cc: linux-raid
In-Reply-To: <87o9ukykmk.fsf@esperi.org.uk>
On 22/05/17 16:30, Nix wrote:
> I'll give it a try -- I hit it twice in succession, once with a
> --backup-file, once without. Since mdadm does not warn about the lack of
> a --backup-file, I guess the statement in the manual that it is
> essential to provide one when changing RAID levels is untrue: I suspect
> that it's necessary *if* you're not increasing the number of disks at
> the same time, but since I'm growing into a spare, adding a
> --backup-file only slows it down?
I did discuss this with Neil while I wrote it, so I hope I got it right :-)
https://raid.wiki.kernel.org/index.php/A_guide_to_mdadm#Array_internals_and_how_it_affects_mdadm
aiui, provided you're using a v1 superblock, the data offset means there
is spare space on the drives precisely for the purpose (one of then at
least) of keeping a backup. So the reshape will start reshaping into the
spare space and eliminate the need for the backup - the new version of
the stripe will be safely written before the space occupied by the old
stripe is required.
Cheers,
Wol
^ permalink raw reply
* Re: raid5 to raid6 reshape never appeared to start, how to cancel/revert
From: Andreas Klauer @ 2017-05-22 19:33 UTC (permalink / raw)
To: Roger Heflin; +Cc: Linux RAID
In-Reply-To: <CAAMCDefCqUCY4Pd5im1u-JPh_SLG6BG8Onb8JxuPwoAkgXOkrA@mail.gmail.com>
On Mon, May 22, 2017 at 01:57:44PM -0500, Roger Heflin wrote:
> I had a 3 disk raid5 with a hot spare. I ran this:
> mdadm --grow /dev/md126 --level=6 --backup-file /root/r6rebuild
>
> I suspect I should have changed the number of devices in the above command to 4.
It doesn't hurt to specify, but that much is implied.
Growing 3 device raid5 + spare to raid6 results in 4 device raid6.
> The backup-file was created on a separate ssd.
Is there anything meaningful in this file?
> trying assemble now gets this:
> mdadm --assemble /dev/md126 /dev/sd[abe]1 /dev/sdd
> --backup-file=/root/r6rebuild
> mdadm: Failed to restore critical section for reshape, sorry.
>
> examine shows this (sdd was the spare when the --grow was issues)
> mdadm --examine /dev/sdd
> /dev/sdd1:
You wrote /dev/sdd above, is it sdd1 now?
> Version : 0.91.00
Ancient metadata. You could probably update it to 1.0...
> Reshape pos'n : 0
So maybe nothing at all changed on disk?
You could try your luck with overlay
https://raid.wiki.kernel.org/index.php/Recovering_a_failed_software_RAID#Making_the_harddisks_read-only_using_an_overlay_file
mdadm --create /dev/md42 --metadata=0.90 --level=5 --chunk=64 \
--raid-devices=3 /dev/overlay/{a,b,c}
> It does appear that I added sdd rather than sdd1 but I don't believe
> that is anything critical to the issue as it should still work fine
> with the entire disk.
It is critical because if you use the wrong one the data will be shifted.
If the partition goes to the very end of the drive, I think the 0.90
metadata could be interpreted both ways (as metadata for partition
as well as whole drive).
If possible you should find some way to migrate to 1.2 metadata.
But worry about it once you have access to your data.
Regards
Andreas Klauer
^ permalink raw reply
* Re: raid5 to raid6 reshape never appeared to start, how to cancel/revert
From: Roger Heflin @ 2017-05-22 20:04 UTC (permalink / raw)
To: Andreas Klauer; +Cc: Linux RAID
In-Reply-To: <20170522193304.GA4379@metamorpher.de>
On Mon, May 22, 2017 at 2:33 PM, Andreas Klauer
<Andreas.Klauer@metamorpher.de> wrote:
> On Mon, May 22, 2017 at 01:57:44PM -0500, Roger Heflin wrote:
>> I had a 3 disk raid5 with a hot spare. I ran this:
>> mdadm --grow /dev/md126 --level=6 --backup-file /root/r6rebuild
>>
>> I suspect I should have changed the number of devices in the above command to 4.
>
> It doesn't hurt to specify, but that much is implied.
> Growing 3 device raid5 + spare to raid6 results in 4 device raid6.
>
Yes.
>> The backup-file was created on a separate ssd.
>
> Is there anything meaningful in this file?
>
16MB in size, but od -x indicates all zeros, so no, there is nothing
meaningful in the file.
>> trying assemble now gets this:
>> mdadm --assemble /dev/md126 /dev/sd[abe]1 /dev/sdd
>> --backup-file=/root/r6rebuild
>> mdadm: Failed to restore critical section for reshape, sorry.
>>
>> examine shows this (sdd was the spare when the --grow was issues)
>> mdadm --examine /dev/sdd
>> /dev/sdd1:
>
> You wrote /dev/sdd above, is it sdd1 now?
>
>> Version : 0.91.00
>
> Ancient metadata. You could probably update it to 1.0...
>
I know.
>> Reshape pos'n : 0
>
> So maybe nothing at all changed on disk?
>
> You could try your luck with overlay
>
> https://raid.wiki.kernel.org/index.php/Recovering_a_failed_software_RAID#Making_the_harddisks_read-only_using_an_overlay_file
>
> mdadm --create /dev/md42 --metadata=0.90 --level=5 --chunk=64 \
> --raid-devices=3 /dev/overlay/{a,b,c}
>
>> It does appear that I added sdd rather than sdd1 but I don't believe
>> that is anything critical to the issue as it should still work fine
>> with the entire disk.
>
> It is critical because if you use the wrong one the data will be shifted.
>
> If the partition goes to the very end of the drive, I think the 0.90
> metadata could be interpreted both ways (as metadata for partition
> as well as whole drive).
>
> If possible you should find some way to migrate to 1.2 metadata.
> But worry about it once you have access to your data.
>
I deal with others messing up partition/no partition recoveries often
enough to not be worried about how to debug and/or fix that mistake.
I found a patch from Neil from 2016 that may be solution to this
issue, I am not clear if it is an exact match to my issue, it looks
pretty close.
http://comments.gmane.org/gmane.linux.raid/51095
> Regards
> Andreas Klauer
^ permalink raw reply
* Re: 4.11.2: reshape raid5 -> raid6 atop bcache deadlocks at start on md_attr_store / raid5_make_request
From: Nix @ 2017-05-22 20:43 UTC (permalink / raw)
To: Wols Lists; +Cc: NeilBrown, linux-raid
In-Reply-To: <592336E6.8080906@youngman.org.uk>
On 22 May 2017, Wols Lists verbalised:
> aiui, provided you're using a v1 superblock, the data offset means there
> is spare space on the drives precisely for the purpose (one of then at
> least) of keeping a backup. So the reshape will start reshaping into the
> spare space and eliminate the need for the backup - the new version of
> the stripe will be safely written before the space occupied by the old
> stripe is required.
But it's only a few KiB by default! The amount of seeking needed to
reshape with such a small intermediate would be fairly horrific. (It was
bad enough as it was: the reshape of 7TiB took more than two days,
running at under 15MiB/s, though the component drives can all handle
220MiB/s easily. The extra time was spent seeking to and from the
backup, it seems.)
> Typically, the new layout will move the data offset, pushing the data
> block up or down.
Will it? Why? You'd need to move the data offset quite a long way to
make enough space for the 500MiB backup file I saw being created during
my last reshape.
(Though, for me, this is somewhat redundant:
Data Offset : 262144 sectors
because I set a data offset specifically to allow reshaping without
having to move anything. Is the idea that reshaping that adds data
spindles will move the data offset such that it is (still) on a chunk or
stripe multiple? That's neat, if so, and means I wasted 128MiB on this,
uh, 12TiB array. OK I'm not terribly blown away by this, particularly
given that I'm wasting the same again inside the bcache partition for
the same reason: I'm sure mdadm won't move *that* data offset.)
--
NULL && (void)
^ permalink raw reply
* Re: 4.11.2: reshape raid5 -> raid6 atop bcache deadlocks at start on md_attr_store / raid5_make_request
From: Nix @ 2017-05-22 21:38 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <87a865jf9a.fsf@notabene.neil.brown.name>
On 22 May 2017, NeilBrown told this:
> Probably something like this:
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index f6ae1d67bcd0..dbca31be22a1 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8364,8 +8364,6 @@ static void md_start_sync(struct work_struct *ws)
> */
> void md_check_recovery(struct mddev *mddev)
> {
> - if (mddev->suspended)
> - return;
>
> if (mddev->bitmap)
> bitmap_daemon_work(mddev);
> @@ -8484,6 +8482,7 @@ void md_check_recovery(struct mddev *mddev)
> clear_bit(MD_RECOVERY_DONE, &mddev->recovery);
>
> if (!test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
> + mddev->suspended ||
> test_bit(MD_RECOVERY_FROZEN, &mddev->recovery))
> goto not_running;
> /* no recovery is running.
>
> though it's late so don't trust anything I write.
>
> If you try again it will almost certainly succeed. I suspect this is a
> hard race to hit - well done!!!
Definitely not a hard race to hit :( I just hit it again with this
patch.
Absolutely identical hang:
[ 495.833520] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 495.840618] mdadm D 0 2700 2537 0x00000000
[ 495.847762] Call Trace:
[ 495.854825] __schedule+0x290/0x810
[ 495.861905] schedule+0x36/0x80
[ 495.868934] mddev_suspend+0xb3/0xe0
[ 495.875926] ? wake_atomic_t_function+0x60/0x60
[ 495.882976] level_store+0x1a7/0x6c0
[ 495.889953] ? md_ioctl+0xb7/0x1c10
[ 495.896901] ? putname+0x53/0x60
[ 495.903807] md_attr_store+0x83/0xc0
[ 495.910684] sysfs_kf_write+0x37/0x40
[ 495.917547] kernfs_fop_write+0x110/0x1a0
[ 495.924429] __vfs_write+0x28/0x120
[ 495.931270] ? kernfs_iop_get_link+0x172/0x1e0
[ 495.938126] ? __alloc_fd+0x3f/0x170
[ 495.944906] vfs_write+0xb6/0x1d0
[ 495.951646] SyS_write+0x46/0xb0
[ 495.958338] entry_SYSCALL_64_fastpath+0x13/0x94
Everything else hangs the same way, too. This was surprising enough that
I double-checked to be sure the patch was applied: it was. I suspect the
deadlock is somewhat different than you supposed... (and quite possibly
not a race at all, or I wouldn't be hitting it so consistently, every
time. I mean, I only need to miss it *once* and I'll have reshaped... :) )
It seems I can reproduce this on demand, so if you want to throw a patch
with piles of extra printks my way, feel free.
^ permalink raw reply
* Re: 4.11.2: reshape raid5 -> raid6 atop bcache deadlocks at start on md_attr_store / raid5_make_request
From: NeilBrown @ 2017-05-23 1:07 UTC (permalink / raw)
To: Nix; +Cc: linux-raid
In-Reply-To: <87o9ukykmk.fsf@esperi.org.uk>
[-- Attachment #1: Type: text/plain, Size: 5211 bytes --]
On Mon, May 22 2017, Nix wrote:
> [linux-bcache removed, not a bcache bug]
>
> On 22 May 2017, NeilBrown said this:
>
>> Congratulations. You have found a bug that dates from 2011.
>
> Oh so not that old then! (My personal record, in a previous job, was a
> bug in allegedly critical functionality that entirely stopped it working
> and had been broken for around fourteen years. Like hell it was
> critical.)
>
>> Commit: 68866e425be2 ("MD: no sync IO while suspended")
>>
>> (I think).
>>
>> A write request gets to raid5_make_request() before mddev_suspend() sets
>> mddev->suspended.
>> raid5_make_request() needs md_write_start() to mark the array "dirty"
>> which it asks the md thread to do, but before the thread gets to the
>> task, mddev->suspend has been set, so md_check_recovery() doesn't update
>> the superblock, so md_write_start() doesn't proceed, so the request
>> never completes, so mddev_suspend() blocks indefinitely.
>
> Ooof. Looks like it's fsync()ing something -- not sure what, though.
> The previous time it was a metadata update...
>
> Hm. I note that ->events is meant to be for things like device additions
> and array starts. RAID5 -> RAID6 reshaping appears to push this up a
> lot:
>
> Events : 1475948
>
> (it was at around 4000 before this -- itself rather mystifying, given
> that I've only rebooted this machine 27 times, and the array's probably
> been assembled less than 20 times, and was created with the same number
> of devices it has now.)
Originally, Events was for any metadata change, including
clean <-> dirty
transitions. This kept waking up spares though, so I change to
change by -1 when changing from dirty to clean, and not bother with
telling the spares. That was a little problematic too, so no
it always increments except f or dirty->clean transitions where there
are spares.
For a reshape, the reshape is checkpointed quite frequently, and each
checkpoint increases the event counter.
>
> The other array has only 92 events (it was at 30-something before I
> tried this reshape).
>
>> I think that md_check_recovery() need to test for mddev->suspended
>> somewhere else.
>> It needs to allow superblock updates, and probably needs to reap the
>> recovery thread, but must not start a new recovery thread.
>
> My question would be, is all the intricate stuff md_check_recovery() is
> doing valid on an mddev that's not only suspended but detached? Because
> we detach right after the suspension in level_store()...
mddev_detach() kills the thread. After it completes,
md_check_recovery() cannot possibly run.
However I now see that my patch was insufficient.
mddev_suspend() is called while mddev_lock() is held, and that prevents
md_check_recovery() from writing the superblock, so it can still
deadlock.
This will require more careful analysis.
>
> (btw, you probably want to remove the comment above enum array_state
> that says that "suspended" is "not supported yet", too.)
Yeah.... though it isn't supported by all personalities I think.
>
>> Probably something like this:
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index f6ae1d67bcd0..dbca31be22a1 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -8364,8 +8364,6 @@ static void md_start_sync(struct work_struct *ws)
>> */
>> void md_check_recovery(struct mddev *mddev)
>> {
>> - if (mddev->suspended)
>> - return;
>>
>> if (mddev->bitmap)
>> bitmap_daemon_work(mddev);
>> @@ -8484,6 +8482,7 @@ void md_check_recovery(struct mddev *mddev)
>> clear_bit(MD_RECOVERY_DONE, &mddev->recovery);
>>
>> if (!test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
>> + mddev->suspended ||
>> test_bit(MD_RECOVERY_FROZEN, &mddev->recovery))
>> goto not_running;
>> /* no recovery is running.
>>
>> though it's late so don't trust anything I write.
>
> This may end up clearing MD_RECOVERY_INTR and MD_RECOVERY_DONE, but I
> guess in this context it's safe to do so... looks OK otherwise to the
> totally ignorant fool reading this patch!
>
> Hm, being picky now, md_check_recovery() might be slightly easier to
> read if that governing mutex_trylock() conditional was inverted:
>
> if (mddev_trylock(mddev))
> return;
>
> Then you could de-indent most of the function by one indentation
> level...
You could send a patch if you liked.... but as it looks like I have more
work to do there, I'll probably do it.
>
>> If you try again it will almost certainly succeed. I suspect this is a
>> hard race to hit - well done!!!
>
> I'll give it a try -- I hit it twice in succession, once with a
> --backup-file, once without. Since mdadm does not warn about the lack of
> a --backup-file, I guess the statement in the manual that it is
> essential to provide one when changing RAID levels is untrue: I suspect
> that it's necessary *if* you're not increasing the number of disks at
> the same time, but since I'm growing into a spare, adding a
> --backup-file only slows it down?
>
> I might run a backup first though. :)
Always a good idea, if you can.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: 4.11.2: reshape raid5 -> raid6 atop bcache deadlocks at start on md_attr_store / raid5_make_request
From: NeilBrown @ 2017-05-23 1:20 UTC (permalink / raw)
To: Nix, Wols Lists; +Cc: linux-raid
In-Reply-To: <87k258y65b.fsf@esperi.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2548 bytes --]
On Mon, May 22 2017, Nix wrote:
> On 22 May 2017, Wols Lists verbalised:
>
>> aiui, provided you're using a v1 superblock, the data offset means there
>> is spare space on the drives precisely for the purpose (one of then at
>> least) of keeping a backup. So the reshape will start reshaping into the
>> spare space and eliminate the need for the backup - the new version of
>> the stripe will be safely written before the space occupied by the old
>> stripe is required.
>
> But it's only a few KiB by default! The amount of seeking needed to
> reshape with such a small intermediate would be fairly horrific. (It was
> bad enough as it was: the reshape of 7TiB took more than two days,
> running at under 15MiB/s, though the component drives can all handle
> 220MiB/s easily. The extra time was spent seeking to and from the
> backup, it seems.)
If the space before were "only a few KiB", it wouldn't be used.
You need at least 1 full stripe, typically more.
Current mdadm leaves several megabytes I think.
>
>> Typically, the new layout will move the data offset, pushing the data
>> block up or down.
>
> Will it? Why? You'd need to move the data offset quite a long way to
> make enough space for the 500MiB backup file I saw being created during
> my last reshape.
>
> (Though, for me, this is somewhat redundant:
>
> Data Offset : 262144 sectors
>
> because I set a data offset specifically to allow reshaping without
> having to move anything. Is the idea that reshaping that adds data
> spindles will move the data offset such that it is (still) on a chunk or
> stripe multiple? That's neat, if so, and means I wasted 128MiB on this,
> uh, 12TiB array. OK I'm not terribly blown away by this, particularly
> given that I'm wasting the same again inside the bcache partition for
> the same reason: I'm sure mdadm won't move *that* data offset.)
Data offset is always moved by a multiple of the chunk size.
When I create a 12-device raid5 on 1TB devices, then examine one of them,
it says:
Data Offset : 262144 sectors
Unused Space : before=262064 sectors, after=0 sectors
Chunk Size : 512K
so there is 130Megabytes of space per device, enough for 255 chunks.
When mdadm moves the Data Offset to allow a reshape to happen without a
backup file, it aims to use half the available space. So it would use
about 60Meg in about 120 chunks or 720Meg total across all devices.
This is more than the 500MiB backup file you saw.
NeilBrown
>
> --
> NULL && (void)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: 4.11.2: reshape raid5 -> raid6 atop bcache deadlocks at start on md_attr_store / raid5_make_request
From: NeilBrown @ 2017-05-23 1:39 UTC (permalink / raw)
To: Wols Lists, Nix; +Cc: linux-raid
In-Reply-To: <592336E6.8080906@youngman.org.uk>
[-- Attachment #1: Type: text/plain, Size: 3565 bytes --]
On Mon, May 22 2017, Wols Lists wrote:
> On 22/05/17 16:30, Nix wrote:
>> I'll give it a try -- I hit it twice in succession, once with a
>> --backup-file, once without. Since mdadm does not warn about the lack of
>> a --backup-file, I guess the statement in the manual that it is
>> essential to provide one when changing RAID levels is untrue: I suspect
>> that it's necessary *if* you're not increasing the number of disks at
>> the same time, but since I'm growing into a spare, adding a
>> --backup-file only slows it down?
>
> I did discuss this with Neil while I wrote it, so I hope I got it right :-)
>
> https://raid.wiki.kernel.org/index.php/A_guide_to_mdadm#Array_internals_and_how_it_affects_mdadm
>
> aiui, provided you're using a v1 superblock, the data offset means there
> is spare space on the drives precisely for the purpose (one of then at
> least) of keeping a backup. So the reshape will start reshaping into the
> spare space and eliminate the need for the backup - the new version of
> the stripe will be safely written before the space occupied by the old
> stripe is required.
>
> Cheers,
> Wol
Proof-reading time. I'll be very picky. You'll ignore places where you
think my pickiness isn't helpful.
> The first arrays did not have a superblock, and were declared in
> mdadm.conf.
The first arrays (linear and raid0 only) did not have a superblock and
were declared in "raidtab" which was managed by the deprecated
"raid-tools" which have been replaced by mdadm.
mdadm does now allow you to declare no-superblock arrays in mdadm.
You can build them with "mdadm --build .....", but that is all.
> and the backup area when reshaping an array.
Not quite. The whole point of being able to move the data offset
is that backup isn't needed. Maybe it could read:
... and some extra space so that the data areas can be moved forward or
backward on the device by several stripes. This is useful when
reshaping.
> All operations that involve moving data around are called reshapes,
> and require some form of backup.
All operations that involve moving data around are called reshapes and
need to be careful only to write to a region of the device which
can safely be corrupted. If a system crash happens during a reshape,
the region that was being written to must be considered to contain
garbage.
The preferred mechanism is to relocate the data by a few stripes,
reading data from a region of the disk that contains live data, and
writing it into a sliding window that is otherwise unused. After a few
stripes have been copied, the metadata in the superblock is updated, so
that the newly written data is now "live" and the location it was
copied from is now "unused" and can have more stripes copied into it.
This process results in the Data Offset being moved by several
megabytes, either forward or backward, and it is to allow for this to
happen that the Data Offset is set to several to many megabytes when an
array is created.
If it is not possible to move the Data Offset, either because there is
no room or because the v0.90 superblock is in use, mdadm will take a
backup of the region of data being reshaped before it allows the
reshape to progress. In the event of a crash, mdadm will restore this
data over the potentially corrupted region of the array before starting
the array.
Your current text already covers some of this, and there probably isn't a
need to replace it all.
But I think it is important not to talk about the reserved space in the
devices a backup space.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* (unknown),
From: mdavis @ 2017-05-23 2:19 UTC (permalink / raw)
To: linux-raid
[-- Attachment #1: 6775563555.zip --]
[-- Type: application/zip, Size: 3184 bytes --]
^ permalink raw reply
* Re: [PATCH v4] super1: fix sb->max_dev when adding a new disk in linear array
From: Lidong Zhong @ 2017-05-23 3:55 UTC (permalink / raw)
To: NeilBrown, linux-raid; +Cc: colyli, Jes.Sorensen
In-Reply-To: <87d1b1jgju.fsf@notabene.neil.brown.name>
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
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox