All of lore.kernel.org
 help / color / mirror / Atom feed
* [GIT PULL] SCSI fixes for 4.13-rc6
@ 2017-08-23  6:42 James Bottomley
  2017-08-23 15:02   ` Bart Van Assche
  0 siblings, 1 reply; 12+ messages in thread
From: James Bottomley @ 2017-08-23  6:42 UTC (permalink / raw)
  To: Andrew Morton, Linus Torvalds; +Cc: linux-scsi, linux-kernel

Six minor and error leg fixes, plus one major change: the reversion of
scsi-mq as the default.  We're doing the latter temporarily (with a
backport to stable) to give us time to fix all the issues that turned
up with this default before trying again.

The patch is available here:

git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git scsi-fixes

The short changelog is:

Christoph Hellwig (1):
      Revert "scsi: default to scsi-mq"

Damien Le Moal (1):
      scsi: sd_zbc: Write unlock zone from sd_uninit_cmnd()

Raghava Aditya Renukunta (1):
      scsi: aacraid: Fix out of bounds in aac_get_name_resp

Varun Prakash (2):
      scsi: cxgb4i: call neigh_event_send() to update MAC address
      scsi: csiostor: fail probe if fw does not support FCoE

weiping zhang (1):
      scsi: megaraid_sas: fix error handle in megasas_probe_one

And the diffstat:

 drivers/scsi/Kconfig                      | 11 +++++++++++
 drivers/scsi/aacraid/aachba.c             |  9 +++++++--
 drivers/scsi/aacraid/aacraid.h            |  2 +-
 drivers/scsi/csiostor/csio_hw.c           |  4 +++-
 drivers/scsi/csiostor/csio_init.c         | 12 ++++++++----
 drivers/scsi/cxgbi/cxgb4i/cxgb4i.c        |  3 +++
 drivers/scsi/megaraid/megaraid_sas_base.c |  2 +-
 drivers/scsi/scsi.c                       |  4 ++++
 drivers/scsi/sd.c                         |  3 +++
 drivers/scsi/sd_zbc.c                     |  9 +++++----
 include/scsi/scsi_cmnd.h                  |  1 +
 11 files changed, 47 insertions(+), 13 deletions(-)

With full diff below.

James

---

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index f4538d7a3016..d145e0d90227 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -47,6 +47,17 @@ config SCSI_NETLINK
 	default	n
 	depends on NET
 
+config SCSI_MQ_DEFAULT
+	bool "SCSI: use blk-mq I/O path by default"
+	depends on SCSI
+	---help---
+	  This option enables the new blk-mq based I/O path for SCSI
+	  devices by default.  With the option the scsi_mod.use_blk_mq
+	  module/boot option defaults to Y, without it to N, but it can
+	  still be overridden either way.
+
+	  If unsure say N.
+
 config SCSI_PROC_FS
 	bool "legacy /proc/scsi/ support"
 	depends on SCSI && PROC_FS
diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 4591113c49de..a1a2c71e1626 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -549,7 +549,9 @@ static void get_container_name_callback(void *context, struct fib * fibptr)
 	if ((le32_to_cpu(get_name_reply->status) == CT_OK)
 	 && (get_name_reply->data[0] != '\0')) {
 		char *sp = get_name_reply->data;
-		sp[sizeof(((struct aac_get_name_resp *)NULL)->data)] = '\0';
+		int data_size = FIELD_SIZEOF(struct aac_get_name_resp, data);
+
+		sp[data_size - 1] = '\0';
 		while (*sp == ' ')
 			++sp;
 		if (*sp) {
@@ -579,12 +581,15 @@ static void get_container_name_callback(void *context, struct fib * fibptr)
 static int aac_get_container_name(struct scsi_cmnd * scsicmd)
 {
 	int status;
+	int data_size;
 	struct aac_get_name *dinfo;
 	struct fib * cmd_fibcontext;
 	struct aac_dev * dev;
 
 	dev = (struct aac_dev *)scsicmd->device->host->hostdata;
 
+	data_size = FIELD_SIZEOF(struct aac_get_name_resp, data);
+
 	cmd_fibcontext = aac_fib_alloc_tag(dev, scsicmd);
 
 	aac_fib_init(cmd_fibcontext);
@@ -593,7 +598,7 @@ static int aac_get_container_name(struct scsi_cmnd * scsicmd)
 	dinfo->command = cpu_to_le32(VM_ContainerConfig);
 	dinfo->type = cpu_to_le32(CT_READ_NAME);
 	dinfo->cid = cpu_to_le32(scmd_id(scsicmd));
-	dinfo->count = cpu_to_le32(sizeof(((struct aac_get_name_resp *)NULL)->data));
+	dinfo->count = cpu_to_le32(data_size - 1);
 
 	status = aac_fib_send(ContainerCommand,
 		  cmd_fibcontext,
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index d31a9bc2ba69..ee2667e20e42 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -2274,7 +2274,7 @@ struct aac_get_name_resp {
 	__le32		parm3;
 	__le32		parm4;
 	__le32		parm5;
-	u8		data[16];
+	u8		data[17];
 };
 
 #define CT_CID_TO_32BITS_UID 165
diff --git a/drivers/scsi/csiostor/csio_hw.c b/drivers/scsi/csiostor/csio_hw.c
index 2029ad225121..5be0086142ca 100644
--- a/drivers/scsi/csiostor/csio_hw.c
+++ b/drivers/scsi/csiostor/csio_hw.c
@@ -3845,8 +3845,10 @@ csio_hw_start(struct csio_hw *hw)
 
 	if (csio_is_hw_ready(hw))
 		return 0;
-	else
+	else if (csio_match_state(hw, csio_hws_uninit))
 		return -EINVAL;
+	else
+		return -ENODEV;
 }
 
 int
diff --git a/drivers/scsi/csiostor/csio_init.c b/drivers/scsi/csiostor/csio_init.c
index ea0c31086cc6..dcd074169aa9 100644
--- a/drivers/scsi/csiostor/csio_init.c
+++ b/drivers/scsi/csiostor/csio_init.c
@@ -969,10 +969,14 @@ static int csio_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	pci_set_drvdata(pdev, hw);
 
-	if (csio_hw_start(hw) != 0) {
-		dev_err(&pdev->dev,
-			"Failed to start FW, continuing in debug mode.\n");
-		return 0;
+	rv = csio_hw_start(hw);
+	if (rv) {
+		if (rv == -EINVAL) {
+			dev_err(&pdev->dev,
+				"Failed to start FW, continuing in debug mode.\n");
+			return 0;
+		}
+		goto err_lnode_exit;
 	}
 
 	sprintf(hw->fwrev_str, "%u.%u.%u.%u\n",
diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
index a69a9ac836f5..1d02cf9fe06c 100644
--- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
+++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
@@ -1635,6 +1635,9 @@ static int init_act_open(struct cxgbi_sock *csk)
 		goto rel_resource;
 	}
 
+	if (!(n->nud_state & NUD_VALID))
+		neigh_event_send(n, NULL);
+
 	csk->atid = cxgb4_alloc_atid(lldi->tids, csk);
 	if (csk->atid < 0) {
 		pr_err("%s, NO atid available.\n", ndev->name);
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index 316c3df0c3fd..71c4746341ea 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -6228,8 +6228,8 @@ static int megasas_probe_one(struct pci_dev *pdev,
 fail_start_aen:
 fail_io_attach:
 	megasas_mgmt_info.count--;
-	megasas_mgmt_info.instance[megasas_mgmt_info.max_index] = NULL;
 	megasas_mgmt_info.max_index--;
+	megasas_mgmt_info.instance[megasas_mgmt_info.max_index] = NULL;
 
 	instance->instancet->disable_intr(instance);
 	megasas_destroy_irqs(instance);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3d38c6d463b8..1bf274e3b2b6 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -800,7 +800,11 @@ MODULE_LICENSE("GPL");
 module_param(scsi_logging_level, int, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(scsi_logging_level, "a bit mask of logging levels");
 
+#ifdef CONFIG_SCSI_MQ_DEFAULT
 bool scsi_use_blk_mq = true;
+#else
+bool scsi_use_blk_mq = false;
+#endif
 module_param_named(use_blk_mq, scsi_use_blk_mq, bool, S_IWUSR | S_IRUGO);
 
 static int __init init_scsi(void)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index bea36adeee17..e2647f2d4430 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1277,6 +1277,9 @@ static void sd_uninit_command(struct scsi_cmnd *SCpnt)
 {
 	struct request *rq = SCpnt->request;
 
+	if (SCpnt->flags & SCMD_ZONE_WRITE_LOCK)
+		sd_zbc_write_unlock_zone(SCpnt);
+
 	if (rq->rq_flags & RQF_SPECIAL_PAYLOAD)
 		__free_page(rq->special_vec.bv_page);
 
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 96855df9f49d..8aa54779aac1 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -294,6 +294,9 @@ int sd_zbc_write_lock_zone(struct scsi_cmnd *cmd)
 	    test_and_set_bit(zno, sdkp->zones_wlock))
 		return BLKPREP_DEFER;
 
+	WARN_ON_ONCE(cmd->flags & SCMD_ZONE_WRITE_LOCK);
+	cmd->flags |= SCMD_ZONE_WRITE_LOCK;
+
 	return BLKPREP_OK;
 }
 
@@ -302,9 +305,10 @@ void sd_zbc_write_unlock_zone(struct scsi_cmnd *cmd)
 	struct request *rq = cmd->request;
 	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
 
-	if (sdkp->zones_wlock) {
+	if (sdkp->zones_wlock && cmd->flags & SCMD_ZONE_WRITE_LOCK) {
 		unsigned int zno = sd_zbc_zone_no(sdkp, blk_rq_pos(rq));
 		WARN_ON_ONCE(!test_bit(zno, sdkp->zones_wlock));
+		cmd->flags &= ~SCMD_ZONE_WRITE_LOCK;
 		clear_bit_unlock(zno, sdkp->zones_wlock);
 		smp_mb__after_atomic();
 	}
@@ -335,9 +339,6 @@ void sd_zbc_complete(struct scsi_cmnd *cmd,
 	case REQ_OP_WRITE_ZEROES:
 	case REQ_OP_WRITE_SAME:
 
-		/* Unlock the zone */
-		sd_zbc_write_unlock_zone(cmd);
-
 		if (result &&
 		    sshdr->sense_key == ILLEGAL_REQUEST &&
 		    sshdr->asc == 0x21)
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index a1266d318c85..6af198d8120b 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -57,6 +57,7 @@ struct scsi_pointer {
 /* for scmd->flags */
 #define SCMD_TAGGED		(1 << 0)
 #define SCMD_UNCHECKED_ISA_DMA	(1 << 1)
+#define SCMD_ZONE_WRITE_LOCK	(1 << 2)
 
 struct scsi_cmnd {
 	struct scsi_request req;

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [GIT PULL] SCSI fixes for 4.13-rc6
  2017-08-23  6:42 [GIT PULL] SCSI fixes for 4.13-rc6 James Bottomley
@ 2017-08-23 15:02   ` Bart Van Assche
  0 siblings, 0 replies; 12+ messages in thread
