mpt3sas: Full mq support, part 1
Subsystems (2)
v2 cover mpt3sas: Full mq support, part 1
Hi all,
this is the first part of my patchset to enable scsi multiqueue for the mpt3sas driver.
While the HBA only has a single mailbox register for submitting commands, it does have individual receive queues per MSI-X interrupt and as such does benefit from converting it to full multiqueue support.
On request from Broadcom the patchset has been split in two parts, one to enable lockless command submission and converting to scsi-mq, and the other one for exposing all hardware queues to the OS.
As usual, comments and reviews are welcome.
Changes to v1:
- Include reviews from Christoph
- Use reserved commands for ioctl passthrough commands
- Include reviews from Sreekanth
Hannes Reinecke (11):
mpt3sas: switch to pci_alloc_irq_vectors
mpt3sas: set default value for cb_idx
mpt3sas: use 'list_splice_init()'
mpt3sas: separate out _base_recovery_check()
mpt3sas: open-code _scsih_scsi_lookup_get()
mpt3sas: Introduce mpt3sas_get_st_from_smid()
mpt3sas: check command status before attempting abort
mpt3sas: always use smid 1 for ioctl passthrough
mpt3sas: lockless command submission for scsi-mq
scsi: allocate reserved commands
mpt3sas: register reserved commands
drivers/scsi/mpt3sas/mpt3sas_base.c | 277 +++++++++--------- drivers/scsi/mpt3sas/mpt3sas_base.h | 17 +- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 85 ++++-- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 482 ++++++++++++++----------------- drivers/scsi/mpt3sas/mpt3sas_warpdrive.c | 22 +- drivers/scsi/scsi_lib.c | 1 + include/scsi/scsi_host.h | 1 + 7 files changed, 426 insertions(+), 459 deletions(-)
Signature · 1 line
1.8.5.6
PATCH v2 1/11 mpt3sas: switch to pci_alloc_irq_vectors
Cleanup the MSI-X handling allowing us to use
the PCI-layer provided vector allocation.
--- drivers/scsi/mpt3sas/mpt3sas_base.c | 105 +++++++++++++++++------------------- drivers/scsi/mpt3sas/mpt3sas_base.h | 2 - 2 files changed, 48 insertions(+), 59 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_base.c +48 -57
@@ -1148,7 +1148,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)/* TMs are on msix_index == 0 */ if (reply_q->msix_index ==0 )continue ; -synchronize_irq (reply_q->vector); +synchronize_irq (pci_irq_vector (ioc->pdev, reply_q->msix_index)); } }
@@ -1837,11 +1837,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)list_for_each_entry_safe (reply_q, next, &ioc->reply_queue_list, list) {list_del (&reply_q->list); -if (smp_affinity_enable) { -irq_set_affinity_hint (reply_q->vector,NULL ); -free_cpumask_var (reply_q->affinity_hint); - } -free_irq (reply_q->vector, reply_q); +free_irq (pci_irq_vector (ioc->pdev, reply_q->msix_index), + reply_q);kfree (reply_q); } }
@@ -1850,13 +1847,13 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)* _base_request_irq - request irq * @ioc: per adapter object * @index: msix index into vector table -* @vector: irq vector * * Inserting respective reply_queue into the list. */ static int -_base_request_irq (struct MPT3SAS_ADAPTER *ioc,u8 index,u32 vector) +_base_request_irq (struct MPT3SAS_ADAPTER *ioc,u8 index) { +struct pci_dev *pdev = ioc->pdev;struct adapter_reply_queue *reply_q;int r;
@@ -1868,14 +1865,6 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) } reply_q->ioc = ioc; reply_q->msix_index = index; - reply_q->vector = vector; - -if (smp_affinity_enable) { -if (!zalloc_cpumask_var (&reply_q->affinity_hint,GFP_KERNEL )) { -kfree (reply_q); -return -ENOMEM ; - } - }atomic_set (&reply_q->busy,0 );if (ioc->msix_enable)
@@ -1884,12 +1873,11 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)else snprintf (reply_q->name,MPT_NAME_LENGTH ,"%s%d" , ioc->driver_name, ioc->id); - r =request_irq (vector, _base_interrupt,IRQF_SHARED , reply_q->name, - reply_q); + r =request_irq (pci_irq_vector (pdev, index), _base_interrupt, +IRQF_SHARED , reply_q->name, reply_q);if (r) {pr_err (MPT3SAS_FMT "unable to allocate interrupt %d!\n" , - reply_q->name, vector); -free_cpumask_var (reply_q->affinity_hint); + reply_q->name,pci_irq_vector (pdev, index));kfree (reply_q);return -EBUSY ; }
@@ -1925,6 +1913,21 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)if (!nr_msix)return ; +if (smp_affinity_enable) { +list_for_each_entry (reply_q, &ioc->reply_queue_list, list) { +const cpumask_t *mask =pci_irq_get_affinity (ioc->pdev, + reply_q->msix_index); +if (!mask) { +pr_warn (MPT3SAS_FMT "no affinity for msi %x\n" , + ioc->name, reply_q->msix_index); +continue ; + } + +for_each_cpu (cpu, mask) + ioc->cpu_msix_table[cpu] = reply_q->msix_index; + } +return ; + } cpu =cpumask_first (cpu_online_mask);list_for_each_entry (reply_q, &ioc->reply_queue_list, list) {
@@ -1938,18 +1941,9 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) group++;for (i =0 ; i < group ; i++) { - ioc->cpu_msix_table[cpu] = index; -if (smp_affinity_enable) -cpumask_or (reply_q->affinity_hint, - reply_q->affinity_hint,get_cpu_mask (cpu)); + ioc->cpu_msix_table[cpu] = reply_q->msix_index; cpu =cpumask_next (cpu, cpu_online_mask); } -if (smp_affinity_enable) -if (irq_set_affinity_hint (reply_q->vector, - reply_q->affinity_hint)) -dinitprintk (ioc,pr_info (MPT3SAS_FMT -"Err setting affinity hint to irq vector %d\n" , - ioc->name, reply_q->vector)); index++; } }
@@ -1976,10 +1970,10 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)static int _base_enable_msix (struct MPT3SAS_ADAPTER *ioc) { -struct msix_entry *entries, *a;int r;int i, local_max_msix_vectors;u8 try_msix =0 ; +unsigned int irq_flags =PCI_IRQ_MSIX ;if (msix_disable ==-1 || msix_disable ==0 ) try_msix =1 ;
@@ -1991,7 +1985,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) goto try_ioapic; ioc->reply_queue_count =min_t (int, ioc->cpu_count, - ioc->msix_vector_count); + ioc->msix_vector_count); printk(MPT3SAS_FMT "MSI-X vectors supported: %d, no of cores" ": %d, max_msix_vectors: %d\n", ioc->name, ioc->msix_vector_count,
@@ -2002,56 +1996,51 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)else local_max_msix_vectors = max_msix_vectors; -if (local_max_msix_vectors >0 ) { +if (local_max_msix_vectors >0 ) ioc->reply_queue_count =min_t (int, local_max_msix_vectors, ioc->reply_queue_count); - ioc->msix_vector_count = ioc->reply_queue_count; - }else if (local_max_msix_vectors ==0 ) +else if (local_max_msix_vectors ==0 ) goto try_ioapic;if (ioc->msix_vector_count < ioc->cpu_count) smp_affinity_enable =0 ; - entries =kcalloc (ioc->reply_queue_count,sizeof (struct msix_entry ), -GFP_KERNEL ); -if (!entries) { -dfailprintk (ioc,pr_info (MPT3SAS_FMT -"kcalloc failed @ at %s:%d/%s() !!!\n" , - ioc->name, __FILE__, __LINE__, __func__)); - goto try_ioapic; - } +if (smp_affinity_enable) + irq_flags |=PCI_IRQ_AFFINITY ; -for (i =0 , a = entries; i < ioc->reply_queue_count; i++, a++) - a->entry = i; - - r =pci_enable_msix_exact (ioc->pdev, entries, ioc->reply_queue_count); -if (r) { + r =pci_alloc_irq_vectors (ioc->pdev,1 , ioc->reply_queue_count, + irq_flags); +if (r <0 ) {dfailprintk (ioc,pr_info (MPT3SAS_FMT -"pci_enable_msix_exact failed (r=%d) !!!\n" , +"pci_alloc_irq_vectors failed (r=%d) !!!\n" , ioc->name, r)); -kfree (entries); goto try_ioapic; } ioc->msix_enable =1 ; -for (i =0 , a = entries; i < ioc->reply_queue_count; i++, a++) { - r =_base_request_irq (ioc, i, a->vector); + ioc->reply_queue_count = r; +for (i =0 ; i < ioc->reply_queue_count; i++) { + r =_base_request_irq (ioc, i);if (r) {_base_free_irq (ioc);_base_disable_msix (ioc); -kfree (entries); goto try_ioapic; } } -kfree (entries);return 0 ;/* failback to io_apic interrupt routing */ try_ioapic: ioc->reply_queue_count =1 ; - r =_base_request_irq (ioc,0 , ioc->pdev->irq); + r =pci_alloc_irq_vectors (ioc->pdev,1 ,1 ,PCI_IRQ_LEGACY ); +if (r <0 ) { +dfailprintk (ioc,pr_info (MPT3SAS_FMT +"pci_alloc_irq_vector(legacy) failed (r=%d) !!!\n" , + ioc->name, r)); + }else + r =_base_request_irq (ioc,0 );return r; }
@@ -2222,7 +2211,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)list_for_each_entry (reply_q, &ioc->reply_queue_list, list)pr_info (MPT3SAS_FMT "%s: IRQ %d\n" , reply_q->name, ((ioc->msix_enable) ?"PCI-MSI-X enabled" : -"IO-APIC enabled" ), reply_q->vector); +"IO-APIC enabled" ), +pci_irq_vector (ioc->pdev, reply_q->msix_index));pr_info (MPT3SAS_FMT "iomem(0x%016llx), mapped(0x%p), size(%d)\n" , ioc->name, (unsigned long long )chip_phys, ioc->chip, memap_sz);
@@ -5357,7 +5347,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) sizeof(resource_size_t *), GFP_KERNEL);if (!ioc->reply_post_host_index) {dfailprintk (ioc,pr_info (MPT3SAS_FMT "allocation " -"for cpu_msix_table failed!!!\n" , ioc->name)); +"for reply_post_host_index failed!!!\n" , + ioc->name)); r = -ENOMEM ; goto out_free_resources; }
drivers/scsi/mpt3sas/mpt3sas_base.h +0 -2
@@ -719,12 +719,10 @@ struct _event_ack_list {struct adapter_reply_queue {struct MPT3SAS_ADAPTER *ioc;u8 msix_index; -unsigned int vector;u32 reply_post_host_index;Mpi2ReplyDescriptorsUnion_t *reply_post_free;char name[MPT_NAME_LENGTH ];atomic_t busy; -cpumask_var_t affinity_hint;struct list_head list; };
Signature · 1 line
1.8.5.6
PATCH v2 2/11 mpt3sas: set default value for cb_idx
In reply to Hannes ReineckeNo functional change.
--- drivers/scsi/mpt3sas/mpt3sas_base.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_base.c +2 -3
@@ -873,7 +873,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) _base_get_cb_idx(struct MPT3SAS_ADAPTER *ioc, u16 smid) { int i; -u8 cb_idx; +u8 cb_idx =0xFF ;if (smid < ioc->hi_priority_smid) { i = smid -1 ;
@@ -884,8 +884,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) }else if (smid <= ioc->hba_queue_depth) { i = smid - ioc->internal_smid; cb_idx = ioc->internal_lookup[i].cb_idx; - }else - cb_idx =0xFF ; + }return cb_idx; }
Signature · 1 line
1.8.5.6
PATCH v2 3/11 mpt3sas: use 'list_splice_init()'
In reply to Hannes ReineckeUse 'list_splice_init()' instead of hand-crafted function. No functional change.
--- drivers/scsi/mpt3sas/mpt3sas_base.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_base.c +1 -9
@@ -2395,20 +2395,12 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) {unsigned long flags;int i; -struct chain_tracker *chain_req, *next;spin_lock_irqsave (&ioc->scsi_lookup_lock, flags);if (smid < ioc->hi_priority_smid) {/* scsiio queue */ i = smid -1 ; -if (!list_empty (&ioc->scsi_lookup[i].chain_list)) { -list_for_each_entry_safe (chain_req, next, - &ioc->scsi_lookup[i].chain_list, tracker_list) { -list_del_init (&chain_req->tracker_list); -list_add (&chain_req->tracker_list, - &ioc->free_chain_list); - } - } +list_splice_init (&st->chain_list, &ioc->free_chain_list); ioc->scsi_lookup[i].cb_idx =0xFF ; ioc->scsi_lookup[i].scmd =NULL ; ioc->scsi_lookup[i].direct_io =0 ;
Signature · 1 line
1.8.5.6
Re: [PATCHv2 03/11] mpt3sas: use 'list_splice_init()'
Quoted 4 lines · Hannes Reinecke
Use '
list_splice_init()' instead of hand-crafted function. No functional change.Signed-off-by Hannes Reinecke <[email protected]>
Looks fine,
PATCH v2 4/11 mpt3sas: separate out _base_recovery_check()
In reply to Hannes ReineckeNo functional change.
--- drivers/scsi/mpt3sas/mpt3sas_base.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_base.c +14 -9
@@ -2383,6 +2383,19 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)return smid; } +static void +_base_recovery_check (struct MPT3SAS_ADAPTER *ioc) +{ +/* +* See _wait_for_commands_to_complete() call with regards to this code. +*/ +if (ioc->shost_recovery && ioc->pending_io_count) { +if (ioc->pending_io_count ==1 ) +wake_up (&ioc->reset_wq); + ioc->pending_io_count--; + } +} +/** * mpt3sas_base_free_smid - put smid back on free_list * @ioc: per adapter object
@@ -2407,15 +2420,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)list_add (&ioc->scsi_lookup[i].tracker_list, &ioc->free_list);spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags); -/* -* See _wait_for_commands_to_complete() call with regards -* to this code. -*/ -if (ioc->shost_recovery && ioc->pending_io_count) { -if (ioc->pending_io_count ==1 ) -wake_up (&ioc->reset_wq); - ioc->pending_io_count--; - } +_base_recovery_check (ioc);return ; }else if (smid < ioc->internal_smid) {/* hi-priority */
Signature · 1 line
1.8.5.6
PATCH v2 5/11 mpt3sas: open-code _scsih_scsi_lookup_get()
In reply to Hannes ReineckeJust a wrapper around the scsi lookup array and only used in one place, so open-code it.
--- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_scsih.c +1 -14
@@ -1061,19 +1061,6 @@ struct _sas_node * }/** -* _scsih_scsi_lookup_get - returns scmd entry -* @ioc: per adapter object -* @smid: system request message index -* -* Returns the smid stored scmd pointer. -*/ -static struct scsi_cmnd * -_scsih_scsi_lookup_get (struct MPT3SAS_ADAPTER *ioc,u16 smid) -{ -return ioc->scsi_lookup[smid -1 ].scmd; -} - -/** * __scsih_scsi_lookup_get_clear - returns scmd entry without * holding any lock. * @ioc: per adapter object
@@ -6126,7 +6113,7 @@ static inline bool ata_12_16_cmd(struct scsi_cmnd *scmd)for (smid =1 ; smid <= ioc->scsiio_depth; smid++) {if (ioc->shost_recovery) goto out; - scmd =_scsih_scsi_lookup_get (ioc, smid); + scmd = ioc->scsi_lookup[smid -1 ].scmd;if (!scmd)continue ; sdev = scmd->device;
Signature · 1 line
1.8.5.6
PATCH v2 6/11 mpt3sas: Introduce mpt3sas_get_st_from_smid()
In reply to Hannes ReineckeAbstract accesses to the scsi_lookup array by introducing mpt3sas_get_st_from_smid().
--- drivers/scsi/mpt3sas/mpt3sas_base.c | 20 ++++++++++++++++---- drivers/scsi/mpt3sas/mpt3sas_base.h | 2 ++ drivers/scsi/mpt3sas/mpt3sas_scsih.c | 2 +- drivers/scsi/mpt3sas/mpt3sas_warpdrive.c | 4 +++- 4 files changed, 22 insertions(+), 6 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_base.c +16 -4
@@ -862,6 +862,14 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)return 1 ; } +struct scsiio_tracker * +mpt3sas_get_st_from_smid (struct MPT3SAS_ADAPTER *ioc,u16 smid) +{ +WARN_ON (!smid); +WARN_ON (smid >= ioc->hi_priority_smid); +return &ioc->scsi_lookup[smid -1 ]; +} +/** * _base_get_cb_idx - obtain the callback index * @ioc: per adapter object
@@ -876,8 +884,11 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)u8 cb_idx =0xFF ;if (smid < ioc->hi_priority_smid) { - i = smid -1 ; - cb_idx = ioc->scsi_lookup[i].cb_idx; +struct scsiio_tracker *st; + + st =mpt3sas_get_st_from_smid (ioc, smid); +if (st) + cb_idx = st->cb_idx; }else if (smid < ioc->internal_smid) { i = smid - ioc->hi_priority_smid; cb_idx = ioc->hpr_lookup[i].cb_idx;
@@ -1268,6 +1279,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) _base_get_chain_buffer_tracker(struct MPT3SAS_ADAPTER *ioc, u16 smid) {struct chain_tracker *chain_req; +struct scsiio_tracker *st;unsigned long flags;spin_lock_irqsave (&ioc->scsi_lookup_lock, flags);
@@ -1280,8 +1292,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) chain_req =list_entry (ioc->free_chain_list.next, struct chain_tracker, tracker_list);list_del_init (&chain_req->tracker_list); -list_add_tail (&chain_req->tracker_list, - &ioc->scsi_lookup[smid -1 ].chain_list); + st =mpt3sas_get_st_from_smid (ioc, smid); +list_add_tail (&chain_req->tracker_list, &st->chain_list);spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags);return chain_req; }
drivers/scsi/mpt3sas/mpt3sas_base.h +2 -0
@@ -1247,6 +1247,8 @@ __le32 mpt3sas_base_get_sense_buffer_dma(struct MPT3SAS_ADAPTER *ioc,u16 mpt3sas_base_get_smid_hpr (struct MPT3SAS_ADAPTER *ioc,u8 cb_idx);u16 mpt3sas_base_get_smid_scsiio (struct MPT3SAS_ADAPTER *ioc,u8 cb_idx,struct scsi_cmnd *scmd); +struct scsiio_tracker *mpt3sas_get_st_from_smid (struct MPT3SAS_ADAPTER *ioc, +u16 smid);u16 mpt3sas_base_get_smid (struct MPT3SAS_ADAPTER *ioc,u8 cb_idx);void mpt3sas_base_free_smid (struct MPT3SAS_ADAPTER *ioc,u16 smid);
drivers/scsi/mpt3sas/mpt3sas_scsih.c +1 -1
@@ -2269,7 +2269,7 @@ struct _sas_node * }if (type ==MPI2_SCSITASKMGMT_TASKTYPE_ABORT_TASK ) - scsi_lookup = &ioc->scsi_lookup[smid_task -1 ]; + scsi_lookup =mpt3sas_get_st_from_smid (ioc, smid_task); dtmprintk(ioc, pr_info(MPT3SAS_FMT "sending tm: handle(0x%04x), task_type(0x%02x), smid(%d)\n",
drivers/scsi/mpt3sas/mpt3sas_warpdrive.c +3 -1
@@ -270,7 +270,9 @@inline u8 mpt3sas_scsi_direct_io_get (struct MPT3SAS_ADAPTER *ioc,u16 smid) { -return ioc->scsi_lookup[smid -1 ].direct_io; +struct scsiio_tracker *st =mpt3sas_get_st_from_smid (ioc, smid); + +return st ? st->direct_io :0 ; }/**
Signature · 1 line
1.8.5.6
Re: [PATCHv2 06/11] mpt3sas: Introduce mpt3sas_get_st_from_smid()
drivers/scsi/mpt3sas/mpt3sas_base.c static int mpt3sas_remove_dead_ioc_func(void *arg) · line 871
Quoted 7 lines · Hannes Reinecke
drivers/scsi/mpt3sas/mpt3sas_base.c +7
-0+struct scsiio_tracker * +mpt3sas_get_st_from_smid (struct MPT3SAS_ADAPTER *ioc,u16 smid) +{ +WARN_ON (!smid); +WARN_ON (smid >= ioc->hi_priority_smid); +return &ioc->scsi_lookup[smid -1 ]; +}
Hmm if smid == 0 we'd be accessing &ioc->scsi_lookup[-1] and that's an array out of bounds.
In patch 9/11 you'll get a unique tag of -2 (which is _not_ SCSI_NO_TAG) and will be an array out of bounds as well in blk_map_queue_find_tag() or blk_mq_tag_to_rq().
if (WARN_ON(!smid))
return NULL;
Should fix both.
Signature · 6 lines
Johannes Thumshirn Storage [email protected] +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCHv2 06/11] mpt3sas: Introduce mpt3sas_get_st_from_smid()
Quoted 12 lines · Johannes Thumshirn
drivers/scsi/mpt3sas/mpt3sas_base.c static int mpt3sas_remove_dead_ioc_func(void *arg) · line 871
Quoted 7 lines · Hannes Reinecke
drivers/scsi/mpt3sas/mpt3sas_base.c +7
-0+struct scsiio_tracker * +mpt3sas_get_st_from_smid (struct MPT3SAS_ADAPTER *ioc,u16 smid) +{ +WARN_ON (!smid); +WARN_ON (smid >= ioc->hi_priority_smid); +return &ioc->scsi_lookup[smid -1 ]; +}Hmm if smid == 0 we'd be accessing &ioc->scsi_lookup[-1] and that's an array out of bounds.
Quoted 1 line
In patch 9/11 you'll get a unique tag of -2 (which is _not_
SCSI_NO_TAG)
Of cause not you'll get -1 and this is SCSI_NO_TAG.
Not my day...
Signature · 6 lines
Johannes Thumshirn Storage [email protected] +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH v2 7/11 mpt3sas: check command status before attempting abort
In reply to Hannes ReineckeWhen attempting a command abort we should check the command status prior to sending the abort; the command might've been completed already.
--- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_scsih.c +6 -3
@@ -2261,6 +2261,12 @@ struct _sas_node *return (!rc) ?SUCCESS :FAILED ; } +if (type ==MPI2_SCSITASKMGMT_TASKTYPE_ABORT_TASK ) { + scsi_lookup =mpt3sas_get_st_from_smid (ioc, smid_task); +if (scsi_lookup->cb_idx ==0xFF ) +return SUCCESS ; + } + smid =mpt3sas_base_get_smid_hpr (ioc, ioc->tm_cb_idx);if (!smid) { pr_err(MPT3SAS_FMT "%s: failed obtaining a smid\n",
@@ -2268,9 +2274,6 @@ struct _sas_node *return FAILED ; } -if (type ==MPI2_SCSITASKMGMT_TASKTYPE_ABORT_TASK ) - scsi_lookup =mpt3sas_get_st_from_smid (ioc, smid_task); -dtmprintk (ioc,pr_info (MPT3SAS_FMT "sending tm: handle(0x%04x), task_type(0x%02x), smid(%d)\n" , ioc->name, handle, type, smid_task));
Signature · 1 line
1.8.5.6
Re: [PATCHv2 07/11] mpt3sas: check command status before attempting abort
Quoted 14 lines · Hannes Reinecke
When attempting a command abort we should check the command status prior to sending the abort; the command might've been completed already.
Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/scsi/mpt3sas/mpt3sas_scsih.c +0
-0
drivers/scsi/mpt3sas/mpt3sas_scsih.c struct _sas_node * · line 2268
Quoted 8 lines
drivers/scsi/mpt3sas/mpt3sas_scsih.c +5
-0@@ -2261,6 +2261,12 @@ struct _sas_node *return (!rc) ?SUCCESS :FAILED ; } +if (type ==MPI2_SCSITASKMGMT_TASKTYPE_ABORT_TASK ) { + scsi_lookup =mpt3sas_get_st_from_smid (ioc, smid_task); +if (scsi_lookup->cb_idx ==0xFF ) +return SUCCESS ; + }
Not really new in this patch, but I'm worried that we don't check for a NULL return here. Are we 100% sure no invalid smid can be passed in?
But the patch itself looks fine:
Re: [PATCHv2 07/11] mpt3sas: check command status before attempting abort
Quoted 18 lines · Christoph Hellwig
Quoted 17 lines · Hannes Reinecke
When attempting a command abort we should check the command status prior to sending the abort; the command might've been completed already.
Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/scsi/mpt3sas/mpt3sas_scsih.c +0
-0@@ -2261,6 +2261,12 @@ struct _sas_node *return (!rc) ?SUCCESS :FAILED ; }
Quoted 9 lines
drivers/scsi/mpt3sas/mpt3sas_scsih.c struct _sas_node * · line 2268
Quoted 5 lines
drivers/scsi/mpt3sas/mpt3sas_scsih.c +5
-0+if (type ==MPI2_SCSITASKMGMT_TASKTYPE_ABORT_TASK ) { + scsi_lookup =mpt3sas_get_st_from_smid (ioc, smid_task); +if (scsi_lookup->cb_idx ==0xFF ) +return SUCCESS ; + }Not really new in this patch, but I'm worried that we don't check for a NULL return here. Are we 100% sure no invalid smid can be passed in?
The check for that is being added in path 09/11; should've moved it to here.
Cheers,
Hannes
Signature · 5 lines
Dr. Hannes Reinecke Teamlead Storage & Networking [email protected] +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
PATCH v2 8/11 mpt3sas: always use smid 1 for ioctl passthrough
In reply to Hannes Reineckeioctl passthrough commands require a SCSIIO smid, but cannot easily integrate with the block layer. But as we're only ever allowing one ioctl command at a time we can reserve smid 1 for ioctl commands.
--- drivers/scsi/mpt3sas/mpt3sas_base.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_base.c +20 -8
@@ -2355,13 +2355,20 @@ struct scsiio_tracker *return 0 ; } - request =list_entry (ioc->free_list.next, - struct scsiio_tracker, tracker_list); - request->scmd = scmd; +if (!scmd) { +/* ioctl command, always use the first slot */ + request = ioc->lookup[0 ]; + request->scmd =NULL ; + }else { + request =list_entry (ioc->free_list.next, + struct scsiio_tracker, tracker_list); + request->scmd = scmd; + } request->cb_idx = cb_idx; smid = request->smid; request->msix_io =_base_get_msix_index (ioc); -list_del (&request->tracker_list); +if (scmd) +list_del (&request->tracker_list);spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags);return smid; }
@@ -2429,7 +2436,9 @@ struct scsiio_tracker * ioc->scsi_lookup[i].cb_idx =0xFF ; ioc->scsi_lookup[i].scmd =NULL ; ioc->scsi_lookup[i].direct_io =0 ; -list_add (&ioc->scsi_lookup[i].tracker_list, &ioc->free_list); +if (i >0 ) +list_add (&ioc->scsi_lookup[i].tracker_list, + &ioc->free_list);spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags);_base_recovery_check (ioc);
@@ -5165,14 +5174,17 @@ struct scsiio_tracker *spin_lock_irqsave (&ioc->scsi_lookup_lock, flags);INIT_LIST_HEAD (&ioc->free_list); smid =1 ; -for (i =0 ; i < ioc->scsiio_depth; i++, smid++) { +for (i =1 ; i < ioc->scsiio_depth; i++, smid++) {INIT_LIST_HEAD (&ioc->scsi_lookup[i].chain_list); ioc->scsi_lookup[i].cb_idx =0xFF ; ioc->scsi_lookup[i].smid = smid; ioc->scsi_lookup[i].scmd =NULL ; ioc->scsi_lookup[i].direct_io =0 ; -list_add_tail (&ioc->scsi_lookup[i].tracker_list, - &ioc->free_list); +if (i ==1 ) +INIT_LIST_HEAD (&ioc->lookup[i].tracker_list); +else +list_add_tail (&ioc->scsi_lookup[i].tracker_list, + &ioc->free_list); }/* hi-priority queue */
Signature · 1 line
1.8.5.6
Re: [PATCHv2 08/11] mpt3sas: always use smid 1 for ioctl passthrough
Quoted 4 lines · Hannes Reinecke
ioctl passthrough commands require a SCSIIO smid, but cannot easily integrate with the block layer. But as we're only ever allowing one ioctl command at a time we can reserve smid 1 for ioctl commands.
I like the idea, but I don't think the implementation is correct. More below.
Quoted 16 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +7
-3@@ -2355,13 +2355,20 @@ struct scsiio_tracker *return 0 ; } - request =list_entry (ioc->free_list.next, - struct scsiio_tracker, tracker_list); - request->scmd = scmd; +if (!scmd) { +/* ioctl command, always use the first slot */ + request = ioc->lookup[0 ]; + request->scmd =NULL ; + }else { + request =list_entry (ioc->free_list.next, + struct scsiio_tracker, tracker_list);
drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 2373
Quoted 10 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +4
-1+ request->scmd = scmd; + } request->cb_idx = cb_idx; smid = request->smid; request->msix_io =_base_get_msix_index (ioc); -list_del (&request->tracker_list); +if (scmd) +list_del (&request->tracker_list);spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags);return smid;
The freelist check just before the patch context and the locking don't make much sense here. I'd say just use a separate helper for the ioctl smid, ala:
u16mpt3sas_base_init_smid_pt(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx)
{
struct scsiio_tracker *request = ioc->lookup[0];
request->cb_idx = cb_idx;
request->msix_io = _base_get_msix_index(ioc);
return request->smid;
}or given how trivial this is just open-code it in the caller.
drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 2441
Quoted 7 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +0
-0ioc->scsi_lookup[i].cb_idx =0xFF ;drivers/scsi/mpt3sas/mpt3sas_base.c +3
-1ioc->scsi_lookup[i].scmd =NULL ; ioc->scsi_lookup[i].direct_io =0 ; -list_add (&ioc->scsi_lookup[i].tracker_list, &ioc->free_list); +if (i >0 ) +list_add (&ioc->scsi_lookup[i].tracker_list, + &ioc->free_list);
Here we special case only request 0 as not added to the list.
Quoted 8 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +1
-1@@ -5165,14 +5174,17 @@ struct scsiio_tracker *spin_lock_irqsave (&ioc->scsi_lookup_lock, flags);INIT_LIST_HEAD (&ioc->free_list); smid =1 ; -for (i =0 ; i < ioc->scsiio_depth; i++, smid++) { +for (i =1 ; i < ioc->scsiio_depth; i++, smid++) {INIT_LIST_HEAD (&ioc->scsi_lookup[i].chain_list); ioc->scsi_lookup[i].cb_idx =0xFF ;
drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 5187
Quoted 10 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +5
-2ioc->scsi_lookup[i].smid = smid; ioc->scsi_lookup[i].scmd =NULL ; ioc->scsi_lookup[i].direct_io =0 ; -list_add_tail (&ioc->scsi_lookup[i].tracker_list, - &ioc->free_list); +if (i ==1 ) +INIT_LIST_HEAD (&ioc->lookup[i].tracker_list); +else +list_add_tail (&ioc->scsi_lookup[i].tracker_list, + &ioc->free_list);
And here we don't even iterate over smid 0, and then special case smid 1.
And note that the code in both loops is otherwise duplicated to start with and could realy use a little helper to initialize the scsiio_tracker structure.
Last but not least can_queue is initialized as:
ioc->shost->can_queue = ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT;
which doesn't take this new stolen smid into account.
Re: [PATCHv2 08/11] mpt3sas: always use smid 1 for ioctl passthrough
Quoted 50 lines · Christoph Hellwig
Quoted 4 lines · Hannes Reinecke
ioctl passthrough commands require a SCSIIO smid, but cannot easily integrate with the block layer. But as we're only ever allowing one ioctl command at a time we can reserve smid 1 for ioctl commands.
I like the idea, but I don't think the implementation is correct. More below.
Quoted 16 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +7
-3@@ -2355,13 +2355,20 @@ struct scsiio_tracker *return 0 ; } - request =list_entry (ioc->free_list.next, - struct scsiio_tracker, tracker_list); - request->scmd = scmd; +if (!scmd) { +/* ioctl command, always use the first slot */ + request = ioc->lookup[0 ]; + request->scmd =NULL ; + }else { + request =list_entry (ioc->free_list.next, + struct scsiio_tracker, tracker_list);drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 2373
Quoted 10 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +4
-1+ request->scmd = scmd; + } request->cb_idx = cb_idx; smid = request->smid; request->msix_io =_base_get_msix_index (ioc); -list_del (&request->tracker_list); +if (scmd) +list_del (&request->tracker_list);spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags);return smid;The freelist check just before the patch context and the locking don't make much sense here. I'd say just use a separate helper for the ioctl smid, ala:
u16
mpt3sas_base_init_smid_pt(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx){ struct scsiio_tracker *request = ioc->lookup[0]; request->cb_idx = cb_idx; request->msix_io = _base_get_msix_index(ioc); return request->smid; }
Quoted 3 lines
or given how trivial this is just open-code it in the caller.
In principle, yes.
However, most of that is obsoleted when switching over to using reserved commands in patch 11/11; then we properly register 'reserved' commands with the block layer and this whole issue goes away.
Quoted 31 lines
drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 2441
Quoted 7 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +0
-0ioc->scsi_lookup[i].cb_idx =0xFF ;drivers/scsi/mpt3sas/mpt3sas_base.c +3
-1ioc->scsi_lookup[i].scmd =NULL ; ioc->scsi_lookup[i].direct_io =0 ; -list_add (&ioc->scsi_lookup[i].tracker_list, &ioc->free_list); +if (i >0 ) +list_add (&ioc->scsi_lookup[i].tracker_list, + &ioc->free_list);Here we special case only request 0 as not added to the list.
Quoted 8 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +1
-1@@ -5165,14 +5174,17 @@ struct scsiio_tracker *spin_lock_irqsave (&ioc->scsi_lookup_lock, flags);INIT_LIST_HEAD (&ioc->free_list); smid =1 ; -for (i =0 ; i < ioc->scsiio_depth; i++, smid++) { +for (i =1 ; i < ioc->scsiio_depth; i++, smid++) {INIT_LIST_HEAD (&ioc->scsi_lookup[i].chain_list); ioc->scsi_lookup[i].cb_idx =0xFF ;drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 5187
Quoted 10 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +5
-2ioc->scsi_lookup[i].smid = smid; ioc->scsi_lookup[i].scmd =NULL ; ioc->scsi_lookup[i].direct_io =0 ; -list_add_tail (&ioc->scsi_lookup[i].tracker_list, - &ioc->free_list); +if (i ==1 ) +INIT_LIST_HEAD (&ioc->lookup[i].tracker_list); +else +list_add_tail (&ioc->scsi_lookup[i].tracker_list, + &ioc->free_list);And here we don't even iterate over smid 0, and then special case smid 1.
Quoted 4 lines
And note that the code in both loops is otherwise duplicated to start with and could realy use a little helper to initialize the
scsiio_trackerstructure.
Again, most of that code will be obsoleted with patch 09/11, which switches over to scsi-mq.
This whole code is just a band-aid until the driver is switched over to scsi-mq, where we have correct allocation for reserved commands. I just couldn't be bothered to implement reserved commands for legacy sq, seeing that it'll be obsoleted at some point in the future anyway.
Quoted 6 lines
Last but not least
can_queueis initialized as:ioc->shost->can_queue = ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT;which doesn't take this new stolen smid into account.
Thing is, the internal commands are _never_ used anywhere, so 'can_queue' is actually still correct :-)
Cheers,
Hannes
Signature · 5 lines
Dr. Hannes Reinecke Teamlead Storage & Networking [email protected] +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
PATCH v2 9/11 mpt3sas: lockless command submission for scsi-mq
In reply to Hannes ReineckeEnable lockless command submission for scsi-mq by moving the command structure into the payload for struct request.
--- drivers/scsi/mpt3sas/mpt3sas_base.c | 123 ++++----- drivers/scsi/mpt3sas/mpt3sas_base.h | 13 +- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 85 ++++-- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 460 ++++++++++++++----------------- drivers/scsi/mpt3sas/mpt3sas_warpdrive.c | 20 +- 5 files changed, 330 insertions(+), 371 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_base.c +54 -69
@@ -865,9 +865,17 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)struct scsiio_tracker *mpt3sas_get_st_from_smid (struct MPT3SAS_ADAPTER *ioc,u16 smid) { +u32 unique_tag; +struct scsi_cmnd *cmd; +WARN_ON (!smid);WARN_ON (smid >= ioc->hi_priority_smid); -return &ioc->scsi_lookup[smid -1 ]; + unique_tag = smid -1 ; + cmd =scsi_host_find_tag (ioc->shost, unique_tag); +if (cmd) +return scsi_cmd_priv (cmd); + +return NULL ; }/**
@@ -2343,34 +2351,23 @@ struct scsiio_tracker * mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx, struct scsi_cmnd *scmd) { - unsigned long flags; struct scsiio_tracker *request;u16 smid; -spin_lock_irqsave (&ioc->scsi_lookup_lock, flags); -if (list_empty (&ioc->free_list)) { -spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags); -pr_err (MPT3SAS_FMT "%s: smid not available\n" , - ioc->name, __func__); -return 0 ; - } -if (!scmd) { -/* ioctl command, always use the first slot */ - request = ioc->lookup[0 ]; - request->scmd =NULL ; + smid =1 ; + request =mpt3sas_get_st_from_smid (ioc, smid); }else { - request =list_entry (ioc->free_list.next, - struct scsiio_tracker, tracker_list); - request->scmd = scmd; +u32 unique_tag =blk_mq_unique_tag (scmd->request); +u16 tag =blk_mq_unique_tag_to_tag (unique_tag); + request =scsi_cmd_priv (scmd); + smid = tag +1 ; } request->cb_idx = cb_idx; - smid = request->smid; request->msix_io =_base_get_msix_index (ioc); -if (scmd) -list_del (&request->tracker_list); -spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags); -return smid; + request->smid = smid; +INIT_LIST_HEAD (&request->chain_list); +return request->smid; }/**
@@ -2415,6 +2412,22 @@ struct scsiio_tracker * } } +void mpt3sas_base_clear_st (struct MPT3SAS_ADAPTER *ioc, +struct scsiio_tracker *st) +{ +if (WARN_ON (st->smid ==0 )) +return ; + st->cb_idx =0xFF ; + st->direct_io =0 ; +if (!list_empty (&st->chain_list)) { +unsigned long flags; + +spin_lock_irqsave (&ioc->scsi_lookup_lock, flags); +list_splice_init (&st->chain_list, &ioc->free_chain_list); +spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags); + } +} +/** * mpt3sas_base_free_smid - put smid back on free_list * @ioc: per adapter object
@@ -2428,22 +2441,21 @@ struct scsiio_tracker *unsigned long flags;int i; -spin_lock_irqsave (&ioc->scsi_lookup_lock, flags);if (smid < ioc->hi_priority_smid) { -/* scsiio queue */ - i = smid -1 ; -list_splice_init (&st->chain_list, &ioc->free_chain_list); - ioc->scsi_lookup[i].cb_idx =0xFF ; - ioc->scsi_lookup[i].scmd =NULL ; - ioc->scsi_lookup[i].direct_io =0 ; -if (i >0 ) -list_add (&ioc->scsi_lookup[i].tracker_list, - &ioc->free_list); -spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags); +struct scsiio_tracker *st; + st =mpt3sas_get_st_from_smid (ioc, smid); +if (WARN_ON (!st)) { +_base_recovery_check (ioc); +return ; + } +mpt3sas_base_clear_st (ioc, st);_base_recovery_check (ioc);return ; - }else if (smid < ioc->internal_smid) { + } + +spin_lock_irqsave (&ioc->scsi_lookup_lock, flags); +if (smid < ioc->internal_smid) {/* hi-priority */ i = smid - ioc->hi_priority_smid; ioc->hpr_lookup[i].cb_idx =0xFF ;
@@ -3276,10 +3288,6 @@ struct scsiio_tracker * ioc->config_page, ioc->config_page_dma); } -if (ioc->scsi_lookup) { -free_pages ((ulong )ioc->scsi_lookup, ioc->scsi_lookup_pages); - ioc->scsi_lookup =NULL ; - }kfree (ioc->hpr_lookup);kfree (ioc->internal_lookup);if (ioc->chain_lookup) {
@@ -3573,16 +3581,6 @@ struct scsiio_tracker * ioc->name, (unsigned long long) ioc->request_dma)); total_sz += sz; - sz = ioc->scsiio_depth *sizeof (struct scsiio_tracker ); - ioc->scsi_lookup_pages =get_order (sz); - ioc->scsi_lookup = (struct scsiio_tracker *)__get_free_pages ( -GFP_KERNEL , ioc->scsi_lookup_pages); -if (!ioc->scsi_lookup) { -pr_err (MPT3SAS_FMT "scsi_lookup: get_free_pages failed, sz(%d)\n" , - ioc->name, (int )sz); - goto out; - } -dinitprintk (ioc,pr_info (MPT3SAS_FMT "scsiio(0x%p): depth(%d)\n" , ioc->name, ioc->request, ioc->scsiio_depth));
@@ -5170,22 +5168,7 @@ struct scsiio_tracker *kfree (delayed_event_ack); } -/* initialize the scsi lookup free list */ spin_lock_irqsave (&ioc->scsi_lookup_lock, flags); -INIT_LIST_HEAD (&ioc->free_list); - smid =1 ; -for (i =1 ; i < ioc->scsiio_depth; i++, smid++) { -INIT_LIST_HEAD (&ioc->scsi_lookup[i].chain_list); - ioc->scsi_lookup[i].cb_idx =0xFF ; - ioc->scsi_lookup[i].smid = smid; - ioc->scsi_lookup[i].scmd =NULL ; - ioc->scsi_lookup[i].direct_io =0 ; -if (i ==1 ) -INIT_LIST_HEAD (&ioc->lookup[i].tracker_list); -else -list_add_tail (&ioc->scsi_lookup[i].tracker_list, - &ioc->free_list); - }/* hi-priority queue */ INIT_LIST_HEAD (&ioc->hpr_free_list);
@@ -5685,6 +5668,13 @@ struct scsiio_tracker * } } +void _count_pending (struct request *req,void *data,bool reserved) +{ +struct MPT3SAS_ADAPTER *ioc = data; + + ioc->pending_io_count++; +} +/** * _wait_for_commands_to_complete - reset controller * @ioc: Pointer to MPT_ADAPTER structure
@@ -5696,8 +5686,6 @@ struct scsiio_tracker * _wait_for_commands_to_complete(struct MPT3SAS_ADAPTER *ioc) {u32 ioc_state; -unsigned long flags; -u16 i; ioc->pending_io_count =0 ;
@@ -5706,11 +5694,8 @@ struct scsiio_tracker *return ;/* pending command count */ -spin_lock_irqsave (&ioc->scsi_lookup_lock, flags); -for (i =0 ; i < ioc->scsiio_depth; i++) -if (ioc->scsi_lookup[i].cb_idx !=0xFF ) - ioc->pending_io_count++; -spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags); +blk_mq_tagset_busy_iter (&ioc->shost->tag_set, + _count_pending, ioc);if (!ioc->pending_io_count)return ;
drivers/scsi/mpt3sas/mpt3sas_base.h +4 -9
@@ -646,19 +646,16 @@ struct chain_tracker {/** * struct scsiio_tracker - scsi mf request tracker * @smid: system message id -* @scmd: scsi request pointer * @cb_idx: callback index * @direct_io: To indicate whether I/O is direct (WARPDRIVE) -* @tracker_list: list of free request (ioc->free_list) +* @chain_list: list of associated firmware chain tracker * @msix_io: IO's msix */ struct scsiio_tracker {u16 smid; -struct scsi_cmnd *scmd;u8 cb_idx;u8 direct_io;struct list_head chain_list; -struct list_head tracker_list;u16 msix_io; };
@@ -1100,10 +1097,7 @@ struct MPT3SAS_ADAPTER {u8 *request;dma_addr_t request_dma;u32 request_dma_sz; -struct scsiio_tracker *scsi_lookup; -ulong scsi_lookup_pages;spinlock_t scsi_lookup_lock; -struct list_head free_list;int pending_io_count;wait_queue_head_t reset_wq;
@@ -1249,6 +1243,8 @@ u16 mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx, struct scsi_cmnd *scmd);struct scsiio_tracker *mpt3sas_get_st_from_smid (struct MPT3SAS_ADAPTER *ioc,u16 smid); +void mpt3sas_base_clear_st (struct MPT3SAS_ADAPTER *ioc, +struct scsiio_tracker *st);u16 mpt3sas_base_get_smid (struct MPT3SAS_ADAPTER *ioc,u8 cb_idx);void mpt3sas_base_free_smid (struct MPT3SAS_ADAPTER *ioc,u16 smid);
@@ -1459,8 +1455,7 @@ void mpt3sas_init_warpdrive_properties(struct MPT3SAS_ADAPTER *ioc, mpt3sas_scsi_direct_io_set(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 direct_io);void mpt3sas_setup_direct_io (struct MPT3SAS_ADAPTER *ioc,struct scsi_cmnd *scmd, -struct _raid_device *raid_device,Mpi2SCSIIORequest_t *mpi_request, -u16 smid); +struct _raid_device *raid_device,Mpi2SCSIIORequest_t *mpi_request);/* NCQ Prio Handling Check */ bool scsih_ncq_prio_supp (struct scsi_device *sdev);
drivers/scsi/mpt3sas/mpt3sas_ctl.c +59 -26
117 lines not on this page · raw message
drivers/scsi/mpt3sas/mpt3sas_scsih.c +210 -250
620 lines not on this page · raw message
drivers/scsi/mpt3sas/mpt3sas_warpdrive.c +3 -17
@@ -276,20 +276,6 @@ }/** -* mpt3sas_scsi_direct_io_set - sets direct io flag -* @ioc: per adapter object -* @smid: system request message index -* @direct_io: Zero or non-zero value to set in the direct_io flag -* -* Returns Nothing. -*/ -inline void -mpt3sas_scsi_direct_io_set (struct MPT3SAS_ADAPTER *ioc,u16 smid,u8 direct_io) -{ - ioc->scsi_lookup[smid -1 ].direct_io = direct_io; -} - -/** * mpt3sas_setup_direct_io - setup MPI request for WARPDRIVE Direct I/O * @ioc: per adapter object * @scmd: pointer to scsi command object
@@ -301,12 +287,12 @@*/ void mpt3sas_setup_direct_io (struct MPT3SAS_ADAPTER *ioc,struct scsi_cmnd *scmd, -struct _raid_device *raid_device,Mpi2SCSIIORequest_t *mpi_request, -u16 smid) +struct _raid_device *raid_device,Mpi2SCSIIORequest_t *mpi_request) {sector_t v_lba, p_lba, stripe_off, column, io_size;u32 stripe_sz, stripe_exp;u8 num_pds, cmd = scmd->cmnd[0 ]; +struct scsiio_tracker *st =scsi_cmd_priv (scmd);if (cmd !=READ_10 && cmd !=WRITE_10 && cmd !=READ_16 && cmd !=WRITE_16 )
@@ -342,5 +328,5 @@else put_unaligned_be64 (p_lba, &mpi_request->CDB.CDB32[2 ]); -mpt3sas_scsi_direct_io_set (ioc, smid,1 ); + st->direct_io =1 ; }
Signature · 1 line
1.8.5.6
Re: [PATCHv2 09/11] mpt3sas: lockless command submission for scsi-mq
Quoted 47 lines · Hannes Reinecke
Enable lockless command submission for scsi-mq by moving the command structure into the payload for struct request.
Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/mpt3sas/mpt3sas_base.c | 123 ++++----- drivers/scsi/mpt3sas/mpt3sas_base.h | 13 +- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 85 ++++-- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 460 ++++++++++++++----------------- drivers/scsi/mpt3sas/mpt3sas_warpdrive.c | 20 +- 5 files changed, 330 insertions(+), 371 deletions(-)drivers/scsi/mpt3sas/mpt3sas_base.c +9
-6@@ -865,9 +865,17 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)struct scsiio_tracker *mpt3sas_get_st_from_smid (struct MPT3SAS_ADAPTER *ioc,u16 smid) { +u32 unique_tag; +struct scsi_cmnd *cmd; +WARN_ON (!smid);WARN_ON (smid >= ioc->hi_priority_smid); -return &ioc->scsi_lookup[smid -1 ]; + unique_tag = smid -1 ; + cmd =scsi_host_find_tag (ioc->shost, unique_tag); +if (cmd) +return scsi_cmd_priv (cmd); + +return NULL ; }/** @@ -2343,34 +2351,23 @@ struct scsiio_tracker * mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx, struct scsi_cmnd *scmd) { - unsigned long flags; struct scsiio_tracker *request;u16 smid; -spin_lock_irqsave (&ioc->scsi_lookup_lock, flags); -if (list_empty (&ioc->free_list)) { -spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags); - pr_err(MPT3SAS_FMT "%s: smid not available\n",
drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 2359
Quoted 10 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +2
-7- ioc->name, __func__); -return 0 ; - } -if (!scmd) { -/* ioctl command, always use the first slot */ - request = ioc->lookup[0 ]; - request->scmd =NULL ; + smid =1 ; + request =mpt3sas_get_st_from_smid (ioc, smid);
How is this going to work? mpt3sas_get_st_from_smid fails if we don't find a block layer tag derived from smid?
Quoted 7 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +0
-0/** * mpt3sas_base_free_smid - put smid back on free_list * @ioc: per adapter object @@ -2428,22 +2441,21 @@ struct scsiio_tracker *unsigned long flags;int i;
drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 2453
Quoted 10 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +7
-0if (smid < ioc->hi_priority_smid) { +struct scsiio_tracker *st; + st =mpt3sas_get_st_from_smid (ioc, smid); +if (WARN_ON (!st)) { +_base_recovery_check (ioc); +return ; + } +mpt3sas_base_clear_st (ioc, st);_base_recovery_check (ioc);
st = mpt3sas_get_st_from_smid(ioc, smid); if (!WARN_ON_ONCE(!st)) mpt3sas_base_clear_st(ioc, st); _base_recovery_check(ioc); return;
drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 5698
Quoted 8 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +2
-5/* pending command count */ -spin_lock_irqsave (&ioc->scsi_lookup_lock, flags); -for (i =0 ; i < ioc->scsiio_depth; i++) -if (ioc->scsi_lookup[i].cb_idx !=0xFF ) - ioc->pending_io_count++; -spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags); +blk_mq_tagset_busy_iter (&ioc->shost->tag_set, + _count_pending, ioc);
You can't rely on blk-mq being used, and we'd really want to avoid layering violations like this anyway.
Re: [PATCHv2 09/11] mpt3sas: lockless command submission for scsi-mq
Quoted 53 lines · Christoph Hellwig
Quoted 52 lines · Hannes Reinecke
Enable lockless command submission for scsi-mq by moving the command structure into the payload for struct request.
Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/mpt3sas/mpt3sas_base.c | 123 ++++----- drivers/scsi/mpt3sas/mpt3sas_base.h | 13 +- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 85 ++++-- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 460 ++++++++++++++----------------- drivers/scsi/mpt3sas/mpt3sas_warpdrive.c | 20 +- 5 files changed, 330 insertions(+), 371 deletions(-)drivers/scsi/mpt3sas/mpt3sas_base.c +9
-10@@ -865,9 +865,17 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)struct scsiio_tracker *mpt3sas_get_st_from_smid (struct MPT3SAS_ADAPTER *ioc,u16 smid) { +u32 unique_tag; +struct scsi_cmnd *cmd; +WARN_ON (!smid);WARN_ON (smid >= ioc->hi_priority_smid); -return &ioc->scsi_lookup[smid -1 ]; + unique_tag = smid -1 ; + cmd =scsi_host_find_tag (ioc->shost, unique_tag); +if (cmd) +return scsi_cmd_priv (cmd); + +return NULL ; }/** @@ -2343,34 +2351,23 @@ struct scsiio_tracker * mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx, struct scsi_cmnd *scmd) { - unsigned long flags; struct scsiio_tracker *request;u16 smid; -spin_lock_irqsave (&ioc->scsi_lookup_lock, flags); -if (list_empty (&ioc->free_list)) { -spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags); -pr_err (MPT3SAS_FMT "%s: smid not available\n" , - ioc->name, __func__); -return 0 ; - } -if (!scmd) {
Quoted 9 lines
drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 2359
Quoted 5 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +2
-3-/* ioctl command, always use the first slot */ - request = ioc->lookup[0 ]; - request->scmd =NULL ; + smid =1 ; + request =mpt3sas_get_st_from_smid (ioc, smid);How is this going to work?
mpt3sas_get_st_from_smidfails if we don't find a block layer tag derived from smid?
Yes, true; it will fail.
It will be fixed up by using reserved commands in patch 11.
Quoted 12 lines
drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 5698
Quoted 8 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +2
-5/* pending command count */ -spin_lock_irqsave (&ioc->scsi_lookup_lock, flags); -for (i =0 ; i < ioc->scsiio_depth; i++) -if (ioc->scsi_lookup[i].cb_idx !=0xFF ) - ioc->pending_io_count++; -spin_unlock_irqrestore (&ioc->scsi_lookup_lock, flags); +blk_mq_tagset_busy_iter (&ioc->shost->tag_set, + _count_pending, ioc);You can't rely on blk-mq being used, and we'd really want to avoid layering violations like this anyway.
Well, I _did_ enable blk-mq later on, so from that perspective, yes, I can.
But if that's a layering violation, how am I supposed to check this? Would be a wrapper in the scsi midlayer be acceptable?
Or is using blk_mq_tagset_busy_iter considered internal to the block layer?
Cheers,
Hannes
Signature · 5 lines
Dr. Hannes Reinecke Teamlead Storage & Networking [email protected] +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
Re: [PATCHv2 09/11] mpt3sas: lockless command submission for scsi-mq
Quoted 3 lines · Hannes Reinecke
Yes, true; it will fail.
It will be fixed up by using reserved commands in patch 11.
Patches should be fully bisectable. If that is too much work they need to be merged with a good explanation on why it is done.
Quoted 5 lines
Quoted 3 lines
You can't rely on blk-mq being used, and we'd really want to avoid layering violations like this anyway.
Well, I _did_ enable blk-mq later on, so from that perspective, yes, I can.
No you can't. Drivers can not enable blk-mq, it's a global policy setting.
Quoted 3 lines
But if that's a layering violation, how am I supposed to check this? Would be a wrapper in the scsi midlayer be acceptable?
Or is usingblk_mq_tagset_busy_iterconsidered internal to the block layer?
I think the right answer is to figure out what you're actually doing, and if it's a) nessecary at all, b) if it's already implemented in common code and c) if it should be implemented in common code.
PATCH v2 10/11 scsi: allocate reserved commands
In reply to Hannes ReineckeThe block layer already has the notion of 'reserved' commands, so we should be enabling hosts to allocate them.
--- drivers/scsi/scsi_lib.c | 1 + include/scsi/scsi_host.h | 1 + 2 files changed, 2 insertions(+)
drivers/scsi/scsi_lib.c +1 -0
@@ -2129,6 +2129,7 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost) shost->tag_set.ops = &scsi_mq_ops; shost->tag_set.nr_hw_queues = shost->nr_hw_queues ? :1 ; shost->tag_set.queue_depth = shost->can_queue; + shost->tag_set.reserved_tags = shost->reserved_cmds; shost->tag_set.cmd_size = cmd_size; shost->tag_set.numa_node =NUMA_NO_NODE ; shost->tag_set.flags =BLK_MQ_F_SHOULD_MERGE |BLK_MQ_F_SG_MERGE ;
include/scsi/scsi_host.h +1 -0
@@ -622,6 +622,7 @@ struct Scsi_Host {int this_id;int can_queue; +int reserved_cmds;short cmd_per_lun;short unsigned int sg_tablesize;short unsigned int sg_prot_tablesize;
Signature · 1 line
1.8.5.6
Re: [PATCHv2 10/11] scsi: allocate reserved commands
Quoted 2 lines · Hannes Reinecke
The block layer already has the notion of 'reserved' commands, so we should be enabling hosts to allocate them.
How does this interact with the non blk-mq path?
Re: [PATCHv2 10/11] scsi: allocate reserved commands
Quoted 6 lines · Christoph Hellwig
Quoted 2 lines · Hannes Reinecke
The block layer already has the notion of 'reserved' commands, so we should be enabling hosts to allocate them.
How does this interact with the non blk-mq path?
It doesn't; legacy sq doesn't have the notion of 'reserved' command.
So here's my question: how are 'reserved' commands are imagined to be used? ATM they exist for blk-mq solely, and are not even implemented for legacy sq.
At the same time quite some drivers (like mpt3sas) really could make use of the notion of 'reserved' commands.
Does it make sense to implement 'reserved' commands for the legacy sq code path, too?
I'd be happy to implement something like this; would make life far easier in certain other places, too (fnic springs to mind ...)
Bug if so, would such a patch being accepted?
Any attempts to modify legacy sq has been met with heavy resistance so far ...
Cheers,
Hannes
Signature · 5 lines
Dr. Hannes Reinecke Teamlead Storage & Networking [email protected] +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
Re: [PATCHv2 10/11] scsi: allocate reserved commands
Quoted 3 lines · Hannes Reinecke
So here's my question: how are 'reserved' commands are imagined to be used? ATM they exist for blk-mq solely, and are not even implemented for legacy sq.
Yes.
Quoted 4 lines
At the same time quite some drivers (like mpt3sas) really could make use of the notion of 'reserved' commands.
Does it make sense to implement 'reserved' commands for the legacy sq code path, too?
Yes.
PATCH v2 11/11 mpt3sas: register reserved commands
In reply to Hannes ReineckeThe mpt3sas driver requires a reserved command space to handle SCSI passthrough commands.
--- drivers/scsi/mpt3sas/mpt3sas_base.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/scsi/mpt3sas/mpt3sas_base.c +4 -1
@@ -2360,6 +2360,8 @@ struct scsiio_tracker * }else {u32 unique_tag =blk_mq_unique_tag (scmd->request);u16 tag =blk_mq_unique_tag_to_tag (unique_tag); + +WARN_ON (tag < ioc->shost->reserved_cmds); request =scsi_cmd_priv (scmd); smid = tag +1 ; }
@@ -3521,7 +3523,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,/* set the scsi host can_queue depth * with some internal commands that could be outstanding */ - ioc->shost->can_queue = ioc->scsiio_depth -INTERNAL_SCSIIO_CMDS_COUNT ; + ioc->shost->reserved_cmds =INTERNAL_SCSIIO_CMDS_COUNT ; + ioc->shost->can_queue = ioc->scsiio_depth - ioc->shost->reserved_cmds;dinitprintk (ioc,pr_info (MPT3SAS_FMT "scsi host: can_queue depth (%d)\n" , ioc->name, ioc->shost->can_queue));
Signature · 1 line
1.8.5.6
Re: [PATCHv2 11/11] mpt3sas: register reserved commands
Quoted 22 lines · Hannes Reinecke
The mpt3sas driver requires a reserved command space to handle SCSI passthrough commands.
Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/mpt3sas/mpt3sas_base.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/scsi/mpt3sas/mpt3sas_base.c +2
-0@@ -2360,6 +2360,8 @@ struct scsiio_tracker * }else {u32 unique_tag =blk_mq_unique_tag (scmd->request);u16 tag =blk_mq_unique_tag_to_tag (unique_tag); + +WARN_ON (tag < ioc->shost->reserved_cmds); request =scsi_cmd_priv (scmd); smid = tag +1 ; }
drivers/scsi/mpt3sas/mpt3sas_base.c void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, · line 3527
Quoted 6 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +2
-1@@ -3521,7 +3523,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,/* set the scsi host can_queue depth * with some internal commands that could be outstanding */ - ioc->shost->can_queue = ioc->scsiio_depth -INTERNAL_SCSIIO_CMDS_COUNT ; + ioc->shost->reserved_cmds =INTERNAL_SCSIIO_CMDS_COUNT ; + ioc->shost->can_queue = ioc->scsiio_depth - ioc->shost->reserved_cmds;
You're never allocating a reserved request. Just remove the number of reserved cmds from can_queue and you can use them on your own.
So I don't think you'll actually need to use reserved request here, but instead you should set up can_queue properly earlier in the series.
Re: [PATCHv2 11/11] mpt3sas: register reserved commands
Quoted 33 lines · Christoph Hellwig
Quoted 22 lines · Hannes Reinecke
The mpt3sas driver requires a reserved command space to handle SCSI passthrough commands.
Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/mpt3sas/mpt3sas_base.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/scsi/mpt3sas/mpt3sas_base.c +2
-0@@ -2360,6 +2360,8 @@ struct scsiio_tracker * }else {u32 unique_tag =blk_mq_unique_tag (scmd->request);u16 tag =blk_mq_unique_tag_to_tag (unique_tag); + +WARN_ON (tag < ioc->shost->reserved_cmds); request =scsi_cmd_priv (scmd); smid = tag +1 ; }drivers/scsi/mpt3sas/mpt3sas_base.c void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, · line 3527
Quoted 6 lines
drivers/scsi/mpt3sas/mpt3sas_base.c +2
-1@@ -3521,7 +3523,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,/* set the scsi host can_queue depth * with some internal commands that could be outstanding */ - ioc->shost->can_queue = ioc->scsiio_depth -INTERNAL_SCSIIO_CMDS_COUNT ; + ioc->shost->reserved_cmds =INTERNAL_SCSIIO_CMDS_COUNT ; + ioc->shost->can_queue = ioc->scsiio_depth - ioc->shost->reserved_cmds;You're never allocating a reserved request. Just remove the number of reserved cmds from
can_queueand you can use them on your own.
Quoted 3 lines
So I don't think you'll actually need to use reserved request here, but instead you should set up
can_queueproperly earlier in the series.
Well, once I'm switching over to embedded commands I'll have to integrate with SCSI midlayer to actually _allocate_ the tracker structure for me.
At the same time the midlayer should _not_ use that structure for normal I/O; which sounds suspiciously like reserved commands to me ... Oh well; guess I'll have to do it, then.
Cheers,
Hannes
Signature · 5 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +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: [PATCHv2 11/11] mpt3sas: register reserved commands
Quoted 5 lines · Hannes Reinecke
Well, once I'm switching over to embedded commands I'll have to integrate with SCSI midlayer to actually _allocate_ the tracker structure for me.
At the same time the midlayer should _not_ use that structure for normal I/O; which sounds suspiciously like reserved commands to me ...
Let's defer the issue until that code is actually introduced. I think having reserved tag support even for !mq will be useful for the transition period, so I'm not against it. But as far as this series is concerned it's not actually needed, or to be more blunt incorrectly used.