[PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Bart Van Assche]

For an unknown reason this patch causes the boot process to hang on
PowerPC systems:

sd 0:2:0:0: [sda] 272646144 512-byte logical blocks: (140 GB/130 GiB)
sd 0:2:0:0: [sda] 4096-byte physical blocks
sd 0:2:0:0: [sda] Write Protect is off
INFO: task swapper/5:1 blocked for more than 120 seconds.
      Not tainted 4.13.0-rc4-next-20170810-autotest #1
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
swapper/5       D 9936     1      0 0x00000800
Call Trace:
[c0000007f8483a10] [c0000007f8483a80] 0xc0000007f8483a80 (unreliable)
[c0000007f8483be0] [c00000000001b358] __switch_to+0x2e8/0x430
[c0000007f8483c40] [c0000000009d134c] __schedule+0x38c/0xaf0
[c0000007f8483d20] [c0000000009d1af0] schedule+0x40/0xb0
[c0000007f8483d50] [c000000000110bd4] async_synchronize_cookie_domain+0xd4/0x150
[c0000007f8483dc0] [c00000000000d8f8] kernel_init+0x28/0x140
[c0000007f8483e30] [c00000000000bc60] ret_from_kernel_thread+0x5c/0x7c

Hence revert commit 270065e92c31.

Fixes: commit 270065e92c31 ("scsi: scsi-mq: Always unprepare before requeuing a request")
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Brian King <brking@linux.vnet.ibm.com>
Cc: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
Cc: <stable@vger.kernel.org>
---
 drivers/scsi/scsi_lib.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7e92818e6597..e57c80d4648d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -44,8 +44,6 @@ static struct kmem_cache *scsi_sense_cache;
 static struct kmem_cache *scsi_sense_isadma_cache;
 static DEFINE_MUTEX(scsi_sense_cache_mutex);
 
-static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd);
-
 static inline struct kmem_cache *
 scsi_select_sense_cache(bool unchecked_isa_dma)
 {
@@ -142,12 +140,6 @@ static void scsi_mq_requeue_cmd(struct scsi_cmnd *cmd)
 {
 	struct scsi_device *sdev = cmd->device;
 
-	if (cmd->request->rq_flags & RQF_DONTPREP) {
-		cmd->request->rq_flags &= ~RQF_DONTPREP;
-		scsi_mq_uninit_cmd(cmd);
-	} else {
-		WARN_ON_ONCE(true);
-	}
 	blk_mq_requeue_request(cmd->request, true);
 	put_device(&sdev->sdev_gendev);
 }
@@ -985,6 +977,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		 * A new command will be prepared and issued.
 		 */
 		if (q->mq_ops) {
+			cmd->request->rq_flags &= ~RQF_DONTPREP;
+			scsi_mq_uninit_cmd(cmd);
 			scsi_mq_requeue_cmd(cmd);
 		} else {
 			scsi_release_buffers(cmd);
-- 
2.14.0

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Martin K. Petersen]

Bart,

> For an unknown reason this patch causes the boot process to hang on
> PowerPC systems:

OK, dropped it from fixes for now.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Damien Le Moal]

Martin,

On 8/17/17 09:11, Martin K. Petersen wrote:
> 
> Bart,
> 
>> For an unknown reason this patch causes the boot process to hang on
>> PowerPC systems:
> 
> OK, dropped it from fixes for now.
> 
> Thanks!

It means that commit 70e42fd02c46e2aa9ab07b766d418637e3a51de7 "scsi:
sd_zbc: Write unlock zone from sd_uninit_cmnd()" will need to be
reverted too as it will not solve the potential deadlock anymore. Bart's
patch was needed for it to work.

So we are back to the initial patch I sent that Chirstoph nacked, that
is, not write locking zones with scsi-mq to avoid deadlocking (blk-mq
does not dispatch write requests in order anyway, so SMR does not work
with mq for now).

Other solutions would be:
1) Do not change the zone locking and add a big fat warning for scanned
zoned block devices that things can get nasty
2) Fail initialization of zoned block devices with scsi-mq (do not
create the block devices)

(2) may actually be the better solution for now until I get a fix for
blk-mq.

Thoughts ?

Best regards.

-- 
Damien Le Moal,
Western Digital

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Damien Le Moal]

Martin,

