[PATCH 1/2] raid5: Use raw_spinlock for stripe management.

[PATCH 1/2] raid5: Use raw_spinlock for stripe management.

[Alexander GQ Gerasiov]

From: Alexander GQ Gerasiov <gq@redlab-i.ru>

This commit fixes the following warning:

[ ] BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:993
[ ] in_atomic(): 0, irqs_disabled(): 1, pid: 58, name: kworker/u12:1
[ ] CPU: 5 PID: 58 Comm: kworker/u12:1 Tainted: G        W       4.9.20-rt16-stand6-686 #1
[ ] Hardware name: Supermicro SYS-5027R-WRF/X9SRW-F, BIOS 3.2a 10/28/2015
[ ] Workqueue: writeback wb_workfn (flush-253:0)
[ ]  edf21a34 c12cdc6f c1078f0a edf08c40 edf21a64 c10787c1 c1668778 00000000
[ ]  00000001 0000003a edf09144 00000000 80000000 ec694b18 ec775748 ec694ad0
[ ]  edf21a70 c1584587 ec775790 edf21ac4 f893e8a3 00000000 c1078e52 00000000
[ ] Call Trace:
[ ]  [<c12cdc6f>] dump_stack+0x47/0x68
[ ]  [<c1078f0a>] ? migrate_enable+0x4a/0xf0
[ ]  [<c10787c1>] ___might_sleep+0x101/0x180
[ ]  [<c1584587>] rt_spin_lock+0x17/0x40
[ ]  [<f893e8a3>] add_stripe_bio+0x4e3/0x6c0 [raid456]
[ ]  [<c1078e52>] ? preempt_count_add+0x42/0xb0
[ ]  [<f89408f7>] raid5_make_request+0x737/0xdd0 [raid456]

Signed-off-by: Alexander GQ Gerasiov <gq@redlab-i.ru>
---
 drivers/md/raid5.c | 82 +++++++++++++++++++++++++++---------------------------
 drivers/md/raid5.h |  4 +--
 2 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index fa2c4de32a64..e69525b5db7d 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -734,18 +734,18 @@ static void lock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
 {
 	local_irq_disable();
 	if (sh1 > sh2) {
-		spin_lock(&sh2->stripe_lock);
-		spin_lock_nested(&sh1->stripe_lock, 1);
+		raw_spin_lock(&sh2->stripe_lock);
+		raw_spin_lock_nested(&sh1->stripe_lock, 1);
 	} else {
-		spin_lock(&sh1->stripe_lock);
-		spin_lock_nested(&sh2->stripe_lock, 1);
+		raw_spin_lock(&sh1->stripe_lock);
+		raw_spin_lock_nested(&sh2->stripe_lock, 1);
 	}
 }
 
 static void unlock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
 {
-	spin_unlock(&sh1->stripe_lock);
-	spin_unlock(&sh2->stripe_lock);
+	raw_spin_unlock(&sh1->stripe_lock);
+	raw_spin_unlock(&sh2->stripe_lock);
 	local_irq_enable();
 }
 
