Re: [PATCH] md/raid5: fix locking in handle_stripe_clean_event()

[Neil Brown]

On Fri, Oct 30 2015, Roman Gushchin wrote:

> 29.10.2015, 03:35, "Neil Brown" <neilb@xxxxxxx>:
>> On Wed, Oct 28 2015, Roman Gushchin wrote:
>>
>>> ÂAfter commit 566c09c53455 ("raid5: relieve lock contention in get_active_stripe()")
>>> Â__find_stripe() is called under conf->hash_locks + hash.
>>> ÂBut handle_stripe_clean_event() calls remove_hash() under
>>> Âconf->device_lock.
>>>
>>> ÂUnder some cirscumstances the hash chain can be circuited,
>>> Âand we get an infinite loop with disabled interrupts and locked hash
>>> Âlock in __find_stripe(). This leads to hard lockup on multiple CPUs
>>> Âand following system crash.
>>>
>>> ÂI was able to reproduce this behavior on raid6 over 6 ssd disks.
>>> ÂThe devices_handle_discard_safely option should be set to enable trim
>>> Âsupport. The following script was used:
>>>
>>> Âfor i in `seq 1 32`; do
>>> ÂÂÂÂÂdd if=/dev/zero of=large$i bs=10M count=100 &
>>> Âdone
>>>
>>> ÂSigned-off-by: Roman Gushchin <klamm@xxxxxxxxxxxxxx>
>>> ÂCc: Neil Brown <neilb@xxxxxxx>
>>> ÂCc: Shaohua Li <shli@xxxxxxxxxx>
>>> ÂCc: linux-raid@xxxxxxxxxxxxxxx
>>> ÂCc: <stable@xxxxxxxxxxxxxxx> # 3.10 - 3.19
>>
>> Hi Roman,
>> Âthanks for reporting this and providing a fix.
>>
>> I'm a bit confused by that stable range: 3.10 - 3.19
>>
>> The commit you identify as introducing the bug was added in 3.13, so
>> presumably 3.10, 3.11, 3.12 are not affected.
>
> Sure, it's my mistake. Correct range is 3.13 - 3.19. Sorry.
>
>> Also the bug is still present in mainline, so 4.0, 4.1, 4.2 are also
>> affected, though the patch needs to be revised a bit for 4.1 and later.
>
> Yes, exactly, but things are a bit more complicated in mainline.
> I'll try to prepare a patch for mainline in a couple of days.
>
Thanks for the confirmation.

Isn't the 4.1 fix just:

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index e5befa356dbe..6e4350a78257 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -3522,16 +3522,16 @@ returnbi:
 		 * no updated data, so remove it from hash list and the stripe
 		 * will be reinitialized
 		 */
-		spin_lock_irq(&conf->device_lock);
 unhash:
+		spin_lock_irq(conf->hash_locks + sh->hash_lock_index);
 		remove_hash(sh);
+		spin_unlock_irq(conf->hash_locks + sh->hash_lock_index);
 		if (head_sh->batch_head) {
 			sh = list_first_entry(&sh->batch_list,
 					      struct stripe_head, batch_list);
 			if (sh != head_sh)
 					goto unhash;
 		}
-		spin_unlock_irq(&conf->device_lock);
 		sh = head_sh;
 
 		if (test_bit(STRIPE_SYNC_REQUESTED, &sh->state))

??

Or maybe
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index e5befa356dbe..704ef7fcfbf8 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -3509,6 +3509,7 @@ returnbi:
 
 	if (!discard_pending &&
 	    test_bit(R5_Discard, &sh->dev[sh->pd_idx].flags)) {
+		int hash;
 		clear_bit(R5_Discard, &sh->dev[sh->pd_idx].flags);
 		clear_bit(R5_UPTODATE, &sh->dev[sh->pd_idx].flags);
 		if (sh->qd_idx >= 0) {
@@ -3522,16 +3523,17 @@ returnbi:
 		 * no updated data, so remove it from hash list and the stripe
 		 * will be reinitialized
 		 */
-		spin_lock_irq(&conf->device_lock);
 unhash:
+		hash = sh->hash_lock_index;
+		spin_lock_irq(conf->hash_locks + hash);
 		remove_hash(sh);
+		spin_unlock_irq(conf->hash_locks + hash);
 		if (head_sh->batch_head) {
 			sh = list_first_entry(&sh->batch_list,
 					      struct stripe_head, batch_list);
 			if (sh != head_sh)
 					goto unhash;
 		}
-		spin_unlock_irq(&conf->device_lock);
 		sh = head_sh;
 
 		if (test_bit(STRIPE_SYNC_REQUESTED, &sh->state))


For personal reasons I would like to get this resolved today or
tomorrow, though it would be silly to rush if there is any uncertainty.

Thanks,
NeilBrown

Attachment: signature.asc
Description: PGP signature