On 8/17/17 09:41, Damien Le Moal wrote:
> Martin,
> 
> On 8/17/17 09:11, Martin K. Petersen wrote:
>>
>> Bart,
>>
>>> For an unknown reason this patch causes the boot process to hang on
>>> PowerPC systems:
>>
>> OK, dropped it from fixes for now.
>>
>> Thanks!
> 
> It means that commit 70e42fd02c46e2aa9ab07b766d418637e3a51de7 "scsi:
> sd_zbc: Write unlock zone from sd_uninit_cmnd()" will need to be
> reverted too as it will not solve the potential deadlock anymore. Bart's
> patch was needed for it to work.
> 
> So we are back to the initial patch I sent that Chirstoph nacked, that
> is, not write locking zones with scsi-mq to avoid deadlocking (blk-mq
> does not dispatch write requests in order anyway, so SMR does not work
> with mq for now).
> 
> Other solutions would be:
> 1) Do not change the zone locking and add a big fat warning for scanned
> zoned block devices that things can get nasty
> 2) Fail initialization of zoned block devices with scsi-mq (do not
> create the block devices)
> 
> (2) may actually be the better solution for now until I get a fix for
> blk-mq.
> 
> Thoughts ?
> 
> Best regards.

Actually, we can keep that commit as it makes the sq case cleaner
anyway. But we need something else to prevent deadlock in the mq case...

-- 
Damien Le Moal,
Western Digital

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Martin K. Petersen]

Damien,

> Actually, we can keep that commit as it makes the sq case cleaner
> anyway. But we need something else to prevent deadlock in the mq
> case...

OK, put it back.

Please send a lockout patch. We can always zap it if we find out what
went wrong with Bart's patch and queue a "real" fix.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Bart Van Assche]

On Wed, 2017-08-16 at 20:52 -0400, Martin K. Petersen wrote:
> Damien,
> 
> > Actually, we can keep that commit as it makes the sq case cleaner
> > anyway. But we need something else to prevent deadlock in the mq
> > case...
> 
> OK, put it back.
> 
> Please send a lockout patch. We can always zap it if we find out what
> went wrong with Bart's patch and queue a "real" fix.

Hello Martin,

I'm 99% sure there is nothing wrong with my patch but that my patch uncovered
a bug in the ipr driver (drivers/scsi/ipr). See also "WARNING: CPU: 15 PID: 0
at block/blk-mq.c:1111 __blk_mq_run_hw_queue+0x1d8/0x1f0F"
(https://marc.info/?l=linux-block&m=150290686719644).

Bart.

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Martin K. Petersen]

Bart,

> I'm 99% sure there is nothing wrong with my patch but that my patch
> uncovered a bug in the ipr driver (drivers/scsi/ipr). See also
> "WARNING: CPU: 15 PID: 0 at block/blk-mq.c:1111
> __blk_mq_run_hw_queue+0x1d8/0x1f0F"
> (https://marc.info/?l=linux-block&m=150290686719644).

But I already have the ipr patch queued. What am I missing?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Bart Van Assche]

On Wed, 2017-08-16 at 22:00 -0400, Martin K. Petersen wrote:
> Bart,
> 
> > I'm 99% sure there is nothing wrong with my patch but that my patch
> > uncovered a bug in the ipr driver (drivers/scsi/ipr). See also
> > "WARNING: CPU: 15 PID: 0 at block/blk-mq.c:1111
> > __blk_mq_run_hw_queue+0x1d8/0x1f0F"
> > (https://marc.info/?l=linux-block&m=150290686719644).
> 
> But I already have the ipr patch queued. What am I missing?

Hello Martin,

When I checked earlier today the ipr patch was not yet in linux-next ...

Bart.

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Martin K. Petersen]

Bart,

> When I checked earlier today the ipr patch was not yet in linux-next

That's weird. They were both committed two weeks ago.

They appear to be in there now, though:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/drivers/scsi?ofs=50

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Bart Van Assche]

On Wed, 2017-08-16 at 22:51 -0400, Martin K. Petersen wrote:
> > When I checked earlier today the ipr patch was not yet in linux-next
> 
> That's weird. They were both committed two weeks ago.
> 
> They appear to be in there now, though:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/drivers/scsi?ofs=50

Hello Martin,

