Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Sergey Senozhatsky]

On (08/23/17 13:35), Boqun Feng wrote:
> > KERN_CONT and "\n" should not be together. "\n" flushes the cont
> > buffer immediately.
> > 
> 
> Hmm.. Not quite familiar with printk() stuffs, but I could see several
> usages of printk(KERN_CONT "...\n") in kernel.
> 
> Did a bit research myself, and I now think the inappropriate use is to
> use a KERN_CONT printk *after* another printk ending with a "\n".

ah... I didn't check __print_lock_name(): it leaves unflushed cont buffer
upon the return. sorry, your code is correct.

-ss

> > >   printk("\n *** DEADLOCK ***\n\n");
> > > + } else if (cross_lock(src->instance)) {
> > > + printk(" Possible unsafe locking scenario by crosslock:\n\n");
> > > + printk("   CPU0CPU1\n");
> > > + printk("   \n");
> > > + printk("  lock(");
> > > + __print_lock_name(target);
> > > + printk(KERN_CONT ");\n");
> > > + printk("  lock(");
> > > + __print_lock_name(source);
> > > + printk(KERN_CONT ");\n");
> > > + printk("   lock(");
> > > + __print_lock_name(parent == source ? target : parent);
> > > + printk(KERN_CONT ");\n");
> > > + printk("   unlock(");
> > > + __print_lock_name(source);
> > > + printk(KERN_CONT ");\n");
> > > + printk("\n *** DEADLOCK ***\n\n");
> > >   } else {
> > >   printk(" Possible unsafe locking scenario:\n\n");
> > >   printk("   CPU0CPU1\n");
> > > -- 
> > > 2.14.1
> > > 

Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Sergey Senozhatsky]

On (08/23/17 13:35), Boqun Feng wrote:
[..]
> > >   printk(KERN_CONT ");\n");
> > 
> > KERN_CONT and "\n" should not be together. "\n" flushes the cont
> > buffer immediately.
> > 
> 
> Hmm.. Not quite familiar with printk() stuffs, but I could see several
> usages of printk(KERN_CONT "...\n") in kernel.
> 
> Did a bit research myself, and I now think the inappropriate use is to
> use a KERN_CONT printk *after* another printk ending with a "\n". Am I
> missing some recent changes or rules of KERN_CONT?

has been this way for quite some time (if not always).

LOG_NEWLINE results in cont_flush(), which log_store() the content
of KERN_CONT buffer.

if we see that supplied message has no \n then we store it in a
dedicated buffer (cont buffer)

if (!(lflags & LOG_NEWLINE))
return cont_add();

return log_store();

we flush that buffer (move its content to the kernel log buffer) when
we receive a message with a \n or when printk() from another task/context
interrupts the current cont line and, thus, forces us to flush.

-ss

Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Byungchul Park]

On Wed, Aug 23, 2017 at 12:38:13PM +0800, Boqun Feng wrote:
> From: Boqun Feng 
> Date: Wed, 23 Aug 2017 12:12:16 +0800
> Subject: [PATCH] lockdep: Print proper scenario if cross deadlock detected at
>  acquisition time
> 
> For a potential deadlock about CROSSRELEASE as follow:
> 
>   P1  P2
>   === =
>   lock(A)
>   lock(X)
>   lock(A)
>   commit(X)
> 
>   A: normal lock, X: cross lock
> 
> , we could detect it at two places:
> 
> 1. commit time:
> 
>   We have run P1 first, and have dependency A --> X in graph, and
>   then we run P2, and find the deadlock.
> 
> 2. acquisition time:
> 
>   We have run P2 first, and have dependency A --> X, in

X -> A

>   graph(because another P3 may run previously and is acquiring for

".. another P3 may have run previously and was holding .."
^
Additionally, not only P3 but also P2 like:

lock(A)
lock(X)
lock(X) // I mean it's at _P2_
lock(A)
commit(X)

>   lock X), and then we run P1 and find the deadlock.
>
> In current print_circular_lock_scenario(), for 1) we could print the
> right scenario and note that's a deadlock related to CROSSRELEASE,
> however for 2) we print the scenario as a normal lockdep deadlock.
> 
> It's better to print a proper scenario related to CROSSRELEASE to help
> users find their bugs more easily, so improve this.
> 
> Signed-off-by: Boqun Feng 
> ---
>  kernel/locking/lockdep.c | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 642fb5362507..a3709e15f609 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -1156,6 +1156,23 @@ print_circular_lock_scenario(struct held_lock *src,
>   __print_lock_name(target);
>   printk(KERN_CONT ");\n");
>   printk("\n *** DEADLOCK ***\n\n");
> + } else if (cross_lock(src->instance)) {
> + printk(" Possible unsafe locking scenario by crosslock:\n\n");
> + printk("   CPU0CPU1\n");
> + printk("   \n");
> + printk("  lock(");
> + __print_lock_name(target);
> + printk(KERN_CONT ");\n");
> + printk("  lock(");
> + __print_lock_name(source);
> + printk(KERN_CONT ");\n");
> + printk("   lock(");
> + __print_lock_name(parent == source ? target : parent);
> + printk(KERN_CONT ");\n");
> + printk("   unlock(");
> + __print_lock_name(source);
> + printk(KERN_CONT ");\n");
> + printk("\n *** DEADLOCK ***\n\n");
>   } else {
>   printk(" Possible unsafe locking scenario:\n\n");
>   printk("   CPU0CPU1\n");

I need time to be sure if it's correct.

Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Boqun Feng]

On Wed, Aug 23, 2017 at 01:46:48PM +0900, Sergey Senozhatsky wrote:
> On (08/23/17 12:38), Boqun Feng wrote:
> [..]
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index 642fb5362507..a3709e15f609 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -1156,6 +1156,23 @@ print_circular_lock_scenario(struct held_lock *src,
> > __print_lock_name(target);
> > printk(KERN_CONT ");\n");
> 
> KERN_CONT and "\n" should not be together. "\n" flushes the cont
> buffer immediately.
> 

Hmm.. Not quite familiar with printk() stuffs, but I could see several
usages of printk(KERN_CONT "...\n") in kernel.

Did a bit research myself, and I now think the inappropriate use is to
use a KERN_CONT printk *after* another printk ending with a "\n". Am I
missing some recent changes or rules of KERN_CONT?

Regards,
Boqun

>   -ss
> 
> > printk("\n *** DEADLOCK ***\n\n");
> > +   } else if (cross_lock(src->instance)) {
> > +   printk(" Possible unsafe locking scenario by crosslock:\n\n");
> > +   printk("   CPU0CPU1\n");
> > +   printk("   \n");
> > +   printk("  lock(");
> > +   __print_lock_name(target);
> > +   printk(KERN_CONT ");\n");
> > +   printk("  lock(");
> > +   __print_lock_name(source);
> > +   printk(KERN_CONT ");\n");
> > +   printk("   lock(");
> > +   __print_lock_name(parent == source ? target : parent);
> > +   printk(KERN_CONT ");\n");
> > +   printk("   unlock(");
> > +   __print_lock_name(source);
> > +   printk(KERN_CONT ");\n");
> > +   printk("\n *** DEADLOCK ***\n\n");
> > } else {
> > printk(" Possible unsafe locking scenario:\n\n");
> > printk("   CPU0CPU1\n");
> > -- 
> > 2.14.1
> > 


signature.asc
Description: PGP signature

Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Boqun Feng]

