Re: [PATCH] bfa: fix access to bfad_im_port_s
James Bottomley writes:
> On Tue, 2017-11-28 at 16:26 +0100, Johannes Thumshirn wrote:
>> Commit 'cd21c605b2cf ("scsi: fc: provide fc_bsg_to_shost() helper")'
>> changed access to bfa's 'struct bfad_im_port_s' by using shost_priv()
>> instead of shost->hostdata[0].
>>
>> This lead to crashes like in the following back-trace:
>>
>> task: 880046375300 ti: 8800a2ef8000 task.ti: 8800a2ef8000
>> RIP: e030:[] []
>> bfa_fcport_get_attr+0x82/0x260 [bfa]
>> RSP: e02b:8800a2efba10 EFLAGS: 00010046
>> RAX: 575f415441536432 RBX: 8800a2efba28 RCX:
>> RDX: RSI: 8800a2efba28 RDI: 880004dc31d8
>> RBP: 880004dc31d8 R08: R09: 0001
>> R10: 88011fadc468 R11: 0001 R12: 880004dc31f0
>> R13: 0200 R14: 880004dc61d0 R15: 880004947a10
>> FS: 7feb1e489700() GS:88011fac()
>> knlGS:
>> CS: e033 DS: ES: CR0: 8005003b
>> CR2: 7ffe14e46c10 CR3: 957b8000 CR4: 0660
>> Stack:
>> 88001d4da000 880004dc31c0 a048a9df 81e56380
>>
>> [] bfad_iocmd_ioc_get_info+0x4f/0x220 [bfa]
>> [] bfad_iocmd_handler+0xa00/0xd40 [bfa]
>> [] bfad_im_bsg_request+0xee/0x1b0 [bfa]
>> [] fc_bsg_dispatch+0x10b/0x1b0 [scsi_transport_fc]
>> [] bsg_request_fn+0x11d/0x1c0
>> [] __blk_run_queue+0x2f/0x40
>> [] blk_execute_rq_nowait+0xa8/0x160
>> [] blk_execute_rq+0x77/0x120
>> [] bsg_ioctl+0x1b6/0x200
>> [] do_vfs_ioctl+0x2cd/0x4a0
>> [] SyS_ioctl+0x74/0x80
>> [] entry_SYSCALL_64_fastpath+0x12/0x6d
>>
>> Fixes: cd21c605b2cf ("scsi: fc: provide fc_bsg_to_shost() helper")
>> Signed-off-by: Johannes Thumshirn
>> Cc: Michal Koutný
>> ---
>> drivers/scsi/bfa/bfad_bsg.c | 6 --
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/bfa/bfad_bsg.c
>> b/drivers/scsi/bfa/bfad_bsg.c
>> index 72ca2a2e08e2..09ef68c8225f 100644
>> --- a/drivers/scsi/bfa/bfad_bsg.c
>> +++ b/drivers/scsi/bfa/bfad_bsg.c
>> @@ -3135,7 +3135,8 @@ bfad_im_bsg_vendor_request(struct bsg_job *job)
>> struct fc_bsg_request *bsg_request = job->request;
>> struct fc_bsg_reply *bsg_reply = job->reply;
>> uint32_t vendor_cmd = bsg_request-
>> >rqst_data.h_vendor.vendor_cmd[0];
>> -struct bfad_im_port_s *im_port =
>> shost_priv(fc_bsg_to_shost(job));
>> +struct Scsi_Host *shost = fc_bsg_to_shost(job);
>> +struct bfad_im_port_s *im_port = shost->hostdata[0];
>> struct bfad_s *bfad = im_port->bfad;
>> void *payload_kbuf;
>> int rc = -EINVAL;
>> @@ -3350,7 +3351,8 @@ int
>> bfad_im_bsg_els_ct_request(struct bsg_job *job)
>> {
>> struct bfa_bsg_data *bsg_data;
>> -struct bfad_im_port_s *im_port =
>> shost_priv(fc_bsg_to_shost(job));
>> +struct Scsi_Host *shost = fc_bsg_to_shost(job);
>> +struct bfad_im_port_s *im_port = shost->hostdata[0];
>> struct bfad_s *bfad = im_port->bfad;
>> bfa_bsg_fcpt_t *bsg_fcpt;
>> struct bfad_fcxp*drv_fcxp;
>
> OK, so we had a linux next failure with this:
>
> After merging the scsi tree, today's linux-next build (x86_64
> allmodconfig) produced these warnings:
>
> drivers/scsi/bfa/bfad_bsg.c: In function 'bfad_im_bsg_vendor_request':
> drivers/scsi/bfa/bfad_bsg.c:3137:35: warning: initialization makes
> pointer from integer without a cast [-Wint-conversion]
> struct bfad_im_port_s *im_port = shost->hostdata[0];
>^
> drivers/scsi/bfa/bfad_bsg.c: In function 'bfad_im_bsg_els_ct_request':
> drivers/scsi/bfa/bfad_bsg.c:3353:35: warning: initialization makes
> pointer from integer without a cast [-Wint-conversion]
> struct bfad_im_port_s *im_port = shost->hostdata[0];
>^
>
> Introduced by commit
>
> 45349821ab3a ("scsi: bfa: fix access to bfad_im_port_s")
>
> --
> Cheers,
> Stephen Rothwell
>
> The reason is you should be using shost_priv(shost) not shost->hostdata[0].
Yes, but using shost_priv() was resulting in the crash beforehand.
shost_priv() doesn't do the same actually:
static inline void *shost_priv(struct Scsi_Host *shost)
{
return (void *)shost->hostdata;
}
vs
shost->hostdata[0];
and:
drivers/scsi/bfa/bfad_im.c:567: im_port->shost->hostdata[0] = (unsigned
long)im_port;
The quick fix would be:
diff --git a/drivers/scsi/bfa/bfad_bsg.c b/drivers/scsi/bfa/bfad_bsg.c
index 09ef68c8225f..075185db533f 100644
--- a/drivers/scsi/bfa/bfad_bsg.c
+++ b/drivers/scsi/bfa/bfad_bsg.c
@@ -3136,7 +3136,7 @@ bfad_im_bsg_vendor_request(struct bsg_job *job)
struct fc_bsg_reply *bsg_reply = job->reply;
uint32_t vendor_cmd = bsg_request->rqst_data.h_vendor.vendor_cmd[0];
struct Scsi_Host *shost = fc_bsg_to_shost(job);
- struct bfad_im_port_s *im_port = shost->hostdata[0];
+ struct bfad_im_po
[PATCH] scsi: bfa: fix type conversion warning
A regression fix introduced a harmless type mismatch warning:
drivers/scsi/bfa/bfad_bsg.c: In function 'bfad_im_bsg_vendor_request':
drivers/scsi/bfa/bfad_bsg.c:3137:35: error: initialization of 'struct
bfad_im_port_s *' from 'long unsigned int' makes pointer from integer without a
cast [-Werror=int-conversion]
struct bfad_im_port_s *im_port = shost->hostdata[0];
^
drivers/scsi/bfa/bfad_bsg.c: In function 'bfad_im_bsg_els_ct_request':
drivers/scsi/bfa/bfad_bsg.c:3353:35: error: initialization of 'struct
bfad_im_port_s *' from 'long unsigned int' makes pointer from integer without a
cast [-Werror=int-conversion]
struct bfad_im_port_s *im_port = shost->hostdata[0];
This changes the code back to shost_priv() once more, but encapsulates
it in an inline function to document the rather unusual way of
using the private data only as a pointer to the previously allocated
structure.
I did not try to get rid of the extra indirection level entirely,
which would have been rather invasive and required reworking the entire
initialization sequence.
Fixes: 45349821ab3a ("scsi: bfa: fix access to bfad_im_port_s")
Signed-off-by: Arnd Bergmann
---
drivers/scsi/bfa/bfad_bsg.c | 4 ++--
drivers/scsi/bfa/bfad_im.c | 6 --
drivers/scsi/bfa/bfad_im.h | 10 ++
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/bfa/bfad_bsg.c b/drivers/scsi/bfa/bfad_bsg.c
index fa9a966e2c2a..3976e787ba64 100644
--- a/drivers/scsi/bfa/bfad_bsg.c
+++ b/drivers/scsi/bfa/bfad_bsg.c
@@ -3134,7 +3134,7 @@ bfad_im_bsg_vendor_request(struct bsg_job *job)
struct fc_bsg_reply *bsg_reply = job->reply;
uint32_t vendor_cmd = bsg_request->rqst_data.h_vendor.vendor_cmd[0];
struct Scsi_Host *shost = fc_bsg_to_shost(job);
- struct bfad_im_port_s *im_port = shost->hostdata[0];
+ struct bfad_im_port_s *im_port = bfad_get_im_port(shost);
struct bfad_s *bfad = im_port->bfad;
void *payload_kbuf;
int rc = -EINVAL;
@@ -3350,7 +3350,7 @@ bfad_im_bsg_els_ct_request(struct bsg_job *job)
{
struct bfa_bsg_data *bsg_data;
struct Scsi_Host *shost = fc_bsg_to_shost(job);
- struct bfad_im_port_s *im_port = shost->hostdata[0];
+ struct bfad_im_port_s *im_port = bfad_get_im_port(shost);
struct bfad_s *bfad = im_port->bfad;
bfa_bsg_fcpt_t *bsg_fcpt;
struct bfad_fcxp*drv_fcxp;
diff --git a/drivers/scsi/bfa/bfad_im.c b/drivers/scsi/bfa/bfad_im.c
index 24e657a4ec80..c05d6e91e4bd 100644
--- a/drivers/scsi/bfa/bfad_im.c
+++ b/drivers/scsi/bfa/bfad_im.c
@@ -546,6 +546,7 @@ int
bfad_im_scsi_host_alloc(struct bfad_s *bfad, struct bfad_im_port_s *im_port,
struct device *dev)
{
+ struct bfad_im_port_pointer *im_portp;
int error = 1;
mutex_lock(&bfad_mutex);
@@ -564,7 +565,8 @@ bfad_im_scsi_host_alloc(struct bfad_s *bfad, struct
bfad_im_port_s *im_port,
goto out_free_idr;
}
- im_port->shost->hostdata[0] = (unsigned long)im_port;
+ im_portp = shost_priv(im_port->shost);
+ im_portp->p = im_port;
im_port->shost->unique_id = im_port->idr_id;
im_port->shost->this_id = -1;
im_port->shost->max_id = MAX_FCP_TARGET;
@@ -748,7 +750,7 @@ bfad_scsi_host_alloc(struct bfad_im_port_s *im_port, struct
bfad_s *bfad)
sht->sg_tablesize = bfad->cfg_data.io_max_sge;
- return scsi_host_alloc(sht, sizeof(unsigned long));
+ return scsi_host_alloc(sht, sizeof(struct bfad_im_port_pointer));
}
void
diff --git a/drivers/scsi/bfa/bfad_im.h b/drivers/scsi/bfa/bfad_im.h
index 7f7616c52814..af66275570c3 100644
--- a/drivers/scsi/bfa/bfad_im.h
+++ b/drivers/scsi/bfa/bfad_im.h
@@ -69,6 +69,16 @@ struct bfad_im_port_s {
struct fc_vport *fc_vport;
};
+struct bfad_im_port_pointer {
+ struct bfad_im_port_s *p;
+};
+
+static inline struct bfad_im_port_s *bfad_get_im_port(struct Scsi_Host *host)
+{
+ struct bfad_im_port_pointer *im_portp = shost_priv(host);
+ return im_portp->p;
+}
+
enum bfad_itnim_state {
ITNIM_STATE_NONE,
ITNIM_STATE_ONLINE,
--
2.9.0
[PATCH] scsi: fusion: clean up some indentations
From: Colin Ian King
There are several places where the source is not indented correctly
with either too many or too few levels of intentation. Fix these.
Signed-off-by: Colin Ian King
---
drivers/message/fusion/mptbase.c | 57
1 file changed, 29 insertions(+), 28 deletions(-)
diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 7a93400eea2a..8df37fa1e977 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -958,7 +958,7 @@ mpt_put_msg_frame(u8 cb_idx, MPT_ADAPTER *ioc,
MPT_FRAME_HDR *mf)
{
u32 mf_dma_addr;
int req_offset;
- u16 req_idx; /* Request index */
+ u16 req_idx;/* Request index */
/* ensure values are reset properly! */
mf->u.frame.hwhdr.msgctxu.fld.cb_idx = cb_idx; /* byte */
@@ -994,7 +994,7 @@ mpt_put_msg_frame_hi_pri(u8 cb_idx, MPT_ADAPTER *ioc,
MPT_FRAME_HDR *mf)
{
u32 mf_dma_addr;
int req_offset;
- u16 req_idx; /* Request index */
+ u16 req_idx;/* Request index */
/* ensure values are reset properly! */
mf->u.frame.hwhdr.msgctxu.fld.cb_idx = cb_idx;
@@ -1128,11 +1128,12 @@ mpt_add_sge_64bit_1078(void *pAddr, u32 flagslength,
dma_addr_t dma_addr)
static void
mpt_add_chain(void *pAddr, u8 next, u16 length, dma_addr_t dma_addr)
{
- SGEChain32_t *pChain = (SGEChain32_t *) pAddr;
- pChain->Length = cpu_to_le16(length);
- pChain->Flags = MPI_SGE_FLAGS_CHAIN_ELEMENT;
- pChain->NextChainOffset = next;
- pChain->Address = cpu_to_le32(dma_addr);
+ SGEChain32_t *pChain = (SGEChain32_t *) pAddr;
+
+ pChain->Length = cpu_to_le16(length);
+ pChain->Flags = MPI_SGE_FLAGS_CHAIN_ELEMENT;
+ pChain->NextChainOffset = next;
+ pChain->Address = cpu_to_le32(dma_addr);
}
/*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
@@ -1147,18 +1148,18 @@ mpt_add_chain(void *pAddr, u8 next, u16 length,
dma_addr_t dma_addr)
static void
mpt_add_chain_64bit(void *pAddr, u8 next, u16 length, dma_addr_t dma_addr)
{
- SGEChain64_t *pChain = (SGEChain64_t *) pAddr;
- u32 tmp = dma_addr & 0x;
+ SGEChain64_t *pChain = (SGEChain64_t *) pAddr;
+ u32 tmp = dma_addr & 0x;
- pChain->Length = cpu_to_le16(length);
- pChain->Flags = (MPI_SGE_FLAGS_CHAIN_ELEMENT |
-MPI_SGE_FLAGS_64_BIT_ADDRESSING);
+ pChain->Length = cpu_to_le16(length);
+ pChain->Flags = (MPI_SGE_FLAGS_CHAIN_ELEMENT |
+MPI_SGE_FLAGS_64_BIT_ADDRESSING);
- pChain->NextChainOffset = next;
+ pChain->NextChainOffset = next;
- pChain->Address.Low = cpu_to_le32(tmp);
- tmp = (u32)(upper_32_bits(dma_addr));
- pChain->Address.High = cpu_to_le32(tmp);
+ pChain->Address.Low = cpu_to_le32(tmp);
+ tmp = (u32)(upper_32_bits(dma_addr));
+ pChain->Address.High = cpu_to_le32(tmp);
}
/*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
@@ -1360,7 +1361,7 @@ mpt_host_page_alloc(MPT_ADAPTER *ioc, pIOCInit_t ioc_init)
ioc->add_sge(psge, flags_length, ioc->HostPageBuffer_dma);
ioc->facts.HostPageBufferSGE = ioc_init->HostPageBufferSGE;
-return 0;
+ return 0;
}
/*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
@@ -2152,7 +2153,7 @@ mpt_suspend(struct pci_dev *pdev, pm_message_t state)
device_state);
/* put ioc into READY_STATE */
- if(SendIocReset(ioc, MPI_FUNCTION_IOC_MESSAGE_UNIT_RESET, CAN_SLEEP)) {
+ if (SendIocReset(ioc, MPI_FUNCTION_IOC_MESSAGE_UNIT_RESET, CAN_SLEEP)) {
printk(MYIOC_s_ERR_FMT
"pci-suspend: IOC msg unit reset failed!\n", ioc->name);
}
@@ -6348,7 +6349,7 @@ mpt_config(MPT_ADAPTER *ioc, CONFIGPARMS *pCfg)
u8 page_type = 0, extend_page;
unsigned longtimeleft;
unsigned longflags;
-int in_isr;
+ int in_isr;
u8 issue_hard_reset = 0;
u8 retry_count = 0;
@@ -8092,15 +8093,15 @@ mpt_spi_log_info(MPT_ADAPTER *ioc, u32 log_info)
static void
mpt_sas_log_info(MPT_ADAPTER *ioc, u32 log_info, u8 cb_idx)
{
-union loginfo_type {
- u32 loginfo;
- struct {
- u32 subcode:16;
- u32 code:8;
- u32 originator:4;
- u32 bus_type:4;
- }dw;
-};
+ union loginfo_type {
+ u32 loginfo;
+ struct {
+ u32 subcode:16;
+ u32 code:8;
+ u32 originator:4;
+ u32 bus_type:4;
Re: [PATCH] bfa: fix access to bfad_im_port_s
On Wed, 2017-12-06 at 09:07 +0100, Johannes Thumshirn wrote:
> James Bottomley writes:
> >
> > On Tue, 2017-11-28 at 16:26 +0100, Johannes Thumshirn wrote:
> > >
> > > Commit 'cd21c605b2cf ("scsi: fc: provide fc_bsg_to_shost()
> > > helper")'
> > > changed access to bfa's 'struct bfad_im_port_s' by using
> > > shost_priv()
> > > instead of shost->hostdata[0].
> > >
> > > This lead to crashes like in the following back-trace:
> > >
> > > task: 880046375300 ti: 8800a2ef8000 task.ti:
> > > 8800a2ef8000
> > > RIP: e030:[] []
> > > bfa_fcport_get_attr+0x82/0x260 [bfa]
> > > RSP: e02b:8800a2efba10 EFLAGS: 00010046
> > > RAX: 575f415441536432 RBX: 8800a2efba28 RCX:
> > > RDX: RSI: 8800a2efba28 RDI: 880004dc31d8
> > > RBP: 880004dc31d8 R08: R09: 0001
> > > R10: 88011fadc468 R11: 0001 R12: 880004dc31f0
> > > R13: 0200 R14: 880004dc61d0 R15: 880004947a10
> > > FS: 7feb1e489700() GS:88011fac()
> > > knlGS:
> > > CS: e033 DS: ES: CR0: 8005003b
> > > CR2: 7ffe14e46c10 CR3: 957b8000 CR4: 0660
> > > Stack:
> > > 88001d4da000 880004dc31c0 a048a9df
> > > 81e56380
> > >
> > >
> > > [] bfad_iocmd_ioc_get_info+0x4f/0x220 [bfa]
> > > [] bfad_iocmd_handler+0xa00/0xd40 [bfa]
> > > [] bfad_im_bsg_request+0xee/0x1b0 [bfa]
> > > [] fc_bsg_dispatch+0x10b/0x1b0 [scsi_transport_fc]
> > > [] bsg_request_fn+0x11d/0x1c0
> > > [] __blk_run_queue+0x2f/0x40
> > > [] blk_execute_rq_nowait+0xa8/0x160
> > > [] blk_execute_rq+0x77/0x120
> > > [] bsg_ioctl+0x1b6/0x200
> > > [] do_vfs_ioctl+0x2cd/0x4a0
> > > [] SyS_ioctl+0x74/0x80
> > > [] entry_SYSCALL_64_fastpath+0x12/0x6d
> > >
> > > Fixes: cd21c605b2cf ("scsi: fc: provide fc_bsg_to_shost()
> > > helper")
> > > Signed-off-by: Johannes Thumshirn
> > > Cc: Michal Koutný
> > > ---
> > > drivers/scsi/bfa/bfad_bsg.c | 6 --
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/scsi/bfa/bfad_bsg.c
> > > b/drivers/scsi/bfa/bfad_bsg.c
> > > index 72ca2a2e08e2..09ef68c8225f 100644
> > > --- a/drivers/scsi/bfa/bfad_bsg.c
> > > +++ b/drivers/scsi/bfa/bfad_bsg.c
> > > @@ -3135,7 +3135,8 @@ bfad_im_bsg_vendor_request(struct bsg_job
> > > *job)
> > > struct fc_bsg_request *bsg_request = job->request;
> > > struct fc_bsg_reply *bsg_reply = job->reply;
> > > uint32_t vendor_cmd = bsg_request-
> > > >
> > > > rqst_data.h_vendor.vendor_cmd[0];
> > > - struct bfad_im_port_s *im_port =
> > > shost_priv(fc_bsg_to_shost(job));
> > > + struct Scsi_Host *shost = fc_bsg_to_shost(job);
> > > + struct bfad_im_port_s *im_port = shost->hostdata[0];
> > > struct bfad_s *bfad = im_port->bfad;
> > > void *payload_kbuf;
> > > int rc = -EINVAL;
> > > @@ -3350,7 +3351,8 @@ int
> > > bfad_im_bsg_els_ct_request(struct bsg_job *job)
> > > {
> > > struct bfa_bsg_data *bsg_data;
> > > - struct bfad_im_port_s *im_port =
> > > shost_priv(fc_bsg_to_shost(job));
> > > + struct Scsi_Host *shost = fc_bsg_to_shost(job);
> > > + struct bfad_im_port_s *im_port = shost->hostdata[0];
> > > struct bfad_s *bfad = im_port->bfad;
> > > bfa_bsg_fcpt_t *bsg_fcpt;
> > > struct bfad_fcxp*drv_fcxp;
> >
> > OK, so we had a linux next failure with this:
> >
> > After merging the scsi tree, today's linux-next build (x86_64
> > allmodconfig) produced these warnings:
> >
> > drivers/scsi/bfa/bfad_bsg.c: In function
> > 'bfad_im_bsg_vendor_request':
> > drivers/scsi/bfa/bfad_bsg.c:3137:35: warning: initialization
> > makes pointer from integer without a cast [-Wint-conversion]
> > struct bfad_im_port_s *im_port = shost->hostdata[0];
> > ^
> > drivers/scsi/bfa/bfad_bsg.c: In function
> > 'bfad_im_bsg_els_ct_request':
> > drivers/scsi/bfa/bfad_bsg.c:3353:35: warning: initialization
> > makes pointer from integer without a cast [-Wint-conversion]
> > struct bfad_im_port_s *im_port = shost->hostdata[0];
> > ^
> >
> > Introduced by commit
> >
> > 45349821ab3a ("scsi: bfa: fix access to bfad_im_port_s")
> >
> > --
> > Cheers,
> > Stephen Rothwell
> >
> > The reason is you should be using shost_priv(shost) not shost-
> > >hostdata[0].
>
>
> Yes, but using shost_priv() was resulting in the crash beforehand.
>
> shost_priv() doesn't do the same actually:
> static inline void *shost_priv(struct Scsi_Host *shost)
> {
> return (void *)shost->hostdata;
> }
> vs
> shost->hostdata[0];
> and:
> drivers/scsi/bfa/bfad_im.c:567: im_port->shost->hostdata[0] =
> (unsigned
> long)im_port;
>
> The quick fix would be:
>
> diff --git a/drivers/scsi/bfa/bfad_bsg.c
> b/drivers/scsi/bfa/bfad_bsg.c
> index 09ef68c8225f..075185db533f 100644
> --- a
Re: [PATCH] SCSI: run queue if SCSI device queue isn't ready and queue is idle
On Wed, 2017-12-06 at 09:52 +0800, Ming Lei wrote:
> On Wed, Dec 06, 2017 at 12:28:25AM +0800, Ming Lei wrote:
> > On Tue, Dec 05, 2017 at 04:08:20PM +, Bart Van Assche wrote:
> > > The patch below is not a full solution but resulted in a significant
> > > improvement in my tests:
> > >
> > > diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> > > index 69e3226e66ca..9d86876ec503 100644
> > > --- a/block/blk-mq-sched.c
> > > +++ b/block/blk-mq-sched.c
> > > @@ -226,6 +226,7 @@ void blk_mq_sched_dispatch_requests(struct
> > > blk_mq_hw_ctx *hctx)
> > >* TODO: get more budgets, and dequeue more requests in
> > >* one time.
> > >*/
> > > + blk_mq_sched_mark_restart_hctx(hctx);
> > > blk_mq_do_dispatch_ctx(hctx);
> > > } else {
> > > blk_mq_flush_busy_ctxs(hctx, &rq_list);
>
> BTW, this kind of change can't cover scsi_set_blocked() which is
> triggered by timeout, scsi dispatch failure. You will see that
> easily if you run the SCSI test script I provided in the commit log.
Hello Ming,
I am aware that the above change does not cover all cases. That's why I wrote
in my previous e-mail that that patch is not a full solution. The reason I
posted that change anyway is because I prefer a solution that is not based on
delayed queue runs over a solution that is based on delayed queue runs
(blk_mq_delay_run_hw_queue()). My concern is that performance of a solution
based on delayed queue runs will be suboptimal.
Bart.
[PATCH 18/45] drivers: scsi: qla2xxx: remove duplicate includes
These duplicate includes have been found with scripts/checkincludes.pl but
they have been removed manually to avoid removing false positives.
Signed-off-by: Pravin Shedge
---
drivers/scsi/qla2xxx/qla_nx2.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_nx2.c b/drivers/scsi/qla2xxx/qla_nx2.c
index 0aa9c38..525ac35 100644
--- a/drivers/scsi/qla2xxx/qla_nx2.c
+++ b/drivers/scsi/qla2xxx/qla_nx2.c
@@ -11,8 +11,6 @@
#include "qla_def.h"
#include "qla_gbl.h"
-#include
-
#define TIMEOUT_100_MS 100
static const uint32_t qla8044_reg_tbl[] = {
--
2.7.4
Re: [PATCH] SCSI: run queue if SCSI device queue isn't ready and queue is idle
On 12/05/17 08:52, Ming Lei wrote:
> Before commit 0df21c86bdbf ("scsi: implement .get_budget and .put_budget
> for blk-mq"), we run queue after 3ms if queue is idle and SCSI device
> queue isn't ready, which is done in handling BLK_STS_RESOURCE. After
> commit 0df21c86bdbf is introduced, queue won't be run any more under
> this situation.
>
> IO hang is observed when timeout happened, and this patch fixes the IO
> hang issue by running queue after delay in scsi_dev_queue_ready, just like
> non-mq. This issue can be triggered by the following script[1].
>
> There is another issue which can be covered by running idle queue:
> when .get_budget() is called on request coming from hctx->dispatch_list,
> if one request just completes during .get_budget(), we can't depend on
> SCSI's restart to make progress any more. This patch fixes the race too.
>
> With this patch, we basically recover to previous behaviour(before commit
> 0df21c86bdbf) of handling idle queue when running out of resource.
>
> [1] script for test/verify SCSI timeout
> rmmod scsi_debug
> modprobe scsi_debug max_queue=1
>
> DEVICE=`ls -d
> /sys/bus/pseudo/drivers/scsi_debug/adapter*/host*/target*/*/block/* | head -1
> | xargs basename`
> DISK_DIR=`ls -d /sys/block/$DEVICE/device/scsi_disk/*`
>
> echo "using scsi device $DEVICE"
> echo "-1" >/sys/bus/pseudo/drivers/scsi_debug/every_nth
> echo "temporary write through" >$DISK_DIR/cache_type
> echo "128" >/sys/bus/pseudo/drivers/scsi_debug/opts
> echo none > /sys/block/$DEVICE/queue/scheduler
> dd if=/dev/$DEVICE of=/dev/null bs=1M iflag=direct count=1 &
> sleep 5
> echo "0" >/sys/bus/pseudo/drivers/scsi_debug/opts
> wait
> echo "SUCCESS"
>
> Fixes: 0df21c86bdbf ("scsi: implement .get_budget and .put_budget for blk-mq")
> Signed-off-by: Ming Lei
> ---
> drivers/scsi/scsi_lib.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index db9556662e27..1816dd8259b3 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1967,6 +1967,8 @@ static bool scsi_mq_get_budget(struct blk_mq_hw_ctx
> *hctx)
> out_put_device:
> put_device(&sdev->sdev_gendev);
> out:
> + if (atomic_read(&sdev->device_busy) == 0 && !scsi_device_blocked(sdev))
> + blk_mq_delay_run_hw_queue(hctx, SCSI_QUEUE_DELAY);
> return false;
> }
So just to follow up on this: with this patch I haven't encountered any
new hangs with blk-mq, regardless of medium (SSD/rotating disk) or scheduler.
I cannot speak for other hangs that may be reproducible by other means,
but for now here's my:
Tested-by: Holger Hoffstätte
cheers,
Holger
Re: [PATCH 0/17] scsi: arcmsr: add some driver options and support new adapter ARC-1884
Ching, > Due to 4.16/scsi-queue conflict with 4.15-rc1, so I resubmit these > patches again. The following patches apply to Linus' 4.15-rc1 tree. Thanks for reworking this series! I have applied it to 4.16/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] SCSI: run queue if SCSI device queue isn't ready and queue is idle
On Wed, Dec 06, 2017 at 04:07:17PM +, Bart Van Assche wrote:
> On Wed, 2017-12-06 at 09:52 +0800, Ming Lei wrote:
> > On Wed, Dec 06, 2017 at 12:28:25AM +0800, Ming Lei wrote:
> > > On Tue, Dec 05, 2017 at 04:08:20PM +, Bart Van Assche wrote:
> > > > The patch below is not a full solution but resulted in a significant
> > > > improvement in my tests:
> > > >
> > > > diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> > > > index 69e3226e66ca..9d86876ec503 100644
> > > > --- a/block/blk-mq-sched.c
> > > > +++ b/block/blk-mq-sched.c
> > > > @@ -226,6 +226,7 @@ void blk_mq_sched_dispatch_requests(struct
> > > > blk_mq_hw_ctx *hctx)
> > > > * TODO: get more budgets, and dequeue more requests in
> > > > * one time.
> > > > */
> > > > + blk_mq_sched_mark_restart_hctx(hctx);
> > > > blk_mq_do_dispatch_ctx(hctx);
> > > > } else {
> > > > blk_mq_flush_busy_ctxs(hctx, &rq_list);
> >
> > BTW, this kind of change can't cover scsi_set_blocked() which is
> > triggered by timeout, scsi dispatch failure. You will see that
> > easily if you run the SCSI test script I provided in the commit log.
>
> Hello Ming,
>
> I am aware that the above change does not cover all cases. That's why I wrote
> in my previous e-mail that that patch is not a full solution. The reason I
> posted that change anyway is because I prefer a solution that is not based on
> delayed queue runs over a solution that is based on delayed queue runs
> (blk_mq_delay_run_hw_queue()). My concern is that performance of a solution
> based on delayed queue runs will be suboptimal.
Hi,
The patch I posted won't cause any performance regression because it is
only triggered when queue is becoming idle, also that is exact the way for us
to deal with these cases before.
But if you always call blk_mq_sched_mark_restart_hctx() before a new
dispatch, that may affect performance on NVMe which may never trigger
BLK_STS_RESOURCE.
--
Ming
Re: [PATCH] SCSI: run queue if SCSI device queue isn't ready and queue is idle
On Thu, Dec 07, 2017 at 12:10:51AM +0100, Holger Hoffstätte wrote:
> On 12/05/17 08:52, Ming Lei wrote:
> > Before commit 0df21c86bdbf ("scsi: implement .get_budget and .put_budget
> > for blk-mq"), we run queue after 3ms if queue is idle and SCSI device
> > queue isn't ready, which is done in handling BLK_STS_RESOURCE. After
> > commit 0df21c86bdbf is introduced, queue won't be run any more under
> > this situation.
> >
> > IO hang is observed when timeout happened, and this patch fixes the IO
> > hang issue by running queue after delay in scsi_dev_queue_ready, just like
> > non-mq. This issue can be triggered by the following script[1].
> >
> > There is another issue which can be covered by running idle queue:
> > when .get_budget() is called on request coming from hctx->dispatch_list,
> > if one request just completes during .get_budget(), we can't depend on
> > SCSI's restart to make progress any more. This patch fixes the race too.
> >
> > With this patch, we basically recover to previous behaviour(before commit
> > 0df21c86bdbf) of handling idle queue when running out of resource.
> >
> > [1] script for test/verify SCSI timeout
> > rmmod scsi_debug
> > modprobe scsi_debug max_queue=1
> >
> > DEVICE=`ls -d
> > /sys/bus/pseudo/drivers/scsi_debug/adapter*/host*/target*/*/block/* | head
> > -1 | xargs basename`
> > DISK_DIR=`ls -d /sys/block/$DEVICE/device/scsi_disk/*`
> >
> > echo "using scsi device $DEVICE"
> > echo "-1" >/sys/bus/pseudo/drivers/scsi_debug/every_nth
> > echo "temporary write through" >$DISK_DIR/cache_type
> > echo "128" >/sys/bus/pseudo/drivers/scsi_debug/opts
> > echo none > /sys/block/$DEVICE/queue/scheduler
> > dd if=/dev/$DEVICE of=/dev/null bs=1M iflag=direct count=1 &
> > sleep 5
> > echo "0" >/sys/bus/pseudo/drivers/scsi_debug/opts
> > wait
> > echo "SUCCESS"
> >
> > Fixes: 0df21c86bdbf ("scsi: implement .get_budget and .put_budget for
> > blk-mq")
> > Signed-off-by: Ming Lei
> > ---
> > drivers/scsi/scsi_lib.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index db9556662e27..1816dd8259b3 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -1967,6 +1967,8 @@ static bool scsi_mq_get_budget(struct blk_mq_hw_ctx
> > *hctx)
> > out_put_device:
> > put_device(&sdev->sdev_gendev);
> > out:
> > + if (atomic_read(&sdev->device_busy) == 0 && !scsi_device_blocked(sdev))
> > + blk_mq_delay_run_hw_queue(hctx, SCSI_QUEUE_DELAY);
> > return false;
> > }
>
> So just to follow up on this: with this patch I haven't encountered any
> new hangs with blk-mq, regardless of medium (SSD/rotating disk) or scheduler.
> I cannot speak for other hangs that may be reproducible by other means,
> but for now here's my:
>
> Tested-by: Holger Hoffstätte
Hi Holger,
That is great to see this patch fixes your issue, and thanks for your
test!
Jens, Martin, would any of you mind making this patch in V4.15? Since
it fixes real use cases and this way is exact what we do before
0df21c86bdbf("scsi: implement .get_budget and .put_budget for blk-mq").
Thanks,
Ming
Re: [PATCH] scsi: libsas: fix length error in sas_smp_handler()
Can anybody review this patch? Our test of SG_IO all failed because of
this bug.
On 2017/12/5 17:39, Jason Yan wrote:
The bsg_job_done() requires the length of payload received, but we give
it the untransferred residual.
Fixes: 651a01364994 ("scsi: scsi_transport_sas: switch to bsg-lib for SMP")
Reported-and-tested-by: chenqilin
Signed-off-by: Jason Yan
CC: Christoph Hellwig
---
drivers/scsi/libsas/sas_expander.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/libsas/sas_expander.c
b/drivers/scsi/libsas/sas_expander.c
index 50cb0f3..8323dc1 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -2177,9 +2177,9 @@ void sas_smp_handler(struct bsg_job *job, struct
Scsi_Host *shost,
ret = smp_execute_task_sg(dev, job->request_payload.sg_list,
job->reply_payload.sg_list);
- if (ret > 0) {
+ if (ret >= 0) {
/* positive number is the untransferred residual */
- reslen = ret;
+ reslen = job->reply_payload.payload_len - ret;
ret = 0;
}
Re: [PATCH v3 0/2] Ensure that the SCSI error handler gets woken up
Bart, > As reported by Pavel Tikhomirov it can happen that the SCSI error > handler does not get woken up. This is very annoying because it > results in a queue stall. The two patches in this series address this > issue without acquiring the SCSI host lock in the hot path. Please > consider these patches for kernel v4.16. Applied to 4.16/scsi-queue. Thank you! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 0/3] SCSI device blacklist handling improvements
Bart, > These three patches is what I came up with after having reviewed > recent changes in the code for handling blacklist flags > handling. Please consider these patches for kernel v4.16. I applied 1 and 3 to 4.16/scsi-queue. I am still not a fan of forcing u32. That's a recipe for disaster when we add the next flag. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v2] scsi_debug: add cdb_len paramete
Doug, > While testing "sd: Micro-optimize READ / WRITE CDB encoding" patches > it was helpful to check various code paths associated with READ/WRITE > 6, 10 and 16 byte cdb variants. There seems to be no user space > "knobs" to twiddle use_10_for_rw and friends in the scsi_device > structure. So add a parameter to scsi_debug called "cdb_len" for this > purpose. Applied to 4.16/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v3 00/22] qla2xxx: Bug fixes for 4.15-rc2
Himanshu, >> This looks pretty big for a series of bug fixes. Are all these >> patches really candidates for 4.15 and stable backports all the way >> back to 4.10? > Yes Please. I would want them back ported to 4.10 since these issues > were discovered in combination of 4.10/4.11 kernel. James already sent Linus a pull request for this week. So that would put your series out another week. 22 patches and 400+ insertions. That's a lot of churn for rc4, and while the changes are indeed fixes and not new features, the whole thing still looks like a regular merge window driver update to me. This series really should have been posted before the 4.15 merge window opened. However, the first iteration of this series was posted after rc1 was out. I'm terribly sorry, but that's just too late for something this size. I queued everything up for 4.16. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v3 00/22] qla2xxx: Bug fixes for 4.15-rc2
Hi Martin, > On Dec 6, 2017, at 6:43 PM, Martin K. Petersen > wrote: > > > Himanshu, > >>> This looks pretty big for a series of bug fixes. Are all these >>> patches really candidates for 4.15 and stable backports all the way >>> back to 4.10? > >> Yes Please. I would want them back ported to 4.10 since these issues >> were discovered in combination of 4.10/4.11 kernel. > > James already sent Linus a pull request for this week. So that would put > your series out another week. 22 patches and 400+ insertions. That's a > lot of churn for rc4, and while the changes are indeed fixes and not new > features, the whole thing still looks like a regular merge window driver > update to me. > > This series really should have been posted before the 4.15 merge window > opened. However, the first iteration of this series was posted after rc1 > was out. I'm terribly sorry, but that's just too late for something this > size. > > I queued everything up for 4.16. > Understood the concern and i am okay this to be queued for 4.16 > -- > Martin K. PetersenOracle Linux Engineering Thanks, - Himanshu
Re: [PATCH v3 1/2] Ensure that the SCSI error handler gets woken up
In case (a) scsi_eh_inc_host_failed() will wake up the error handler. And in case (b) scsi_dec_host_busy() will wake up the error handler. So it's not clear to me why you think that there is a scenario in which the EH won't be woken up? >>> >>> So in case (b), in my understanding, scsi_dec_host_busy can skip wakeups >>> as it does not see host_failed change yet. >> >> That's not correct. If scsi_dec_host_busy() obtains the SCSI host lock before >> scsi_eh_inc_host_failed() obtains it then the latter function will trigger a >> SCSI EH wakeup. > > You are right! Thanks a lot for pointing that out! Now when I understand it, > your patch looks good for me: > > Reviewed-by: Pavel Tikhomirov > > By the way, I very much like your idea of using rcu for these case. > > Thanks, Pavel. > This patch tests ok on my system, too... it's run for over 24 hours, on a system that typically fails within ten minutes without the patch... Tested-by: Stuart Hayes Thanks, Stuart --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
