* Re: [PATCH] isci: fix typo in deg_dbg message
From: Bart Van Assche @ 2016-11-14 22:23 UTC (permalink / raw)
To: Colin King, Intel SCU Linux support, Artur Paszkiewicz,
James E . J . Bottomley, Martin K . Petersen, linux-scsi
Cc: linux-kernel
In-Reply-To: <20161112183026.9626-1-colin.king@canonical.com>
On 11/12/2016 10:30 AM, Colin King wrote:
> Trivial fix to typo "repsonse" to "response" in dev_dbg message.
Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>
^ permalink raw reply
* Re: [PATCH] qla2xxx: do not abort all commands in the adapter during EEH recovery
From: Madhani, Himanshu @ 2016-11-14 22:07 UTC (permalink / raw)
To: Mauricio Faria de Oliveira, qla2xxx-upstream@qlogic.com
Cc: martin.petersen@oracle.com, jejb@linux.vnet.ibm.com,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1479158782-4544-1-git-send-email-mauricfo@linux.vnet.ibm.com>
On 11/14/16, 1:26 PM, "Mauricio Faria de Oliveira" <mauricfo@linux.vnet.ibm.com> wrote:
>The previous commit ("qla2xxx: fix invalid DMA access after command
>aborts in PCI device remove") introduced a regression during an EEH
>recovery, since the change to the qla2x00_abort_all_cmds() function
>calls qla2xxx_eh_abort(), which verifies the EEH recovery condition
>but handles it heavy-handed. (commit a465537ad1a4 "qla2xxx: Disable
>the adapter and skip error recovery in case of register disconnect.")
>
>This problem warrants a more general/optimistic solution right into
>qla2xxx_eh_abort() (eg in case a real command abort arrives during
>EEH recovery, or if it takes long enough to trigger command aborts);
>but it's still worth to add a check to ensure the code added by the
>previous commit is correct and contained within its owner function.
>
>This commit just adds a 'if (!ha->flags.eeh_busy)' check around it.
>(ahem; a trivial fix for this -rc series; sorry for this oversight.)
>
>With it applied, both PCI device remove and EEH recovery works fine.
>
>Fixes: 1535aa75a3d8 ("scsi: qla2xxx: fix invalid DMA access after
>command aborts in PCI device remove")
>Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
>---
> drivers/scsi/qla2xxx/qla_os.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
>index 567fa080e261..56d6142852a5 100644
>--- a/drivers/scsi/qla2xxx/qla_os.c
>+++ b/drivers/scsi/qla2xxx/qla_os.c
>@@ -1456,15 +1456,20 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data *ha)
> for (cnt = 1; cnt < req->num_outstanding_cmds; cnt++) {
> sp = req->outstanding_cmds[cnt];
> if (sp) {
>- /* Get a reference to the sp and drop the lock.
>- * The reference ensures this sp->done() call
>- * - and not the call in qla2xxx_eh_abort() -
>- * ends the SCSI command (with result 'res').
>+ /* Don't abort commands in adapter during EEH
>+ * recovery as it's not accessible/responding.
> */
>- sp_get(sp);
>- spin_unlock_irqrestore(&ha->hardware_lock, flags);
>- qla2xxx_eh_abort(GET_CMD_SP(sp));
>- spin_lock_irqsave(&ha->hardware_lock, flags);
>+ if (!ha->flags.eeh_busy) {
>+ /* Get a reference to the sp and drop the lock.
>+ * The reference ensures this sp->done() call
>+ * - and not the call in qla2xxx_eh_abort() -
>+ * ends the SCSI command (with result 'res').
>+ */
>+ sp_get(sp);
>+ spin_unlock_irqrestore(&ha->hardware_lock, flags);
>+ qla2xxx_eh_abort(GET_CMD_SP(sp));
>+ spin_lock_irqsave(&ha->hardware_lock, flags);
>+ }
> req->outstanding_cmds[cnt] = NULL;
> sp->done(vha, sp, res);
> }
>--
>1.8.3.1
>
Acked-by: Himanshu Madhani <himanshu.madhani@cavium.com>
Thanks,
Himanshu
^ permalink raw reply
* qed, qedi patchset submission
From: Arun Easi @ 2016-11-14 21:53 UTC (permalink / raw)
To: Martin K. Petersen, David Miller, linux-scsi, netdev
Hi Martin, David,
This is regarding the submission of the recent patch series we have posted
to linux-scsi and netdev:
[PATCH v2 0/6] Add QLogic FastLinQ iSCSI (qedi) driver.
[PATCH v2 1/6] qed: Add support for hardware offloaded iSCSI.
[PATCH v2 2/6] qed: Add iSCSI out of order packet handling.
[PATCH v2 3/6] qedi: Add QLogic FastLinQ offload iSCSI driver framework.
[PATCH v2 4/6] qedi: Add LL2 iSCSI interface for offload iSCSI.
[PATCH v2 5/6] qedi: Add support for iSCSI session management.
[PATCH v2 6/6] qedi: Add support for data path.
Patches 1 & 2 are "qed" module patches that goes under
drivers/net/ethernet/qlogic/qed/ and include/linux/qed/ directory.
- These are the iSCSI enablement changes to the common "qed"
module. There is no dependency for these patches and so
can go independently.
Patches 3, 4, 5 & 6 are the qedi patches that is aimed towards
drivers/scsi/qedi/ directory.
- These are the core qedi changes and is dependent on the
qed changes (invokes qed_XXX functions).
As qed sits in the net tree, the patches are usually submitted via netdev.
Do you have any preference or thoughts on how the "qed" patches be
approached? Just as a reference, our rdma driver "qedr" went through
something similar[1], and eventually "qed" patches were taken by David
in the net tree and "qedr", in the rdma tree (obviously) by Doug L.
Hi David,
For the "qed" enablement sent with the v2 series, we did not prefix the
qed patches with "[PATCH net-next]" prefix, so netdev folks may have
failed to notice/review that, sorry about that. We will send the next (v3)
series with that corrected.
Right now, we are basing the "qed" patches on top of latest net + net-next
tree. FYI, I tried a test merge of net-next/master + qed patches with
"net/master" and I see no conflict in qed.
Regards,
-Arun
[1] http://marc.info/?l=linux-rdma&m=147509152719831&w=2
^ permalink raw reply
* [PATCH] qla2xxx: do not abort all commands in the adapter during EEH recovery
From: Mauricio Faria de Oliveira @ 2016-11-14 21:26 UTC (permalink / raw)
To: Himanshu.Madhani, qla2xxx-upstream
Cc: martin.petersen, jejb, linux-scsi, linux-kernel
The previous commit ("qla2xxx: fix invalid DMA access after command
aborts in PCI device remove") introduced a regression during an EEH
recovery, since the change to the qla2x00_abort_all_cmds() function
calls qla2xxx_eh_abort(), which verifies the EEH recovery condition
but handles it heavy-handed. (commit a465537ad1a4 "qla2xxx: Disable
the adapter and skip error recovery in case of register disconnect.")
This problem warrants a more general/optimistic solution right into
qla2xxx_eh_abort() (eg in case a real command abort arrives during
EEH recovery, or if it takes long enough to trigger command aborts);
but it's still worth to add a check to ensure the code added by the
previous commit is correct and contained within its owner function.
This commit just adds a 'if (!ha->flags.eeh_busy)' check around it.
(ahem; a trivial fix for this -rc series; sorry for this oversight.)
With it applied, both PCI device remove and EEH recovery works fine.
Fixes: 1535aa75a3d8 ("scsi: qla2xxx: fix invalid DMA access after
command aborts in PCI device remove")
Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
---
drivers/scsi/qla2xxx/qla_os.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 567fa080e261..56d6142852a5 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -1456,15 +1456,20 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data *ha)
for (cnt = 1; cnt < req->num_outstanding_cmds; cnt++) {
sp = req->outstanding_cmds[cnt];
if (sp) {
- /* Get a reference to the sp and drop the lock.
- * The reference ensures this sp->done() call
- * - and not the call in qla2xxx_eh_abort() -
- * ends the SCSI command (with result 'res').
+ /* Don't abort commands in adapter during EEH
+ * recovery as it's not accessible/responding.
*/
- sp_get(sp);
- spin_unlock_irqrestore(&ha->hardware_lock, flags);
- qla2xxx_eh_abort(GET_CMD_SP(sp));
- spin_lock_irqsave(&ha->hardware_lock, flags);
+ if (!ha->flags.eeh_busy) {
+ /* Get a reference to the sp and drop the lock.
+ * The reference ensures this sp->done() call
+ * - and not the call in qla2xxx_eh_abort() -
+ * ends the SCSI command (with result 'res').
+ */
+ sp_get(sp);
+ spin_unlock_irqrestore(&ha->hardware_lock, flags);
+ qla2xxx_eh_abort(GET_CMD_SP(sp));
+ spin_lock_irqsave(&ha->hardware_lock, flags);
+ }
req->outstanding_cmds[cnt] = NULL;
sp->done(vha, sp, res);
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] sd_zbc: Force use of READ16/WRITE16
From: Jens Axboe @ 2016-11-14 20:17 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-block, linux-scsi, Christoph Hellwig, Martin K . Petersen,
Hannes Reinecke, Shaun Tancheff
In-Reply-To: <1478843606-15647-1-git-send-email-damien.lemoal@wdc.com>
On 11/10/2016 10:53 PM, Damien Le Moal wrote:
> Normally, sd_read_capacity sets sdp->use_16_for_rw to 1 based on the
> disk capacity so that READ16/WRITE16 are used for large drives.
> However, for a zoned disk with RC_BASIS set to 0, the capacity reported
> through READ_CAPACITY may be very small, leading to use_16_for_rw not being
> set and READ10/WRITE10 commands being used, even after the actual zoned disk
> capacity is corrected in sd_zbc_read_zones. This causes LBA offset overflow for
> accesses beyond 2TB.
>
> As the ZBC standard makes it mandatory for ZBC drives to support
> the READ16/WRITE16 commands anyway, make sure that use_16_for_rw is set.
Added to the 4.10 branch, thanks.
--
Jens Axboe
^ permalink raw reply
* [PATCH 2/2] blk-mq: Avoid memory reclaim when remapping queues
From: Gabriel Krisman Bertazi @ 2016-11-14 19:24 UTC (permalink / raw)
To: axboe
Cc: linux-block, Gabriel Krisman Bertazi, Brian King, Douglas Miller,
linux-scsi
In-Reply-To: <1479151478-19725-1-git-send-email-krisman@linux.vnet.ibm.com>
While stressing memory and IO at the same time we changed SMT settings,
we were able to consistently trigger deadlocks in the mm system, which
froze the entire machine.
I think that under memory stress conditions, the large allocations
performed by blk_mq_init_rq_map may trigger a reclaim, which stalls
waiting on the block layer remmaping completion, thus deadlocking the
system. The trace below was collected after the machine stalled,
waiting for the hotplug event completion.
The simplest fix for this is to make allocations in this path
non-reclaimable, with GFP_NOWAIT. With this patch, We couldn't hit the
issue anymore.
This should apply on top of Jen's for-next branch cleanly.
Call Trace:
[c000000f0160aaf0] [c000000f0160ab50] 0xc000000f0160ab50 (unreliable)
[c000000f0160acc0] [c000000000016624] __switch_to+0x2e4/0x430
[c000000f0160ad20] [c000000000b1a880] __schedule+0x310/0x9b0
[c000000f0160ae00] [c000000000b1af68] schedule+0x48/0xc0
[c000000f0160ae30] [c000000000b1b4b0] schedule_preempt_disabled+0x20/0x30
[c000000f0160ae50] [c000000000b1d4fc] __mutex_lock_slowpath+0xec/0x1f0
[c000000f0160aed0] [c000000000b1d678] mutex_lock+0x78/0xa0
[c000000f0160af00] [d000000019413cac] xfs_reclaim_inodes_ag+0x33c/0x380 [xfs]
[c000000f0160b0b0] [d000000019415164] xfs_reclaim_inodes_nr+0x54/0x70 [xfs]
[c000000f0160b0f0] [d0000000194297f8] xfs_fs_free_cached_objects+0x38/0x60 [xfs]
[c000000f0160b120] [c0000000003172c8] super_cache_scan+0x1f8/0x210
[c000000f0160b190] [c00000000026301c] shrink_slab.part.13+0x21c/0x4c0
[c000000f0160b2d0] [c000000000268088] shrink_zone+0x2d8/0x3c0
[c000000f0160b380] [c00000000026834c] do_try_to_free_pages+0x1dc/0x520
[c000000f0160b450] [c00000000026876c] try_to_free_pages+0xdc/0x250
[c000000f0160b4e0] [c000000000251978] __alloc_pages_nodemask+0x868/0x10d0
[c000000f0160b6f0] [c000000000567030] blk_mq_init_rq_map+0x160/0x380
[c000000f0160b7a0] [c00000000056758c] blk_mq_map_swqueue+0x33c/0x360
[c000000f0160b820] [c000000000567904] blk_mq_queue_reinit+0x64/0xb0
[c000000f0160b850] [c00000000056a16c] blk_mq_queue_reinit_notify+0x19c/0x250
[c000000f0160b8a0] [c0000000000f5d38] notifier_call_chain+0x98/0x100
[c000000f0160b8f0] [c0000000000c5fb0] __cpu_notify+0x70/0xe0
[c000000f0160b930] [c0000000000c63c4] notify_prepare+0x44/0xb0
[c000000f0160b9b0] [c0000000000c52f4] cpuhp_invoke_callback+0x84/0x250
[c000000f0160ba10] [c0000000000c570c] cpuhp_up_callbacks+0x5c/0x120
[c000000f0160ba60] [c0000000000c7cb8] _cpu_up+0xf8/0x1d0
[c000000f0160bac0] [c0000000000c7eb0] do_cpu_up+0x120/0x150
[c000000f0160bb40] [c0000000006fe024] cpu_subsys_online+0x64/0xe0
[c000000f0160bb90] [c0000000006f5124] device_online+0xb4/0x120
[c000000f0160bbd0] [c0000000006f5244] online_store+0xb4/0xc0
[c000000f0160bc20] [c0000000006f0a68] dev_attr_store+0x68/0xa0
[c000000f0160bc60] [c0000000003ccc30] sysfs_kf_write+0x80/0xb0
[c000000f0160bca0] [c0000000003cbabc] kernfs_fop_write+0x17c/0x250
[c000000f0160bcf0] [c00000000030fe6c] __vfs_write+0x6c/0x1e0
[c000000f0160bd90] [c000000000311490] vfs_write+0xd0/0x270
[c000000f0160bde0] [c0000000003131fc] SyS_write+0x6c/0x110
[c000000f0160be30] [c000000000009204] system_call+0x38/0xec
Signed-off-by: Gabriel Krisman Bertazi <krisman@linux.vnet.ibm.com>
Cc: Brian King <brking@linux.vnet.ibm.com>
Cc: Douglas Miller <dougmill@linux.vnet.ibm.com>
Cc: linux-block@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
block/blk-mq.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 7f7c4ba91adf..3e44303646cb 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1597,7 +1597,7 @@ static struct blk_mq_tags *blk_mq_init_rq_map(struct blk_mq_tag_set *set,
INIT_LIST_HEAD(&tags->page_list);
tags->rqs = kzalloc_node(set->queue_depth * sizeof(struct request *),
- GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY,
+ GFP_NOWAIT | __GFP_NOWARN | __GFP_NORETRY,
set->numa_node);
if (!tags->rqs) {
blk_mq_free_tags(tags);
@@ -1623,7 +1623,7 @@ static struct blk_mq_tags *blk_mq_init_rq_map(struct blk_mq_tag_set *set,
do {
page = alloc_pages_node(set->numa_node,
- GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY | __GFP_ZERO,
+ GFP_NOWAIT | __GFP_NOWARN | __GFP_NORETRY | __GFP_ZERO,
this_order);
if (page)
break;
@@ -1644,7 +1644,7 @@ static struct blk_mq_tags *blk_mq_init_rq_map(struct blk_mq_tag_set *set,
* Allow kmemleak to scan these pages as they contain pointers
* to additional allocations like via ops->init_request().
*/
- kmemleak_alloc(p, order_to_size(this_order), 1, GFP_KERNEL);
+ kmemleak_alloc(p, order_to_size(this_order), 1, GFP_NOWAIT);
entries_per_page = order_to_size(this_order) / rq_size;
to_do = min(entries_per_page, set->queue_depth - i);
left -= to_do * rq_size;
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] blk-mq: Fix failed allocation path when mapping queues
From: Gabriel Krisman Bertazi @ 2016-11-14 19:24 UTC (permalink / raw)
To: axboe
Cc: linux-block, Gabriel Krisman Bertazi, Brian King, Douglas Miller,
linux-scsi
In blk_mq_map_swqueue, there is a memory optimization that frees the
tags of a queue that has gone unmapped. Later, if that hctx is remapped
after another topology change, the tags need to be reallocated.
If this allocation fails, a simple WARN_ON triggers, but the block layer
ends up with an active hctx without any corresponding set of tags.
Then, any income IO to that hctx can trigger an Oops.
I can reproduce it consistently by running IO, flipping CPUs on and off
and eventually injecting a memory allocation failure in that path.
In the fix below, if the system experiences a failed allocation of any
hctx's tags, we remap all the ctxs of that queue to the hctx_0, which
should always keep it's tags. There is a minor performance hit, since
our mapping just got worse after the error path, but this is
the simplest solution to handle this error path. The performance hit
will disappear after another successful remap.
I considered dropping the memory optimization all together, but it
seemed a bad trade-off to handle this very specific error case.
My one concern about this patch is if remapping an arbitrary queue to
hctx_0 could result in outstanding requests getting submitted to the
wrong hctx. I couldn't observe this happening during tests, but I'm not
entirely sure it'll never happen. I believe the queue will be empty if
we are trying to allocate tags for it, unless it was using another alive
hctx queue and for some reason got reassigned to this new hctx. is this
possible at all?
This should apply cleanly on top of Jen's for-next branch.
The Oops is the one below:
SP (3fff935ce4d0) is in userspace
1:mon> e
cpu 0x1: Vector: 300 (Data Access) at [c000000fe99eb110]
pc: c0000000005e868c: __sbitmap_queue_get+0x2c/0x180
lr: c000000000575328: __bt_get+0x48/0xd0
sp: c000000fe99eb390
msr: 900000010280b033
dar: 28
dsisr: 40000000
current = 0xc000000fe9966800
paca = 0xc000000007e80300 softe: 0 irq_happened: 0x01
pid = 11035, comm = aio-stress
Linux version 4.8.0-rc6+ (root@bean) (gcc version 5.4.0 20160609
(Ubuntu/IBM 5.4.0-6ubuntu1~16.04.2) ) #3 SMP Mon Oct 10 20:16:53 CDT 2016
1:mon> s
[c000000fe99eb3d0] c000000000575328 __bt_get+0x48/0xd0
[c000000fe99eb400] c000000000575838 bt_get.isra.1+0x78/0x2d0
[c000000fe99eb480] c000000000575cb4 blk_mq_get_tag+0x44/0x100
[c000000fe99eb4b0] c00000000056f6f4 __blk_mq_alloc_request+0x44/0x220
[c000000fe99eb500] c000000000570050 blk_mq_map_request+0x100/0x1f0
[c000000fe99eb580] c000000000574650 blk_mq_make_request+0xf0/0x540
[c000000fe99eb640] c000000000561c44 generic_make_request+0x144/0x230
[c000000fe99eb690] c000000000561e00 submit_bio+0xd0/0x200
[c000000fe99eb740] c0000000003ef740 ext4_io_submit+0x90/0xb0
[c000000fe99eb770] c0000000003e95d8 ext4_writepages+0x588/0xdd0
[c000000fe99eb910] c00000000025a9f0 do_writepages+0x60/0xc0
[c000000fe99eb940] c000000000246c88 __filemap_fdatawrite_range+0xf8/0x180
[c000000fe99eb9e0] c000000000246f90 filemap_write_and_wait_range+0x70/0xf0
[c000000fe99eba20] c0000000003dd844 ext4_sync_file+0x214/0x540
[c000000fe99eba80] c000000000364718 vfs_fsync_range+0x78/0x130
[c000000fe99ebad0] c0000000003dd46c ext4_file_write_iter+0x35c/0x430
[c000000fe99ebb90] c00000000038c280 aio_run_iocb+0x3b0/0x450
[c000000fe99ebce0] c00000000038dc28 do_io_submit+0x368/0x730
[c000000fe99ebe30] c000000000009404 system_call+0x38/0xec
Signed-off-by: Gabriel Krisman Bertazi <krisman@linux.vnet.ibm.com>
Cc: Brian King <brking@linux.vnet.ibm.com>
Cc: Douglas Miller <dougmill@linux.vnet.ibm.com>
Cc: linux-block@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
block/blk-mq.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 3c5aaead71bb..7f7c4ba91adf 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1862,7 +1862,7 @@ static void blk_mq_init_cpu_queues(struct request_queue *q,
static void blk_mq_map_swqueue(struct request_queue *q,
const struct cpumask *online_mask)
{
- unsigned int i;
+ unsigned int i, hctx_idx;
struct blk_mq_hw_ctx *hctx;
struct blk_mq_ctx *ctx;
struct blk_mq_tag_set *set = q->tag_set;
@@ -1885,6 +1885,15 @@ static void blk_mq_map_swqueue(struct request_queue *q,
if (!cpumask_test_cpu(i, online_mask))
continue;
+ hctx_idx = q->mq_map[i];
+ /* unmapped hw queue can be remapped after CPU topo changed */
+ if (!set->tags[hctx_idx]) {
+ set->tags[hctx_idx] = blk_mq_init_rq_map(set, hctx_idx);
+
+ if (!set->tags[hctx_idx])
+ q->mq_map[i] = 0;
+ }
+
ctx = per_cpu_ptr(q->queue_ctx, i);
hctx = blk_mq_map_queue(q, i);
@@ -1901,7 +1910,10 @@ static void blk_mq_map_swqueue(struct request_queue *q,
* disable it and free the request entries.
*/
if (!hctx->nr_ctx) {
- if (set->tags[i]) {
+ /* Never unmap queue 0. We need it as a
+ * fallback in case of a new remap fails
+ * allocation. */
+ if (i && set->tags[i]) {
blk_mq_free_rq_map(set, set->tags[i], i);
set->tags[i] = NULL;
}
@@ -1909,11 +1921,8 @@ static void blk_mq_map_swqueue(struct request_queue *q,
continue;
}
- /* unmapped hw queue can be remapped after CPU topo changed */
- if (!set->tags[i])
- set->tags[i] = blk_mq_init_rq_map(set, i);
hctx->tags = set->tags[i];
- WARN_ON(!hctx->tags);
+ BUG_ON(!hctx->tags);
/*
* Set the map size to the number of mapped software queues.
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 0/2] sd: Fix a deadlock between event checking and device removal
From: Bart Van Assche @ 2016-11-14 18:35 UTC (permalink / raw)
To: James Bottomley, Martin K. Petersen
Cc: Mike Snitzer, Hannes Reinecke, linux-scsi@vger.kernel.org
In-Reply-To: <1479016028.17624.16.camel@linux.vnet.ibm.com>
On 11/12/2016 09:47 PM, James Bottomley wrote:
> On Fri, 2016-11-11 at 16:38 -0800, Bart Van Assche wrote:
>> Hello James and Martin,
>>
>> This short patch series fixes a deadlock that I ran into for the
>> first time several months ago but for which I had not yet had the
>> time to post a fix. As usual, feedback is appreciated.
>
> First question would be why do we need to push highly dm specific
> knowledge into block, like REQ_FAIL_IF_NO_PATH. Can't we just use
> REQ_FAILFAST_X for this?
>
> Also, I don't quite understand how you've configured multipath
> underneath SCSI. I thought dm-mp always went on top?
Hello James,
dm-multipath was running on top of SCSI in my setup which means that at
least the patch description is wrong. Since it was some time ago that I
ran into this deadlock, I will check first whether I can still reproduce it.
Bart.
^ permalink raw reply
* Re: [PATCH] sd_zbc: Force use of READ16/WRITE16
From: Christoph Hellwig @ 2016-11-14 16:10 UTC (permalink / raw)
To: Damien Le Moal
Cc: Jens Axboe, linux-block, linux-scsi, Christoph Hellwig,
Martin K . Petersen, Hannes Reinecke, Shaun Tancheff
In-Reply-To: <1478843606-15647-1-git-send-email-damien.lemoal@wdc.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH] scsi_transport_fc: Hold queue lock while calling blk_run_queue_async()
From: James Smart @ 2016-11-14 15:55 UTC (permalink / raw)
To: Bart Van Assche, James Bottomley, Martin K. Petersen
Cc: Hannes Reinecke, James Smart, linux-scsi@vger.kernel.org
In-Reply-To: <fb002f66-4648-9653-63b1-9c24a2c02666@sandisk.com>
Reviewed-By: James Smart <james.smart@broadcom.com>
(Bart: fyi - note my change in email address)
-- james
On 11/11/2016 4:55 PM, Bart Van Assche wrote:
> It is required to hold the queue lock when calling blk_run_queue_async()
> to avoid that a race between blk_run_queue_async() and
> blk_cleanup_queue() is triggered. Additionally, remove the get_device()
> and put_device() calls from fc_bsg_goose_queue. It is namely the
> responsibility of the caller of fc_bsg_goose_queue() to ensure that
> the bsg queue does not disappear while fc_bsg_goose_queue() is in
> progress.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: James Smart <james.smart@avagotech.com>
> ---
> drivers/scsi/scsi_transport_fc.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index 0f3a386..a32ab8e 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -3855,15 +3855,15 @@ fc_bsg_host_dispatch(struct request_queue *q, struct Scsi_Host *shost,
> static void
> fc_bsg_goose_queue(struct fc_rport *rport)
> {
> - if (!rport->rqst_q)
> + struct request_queue *q = rport->rqst_q;
> + unsigned long flags;
> +
> + if (!q)
> return;
>
> - /*
> - * This get/put dance makes no sense
> - */
> - get_device(&rport->dev);
> - blk_run_queue_async(rport->rqst_q);
> - put_device(&rport->dev);
> + spin_lock_irqsave(q->queue_lock, flags);
> + blk_run_queue_async(q);
> + spin_unlock_irqrestore(q->queue_lock, flags);
> }
>
> /**
^ permalink raw reply
* Re: [PATCH] scsi: hpsa: free irq on q indexed by h->intr_mode and not i
From: Christoph Hellwig @ 2016-11-14 15:42 UTC (permalink / raw)
To: Colin King
Cc: Christoph Hellwig, Don Brace, James E . J . Bottomley,
Martin K . Petersen, esc.storagedev, linux-scsi, linux-kernel
In-Reply-To: <20161114125935.12158-1-colin.king@canonical.com>
Looks fine, I was actually about to send the same fix..
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* [PATCH] scsi: hpsa: free irq on q indexed by h->intr_mode and not i
From: Colin King @ 2016-11-14 12:59 UTC (permalink / raw)
To: Christoph Hellwig, Don Brace, James E . J . Bottomley,
Martin K . Petersen, esc.storagedev, linux-scsi
Cc: linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Use correct index on q, use h->intr_mode instead of i. Issue
detected using static analysis with cppcheck
Fixes: bc2bb1543e62a5d0 ("scsi: hpsa: use pci_alloc_irq_vectors and automatic irq affinity")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/scsi/hpsa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 9459925..0d4f21c 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -8220,7 +8220,7 @@ static void hpsa_free_irqs(struct ctlr_info *h)
if (!h->msix_vectors || h->intr_mode != PERF_MODE_INT) {
/* Single reply queue, only one irq to free */
- free_irq(pci_irq_vector(h->pdev, 0), &h->q[i]);
+ free_irq(pci_irq_vector(h->pdev, 0), &h->q[h->intr_mode]);
h->q[h->intr_mode] = 0;
return;
}
--
2.10.2
^ permalink raw reply related
* Re: [PATCH 1/5] megaraid_sas: switch to pci_alloc_irq_vectors
From: Christoph Hellwig @ 2016-11-14 12:48 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Martin K. Petersen, Christoph Hellwig, James Bottomley,
Sumit Saxena, linux-scsi, Hannes Reinecke
In-Reply-To: <1478857492-4581-2-git-send-email-hare@suse.de>
> if (instance->msix_vectors)
> for (i = 0; i < instance->msix_vectors; i++) {
> + free_irq(pci_irq_vector(instance->pdev, i),
> &instance->irq_context[i]);
> }
> else
> + free_irq(pci_irq_vector(instance->pdev, 0),
> + &instance->irq_context[0]);
> }
Don't forget to replace the call to pci_disable_msix with one to pci_free_irq_vectors.
>
> /**
> @@ -5018,6 +5004,7 @@ static int megasas_init_fw(struct megasas_instance *instance)
> int i, loop, fw_msix_count = 0;
> struct IOV_111 *iovPtr;
> struct fusion_context *fusion;
> + int irq_flags = PCI_IRQ_MSIX;
>
> fusion = instance->ctrl_context;
>
> @@ -5134,15 +5121,18 @@ static int megasas_init_fw(struct megasas_instance *instance)
> /* Don't bother allocating more MSI-X vectors than cpus */
> instance->msix_vectors = min(instance->msix_vectors,
> (unsigned int)num_online_cpus());
> - for (i = 0; i < instance->msix_vectors; i++)
> - instance->msixentry[i].entry = i;
> - i = pci_enable_msix_range(instance->pdev, instance->msixentry,
> - 1, instance->msix_vectors);
> + if (smp_affinity_enable)
> + irq_flags |= PCI_IRQ_AFFINITY;
> + i = pci_alloc_irq_vectors(instance->pdev, 1,
> + instance->msix_vectors, irq_flags);
> if (i > 0)
> instance->msix_vectors = i;
> else
> instance->msix_vectors = 0;
> }
> + i = pci_alloc_irq_vectors(instance->pdev, 1, 1, PCI_IRQ_LEGACY);
> + if (i < 0)
> + goto fail_setup_irqs;
This looks wrong - you have to call to pci_alloc_irq_vectors right next
to each other here, so for the MSI-X case you'll still end up calling
the second one as well, which will then fail. I think the better way
to structure the driver would be to do the following here.
- Rename the msix_vectors field to irq_vectors, and make sure it's
initialized to 1 for non-MSI-X capable adapters.
- Have a single call to pci_alloc_irq_vectors here.
- Have all the request_irq/free_irq code iterate over ->irq_vectors
instead of duplicating it
- use pdev->msix_enabled for the few cases that need to check for
MSI-X mode specificly.
> /* Now re-enable MSI-X */
> - if (instance->msix_vectors &&
> - pci_enable_msix_exact(instance->pdev, instance->msixentry,
> - instance->msix_vectors))
> + if (instance->msix_vectors)
> + irq_flags = PCI_IRQ_MSIX;
> + if (smp_affinity_enable)
> + irq_flags |= PCI_IRQ_AFFINITY;
> + rval = pci_alloc_irq_vectors(instance->pdev, 1,
> + instance->msix_vectors ?
> + instance->msix_vectors : 1, irq_flags);
> + if (rval < 0)
The old code is doing a pci_enable_msix_exact so you also need to pass
the number of vectors as the first argument here.
^ permalink raw reply
* Re: [PATCH 1/5] megaraid_sas: switch to pci_alloc_irq_vectors
From: Christoph Hellwig @ 2016-11-14 12:33 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Sumit Saxena, Martin K. Petersen, Christoph Hellwig,
James Bottomley, linux-scsi, Hannes Reinecke
In-Reply-To: <30690328-e04f-7f41-d2d0-6d05c41b3290@suse.de>
On Fri, Nov 11, 2016 at 03:59:05PM +0100, Hannes Reinecke wrote:
> You are right; irq affinity only makes sense for MSI-X.
> I'll be fixing it up.
It works for multi-MSI and MSI-X. And even for single-MSI or
INTx it's harmless as it will be ignored if only a single
vector is present.
^ permalink raw reply
* RE: [PATCH 4/5] megaraid_sas: scsi-mq support
From: Kashyap Desai @ 2016-11-14 11:07 UTC (permalink / raw)
To: Hannes Reinecke, Martin K. Petersen
Cc: Christoph Hellwig, James Bottomley, Sumit Saxena, linux-scsi,
Hannes Reinecke
In-Reply-To: <1478857492-4581-5-git-send-email-hare@suse.de>
> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Hannes Reinecke
> Sent: Friday, November 11, 2016 3:15 PM
> To: Martin K. Petersen
> Cc: Christoph Hellwig; James Bottomley; Sumit Saxena; linux-
> scsi@vger.kernel.org; Hannes Reinecke; Hannes Reinecke
> Subject: [PATCH 4/5] megaraid_sas: scsi-mq support
>
> This patch enables full scsi multiqueue support. But as I have been
seeing
> performance regressions I've also added a module parameter 'use_blk_mq',
> allowing multiqueue to be switched off if required.
Hannes - As discussed for hpsa driver related thread for similar support,
I don't think this code changes are helping MR as well.
>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
> drivers/scsi/megaraid/megaraid_sas_base.c | 22 +++++++++++++++++
> drivers/scsi/megaraid/megaraid_sas_fusion.c | 38
+++++++++++++++++++++----
> ----
> 2 files changed, 50 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> b/drivers/scsi/megaraid/megaraid_sas_base.c
> index d580406..1af47e6 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -48,6 +48,7 @@
> #include <linux/blkdev.h>
> #include <linux/mutex.h>
> #include <linux/poll.h>
> +#include <linux/blk-mq-pci.h>
>
> #include <scsi/scsi.h>
> #include <scsi/scsi_cmnd.h>
> @@ -104,6 +105,9 @@
> module_param(scmd_timeout, int, S_IRUGO);
> MODULE_PARM_DESC(scmd_timeout, "scsi command timeout (10-90s), default
> 90s. See megasas_reset_timer.");
>
> +bool use_blk_mq = true;
> +module_param_named(use_blk_mq, use_blk_mq, bool, 0);
> +
> MODULE_LICENSE("GPL");
> MODULE_VERSION(MEGASAS_VERSION);
> MODULE_AUTHOR("megaraidlinux.pdl@avagotech.com");
> @@ -1882,6 +1886,17 @@ static void megasas_slave_destroy(struct
> scsi_device *sdev)
> sdev->hostdata = NULL;
> }
>
> +static int megasas_map_queues(struct Scsi_Host *shost) {
> + struct megasas_instance *instance = (struct megasas_instance *)
> + shost->hostdata;
> +
> + if (!instance->ctrl_context)
> + return 0;
> +
> + return blk_mq_pci_map_queues(&shost->tag_set, instance->pdev, 0);
}
> +
> /*
> * megasas_complete_outstanding_ioctls - Complete outstanding ioctls
after a
> * kill adapter
> @@ -2986,6 +3001,7 @@ struct device_attribute *megaraid_host_attrs[] = {
> .slave_configure = megasas_slave_configure,
> .slave_alloc = megasas_slave_alloc,
> .slave_destroy = megasas_slave_destroy,
> + .map_queues = megasas_map_queues,
> .queuecommand = megasas_queue_command,
> .eh_target_reset_handler = megasas_reset_target,
> .eh_abort_handler = megasas_task_abort, @@ -5610,6 +5626,12 @@
> static int megasas_io_attach(struct megasas_instance *instance)
> host->max_id = MEGASAS_MAX_DEV_PER_CHANNEL;
> host->max_lun = MEGASAS_MAX_LUN;
> host->max_cmd_len = 16;
> + host->nr_hw_queues = instance->msix_vectors ?
> + instance->msix_vectors : 1;
> + if (use_blk_mq) {
> + host->can_queue = instance->max_scsi_cmds / host-
> >nr_hw_queues;
> + host->use_blk_mq = 1;
> + }
>
> /*
> * Notify the mid-layer about the new controller diff --git
> a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index eb3cb0f..aba53c0 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -1703,6 +1703,7 @@ static int megasas_create_sg_sense_fusion(struct
> megasas_instance *instance)
> struct IO_REQUEST_INFO io_info;
> struct fusion_context *fusion;
> struct MR_DRV_RAID_MAP_ALL *local_map_ptr;
> + u16 msix_index;
> u8 *raidLUN;
>
> device_id = MEGASAS_DEV_INDEX(scp);
> @@ -1792,11 +1793,13 @@ static int megasas_create_sg_sense_fusion(struct
> megasas_instance *instance)
> fp_possible = io_info.fpOkForIo;
> }
>
> - /* Use raw_smp_processor_id() for now until cmd->request->cpu is
CPU
> - id by default, not CPU group id, otherwise all MSI-X queues
won't
> - be utilized */
> - cmd->request_desc->SCSIIO.MSIxIndex = instance->msix_vectors ?
> - raw_smp_processor_id() % instance->msix_vectors : 0;
> + if (shost_use_blk_mq(instance->host)) {
> + u32 blk_tag = blk_mq_unique_tag(scp->request);
> + msix_index = blk_mq_unique_tag_to_hwq(blk_tag);
> + } else
> + msix_index = instance->msix_vectors ?
> + raw_smp_processor_id() % instance->msix_vectors :
0;
> + cmd->request_desc->SCSIIO.MSIxIndex = msix_index;
>
> if (fp_possible) {
> megasas_set_pd_lba(io_request, scp->cmd_len, &io_info,
scp,
> @@ -1971,6 +1974,7 @@ static void megasas_build_ld_nonrw_fusion(struct
> megasas_instance *instance,
> struct RAID_CONTEXT *pRAID_Context;
> struct MR_PD_CFG_SEQ_NUM_SYNC *pd_sync;
> struct fusion_context *fusion = instance->ctrl_context;
> + u16 msix_index;
> pd_sync = (void *)fusion->pd_seq_sync[(instance->pd_seq_map_id -
1)
> & 1];
>
> device_id = MEGASAS_DEV_INDEX(scmd);
> @@ -2013,11 +2017,14 @@ static void megasas_build_ld_nonrw_fusion(struct
> megasas_instance *instance,
> io_request->DevHandle = cpu_to_le16(0xFFFF);
> }
>
> + if (shost_use_blk_mq(instance->host)) {
> + u32 blk_tag = blk_mq_unique_tag(scmd->request);
> + msix_index = blk_mq_unique_tag_to_hwq(blk_tag);
> + } else
> + msix_index = instance->msix_vectors ?
> + (raw_smp_processor_id() % instance->msix_vectors)
:
> 0;
> + cmd->request_desc->SCSIIO.MSIxIndex = msix_index;
> cmd->request_desc->SCSIIO.DevHandle = io_request->DevHandle;
> - cmd->request_desc->SCSIIO.MSIxIndex =
> - instance->msix_vectors ?
> - (raw_smp_processor_id() % instance->msix_vectors) : 0;
> -
>
> if (!fp_possible) {
> /* system pd firmware path */
> @@ -2188,7 +2195,18 @@ static void megasas_build_ld_nonrw_fusion(struct
> megasas_instance *instance,
> return SCSI_MLQUEUE_DEVICE_BUSY;
> }
>
> - cmd = megasas_get_cmd_fusion(instance, scmd->request->tag);
> + if (shost_use_blk_mq(instance->host)) {
> + int msix_vectors, hwq_size;
> + u32 blk_tag = blk_mq_unique_tag(scmd->request);
> + u16 hwq = blk_mq_unique_tag_to_hwq(blk_tag);
> + u16 tag = blk_mq_unique_tag_to_tag(blk_tag);
> +
> + msix_vectors = instance->msix_vectors ?
> + instance->msix_vectors : 1;
> + hwq_size = instance->max_scsi_cmds / msix_vectors;
> + cmd = megasas_get_cmd_fusion(instance, (hwq * hwq_size) +
> tag);
> + } else
> + cmd = megasas_get_cmd_fusion(instance, scmd->request-
> >tag);
>
> index = cmd->index;
>
> --
> 1.8.5.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of
> a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [PATCH] hpsa: scsi-mq support
From: Kashyap Desai @ 2016-11-14 11:05 UTC (permalink / raw)
To: Christoph Hellwig, Hannes Reinecke
Cc: Christoph Hellwig, Hannes Reinecke, Martin K. Petersen,
James Bottomley, Don Brace, linux-scsi, Jens Axboe
In-Reply-To: <20161113115841.GA4818@lst.de>
> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Christoph Hellwig
> Sent: Sunday, November 13, 2016 5:29 PM
> To: Hannes Reinecke
> Cc: Christoph Hellwig; Hannes Reinecke; Christoph Hellwig; Martin K.
Petersen;
> James Bottomley; Don Brace; linux-scsi@vger.kernel.org; Jens Axboe
> Subject: Re: [PATCH] hpsa: scsi-mq support
>
> On Sun, Nov 13, 2016 at 10:44:47AM +0100, Hannes Reinecke wrote:
> > One day to mark with bright red in the calendar.
> >
> > Christoph Hellwig is telling me _NOT_ to use scsi-mq.
>
> That's not what I'm doing.
>
> > This patch was done so see what would needed to be done to convert a
> > legacy driver.
> > As I was under the impression that scsi-mq is the way forward, seeing
> > that it should be enabled per default.
> > But I must have been mistaken. Apparently.
>
> What I am doing is to tell you you should not expose multiple queues
unless the
> hardware actually has multiple submissions queues. The blk-mq and
scsi-mq
> code works just fine with a single submission queue, and the hpsa driver
in
> particular works really well with scsi-mq and a single submission queue.
E.g. the
> RAID HBA on slide 19 of this presentations is an hpsa one:
I have similar results for MegaRaid where seen MR driver gives significant
improvement for Single Submission Queue and multiple Completion Queue.
Having said that scsi-mq is enabled but with single Queue is more than
enough to maximize improvement of SCSI-MQ.
Major advantage was seen while IO load is cross the boundary of Physical
CPU socket.
>From this discussion I understood that - Similar logical changes proposed
for megaraid_sas and we are not really going to gain with fake multiple
submission exposed to SML.
Kashyap
>
> http://events.linuxfoundation.org/sites/events/files/slides/scsi.pdf
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of
> a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* PLEASE VIEW THE ATTACHED FILE AND CONTACT ME.
From: Dr. Felix Collins @ 2016-11-14 7:43 UTC (permalink / raw)
In-Reply-To: <1666584935.3489547.1479109399212.ref@mail.yahoo.com>
[-- Attachment #1: Type: text/plain, Size: 1 bytes --]
[-- Attachment #2: FROM FIRST NATIONAL BANK OF SOUTH AFRICA (F.N.B)..rtf --]
[-- Type: application/msword, Size: 3007 bytes --]
^ permalink raw reply
* [PATCH] 53c700: fix memory leak of dma non-cosistent memory
From: Li Qiang @ 2016-11-14 7:07 UTC (permalink / raw)
To: James.Bottomley, linux-scsi; +Cc: Li Qiang
From: Li Qiang <liq3ea@gmail.com>
In NCR_700_detect function, if an error occurs it will return
NULL without freeing the dma non-cosistent memory once allocated.
This patch avoid this.
Signed-off-by: Li Qiang <liq3ea@gmail.com>
---
drivers/scsi/53c700.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/53c700.c b/drivers/scsi/53c700.c
index 95e32a4..d5a2ba3 100644
--- a/drivers/scsi/53c700.c
+++ b/drivers/scsi/53c700.c
@@ -332,8 +332,10 @@ struct Scsi_Host *
tpnt->proc_name = "53c700";
host = scsi_host_alloc(tpnt, 4);
- if (!host)
+ if (!host) {
+ dma_free_noncoherent(hostdata->dev, TOTAL_MEM_SIZE, memory, pScript);
return NULL;
+ }
memset(hostdata->slots, 0, sizeof(struct NCR_700_command_slot)
* NCR_700_COMMAND_SLOTS_PER_HOST);
for (j = 0; j < NCR_700_COMMAND_SLOTS_PER_HOST; j++) {
@@ -394,6 +396,7 @@ struct Scsi_Host *
if (scsi_add_host(host, dev)) {
dev_printk(KERN_ERR, dev, "53c700: scsi_add_host failed\n");
+ dma_free_noncoherent(hostdata->dev, TOTAL_MEM_SIZE, memory, pScript);
scsi_host_put(host);
return NULL;
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] hpsa: scsi-mq support
From: Christoph Hellwig @ 2016-11-13 11:58 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Christoph Hellwig, Hannes Reinecke, Christoph Hellwig,
Martin K. Petersen, James Bottomley, Don Brace, linux-scsi,
Jens Axboe
In-Reply-To: <cba051f8-3055-53c4-2860-ee4c51a7043d@suse.com>
On Sun, Nov 13, 2016 at 10:44:47AM +0100, Hannes Reinecke wrote:
> One day to mark with bright red in the calendar.
>
> Christoph Hellwig is telling me _NOT_ to use scsi-mq.
That's not what I'm doing.
> This patch was done so see what would needed to be done to convert a legacy
> driver.
> As I was under the impression that scsi-mq is the way forward, seeing that
> it should be enabled per default.
> But I must have been mistaken. Apparently.
What I am doing is to tell you you should not expose multiple queues
unless the hardware actually has multiple submissions queues. The blk-mq
and scsi-mq code works just fine with a single submission queue, and
the hpsa driver in particular works really well with scsi-mq and a single
submission queue. E.g. the RAID HBA on slide 19 of this presentations is
an hpsa one:
http://events.linuxfoundation.org/sites/events/files/slides/scsi.pdf
^ permalink raw reply
* Re: [PATCH] hpsa: scsi-mq support
From: Hannes Reinecke @ 2016-11-13 9:44 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Hannes Reinecke, Christoph Hellwig, Martin K. Petersen,
James Bottomley, Don Brace, linux-scsi, Jens Axboe
In-Reply-To: <20161112173223.GA22158@infradead.org>
On 11/12/2016 06:32 PM, Christoph Hellwig wrote:
> On Fri, Nov 11, 2016 at 07:22:05PM +0100, Hannes Reinecke wrote:
>> Well, it's the same as with megasas and mpt3sas. Each of those have
>> a single MMIO register where the driver writes the address of the
>> command into. What exactly the hardware does in the back doesn't
>> really matter here; the command is in memory and the hardware can
>> access it as it sees fit. So from that point of view we can assume
>> having a submission queue to match the completion queue;
>> With that setup we do have a contention point on the single command
>> register, but that's about it.
>> We still should benefit from scsi-mq, though.
>
> How do we benefit from scsi-mq in this case? We still hit global
> cachelines like commands_outstanding in the driver, and we lost the
> batching done by the ctx -> hw_ctx layering for the single queue
> blk-mq case. We also get much less efficient merging and will not
> have the chance of having and I/O schedule in the near future.
>
One day to mark with bright red in the calendar.
Christoph Hellwig is telling me _NOT_ to use scsi-mq.
> But back to my question from the last mail: What workload is improved
> by using this patch?
>
This patch was done so see what would needed to be done to convert a
legacy driver.
As I was under the impression that scsi-mq is the way forward, seeing
that it should be enabled per default.
But I must have been mistaken. Apparently.
Slightly confused,
Hannes
^ permalink raw reply
* Re: [PATCH 0/2] sd: Fix a deadlock between event checking and device removal
From: James Bottomley @ 2016-11-13 5:47 UTC (permalink / raw)
To: Bart Van Assche, Martin K. Petersen
Cc: Mike Snitzer, Hannes Reinecke, linux-scsi@vger.kernel.org
In-Reply-To: <a8f0ee8f-7b40-2375-47a4-52f78af29d8d@sandisk.com>
On Fri, 2016-11-11 at 16:38 -0800, Bart Van Assche wrote:
> Hello James and Martin,
>
> This short patch series fixes a deadlock that I ran into for the
> first time several months ago but for which I had not yet had the
> time to post a fix. As usual, feedback is appreciated.
First question would be why do we need to push highly dm specific
knowledge into block, like REQ_FAIL_IF_NO_PATH. Can't we just use
REQ_FAILFAST_X for this?
Also, I don't quite understand how you've configured multipath
underneath SCSI. I thought dm-mp always went on top?
James
^ permalink raw reply
* Re: [PATCH 04/12] target: avoid to access .bi_vcnt directly
From: Sagi Grimberg @ 2016-11-12 21:37 UTC (permalink / raw)
To: Ming Lei, Jens Axboe, linux-kernel
Cc: linux-block, linux-fsdevel, Christoph Hellwig,
Nicholas A. Bellinger, open list:TARGET SUBSYSTEM,
open list:TARGET SUBSYSTEM
In-Reply-To: <1478865957-25252-5-git-send-email-tom.leiming@gmail.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply
* Re: [PATCH] hpsa: scsi-mq support
From: Jens Axboe @ 2016-11-12 18:30 UTC (permalink / raw)
To: Hannes Reinecke, Christoph Hellwig, Hannes Reinecke
Cc: Christoph Hellwig, Martin K. Petersen, James Bottomley, Don Brace,
linux-scsi
In-Reply-To: <39d1198d-eb49-71ed-9139-f0efd9eefd0d@suse.com>
On 11/11/2016 11:22 AM, Hannes Reinecke wrote:
> On 11/11/2016 05:35 PM, Christoph Hellwig wrote:
>> On Fri, Nov 11, 2016 at 04:46:34PM +0100, Hannes Reinecke wrote:
>>> This patch enables full scsi-mq support for the hpsa driver.
>>> Due to some reports of performance regressions this patch
>>> also adds a parameter 'use_blk_mq' which can be used to
>>> disable multiqueue support if required.
>>
>> This patch looks odd to me. The hardware does not seem to support
>> multiple submission queues, which makes exposing nr_hw_queues > 1
>> rather pointless given that the block layer (using blk-mq or the legacy
>> path) can already steer completions to the submitting cpu.
>>
> Well, it's the same as with megasas and mpt3sas. Each of those have
> a single MMIO register where the driver writes the address of the
> command into. What exactly the hardware does in the back doesn't
> really matter here; the command is in memory and the hardware can
> access it as it sees fit. So from that point of view we can assume
> having a submission queue to match the completion queue;
> With that setup we do have a contention point on the single command
> register, but that's about it.
That's just wrong. We can have multiple completion queues without having
multiple submission queues. And for that to work ideally, you tell
blk/scsi-mq that you have 1 queue, since that is what you have. You
don't get great submission side scaling, but we can't pull that stuff
out of thin air. How you interface to the hardware DOES matter, it's the
whole point of multiple submission queues. Trying to make up some
imaginary 1:1 mapping between submission and completion queues is
nonsensical, when it doesn't exist.
> We still should benefit from scsi-mq, though.
Sure, but with 1 submission queue.
--
Jens Axboe
^ permalink raw reply
* [PATCH] isci: fix typo in deg_dbg message
From: Colin King @ 2016-11-12 18:30 UTC (permalink / raw)
To: Intel SCU Linux support, Artur Paszkiewicz,
James E . J . Bottomley, Martin K . Petersen, linux-scsi
Cc: linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Trivial fix to typo "repsonse" to "response" in dev_dbg message.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/scsi/isci/request.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/isci/request.c b/drivers/scsi/isci/request.c
index b709d2b..47f66e9 100644
--- a/drivers/scsi/isci/request.c
+++ b/drivers/scsi/isci/request.c
@@ -2473,7 +2473,7 @@ static void isci_request_process_response_iu(
"%s: resp_iu = %p "
"resp_iu->status = 0x%x,\nresp_iu->datapres = %d "
"resp_iu->response_data_len = %x, "
- "resp_iu->sense_data_len = %x\nrepsonse data: ",
+ "resp_iu->sense_data_len = %x\nresponse data: ",
__func__,
resp_iu,
resp_iu->status,
--
2.10.2
^ permalink raw reply related
* Re: [PATCH] hpsa: scsi-mq support
From: Christoph Hellwig @ 2016-11-12 17:32 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Christoph Hellwig, Hannes Reinecke, Christoph Hellwig,
Martin K. Petersen, James Bottomley, Don Brace, linux-scsi,
Jens Axboe
In-Reply-To: <39d1198d-eb49-71ed-9139-f0efd9eefd0d@suse.com>
On Fri, Nov 11, 2016 at 07:22:05PM +0100, Hannes Reinecke wrote:
> Well, it's the same as with megasas and mpt3sas. Each of those have
> a single MMIO register where the driver writes the address of the
> command into. What exactly the hardware does in the back doesn't
> really matter here; the command is in memory and the hardware can
> access it as it sees fit. So from that point of view we can assume
> having a submission queue to match the completion queue;
> With that setup we do have a contention point on the single command
> register, but that's about it.
> We still should benefit from scsi-mq, though.
How do we benefit from scsi-mq in this case? We still hit global
cachelines like commands_outstanding in the driver, and we lost the
batching done by the ctx -> hw_ctx layering for the single queue
blk-mq case. We also get much less efficient merging and will not
have the chance of having and I/O schedule in the near future.
But back to my question from the last mail: What workload is improved
by using this patch?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox