Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1633053 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] md/raid5: make use of spin_lock_irq over local_irq_disable + spin_lock Julia Cartwright <julia@ni.com> - 2017-04-28 19:50 +0200
    Re: [PATCH] md/raid5: make use of spin_lock_irq over  local_irq_disable + spin_lock Shaohua Li <shli@kernel.org> - 2017-05-02 01:20 +0200

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

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

[toc] | [next] | [standalone]

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

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
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web