From: Bart Van Assche @ 2017-08-23 15:02 UTC (permalink / raw)
  To: torvalds, James.Bottomley, akpm; +Cc: linux-scsi, linux-kernel

On Wed, 2017-08-23 at 07:42 +0100, James Bottomley wrote:
> Six minor and error leg fixes, plus one major change: the reversion of
> scsi-mq as the default.  We're doing the latter temporarily (with a
> backport to stable) to give us time to fix all the issues that turned
> up with this default before trying again.
> [ ... ]
> Damien Le Moal (1):
>       scsi: sd_zbc: Write unlock zone from sd_uninit_cmnd()
> [ ... ]

Hello James,

Had you noticed that Damien had asked not to send the "sd_zbc: Write unlock
zone from sd_uninit_cmnd()" patch to Linus without my "scsi-mq: Always
unprepare before requeuing a request" patch? A quote from an e-mail from
Damien (https://www.spinics.net/lists/stable/msg185436.html):

---------------------------------------------------------------------------
Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"
>> For an unknown reason this patch causes the boot process to hang on
>> PowerPC systems:
> 
> OK, dropped it from fixes for now.

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.
---------------------------------------------------------------------------

Bart.

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [GIT PULL] SCSI fixes for 4.13-rc6
@ 2017-08-23 15:02   ` Bart Van Assche
  0 siblings, 0 replies; 12+ messages in thread
From: Bart Van Assche @ 2017-08-23 15:02 UTC (permalink / raw)
  To: torvalds, James.Bottomley, akpm; +Cc: linux-scsi, linux-kernel

On Wed, 2017-08-23 at 07:42 +0100, James Bottomley wrote:
> Six minor and error leg fixes, plus one major change: the reversion of
> scsi-mq as the default.  We're doing the latter temporarily (with a
> backport to stable) to give us time to fix all the issues that turned
> up with this default before trying again.
> [ ... ]
> Damien Le Moal (1):
>       scsi: sd_zbc: Write unlock zone from sd_uninit_cmnd()
> [ ... ]

Hello James,

Had you noticed that Damien had asked not to send the "sd_zbc: Write unlock
zone from sd_uninit_cmnd()" patch to Linus without my "scsi-mq: Always
unprepare before requeuing a request" patch? A quote from an e-mail from
Damien (https://www.spinics.net/lists/stable/msg185436.html):

---------------------------------------------------------------------------
Re: [PATCH] Revert "scsi-mq: Always unprepare before requeuing a request"
>> For an unknown reason this patch causes the boot process to hang on
>> PowerPC systems:
> 
> OK, dropped it from fixes for now.

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.
---------------------------------------------------------------------------

Bart.

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [GIT PULL] SCSI fixes for 4.13-rc6
  2017-08-23 15:02   ` Bart Van Assche
@ 2017-08-23 15:27     ` Martin K. Petersen
  -1 siblings, 0 replies; 12+ messages in thread
From: Martin K. Petersen @ 2017-08-23 15:27 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: torvalds, James.Bottomley, akpm, linux-scsi, linux-kernel


Hi Bart,

> Had you noticed that Damien had asked not to send the "sd_zbc: Write
> unlock zone from sd_uninit_cmnd()" patch to Linus without my "scsi-mq:
> Always unprepare before requeuing a request" patch?

He did change his mind later in that thread, though.

However, what's more important is that we still need a good version of
your patch for 4.13. I took Brian's workaround for ipr but I still think
Christoph's concerns need to be addressed for me to put your change back
in.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [GIT PULL] SCSI fixes for 4.13-rc6
@ 2017-08-23 15:27     ` Martin K. Petersen
  0 siblings, 0 replies; 12+ messages in thread
From: Martin K. Petersen @ 2017-08-23 15:27 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: torvalds, James.Bottomley, akpm, linux-scsi, linux-kernel


Hi Bart,

> Had you noticed that Damien had asked not to send the "sd_zbc: Write
> unlock zone from sd_uninit_cmnd()" patch to Linus without my "scsi-mq:
> Always unprepare before requeuing a request" patch?

He did change his mind later in that thread, though.

However, what's more important is that we still need a good version of
your patch for 4.13. I took Brian's workaround for ipr but I still think
Christoph's concerns need to be addressed for me to put your change back
in.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [GIT PULL] SCSI fixes for 4.13-rc6
  2017-08-23 15:27     ` Martin K. Petersen
@ 2017-08-23 15:44       ` Bart Van Assche
  -1 siblings, 0 replies; 12+ messages in thread
From: Bart Van Assche @ 2017-08-23 15:44 UTC (permalink / raw)
  To: martin.petersen; +Cc: torvalds, James.Bottomley, linux-kernel, linux-scsi, akpm

On Wed, 2017-08-23 at 11:27 -0400, Martin K. Petersen wrote:
> However, what's more important is that we still need a good version of
> your patch for 4.13. I took Brian's workaround for ipr but I still think
> Christoph's concerns need to be addressed for me to put your change back
> in.

Hello Martin,

I am not aware of any requests to modify the patch "scsi-mq: Always unprepare
before requeuing a request". See also
https://www.spinics.net/lists/linux-scsi/msg111541.html. Are you perhaps
referring to another patch?

What I remember is that my patch uncovered a bug in the ipr driver. As you
mentioned, a workaround for that bug has already been queued for kernel v4.14
(https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=4.13/scsi-fixes&id=723cd772fde2344a9810eeaf5106787d535ec4a4).

Further improvements for the SCSI core that are the result of the analysis of
the behavior of the SCSI subsystem on PowerPC systems are under discussion. See
also "[PATCHv2 1/2] scsi: Move scsi_cmd->jiffies_at_alloc initialization to
allocation time" (https://marc.info/?l=linux-next&m=150335524812989) and
"[PATCH 2/2] scsi: Preserve retry counter through scsi_prep_fn"
(https://marc.info/?l=linux-scsi&m=150335371112485).

Please correct me if I got anything wrong.

Bart.

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [GIT PULL] SCSI fixes for 4.13-rc6
@ 2017-08-23 15:44       ` Bart Van Assche
  0 siblings, 0 replies; 12+ messages in thread
From: Bart Van Assche @ 2017-08-23 15:44 UTC (permalink / raw)
  To: martin.petersen; +Cc: torvalds, James.Bottomley, linux-kernel, linux-scsi, akpm

On Wed, 2017-08-23 at 11:27 -0400, Martin K. Petersen wrote:
> However, what's more important is that we still need a good version of
> your patch for 4.13. I took Brian's workaround for ipr but I still think
> Christoph's concerns need to be addressed for me to put your change back
> in.

Hello Martin,

I am not aware of any requests to modify the patch "scsi-mq: Always unprepare
before requeuing a request". See also
https://www.spinics.net/lists/linux-scsi/msg111541.html. Are you perhaps
referring to another patch?

What I remember is that my patch uncovered a bug in the ipr driver. As you
mentioned, a workaround for that bug has already been queued for kernel v4.14
(https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=4.13/scsi-fixes&id=723cd772fde2344a9810eeaf5106787d535ec4a4).

Further improvements for the SCSI core that are the result of the analysis of
the behavior of the SCSI subsystem on PowerPC systems are under discussion. See
also "[PATCHv2 1/2] scsi: Move scsi_cmd->jiffies_at_alloc initialization to
allocation time" (https://marc.info/?l=linux-next&m=150335524812989) and
"[PATCH 2/2] scsi: Preserve retry counter through scsi_prep_fn"
(https://marc.info/?l=linux-scsi&m=150335371112485).

Please correct me if I got anything wrong.

Bart.

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [GIT PULL] SCSI fixes for 4.13-rc6
  2017-08-23 15:44       ` Bart Van Assche
@ 2017-08-23 19:13         ` Brian King
  -1 siblings, 0 replies; 12+ messages in thread
From: Brian King @ 2017-08-23 19:13 UTC (permalink / raw)
  To: Bart Van Assche, martin.petersen
  Cc: torvalds, James.Bottomley, linux-kernel, linux-scsi, akpm

On 08/23/2017 10:44 AM, Bart Van Assche wrote:
> On Wed, 2017-08-23 at 11:27 -0400, Martin K. Petersen wrote:
>> However, what's more important is that we still need a good version of
>> your patch for 4.13. I took Brian's workaround for ipr but I still think
>> Christoph's concerns need to be addressed for me to put your change back
>> in.
> 
> Hello Martin,
> 
> I am not aware of any requests to modify the patch "scsi-mq: Always unprepare
> before requeuing a request". See also
> https://www.spinics.net/lists/linux-scsi/msg111541.html. Are you perhaps
> referring to another patch?
> 
> What I remember is that my patch uncovered a bug in the ipr driver. As you
> mentioned, a workaround for that bug has already been queued for kernel v4.14
> (https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=4.13/scsi-fixes&id=723cd772fde2344a9810eeaf5106787d535ec4a4).

I don't completely agree with that statement. The patch "scsi-mq: Always unprepare
before requeueing a request" introduces a regression in the scsi stack.
It alters the behavior of retries such that the retry counter no longer works
and the jiffies_at_alloc use to ensure we don't spend a tremendous amount of
time retrying ops gets broken as well.

As far as ipr is concerned, we do have the workaround in place now and I'll also
queue up a further improvement to ipr to return a better failure response. However,
until we fix the retry counter in scsi, any driver that returns an error response that
scsi wants to retry could get us stuck in an eternal retry loop, like we were
seeing with ipr.

> Further improvements for the SCSI core that are the result of the analysis of
> the behavior of the SCSI subsystem on PowerPC systems are under discussion. See
> also "[PATCHv2 1/2] scsi: Move scsi_cmd->jiffies_at_alloc initialization to
> allocation time" (https://marc.info/?l=linux-next&m=150335524812989) and
> "[PATCH 2/2] scsi: Preserve retry counter through scsi_prep_fn"
> (https://marc.info/?l=linux-scsi&m=150335371112485).

While my patches highlight the problem, I don't think they are the right fix
and need to be reworked. It looks like we go through scsi_init_rq at
hctx setup time rather than for each new i/o submission. Adding a simple
kprobe to scsi_init_rq never triggers when issuing i/o to already configured
devices. Therefore, we cannot simply move the initialization of jiffies_at_alloc to
scsi_init_rq. I've been working on moving the retry counter and jiffies_at_alloc
into struct scsi_request, as that doesn't get reinitialized.

Thanks,

Brian 

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [GIT PULL] SCSI fixes for 4.13-rc6
@ 2017-08-23 19:13         ` Brian King
  0 siblings, 0 replies; 12+ messages in thread
From: Brian King @ 2017-08-23 19:13 UTC (permalink / raw)
  To: Bart Van Assche, martin.petersen
  Cc: torvalds, James.Bottomley, linux-kernel, linux-scsi, akpm

On 08/23/2017 10:44 AM, Bart Van Assche wrote:
> On Wed, 2017-08-23 at 11:27 -0400, Martin K. Petersen wrote:
>> However, what's more important is that we still need a good version of
>> your patch for 4.13. I took Brian's workaround for ipr but I still think
>> Christoph's concerns need to be addressed for me to put your change back
>> in.
> 
> Hello Martin,
> 
> I am not aware of any requests to modify the patch "scsi-mq: Always unprepare
> before requeuing a request". See also
> https://www.spinics.net/lists/linux-scsi/msg111541.html. Are you perhaps
> referring to another patch?
> 
> What I remember is that my patch uncovered a bug in the ipr driver. As you
> mentioned, a workaround for that bug has already been queued for kernel v4.14
> (https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=4.13/scsi-fixes&id=723cd772fde2344a9810eeaf5106787d535ec4a4).

I don't completely agree with that statement. The patch "scsi-mq: Always unprepare
before requeueing a request" introduces a regression in the scsi stack.
It alters the behavior of retries such that the retry counter no longer works
and the jiffies_at_alloc use to ensure we don't spend a tremendous amount of
time retrying ops gets broken as well.

As far as ipr is concerned, we do have the workaround in place now and I'll also
queue up a further improvement to ipr to return a better failure response. However,
until we fix the retry counter in scsi, any driver that returns an error response that
scsi wants to retry could get us stuck in an eternal retry loop, like we were
seeing with ipr.

> Further improvements for the SCSI core that are the result of the analysis of
> the behavior of the SCSI subsystem on PowerPC systems are under discussion. See
> also "[PATCHv2 1/2] scsi: Move scsi_cmd->jiffies_at_alloc initialization to
> allocation time" (https://marc.info/?l=linux-next&m=150335524812989) and
> "[PATCH 2/2] scsi: Preserve retry counter through scsi_prep_fn"
> (https://marc.info/?l=linux-scsi&m=150335371112485).

While my patches highlight the problem, I don't think they are the right fix
and need to be reworked. It looks like we go through scsi_init_rq at
hctx setup time rather than for each new i/o submission. Adding a simple
kprobe to scsi_init_rq never triggers when issuing i/o to already configured
devices. Therefore, we cannot simply move the initialization of jiffies_at_alloc to
scsi_init_rq. I've been working on moving the retry counter and jiffies_at_alloc
into struct scsi_request, as that doesn't get reinitialized.

Thanks,

Brian 

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [GIT PULL] SCSI fixes for 4.13-rc6
  2017-08-23 15:27     ` Martin K. Petersen
@ 2017-08-24  0:49       ` Damien Le Moal
  -1 siblings, 0 replies; 12+ messages in thread
From: Damien Le Moal @ 2017-08-24  0:49 UTC (permalink / raw)
  To: Martin K. Petersen, Bart Van Assche
  Cc: torvalds, James.Bottomley, akpm, linux-scsi, linux-kernel


On 8/24/17 00:27, Martin K. Petersen wrote:
> 
> Hi Bart,
> 
>> Had you noticed that Damien had asked not to send the "sd_zbc: Write
>> unlock zone from sd_uninit_cmnd()" patch to Linus without my "scsi-mq:
>> Always unprepare before requeuing a request" patch?
> 
> He did change his mind later in that thread, though.
> 
> However, what's more important is that we still need a good version of
> your patch for 4.13. I took Brian's workaround for ipr but I still think
> Christoph's concerns need to be addressed for me to put your change back
> in.

Yes, I did ask for the patch to be kept since it does not make things
worse for the SMR/mq case (still have a potential dispatch deadlock) but
does cleanup the code in the legacy/sq case.

Since it looks like the jiffies_at_alloc and retry counter
initialization fixes are progressing, we hopefully should be able to get
Bart's pacth back in soon (this week ?) and remove the mq dispatch deadlock.

Best regards.

-- 
Damien Le Moal,
Western Digital Research

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [GIT PULL] SCSI fixes for 4.13-rc6
@ 2017-08-24  0:49       ` Damien Le Moal
  0 siblings, 0 replies; 12+ messages in thread
From: Damien Le Moal @ 2017-08-24  0:49 UTC (permalink / raw)
  To: Martin K. Petersen, Bart Van Assche
  Cc: torvalds, James.Bottomley, akpm, linux-scsi, linux-kernel


On 8/24/17 00:27, Martin K. Petersen wrote:
> 
> Hi Bart,
> 
>> Had you noticed that Damien had asked not to send the "sd_zbc: Write
>> unlock zone from sd_uninit_cmnd()" patch to Linus without my "scsi-mq:
>> Always unprepare before requeuing a request" patch?
> 
> He did change his mind later in that thread, though.
> 
> However, what's more important is that we still need a good version of
> your patch for 4.13. I took Brian's workaround for ipr but I still think
> Christoph's concerns need to be addressed for me to put your change back
> in.

Yes, I did ask for the patch to be kept since it does not make things
worse for the SMR/mq case (still have a potential dispatch deadlock) but
does cleanup the code in the legacy/sq case.

Since it looks like the jiffies_at_alloc and retry counter
initialization fixes are progressing, we hopefully should be able to get
Bart's pacth back in soon (this week ?) and remove the mq dispatch deadlock.

Best regards.

-- 
Damien Le Moal,
Western Digital Research

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [GIT PULL] SCSI fixes for 4.13-rc6
@ 2017-08-30 13:29 James Bottomley
  0 siblings, 0 replies; 12+ messages in thread
From: James Bottomley @ 2017-08-30 13:29 UTC (permalink / raw)
  To: Andrew Morton, Linus Torvalds; +Cc: linux-scsi, linux-kernel

Three minor fixes: a NULL deref in qedf, an off by one in sg and a fix
to IPR to prevent an error on initialisation.

The patch is available here:

git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git scsi-fixes

The short changelog is:

Brian King (1):
      scsi: ipr: Set no_report_opcodes for RAID arrays

Christophe JAILLET (1):
      scsi: qedf: Fix a potential NULL pointer dereference

Dan Carpenter (1):
      scsi: sg: off by one in sg_ioctl()

And the diffstat:

 drivers/scsi/ipr.c           |  1 +
 drivers/scsi/qedf/qedf_els.c | 14 ++++++++------
 drivers/scsi/sg.c            |  2 +-
 3 files changed, 10 insertions(+), 7 deletions(-)

With full diff below.

James

---

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index da5bdbdcce52..f838bd73befa 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -4945,6 +4945,7 @@ static int ipr_slave_configure(struct scsi_device *sdev)
 		}
 		if (ipr_is_vset_device(res)) {
 			sdev->scsi_level = SCSI_SPC_3;
+			sdev->no_report_opcodes = 1;
 			blk_queue_rq_timeout(sdev->request_queue,
 					     IPR_VSET_RW_TIMEOUT);
 			blk_queue_max_hw_sectors(sdev->request_queue, IPR_VSET_MAX_SECTORS);
diff --git a/drivers/scsi/qedf/qedf_els.c b/drivers/scsi/qedf/qedf_els.c
index eb07f1de8afa..59c18ca4cda9 100644
--- a/drivers/scsi/qedf/qedf_els.c
+++ b/drivers/scsi/qedf/qedf_els.c
@@ -489,7 +489,7 @@ static void qedf_srr_compl(struct qedf_els_cb_arg *cb_arg)
 
 	/* If a SRR times out, simply free resources */
 	if (srr_req->event == QEDF_IOREQ_EV_ELS_TMO)
-		goto out_free;
+		goto out_put;
 
 	/* Normalize response data into struct fc_frame */
 	mp_req = &(srr_req->mp_req);
@@ -501,7 +501,7 @@ static void qedf_srr_compl(struct qedf_els_cb_arg *cb_arg)
 	if (!fp) {
 		QEDF_ERR(&(qedf->dbg_ctx),
 		    "fc_frame_alloc failure.\n");
-		goto out_free;
+		goto out_put;
 	}
 
 	/* Copy frame header from firmware into fp */
@@ -526,9 +526,10 @@ static void qedf_srr_compl(struct qedf_els_cb_arg *cb_arg)
 	}
 
 	fc_frame_free(fp);
-out_free:
+out_put:
 	/* Put reference for original command since SRR completed */
 	kref_put(&orig_io_req->refcount, qedf_release_cmd);
+out_free:
 	kfree(cb_arg);
 }
 
@@ -780,7 +781,7 @@ static void qedf_rec_compl(struct qedf_els_cb_arg *cb_arg)
 
 	/* If a REC times out, free resources */
 	if (rec_req->event == QEDF_IOREQ_EV_ELS_TMO)
-		goto out_free;
+		goto out_put;
 
 	/* Normalize response data into struct fc_frame */
 	mp_req = &(rec_req->mp_req);
@@ -792,7 +793,7 @@ static void qedf_rec_compl(struct qedf_els_cb_arg *cb_arg)
 	if (!fp) {
 		QEDF_ERR(&(qedf->dbg_ctx),
 		    "fc_frame_alloc failure.\n");
-		goto out_free;
+		goto out_put;
 	}
 
 	/* Copy frame header from firmware into fp */
@@ -884,9 +885,10 @@ static void qedf_rec_compl(struct qedf_els_cb_arg *cb_arg)
 
 out_free_frame:
 	fc_frame_free(fp);
-out_free:
+out_put:
 	/* Put reference for original command since REC completed */
 	kref_put(&orig_io_req->refcount, qedf_release_cmd);
+out_free:
 	kfree(cb_arg);
 }
 
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(&sfp->rq_list_lock, iflags);
 			val = 0;
 			list_for_each_entry(srp, &sfp->rq_list, entry) {
-				if (val > SG_MAX_QUEUE)
+				if (val >= SG_MAX_QUEUE)
 					break;
 				memset(&rinfo[val], 0, SZ_SG_REQ_INFO);
 				rinfo[val].req_state = srp->done + 1;

^ permalink raw reply related	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-08-30 13:30 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-23  6:42 [GIT PULL] SCSI fixes for 4.13-rc6 James Bottomley
2017-08-23 15:02 ` Bart Van Assche
2017-08-23 15:02   ` Bart Van Assche
2017-08-23 15:27   ` Martin K. Petersen
2017-08-23 15:27     ` Martin K. Petersen
2017-08-23 15:44     ` Bart Van Assche
2017-08-23 15:44       ` Bart Van Assche
2017-08-23 19:13       ` Brian King
2017-08-23 19:13         ` Brian King
2017-08-24  0:49     ` Damien Le Moal
2017-08-24  0:49       ` Damien Le Moal
2017-08-30 13:29 James Bottomley

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.