@@ -823,10 +823,10 @@ static void stripe_add_to_batch_list(struct r5conf *conf, struct stripe_head *sh
 		goto unlock_out;
 
 	if (head->batch_head) {
-		spin_lock(&head->batch_head->batch_lock);
+		raw_spin_lock(&head->batch_head->batch_lock);
 		/* This batch list is already running */
 		if (!stripe_can_batch(head)) {
-			spin_unlock(&head->batch_head->batch_lock);
+			raw_spin_unlock(&head->batch_head->batch_lock);
 			goto unlock_out;
 		}
 
@@ -835,15 +835,15 @@ static void stripe_add_to_batch_list(struct r5conf *conf, struct stripe_head *sh
 		 * can still add the stripe to batch list
 		 */
 		list_add(&sh->batch_list, &head->batch_list);
-		spin_unlock(&head->batch_head->batch_lock);
+		raw_spin_unlock(&head->batch_head->batch_lock);
 
 		sh->batch_head = head->batch_head;
 	} else {
 		head->batch_head = head;
 		sh->batch_head = head->batch_head;
-		spin_lock(&head->batch_lock);
+		raw_spin_lock(&head->batch_lock);
 		list_add_tail(&sh->batch_list, &head->batch_list);
-		spin_unlock(&head->batch_lock);
+		raw_spin_unlock(&head->batch_lock);
 	}
 
 	if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
@@ -1230,10 +1230,10 @@ static void ops_run_biofill(struct stripe_head *sh)
 		struct r5dev *dev = &sh->dev[i];
 		if (test_bit(R5_Wantfill, &dev->flags)) {
 			struct bio *rbi;
-			spin_lock_irq(&sh->stripe_lock);
+			raw_spin_lock_irq(&sh->stripe_lock);
 			dev->read = rbi = dev->toread;
 			dev->toread = NULL;
-			spin_unlock_irq(&sh->stripe_lock);
+			raw_spin_unlock_irq(&sh->stripe_lock);
 			while (rbi && rbi->bi_iter.bi_sector <
 				dev->sector + STRIPE_SECTORS) {
 				tx = async_copy_data(0, rbi, &dev->page,
@@ -1618,13 +1618,13 @@ ops_run_biodrain(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)
 
 again:
 			dev = &sh->dev[i];
-			spin_lock_irq(&sh->stripe_lock);
+			raw_spin_lock_irq(&sh->stripe_lock);
 			chosen = dev->towrite;
 			dev->towrite = NULL;
 			sh->overwrite_disks = 0;
 			BUG_ON(dev->written);
 			wbi = dev->written = chosen;
-			spin_unlock_irq(&sh->stripe_lock);
+			raw_spin_unlock_irq(&sh->stripe_lock);
 			WARN_ON(dev->page != dev->orig_page);
 
 			while (wbi && wbi->bi_iter.bi_sector <
@@ -1998,8 +1998,8 @@ static struct stripe_head *alloc_stripe(struct kmem_cache *sc, gfp_t gfp,
 
 	sh = kmem_cache_zalloc(sc, gfp);
 	if (sh) {
-		spin_lock_init(&sh->stripe_lock);
-		spin_lock_init(&sh->batch_lock);
+		raw_spin_lock_init(&sh->stripe_lock);
+		raw_spin_lock_init(&sh->batch_lock);
 		INIT_LIST_HEAD(&sh->batch_list);
 		INIT_LIST_HEAD(&sh->lru);
 		atomic_set(&sh->count, 1);
@@ -2984,7 +2984,7 @@ static int add_stripe_bio(struct stripe_head *sh, struct bio *bi, int dd_idx,
 	 * stripe. If a stripe is owned by one stripe, the stripe lock will
 	 * protect it.
 	 */
-	spin_lock_irq(&sh->stripe_lock);
+	raw_spin_lock_irq(&sh->stripe_lock);
 	/* Don't allow new IO added to stripes in batch list */
 	if (sh->batch_head)
 		goto overlap;
@@ -3044,17 +3044,17 @@ static int add_stripe_bio(struct stripe_head *sh, struct bio *bi, int dd_idx,
 		 * any more.
 		 */
 		set_bit(STRIPE_BITMAP_PENDING, &sh->state);
-		spin_unlock_irq(&sh->stripe_lock);
+		raw_spin_unlock_irq(&sh->stripe_lock);
 		bitmap_startwrite(conf->mddev->bitmap, sh->sector,
 				  STRIPE_SECTORS, 0);
-		spin_lock_irq(&sh->stripe_lock);
+		raw_spin_lock_irq(&sh->stripe_lock);
 		clear_bit(STRIPE_BITMAP_PENDING, &sh->state);
 		if (!sh->batch_head) {
 			sh->bm_seq = conf->seq_flush+1;
 			set_bit(STRIPE_BIT_DELAY, &sh->state);
 		}
 	}
-	spin_unlock_irq(&sh->stripe_lock);
+	raw_spin_unlock_irq(&sh->stripe_lock);
 
 	if (stripe_can_batch(sh))
 		stripe_add_to_batch_list(conf, sh);
@@ -3062,7 +3062,7 @@ static int add_stripe_bio(struct stripe_head *sh, struct bio *bi, int dd_idx,
 
  overlap:
 	set_bit(R5_Overlap, &sh->dev[dd_idx].flags);
-	spin_unlock_irq(&sh->stripe_lock);
+	raw_spin_unlock_irq(&sh->stripe_lock);
 	return 0;
 }
 
@@ -3114,12 +3114,12 @@ handle_failed_stripe(struct r5conf *conf, struct stripe_head *sh,
 				rdev_dec_pending(rdev, conf->mddev);
 			}
 		}
-		spin_lock_irq(&sh->stripe_lock);
+		raw_spin_lock_irq(&sh->stripe_lock);
 		/* fail all writes first */
 		bi = sh->dev[i].towrite;
 		sh->dev[i].towrite = NULL;
 		sh->overwrite_disks = 0;
-		spin_unlock_irq(&sh->stripe_lock);
+		raw_spin_unlock_irq(&sh->stripe_lock);
 		if (bi)
 			bitmap_end = 1;
 
@@ -3171,10 +3171,10 @@ handle_failed_stripe(struct r5conf *conf, struct stripe_head *sh,
 		    s->failed > conf->max_degraded &&
 		    (!test_bit(R5_Insync, &sh->dev[i].flags) ||
 		      test_bit(R5_ReadError, &sh->dev[i].flags))) {
-			spin_lock_irq(&sh->stripe_lock);
+			raw_spin_lock_irq(&sh->stripe_lock);
 			bi = sh->dev[i].toread;
 			sh->dev[i].toread = NULL;
-			spin_unlock_irq(&sh->stripe_lock);
+			raw_spin_unlock_irq(&sh->stripe_lock);
 			if (test_and_clear_bit(R5_Overlap, &sh->dev[i].flags))
 				wake_up(&conf->wait_for_overlap);
 			if (bi)
@@ -4219,9 +4219,9 @@ static int clear_batch_ready(struct stripe_head *sh)
 	struct stripe_head *tmp;
 	if (!test_and_clear_bit(STRIPE_BATCH_READY, &sh->state))
 		return (sh->batch_head && sh->batch_head != sh);
-	spin_lock(&sh->stripe_lock);
+	raw_spin_lock(&sh->stripe_lock);
 	if (!sh->batch_head) {
-		spin_unlock(&sh->stripe_lock);
+		raw_spin_unlock(&sh->stripe_lock);
 		return 0;
 	}
 
@@ -4230,14 +4230,14 @@ static int clear_batch_ready(struct stripe_head *sh)
 	 * BATCH_READY, skips it
 	 */
 	if (sh->batch_head != sh) {
-		spin_unlock(&sh->stripe_lock);
+		raw_spin_unlock(&sh->stripe_lock);
 		return 1;
 	}
-	spin_lock(&sh->batch_lock);
+	raw_spin_lock(&sh->batch_lock);
 	list_for_each_entry(tmp, &sh->batch_list, batch_list)
 		clear_bit(STRIPE_BATCH_READY, &tmp->state);
-	spin_unlock(&sh->batch_lock);
-	spin_unlock(&sh->stripe_lock);
+	raw_spin_unlock(&sh->batch_lock);
+	raw_spin_unlock(&sh->stripe_lock);
 
 	/*
 	 * BATCH_READY is cleared, no new stripes can be added.
@@ -4288,17 +4288,17 @@ static void break_stripe_batch_list(struct stripe_head *head_sh,
 			sh->dev[i].flags = head_sh->dev[i].flags &
 				(~((1 << R5_WriteError) | (1 << R5_Overlap)));
 		}
-		spin_lock_irq(&sh->stripe_lock);
+		raw_spin_lock_irq(&sh->stripe_lock);
 		sh->batch_head = NULL;
-		spin_unlock_irq(&sh->stripe_lock);
+		raw_spin_unlock_irq(&sh->stripe_lock);
 		if (handle_flags == 0 ||
 		    sh->state & handle_flags)
 			set_bit(STRIPE_HANDLE, &sh->state);
 		raid5_release_stripe(sh);
 	}
-	spin_lock_irq(&head_sh->stripe_lock);
+	raw_spin_lock_irq(&head_sh->stripe_lock);
 	head_sh->batch_head = NULL;
-	spin_unlock_irq(&head_sh->stripe_lock);
+	raw_spin_unlock_irq(&head_sh->stripe_lock);
 	for (i = 0; i < head_sh->disks; i++)
 		if (test_and_clear_bit(R5_Overlap, &head_sh->dev[i].flags))
 			do_wakeup = 1;
@@ -4335,7 +4335,7 @@ static void handle_stripe(struct stripe_head *sh)
 		break_stripe_batch_list(sh, 0);
 
 	if (test_bit(STRIPE_SYNC_REQUESTED, &sh->state) && !sh->batch_head) {
-		spin_lock(&sh->stripe_lock);
+		raw_spin_lock(&sh->stripe_lock);
 		/* Cannot process 'sync' concurrently with 'discard' */
 		if (!test_bit(STRIPE_DISCARD, &sh->state) &&
 		    test_and_clear_bit(STRIPE_SYNC_REQUESTED, &sh->state)) {
@@ -4343,7 +4343,7 @@ static void handle_stripe(struct stripe_head *sh)
 			clear_bit(STRIPE_INSYNC, &sh->state);
 			clear_bit(STRIPE_REPLACED, &sh->state);
 		}
-		spin_unlock(&sh->stripe_lock);
+		raw_spin_unlock(&sh->stripe_lock);
 	}
 	clear_bit(STRIPE_DELAYED, &sh->state);
 
@@ -5113,13 +5113,13 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
 			goto again;
 		}
 		clear_bit(R5_Overlap, &sh->dev[sh->pd_idx].flags);
-		spin_lock_irq(&sh->stripe_lock);
+		raw_spin_lock_irq(&sh->stripe_lock);
 		for (d = 0; d < conf->raid_disks; d++) {
 			if (d == sh->pd_idx || d == sh->qd_idx)
 				continue;
 			if (sh->dev[d].towrite || sh->dev[d].toread) {
 				set_bit(R5_Overlap, &sh->dev[d].flags);
-				spin_unlock_irq(&sh->stripe_lock);
+				raw_spin_unlock_irq(&sh->stripe_lock);
 				raid5_release_stripe(sh);
 				schedule();
 				goto again;
@@ -5136,7 +5136,7 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
 			raid5_inc_bi_active_stripes(bi);
 			sh->overwrite_disks++;
 		}
-		spin_unlock_irq(&sh->stripe_lock);
+		raw_spin_unlock_irq(&sh->stripe_lock);
 		if (conf->mddev->bitmap) {
 			for (d = 0;
 			     d < conf->raid_disks - conf->max_degraded;
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index 0739604990b7..984b156fcf01 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -216,12 +216,12 @@ struct stripe_head {
 						  */
 	enum check_states	check_state;
 	enum reconstruct_states reconstruct_state;
-	spinlock_t		stripe_lock;
+	raw_spinlock_t		stripe_lock;
 	int			cpu;
 	struct r5worker_group	*group;
 
 	struct stripe_head	*batch_head; /* protected by stripe lock */
-	spinlock_t		batch_lock; /* only header's lock is useful */
+	raw_spinlock_t		batch_lock; /* only header's lock is useful */
 	struct list_head	batch_list; /* protected by head's batch lock*/
 
 	struct r5l_io_unit	*log_io;
-- 
2.11.0

[PATCH 2/2] raid5: Use local_irq_save_nort() in raid5_release_stripe().

[Alexander GQ Gerasiov]

From: Alexander GQ Gerasiov <gq@redlab-i.ru>

This will fix the following warning:

[ ] BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:993
[ ] in_atomic(): 0, irqs_disabled(): 1, pid: 1482, name: mdadm
[ ] CPU: 0 PID: 1482 Comm: mdadm Not tainted 4.9.20-rt16-stand6-686 #1
[ ] Hardware name: Supermicro SYS-5027R-WRF/X9SRW-F, BIOS 3.2a 10/28/2015
[ ]  ec6c1bcc c12cdc6f c1078f0a eca41880 ec6c1bfc c10787c1 c1668778 00000000
[ ]  00000001 000005ca eca41d84 00000000 80000000 ecce02c0 ecce0000 ecce02c0
[ ]  ec6c1c08 c1584587 ecce1c30 ec6c1c1c c1093ee0 ecce02c0 ecce1c00 ecce0000
[ ] Call Trace:
[ ]  [<c12cdc6f>] dump_stack+0x47/0x68
[ ]  [<c1078f0a>] ? migrate_enable+0x4a/0xf0
[ ]  [<c10787c1>] ___might_sleep+0x101/0x180
[ ]  [<c1584587>] rt_spin_lock+0x17/0x40
[ ]  [<c1093ee0>] atomic_dec_and_spin_lock+0x50/0x70
[ ]  [<f88eda95>] raid5_release_stripe+0x95/0x100 [raid456]
[ ]  [<f88efabd>] grow_one_stripe+0xcd/0xf0 [raid456]
[ ]  [<f88f0274>] setup_conf+0x794/0x900 [raid456]
[ ]  [<f88f11e5>] raid5_run+0xa85/0xcc0 [raid456]

Signed-off-by: Alexander GQ Gerasiov <gq@redlab-i.ru>
---
 drivers/md/raid5.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index e69525b5db7d..f879c1b94dba 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -429,7 +429,7 @@ void raid5_release_stripe(struct stripe_head *sh)
 		md_wakeup_thread(conf->mddev->thread);
 	return;
 slow_path:
-	local_irq_save(flags);
+	local_irq_save_nort(flags);
 	/* we are ok here if STRIPE_ON_RELEASE_LIST is set or not */
 	if (atomic_dec_and_lock(&sh->count, &conf->device_lock)) {
 		INIT_LIST_HEAD(&list);
@@ -438,7 +438,7 @@ void raid5_release_stripe(struct stripe_head *sh)
 		spin_unlock(&conf->device_lock);
 		release_inactive_stripe_list(conf, &list, hash);
 	}
-	local_irq_restore(flags);
+	local_irq_restore_nort(flags);
 }
 
 static inline void remove_hash(struct stripe_head *sh)
-- 
2.11.0

Re: [PATCH 1/2] raid5: Use raw_spinlock for stripe management.

[Julia Cartwright]

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

On Mon, Apr 24, 2017 at 04:33:35PM +0300, Alexander GQ Gerasiov wrote:
> From: Alexander GQ Gerasiov <gq@redlab-i.ru>

Hello Alexander-

> This commit fixes the following warning:

In general, with the class of "convert to raw spinlock" patches, it's
nice to have written affirmation in the commit message that you've
looked through all of the codepaths and ensured bounded, minimal
behavior.

> [ ] BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:993
> [ ] in_atomic(): 0, irqs_disabled(): 1, pid: 58, name: kworker/u12:1
> [ ] CPU: 5 PID: 58 Comm: kworker/u12:1 Tainted: G        W       4.9.20-rt16-stand6-686 #1
> [ ] Hardware name: Supermicro SYS-5027R-WRF/X9SRW-F, BIOS 3.2a 10/28/2015
> [ ] Workqueue: writeback wb_workfn (flush-253:0)
> [ ]  edf21a34 c12cdc6f c1078f0a edf08c40 edf21a64 c10787c1 c1668778 00000000
> [ ]  00000001 0000003a edf09144 00000000 80000000 ec694b18 ec775748 ec694ad0
> [ ]  edf21a70 c1584587 ec775790 edf21ac4 f893e8a3 00000000 c1078e52 00000000
> [ ] Call Trace:
> [ ]  [<c12cdc6f>] dump_stack+0x47/0x68
> [ ]  [<c1078f0a>] ? migrate_enable+0x4a/0xf0
> [ ]  [<c10787c1>] ___might_sleep+0x101/0x180
> [ ]  [<c1584587>] rt_spin_lock+0x17/0x40
> [ ]  [<f893e8a3>] add_stripe_bio+0x4e3/0x6c0 [raid456]
> [ ]  [<c1078e52>] ? preempt_count_add+0x42/0xb0
> [ ]  [<f89408f7>] raid5_make_request+0x737/0xdd0 [raid456]

Did you cut off the bottom of the callstack, here?

This conversion seems a bit fishy to me... is any of the raid5 code
invoked from hard interrupt context?

   Julia

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

Re: [PATCH 1/2] raid5: Use raw_spinlock for stripe management.

[Julia Cartwright]

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

On Tue, Apr 25, 2017 at 10:46:46AM -0500, Julia Cartwright wrote:
> On Mon, Apr 24, 2017 at 04:33:35PM +0300, Alexander GQ Gerasiov wrote:
> > From: Alexander GQ Gerasiov <gq@redlab-i.ru>
> 
> Hello Alexander-
> 
> > This commit fixes the following warning:
> 
> In general, with the class of "convert to raw spinlock" patches, it's
> nice to have written affirmation in the commit message that you've
> looked through all of the codepaths and ensured bounded, minimal
> behavior.
> 
> > [ ] BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:993
> > [ ] in_atomic(): 0, irqs_disabled(): 1, pid: 58, name: kworker/u12:1
> > [ ] CPU: 5 PID: 58 Comm: kworker/u12:1 Tainted: G        W       4.9.20-rt16-stand6-686 #1
> > [ ] Hardware name: Supermicro SYS-5027R-WRF/X9SRW-F, BIOS 3.2a 10/28/2015
> > [ ] Workqueue: writeback wb_workfn (flush-253:0)
> > [ ]  edf21a34 c12cdc6f c1078f0a edf08c40 edf21a64 c10787c1 c1668778 00000000
> > [ ]  00000001 0000003a edf09144 00000000 80000000 ec694b18 ec775748 ec694ad0
> > [ ]  edf21a70 c1584587 ec775790 edf21ac4 f893e8a3 00000000 c1078e52 00000000
> > [ ] Call Trace:
> > [ ]  [<c12cdc6f>] dump_stack+0x47/0x68
> > [ ]  [<c1078f0a>] ? migrate_enable+0x4a/0xf0
> > [ ]  [<c10787c1>] ___might_sleep+0x101/0x180
> > [ ]  [<c1584587>] rt_spin_lock+0x17/0x40
> > [ ]  [<f893e8a3>] add_stripe_bio+0x4e3/0x6c0 [raid456]
> > [ ]  [<c1078e52>] ? preempt_count_add+0x42/0xb0
> > [ ]  [<f89408f7>] raid5_make_request+0x737/0xdd0 [raid456]
> 
> Did you cut off the bottom of the callstack, here?

Looking at this some more, I imagine the callpath which is problematic
is:

   <writeback kworker>
   ...
   add_stripe_bio()
     stripe_add_to_batch_list()
       lock_two_stripes()
         local_irq_disable()
         spin_lock(&sh{1,2}->stripe_lock) --> ___might_sleep() --> BOOM

That is, it's the local_irq_disable() in lock_two_stripes() which is the
problem.  That's my gut feel, anyway. Can you give this patch a try and
see if works?  You'll probably still want the second patch in your
series.

   Julia

-- 8< --
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index fa2c4de32a64..54dc2995aeee 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -110,8 +110,7 @@ static inline void unlock_device_hash_lock(struct r5conf *conf, int hash)
 static inline void lock_all_device_hash_locks_irq(struct r5conf *conf)
 {
 	int i;
-	local_irq_disable();
-	spin_lock(conf->hash_locks);
+	spin_lock_irq(conf->hash_locks);
 	for (i = 1; i < NR_STRIPE_HASH_LOCKS; i++)
 		spin_lock_nest_lock(conf->hash_locks + i, conf->hash_locks);
 	spin_lock(&conf->device_lock);
@@ -121,9 +120,9 @@ static inline void unlock_all_device_hash_locks_irq(struct r5conf *conf)
 {
 	int i;
 	spin_unlock(&conf->device_lock);
-	for (i = NR_STRIPE_HASH_LOCKS; i; i--)
-		spin_unlock(conf->hash_locks + i - 1);
-	local_irq_enable();
+	for (i = NR_STRIPE_HASH_LOCKS - 1; i; i--)
+		spin_unlock(conf->hash_locks + i);
+	spin_unlock_irq(conf->hash_locks);
 }
 
 /* bio's attached to a stripe+device for I/O are linked together in bi_sector
@@ -732,12 +731,11 @@ static bool is_full_stripe_write(struct stripe_head *sh)
 
 static void lock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
 {
-	local_irq_disable();
 	if (sh1 > sh2) {
-		spin_lock(&sh2->stripe_lock);
+		spin_lock_irq(&sh2->stripe_lock);
 		spin_lock_nested(&sh1->stripe_lock, 1);
 	} else {
-		spin_lock(&sh1->stripe_lock);
+		spin_lock_irq(&sh1->stripe_lock);
 		spin_lock_nested(&sh2->stripe_lock, 1);
 	}
 }
@@ -745,8 +743,7 @@ static void lock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
 static void unlock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
 {
 	spin_unlock(&sh1->stripe_lock);
-	spin_unlock(&sh2->stripe_lock);
-	local_irq_enable();
+	spin_unlock_irq(&sh2->stripe_lock);
 }
 
 /* Only freshly new full stripe normal write stripe can be added to a batch list */

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

Re: [PATCH 1/2] raid5: Use raw_spinlock for stripe management.

[Alexander Gerasiov]

Hello Julia,

On Tue, 25 Apr 2017 16:09:29 -0500
Julia Cartwright <julia@ni.com> wrote:

> Looking at this some more, I imagine the callpath which is problematic
> is:
> 
>    <writeback kworker>
>    ...
>    add_stripe_bio()
>      stripe_add_to_batch_list()
>        lock_two_stripes()
>          local_irq_disable()
>          spin_lock(&sh{1,2}->stripe_lock) --> ___might_sleep() -->
> BOOM
> 
> That is, it's the local_irq_disable() in lock_two_stripes() which is
> the problem.  That's my gut feel, anyway. Can you give this patch a
> try and see if works?  You'll probably still want the second patch in
> your series.

Ouch, you are absolutely right, it was not atomic context this time, so
changing spinlocks from mutex to real ones is not necessary. My fault.

Do you want me to resubmit the patch, or do it yourself?

-- 
Best regards,
 Alexander Gerasiov

 Contacts:
 e-mail: gq@cs.msu.su  Homepage: http://gerasiov.net  Skype: gerasiov
 PGP fingerprint: 04B5 9D90 DF7C C2AB CD49  BAEA CA87 E9E8 2AAC 33F1

Re: [PATCH 1/2] raid5: Use raw_spinlock for stripe management.

[Julia Cartwright]

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

On Thu, Apr 27, 2017 at 06:34:07PM +0300, Alexander Gerasiov wrote:
> Hello Julia,
> 
> On Tue, 25 Apr 2017 16:09:29 -0500
> Julia Cartwright <julia@ni.com> wrote:
> 
> > Looking at this some more, I imagine the callpath which is problematic
> > is:
> > 
> >    <writeback kworker>
> >    ...
> >    add_stripe_bio()
> >      stripe_add_to_batch_list()
> >        lock_two_stripes()
> >          local_irq_disable()
> >          spin_lock(&sh{1,2}->stripe_lock) --> ___might_sleep() -->
> > BOOM
> > 
> > That is, it's the local_irq_disable() in lock_two_stripes() which is
> > the problem.  That's my gut feel, anyway. Can you give this patch a
> > try and see if works?  You'll probably still want the second patch in
> > your series.
> 
> Ouch, you are absolutely right, it was not atomic context this time, so
> changing spinlocks from mutex to real ones is not necessary. My fault.
> 
> Do you want me to resubmit the patch, or do it yourself?

If you can confirm that the proposed patch fixes the problem you are
seeing, I'll send a proper patch.

Thanks,
   Julia

PS. Also, interestingly, this is not an altogether uncommon problem:

   virtual report

   @r exists@
   position p1;
   position p2;
   @@

   (
   local_irq_disable@p1
   |
   local_irq_save@p1
   )(...);
   ... when != local_irq_enable(...)
       when != local_irq_restore(...)
   (
   spin_lock@p2
   |
   spin_lock_irq@p2
   |
   spin_lock_irqsave@p2
   )(...);

   @script:python depends on r && report@
   p1 << r.p1;
   p2 << r.p2;
   @@

   msg="ERROR: sleeping spinlock acquired, though interrupts disabled on line %s" % (p1[0].line)
   coccilib.report.print_report(p2[0], msg)

Reports:

   drivers/tty/serial/pch_uart.c:1672:2-11: ERROR: sleeping spinlock acquired, though interrupts disabled on line 1662
   drivers/tty/serial/st-asc.c:812:2-11: ERROR: sleeping spinlock acquired, though interrupts disabled on line 806
   drivers/tty/serial/meson_uart.c:497:2-11: ERROR: sleeping spinlock acquired, though interrupts disabled on line 491
   drivers/tty/serial/bcm63xx_uart.c:713:2-11: ERROR: sleeping spinlock acquired, though interrupts disabled on line 706
   drivers/tty/serial/ar933x_uart.c:555:2-11: ERROR: sleeping spinlock acquired, though interrupts disabled on line 548
   drivers/tty/serial/stm32-usart.c:940:2-11: ERROR: sleeping spinlock acquired, though interrupts disabled on line 934
   drivers/net/ethernet/tehuti/tehuti.c:1632:1-10: ERROR: sleeping spinlock acquired, though interrupts disabled on line 1631
   drivers/tty/serial/pxa.c:659:2-11: ERROR: sleeping spinlock acquired, though interrupts disabled on line 653
   drivers/tty/serial/lpc32xx_hs.c:153:2-11: ERROR: sleeping spinlock acquired, though interrupts disabled on line 147
   arch/x86/kernel/reboot.c:98:1-10: ERROR: sleeping spinlock acquired, though interrupts disabled on line 86
   net/core/drop_monitor.c:171:1-10: ERROR: sleeping spinlock acquired, though interrupts disabled on line 169
   drivers/tty/serial/sh-sci.c:2800:2-11: ERROR: sleeping spinlock acquired, though interrupts disabled on line 2791
   drivers/usb/gadget/udc/dummy_hcd.c:755:1-10: ERROR: sleeping spinlock acquired, though interrupts disabled on line 754
   arch/alpha/kernel/srmcons.c:81:1-10: ERROR: sleeping spinlock acquired, though interrupts disabled on line 74
   lib/dma-debug.c:946:2-11: ERROR: sleeping spinlock acquired, though interrupts disabled on line 943
   arch/mn10300/kernel/mn10300-serial.c:1594:2-11: ERROR: sleeping spinlock acquired, though interrupts disabled on line 1587
   arch/metag/kernel/smp.c:451:2-11: ERROR: sleeping spinlock acquired, though interrupts disabled on line 446

There isn't an entirely straightforward way to fix each of these; it will
require some auditing, if anyone is looking for more work to do :).

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

Re: [PATCH 1/2] raid5: Use raw_spinlock for stripe management.

[Alexander Gerasiov]

Hello Julia,

On Thu, 27 Apr 2017 10:43:17 -0500
Julia Cartwright <julia@ni.com> wrote:

> On Thu, Apr 27, 2017 at 06:34:07PM +0300, Alexander Gerasiov wrote:
> > Hello Julia,
> > 
> > On Tue, 25 Apr 2017 16:09:29 -0500
> > Julia Cartwright <julia@ni.com> wrote:
> >   
> > > Looking at this some more, I imagine the callpath which is
> > > problematic is:
> > > 
> > >    <writeback kworker>
> > >    ...
> > >    add_stripe_bio()
> > >      stripe_add_to_batch_list()
> > >        lock_two_stripes()
> > >          local_irq_disable()
> > >          spin_lock(&sh{1,2}->stripe_lock) --> ___might_sleep() -->
> > > BOOM
> > > 
> > > That is, it's the local_irq_disable() in lock_two_stripes() which
> > > is the problem.  That's my gut feel, anyway. Can you give this
> > > patch a try and see if works?  You'll probably still want the
> > > second patch in your series.  
> > 
> > Ouch, you are absolutely right, it was not atomic context this
> > time, so changing spinlocks from mutex to real ones is not
> > necessary. My fault.
> > 
> > Do you want me to resubmit the patch, or do it yourself?  
> 
> If you can confirm that the proposed patch fixes the problem you are
> seeing, I'll send a proper patch.
Yes, I confirm the fix.


-- 
Best regards,
 Alexander Gerasiov

 Contacts:
 e-mail: gq@cs.msu.su  Homepage: http://gerasiov.net  Skype: gerasiov
 PGP fingerprint: 04B5 9D90 DF7C C2AB CD49  BAEA CA87 E9E8 2AAC 33F1

-- 
Best regards,
 Alexander Gerasiov

 Contacts:
 e-mail: gq@cs.msu.su  Homepage: http://gerasiov.net  Skype: gerasiov
 PGP fingerprint: 04B5 9D90 DF7C C2AB CD49  BAEA CA87 E9E8 2AAC 33F1

[PATCH] md/raid5: make use of spin_lock_irq over local_irq_disable + spin_lock

[Julia Cartwright]

On mainline, there is no functional difference, just less code, and
symmetric lock/unlock paths.

On PREEMPT_RT builds, this fixes the following warning, seen by
Alexander GQ Gerasiov, due to the sleeping nature of spinlocks.

   BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:993
   in_atomic(): 0, irqs_disabled(): 1, pid: 58, name: kworker/u12:1
   CPU: 5 PID: 58 Comm: kworker/u12:1 Tainted: G        W       4.9.20-rt16-stand6-686 #1
   Hardware name: Supermicro SYS-5027R-WRF/X9SRW-F, BIOS 3.2a 10/28/2015
   Workqueue: writeback wb_workfn (flush-253:0)
   Call Trace:
    dump_stack+0x47/0x68
    ? migrate_enable+0x4a/0xf0
    ___might_sleep+0x101/0x180
    rt_spin_lock+0x17/0x40
    add_stripe_bio+0x4e3/0x6c0 [raid456]
    ? preempt_count_add+0x42/0xb0
    raid5_make_request+0x737/0xdd0 [raid456]

Reported-by: Alexander GQ Gerasiov <gq@redlab-i.ru>
Tested-by: Alexander GQ Gerasiov <gq@redlab-i.ru>
Signed-off-by: Julia Cartwright <julia@ni.com>
---
Hey All-

While this fixes a problem on RT primarily, the patch is equally applicable
upstream, as such probably makes sense to be pulled through the md tree.  It
may also make sense to be pulled directly into rt-devel.

Alexander-

I turned your "I confirm the fix" to a 'Tested-by', let me know if that's a problem.

Thanks,

   Julia

 drivers/md/raid5.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index fa2c4de32a64..54dc2995aeee 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -110,8 +110,7 @@ static inline void unlock_device_hash_lock(struct r5conf *conf, int hash)
 static inline void lock_all_device_hash_locks_irq(struct r5conf *conf)
 {
 	int i;
-	local_irq_disable();
-	spin_lock(conf->hash_locks);
+	spin_lock_irq(conf->hash_locks);
 	for (i = 1; i < NR_STRIPE_HASH_LOCKS; i++)
 		spin_lock_nest_lock(conf->hash_locks + i, conf->hash_locks);
 	spin_lock(&conf->device_lock);
@@ -121,9 +120,9 @@ static inline void unlock_all_device_hash_locks_irq(struct r5conf *conf)
 {
 	int i;
 	spin_unlock(&conf->device_lock);
-	for (i = NR_STRIPE_HASH_LOCKS; i; i--)
-		spin_unlock(conf->hash_locks + i - 1);
-	local_irq_enable();
+	for (i = NR_STRIPE_HASH_LOCKS - 1; i; i--)
+		spin_unlock(conf->hash_locks + i);
+	spin_unlock_irq(conf->hash_locks);
 }
 
 /* bio's attached to a stripe+device for I/O are linked together in bi_sector
@@ -732,12 +731,11 @@ static bool is_full_stripe_write(struct stripe_head *sh)
 
 static void lock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
 {
-	local_irq_disable();
 	if (sh1 > sh2) {
-		spin_lock(&sh2->stripe_lock);
+		spin_lock_irq(&sh2->stripe_lock);
 		spin_lock_nested(&sh1->stripe_lock, 1);
 	} else {
-		spin_lock(&sh1->stripe_lock);
+		spin_lock_irq(&sh1->stripe_lock);
 		spin_lock_nested(&sh2->stripe_lock, 1);
 	}
 }
@@ -745,8 +743,7 @@ static void lock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
 static void unlock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
 {
 	spin_unlock(&sh1->stripe_lock);
-	spin_unlock(&sh2->stripe_lock);
-	local_irq_enable();
+	spin_unlock_irq(&sh2->stripe_lock);
 }
 
 /* Only freshly new full stripe normal write stripe can be added to a batch list */
-- 
2.12.0

[PATCH] md/raid5: make use of spin_lock_irq over local_irq_disable + spin_lock

[Julia Cartwright]

On mainline, there is no functional difference, just less code, and
symmetric lock/unlock paths.

On PREEMPT_RT builds, this fixes the following warning, seen by
Alexander GQ Gerasiov, due to the sleeping nature of spinlocks.

   BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:993
   in_atomic(): 0, irqs_disabled(): 1, pid: 58, name: kworker/u12:1
   CPU: 5 PID: 58 Comm: kworker/u12:1 Tainted: G        W       4.9.20-rt16-stand6-686 #1
   Hardware name: Supermicro SYS-5027R-WRF/X9SRW-F, BIOS 3.2a 10/28/2015
   Workqueue: writeback wb_workfn (flush-253:0)
   Call Trace:
    dump_stack+0x47/0x68
    ? migrate_enable+0x4a/0xf0
    ___might_sleep+0x101/0x180
    rt_spin_lock+0x17/0x40
    add_stripe_bio+0x4e3/0x6c0 [raid456]
    ? preempt_count_add+0x42/0xb0
    raid5_make_request+0x737/0xdd0 [raid456]

Reported-by: Alexander GQ Gerasiov <gq@redlab-i.ru>
Tested-by: Alexander GQ Gerasiov <gq@redlab-i.ru>
Signed-off-by: Julia Cartwright <julia@ni.com>
---
Hey All-

While this fixes a problem on RT primarily, the patch is equally applicable
upstream, as such probably makes sense to be pulled through the md tree.  It
may also make sense to be pulled directly into rt-devel.

Alexander-

I turned your "I confirm the fix" to a 'Tested-by', let me know if that's a problem.

Thanks,

   Julia

 drivers/md/raid5.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index fa2c4de32a64..54dc2995aeee 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -110,8 +110,7 @@ static inline void unlock_device_hash_lock(struct r5conf *conf, int hash)
 static inline void lock_all_device_hash_locks_irq(struct r5conf *conf)
 {
 	int i;
-	local_irq_disable();
-	spin_lock(conf->hash_locks);
+	spin_lock_irq(conf->hash_locks);
 	for (i = 1; i < NR_STRIPE_HASH_LOCKS; i++)
 		spin_lock_nest_lock(conf->hash_locks + i, conf->hash_locks);
 	spin_lock(&conf->device_lock);
@@ -121,9 +120,9 @@ static inline void unlock_all_device_hash_locks_irq(struct r5conf *conf)
 {
 	int i;
 	spin_unlock(&conf->device_lock);
-	for (i = NR_STRIPE_HASH_LOCKS; i; i--)
-		spin_unlock(conf->hash_locks + i - 1);
-	local_irq_enable();
+	for (i = NR_STRIPE_HASH_LOCKS - 1; i; i--)
+		spin_unlock(conf->hash_locks + i);
+	spin_unlock_irq(conf->hash_locks);
 }
 
 /* bio's attached to a stripe+device for I/O are linked together in bi_sector
@@ -732,12 +731,11 @@ static bool is_full_stripe_write(struct stripe_head *sh)
 
 static void lock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
 {
-	local_irq_disable();
 	if (sh1 > sh2) {
-		spin_lock(&sh2->stripe_lock);
+		spin_lock_irq(&sh2->stripe_lock);
 		spin_lock_nested(&sh1->stripe_lock, 1);
 	} else {
-		spin_lock(&sh1->stripe_lock);
+		spin_lock_irq(&sh1->stripe_lock);
 		spin_lock_nested(&sh2->stripe_lock, 1);
 	}
 }
@@ -745,8 +743,7 @@ static void lock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
 static void unlock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
 {
 	spin_unlock(&sh1->stripe_lock);
-	spin_unlock(&sh2->stripe_lock);
-	local_irq_enable();
+	spin_unlock_irq(&sh2->stripe_lock);
 }
 
 /* Only freshly new full stripe normal write stripe can be added to a batch list */
-- 
2.12.0

Re: [PATCH] md/raid5: make use of spin_lock_irq over local_irq_disable + spin_lock

[Shaohua Li]

On Fri, Apr 28, 2017 at 12:41:02PM -0500, Julia Cartwright wrote:
> On mainline, there is no functional difference, just less code, and
> symmetric lock/unlock paths.
> 
> On PREEMPT_RT builds, this fixes the following warning, seen by
> Alexander GQ Gerasiov, due to the sleeping nature of spinlocks.
> 
>    BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:993
>    in_atomic(): 0, irqs_disabled(): 1, pid: 58, name: kworker/u12:1
>    CPU: 5 PID: 58 Comm: kworker/u12:1 Tainted: G        W       4.9.20-rt16-stand6-686 #1
>    Hardware name: Supermicro SYS-5027R-WRF/X9SRW-F, BIOS 3.2a 10/28/2015
>    Workqueue: writeback wb_workfn (flush-253:0)
>    Call Trace:
>     dump_stack+0x47/0x68
>     ? migrate_enable+0x4a/0xf0
>     ___might_sleep+0x101/0x180
>     rt_spin_lock+0x17/0x40
>     add_stripe_bio+0x4e3/0x6c0 [raid456]
>     ? preempt_count_add+0x42/0xb0
>     raid5_make_request+0x737/0xdd0 [raid456]
> 
> Reported-by: Alexander GQ Gerasiov <gq@redlab-i.ru>
> Tested-by: Alexander GQ Gerasiov <gq@redlab-i.ru>
> Signed-off-by: Julia Cartwright <julia@ni.com>

applied, thanks!

> ---
> Hey All-
> 
> While this fixes a problem on RT primarily, the patch is equally applicable
> upstream, as such probably makes sense to be pulled through the md tree.  It
> may also make sense to be pulled directly into rt-devel.
> 
> Alexander-
> 
> I turned your "I confirm the fix" to a 'Tested-by', let me know if that's a problem.
> 
> Thanks,
> 
>    Julia
> 
>  drivers/md/raid5.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index fa2c4de32a64..54dc2995aeee 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -110,8 +110,7 @@ static inline void unlock_device_hash_lock(struct r5conf *conf, int hash)
>  static inline void lock_all_device_hash_locks_irq(struct r5conf *conf)
>  {
>  	int i;
> -	local_irq_disable();
> -	spin_lock(conf->hash_locks);
> +	spin_lock_irq(conf->hash_locks);
>  	for (i = 1; i < NR_STRIPE_HASH_LOCKS; i++)
>  		spin_lock_nest_lock(conf->hash_locks + i, conf->hash_locks);
>  	spin_lock(&conf->device_lock);
> @@ -121,9 +120,9 @@ static inline void unlock_all_device_hash_locks_irq(struct r5conf *conf)
>  {
>  	int i;
>  	spin_unlock(&conf->device_lock);
> -	for (i = NR_STRIPE_HASH_LOCKS; i; i--)
> -		spin_unlock(conf->hash_locks + i - 1);
> -	local_irq_enable();
> +	for (i = NR_STRIPE_HASH_LOCKS - 1; i; i--)
> +		spin_unlock(conf->hash_locks + i);
> +	spin_unlock_irq(conf->hash_locks);
>  }
>  
>  /* bio's attached to a stripe+device for I/O are linked together in bi_sector
> @@ -732,12 +731,11 @@ static bool is_full_stripe_write(struct stripe_head *sh)
>  
>  static void lock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
>  {
> -	local_irq_disable();
>  	if (sh1 > sh2) {
> -		spin_lock(&sh2->stripe_lock);
> +		spin_lock_irq(&sh2->stripe_lock);
>  		spin_lock_nested(&sh1->stripe_lock, 1);
>  	} else {
> -		spin_lock(&sh1->stripe_lock);
> +		spin_lock_irq(&sh1->stripe_lock);
>  		spin_lock_nested(&sh2->stripe_lock, 1);
>  	}
>  }
> @@ -745,8 +743,7 @@ static void lock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
>  static void unlock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
>  {
>  	spin_unlock(&sh1->stripe_lock);
> -	spin_unlock(&sh2->stripe_lock);
> -	local_irq_enable();
> +	spin_unlock_irq(&sh2->stripe_lock);
>  }
>  
>  /* Only freshly new full stripe normal write stripe can be added to a batch list */
> -- 
> 2.12.0
>