On Wed, Aug 23, 2017 at 01:46:17PM +0900, Byungchul Park wrote:
> On Wed, Aug 23, 2017 at 11:49:51AM +0800, Boqun Feng wrote:
> > Hi Byungchul,
> > 
> > On Wed, Aug 23, 2017 at 09:03:04AM +0900, Byungchul Park wrote:
> > > On Tue, Aug 22, 2017 at 09:43:56PM +, Bart Van Assche wrote:
> > > > On Tue, 2017-08-22 at 19:47 +0900, Sergey Senozhatsky wrote:
> > > > > ==
> > > > > WARNING: possible circular locking dependency detected
> > > > > 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not 
> > > > > tainted
> > > > > --
> > > > > fsck.ext4/148 is trying to acquire lock:
> > > > >  (>bd_mutex){+.+.}, at: [] 
> > > > > __blkdev_put+0x33/0x190
> > > > > 
> > > > >  but now in release context of a crosslock acquired at the following:
> > > > >  ((complete)#2){+.+.}, at: [] 
> > > > > blk_execute_rq+0xbb/0xda
> > > > > 
> > > > >  which lock already depends on the new lock.
> > > > > 
> > 
> > I felt this message really misleading, because the deadlock is detected
> > at the commit time of "((complete)#2)" rather than the acquisition
> > time of "(>bd_mutex)", so I made the following improvement.
> > 
> > Thoughts?
> > 
> > Regards,
> > Boqun
> > 
> > --->8
> > From: Boqun Feng <boqun.f...@gmail.com>
> > Date: Wed, 23 Aug 2017 10:18:30 +0800
> > Subject: [PATCH] lockdep: Improve the readibility of crossrelease related
> >  splats
> > 
> > When a crossrelease related deadlock is detected in a commit, the
> > current implemention makes splats like:
> > 
> > > fsck.ext4/148 is trying to acquire lock:
> > >  (>bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
> > >
> > >  but now in release context of a crosslock acquired at the following:
> > >  ((complete)#2){+.+.}, at: [] 
> > > blk_execute_rq+0xbb/0xda
> > >
> > >  which lock already depends on the new lock.
> > > ...
> > 
> > However, it could be misleading because the current task has got the
> > lock already, and in fact the deadlock is detected when it is doing the
> > commit of the crossrelease lock. So make the splats more accurate to
> > describe the deadlock case.
> > 
> > Signed-off-by: Boqun Feng <boqun.f...@gmail.com>
> > ---
> >  kernel/locking/lockdep.c | 22 ++
> >  1 file changed, 14 insertions(+), 8 deletions(-)
> > 
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index 66011c9f5df3..642fb5362507 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -1195,17 +1195,23 @@ print_circular_bug_header(struct lock_list *entry, 
> > unsigned int depth,
> > pr_warn("WARNING: possible circular locking dependency detected\n");
> > print_kernel_ident();
> > pr_warn("--\n");
> > -   pr_warn("%s/%d is trying to acquire lock:\n",
> > -   curr->comm, task_pid_nr(curr));
> > -   print_lock(check_src);
> >  
> > -   if (cross_lock(check_tgt->instance))
> > -   pr_warn("\nbut now in release context of a crosslock acquired 
> > at the following:\n");
> > -   else
> > +   if (cross_lock(check_tgt->instance)) {
> > +   pr_warn("%s/%d is committing a crossrelease lock:\n",
> > +   curr->comm, task_pid_nr(curr));
> 
> I think it would be better to print something in term of acquisition,
> since the following print_lock() will print infromation of acquisition.
> 

Well, that print_lock() will print the cross lock acquisition
information at other contexts, but the current thread is doing the
commit. So I think the information would be a little misleading. I will
add "aacquired at" to indicate the lock information is for acquisition.

> > +   print_lock(check_tgt);
> > +   pr_warn("\n, with the following lock held:\n");
> 
> The lock does not have to be held at the commit.
> 

Ah.. right.

How about this:

pr_warn("%s/%d is committing a crossrelease lock acquired 
at:\n",
curr->comm, task_pid_nr(curr));
print_lock(check_tgt);
pr_warn("\n, after having the fol

Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Sergey Senozhatsky]

On (08/23/17 12:38), Boqun Feng wrote:
[..]
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 642fb5362507..a3709e15f609 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -1156,6 +1156,23 @@ print_circular_lock_scenario(struct held_lock *src,
>   __print_lock_name(target);
>   printk(KERN_CONT ");\n");

KERN_CONT and "\n" should not be together. "\n" flushes the cont
buffer immediately.

-ss

>   printk("\n *** DEADLOCK ***\n\n");
> + } else if (cross_lock(src->instance)) {
> + printk(" Possible unsafe locking scenario by crosslock:\n\n");
> + printk("   CPU0CPU1\n");
> + printk("   \n");
> + printk("  lock(");
> + __print_lock_name(target);
> + printk(KERN_CONT ");\n");
> + printk("  lock(");
> + __print_lock_name(source);
> + printk(KERN_CONT ");\n");
> + printk("   lock(");
> + __print_lock_name(parent == source ? target : parent);
> + printk(KERN_CONT ");\n");
> + printk("   unlock(");
> + __print_lock_name(source);
> + printk(KERN_CONT ");\n");
> + printk("\n *** DEADLOCK ***\n\n");
>   } else {
>   printk(" Possible unsafe locking scenario:\n\n");
>   printk("   CPU0CPU1\n");
> -- 
> 2.14.1
> 

Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Byungchul Park]

On Wed, Aug 23, 2017 at 11:49:51AM +0800, Boqun Feng wrote:
> Hi Byungchul,
> 
> On Wed, Aug 23, 2017 at 09:03:04AM +0900, Byungchul Park wrote:
> > On Tue, Aug 22, 2017 at 09:43:56PM +, Bart Van Assche wrote:
> > > On Tue, 2017-08-22 at 19:47 +0900, Sergey Senozhatsky wrote:
> > > > ==
> > > > WARNING: possible circular locking dependency detected
> > > > 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not tainted
> > > > --
> > > > fsck.ext4/148 is trying to acquire lock:
> > > >  (>bd_mutex){+.+.}, at: [] 
> > > > __blkdev_put+0x33/0x190
> > > > 
> > > >  but now in release context of a crosslock acquired at the following:
> > > >  ((complete)#2){+.+.}, at: [] 
> > > > blk_execute_rq+0xbb/0xda
> > > > 
> > > >  which lock already depends on the new lock.
> > > > 
> 
> I felt this message really misleading, because the deadlock is detected
> at the commit time of "((complete)#2)" rather than the acquisition
> time of "(>bd_mutex)", so I made the following improvement.
> 
> Thoughts?
> 
> Regards,
> Boqun
> 
> --->8
> From: Boqun Feng <boqun.f...@gmail.com>
> Date: Wed, 23 Aug 2017 10:18:30 +0800
> Subject: [PATCH] lockdep: Improve the readibility of crossrelease related
>  splats
> 
> When a crossrelease related deadlock is detected in a commit, the
> current implemention makes splats like:
> 
> > fsck.ext4/148 is trying to acquire lock:
> >  (>bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
> >
> >  but now in release context of a crosslock acquired at the following:
> >  ((complete)#2){+.+.}, at: [] 
> > blk_execute_rq+0xbb/0xda
> >
> >  which lock already depends on the new lock.
> > ...
> 
> However, it could be misleading because the current task has got the
> lock already, and in fact the deadlock is detected when it is doing the
> commit of the crossrelease lock. So make the splats more accurate to
> describe the deadlock case.
> 
> Signed-off-by: Boqun Feng <boqun.f...@gmail.com>
> ---
>  kernel/locking/lockdep.c | 22 ++
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 66011c9f5df3..642fb5362507 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -1195,17 +1195,23 @@ print_circular_bug_header(struct lock_list *entry, 
> unsigned int depth,
>   pr_warn("WARNING: possible circular locking dependency detected\n");
>   print_kernel_ident();
>   pr_warn("--\n");
> - pr_warn("%s/%d is trying to acquire lock:\n",
> - curr->comm, task_pid_nr(curr));
> - print_lock(check_src);
>  
> - if (cross_lock(check_tgt->instance))
> - pr_warn("\nbut now in release context of a crosslock acquired 
> at the following:\n");
> - else
> + if (cross_lock(check_tgt->instance)) {
> + pr_warn("%s/%d is committing a crossrelease lock:\n",
> + curr->comm, task_pid_nr(curr));

I think it would be better to print something in term of acquisition,
since the following print_lock() will print infromation of acquisition.

> + print_lock(check_tgt);
> + pr_warn("\n, with the following lock held:\n");

The lock does not have to be held at the commit.

> + print_lock(check_src);
> + pr_warn("\non which lock the crossrelease lock already 
> depends.\n\n");
> + } else {
> + pr_warn("%s/%d is trying to acquire lock:\n",
> + curr->comm, task_pid_nr(curr));
> + print_lock(check_src);
>   pr_warn("\nbut task is already holding lock:\n");
> + print_lock(check_tgt);
> + pr_warn("\nwhich lock already depends on the new lock.\n\n");
> + }
>  
> - print_lock(check_tgt);
> - pr_warn("\nwhich lock already depends on the new lock.\n\n");
>   pr_warn("\nthe existing dependency chain (in reverse order) is:\n");
>  
>   print_circular_bug_entry(entry, depth);
> -- 
> 2.14.1

Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Boqun Feng]

On Wed, Aug 23, 2017 at 11:49:51AM +0800, Boqun Feng wrote:
> Hi Byungchul,
> 
> On Wed, Aug 23, 2017 at 09:03:04AM +0900, Byungchul Park wrote:
> > On Tue, Aug 22, 2017 at 09:43:56PM +, Bart Van Assche wrote:
> > > On Tue, 2017-08-22 at 19:47 +0900, Sergey Senozhatsky wrote:
> > > > ==
> > > > WARNING: possible circular locking dependency detected
> > > > 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not tainted
> > > > --
> > > > fsck.ext4/148 is trying to acquire lock:
> > > >  (>bd_mutex){+.+.}, at: [] 
> > > > __blkdev_put+0x33/0x190
> > > > 
> > > >  but now in release context of a crosslock acquired at the following:
> > > >  ((complete)#2){+.+.}, at: [] 
> > > > blk_execute_rq+0xbb/0xda
> > > > 
> > > >  which lock already depends on the new lock.
> > > > 
> 
> I felt this message really misleading, because the deadlock is detected
> at the commit time of "((complete)#2)" rather than the acquisition
> time of "(>bd_mutex)", so I made the following improvement.
> 
> Thoughts?
> 
> Regards,
> Boqun
> 

While I'm on this one, I think we should also add a case in @check_src
is a cross lock, i.e. we detect cross deadlock at the acquisition time
of the cross lock. How about the following?

Regards,
Boqun

--->8
From: Boqun Feng <boqun.f...@gmail.com>
Date: Wed, 23 Aug 2017 12:12:16 +0800
Subject: [PATCH] lockdep: Print proper scenario if cross deadlock detected at
 acquisition time

For a potential deadlock about CROSSRELEASE as follow:

P1  P2
=== =
lock(A)
lock(X)
lock(A)
commit(X)

A: normal lock, X: cross lock

, we could detect it at two places:

1. commit time:

We have run P1 first, and have dependency A --> X in graph, and
then we run P2, and find the deadlock.

2. acquisition time:

We have run P2 first, and have dependency A --> X, in
graph(because another P3 may run previously and is acquiring for
lock X), and then we run P1 and find the deadlock.

In current print_circular_lock_scenario(), for 1) we could print the
right scenario and note that's a deadlock related to CROSSRELEASE,
however for 2) we print the scenario as a normal lockdep deadlock.

It's better to print a proper scenario related to CROSSRELEASE to help
users find their bugs more easily, so improve this.

Signed-off-by: Boqun Feng <boqun.f...@gmail.com>
---
 kernel/locking/lockdep.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 642fb5362507..a3709e15f609 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1156,6 +1156,23 @@ print_circular_lock_scenario(struct held_lock *src,
__print_lock_name(target);
printk(KERN_CONT ");\n");
printk("\n *** DEADLOCK ***\n\n");
+   } else if (cross_lock(src->instance)) {
+   printk(" Possible unsafe locking scenario by crosslock:\n\n");
+   printk("   CPU0CPU1\n");
+   printk("   \n");
+   printk("  lock(");
+   __print_lock_name(target);
+   printk(KERN_CONT ");\n");
+   printk("  lock(");
+   __print_lock_name(source);
+   printk(KERN_CONT ");\n");
+   printk("   lock(");
+   __print_lock_name(parent == source ? target : parent);
+   printk(KERN_CONT ");\n");
+   printk("   unlock(");
+   __print_lock_name(source);
+   printk(KERN_CONT ");\n");
+   printk("\n *** DEADLOCK ***\n\n");
} else {
printk(" Possible unsafe locking scenario:\n\n");
printk("   CPU0CPU1\n");
-- 
2.14.1

Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Boqun Feng]

Hi Byungchul,

On Wed, Aug 23, 2017 at 09:03:04AM +0900, Byungchul Park wrote:
> On Tue, Aug 22, 2017 at 09:43:56PM +, Bart Van Assche wrote:
> > On Tue, 2017-08-22 at 19:47 +0900, Sergey Senozhatsky wrote:
> > > ==
> > > WARNING: possible circular locking dependency detected
> > > 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not tainted
> > > --
> > > fsck.ext4/148 is trying to acquire lock:
> > >  (>bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
> > > 
> > >  but now in release context of a crosslock acquired at the following:
> > >  ((complete)#2){+.+.}, at: [] 
> > > blk_execute_rq+0xbb/0xda
> > > 
> > >  which lock already depends on the new lock.
> > > 

I felt this message really misleading, because the deadlock is detected
at the commit time of "((complete)#2)" rather than the acquisition
time of "(>bd_mutex)", so I made the following improvement.

Thoughts?

Regards,
Boqun

--->8
From: Boqun Feng <boqun.f...@gmail.com>
Date: Wed, 23 Aug 2017 10:18:30 +0800
Subject: [PATCH] lockdep: Improve the readibility of crossrelease related
 splats

When a crossrelease related deadlock is detected in a commit, the
current implemention makes splats like:

> fsck.ext4/148 is trying to acquire lock:
>  (>bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
>
>  but now in release context of a crosslock acquired at the following:
>  ((complete)#2){+.+.}, at: [] blk_execute_rq+0xbb/0xda
>
>  which lock already depends on the new lock.
> ...

However, it could be misleading because the current task has got the
lock already, and in fact the deadlock is detected when it is doing the
commit of the crossrelease lock. So make the splats more accurate to
describe the deadlock case.

Signed-off-by: Boqun Feng <boqun.f...@gmail.com>
---
 kernel/locking/lockdep.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 66011c9f5df3..642fb5362507 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1195,17 +1195,23 @@ print_circular_bug_header(struct lock_list *entry, 
unsigned int depth,
pr_warn("WARNING: possible circular locking dependency detected\n");
print_kernel_ident();
pr_warn("--\n");
-   pr_warn("%s/%d is trying to acquire lock:\n",
-   curr->comm, task_pid_nr(curr));
-   print_lock(check_src);
 
-   if (cross_lock(check_tgt->instance))
-   pr_warn("\nbut now in release context of a crosslock acquired 
at the following:\n");
-   else
+   if (cross_lock(check_tgt->instance)) {
+   pr_warn("%s/%d is committing a crossrelease lock:\n",
+   curr->comm, task_pid_nr(curr));
+   print_lock(check_tgt);
+   pr_warn("\n, with the following lock held:\n");
+   print_lock(check_src);
+   pr_warn("\non which lock the crossrelease lock already 
depends.\n\n");
+   } else {
+   pr_warn("%s/%d is trying to acquire lock:\n",
+   curr->comm, task_pid_nr(curr));
+   print_lock(check_src);
pr_warn("\nbut task is already holding lock:\n");
+   print_lock(check_tgt);
+   pr_warn("\nwhich lock already depends on the new lock.\n\n");
+   }
 
-   print_lock(check_tgt);
-   pr_warn("\nwhich lock already depends on the new lock.\n\n");
pr_warn("\nthe existing dependency chain (in reverse order) is:\n");
 
print_circular_bug_entry(entry, depth);
-- 
2.14.1

Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Byungchul Park]

On Wed, Aug 23, 2017 at 11:36:49AM +0900, Sergey Senozhatsky wrote:
> On (08/23/17 09:03), Byungchul Park wrote:
> [..]
> 
> aha, ok
> 
> > The report is talking about the following lockup:
> > 
> > A work in a worker A task work on exit to user
> > -- ---
> > mutex_lock(>bd_mutex)
> >mutext_lock(>bd_mutex)
> > blk_execute_rq()
> >wait_for_completion_io_timeout()
> >complete()
> > 
> > Is this impossible?
> 
> I was really confused how this "unlock" may lead to a deadlock

Hi Sergey,

Right. It should be enhanced.

> 
> > > >  other info that might help us debug this:
> > > >  Possible unsafe locking scenario by crosslock:
> > > >CPU0CPU1
> > > >
> > > >   lock(>bd_mutex);
> > > >   lock((complete)#2);
> > > >lock(>bd_mutex);
> > > >unlock((complete)#2);
> 
> 
> any chance the report can be improved? mention timeout, etc?
> // well, if this functionality will stay.
> 
> 
> p.s.
> Bart Van Assche, thanks for Cc-ing Park Byungchul, I was really
> sure I didn't enabled the cross-release, but apparently I was wrong:
>  CONFIG_LOCKDEP_CROSSRELEASE=y
>  CONFIG_LOCKDEP_COMPLETIONS=y
> 
>   -ss

Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Sergey Senozhatsky]

On (08/23/17 09:03), Byungchul Park wrote:
[..]

aha, ok

> The report is talking about the following lockup:
> 
> A work in a worker A task work on exit to user
> -- ---
> mutex_lock(>bd_mutex)
>mutext_lock(>bd_mutex)
> blk_execute_rq()
>wait_for_completion_io_timeout()
>complete()
> 
> Is this impossible?

I was really confused how this "unlock" may lead to a deadlock

> > >  other info that might help us debug this:
> > >  Possible unsafe locking scenario by crosslock:
> > >CPU0CPU1
> > >
> > >   lock(>bd_mutex);
> > >   lock((complete)#2);
> > >lock(>bd_mutex);
> > >unlock((complete)#2);


any chance the report can be improved? mention timeout, etc?
// well, if this functionality will stay.


p.s.
Bart Van Assche, thanks for Cc-ing Park Byungchul, I was really
sure I didn't enabled the cross-release, but apparently I was wrong:
 CONFIG_LOCKDEP_CROSSRELEASE=y
 CONFIG_LOCKDEP_COMPLETIONS=y

-ss

Re: [PATCH] scsi: sg: off by one in sg_ioctl()

[Martin K. Petersen]

Dan,

> If "val" is SG_MAX_QUEUE then we are one element beyond the end of the
> "rinfo" array so the > should be >=.

Applied to 4.13/scsi-fixes. Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] sg: protect against races between mmap() and SG_SET_RESERVED_SIZE

[Martin K. Petersen]

Todd,

> Take f_mutex around mmap() processing to protect against races with
> the SG_SET_RESERVED_SIZE ioctl.  Ensure the reserve buffer length
> remains consistent during the mapping operation, and set the
> "mmap called" flag to prevent further changes to the reserved buffer
> size as an atomic operation with the mapping.

Applied to 4.14/scsi-queue (with a slight whitespace fix). Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v4 00/14] mpt3sas driver NVMe support:

[Martin K. Petersen]

Suganath,

>   mpt3sas: SGL to PRP Translation for I/Os to NVMe  devices

I'm still confused about this patch.

 - I don't understand why you go through all these hoops to decide
   whether to use PRPs or IEEE scatterlists. If the firmware translation
   is slow, why even bother with the SG format in the first place? Set
   the max I/O size to match MDTS and you're done.

 - What's the benefit of using SG for regular I/O commands?

 - If the unmap translation in firmware is slow, why don't you translate
   WRITE SAME/w UNMAP set to DSM DEALLOCATE without requiring
   applications to do encapsulated passthrough?

Also make sure you attribute your patches correctly (From: root
). And you don't need that
long CC: list. Just send the patch series to linux-scsi@vger.kernel.org.

Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: sg: off by one in sg_ioctl()

[Douglas Gilbert]

On 2017-08-17 03:09 AM, Dan Carpenter wrote:

If "val" is SG_MAX_QUEUE then we are one element beyond the end of the
"rinfo" array so the > should be >=.

Fixes: 109bade9c625 ("scsi: sg: use standard lists for sg_requests")
Signed-off-by: Dan Carpenter 

Acked-by: Douglas Gilbert 

Thanks.



diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index d7ff71e0c85c..84e782d8e7c3 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1021,7 +1021,7 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned 
long arg)
read_lock_irqsave(>rq_list_lock, iflags);
val = 0;
list_for_each_entry(srp, >rq_list, entry) {
-   if (val > SG_MAX_QUEUE)
+   if (val >= SG_MAX_QUEUE)
break;
memset([val], 0, SZ_SG_REQ_INFO);
rinfo[val].req_state = srp->done + 1;

Re: [PATCH] sg: protect against races between mmap() and SG_SET_RESERVED_SIZE

[Douglas Gilbert]

On 2017-08-16 01:41 AM, Todd Poynor wrote:

Take f_mutex around mmap() processing to protect against races with
the SG_SET_RESERVED_SIZE ioctl.  Ensure the reserve buffer length
remains consistent during the mapping operation, and set the
"mmap called" flag to prevent further changes to the reserved buffer
size as an atomic operation with the mapping.

Signed-off-by: Todd Poynor 

Acked-by: Douglas Gilbert 

Thanks.


---
  drivers/scsi/sg.c | 12 +---
  1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 3a44b4bc872b..a20718e9f1f4 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1233,6 +1233,7 @@ sg_mmap(struct file *filp, struct vm_area_struct *vma)
unsigned long req_sz, len, sa;
Sg_scatter_hold *rsv_schp;
int k, length;
+int ret = 0;
  
  	if ((!filp) || (!vma) || (!(sfp = (Sg_fd *) filp->private_data)))

return -ENXIO;
@@ -1243,8 +1244,11 @@ sg_mmap(struct file *filp, struct vm_area_struct *vma)
if (vma->vm_pgoff)
return -EINVAL; /* want no offset */
rsv_schp = >reserve;
-   if (req_sz > rsv_schp->bufflen)
-   return -ENOMEM; /* cannot map more than reserved buffer */
+   mutex_lock(>f_mutex);
+   if (req_sz > rsv_schp->bufflen) {
+   ret = -ENOMEM;  /* cannot map more than reserved buffer */
+   goto out;
+   }
  
  	sa = vma->vm_start;

length = 1 << (PAGE_SHIFT + rsv_schp->page_order);
@@ -1258,7 +1262,9 @@ sg_mmap(struct file *filp, struct vm_area_struct *vma)
vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
vma->vm_private_data = sfp;
vma->vm_ops = _mmap_vm_ops;
-   return 0;
+out:
+   mutex_unlock(>f_mutex);
+   return ret;
  }
  
  static void

Re: [PATCH] sg: recheck MMAP_IO request length with lock held

[Martin K. Petersen]

Todd,

> Commit 1bc0eb044615 ("scsi: sg: protect accesses to 'reserved' page
> array") adds needed concurrency protection for the "reserve" buffer.
> Some checks that are initially made outside the lock are replicated once
> the lock is taken to ensure the checks and resulting decisions are made
> using consistent state.
>
> The check that a request with flag SG_FLAG_MMAP_IO set fits in the
> reserve buffer also needs to be performed again under the lock to
> ensure the reserve buffer length compared against matches the value in
> effect when the request is linked to the reserve buffer.  An -ENOMEM
> should be returned in this case, instead of switching over to an
> indirect buffer as for non-MMAP_IO requests.

Applied to 4.14/scsi-queue, thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Byungchul Park]

On Tue, Aug 22, 2017 at 09:43:56PM +, Bart Van Assche wrote:
> On Tue, 2017-08-22 at 19:47 +0900, Sergey Senozhatsky wrote:
> > ==
> > WARNING: possible circular locking dependency detected
> > 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not tainted
> > --
> > fsck.ext4/148 is trying to acquire lock:
> >  (>bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
> > 
> >  but now in release context of a crosslock acquired at the following:
> >  ((complete)#2){+.+.}, at: [] 
> > blk_execute_rq+0xbb/0xda
> > 
> >  which lock already depends on the new lock.
> > 
> >  the existing dependency chain (in reverse order) is:
> > 
> >  -> #1 ((complete)#2){+.+.}:
> >lock_acquire+0x176/0x19e
> >__wait_for_common+0x50/0x1e3
> >blk_execute_rq+0xbb/0xda
> >scsi_execute+0xc3/0x17d [scsi_mod]
> >sd_revalidate_disk+0x112/0x1549 [sd_mod]
> >rescan_partitions+0x48/0x2c4
> >__blkdev_get+0x14b/0x37c
> >blkdev_get+0x191/0x2c0
> >device_add_disk+0x2b4/0x3e5
> >sd_probe_async+0xf8/0x17e [sd_mod]
> >async_run_entry_fn+0x34/0xe0
> >process_one_work+0x2af/0x4d1
> >worker_thread+0x19a/0x24f
> >kthread+0x133/0x13b
> >ret_from_fork+0x27/0x40
> > 
> >  -> #0 (>bd_mutex){+.+.}:
> >__blkdev_put+0x33/0x190
> >blkdev_close+0x24/0x27
> >__fput+0xee/0x18a
> >task_work_run+0x79/0xa0
> >prepare_exit_to_usermode+0x9b/0xb5
> > 
> >  other info that might help us debug this:
> >  Possible unsafe locking scenario by crosslock:
> >CPU0CPU1
> >
> >   lock(>bd_mutex);
> >   lock((complete)#2);
> >lock(>bd_mutex);
> >unlock((complete)#2);
> > 
> >   *** DEADLOCK ***
> > 4 locks held by fsck.ext4/148:
> >  #0:  (>bd_mutex){+.+.}, at: [] 
> > __blkdev_put+0x33/0x190
> >  #1:  (rcu_read_lock){}, at: [] 
> > rcu_lock_acquire+0x0/0x20
> >  #2:  (&(>lock)->rlock){-.-.}, at: [] 
> > ata_scsi_queuecmd+0x23/0x74 [libata]
> >  #3:  (>wait#14){-...}, at: [] complete+0x18/0x50
> > 
> >  stack backtrace:
> > CPU: 1 PID: 148 Comm: fsck.ext4 Not tainted 
> > 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746
> > Call Trace:
> >  dump_stack+0x67/0x8e
> >  print_circular_bug+0x2a1/0x2af
> >  ? zap_class+0xc5/0xc5
> >  check_prev_add+0x76/0x20d
> >  ? __lock_acquire+0xc27/0xcc8
> >  lock_commit_crosslock+0x327/0x35e
> >  complete+0x24/0x50
> >  scsi_end_request+0x8d/0x176 [scsi_mod]
> >  scsi_io_completion+0x1be/0x423 [scsi_mod]
> >  __blk_mq_complete_request+0x112/0x131
> >  ata_scsi_simulate+0x212/0x218 [libata]
> >  __ata_scsi_queuecmd+0x1be/0x1de [libata]
> >  ata_scsi_queuecmd+0x41/0x74 [libata]
> >  scsi_dispatch_cmd+0x194/0x2af [scsi_mod]
> >  scsi_queue_rq+0x1e0/0x26f [scsi_mod]
> >  blk_mq_dispatch_rq_list+0x193/0x2a7
> >  ? _raw_spin_unlock+0x2e/0x40
> >  blk_mq_sched_dispatch_requests+0x132/0x176
> >  __blk_mq_run_hw_queue+0x59/0xc5
> >  __blk_mq_delay_run_hw_queue+0x5f/0xc1
> >  blk_mq_flush_plug_list+0xfc/0x10b
> >  blk_flush_plug_list+0xc6/0x1eb
> >  blk_finish_plug+0x25/0x32
> >  generic_writepages+0x56/0x63
> >  do_writepages+0x36/0x70
> >  __filemap_fdatawrite_range+0x59/0x5f
> >  filemap_write_and_wait+0x19/0x4f
> >  __blkdev_put+0x5f/0x190
> >  blkdev_close+0x24/0x27
> >  __fput+0xee/0x18a
> >  task_work_run+0x79/0xa0
> >  prepare_exit_to_usermode+0x9b/0xb5
> >  entry_SYSCALL_64_fastpath+0xab/0xad
> 
> Byungchul, did you add the crosslock checks to lockdep? Can you have a look at
> the above report? That report namely doesn't make sense to me.

The report is talking about the following lockup:

A work in a worker A task work on exit to user
-- ---
mutex_lock(>bd_mutex)
   mutext_lock(>bd_mutex)
blk_execute_rq()
   wait_for_completion_io_timeout()
   complete()

Is this impossible?

To Peterz,

Anyway I wanted to avoid lockdep reports in the case using a timeout
interface. Do you think it's still worth reporting the kind of lockup?
I'm ok if you do.

Re: possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Bart Van Assche]

On Tue, 2017-08-22 at 19:47 +0900, Sergey Senozhatsky wrote:
> ==
> WARNING: possible circular locking dependency detected
> 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not tainted
> --
> fsck.ext4/148 is trying to acquire lock:
>  (>bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
> 
>  but now in release context of a crosslock acquired at the following:
>  ((complete)#2){+.+.}, at: [] blk_execute_rq+0xbb/0xda
> 
>  which lock already depends on the new lock.
> 
>  the existing dependency chain (in reverse order) is:
> 
>  -> #1 ((complete)#2){+.+.}:
>lock_acquire+0x176/0x19e
>__wait_for_common+0x50/0x1e3
>blk_execute_rq+0xbb/0xda
>scsi_execute+0xc3/0x17d [scsi_mod]
>sd_revalidate_disk+0x112/0x1549 [sd_mod]
>rescan_partitions+0x48/0x2c4
>__blkdev_get+0x14b/0x37c
>blkdev_get+0x191/0x2c0
>device_add_disk+0x2b4/0x3e5
>sd_probe_async+0xf8/0x17e [sd_mod]
>async_run_entry_fn+0x34/0xe0
>process_one_work+0x2af/0x4d1
>worker_thread+0x19a/0x24f
>kthread+0x133/0x13b
>ret_from_fork+0x27/0x40
> 
>  -> #0 (>bd_mutex){+.+.}:
>__blkdev_put+0x33/0x190
>blkdev_close+0x24/0x27
>__fput+0xee/0x18a
>task_work_run+0x79/0xa0
>prepare_exit_to_usermode+0x9b/0xb5
> 
>  other info that might help us debug this:
>  Possible unsafe locking scenario by crosslock:
>CPU0CPU1
>
>   lock(>bd_mutex);
>   lock((complete)#2);
>lock(>bd_mutex);
>unlock((complete)#2);
> 
>   *** DEADLOCK ***
> 4 locks held by fsck.ext4/148:
>  #0:  (>bd_mutex){+.+.}, at: [] 
> __blkdev_put+0x33/0x190
>  #1:  (rcu_read_lock){}, at: [] 
> rcu_lock_acquire+0x0/0x20
>  #2:  (&(>lock)->rlock){-.-.}, at: [] 
> ata_scsi_queuecmd+0x23/0x74 [libata]
>  #3:  (>wait#14){-...}, at: [] complete+0x18/0x50
> 
>  stack backtrace:
> CPU: 1 PID: 148 Comm: fsck.ext4 Not tainted 
> 4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746
> Call Trace:
>  dump_stack+0x67/0x8e
>  print_circular_bug+0x2a1/0x2af
>  ? zap_class+0xc5/0xc5
>  check_prev_add+0x76/0x20d
>  ? __lock_acquire+0xc27/0xcc8
>  lock_commit_crosslock+0x327/0x35e
>  complete+0x24/0x50
>  scsi_end_request+0x8d/0x176 [scsi_mod]
>  scsi_io_completion+0x1be/0x423 [scsi_mod]
>  __blk_mq_complete_request+0x112/0x131
>  ata_scsi_simulate+0x212/0x218 [libata]
>  __ata_scsi_queuecmd+0x1be/0x1de [libata]
>  ata_scsi_queuecmd+0x41/0x74 [libata]
>  scsi_dispatch_cmd+0x194/0x2af [scsi_mod]
>  scsi_queue_rq+0x1e0/0x26f [scsi_mod]
>  blk_mq_dispatch_rq_list+0x193/0x2a7
>  ? _raw_spin_unlock+0x2e/0x40
>  blk_mq_sched_dispatch_requests+0x132/0x176
>  __blk_mq_run_hw_queue+0x59/0xc5
>  __blk_mq_delay_run_hw_queue+0x5f/0xc1
>  blk_mq_flush_plug_list+0xfc/0x10b
>  blk_flush_plug_list+0xc6/0x1eb
>  blk_finish_plug+0x25/0x32
>  generic_writepages+0x56/0x63
>  do_writepages+0x36/0x70
>  __filemap_fdatawrite_range+0x59/0x5f
>  filemap_write_and_wait+0x19/0x4f
>  __blkdev_put+0x5f/0x190
>  blkdev_close+0x24/0x27
>  __fput+0xee/0x18a
>  task_work_run+0x79/0xa0
>  prepare_exit_to_usermode+0x9b/0xb5
>  entry_SYSCALL_64_fastpath+0xab/0xad

Byungchul, did you add the crosslock checks to lockdep? Can you have a look at
the above report? That report namely doesn't make sense to me.

Bart.

Re: [PATCH] sg: recheck MMAP_IO request length with lock held

[Douglas Gilbert]

On 2017-08-16 12:48 AM, Todd Poynor wrote:

Commit 1bc0eb044615 ("scsi: sg: protect accesses to 'reserved' page
array") adds needed concurrency protection for the "reserve" buffer.
Some checks that are initially made outside the lock are replicated once
the lock is taken to ensure the checks and resulting decisions are made
using consistent state.

The check that a request with flag SG_FLAG_MMAP_IO set fits in the
reserve buffer also needs to be performed again under the lock to
ensure the reserve buffer length compared against matches the value in
effect when the request is linked to the reserve buffer.  An -ENOMEM
should be returned in this case, instead of switching over to an
indirect buffer as for non-MMAP_IO requests.

Signed-off-by: Todd Poynor 

Acked-by: Douglas Gilbert 

Thanks.


---
  drivers/scsi/sg.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index d7ff71e0c85c..3a44b4bc872b 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1735,9 +1735,12 @@ sg_start_req(Sg_request *srp, unsigned char *cmd)
!sfp->res_in_use) {
sfp->res_in_use = 1;
sg_link_reserve(sfp, srp, dxfer_len);
-   } else if ((hp->flags & SG_FLAG_MMAP_IO) && sfp->res_in_use) {
+   } else if (hp->flags & SG_FLAG_MMAP_IO) {
+   res = -EBUSY; /* sfp->res_in_use == 1 */
+   if (dxfer_len > rsv_schp->bufflen)
+   res = -ENOMEM;
mutex_unlock(>f_mutex);
-   return -EBUSY;
+   return res;
} else {
res = sg_build_indirect(req_schp, sfp, dxfer_len);
if (res) {

[PATCH] scsi: lpfc: remove useless code in lpfc_sli4_bsg_link_diag_test

[Gustavo A. R. Silva]

Remove variable assignments. The value stored in local variable _rc_ is
overwritten at line 2448:rc = lpfc_sli4_bsg_set_link_diag_state(phba, 0);
before it can be used.

Addresses-Coverity-ID: 1226935
Signed-off-by: Gustavo A. R. Silva 
---
This issue was detected by Coverity and it was tested by compilation only.
Notice that this code has been there since 2011.

 drivers/scsi/lpfc/lpfc_bsg.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_bsg.c b/drivers/scsi/lpfc/lpfc_bsg.c
index a1686c2..fe9e1c0 100644
--- a/drivers/scsi/lpfc/lpfc_bsg.c
+++ b/drivers/scsi/lpfc/lpfc_bsg.c
@@ -2384,20 +2384,17 @@ lpfc_sli4_bsg_link_diag_test(struct bsg_job *job)
goto job_error;
 
pmboxq = mempool_alloc(phba->mbox_mem_pool, GFP_KERNEL);
-   if (!pmboxq) {
-   rc = -ENOMEM;
+   if (!pmboxq)
goto link_diag_test_exit;
-   }
 
req_len = (sizeof(struct lpfc_mbx_set_link_diag_state) -
   sizeof(struct lpfc_sli4_cfg_mhdr));
alloc_len = lpfc_sli4_config(phba, pmboxq, LPFC_MBOX_SUBSYSTEM_FCOE,
 LPFC_MBOX_OPCODE_FCOE_LINK_DIAG_STATE,
 req_len, LPFC_SLI4_MBX_EMBED);
-   if (alloc_len != req_len) {
-   rc = -ENOMEM;
+   if (alloc_len != req_len)
goto link_diag_test_exit;
-   }
+
run_link_diag_test = >u.mqe.un.link_diag_test;
bf_set(lpfc_mbx_run_diag_test_link_num, _link_diag_test->u.req,
   phba->sli4_hba.lnk_info.lnk_no);
-- 
2.5.0

RE: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

[Long Li]

 
> Wouldn't it make sense to backport the changes to set the virt_boundary
> (which probably still is the SG_GAPS flag in such an old kernel)?

We can make storvsc use SG_GAPS. But the following patch is missing in 4.1 
stable block layer to make this work on some I/O situations. Backporting is 
more difficult and affect other code.

commit 5e7c4274a70aa2d6f485996d0ca1dad52d0039ca
Author: Jens Axboe 
Date:   Thu Sep 3 19:28:20 2015 +0300

block: Check for gaps on front and back merges

We are checking for gaps to previous bio_vec, which can
only detect back merges gaps. Moreover, at the point where
we check for a gap, we don't know if we will attempt a back
or a front merge. Thus, check for gap to prev in a back merge
attempt and check for a gap to next in a front merge attempt.

Signed-off-by: Jens Axboe 
[sagig: Minor rename change]
Signed-off-by: Sagi Grimberg 

Re: [PATCH 15/15] usb: make device_type const

[Heikki Krogerus]

On Sat, Aug 19, 2017 at 01:52:26PM +0530, Bhumika Goyal wrote:
> Make this const as it is only stored in the type field of a device
> structure, which is const.
> Done using Coccinelle.
> 
> Signed-off-by: Bhumika Goyal 

Acked-by: Heikki Krogerus 

> ---
>  drivers/usb/common/ulpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index 930e8f3..4aa5195 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -135,7 +135,7 @@ static void ulpi_dev_release(struct device *dev)
>   kfree(to_ulpi_dev(dev));
>  }
>  
> -static struct device_type ulpi_dev_type = {
> +static const struct device_type ulpi_dev_type = {
>   .name = "ulpi_device",
>   .groups = ulpi_dev_attr_groups,
>   .release = ulpi_dev_release,

Thanks,

-- 
heikki

possible circular locking dependency detected [was: linux-next: Tree for Aug 22]

[Sergey Senozhatsky]

Hello,

==
WARNING: possible circular locking dependency detected
4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746 Not tainted
--
fsck.ext4/148 is trying to acquire lock:
 (>bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190

 but now in release context of a crosslock acquired at the following:
 ((complete)#2){+.+.}, at: [] blk_execute_rq+0xbb/0xda

 which lock already depends on the new lock.

 the existing dependency chain (in reverse order) is:

 -> #1 ((complete)#2){+.+.}:
   lock_acquire+0x176/0x19e
   __wait_for_common+0x50/0x1e3
   blk_execute_rq+0xbb/0xda
   scsi_execute+0xc3/0x17d [scsi_mod]
   sd_revalidate_disk+0x112/0x1549 [sd_mod]
   rescan_partitions+0x48/0x2c4
   __blkdev_get+0x14b/0x37c
   blkdev_get+0x191/0x2c0
   device_add_disk+0x2b4/0x3e5
   sd_probe_async+0xf8/0x17e [sd_mod]
   async_run_entry_fn+0x34/0xe0
   process_one_work+0x2af/0x4d1
   worker_thread+0x19a/0x24f
   kthread+0x133/0x13b
   ret_from_fork+0x27/0x40

 -> #0 (>bd_mutex){+.+.}:
   __blkdev_put+0x33/0x190
   blkdev_close+0x24/0x27
   __fput+0xee/0x18a
   task_work_run+0x79/0xa0
   prepare_exit_to_usermode+0x9b/0xb5

 other info that might help us debug this:
 Possible unsafe locking scenario by crosslock:
   CPU0CPU1
   
  lock(>bd_mutex);
  lock((complete)#2);
   lock(>bd_mutex);
   unlock((complete)#2);

  *** DEADLOCK ***
4 locks held by fsck.ext4/148:
 #0:  (>bd_mutex){+.+.}, at: [] __blkdev_put+0x33/0x190
 #1:  (rcu_read_lock){}, at: [] rcu_lock_acquire+0x0/0x20
 #2:  (&(>lock)->rlock){-.-.}, at: [] 
ata_scsi_queuecmd+0x23/0x74 [libata]
 #3:  (>wait#14){-...}, at: [] complete+0x18/0x50

 stack backtrace:
CPU: 1 PID: 148 Comm: fsck.ext4 Not tainted 
4.13.0-rc6-next-20170822-dbg-00020-g39758ed8aae0-dirty #1746
Call Trace:
 dump_stack+0x67/0x8e
 print_circular_bug+0x2a1/0x2af
 ? zap_class+0xc5/0xc5
 check_prev_add+0x76/0x20d
 ? __lock_acquire+0xc27/0xcc8
 lock_commit_crosslock+0x327/0x35e
 complete+0x24/0x50
 scsi_end_request+0x8d/0x176 [scsi_mod]
 scsi_io_completion+0x1be/0x423 [scsi_mod]
 __blk_mq_complete_request+0x112/0x131
 ata_scsi_simulate+0x212/0x218 [libata]
 __ata_scsi_queuecmd+0x1be/0x1de [libata]
 ata_scsi_queuecmd+0x41/0x74 [libata]
 scsi_dispatch_cmd+0x194/0x2af [scsi_mod]
 scsi_queue_rq+0x1e0/0x26f [scsi_mod]
 blk_mq_dispatch_rq_list+0x193/0x2a7
 ? _raw_spin_unlock+0x2e/0x40
 blk_mq_sched_dispatch_requests+0x132/0x176
 __blk_mq_run_hw_queue+0x59/0xc5
 __blk_mq_delay_run_hw_queue+0x5f/0xc1
 blk_mq_flush_plug_list+0xfc/0x10b
 blk_flush_plug_list+0xc6/0x1eb
 blk_finish_plug+0x25/0x32
 generic_writepages+0x56/0x63
 do_writepages+0x36/0x70
 __filemap_fdatawrite_range+0x59/0x5f
 filemap_write_and_wait+0x19/0x4f
 __blkdev_put+0x5f/0x190
 blkdev_close+0x24/0x27
 __fput+0xee/0x18a
 task_work_run+0x79/0xa0
 prepare_exit_to_usermode+0x9b/0xb5
 entry_SYSCALL_64_fastpath+0xab/0xad
RIP: 0033:0x7ff5755a2f74
RSP: 002b:7ffe46fce038 EFLAGS: 0246 ORIG_RAX: 0003
RAX:  RBX: 555ddeddded0 RCX: 7ff5755a2f74
RDX: 1000 RSI: 555ddede2580 RDI: 0004
RBP:  R08: 555ddede2580 R09: 555ddedde080
R10: 00010800 R11: 0246 R12: 555ddedddfa0
R13: 7ff576523680 R14: 1000 R15: 555ddeddc2b0

-ss

Re: [PATCH 2/2] scsi: Preserve retry counter through scsi_prep_fn

[h...@lst.de]

On Mon, Aug 21, 2017 at 05:14:00PM -0500, Brian King wrote:
> Save / restore the retry counter in scsi_cmd in scsi_init_command.
> This allows us to go back through scsi_init_command for retries
> and not forget we are doing a retry.

So where will we initialize it to zero now?

Re: [PATCH 1/2] scsi: Move scsi_cmd->jiffies_at_alloc initialization to allocation time

[h...@lst.de]

On Mon, Aug 21, 2017 at 05:13:20PM -0500, Brian King wrote:
> Move the initialization of scsi_cmd->jiffies_at_alloc to allocation
> time rather than prep time. Also ensure that jiffies_at_alloc
> is preserved when we go through prep. This lets us send retries
> through prep again and not break the overall retry timer logic
> in scsi_softirq_done.
> 
> Suggested-by: Bart Van Assche 
> Signed-off-by: Brian King 

As far as I can tell this will never set jiffies_at_alloc for the
blk-mq path.

Re: [PATCH 0/2] Allow scsi_prep_fn to occur for retried commands

[Abdul Haleem]

On Mon, 2017-08-21 at 17:11 -0500, Brian King wrote:
> The following two patches address the hang issue being observed
> with Bart's patch on powerpc. The first patch moves the initialization
> of jiffies_at_alloc from scsi_init_command to scsi_init_rq, and ensures
> we don't zero jiffies_at_alloc in scsi_init_command. The second patch
> saves / restores the retry counter in scsi_init_command which lets us
> go through scsi_init_command for retries and not forget why we were
> there. 
> 
> These patches have only been boot tested on my Power machine with ipr
> to ensure they fix the issue I was seeing.
> 
> -Brian
> 

Thank you Brian and Bart, for your efforts to fix this bug.

I tested these patches on my PowerPC machine and system booted fine.

[PATCHv2 1/2] scsi: Move scsi_cmd->jiffies_at_alloc initialization to
allocation time
[PATCH 2/2] scsi: Preserve retry counter through scsi_prep_fn

Reported-and-Tested-by: Abdul Haleem 

-- 
Regard's

Abdul Haleem
IBM Linux Technology Centre

Re: [Regression 4.13-rc1] Resume does not work on Lenovo X60t

[Paul Menzel]

Dear Christoph,


On 2017-08-21 20:41, Christoph Hellwig wrote:


with 4.13-rc6 we're not using blk-mq by default any more, do you
still see the issue with that one?


Yes, I do see it this commit 6470812e2226 (Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc).

```
00.831: [  575.945132] BUG: unable to handle kernel NULL pointer 
dereference at 00f4

00.830: [  575.948009] IP: blk_set_runtime_active+0x27/0x60
00.830: [  575.948009] *pde = 
00.831: [  575.948009]
00.831: [  575.948009] Oops: 0002 [#1] SMP
00.831: [  575.948009] Modules linked in: joydev wacom_w8001 serport 
cpufreq_powersave cpufreq_conservative cpufreq_userspace binfmt_misc 
iTCO_wdt iTCO_vendor_support arc4 coretemp snd_hda_codec_analog 
snd_hda_codec_generic iwl3945 snd_hda_intel pcmcia iwlegacy 
snd_hda_codec kvm mac80211 snd_hda_core irqbypass yenta_socket snd_pcsp 
lpc_ich snd_hwdep thinkpad_acpi pcmcia_rsrc mfd_core serio_raw snd_pcm 
sg pcmcia_core nvram cfg80211 snd_timer rng_core snd rfkill battery 
soundcore shpchp evdev ac acpi_cpufreq parport_pc ppdev lp parport 
ip_tables x_tables autofs4 ext4 crc16 mbcache jbd2 fscrypto ecb cbc 
algif_skcipher af_alg dm_crypt dm_mod sr_mod cdrom sd_mod ata_generic 
ahci libahci sdhci_pci firewire_ohci ata_piix sdhci firewire_core libata 
e1000e i2c_i801 psmouse mmc_core crc_itu_t ptp scsi_mod i915 pps_core
00.831: [  575.948009]  ehci_pci video button uhci_hcd i2c_algo_bit 
ehci_hcd drm_kms_helper thermal usbcore syscopyarea sysfillrect 
sysimgblt fb_sys_fops drm
00.831: [  575.948009] CPU: 0 PID: 1126 Comm: kworker/u4:36 Not tainted 
4.13.0-rc6+ #110
00.831: [  575.948009] Hardware name: LENOVO 636338U/636338U, BIOS 
CBET4000 TIMELESS 01/01/1970

00.831: [  575.948009] Workqueue: events_unbound async_run_entry_fn
00.831: [  575.948009] task: f2ed8bc0 task.stack: f2ecc000
00.831: [  575.948009] EIP: blk_set_runtime_active+0x27/0x60
00.831: [  575.948009] EFLAGS: 00010046 CPU: 0
00.831: [  575.948009] EAX:  EBX: f5f3f820 ECX: f5f3f918 EDX: 
00010d7b
00.831: [  575.948009] ESI: f8ac3cc0 EDI: 0010 EBP: 0010 ESP: 
f2ecdea4

00.831: [  575.948009]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
00.831: [  575.948009] CR0: 80050033 CR2: 00f4 CR3: 0e3a9000 CR4: 
06d0

00.831: [  575.948009] Call Trace:
00.831: [  575.948009]  ? scsi_bus_resume_common+0x6e/0x110 [scsi_mod]
00.831: [  575.948009]  ? dpm_run_callback+0x4f/0x150
00.831: [  575.948009]  ? wait_for_completion+0x29/0x140
00.831: [  575.948009]  ? scsi_bus_thaw+0x10/0x10 [scsi_mod]
00.831: [  575.948009]  ? device_resume+0x8e/0x180
00.831: [  575.948009]  ? async_resume+0x1b/0x40
00.831: [  575.948009]  ? async_run_entry_fn+0x3f/0x1a0
00.831: [  575.948009]  ? process_one_work+0x136/0x310
00.831: [  575.948009]  ? worker_thread+0x39/0x3b0
00.831: [  575.948009]  ? kthread+0xd7/0x110
00.831: [  575.948009]  ? process_one_work+0x310/0x310
00.831: [  575.948009]  ? kthread_create_on_node+0x30/0x30
00.831: [  575.948009]  ? ret_from_fork+0x19/0x24
00.831: [  575.948009] Code: 8d 74 26 00 3e 8d 74 26 00 53 89 c3 8b 80 
fc 00 00 00 e8 2d 48 32 00 31 c0 8b 15 20 9e 24 ce 89 83 54 01 00 00 8b 
83 50 01 00 00 <89> 90 f4 00 00 00 ba 09 00 00 00 8b 83 50 01 00 00 e8 
f3 f2 16
00.831: [  575.948009] EIP: blk_set_runtime_active+0x27/0x60 SS:ESP: 
0068:f2ecdea4

00.831: [  575.948009] CR2: 00f4
00.831: [  575.948009] ---[ end trace b3f1ac10115418ab ]---
00.831: [  576.195662] pciehp :00:1c.0:pcie004: Timeout on hotplug 
command 0x1038 (issued 574920 msec ago)
00.831: [  576.204847] pciehp :00:1c.0:pcie004: Device :01:00.0 
already exists at :01:00, cannot hot-add
00.832: [  576.214460] pciehp :00:1c.0:pcie004: Cannot add device at 
:01:00
00.834: [  576.223117] atkbd serio0: Spurious ACK on isa0060/serio0. 
Some program might be trying to access hardware directly.

00.834: [  576.233968] ata1.00: configured for UDMA/33
00.927: [  576.328159] pciehp :00:1c.0:pcie004: Device :01:00.0 
already exists at :01:00, cannot hot-add
00.929: [  576.340348] pciehp :00:1c.0:pcie004: Cannot add device at 
:01:00
01.002: [  576.420139] usb 5-6: reset high-speed USB device number 2 
using ehci-pci
01.372: [  576.796072] firewire_core :05:00.1: rediscovered device 
fw0
03.010: [  578.440083] ata3: SATA link up 1.5 Gbps (SStatus 113 SControl 
300)

05.274: [  580.710027] ata3.00: ATA Identify Device Log not supported
05.276: [  580.718136] ata3.00: Security Log not supported
05.279: [  580.725856] ata3.00: ATA Identify Device Log not supported
05.282: [  580.733887] ata3.00: Security Log not supported
05.284: [  580.740838] ata3.00: configured for UDMA/100
```


Kind regards,

Paul

Re: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

[Christoph Hellwig]

Wouldn't it make sense to backport the changes to set the virt_boundary
(which probably still is the SG_GAPS flag in such an old kernel)?