As far as I can see commit 270065e92c31 ("scsi-mq: Always unprepare before
requeuing a request") was yesterday in linux-next but not the ipr fix
(commit b0e17a9b0df2 ("scsi: ipr: Fix scsi-mq lockdep issue")). The
ipr fix is in today's linux-next but was not in linux-next yesterday (the
Australian time zone applies to the linux-next date labels):
$ git tag --contains 270065e92c31
next-20170816
$ git tag --contains b0e17a9b0df2
next-20170817
$ git show next-20170816 | head -3
tag next-20170816
Tagger: Stephen Rothwell <sfr@canb.auug.org.au>
Date:   Wed Aug 16 15:26:43 2017 +1000
$ git show next-20170817 | head -3  
tag next-20170817
Tagger: Stephen Rothwell <sfr@canb.auug.org.au>
Date:   Thu Aug 17 16:34:30 2017 +1000

I think this means that the ipr fix went upstream before it ended up in
linux-next.

Bart.

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Martin K. Petersen]

Bart,

> I think this means that the ipr fix went upstream before it ended up in
> linux-next.

OK.

Brian: Please make sure you test with your ipr change in place as well
as Bart's patch. And then report back.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Brian King]

On 08/17/2017 11:45 AM, Martin K. Petersen wrote:
> 
> Bart,
> 
>> I think this means that the ipr fix went upstream before it ended up in
>> linux-next.
> 
> OK.
> 
> Brian: Please make sure you test with your ipr change in place as well
> as Bart's patch. And then report back.

They are separate issues. The ipr fix that is now upstream is for a
locking issue in ipr. The boot hang issue is a separate issue. I
can reproduce the boot hang on a system with my ipr patch. I've
confirmed it goes away when reverting Bart's patch but it makes
no sense as of yet as to why. 

-Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Martin K. Petersen]

Brian,

> They are separate issues. The ipr fix that is now upstream is for a
> locking issue in ipr. The boot hang issue is a separate issue. I can
> reproduce the boot hang on a system with my ipr patch. I've confirmed
> it goes away when reverting Bart's patch but it makes no sense as of
> yet as to why.

OK. Thanks for the update. Let us know if there is something we can do
to help.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Michael Ellerman]

Bart Van Assche <Bart.VanAssche@wdc.com> writes:

> On Wed, 2017-08-16 at 22:51 -0400, Martin K. Petersen wrote:
>> > When I checked earlier today the ipr patch was not yet in linux-next
>> 
>> That's weird. They were both committed two weeks ago.
>> 
>> They appear to be in there now, though:
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/drivers/scsi?ofs=50
>
> Hello Martin,
>
> As far as I can see commit 270065e92c31 ("scsi-mq: Always unprepare before
> requeuing a request") was yesterday in linux-next but not the ipr fix
> (commit b0e17a9b0df2 ("scsi: ipr: Fix scsi-mq lockdep issue")). The
> ipr fix is in today's linux-next but was not in linux-next yesterday (the
> Australian time zone applies to the linux-next date labels):
> $ git tag --contains 270065e92c31
> next-20170816
> $ git tag --contains b0e17a9b0df2
> next-20170817
> $ git show next-20170816 | head -3
> tag next-20170816
> Tagger: Stephen Rothwell <sfr@canb.auug.org.au>
> Date:   Wed Aug 16 15:26:43 2017 +1000
> $ git show next-20170817 | head -3  
> tag next-20170817
> Tagger: Stephen Rothwell <sfr@canb.auug.org.au>
> Date:   Thu Aug 17 16:34:30 2017 +1000
>
> I think this means that the ipr fix went upstream before it ended up in
> linux-next.

It was in linux-next, but as a different commit. I don't know why.

$ git log -1 --format=oneline next-20170816 drivers/scsi/ipr.c
48b580cacfae123471f8cd43ca81b0e53c9cf702 scsi: ipr: Fix scsi-mq lockdep issue


$ git tag --contains 48b580cacfae123471f8cd43ca81b0e53c9cf702
next-20170809
next-20170810
next-20170811
next-20170815
next-20170816


cheers

Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"

[Martin K. Petersen]

Damien,

> It means that commit 70e42fd02c46e2aa9ab07b766d418637e3a51de7 "scsi:
> sd_zbc: Write unlock zone from sd_uninit_cmnd()" will need to be
> reverted too as it will not solve the potential deadlock anymore. Bart's
> patch was needed for it to work.

OK, also dropped.

> (2) may actually be the better solution for now until I get a fix for
> blk-mq.

Yeah, that's fine with me.

-- 
Martin K. Petersen	Oracle Linux Engineering