md: fix raid5 livelock

PATCH In review Heinz Mauelshagen 3 participants 6 messages Jan 25, 2015 – Feb 2, 2015

Subsystems (2)

MD RAID Device mapper

lore

Share & tools

lore

Heinz Mauelshagen

author

2015-01-25 20:06 UTC #0 lore raw

PATCH 1/1 md: fix raid5 livelock

From: Heinz Mauelshagen <[email protected]>

Hi Neil,

the reconstruct write optimization in raid5, function fetch_block causes livelocks in LVM raid4/5 tests.

Test scenarios:
the tests wait for full initial array resynchronization before making a filesystem on the raid4/5 logical volume, mounting it, writing to the filesystem and failing one physical volume holding a raiddev.

In short, we're seeing livelocks on fully synchronized raid4/5 arrays with a failed device.

This patch fixes the issue but likely in a suboptimnal way.

Do you think there is a better solution to avoid livelocks on reconstruct writes?

Regards,
Heinz

Signed-off-by Heinz Mauelshagen <[email protected]>

Tested-by Jon Brassow <[email protected]>

Tested-by Heinz Mauelshagen <[email protected]>

---
 drivers/md/raid5.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/md/raid5.c +1 -1

index c1b0d52..0fc8737 100644
@@ -2915,7 +2915,7 @@ static int fetch_block(struct stripe_head *sh, struct stripe_head_state *s,
             (s->failed >= 1 && fdev[0]->toread) ||
             (s->failed >= 2 && fdev[1]->toread) ||
             (sh->raid_conf->level <= 5 && s->failed && fdev[0]->towrite &&
-             (!test_bit(R5_Insync, &dev->flags) || test_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) &&
+             (!test_bit(R5_Insync, &dev->flags) || test_bit(STRIPE_PREREAD_ACTIVE, &sh->state) || s->non_overwrite) &&
              !test_bit(R5_OVERWRITE, &fdev[0]->flags)) ||
             ((sh->raid_conf->level == 6 ||
               sh->sector >= sh->raid_conf->mddev->recovery_cp)

Signature · 1 line

2.1.0

NeilBrown

2015-01-28 02:37 UTC #1 lore raw

Quoted 46 lines · On Sun, 25 Jan 2015 21:06:20 +0100 Heinz Mauelshagen <[email protected]> wrote:

From: Heinz Mauelshagen <[email protected]>
Hi Neil,
the reconstruct write optimization in raid5, function fetch_block causes livelocks in LVM raid4/5 tests.
Test scenarios:
the tests wait for full initial array resynchronization before making a filesystem
on the raid4/5 logical volume, mounting it, writing to the filesystem and failing
one physical volume holding a raiddev.
In short, we're seeing livelocks on fully synchronized raid4/5 arrays with a failed device.
This patch fixes the issue but likely in a suboptimnal way.
Do you think there is a better solution to avoid livelocks on reconstruct writes?
Regards,
Heinz
Signed-off-by Heinz Mauelshagen <[email protected]>
Tested-by Jon Brassow <[email protected]>
Tested-by Heinz Mauelshagen <[email protected]>
---
  drivers/md/raid5.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/md/raid5.c +0 -1
index c1b0d52..0fc8737 100644
@@ -2915,7 +2915,7 @@ static int fetch_block(struct stripe_head *sh, 
 struct stripe_head_state *s,
              (s->failed >= 1 && fdev[0]->toread) ||
              (s->failed >= 2 && fdev[1]->toread) ||
              (sh->raid_conf->level <= 5 && s->failed && fdev[0]->towrite &&
-             (!test_bit(R5_Insync, &dev->flags) || 
test_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) &&
+ (!test_bit(R5_Insync, &dev->flags) ||
test_bit(STRIPE_PREREAD_ACTIVE, &sh->state) || s->non_overwrite) &&

Quoted 3 lines

               !test_bit(R5_OVERWRITE, &fdev[0]->flags)) ||
              ((sh->raid_conf->level == 6 ||
                sh->sector >= sh->raid_conf->mddev->recovery_cp)

That is a bit heavy handed, but knowing that fixes the problem helps a lot.

I think the problem happens when processes a non-overwrite write to a failed device.

fetch_block() should, in that case, pre-read all of the working device, but since

	      (!test_bit(R5_Insync, &dev->flags) || test_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) &&

was added, it sometimes doesn't. The root problem is that handle_stripe_dirtying is getting confused because neither rmw or rcw seem to work, so it doesn't start the chain of events to set STRIPE_PREREAD_ACTIVE.

The following (which is against mainline) might fix it. Can you test?

Thanks,
NeilBrown

drivers/md/raid5.c +4 -0

index c1b0d52bfcb0..793cf2861e97 100644
@@ -3195,6 +3195,10 @@ static void handle_stripe_dirtying(struct r5conf *conf,
 					  (unsigned long long)sh->sector,
 					  rcw, qread, test_bit(STRIPE_DELAYED, &sh->state));
 	}
+	if (rcw > disks && rmw > disks &&
+	    !test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
+		set_bit(STRIPE_DELAYED, &sh->state);
+
 	/* now if nothing is locked, and if we have enough data,
 	 * we can start a write request
 	 */

This code really really needs to be tidied up and commented better!!!

Thanks,
NeilBrown

Heinz Mauelshagen

author

2015-01-28 12:03 UTC #2 lore raw

Neil,

thanks for providing the patch.

Test with it will take some hours in order to tell any success.

Regards,
Heinz

Quoted 93 lines · NeilBrown

Quoted 46 lines · On Sun, 25 Jan 2015 21:06:20 +0100 Heinz Mauelshagen <[email protected]> wrote:
From: Heinz Mauelshagen <[email protected]>
Hi Neil,
the reconstruct write optimization in raid5, function fetch_block causes livelocks in LVM raid4/5 tests.
Test scenarios:
the tests wait for full initial array resynchronization before making a filesystem
on the raid4/5 logical volume, mounting it, writing to the filesystem and failing
one physical volume holding a raiddev.
In short, we're seeing livelocks on fully synchronized raid4/5 arrays with a failed device.
This patch fixes the issue but likely in a suboptimnal way.
Do you think there is a better solution to avoid livelocks on reconstruct writes?
Regards,
Heinz
Signed-off-by Heinz Mauelshagen <[email protected]>
Tested-by Jon Brassow <[email protected]>
Tested-by Heinz Mauelshagen <[email protected]>
---
   drivers/md/raid5.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
drivers/md/raid5.c +0 -1
index c1b0d52..0fc8737 100644
@@ -2915,7 +2915,7 @@ static int fetch_block(struct stripe_head *sh,
 struct stripe_head_state *s,
               (s->failed >= 1 && fdev[0]->toread) ||
               (s->failed >= 2 && fdev[1]->toread) ||
               (sh->raid_conf->level <= 5 && s->failed && fdev[0]->towrite &&
-             (!test_bit(R5_Insync, &dev->flags) ||
test_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) &&
+ (!test_bit(R5_Insync, &dev->flags) ||
test_bit(STRIPE_PREREAD_ACTIVE, &sh->state) || s->non_overwrite) &&
Quoted 3 lines
                !test_bit(R5_OVERWRITE, &fdev[0]->flags)) ||
               ((sh->raid_conf->level == 6 ||
                 sh->sector >= sh->raid_conf->mddev->recovery_cp)
That is a bit heavy handed, but knowing that fixes the problem helps a lot.
I think the problem happens when processes a non-overwrite write to a failed device.
fetch_block() should, in that case, pre-read all of the working device, but since
	      (!test_bit(R5_Insync, &dev->flags) || test_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) &&
was added, it sometimes doesn't. The root problem is that handle_stripe_dirtying is getting confused because neither rmw or rcw seem to work, so it doesn't start the chain of events to set STRIPE_PREREAD_ACTIVE.
The following (which is against mainline) might fix it. Can you test?
Thanks,
NeilBrown
drivers/md/raid5.c +4 -0
index c1b0d52bfcb0..793cf2861e97 100644
@@ -3195,6 +3195,10 @@ static void handle_stripe_dirtying(struct r5conf *conf,
  					  (unsigned long long)sh->sector,
  					  rcw, qread, test_bit(STRIPE_DELAYED, &sh->state));
  	}
+	if (rcw > disks && rmw > disks &&
+	    !test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
+		set_bit(STRIPE_DELAYED, &sh->state);
+
  	/* now if nothing is locked, and if we have enough data,
  	 * we can start a write request
  	 */
This code really really needs to be tidied up and commented better!!!
Thanks,
NeilBrown

Heinz Mauelshagen

author

2015-01-29 11:24 UTC #3 lore raw

Neil,

the patch worked fine in overnight test runs without the previous livelock. No regressions have been triggered.

Yes, tidying up that optimization logic (e.g. in fetch_block()) is very much appreciated :-)

Thanks,
Heinz

Quoted 113 lines · Heinz Mauelshagen

Neil,
thanks for providing the patch.
Test with it will take some hours in order to tell any success.
Regards,
Heinz
Quoted 97 lines · NeilBrown
Quoted 47 lines · On Sun, 25 Jan 2015 21:06:20 +0100 Heinz Mauelshagen <[email protected]> wrote:
From: Heinz Mauelshagen <[email protected]>
Hi Neil,
the reconstruct write optimization in raid5, function fetch_block causes livelocks in LVM raid4/5 tests.
Test scenarios:
the tests wait for full initial array resynchronization before making a filesystem
on the raid4/5 logical volume, mounting it, writing to the filesystem and failing
one physical volume holding a raiddev.
In short, we're seeing livelocks on fully synchronized raid4/5 arrays with a failed device.
This patch fixes the issue but likely in a suboptimnal way.
Do you think there is a better solution to avoid livelocks on reconstruct writes?
Regards,
Heinz
Signed-off-by Heinz Mauelshagen <[email protected]>
Tested-by Jon Brassow <[email protected]>
Tested-by Heinz Mauelshagen <[email protected]>
---
   drivers/md/raid5.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
drivers/md/raid5.c +0 -0
index c1b0d52..0fc8737 100644
@@ -2915,7 +2915,7 @@ static int fetch_block(struct stripe_head *sh,
 struct stripe_head_state *s,
               (s->failed >= 1 && fdev[0]->toread) ||
               (s->failed >= 2 && fdev[1]->toread) ||
               (sh->raid_conf->level <= 5 && s->failed && 
fdev[0]->towrite &&
- (!test_bit(R5_Insync, &dev->flags) ||
test_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) &&
+ (!test_bit(R5_Insync, &dev->flags) ||
test_bit(STRIPE_PREREAD_ACTIVE, &sh->state) || s->non_overwrite) &&
Quoted 3 lines
                !test_bit(R5_OVERWRITE, &fdev[0]->flags)) ||
               ((sh->raid_conf->level == 6 ||
                 sh->sector >= sh->raid_conf->mddev->recovery_cp)
That is a bit heavy handed, but knowing that fixes the problem helps a lot.
I think the problem happens when processes a non-overwrite write to a failed device.
fetch_block() should, in that case, pre-read all of the working device, but since
          (!test_bit(R5_Insync, &dev->flags) || 
test_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) &&
was added, it sometimes doesn't. The root problem is that handle_stripe_dirtying is getting confused because neither rmw or rcw seem to work, so it doesn't start the chain of events to set STRIPE_PREREAD_ACTIVE.
The following (which is against mainline) might fix it. Can you test?
Thanks,
NeilBrown
drivers/md/raid5.c +4 -0
index c1b0d52bfcb0..793cf2861e97 100644
@@ -3195,6 +3195,10 @@ static void handle_stripe_dirtying(struct 
 r5conf *conf,
                        (unsigned long long)sh->sector,
                        rcw, qread, test_bit(STRIPE_DELAYED, 
 &sh->state));
      }
+    if (rcw > disks && rmw > disks &&
+        !test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
+        set_bit(STRIPE_DELAYED, &sh->state);
+
      /* now if nothing is locked, and if we have enough data,
* we can start a write request
*/
This code really really needs to be tidied up and commented better!!!
Thanks,
NeilBrown
Signature · 3 lines
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

NeilBrown

Tested-by Heinz Mauelshagen

2015-02-02 00:06 UTC #4 lore raw

Quoted 9 lines · On Thu, 29 Jan 2015 12:24:00 +0100 Heinz Mauelshagen <[email protected]> wrote:

Neil,
the patch worked fine in overnight test runs without the previous livelock. No regressions have been triggered.
Yes, tidying up that optimization logic (e.g. in fetch_block()) is very much appreciated :-)

Thanks!
The following is what should appear in -next soonish. If there are any *-by:
tags to be added or changed, please let me know.

NeilBrown

From: NeilBrown <[email protected]>
Date: Mon, 2 Feb 2015 10:44:29 +1100
Subject: [PATCH] md/raid5: fix another livelock caused by non-aligned writes.

If a non-page-aligned write is destined for a device which is missing/faulty, we can deadlock.

As the target device is missing, a read-modify-write cycle is not possible.
As the write is not for a full-page, a recontruct-write cycle is not possible.

This should be handled by logic in fetch_block() which notices there is a non-R5_OVERWRITE write to a missing device, and so loads all blocks.

However since commit 67f455486d2ea2, that code requires STRIPE_PREREAD_ACTIVE before it will active, and those circumstances never set STRIPE_PREREAD_ACTIVE.

So: in handle_stripe_dirtying, if neither rmw or rcw was possible, set STRIPE_DELAYED, which will cause STRIPE_PREREAD_ACTIVE be set after a suitable delay.

Fixes 67f455486d2ea20b2d94d6adf5b9b783d079e321

Cc [email protected] (v3.16+)

Reported-by Mikulas Patocka <[email protected]>

Tested-by Heinz Mauelshagen <[email protected]>

Signed-off-by NeilBrown <[email protected]>

drivers/md/raid5.c +5 -0

index 41494d904859..274db1834d43 100644
@@ -3192,6 +3192,11 @@ static void handle_stripe_dirtying(struct r5conf *conf,
 					  (unsigned long long)sh->sector,
 					  rcw, qread, test_bit(STRIPE_DELAYED, &sh->state));
 	}
+
+	if (rcw > disks && rmw > disks &&
+	    !test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
+		set_bit(STRIPE_DELAYED, &sh->state);
+
 	/* now if nothing is locked, and if we have enough data,
 	 * we can start a write request
 	 */

Jes Sorensen

2015-01-29 17:17 UTC #5 lore raw

In reply to NeilBrown

Quoted 90 lines · NeilBrown

Quoted 46 lines · On Sun, 25 Jan 2015 21:06:20 +0100 Heinz Mauelshagen <[email protected]> wrote:
From: Heinz Mauelshagen <[email protected]>
Hi Neil,
the reconstruct write optimization in raid5, function fetch_block causes livelocks in LVM raid4/5 tests.
Test scenarios:
the tests wait for full initial array resynchronization before making a filesystem
on the raid4/5 logical volume, mounting it, writing to the filesystem and failing
one physical volume holding a raiddev.
In short, we're seeing livelocks on fully synchronized raid4/5 arrays with a failed device.
This patch fixes the issue but likely in a suboptimnal way.
Do you think there is a better solution to avoid livelocks on reconstruct writes?
Regards,
Heinz
Signed-off-by Heinz Mauelshagen <[email protected]>
Tested-by Jon Brassow <[email protected]>
Tested-by Heinz Mauelshagen <[email protected]>
---
  drivers/md/raid5.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/md/raid5.c +0 -1
index c1b0d52..0fc8737 100644
@@ -2915,7 +2915,7 @@ static int fetch_block(struct stripe_head *sh, 
 struct stripe_head_state *s,
              (s->failed >= 1 && fdev[0]->toread) ||
              (s->failed >= 2 && fdev[1]->toread) ||
              (sh->raid_conf->level <= 5 && s->failed && fdev[0]->towrite &&
-             (!test_bit(R5_Insync, &dev->flags) || 
test_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) &&
+ (!test_bit(R5_Insync, &dev->flags) ||
test_bit(STRIPE_PREREAD_ACTIVE, &sh->state) || s->non_overwrite) &&
Quoted 3 lines
               !test_bit(R5_OVERWRITE, &fdev[0]->flags)) ||
              ((sh->raid_conf->level == 6 ||
                sh->sector >= sh->raid_conf->mddev->recovery_cp)
That is a bit heavy handed, but knowing that fixes the problem helps a lot.
I think the problem happens when processes a non-overwrite write to a failed device.
fetch_block() should, in that case, pre-read all of the working device, but since
	      (!test_bit(R5_Insync, &dev->flags) || test_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) &&
was added, it sometimes doesn't. The root problem is that handle_stripe_dirtying is getting confused because neither rmw or rcw seem to work, so it doesn't start the chain of events to set STRIPE_PREREAD_ACTIVE.
The following (which is against mainline) might fix it. Can you test?
Thanks,
NeilBrown
drivers/md/raid5.c +4 -0
index c1b0d52bfcb0..793cf2861e97 100644
@@ -3195,6 +3195,10 @@ static void handle_stripe_dirtying(struct r5conf *conf,
 					  (unsigned long long)sh->sector,
 					  rcw, qread, test_bit(STRIPE_DELAYED, &sh->state));
 	}
+	if (rcw > disks && rmw > disks &&
+	    !test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
+		set_bit(STRIPE_DELAYED, &sh->state);
+
 	/* now if nothing is locked, and if we have enough data,
 	 * we can start a write request
 	 */

Quoted 1 line

This code really really needs to be tidied up and commented better!!!

Neil,

Since this one seems to do the trick, will you be pushing it into you tree anytime soon?

Cheers,
Jes