[PATCH] dm-raid: check events in super_validate

[PATCH] dm-raid: check events in super_validate

[Nate Dailey]

If an LVM raid1 recovery is interrupted by deactivating the LV, when the 
LV is reactivated it comes up with both members in sync--the recovery 
never completes.

I've been trying to figure out how to fix this. Does this approach look 
okay? I'm not sure what else to use to determine that a member disk is 
out of sync. It looks like if disk_recovery_offset in the superblock 
were updated during the recovery, that would also cause it to resume 
after interruption--but MD skips the recovery target disk when writing 
superblocks, so this doesn't work.

Comments?

Thanks,

Nate Dailey
Stratus Technologies



diff -Nupr linux-3.12.9.orig/drivers/md/dm-raid.c 
linux-3.12.9/drivers/md/dm-raid.c
--- linux-3.12.9.orig/drivers/md/dm-raid.c    2014-02-01 
08:46:51.088086299 -0500
+++ linux-3.12.9/drivers/md/dm-raid.c    2014-02-01 09:02:06.657149550 -0500
@@ -1042,6 +1042,21 @@ static int super_validate(struct mddev *
          rdev->recovery_offset = le64_to_cpu(sb->disk_recovery_offset);
          if (rdev->recovery_offset != MaxSector)
              clear_bit(In_sync, &rdev->flags);
+        else if (!test_bit(Faulty, &rdev->flags)) {
+            uint64_t events_sb;
+
+            /*
+             * Trigger recovery if events is out-of-date.
+             */
+            events_sb = le64_to_cpu(sb->events);
+            if (events_sb < mddev->events) {
+                DMINFO("Force recovery on out-of-date device #%d.",
+                       rdev->raid_disk);
+                clear_bit(In_sync, &rdev->flags);
+                rdev->saved_raid_disk = rdev->raid_disk;
+                rdev->recovery_offset = 0;
+            }
+        }
      }

      /*

Re: [PATCH] dm-raid: check events in super_validate

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2475 bytes --]

On Sat, 1 Feb 2014 09:35:20 -0500 Nate Dailey <nate.dailey@stratus.com> wrote:

> If an LVM raid1 recovery is interrupted by deactivating the LV, when the 
> LV is reactivated it comes up with both members in sync--the recovery 
> never completes.
> 
> I've been trying to figure out how to fix this. Does this approach look 
> okay? I'm not sure what else to use to determine that a member disk is 
> out of sync. It looks like if disk_recovery_offset in the superblock 
> were updated during the recovery, that would also cause it to resume 
> after interruption--but MD skips the recovery target disk when writing 
> superblocks, so this doesn't work.
> 
> Comments?

I know it is confusing, but this should really have gone to dm-devel rather
than linux-raid, to make sure Jon Brassow see it (hi Jon!).

Setting recovery_offset to 0 certainly looks wrong, it should be set to
  sb->disk_recovery_offset
like the code just above your change.
Why does the code there not meet your need.

Jon: can you help?

NeilBrown

> 
> Thanks,
> 
> Nate Dailey
> Stratus Technologies
> 
> 
> 
> diff -Nupr linux-3.12.9.orig/drivers/md/dm-raid.c 
> linux-3.12.9/drivers/md/dm-raid.c
> --- linux-3.12.9.orig/drivers/md/dm-raid.c    2014-02-01 
> 08:46:51.088086299 -0500
> +++ linux-3.12.9/drivers/md/dm-raid.c    2014-02-01 09:02:06.657149550 -0500
> @@ -1042,6 +1042,21 @@ static int super_validate(struct mddev *
>           rdev->recovery_offset = le64_to_cpu(sb->disk_recovery_offset);
>           if (rdev->recovery_offset != MaxSector)
>               clear_bit(In_sync, &rdev->flags);
> +        else if (!test_bit(Faulty, &rdev->flags)) {
> +            uint64_t events_sb;
> +
> +            /*
> +             * Trigger recovery if events is out-of-date.
> +             */
> +            events_sb = le64_to_cpu(sb->events);
> +            if (events_sb < mddev->events) {
> +                DMINFO("Force recovery on out-of-date device #%d.",
> +                       rdev->raid_disk);
> +                clear_bit(In_sync, &rdev->flags);
> +                rdev->saved_raid_disk = rdev->raid_disk;
> +                rdev->recovery_offset = 0;
> +            }
> +        }
>       }
> 
>       /*
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] dm-raid: check events in super_validate

[Brassow Jonathan]

On Feb 24, 2014, at 11:30 PM, NeilBrown wrote:

> On Sat, 1 Feb 2014 09:35:20 -0500 Nate Dailey <nate.dailey@stratus.com> wrote:
> 
>> If an LVM raid1 recovery is interrupted by deactivating the LV, when the 
>> LV is reactivated it comes up with both members in sync--the recovery 
>> never completes.
>> 
>> I've been trying to figure out how to fix this. Does this approach look 
>> okay? I'm not sure what else to use to determine that a member disk is 
>> out of sync. It looks like if disk_recovery_offset in the superblock 
>> were updated during the recovery, that would also cause it to resume 
>> after interruption--but MD skips the recovery target disk when writing 
>> superblocks, so this doesn't work.
>> 
>> Comments?
> 
> I know it is confusing, but this should really have gone to dm-devel rather
> than linux-raid, to make sure Jon Brassow see it (hi Jon!).
> 
> Setting recovery_offset to 0 certainly looks wrong, it should be set to
>  sb->disk_recovery_offset
> like the code just above your change.
> Why does the code there not meet your need.
> 
> Jon: can you help?

Sure, thanks for forwarding.

Could you describe first how you are creating the problem?

When I create a RAID1 LV, deactivate it, and reactivate it; I don't see it skip the sync.  Also, if I replace a single drive and cycle the LV, I don't see it skip the sync.  What steps am I missing?

 brassow

Re: [PATCH] dm-raid: check events in super_validate

[Nate Dailey]

Here's what I've done to reproduce this:

- remove a disk containing one leg of an LVM raid1 mirror
- do enough IO that a lengthy recovery will be required
- insert the removed disk
- let recovery begin, but deactivate the LV before it completes
- activate the LV

This is the point where the recovery should start back up, but it 
doesn't. I haven't tried this in a few weeks, but am happy to try it 
again if it would help.

Nate




On 02/25/2014 05:13 PM, Brassow Jonathan wrote:
> On Feb 24, 2014, at 11:30 PM, NeilBrown wrote:
>
>> On Sat, 1 Feb 2014 09:35:20 -0500 Nate Dailey <nate.dailey@stratus.com> wrote:
>>
>>> If an LVM raid1 recovery is interrupted by deactivating the LV, when the
>>> LV is reactivated it comes up with both members in sync--the recovery
>>> never completes.
>>>
>>> I've been trying to figure out how to fix this. Does this approach look
>>> okay? I'm not sure what else to use to determine that a member disk is
>>> out of sync. It looks like if disk_recovery_offset in the superblock
>>> were updated during the recovery, that would also cause it to resume
>>> after interruption--but MD skips the recovery target disk when writing
>>> superblocks, so this doesn't work.
>>>
>>> Comments?
>> I know it is confusing, but this should really have gone to dm-devel rather
>> than linux-raid, to make sure Jon Brassow see it (hi Jon!).
>>
>> Setting recovery_offset to 0 certainly looks wrong, it should be set to
>>   sb->disk_recovery_offset
>> like the code just above your change.
>> Why does the code there not meet your need.
>>
>> Jon: can you help?
> Sure, thanks for forwarding.
>
> Could you describe first how you are creating the problem?
>
> When I create a RAID1 LV, deactivate it, and reactivate it; I don't see it skip the sync.  Also, if I replace a single drive and cycle the LV, I don't see it skip the sync.  What steps am I missing?
>
>   brassow
>

Re: [PATCH] dm-raid: check events in super_validate

[Brassow Jonathan]

On Feb 25, 2014, at 4:22 PM, Nate Dailey wrote:

> Here's what I've done to reproduce this:
> 
> - remove a disk containing one leg of an LVM raid1 mirror
> - do enough IO that a lengthy recovery will be required
> - insert the removed disk
> - let recovery begin, but deactivate the LV before it completes
> - activate the LV
> 
> This is the point where the recovery should start back up, but it doesn't. I haven't tried this in a few weeks, but am happy to try it again if it would help.

Confirmed (test output below).  I'll get started on this.  This code can be a bit tricky and I've been away from it for a while.  It will take me a bit to re-familiarize myself with it and review your patch.

thanks,
 brassow


[root@bp-02 ~]# lvs -a -o name,attr,size,sync_percent vg
  LV               Attr       LSize   Cpy%Sync
  raid1            Rwi-a-r--- 300.00g   100.00
  [raid1_rimage_0] iwi-aor--- 300.00g         
  [raid1_rimage_1] iwi-aor--- 300.00g         
  [raid1_rmeta_0]  ewi-aor---   4.00m         
  [raid1_rmeta_1]  ewi-aor---   4.00m         
[root@bp-02 ~]# off.sh sda
Turning off sda
[root@bp-02 ~]# dd if=/dev/zero of=/dev/vg/raid1 bs=4M count=10000
10000+0 records in
10000+0 records out
41943040000 bytes (42 GB) copied, 147.139 s, 285 MB/s
[root@bp-02 ~]# on.sh sda
Turning on sda
[root@bp-02 ~]# lvs -a -o name,attr,size,sync_percent vg
  LV               Attr       LSize   Cpy%Sync
  raid1            Rwi-a-r-r- 300.00g   100.00
  [raid1_rimage_0] iwi-aor-r- 300.00g         
  [raid1_rimage_1] iwi-aor--- 300.00g         
  [raid1_rmeta_0]  ewi-aor-r-   4.00m         
  [raid1_rmeta_1]  ewi-aor---   4.00m         
[root@bp-02 ~]# lvchange --refresh vg/raid1
[root@bp-02 ~]# lvs -a -o name,attr,size,sync_percent vg
  LV               Attr       LSize   Cpy%Sync
  raid1            Rwi-a-r--- 300.00g     0.00
  [raid1_rimage_0] Iwi-aor--- 300.00g         
  [raid1_rimage_1] iwi-aor--- 300.00g         
  [raid1_rmeta_0]  ewi-aor---   4.00m         
  [raid1_rmeta_1]  ewi-aor---   4.00m         
[root@bp-02 ~]# lvs -a -o name,attr,size,sync_percent vg
  LV               Attr       LSize   Cpy%Sync
  raid1            Rwi-a-r--- 300.00g     0.29
  [raid1_rimage_0] Iwi-aor--- 300.00g         
  [raid1_rimage_1] iwi-aor--- 300.00g         
  [raid1_rmeta_0]  ewi-aor---   4.00m         
  [raid1_rmeta_1]  ewi-aor---   4.00m         
[root@bp-02 ~]# lvchange -an vg
[root@bp-02 ~]# lvchange -ay vg; lvs -a -o name,attr,size,sync_percent vg
  LV               Attr       LSize   Cpy%Sync
  raid1            Rwi-a-r--- 300.00g   100.00
  [raid1_rimage_0] iwi-aor--- 300.00g         
  [raid1_rimage_1] iwi-aor--- 300.00g         
  [raid1_rmeta_0]  ewi-aor---   4.00m         
  [raid1_rmeta_1]  ewi-aor---   4.00m         

Re: [dm-devel] [PATCH] dm-raid: check events in super_validate

[Brassow Jonathan]

On Feb 25, 2014, at 4:59 PM, Brassow Jonathan wrote:

> 
> On Feb 25, 2014, at 4:22 PM, Nate Dailey wrote:
> 
>> Here's what I've done to reproduce this:
>> 
>> - remove a disk containing one leg of an LVM raid1 mirror
>> - do enough IO that a lengthy recovery will be required
>> - insert the removed disk
>> - let recovery begin, but deactivate the LV before it completes
>> - activate the LV
>> 
>> This is the point where the recovery should start back up, but it doesn't. I haven't tried this in a few weeks, but am happy to try it again if it would help.
> 
> Confirmed (test output below).  I'll get started on this.  This code can be a bit tricky and I've been away from it for a while.  It will take me a bit to re-familiarize myself with it and review your patch.

I've tested this again with the latest code from upstream (kernel 3.14.0-rc4) and I cannot reproduce the problem there.  I'll see if I can find the last non-working version...

 brassow

Re: [dm-devel] [PATCH] dm-raid: check events in super_validate

[Nate Dailey]

Interesting. I had tried this with the latest stable kernel.org kernel 
(as of a month or so ago) and still hit it.

I'll mention that the initial resync on creating the raid1 can be 
interrupted okay; the problem only happens after that completes, and a 
disk is removed and re-added.

Nate



On 02/26/2014 05:21 PM, Brassow Jonathan wrote:
> On Feb 25, 2014, at 4:59 PM, Brassow Jonathan wrote:
>
>> On Feb 25, 2014, at 4:22 PM, Nate Dailey wrote:
>>
>>> Here's what I've done to reproduce this:
>>>
>>> - remove a disk containing one leg of an LVM raid1 mirror
>>> - do enough IO that a lengthy recovery will be required
>>> - insert the removed disk
>>> - let recovery begin, but deactivate the LV before it completes
>>> - activate the LV
>>>
>>> This is the point where the recovery should start back up, but it doesn't. I haven't tried this in a few weeks, but am happy to try it again if it would help.
>> Confirmed (test output below).  I'll get started on this.  This code can be a bit tricky and I've been away from it for a while.  It will take me a bit to re-familiarize myself with it and review your patch.
> I've tested this again with the latest code from upstream (kernel 3.14.0-rc4) and I cannot reproduce the problem there.  I'll see if I can find the last non-working version...
>
>   brassow
>

Re: [dm-devel] [PATCH] dm-raid: check events in super_validate

[Brassow Jonathan]

The commit that fixed the problem was:
f466722ca614edcd14f3337373f33132117c7612
f466722 md: Change handling of save_raid_disk and metadata update during recover

I don't know if this is safe to independently apply to previous kernels or not yet.  Neil, do you know?

I have applied it directly to the 3.13 kernel and it solves the problem.  However, applying it to the 3.12 kernel does not solve the problem.  There is obviously something else I'm missing or the fuzziness of the patch on 3.12 is causing things to not behave as expected.

 brassow


On Feb 26, 2014, at 6:17 PM, Nate Dailey wrote:

> Interesting. I had tried this with the latest stable kernel.org kernel (as of a month or so ago) and still hit it.
> 
> I'll mention that the initial resync on creating the raid1 can be interrupted okay; the problem only happens after that completes, and a disk is removed and re-added.
> 
> Nate
> 
> 
> 
> On 02/26/2014 05:21 PM, Brassow Jonathan wrote:
>> On Feb 25, 2014, at 4:59 PM, Brassow Jonathan wrote:
>> 
>>> On Feb 25, 2014, at 4:22 PM, Nate Dailey wrote:
>>> 
>>>> Here's what I've done to reproduce this:
>>>> 
>>>> - remove a disk containing one leg of an LVM raid1 mirror
>>>> - do enough IO that a lengthy recovery will be required
>>>> - insert the removed disk
>>>> - let recovery begin, but deactivate the LV before it completes
>>>> - activate the LV
>>>> 
>>>> This is the point where the recovery should start back up, but it doesn't. I haven't tried this in a few weeks, but am happy to try it again if it would help.
>>> Confirmed (test output below).  I'll get started on this.  This code can be a bit tricky and I've been away from it for a while.  It will take me a bit to re-familiarize myself with it and review your patch.
>> I've tested this again with the latest code from upstream (kernel 3.14.0-rc4) and I cannot reproduce the problem there.  I'll see if I can find the last non-working version...
>> 
>>  brassow
>> 
> 

Re: [dm-devel] [PATCH] dm-raid: check events in super_validate

[Brassow Jonathan]

On Feb 28, 2014, at 2:40 PM, Brassow Jonathan wrote:

> The commit that fixed the problem was:
> f466722ca614edcd14f3337373f33132117c7612
> f466722 md: Change handling of save_raid_disk and metadata update during recover
> 
> I don't know if this is safe to independently apply to previous kernels or not yet.  Neil, do you know?
> 
> I have applied it directly to the 3.13 kernel and it solves the problem.  However, applying it to the 3.12 kernel does not solve the problem.  There is obviously something else I'm missing or the fuzziness of the patch on 3.12 is causing things to not behave as expected.

I was wrong.  After retesting, this commit /does/ fix the problem.  I've tested it all the way back to 3.10 and it fixes the problem.

Neil, would you be willing to have this patch go to stable?

 brassow