Re: [PATCH] scsi_dh: Document alua_rtpg_queue() arguments
Looks good, Reviewed-by: Johannes Thumshirn
Re: [PATCH target-pending] iscsi-target: make sure to wake up sleeping login worker
Hey Florian & Co,
On Fri, 2018-01-19 at 18:26 +0100, Florian Westphal wrote:
> Eric Dumazet wrote:
> > On Fri, 2018-01-19 at 14:36 +0100, Florian Westphal wrote:
> > > diff --git a/drivers/target/iscsi/iscsi_target_nego.c
> > > b/drivers/target/iscsi/iscsi_target_nego.c
> > > index b686e2ce9c0e..3723f8f419aa 100644
> > > --- a/drivers/target/iscsi/iscsi_target_nego.c
> > > +++ b/drivers/target/iscsi/iscsi_target_nego.c
> > > @@ -432,6 +432,9 @@ static void iscsi_target_sk_data_ready(struct sock
> > > *sk)
> > > if (test_and_set_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags)) {
> > > write_unlock_bh(&sk->sk_callback_lock);
> > > pr_debug("Got LOGIN_FLAGS_READ_ACTIVE=1, conn: %p \n",
> > > conn);
> > > + if (WARN_ON(iscsi_target_sk_data_ready ==
> > > conn->orig_data_ready))
> > > + return;
> >
> > Is this WARN_ON() belonging to this fix ?
> > At least make it WARN_ON_ONCE() or pr_err_once()
>
> Nicholas, I don't know this code at all so it would be good if you could
> give advice here (omit all together, WARN_ON_ONCE, ...).
>
This is regular behavior during multi PDU login sequences, and should
not include a WARN_ON.
So with MNC's Tested-by in place, applying to target-pending/for-next
minus the WARN_ON, with a extra 4.14.y stable tag.
Thanks again for taking a look at this.
To your earlier point wrt net.ipv4.tcp_low_latency=1 on 4.13 code not
triggering pre-queue logic. From groking the original patch to drop
prequeue I agree this should really be the case, but am still at a loss
how MNC is triggering on 4.14+ unless something else has changed to
uncover this iscsi-target bug.
Still curious to verify the root cause, but I haven't been able to
reproduce this in VMs on small scale, and haven't had cycles to
reproduce on HW yet.
That said, since the bug appears to be masked on <= 4.13.y +
tcp_low_latency=1, unless someone can reproduce this on earlier code
with tcp_low_latency=0, I'll leave off the older stable tag for now.
Re: [PATCH] scsi_dh: Document alua_rtpg_queue() arguments
On 01/24/2018 12:50 AM, Bart Van Assche wrote:
> Since commit 3a025e1d1c2e ("Add optional check for bad kernel-doc
> comments") building with W=1 causes warnings to appear for issues
> in kernel-doc headers. This patch avoids that the following warnings
> are reported when building with W=1:
>
> drivers/scsi/device_handler/scsi_dh_alua.c:867: warning: No description found
> for parameter 'pg'
> drivers/scsi/device_handler/scsi_dh_alua.c:867: warning: No description found
> for parameter 'sdev'
> drivers/scsi/device_handler/scsi_dh_alua.c:867: warning: No description found
> for parameter 'qdata'
> drivers/scsi/device_handler/scsi_dh_alua.c:867: warning: No description found
> for parameter 'force'
>
> Signed-off-by: Bart Van Assche
> Cc: Christoph Hellwig
> Cc: Hannes Reinecke
> Cc: Johannes Thumshirn
> ---
> drivers/scsi/device_handler/scsi_dh_alua.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
> b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 022e421c2185..4b44325d1a82 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -876,6 +876,11 @@ static void alua_rtpg_work(struct work_struct *work)
>
> /**
> * alua_rtpg_queue() - cause RTPG to be submitted asynchronously
> + * @pg: ALUA port group associated with @sdev.
> + * @sdev: SCSI device for which to submit an RTPG.
> + * @qdata: Information about the callback to invoke after the RTPG.
> + * @force: Whether or not to submit an RTPG if a work item that will submit
> an
> + * RTPG already has been scheduled.
> *
> * Returns true if and only if alua_rtpg_work() will be called
> asynchronously.
> * That function is responsible for calling @qdata->fn().
>
Reviewed-by: Hannes Reinecke
Cheers,
Hannes
--
Dr. Hannes ReineckezSeries & Storage
h...@suse.com +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
Re: [PATCH] scsi_debug: Use scsi-mq if it has been enabled
On 2018-01-23 07:39 PM, Bart Van Assche wrote: Since we want to remove the single queue code from the SCSI core at the appropriate time and since scsi-mq performs better than scsi-sq even when using only a single hardware queue, use scsi-mq if it has been enabled. Signed-off-by: Bart Van Assche Cc: Douglas Gilbert Cc: Hannes Reinecke Cc: Christoph Hellwig Acked: Douglas Gilbert --- drivers/scsi/scsi_debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index a5986dae9020..40df3eea72c8 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -5782,7 +5782,7 @@ static int sdebug_driver_probe(struct device * dev) } /* Decide whether to tell scsi subsystem that we want mq */ /* Following should give the same answer for each host */ - sdebug_mq_active = shost_use_blk_mq(hpnt) && (submit_queues > 1); + sdebug_mq_active = shost_use_blk_mq(hpnt); if (sdebug_mq_active) hpnt->nr_hw_queues = submit_queues;
Re: [PATCH 4/6] mpt3sas: Introduce Base function for cloning.
Hi All, We tried to reproduce below error "drivers/scsi/mpt3sas/mpt3sas_base.c:315:10: error: implicit declaration of function 'mpt3sas_scsih_scsi_lookup_get'; did you mean scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);" with base code and Make file flags (sparse) as mentioned under reproduce in auto build test log. We are not seeing this error. We have reviewed the code and it seems to be fine. Let us know if we miss something here. base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next reproduce: # apt-get install sparse make ARCH=x86_64 allmodconfig make C=1 CF=-D__CHECK_ENDIAN__ drivers/scsi/mpt3sas/mpt3sas_base.c: In function '_clone_sg_entries': drivers/scsi/mpt3sas/mpt3sas_base.c:315:10: error: implicit declaration of function 'mpt3sas_scsih_scsi_lookup_get'; did you mean scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid); ^ mpt3sas_scsih_issue_locked_tm drivers/scsi/mpt3sas/mpt3sas_base.c:315:8: warning: assignment makes pointer from integer without a cast scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid); ^ At top level: drivers/scsi/mpt3sas/mpt3sas_base.c:278:13: warning: '_clone_sg_entries' defined but not used static void _clone_sg_entries(struct MPT3SAS_ADAPTER ^ cc1: some warnings being treated as errors Thanks, Suganath Prabu S On Sat, Jan 20, 2018 at 11:37 PM, kbuild test robot wrote: > Hi Suganath, > > I love your patch! Perhaps something to improve: > > [auto build test WARNING on scsi/for-next] > [also build test WARNING on v4.15-rc8 next-20180119] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Suganath-Prabu-S/mpt3sas-Add-PCI-device-ID-for-Andromeda/20180121-002454 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next > reproduce: > # apt-get install sparse > make ARCH=x86_64 allmodconfig > make C=1 CF=-D__CHECK_ENDIAN__ > > > sparse warnings: (new ones prefixed by >>) > >>> drivers/scsi/mpt3sas/mpt3sas_base.c:142:24: sparse: cast from restricted >>> __le32 >drivers/scsi/mpt3sas/mpt3sas_base.c:142:24: sparse: incorrect type in > argument 1 (different base types) @@ expected unsigned int val @@ got > restrunsigned int val @@ >drivers/scsi/mpt3sas/mpt3sas_base.c:142:24: expected unsigned int val >drivers/scsi/mpt3sas/mpt3sas_base.c:142:24: got restricted __le32 >>> drivers/scsi/mpt3sas/mpt3sas_base.c:142:64: sparse: incorrect type in >>> argument 2 (different address spaces) @@ expected void volatile @@ got @@ >drivers/scsi/mpt3sas/mpt3sas_base.c:142:64: expected void volatile >drivers/scsi/mpt3sas/mpt3sas_base.c:142:64: got void COPYING CREDITS > Documentation Kbuild Kconfig MAINTAINERS Makefile README arch block certs > crypto drivers firmware fs include init ipc kernel lib mm net samples scripts > security sound tools usr virt >drivers/scsi/mpt3sas/mpt3sas_base.c:162:24: sparse: cast removes address > space of expression >drivers/scsi/mpt3sas/mpt3sas_base.c:315:24: sparse: undefined identifier > 'mpt3sas_scsih_scsi_lookup_get' >drivers/scsi/mpt3sas/mpt3sas_base.c:1164:42: sparse: incorrect type in > assignment (different base types) @@ expected unsigned short Event @@ got > short Event @@ >drivers/scsi/mpt3sas/mpt3sas_base.c:1165:49: sparse: incorrect type in > assignment (different base types) @@ expected unsigned int EventContext @@ > got ed int EventContext @@ >drivers/scsi/mpt3sas/mpt3sas_base.c:1383:64: sparse: incorrect type in > argument 2 (different address spaces) @@ expected void volatile @@ got oid > volatile @@ >drivers/scsi/mpt3sas/mpt3sas_base.c:1432:52: sparse: incorrect type in > argument 2 (different address spaces) @@ expected void volatile @@ got oid > volatile @@ >drivers/scsi/mpt3sas/mpt3sas_base.c:2964:32: sparse: cast removes address > space of expression >drivers/scsi/mpt3sas/mpt3sas_base.c:3256:16: sparse: incorrect type in > argument 1 (different base types) @@ expected unsigned long val @@ got > restunsigned long val @@ >drivers/scsi/mpt3sas/mpt3sas_base.c:3256:16: sparse: incorrect type in > argument 1 (different base types) @@ expected unsigned long val @@ got > restunsigned long val @@ >drivers/scsi/mpt3sas/mpt3sas_base.c:3256:16: sparse: incorrect type in > argument 1 (different base types) @@ expected unsigned long val @@ got > restunsigned long val @@ >drivers/scsi/mpt3sas/mpt3sas_base.c:3256:16: sparse: incorrect type in > argument 1 (different base types) @@ expected unsigned long val @@ got > restunsigned long val @@ >drivers/scsi/mpt3sas/mpt3sas_base.c:3256:16: sparse: incorrect type in > argument 1 (different base types) @@ expected unsigned long val @@ got > restunsigned long val @@ >drivers/scsi/mpt3sas/mpt3sas_base.c:3411:16: sparse: incorre
[PATCH 1/1] scsi: ufs: Enable quirk to ignore sending WRITE_SAME command
From: Sujit Reddy Thumma WRITE_SAME command is not supported by UFS. Enable a quirk for the upper level drivers to not send WRITE SAME command. Signed-off-by: Sujit Reddy Thumma Signed-off-by: Subhash Jadavani Signed-off-by: Asutosh Das --- drivers/scsi/ufs/ufshcd.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 8af2af3..5a8dc3b 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -4356,6 +4356,9 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev) /* WRITE_SAME command is not supported*/ sdev->no_write_same = 1; + /* WRITE_SAME command is not supported*/ + sdev->no_write_same = 1; + ufshcd_set_queue_depth(sdev); ufshcd_get_lu_power_on_wp_status(hba, sdev); -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH V3] blk-mq: introduce BLK_STS_DEV_RESOURCE
On Tue, Jan 23, 2018 at 04:57:34PM +, Bart Van Assche wrote: > On Wed, 2018-01-24 at 00:37 +0800, Ming Lei wrote: > > On Tue, Jan 23, 2018 at 04:24:20PM +, Bart Van Assche wrote: > > > My opinion about this patch is as follows: > > > * Changing a blk_mq_delay_run_hw_queue() call followed by return > > > BLK_STS_DEV_RESOURCE into return BLK_STS_RESOURCE is wrong because it > > > changes > > > a guaranteed queue rerun into a queue rerun that may or may not happen > > > depending on whether or not multiple queue runs happen simultaneously. > > > > You may not understand the two: > > > > 1) it is always safe to return BLK_STS_RESOURCE, which will make sure to > > avoid IO hang by blk_mq_delay_run_hw_queue() or blk_mq_run_hw_queue(), > > and using which one depends on SCHED_RESTART. > > > > 2) if driver can make sure the queue will be rerun after some resource > > is available, either by itself or by blk-mq, it will return > > BLK_STS_DEV_RESOURCE > > > > So what is wrong with this way? > > Sorry, I swapped BLK_STS_DEV_RESOURCE and BLK_STS_RESOURCE accidentally in my > reply. What I meant is that changing a blk_mq_delay_run_hw_queue() call > followed > by return BLK_STS_RESOURCE into BLK_STS_DEV_RESOURCE is wrong and introduces a > race condition in code where there was no race condition. OK, then no such race you worried about in this patch. Jens, could you take a look at this patch? Thanks, Ming
Re: [PATCH 1/5] blk-mq: introduce BLK_STS_DEV_RESOURCE
On Tue, Jan 23, 2018 at 10:01:37PM +, Bart Van Assche wrote: > On Wed, 2018-01-24 at 00:59 +0800, Ming Lei wrote: > > How is that enough to fix the IO hang when driver returns STS_RESOURCE > > and the queue is idle? If you want to follow previous dm-rq's way of > > call blk_mq_delay_run_hw_queue() in .queue_rq(), the same trick need > > to be applied to other drivers too, right? > > > > Unfortunately most of STS_RESOURCE don't use this trick, but they need > > to be handled. > > > > The patch of 'blk-mq: introduce BLK_STS_DEV_RESOURCE' can fix all these > > cases. > > The goal of my proposal was to address the race between running the queue and > adding requests back to the dispatch queue only. Regarding the I/O hangs that > can occur if a block driver returns BLK_STS_RESOURCE: since all of these can > be addressed by inserting blk_mq_delay_run_hw_queue() calls in the affected > block drivers I prefer to modify the block drivers instead of making the > blk-mq core even more complicated. IMO, this change doesn't make blk-mq code more complicated, also it is well documented, see the change in block layer: block/blk-core.c | 1 + block/blk-mq.c | 19 +++ include/linux/blk_types.h| 18 ++ Also 21 lines of them are comment, so only 17 lines code change needed in block layer. If inserting blk_mq_delay_run_hw_queue() to driver, the change can be a bit complicated, since call_rcu has to be used, you need to figure out one way to provide callback and the parameter. Even you have to document the change in each driver. [ming@ming linux]$ git grep -n BLK_STS_RESOURCE drivers/ | wc -l 42 There are at least 42 uses of BLK_STS_RESOURCE in drivers, in theory you should insert call_rcu(blk_mq_delay_run_hw_queue) in every BLK_STS_RESOURCE of drivers. I leave the decisions to Jens and drivers maintainers. Thanks, Ming
[PATCH] ibmvfc: fix misdefined reserved field in ibmvfc_fcp_rsp_info
The fcp_rsp_info structure as defined in the FC spec has an initial 3 bytes
reserved field. The ibmvfc driver mistakenly defined this field as 4 bytes
resulting in the rsp_code field being defined in what should be the start of
the second reserved field and thus always being reported as zero by the
driver.
Ideally, we should wire ibmvfc up with libfc for the sake of code
deduplication, and ease of maintaining standardized structures in a single
place. However, for now simply fixup the definition in ibmvfc for
backporting to distros on older kernels. Wiring up with libfc will be done
in a followup patch.
Cc: sta...@vger.kernel.org
Reported-by: Hannes Reinecke
Signed-off-by: Tyrel Datwyler
---
drivers/scsi/ibmvscsi/ibmvfc.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index 9a0696f..b81a53c 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -367,7 +367,7 @@ enum ibmvfc_fcp_rsp_info_codes {
};
struct ibmvfc_fcp_rsp_info {
- __be16 reserved;
+ u8 reserved[3];
u8 rsp_code;
u8 reserved2[4];
}__attribute__((packed, aligned (2)));
--
2.7.4
[PATCH] scsi_debug: Use scsi-mq if it has been enabled
Since we want to remove the single queue code from the SCSI core at the appropriate time and since scsi-mq performs better than scsi-sq even when using only a single hardware queue, use scsi-mq if it has been enabled. Signed-off-by: Bart Van Assche Cc: Douglas Gilbert Cc: Hannes Reinecke Cc: Christoph Hellwig --- drivers/scsi/scsi_debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index a5986dae9020..40df3eea72c8 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -5782,7 +5782,7 @@ static int sdebug_driver_probe(struct device * dev) } /* Decide whether to tell scsi subsystem that we want mq */ /* Following should give the same answer for each host */ - sdebug_mq_active = shost_use_blk_mq(hpnt) && (submit_queues > 1); + sdebug_mq_active = shost_use_blk_mq(hpnt); if (sdebug_mq_active) hpnt->nr_hw_queues = submit_queues; -- 2.15.1
[PATCH 1/6] qla2xxx: Fix a locking imbalance in qlt_24xx_handle_els()
Ensure that upon return the tgt->ha->tgt.sess_lock spin lock is
unlocked no matter which code path is taken through this function.
This was detected by sparse.
Fixes: 82abdcaf3ede ("scsi: qla2xxx: Allow target mode to accept PRLI in dual
mode")
Signed-off-by: Bart Van Assche
Cc: Himanshu Madhani
Cc: Quinn Tran
---
drivers/scsi/qla2xxx/qla_target.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_target.c
b/drivers/scsi/qla2xxx/qla_target.c
index fc89af8fe256..896b2d8bd803 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -4871,8 +4871,6 @@ static int qlt_24xx_handle_els(struct scsi_qla_host *vha,
sess);
qlt_send_term_imm_notif(vha, iocb, 1);
res = 0;
- spin_lock_irqsave(&tgt->ha->tgt.sess_lock,
- flags);
break;
}
--
2.15.1
[PATCH 5/6] qla4xxx: Move an array from a .h into a .c file
This patch does not change any functionality but slightly reduces
the size of the compiled kernel module.
Signed-off-by: Bart Van Assche
Cc: Himanshu Madhani
---
drivers/scsi/qla4xxx/ql4_nx.c | 2 ++
drivers/scsi/qla4xxx/ql4_nx.h | 2 --
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/qla4xxx/ql4_nx.c b/drivers/scsi/qla4xxx/ql4_nx.c
index 968bd85610f8..43f73583ef5c 100644
--- a/drivers/scsi/qla4xxx/ql4_nx.c
+++ b/drivers/scsi/qla4xxx/ql4_nx.c
@@ -45,6 +45,8 @@ qla4_8xxx_pci_base_offsetfset(struct scsi_qla_host *ha,
unsigned long off)
return NULL;
}
+static const int MD_MIU_TEST_AGT_RDDATA[] = { 0x41A8,
+ 0x41AC, 0x41B8, 0x41BC };
#define MAX_CRB_XFORM 60
static unsigned long crb_addr_xform[MAX_CRB_XFORM];
static int qla4_8xxx_crb_table_initialized;
diff --git a/drivers/scsi/qla4xxx/ql4_nx.h b/drivers/scsi/qla4xxx/ql4_nx.h
index 2c098cf9a1ac..98fe78613eb7 100644
--- a/drivers/scsi/qla4xxx/ql4_nx.h
+++ b/drivers/scsi/qla4xxx/ql4_nx.h
@@ -1022,6 +1022,4 @@ struct qla8xxx_minidump_entry_queue {
#define MD_MIU_TEST_AGT_WRDATA_ULO 0x41B0
#define MD_MIU_TEST_AGT_WRDATA_UHI 0x41B4
-static const int MD_MIU_TEST_AGT_RDDATA[] = { 0x41A8,
- 0x41AC, 0x41B8, 0x41BC };
#endif
--
2.15.1
[PATCH 2/6] qla2xxx: Use %p for printing pointers
Using %p instead of %lx to print a pointer allows to remove a cast. Signed-off-by: Bart Van Assche Cc: Himanshu Madhani --- drivers/scsi/qla2xxx/qla_init.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c index aececf664654..995579ea0f7f 100644 --- a/drivers/scsi/qla2xxx/qla_init.c +++ b/drivers/scsi/qla2xxx/qla_init.c @@ -2688,8 +2688,8 @@ qla2x00_chip_diag(scsi_qla_host_t *vha) /* Assume a failed state */ rval = QLA_FUNCTION_FAILED; - ql_dbg(ql_dbg_init, vha, 0x007b, - "Testing device at %lx.\n", (u_long)®->flash_address); + ql_dbg(ql_dbg_init, vha, 0x007b, "Testing device at %p.\n", + ®->flash_address); spin_lock_irqsave(&ha->hardware_lock, flags); -- 2.15.1
[PATCH 6/6] qla2xxx: Fix function argument descriptions
Bring the kernel-doc headers in sync with the function argument lists. Signed-off-by: Bart Van Assche Cc: Himanshu Madhani --- drivers/scsi/qla2xxx/qla_dbg.c| 4 +- drivers/scsi/qla2xxx/qla_gs.c | 78 +++ drivers/scsi/qla2xxx/qla_init.c | 33 + drivers/scsi/qla2xxx/qla_inline.h | 1 + drivers/scsi/qla2xxx/qla_iocb.c | 15 ++-- drivers/scsi/qla2xxx/qla_isr.c| 23 +++- drivers/scsi/qla2xxx/qla_mbx.c| 5 ++- drivers/scsi/qla2xxx/qla_mr.c | 36 ++ drivers/scsi/qla2xxx/qla_nx.c | 7 ++-- drivers/scsi/qla2xxx/qla_nx2.c| 19 +- drivers/scsi/qla2xxx/qla_sup.c| 1 + drivers/scsi/qla2xxx/qla_target.c | 7 ++-- 12 files changed, 125 insertions(+), 104 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c index 3e9dc54b89a3..7e9d8f08b9d5 100644 --- a/drivers/scsi/qla2xxx/qla_dbg.c +++ b/drivers/scsi/qla2xxx/qla_dbg.c @@ -717,7 +717,7 @@ qla2xxx_dump_post_process(scsi_qla_host_t *vha, int rval) /** * qla2300_fw_dump() - Dumps binary data from the 2300 firmware. - * @ha: HA context + * @vha: HA context * @hardware_locked: Called with the hardware_lock */ void @@ -887,7 +887,7 @@ qla2300_fw_dump(scsi_qla_host_t *vha, int hardware_locked) /** * qla2100_fw_dump() - Dumps binary data from the 2100/2200 firmware. - * @ha: HA context + * @vha: HA context * @hardware_locked: Called with the hardware_lock */ void diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c index 5bf9a59432f6..e4d404c24506 100644 --- a/drivers/scsi/qla2xxx/qla_gs.c +++ b/drivers/scsi/qla2xxx/qla_gs.c @@ -21,11 +21,10 @@ static int qla_async_rsnn_nn(scsi_qla_host_t *); /** * qla2x00_prep_ms_iocb() - Prepare common MS/CT IOCB fields for SNS CT query. - * @ha: HA context - * @req_size: request size in bytes - * @rsp_size: response size in bytes + * @vha: HA context + * @arg: CT arguments * - * Returns a pointer to the @ha's ms_iocb. + * Returns a pointer to the @vha's ms_iocb. */ void * qla2x00_prep_ms_iocb(scsi_qla_host_t *vha, struct ct_arg *arg) @@ -61,9 +60,8 @@ qla2x00_prep_ms_iocb(scsi_qla_host_t *vha, struct ct_arg *arg) /** * qla24xx_prep_ms_iocb() - Prepare common CT IOCB fields for SNS CT query. - * @ha: HA context - * @req_size: request size in bytes - * @rsp_size: response size in bytes + * @vha: HA context + * @arg: CT arguments * * Returns a pointer to the @ha's ms_iocb. */ @@ -101,7 +99,7 @@ qla24xx_prep_ms_iocb(scsi_qla_host_t *vha, struct ct_arg *arg) /** * qla2x00_prep_ct_req() - Prepare common CT request fields for SNS query. - * @ct_req: CT request buffer + * @p: CT request buffer * @cmd: GS command * @rsp_size: response size in bytes * @@ -196,7 +194,7 @@ qla2x00_chk_ms_status(scsi_qla_host_t *vha, ms_iocb_entry_t *ms_pkt, /** * qla2x00_ga_nxt() - SNS scan for fabric devices via GA_NXT command. - * @ha: HA context + * @vha: HA context * @fcport: fcport entry to updated * * Returns 0 on success. @@ -283,7 +281,7 @@ qla2x00_gid_pt_rsp_size(scsi_qla_host_t *vha) /** * qla2x00_gid_pt() - SNS scan for fabric devices via GID_PT command. - * @ha: HA context + * @vha: HA context * @list: switch info entries to populate * * NOTE: Non-Nx_Ports are not requested. @@ -371,7 +369,7 @@ qla2x00_gid_pt(scsi_qla_host_t *vha, sw_info_t *list) /** * qla2x00_gpn_id() - SNS Get Port Name (GPN_ID) query. - * @ha: HA context + * @vha: HA context * @list: switch info entries to populate * * Returns 0 on success. @@ -441,7 +439,7 @@ qla2x00_gpn_id(scsi_qla_host_t *vha, sw_info_t *list) /** * qla2x00_gnn_id() - SNS Get Node Name (GNN_ID) query. - * @ha: HA context + * @vha: HA context * @list: switch info entries to populate * * Returns 0 on success. @@ -583,7 +581,7 @@ static void qla2x00_async_sns_sp_done(void *s, int rc) /** * qla2x00_rft_id() - SNS Register FC-4 TYPEs (RFT_ID) supported by the HBA. - * @ha: HA context + * @vha: HA context * * Returns 0 on success. */ @@ -675,7 +673,8 @@ static int qla_async_rftid(scsi_qla_host_t *vha, port_id_t *d_id) /** * qla2x00_rff_id() - SNS Register FC-4 Features (RFF_ID) supported by the HBA. - * @ha: HA context + * @vha: HA context + * @type: not used * * Returns 0 on success. */ @@ -769,7 +768,7 @@ static int qla_async_rffid(scsi_qla_host_t *vha, port_id_t *d_id, /** * qla2x00_rnn_id() - SNS Register Node Name (RNN_ID) of the HBA. - * @ha: HA context + * @vha: HA context * * Returns 0 on success. */ @@ -874,7 +873,7 @@ qla2x00_get_sym_node_name(scsi_qla_host_t *vha, uint8_t *snn, size_t size) /** * qla2x00_rsnn_nn() - SNS Register Symbolic Node Name (RSNN_NN) of the HBA. - * @ha: HA context + * @vha: HA context * * Returns 0 on success. */ @@ -970,7 +969,7 @@ static int qla_async_rsnn_nn(scsi_qla_host_t *vha) /** * qla2x00_prep_sns_cmd() - Prepare common SNS command request fie
[PATCH 3/6] qla2xxx: Remove unused symbols
Remove a few preprocessor macros that are not used anywhere. Signed-off-by: Bart Van Assche Cc: Himanshu Madhani --- drivers/scsi/qla2xxx/qla_nx2.h | 4 1 file changed, 4 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_nx2.h b/drivers/scsi/qla2xxx/qla_nx2.h index 83c1b7e17c80..8ba7c1db07c3 100644 --- a/drivers/scsi/qla2xxx/qla_nx2.h +++ b/drivers/scsi/qla2xxx/qla_nx2.h @@ -23,10 +23,6 @@ #define MD_MIU_TEST_AGT_WRDATA_HI 0x41A4 #define MD_MIU_TEST_AGT_WRDATA_ULO 0x41B0 #define MD_MIU_TEST_AGT_WRDATA_UHI 0x41B4 -#define MD_MIU_TEST_AGT_RDDATA_LO 0x41A8 -#define MD_MIU_TEST_AGT_RDDATA_HI 0x41AC -#define MD_MIU_TEST_AGT_RDDATA_ULO 0x41B8 -#define MD_MIU_TEST_AGT_RDDATA_UHI 0x41BC /* MIU_TEST_AGT_CTRL flags. work for SIU as well */ #define MIU_TA_CTL_WRITE_ENABLE(MIU_TA_CTL_WRITE | MIU_TA_CTL_ENABLE) -- 2.15.1
[PATCH 4/6] qla4xxx: Remove unused symbols
Remove a few preprocessor macros that are not used anywhere.
Signed-off-by: Bart Van Assche
Cc: Himanshu Madhani
---
drivers/scsi/qla4xxx/ql4_nx.h | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/scsi/qla4xxx/ql4_nx.h b/drivers/scsi/qla4xxx/ql4_nx.h
index 337d9fcf6417..2c098cf9a1ac 100644
--- a/drivers/scsi/qla4xxx/ql4_nx.h
+++ b/drivers/scsi/qla4xxx/ql4_nx.h
@@ -1022,11 +1022,6 @@ struct qla8xxx_minidump_entry_queue {
#define MD_MIU_TEST_AGT_WRDATA_ULO 0x41B0
#define MD_MIU_TEST_AGT_WRDATA_UHI 0x41B4
-#define MD_MIU_TEST_AGT_RDDATA_LO 0x41A8
-#define MD_MIU_TEST_AGT_RDDATA_HI 0x41AC
-#define MD_MIU_TEST_AGT_RDDATA_ULO 0x41B8
-#define MD_MIU_TEST_AGT_RDDATA_UHI 0x41BC
-
static const int MD_MIU_TEST_AGT_RDDATA[] = { 0x41A8,
0x41AC, 0x41B8, 0x41BC };
#endif
--
2.15.1
[PATCH 0/6] Six qla2xxx and qla4xxx patches
Hello Martin, The patches in this series are what I came up with after having analyzed the source code of the qla[24]xxx drivers with several source code analysis tools (scripts/kernel-doc, gcc, sparse and smatch). None of the patches in this series have been tested. Yet I'm asking you to consider at least the first patch of this series for kernel v4.16 because it fixes a locking bug in one of the SCSI patches queued for kernel v4.16. Thanks, Bart. Bart Van Assche (6): qla2xxx: Fix a locking imbalance in qlt_24xx_handle_els() qla2xxx: Use %p for printing pointers qla2xxx: Remove unused symbols qla4xxx: Remove unused symbols qla4xxx: Move an array from a .h into a .c file qla2xxx: Fix function argument descriptions drivers/scsi/qla2xxx/qla_dbg.c| 4 +- drivers/scsi/qla2xxx/qla_gs.c | 78 +++ drivers/scsi/qla2xxx/qla_init.c | 37 ++- drivers/scsi/qla2xxx/qla_inline.h | 1 + drivers/scsi/qla2xxx/qla_iocb.c | 15 ++-- drivers/scsi/qla2xxx/qla_isr.c| 23 +++- drivers/scsi/qla2xxx/qla_mbx.c| 5 ++- drivers/scsi/qla2xxx/qla_mr.c | 36 ++ drivers/scsi/qla2xxx/qla_nx.c | 7 ++-- drivers/scsi/qla2xxx/qla_nx2.c| 19 +- drivers/scsi/qla2xxx/qla_nx2.h| 4 -- drivers/scsi/qla2xxx/qla_sup.c| 1 + drivers/scsi/qla2xxx/qla_target.c | 9 ++--- drivers/scsi/qla4xxx/ql4_nx.c | 2 + drivers/scsi/qla4xxx/ql4_nx.h | 7 15 files changed, 129 insertions(+), 119 deletions(-) -- 2.15.1
[PATCH] scsi_dh: Document alua_rtpg_queue() arguments
Since commit 3a025e1d1c2e ("Add optional check for bad kernel-doc
comments") building with W=1 causes warnings to appear for issues
in kernel-doc headers. This patch avoids that the following warnings
are reported when building with W=1:
drivers/scsi/device_handler/scsi_dh_alua.c:867: warning: No description found
for parameter 'pg'
drivers/scsi/device_handler/scsi_dh_alua.c:867: warning: No description found
for parameter 'sdev'
drivers/scsi/device_handler/scsi_dh_alua.c:867: warning: No description found
for parameter 'qdata'
drivers/scsi/device_handler/scsi_dh_alua.c:867: warning: No description found
for parameter 'force'
Signed-off-by: Bart Van Assche
Cc: Christoph Hellwig
Cc: Hannes Reinecke
Cc: Johannes Thumshirn
---
drivers/scsi/device_handler/scsi_dh_alua.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
b/drivers/scsi/device_handler/scsi_dh_alua.c
index 022e421c2185..4b44325d1a82 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -876,6 +876,11 @@ static void alua_rtpg_work(struct work_struct *work)
/**
* alua_rtpg_queue() - cause RTPG to be submitted asynchronously
+ * @pg: ALUA port group associated with @sdev.
+ * @sdev: SCSI device for which to submit an RTPG.
+ * @qdata: Information about the callback to invoke after the RTPG.
+ * @force: Whether or not to submit an RTPG if a work item that will submit an
+ * RTPG already has been scheduled.
*
* Returns true if and only if alua_rtpg_work() will be called asynchronously.
* That function is responsible for calling @qdata->fn().
--
2.15.1
[PATCH] Remove init_rcu_head() and destroy_rcu_head() calls
According to Documentation/RCU/Design/Requirements/Requirements.html
calling these functions is not necessary for dynamically allocated
objects:
The corresponding rcu_head structures that are
dynamically allocated are automatically tracked, but
rcu_head structures allocated on the stack
must be initialized with init_rcu_head_on_stack()
and cleaned up with destroy_rcu_head_on_stack().
Similarly, statically allocated non-stack rcu_head
structures must be initialized with init_rcu_head()
and cleaned up with destroy_rcu_head().
Hence remove the calls to these functions from the SCSI core.
Signed-off-by: Bart Van Assche
---
drivers/scsi/hosts.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index a0a7e4ff255c..7279d3d2e941 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -334,8 +334,6 @@ static void scsi_host_dev_release(struct device *dev)
if (shost->work_q)
destroy_workqueue(shost->work_q);
- destroy_rcu_head(&shost->rcu);
-
if (shost->shost_state == SHOST_CREATED) {
/*
* Free the shost_dev device name here if scsi_host_alloc()
@@ -404,7 +402,6 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template
*sht, int privsize)
INIT_LIST_HEAD(&shost->starved_list);
init_waitqueue_head(&shost->host_wait);
mutex_init(&shost->scan_mutex);
- init_rcu_head(&shost->rcu);
index = ida_simple_get(&host_index_ida, 0, 0, GFP_KERNEL);
if (index < 0)
--
2.15.1
Re: [PATCH 1/5] blk-mq: introduce BLK_STS_DEV_RESOURCE
On Wed, 2018-01-24 at 00:59 +0800, Ming Lei wrote: > How is that enough to fix the IO hang when driver returns STS_RESOURCE > and the queue is idle? If you want to follow previous dm-rq's way of > call blk_mq_delay_run_hw_queue() in .queue_rq(), the same trick need > to be applied to other drivers too, right? > > Unfortunately most of STS_RESOURCE don't use this trick, but they need > to be handled. > > The patch of 'blk-mq: introduce BLK_STS_DEV_RESOURCE' can fix all these > cases. The goal of my proposal was to address the race between running the queue and adding requests back to the dispatch queue only. Regarding the I/O hangs that can occur if a block driver returns BLK_STS_RESOURCE: since all of these can be addressed by inserting blk_mq_delay_run_hw_queue() calls in the affected block drivers I prefer to modify the block drivers instead of making the blk-mq core even more complicated. Thanks, Bart.
[PATCH] qla2xxx: Fix memory corruption during hba reset test
From: Quinn Tran This patch fixes memory corrpution while performing HBA Reset test. Following stack trace is seen [ 466.397219] BUG: unable to handle kernel NULL pointer dereference at 0020 [ 466.433669] IP: [] qlt_free_session_done+0x260/0x5f0 [qla2xxx] [ 466.467731] PGD 0 [ 466.476718] Oops: [#1] SMP Signed-off-by: Quinn Tran Signed-off-by: Himanshu Madhani --- Hi Martin, This patch fixes the crash for HBA reset test. From core dump analysis, memory corruption was discoverd during session cleanup. To prevent this memory corruption, driver needs to wait for session to be deleted and all memory associated with the session released before it can relogin and create new session. Please apply this patch to 4.16/scsi-queue at your earliest convenience. Thanks, Himanshu --- drivers/scsi/qla2xxx/qla_os.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c index 12ee6e02d146..afcb5567998a 100644 --- a/drivers/scsi/qla2xxx/qla_os.c +++ b/drivers/scsi/qla2xxx/qla_os.c @@ -3625,6 +3625,8 @@ qla2x00_remove_one(struct pci_dev *pdev) } qla2x00_wait_for_hba_ready(base_vha); + qla2x00_wait_for_sess_deletion(base_vha); + /* * if UNLOAD flag is already set, then continue unload, * where it was set first. -- 2.12.0
Re: [PATCH 1/5] blk-mq: introduce BLK_STS_DEV_RESOURCE
On Tue, Jan 23, 2018 at 04:54:02PM +, Bart Van Assche wrote: > On Wed, 2018-01-24 at 00:49 +0800, Ming Lei wrote: > > On Tue, Jan 23, 2018 at 04:47:11PM +, Bart Van Assche wrote: > > > On Wed, 2018-01-24 at 00:41 +0800, Ming Lei wrote: > > > > Could you explain where to call call_rcu()? call_rcu() can't be used in > > > > IO path at all. > > > > > > Can you explain what makes you think that call_rcu() can't be used in the > > > I/O path? As you know call_rcu() invokes a function asynchronously. From > > > Documentation/RCU/Design/Requirements/Requirements.html: > > > > > > The call_rcu() function may be used in a number of > > > situations where neither synchronize_rcu() nor > > > synchronize_rcu_expedited() would be legal, > > > including within preempt-disable code, local_bh_disable() code, > > > interrupt-disable code, and interrupt handlers. > > > > OK, suppose it is true, do you want to change most of STS_RESOURCE in > > all drivers to this way? Why can't we use the generic and simple approach > > in this patch? > > Please reread my proposal. I did not propose to change any block drivers. > What I proposed is to change the blk_mq_delay_run_hw_queue() implementation > such that it uses call_rcu() instead of kblockd_mod_delayed_work_on(). > That's a generic and simple approach. How is that enough to fix the IO hang when driver returns STS_RESOURCE and the queue is idle? If you want to follow previous dm-rq's way of call blk_mq_delay_run_hw_queue() in .queue_rq(), the same trick need to be applied to other drivers too, right? Unfortunately most of STS_RESOURCE don't use this trick, but they need to be handled. The patch of 'blk-mq: introduce BLK_STS_DEV_RESOURCE' can fix all these cases. -- Ming
Re: [PATCH V3] blk-mq: introduce BLK_STS_DEV_RESOURCE
On Wed, 2018-01-24 at 00:37 +0800, Ming Lei wrote: > On Tue, Jan 23, 2018 at 04:24:20PM +, Bart Van Assche wrote: > > My opinion about this patch is as follows: > > * Changing a blk_mq_delay_run_hw_queue() call followed by return > > BLK_STS_DEV_RESOURCE into return BLK_STS_RESOURCE is wrong because it > > changes > > a guaranteed queue rerun into a queue rerun that may or may not happen > > depending on whether or not multiple queue runs happen simultaneously. > > You may not understand the two: > > 1) it is always safe to return BLK_STS_RESOURCE, which will make sure to > avoid IO hang by blk_mq_delay_run_hw_queue() or blk_mq_run_hw_queue(), > and using which one depends on SCHED_RESTART. > > 2) if driver can make sure the queue will be rerun after some resource > is available, either by itself or by blk-mq, it will return > BLK_STS_DEV_RESOURCE > > So what is wrong with this way? Sorry, I swapped BLK_STS_DEV_RESOURCE and BLK_STS_RESOURCE accidentally in my reply. What I meant is that changing a blk_mq_delay_run_hw_queue() call followed by return BLK_STS_RESOURCE into BLK_STS_DEV_RESOURCE is wrong and introduces a race condition in code where there was no race condition. Bart.
Re: [PATCH 1/5] blk-mq: introduce BLK_STS_DEV_RESOURCE
On Wed, 2018-01-24 at 00:49 +0800, Ming Lei wrote: > On Tue, Jan 23, 2018 at 04:47:11PM +, Bart Van Assche wrote: > > On Wed, 2018-01-24 at 00:41 +0800, Ming Lei wrote: > > > Could you explain where to call call_rcu()? call_rcu() can't be used in > > > IO path at all. > > > > Can you explain what makes you think that call_rcu() can't be used in the > > I/O path? As you know call_rcu() invokes a function asynchronously. From > > Documentation/RCU/Design/Requirements/Requirements.html: > > > > The call_rcu() function may be used in a number of > > situations where neither synchronize_rcu() nor > > synchronize_rcu_expedited() would be legal, > > including within preempt-disable code, local_bh_disable() code, > > interrupt-disable code, and interrupt handlers. > > OK, suppose it is true, do you want to change most of STS_RESOURCE in > all drivers to this way? Why can't we use the generic and simple approach > in this patch? Please reread my proposal. I did not propose to change any block drivers. What I proposed is to change the blk_mq_delay_run_hw_queue() implementation such that it uses call_rcu() instead of kblockd_mod_delayed_work_on(). That's a generic and simple approach. Bart.
Re: [PATCH 1/5] blk-mq: introduce BLK_STS_DEV_RESOURCE
On Tue, Jan 23, 2018 at 04:47:11PM +, Bart Van Assche wrote: > On Wed, 2018-01-24 at 00:41 +0800, Ming Lei wrote: > > Could you explain where to call call_rcu()? call_rcu() can't be used in > > IO path at all. > > Can you explain what makes you think that call_rcu() can't be used in the > I/O path? As you know call_rcu() invokes a function asynchronously. From > Documentation/RCU/Design/Requirements/Requirements.html: > > The call_rcu() function may be used in a number of > situations where neither synchronize_rcu() nor > synchronize_rcu_expedited() would be legal, > including within preempt-disable code, local_bh_disable() code, > interrupt-disable code, and interrupt handlers. OK, suppose it is true, do you want to change most of STS_RESOURCE in all drivers to this way? Why can't we use the generic and simple approach in this patch? -- Ming
Re: [PATCH 1/5] blk-mq: introduce BLK_STS_DEV_RESOURCE
On Wed, 2018-01-24 at 00:41 +0800, Ming Lei wrote: > Could you explain where to call call_rcu()? call_rcu() can't be used in > IO path at all. Can you explain what makes you think that call_rcu() can't be used in the I/O path? As you know call_rcu() invokes a function asynchronously. From Documentation/RCU/Design/Requirements/Requirements.html: The call_rcu() function may be used in a number of situations where neither synchronize_rcu() nor synchronize_rcu_expedited() would be legal, including within preempt-disable code, local_bh_disable() code, interrupt-disable code, and interrupt handlers. Bart.
Re: [PATCH 1/5] blk-mq: introduce BLK_STS_DEV_RESOURCE
On Tue, Jan 23, 2018 at 08:37:26AM -0800, Bart Van Assche wrote: > On 01/23/18 08:26, Ming Lei wrote: > > On Tue, Jan 23, 2018 at 08:17:02AM -0800, Bart Van Assche wrote: > > > On 01/22/18 16:57, Ming Lei wrote: > > > > Even though RCU lock is held during dispatch, preemption or interrupt > > > > can happen too, so it is simply wrong to depend on the timing to make > > > > sure __blk_mq_run_hw_queue() can see the request in this situation. > > > > > > It is very unlikely that this race will ever be hit because that race > > > exists > > > for less than one microsecond and the smallest timeout that can be > > > specified > > > for delayed queue rerunning is one millisecond. Let's address this race if > > > anyone ever finds a way to hit it. > > > > Please don't depend on the timing which is often fragile, as we can make it > > correct in a generic approach. Also we should avoid to make every driver to > > follow this way for dealing with most of STS_RESOURCE, right? > > Wouldn't it be better to fix that race without changing the block layer API, > e.g. by using call_rcu() for delayed queue runs? As you know call_rcu() will Could you explain where to call call_rcu()? call_rcu() can't be used in IO path at all. > only call the specified function after a grace period. Since pushing back > requests onto the dispatch list happens with the RCU lock held using > call_rcu() for delayed queue runs would be sufficient to address this race. -- Ming
Re: [PATCH V3] blk-mq: introduce BLK_STS_DEV_RESOURCE
On Tue, Jan 23, 2018 at 04:24:20PM +, Bart Van Assche wrote: > On Wed, 2018-01-24 at 00:16 +0800, Ming Lei wrote: > > @@ -1280,10 +1282,18 @@ bool blk_mq_dispatch_rq_list(struct request_queue > > *q, struct list_head *list, > > * - Some but not all block drivers stop a queue before > > * returning BLK_STS_RESOURCE. Two exceptions are scsi-mq > > * and dm-rq. > > +* > > +* If drivers return BLK_STS_RESOURCE and S_SCHED_RESTART > > +* bit is set, run queue after 10ms for avoiding IO hang > > +* because the queue may be idle and the RESTART mechanism > > +* can't work any more. > > */ > > - if (!blk_mq_sched_needs_restart(hctx) || > > + needs_restart = blk_mq_sched_needs_restart(hctx); > > + if (!needs_restart || > > (no_tag && list_empty_careful(&hctx->dispatch_wait.entry))) > > blk_mq_run_hw_queue(hctx, true); > > + else if (needs_restart && (ret == BLK_STS_RESOURCE)) > > + blk_mq_delay_run_hw_queue(hctx, 10); > > } > > My opinion about this patch is as follows: > * Changing a blk_mq_delay_run_hw_queue() call followed by return > BLK_STS_DEV_RESOURCE into return BLK_STS_RESOURCE is wrong because it > changes > a guaranteed queue rerun into a queue rerun that may or may not happen > depending on whether or not multiple queue runs happen simultaneously. You may not understand the two: 1) it is always safe to return BLK_STS_RESOURCE, which will make sure to avoid IO hang by blk_mq_delay_run_hw_queue() or blk_mq_run_hw_queue(), and using which one depends on SCHED_RESTART. 2) if driver can make sure the queue will be rerun after some resource is available, either by itself or by blk-mq, it will return BLK_STS_DEV_RESOURCE So what is wrong with this way? > * This change makes block drivers less readable because anyone who encounters > BLK_STS_DEV_RESOURCE will have to look up its definition to figure out what > it's meaning is. It has been well-documented. BLK_STS_DEV_RESOURCE can be used very less, so it shouldn't be an issue. > * We don't need the new status code BLK_STS_DEV_RESOURCE because a delayed > queue run can be implemented easily with the existing block layer API. You mean to convert every STS_RESOURCE to call the API there, that way need lots of change, and with race in theory, since when the delay run queue is called in driver, the request isn't added to dispatch list. -- Ming
Re: [PATCH 1/5] blk-mq: introduce BLK_STS_DEV_RESOURCE
On 01/23/18 08:26, Ming Lei wrote: On Tue, Jan 23, 2018 at 08:17:02AM -0800, Bart Van Assche wrote: On 01/22/18 16:57, Ming Lei wrote: Even though RCU lock is held during dispatch, preemption or interrupt can happen too, so it is simply wrong to depend on the timing to make sure __blk_mq_run_hw_queue() can see the request in this situation. It is very unlikely that this race will ever be hit because that race exists for less than one microsecond and the smallest timeout that can be specified for delayed queue rerunning is one millisecond. Let's address this race if anyone ever finds a way to hit it. Please don't depend on the timing which is often fragile, as we can make it correct in a generic approach. Also we should avoid to make every driver to follow this way for dealing with most of STS_RESOURCE, right? Wouldn't it be better to fix that race without changing the block layer API, e.g. by using call_rcu() for delayed queue runs? As you know call_rcu() will only call the specified function after a grace period. Since pushing back requests onto the dispatch list happens with the RCU lock held using call_rcu() for delayed queue runs would be sufficient to address this race. Bart.
Re: [PATCH 1/5] blk-mq: introduce BLK_STS_DEV_RESOURCE
On Tue, Jan 23, 2018 at 08:17:02AM -0800, Bart Van Assche wrote: > > > On 01/22/18 16:57, Ming Lei wrote: > > Even though RCU lock is held during dispatch, preemption or interrupt > > can happen too, so it is simply wrong to depend on the timing to make > > sure __blk_mq_run_hw_queue() can see the request in this situation. > > It is very unlikely that this race will ever be hit because that race exists > for less than one microsecond and the smallest timeout that can be specified > for delayed queue rerunning is one millisecond. Let's address this race if > anyone ever finds a way to hit it. Please don't depend on the timing which is often fragile, as we can make it correct in a generic approach. Also we should avoid to make every driver to follow this way for dealing with most of STS_RESOURCE, right? > > > > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > > > > index d9ca1dfab154..55be2550c555 100644 > > > > --- a/drivers/scsi/scsi_lib.c > > > > +++ b/drivers/scsi/scsi_lib.c > > > > @@ -2030,9 +2030,9 @@ static blk_status_t scsi_queue_rq(struct > > > > blk_mq_hw_ctx *hctx, > > > > case BLK_STS_OK: > > > > break; > > > > case BLK_STS_RESOURCE: > > > > - if (atomic_read(&sdev->device_busy) == 0 && > > > > - !scsi_device_blocked(sdev)) > > > > - blk_mq_delay_run_hw_queue(hctx, > > > > SCSI_QUEUE_DELAY); > > > > + if (atomic_read(&sdev->device_busy) || > > > > + scsi_device_blocked(sdev)) > > > > + ret = BLK_STS_DEV_RESOURCE; > > > > break; > > > > default: > > > > /* > > > > > > The above introduces two changes that have not been mentioned in the > > > description of this patch: > > > - The queue rerunning delay is changed from 3 ms into 10 ms. Where is the > > >explanation of this change? Does this change have a positive or > > > negative > > >performance impact? > > > > How can that be a issue for SCSI? The rerunning delay is only triggered > > when there isn't any in-flight requests in SCSI queue. > > That change will result in more scsi_queue_rq() calls and hence in higher > CPU usage. By how much the CPU usage is increased will depend on the CPU > time required by the LLD .queuecommand() callback if that function gets > invoked. No, this patch won't increase CPU usage on SCSI, and the only change is to move the blk_mq_delay_run_hw_queue() out of SCSI's .queue_rq(), and the delay becomes 10. Thanks, Ming
Re: [PATCH V3] blk-mq: introduce BLK_STS_DEV_RESOURCE
On Wed, 2018-01-24 at 00:16 +0800, Ming Lei wrote: > @@ -1280,10 +1282,18 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, > struct list_head *list, >* - Some but not all block drivers stop a queue before >* returning BLK_STS_RESOURCE. Two exceptions are scsi-mq >* and dm-rq. > + * > + * If drivers return BLK_STS_RESOURCE and S_SCHED_RESTART > + * bit is set, run queue after 10ms for avoiding IO hang > + * because the queue may be idle and the RESTART mechanism > + * can't work any more. >*/ > - if (!blk_mq_sched_needs_restart(hctx) || > + needs_restart = blk_mq_sched_needs_restart(hctx); > + if (!needs_restart || > (no_tag && list_empty_careful(&hctx->dispatch_wait.entry))) > blk_mq_run_hw_queue(hctx, true); > + else if (needs_restart && (ret == BLK_STS_RESOURCE)) > + blk_mq_delay_run_hw_queue(hctx, 10); > } My opinion about this patch is as follows: * Changing a blk_mq_delay_run_hw_queue() call followed by return BLK_STS_DEV_RESOURCE into return BLK_STS_RESOURCE is wrong because it changes a guaranteed queue rerun into a queue rerun that may or may not happen depending on whether or not multiple queue runs happen simultaneously. * This change makes block drivers less readable because anyone who encounters BLK_STS_DEV_RESOURCE will have to look up its definition to figure out what it's meaning is. * We don't need the new status code BLK_STS_DEV_RESOURCE because a delayed queue run can be implemented easily with the existing block layer API. Bart.
Re: [PATCH V3] blk-mq: introduce BLK_STS_DEV_RESOURCE
On Tue, Jan 23 2018 at 11:16am -0500, Ming Lei wrote: > This status is returned from driver to block layer if device related > resource is run out of, but driver can guarantee that IO dispatch will > be triggered in future when the resource is available. > > This patch converts some drivers to use this return value. Meantime > if driver returns BLK_STS_RESOURCE and S_SCHED_RESTART is marked, run > queue after 10ms for avoiding IO hang. > > Suggested-by: Jens Axboe > Tested-by: Laurence Oberman > Cc: Christoph Hellwig > Cc: Mike Snitzer > Cc: Laurence Oberman > Cc: Bart Van Assche > Cc: Martin K. Petersen > Cc: James E.J. Bottomley > Signed-off-by: Ming Lei Acked-by: Mike Snitzer Thanks Ming
[PATCH V3] blk-mq: introduce BLK_STS_DEV_RESOURCE
This status is returned from driver to block layer if device related
resource is run out of, but driver can guarantee that IO dispatch will
be triggered in future when the resource is available.
This patch converts some drivers to use this return value. Meantime
if driver returns BLK_STS_RESOURCE and S_SCHED_RESTART is marked, run
queue after 10ms for avoiding IO hang.
Suggested-by: Jens Axboe
Tested-by: Laurence Oberman
Cc: Christoph Hellwig
Cc: Mike Snitzer
Cc: Laurence Oberman
Cc: Bart Van Assche
Cc: Martin K. Petersen
Cc: James E.J. Bottomley
Signed-off-by: Ming Lei
---
V3:
- fix typo, and improvement document
- add tested-by tag
V2:
- add comments on the new introduced status
- patch style fix
- both are suggested by Chritoph
block/blk-core.c | 1 +
block/blk-mq.c | 19 +++
drivers/block/null_blk.c | 2 +-
drivers/block/virtio_blk.c | 2 +-
drivers/block/xen-blkfront.c | 2 +-
drivers/md/dm-rq.c | 4 ++--
drivers/scsi/scsi_lib.c | 6 +++---
include/linux/blk_types.h| 18 ++
8 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index a2005a485335..ac4789c5e329 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -145,6 +145,7 @@ static const struct {
[BLK_STS_MEDIUM]= { -ENODATA, "critical medium" },
[BLK_STS_PROTECTION]= { -EILSEQ,"protection" },
[BLK_STS_RESOURCE] = { -ENOMEM,"kernel resource" },
+ [BLK_STS_DEV_RESOURCE] = { -ENOMEM,"device resource" },
[BLK_STS_AGAIN] = { -EAGAIN,"nonblocking retry" },
/* device mapper special case, should not leak out: */
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 01f271d40825..55c52b9a0f30 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1169,6 +1169,7 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q,
struct list_head *list,
struct request *rq, *nxt;
bool no_tag = false;
int errors, queued;
+ blk_status_t ret = BLK_STS_OK;
if (list_empty(list))
return false;
@@ -1181,7 +1182,6 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q,
struct list_head *list,
errors = queued = 0;
do {
struct blk_mq_queue_data bd;
- blk_status_t ret;
rq = list_first_entry(list, struct request, queuelist);
if (!blk_mq_get_driver_tag(rq, &hctx, false)) {
@@ -1226,7 +1226,7 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q,
struct list_head *list,
}
ret = q->mq_ops->queue_rq(hctx, &bd);
- if (ret == BLK_STS_RESOURCE) {
+ if (ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE) {
/*
* If an I/O scheduler has been configured and we got a
* driver tag for the next request already, free it
@@ -1257,6 +1257,8 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q,
struct list_head *list,
* that is where we will continue on next queue run.
*/
if (!list_empty(list)) {
+ bool needs_restart;
+
spin_lock(&hctx->lock);
list_splice_init(list, &hctx->dispatch);
spin_unlock(&hctx->lock);
@@ -1280,10 +1282,18 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q,
struct list_head *list,
* - Some but not all block drivers stop a queue before
* returning BLK_STS_RESOURCE. Two exceptions are scsi-mq
* and dm-rq.
+*
+* If drivers return BLK_STS_RESOURCE and S_SCHED_RESTART
+* bit is set, run queue after 10ms for avoiding IO hang
+* because the queue may be idle and the RESTART mechanism
+* can't work any more.
*/
- if (!blk_mq_sched_needs_restart(hctx) ||
+ needs_restart = blk_mq_sched_needs_restart(hctx);
+ if (!needs_restart ||
(no_tag && list_empty_careful(&hctx->dispatch_wait.entry)))
blk_mq_run_hw_queue(hctx, true);
+ else if (needs_restart && (ret == BLK_STS_RESOURCE))
+ blk_mq_delay_run_hw_queue(hctx, 10);
}
return (queued + errors) != 0;
@@ -1764,6 +1774,7 @@ static blk_status_t __blk_mq_issue_directly(struct
blk_mq_hw_ctx *hctx,
*cookie = new_cookie;
break;
case BLK_STS_RESOURCE:
+ case BLK_STS_DEV_RESOURCE:
__blk_mq_requeue_request(rq);
break;
default:
@@ -1826,7 +1837,7 @@ static void blk_mq_try_issue_directly(struct
blk_mq_hw_ctx *hctx,
hctx_lock(hctx, &srcu_idx);
ret = __blk_mq_try_issue_directly(hctx, rq, cookie, false);
- if (re
Re: [PATCH 1/5] blk-mq: introduce BLK_STS_DEV_RESOURCE
On 01/22/18 16:57, Ming Lei wrote: Even though RCU lock is held during dispatch, preemption or interrupt can happen too, so it is simply wrong to depend on the timing to make sure __blk_mq_run_hw_queue() can see the request in this situation. It is very unlikely that this race will ever be hit because that race exists for less than one microsecond and the smallest timeout that can be specified for delayed queue rerunning is one millisecond. Let's address this race if anyone ever finds a way to hit it. diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index d9ca1dfab154..55be2550c555 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -2030,9 +2030,9 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx, case BLK_STS_OK: break; case BLK_STS_RESOURCE: - if (atomic_read(&sdev->device_busy) == 0 && - !scsi_device_blocked(sdev)) - blk_mq_delay_run_hw_queue(hctx, SCSI_QUEUE_DELAY); + if (atomic_read(&sdev->device_busy) || + scsi_device_blocked(sdev)) + ret = BLK_STS_DEV_RESOURCE; break; default: /* The above introduces two changes that have not been mentioned in the description of this patch: - The queue rerunning delay is changed from 3 ms into 10 ms. Where is the explanation of this change? Does this change have a positive or negative performance impact? How can that be a issue for SCSI? The rerunning delay is only triggered when there isn't any in-flight requests in SCSI queue. That change will result in more scsi_queue_rq() calls and hence in higher CPU usage. By how much the CPU usage is increased will depend on the CPU time required by the LLD .queuecommand() callback if that function gets invoked. Bart.
Reply
Dear Sir, I am Andrew Wetkas.Personal Assistant to a former Governor in Nigeria. We need your assistance to invest a huge sum in your country. I will give you further details once I hear from you. Regards Andrew
[PATCH v3] scsi: sd: add new match array for cache_type
add user friendly command strings sd_wr_cache to enable/disable
write&read cache. user can enable both write and read cache by one of
the following commands:
echo w1r1 > cache_type
echo "write back" > cache_type
sd_wr_cache[] = {"w0r1", "w0r0", "w1r1", "w1r0"};
for sd_wr_cache 0 means disable, 1 means enable.
Encoding | WCE RCD | Write_cache Read_cache
write through / w0r1 | 0 0 | off on
none / w0r0 | 0 1 | off off
write back / w1r1 | 1 0 | on on
write back, no read (daft) / w1r0 | 1 1 | on off
Signed-off-by: weiping zhang
---
drivers/scsi/sd.c | 21 +++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index a028ab3..ce2fda5 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -134,10 +134,20 @@ static DEFINE_MUTEX(sd_ref_mutex);
static struct kmem_cache *sd_cdb_cache;
static mempool_t *sd_cdb_pool;
+/*
+ * Encoding | WCE RCD | Write_cache Read_cache
+ *
+ * write through / w0r1 | 0 0 | off on
+ * none / w0r0 | 0 1 | off off
+ * write back / w1r1 | 1 0 | on on
+ * write back, no read (daft) / w1r0 | 1 1 | on off
+ */
static const char *sd_cache_types[] = {
"write through", "none", "write back",
"write back, no read (daft)"
};
+/* 0:disable, 1:enable */
+static const char * const sd_wr_cache[] = {"w0r1", "w0r0", "w1r1", "w1r0"};
static void sd_set_flush_flag(struct scsi_disk *sdkp)
{
@@ -172,6 +182,10 @@ cache_type_store(struct device *dev, struct
device_attribute *attr,
* it's not worth the risk */
return -EINVAL;
+ /*
+* for "temporary", we only change request_queue's flag, not send
+* any command to disk, so actually disk'cache dosen't changed yet.
+*/
if (strncmp(buf, temp, sizeof(temp) - 1) == 0) {
buf += sizeof(temp) - 1;
sdkp->cache_override = 1;
@@ -180,8 +194,11 @@ cache_type_store(struct device *dev, struct
device_attribute *attr,
}
ct = sysfs_match_string(sd_cache_types, buf);
- if (ct < 0)
- return -EINVAL;
+ if (ct < 0) {
+ ct = sysfs_match_string(sd_wr_cache, buf);
+ if (ct < 0)
+ return -EINVAL;
+ }
rcd = ct & 0x01 ? 1 : 0;
wce = (ct & 0x02) && !sdkp->write_prot ? 1 : 0;
--
2.9.4
Re: [PATCH v2] scsi: sd: add new match array for cache_type
2018-01-23 8:23 GMT+08:00 Martin K. Petersen :
>
> Hi Weiping,
>
>> currently, there are four combinations as following: "write through",
>> "none", "write back", "write back, no read (daft)"
>>
>> cache_type can control both write and read cache, but for "write
>> through" and "write back" we can not know clearly how to control the
>> read cache.
>
> That's what I meant by using the term "arcane" and alluding to the fact
> that this interface is not well enough documented.
>
>> I prefer use words like"w0r1", "w0r0", "w1r1", "w1r0", that "1" means
>> enable, "0" means disable. The user know clearly what they are doing
>> when typing these short words.
>
> We can't change the existing interface without breaking stuff. We can
> entertain adding stuff, but I do think that a better solution is to
> document what's there so the effect of echoing each of the following
> strings becomes crystal clear:
>
OK, I'll add more detail comments for these words, but I prefer add new
stuff like "w0r1", for old user script keep using "write back", for new script
users can also use "w1r1".
> static const char *sd_cache_types[] = {
> "write through", "none", "write back",
> "write back, no read (daft)"
> };
>
> I would also like to see the "temporary" string documented.
OK, add it in V3.
>
> --
> Martin K. Petersen Oracle Linux Engineering
[PATCH 1/1] scsi: ufs: Allowing power mode change
From: Yaniv Gardi
Due to M-PHY issues, moving from HS to any other mode or gear or
even Hibern8 causes some un-predicted behavior of the device.
This patch fixes this issues.
Signed-off-by: Yaniv Gardi
Signed-off-by: Subhash Jadavani
Signed-off-by: Can Guo
Signed-off-by: Asutosh Das
---
drivers/scsi/ufs/ufshcd.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 011c336..d74d529 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4167,9 +4167,13 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
goto out;
} while (ret && retries--);
- if (ret)
+ if (ret) {
/* failed to get the link up... retire */
goto out;
+ } else {
+ ufshcd_dme_set(hba, UIC_ARG_MIB(TX_LCC_ENABLE), 0);
+ ufshcd_dme_set(hba, UIC_ARG_MIB(TX_LCC_ENABLE), 1);
+ }
if (link_startup_again) {
link_startup_again = false;
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project.
