Re: [PATCH v4 00/11] Zoned block device support improvements
On Wed, 2018-10-24 at 11:37 -0400, Martin K. Petersen wrote: > Mike, > > > > You keep mentioning this, but I don't recall ever seeing anything to > > > that effect. The rest of the kernel appears to be either arbitrary > > > ordering or favoring author SoB as the first tag. > > > > I've always felt the proper order is how Jens likes it too (all dm > > commits from me follow that order). > > That's fine, I don't have any particular preference. And I don't have > any issue with you guys sticking to a certain ordering in your > respective subsystems. I occasionally shuffle tags when I commit things > in SCSI too. > > I just think it should be properly documented if there is a preferred > way to order things... When I tried to look up documentation for this I couldn't find anything under the Documentation directory. Maybe it's there but I didn't look carefully enough. All I could find on the web is e-mails from Linus in which he explains that the order of Signed-off-by's should match the chain of authorship. Bart.
[PATCH V2] Timeouts occur on surprise removal of QLogic adapter
When doing a surprise removal of an adapter, some in flight I/Os can get
stuck and take a while to complete (they actually timeout and are
retried). We are not handling an early error exit from
qla2xxx_eh_abort properly.
Fixes: 45235022da99 ("scsi: qla2xxx: Fix driver unload by shutting down chip")
Signed-off-by: Bill Kuzeja
---
Note: (Reworked ACKed patch to cleanly apply to 4.20/scsi-queue as it
stands today - please ignore patch I sent yesterday).
Patch explanation:
After a hot remove of a Qlogic adapter, the driver's remove function gets
called and we end up aborting all in progress I/Os. Here is the code flow:
qla2x00_remove_one
qla2x00_abort_isp_cleanup
qla2x00_abort_all_cmds
__qla2x00_abort_all_cmds
qla2xxx_eh_abort
At the start of qla2xxx_eh_abort, some sanity checks are done before
actually sending the abort. One of these checks is a call to
fc_block_scsi_eh. In the case of a hot remove, it turns out that this
routine can exit with FAST_IO_FAIL.
When this occurs, we return back to __qla2x00_abort_all_cmds with an
extra reference on sp (because the abort never gets sent). Originally,
this was addressed with another fix:
commit 4cd3b6ebff85 scsi: qla2xxx: Fix extraneous ref on sp's after adapter
break
But this later this added change complicated matters:
commit 45235022da99 scsi: qla2xxx: Fix driver unload by shutting down chip
Because the abort is now being done earlier in the teardown (through
qla2x00_abort_isp_cleanup), in qla2xxx_eh_abort we make it past
the first check because qla2x00_isp_reg_stat(ha) returns zero. When we
fail a few lines later in fc_block_scsi_eh, this error is not handled
properly in __qla2x00_abort_all_cmds and the I/O ends up hanging and
timing out because of the extra reference.
For this fix, a check for FAST_IO_FAIL is added to
__qla2x00_abort_all_cmds where we check to see if qla2xxx_eh_abort
succeeded or not.
This removes the extra reference in this additional early exit case. In
my testing (hw surprise removals and also adapter remove via sysfs), this
eliminates the timeouts and delays and the remove proceeds smoothly.
---
drivers/scsi/qla2xxx/qla_os.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 01607d2..8ee05d9 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -1749,7 +1749,7 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data *ha)
static void
__qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
{
- int cnt;
+ int cnt, status;
unsigned long flags;
srb_t *sp;
scsi_qla_host_t *vha = qp->vha;
@@ -1799,10 +1799,16 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data *ha)
if (!sp_get(sp)) {
spin_unlock_irqrestore
(qp->qp_lock_ptr,
flags);
- qla2xxx_eh_abort(
+ status = qla2xxx_eh_abort(
GET_CMD_SP(sp));
spin_lock_irqsave
(qp->qp_lock_ptr,
flags);
+ /*
+* Get rid of extra reference
caused
+* by early exit from
qla2xxx_eh_abort
+*/
+ if (status == FAST_IO_FAIL)
+
atomic_dec(&sp->ref_count);
}
}
sp->done(sp, res);
--
1.8.3.1
Re: [PATCH 2/8] sg: introduce sg_log macro
Hello Doug, > Examples please, preferably from the SCSI subsystem. If not, I'll do what > most other drivers do, drop all debug statements. $ git grep trace drivers/scsi/scsi* and $ less include/trace/events/scsi.h We currently only have trace points for command dispatch, completion, and a few error scenarios. As of yet there is no explicit tracing in the ULDs. Mostly because so far it hasn't been needed since the ML tracing will parse CDBs and dump most useful fields. And the ULDs generally have no state of their own above what's in the scsi_cmnd. But if you have a specific set of things that would be useful to inspect in the sg state, by all means introduce trace points for them. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v4 00/11] Zoned block device support improvements
Mike, >> You keep mentioning this, but I don't recall ever seeing anything to >> that effect. The rest of the kernel appears to be either arbitrary >> ordering or favoring author SoB as the first tag. > > I've always felt the proper order is how Jens likes it too (all dm > commits from me follow that order). That's fine, I don't have any particular preference. And I don't have any issue with you guys sticking to a certain ordering in your respective subsystems. I occasionally shuffle tags when I commit things in SCSI too. I just think it should be properly documented if there is a preferred way to order things... -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v4 00/11] Zoned block device support improvements
On Tue, Oct 23 2018 at 10:26pm -0400, Martin K. Petersen wrote: > > Jens, > > > 2) The ordering of the signed-off-by. Someone told me that this is > >patchwork, but I absolutely hate it. SOB should go last, not before > >the reviewed-by. I fixed that up too. > > You keep mentioning this, but I don't recall ever seeing anything to > that effect. The rest of the kernel appears to be either arbitrary > ordering or favoring author SoB as the first tag. I've always felt the proper order is how Jens likes it too (all dm commits from me follow that order). Mike
Re: [PATCH] bsg: convert to use blk-mq
On 10/24/18 6:02 AM, Jens Axboe wrote: > On 10/24/18 5:52 AM, Jens Axboe wrote: >> On 10/24/18 5:30 AM, Jens Axboe wrote: >>> On 10/24/18 5:19 AM, Christoph Hellwig wrote: On Mon, Oct 22, 2018 at 03:23:30AM -0600, Jens Axboe wrote: > JFYI, I also reordered the series to make it correct. You can apply > this one: > > http://git.kernel.dk/cgit/linux-block/commit/?h=mq-conversions&id=2b2ffa16193e9a69a076595ed64429b8cc9b42aa > > before the bsg patch, and it should be fine. Or just use the above branch, > of course. Hell no on that one. The behavior of having methods right on the request_queue which can be changed any time is something we absolutely must not introduce into blk-mq. >>> >>> I agree it's not the prettiest, but it's not like it's something >>> that is changed at runtime. >>> Just add pass a timeout hander to bsg_register_queue which is called from the bsg ->timeout handler is a much better way to sort our your problem. It can also easily be turned into an independent prep patch. >>> >>> Good idea, that is cleaner. >> >> Except that STILL doesn't work with mq_ops being a constant, I'd have >> to allocate it. > > Which obviously won't work. I don't see a good way out of this, since > I can't store the private timeout handler anywhere. Open to suggestions, > but until something better comes up, I'm keeping q->timeout and > removing the API to set it. bsg-lib can just manually set it in the > queue. Pushed that out - it's now ->bsg_job_timeout_fn, and nobody sets it but bsg when it initializes the queue. bsg sets up a default timeout handler, and calls ->bsg_job_timeout_fn, if defined. Not that that is any different than from before, but at least it's obvious what it's for now. -- Jens Axboe
Re: [PATCH] bsg: convert to use blk-mq
On 10/24/18 5:52 AM, Jens Axboe wrote: > On 10/24/18 5:30 AM, Jens Axboe wrote: >> On 10/24/18 5:19 AM, Christoph Hellwig wrote: >>> On Mon, Oct 22, 2018 at 03:23:30AM -0600, Jens Axboe wrote: JFYI, I also reordered the series to make it correct. You can apply this one: http://git.kernel.dk/cgit/linux-block/commit/?h=mq-conversions&id=2b2ffa16193e9a69a076595ed64429b8cc9b42aa before the bsg patch, and it should be fine. Or just use the above branch, of course. >>> >>> Hell no on that one. The behavior of having methods right on the >>> request_queue which can be changed any time is something we absolutely >>> must not introduce into blk-mq. >> >> I agree it's not the prettiest, but it's not like it's something >> that is changed at runtime. >> >>> Just add pass a timeout hander to bsg_register_queue which is called >>> from the bsg ->timeout handler is a much better way to sort our your >>> problem. It can also easily be turned into an independent prep patch. >> >> Good idea, that is cleaner. > > Except that STILL doesn't work with mq_ops being a constant, I'd have > to allocate it. Which obviously won't work. I don't see a good way out of this, since I can't store the private timeout handler anywhere. Open to suggestions, but until something better comes up, I'm keeping q->timeout and removing the API to set it. bsg-lib can just manually set it in the queue. -- Jens Axboe
Re: [PATCH] bsg: convert to use blk-mq
On 10/24/18 5:30 AM, Jens Axboe wrote: > On 10/24/18 5:19 AM, Christoph Hellwig wrote: >> On Mon, Oct 22, 2018 at 03:23:30AM -0600, Jens Axboe wrote: >>> JFYI, I also reordered the series to make it correct. You can apply >>> this one: >>> >>> http://git.kernel.dk/cgit/linux-block/commit/?h=mq-conversions&id=2b2ffa16193e9a69a076595ed64429b8cc9b42aa >>> >>> before the bsg patch, and it should be fine. Or just use the above branch, >>> of course. >> >> Hell no on that one. The behavior of having methods right on the >> request_queue which can be changed any time is something we absolutely >> must not introduce into blk-mq. > > I agree it's not the prettiest, but it's not like it's something > that is changed at runtime. > >> Just add pass a timeout hander to bsg_register_queue which is called >> from the bsg ->timeout handler is a much better way to sort our your >> problem. It can also easily be turned into an independent prep patch. > > Good idea, that is cleaner. Except that STILL doesn't work with mq_ops being a constant, I'd have to allocate it. -- Jens Axboe
Re: [PATCH] bsg: convert to use blk-mq
On 10/24/18 5:19 AM, Christoph Hellwig wrote: > On Mon, Oct 22, 2018 at 03:23:30AM -0600, Jens Axboe wrote: >> JFYI, I also reordered the series to make it correct. You can apply >> this one: >> >> http://git.kernel.dk/cgit/linux-block/commit/?h=mq-conversions&id=2b2ffa16193e9a69a076595ed64429b8cc9b42aa >> >> before the bsg patch, and it should be fine. Or just use the above branch, >> of course. > > Hell no on that one. The behavior of having methods right on the > request_queue which can be changed any time is something we absolutely > must not introduce into blk-mq. I agree it's not the prettiest, but it's not like it's something that is changed at runtime. > Just add pass a timeout hander to bsg_register_queue which is called > from the bsg ->timeout handler is a much better way to sort our your > problem. It can also easily be turned into an independent prep patch. Good idea, that is cleaner. -- Jens Axboe
Re: [PATCH] bsg: convert to use blk-mq
On Mon, Oct 22, 2018 at 03:23:30AM -0600, Jens Axboe wrote: > JFYI, I also reordered the series to make it correct. You can apply > this one: > > http://git.kernel.dk/cgit/linux-block/commit/?h=mq-conversions&id=2b2ffa16193e9a69a076595ed64429b8cc9b42aa > > before the bsg patch, and it should be fine. Or just use the above branch, > of course. Hell no on that one. The behavior of having methods right on the request_queue which can be changed any time is something we absolutely must not introduce into blk-mq. Just add pass a timeout hander to bsg_register_queue which is called from the bsg ->timeout handler is a much better way to sort our your problem. It can also easily be turned into an independent prep patch.
[GIT PULL] first round of SCSI updates for the 4.19+ merge window
This is mostly updates of the usual drivers: UFS, esp_scsi, NCR5380, qla2xxx, lpfc, libsas, hisi_sas. In addition there's a set of mostly small updates to the target subsystem a set of conversions to the generic DMA API, which do have some potential for issues in the older drivers but we'll handle those as case by case fixes. A new myrs for the DAC960/mylex raid controllers to replace the block based DAC960 which is also being removed by Jens in this merge window. Plus the usual slew of trivial changes. The patch is available here: git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git scsi-misc The short changelog is: Arnd Bergmann (2): scsi: ufs: fix integer type usage in uapi header scsi: raid_attrs: fix unused variable warning Avri Altman (6): scsi: ufs-bsg: Add support for uic commands in ufs_bsg_request() scsi: ufs-bsg: Add support for raw upiu in ufs_bsg_request() scsi: ufs: Add API to execute raw upiu commands scsi: ufs: Use data structure size in pointer arithmetic scsi: ufs: Add a bsg endpoint that supports UPIUs scsi: uapi: ufs: Make utp_upiu_req visible to user space Bart Van Assche (6): scsi: target/core: Always call transport_complete_callback() upon failure scsi: target/core: Use sg_alloc_table() instead of open-coding it scsi: target/core: Use the SECTOR_SHIFT constant scsi: target/core: Remove the SCF_COMPARE_AND_WRITE_POST flag scsi: target/core: Remove an unused data member from struct xcopy_pt_cmd scsi: target/core: Fix spelling in two source code comments Bryant G. Ly (1): scsi: ibmvscsi_tgt: Remove target_wait_for_sess_cmd() Chengguang Xu (1): scsi: libfc: remove unnecessary condition check Christoph Hellwig (42): scsi: mesh: switch to generic DMA API scsi: ips: switch to generic DMA API scsi: smartpqi: fully convert to the generic DMA API scsi: vmw_pscsi: switch to generic DMA API scsi: snic: switch to generic DMA API scsi: qla4xxx: fully convert to the generic DMA API scsi: qla2xxx: fully convert to the generic DMA API scsi: qla1280: switch to generic DMA API scsi: qedi: fully convert to the generic DMA API scsi: qedf: fully convert to the generic DMA API scsi: pm8001: switch to generic DMA API scsi: nsp32: switch to generic DMA API scsi: mvsas: fully convert to the generic DMA API scsi: mvumi: switch to generic DMA API scsi: mpt3sas: switch to generic DMA API scsi: megaraid_sas: switch to generic DMA API scsi: megaraid_mbox: switch to generic DMA API scsi: hpsa: switch to generic DMA API scsi: fnic: switch to generic DMA API scsi: csiostor: switch to generic DMA API scsi: be2iscsi: switch to generic DMA API scsi: atp870u: switch to generic DMA API scsi: a100u2w: switch to generic DMA API scsi: BusLogic: switch to generic DMA API scsi: 3w-sas: fully convert to the generic DMA API scsi: 3w-xxx: fully convert to the generic DMA API scsi: 3w-9xxx: fully convert to the generic DMA API scsi: aic94xx: fully convert to the generic DMA API scsi: dc395x: use generic DMA API scsi: dc395x: fix DMA API usage in sg_update_list scsi: dc395x: fix dma API usage in srb_done scsi: dc395x: simplify list handling scsi: esp_scsi: remove union in esp_cmd_priv scsi: esp_scsi: move dma mapping into the core code scsi: esp_scsi: remove the dev argument to scsi_esp_register scsi: esp_scsi: use strong typing for the dev field scsi: sun_esp: don't use GFP_ATOMIC for command block allocation scsi: am53c974: use the generic DMA API scsi: lpfc: remove a bogus pci_dma_sync_single_for_device call scsi: megaraid_mbox: remove bogus use of pci_dma_sync_sg_* APIs scsi: ufs: add a low-level __ufshcd_issue_tm_cmd helper scsi: ufs: cleanup struct utp_task_req_desc Colin Ian King (11): scsi: be2iscsi: fix spelling mistake "Retreiving" -> "Retrieving" scsi: lpfc: fix spelling mistake "Resrouce" -> "Resource" scsi: arcmsr: clean up clang warning on extraneous parentheses scsi: qla4xxx: Remove redundant check on drvr_wait scsi: megaraid: fix spelling mistake "maibox" -> "mailbox" scsi: iscsi: target: fix spelling mistake "entires" -> "entries" scsi: message: fusion: fix a few trivial spelling mistakes scsi: qla2xxx: fix typo "CT-PASSTRHU" -> "CT-PASSTHRU" scsi: aic7xxx: remove unused redundant variable num_chip_names scsi: be2iscsi: remove unused variable dmsg scsi: aacraid: remove unused variables dev and cpu Dan Carpenter (4): scsi: myrs: Fix the processor absent message in processor_show() scsi: myrs: Fix a logical vs bitwise bug scsi: qla2xxx: don't allow negative thresholds scsi: lpfc: remove an unnecessary NULL check Darren Trapp (1): scsi: qla2xxx: Allow FC-NVMe und
Re: [PATCH 2/8] sg: introduce sg_log macro
On 2018-10-24 3:58 a.m., Martin K. Petersen wrote: Hi Doug, I'll follow what the scsi mid-level and the other ULDs do. IOW, no change. The debug messages they produce are quite helpful (to me, I use them a lot, and Tony B. has asked for more precision) and well-tuned to the SCSI subsystem (e.g. telling us what sdp represents in useful terms). And they can be compiled out (but not my pr_info above, probably should be a pr_warn). I agree with Johannes. SCSI logging is in sustaining mode. We're trying to remove it, not to add to it. The kernel has much more capable and flexible methods of getting information out to the user these days. No need to resort to arcane logging masks and the like. Examples please, preferably from the SCSI subsystem. If not, I'll do what most other drivers do, drop all debug statements. Doug Gilbert
Re: [PATCH v4 00/11] Zoned block device support improvements
Jens, On 2018/10/23 16:59, Jens Axboe wrote: > On 10/12/18 4:08 AM, Damien Le Moal wrote: >> This series improves zoned block device support (reduce overhead) and >> introduces many simplifications to the code (overall, there are more >> deletions >> than insertions). >> >> In more details: >> * Patches 1 to 3 are SCSI side (sd driver) cleanups and improvements reducing >> the overhead of report zones command execution during disk scan and >> revalidation. >> * Patches 4 to 9 improve the useability and user API of zoned block devices. >> * Patch 10 is the main part of this series. This patch replaces the >> REQ_OP_ZONE_REPORT BIO/request operation for executing report zones >> commands >> with a block device file operation, removing the need for the command reply >> payload in-place rewriting in the BIO buffer. This leads to major >> simplification of the code in many places. >> * Patch 11 further simplifies the code of low level drivers by providing a >> generic implementation of zoned block device reuest queue zone bitmaps >> initialization and revalidation. > > I've applied this, but I have two complaints: > > 1) Two had to be hand applied, it wasn't against the block tree. My apologies for that. I had to rebase against the latest RC because of a conflict with late fixes in DM. I should have checked more carefully against the block tree after that. Sorry about this, I will be more careful going forward. > 2) The ordering of the signed-off-by. Someone told me that this is >patchwork, but I absolutely hate it. SOB should go last, not before >the reviewed-by. I fixed that up too. Thank you. I think this was me rather than patchwork when I a manually added the reviewed-by tags with new versions of the series. I will remember this and tag in you preferred order. Thank you. Best regards. -- Damien Le Moal Western Digital Research
[PATCH] NCR5380: Return false instead of NULL
I overlooked this statement when I recently converted the function result type from struct scsi_cmnd * to bool. No change to object code. Signed-off-by: Finn Thain --- drivers/scsi/NCR5380.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c index 8429c855701f..01c23d27f290 100644 --- a/drivers/scsi/NCR5380.c +++ b/drivers/scsi/NCR5380.c @@ -1198,7 +1198,7 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd) out: if (!hostdata->selecting) - return NULL; + return false; hostdata->selecting = NULL; return ret; } -- 2.18.1
[bug report] SCSI/libiscsi: Add check_protection callback for transports
Hello Sagi Grimberg,
The patch 55e51eda4820: "SCSI/libiscsi: Add check_protection callback
for transports" from Mar 5, 2014, leads to the following static
checker warning:
drivers/scsi/libiscsi.c:865 iscsi_scsi_cmd_rsp()
error: uninitialized symbol 'sector'.
drivers/scsi/libiscsi.c
833 static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn, struct
iscsi_hdr *hdr,
834 struct iscsi_task *task, char *data,
835 int datalen)
836 {
837 struct iscsi_scsi_rsp *rhdr = (struct iscsi_scsi_rsp *)hdr;
838 struct iscsi_session *session = conn->session;
839 struct scsi_cmnd *sc = task->sc;
840
841 iscsi_update_cmdsn(session, (struct iscsi_nopin*)rhdr);
842 conn->exp_statsn = be32_to_cpu(rhdr->statsn) + 1;
843
844 sc->result = (DID_OK << 16) | rhdr->cmd_status;
845
846 if (task->protected) {
847 sector_t sector;
^^^
848 u8 ascq;
849
850 /**
851 * Transports that didn't implement check_protection
852 * callback but still published T10-PI support to
scsi-mid
853 * deserve this BUG_ON.
854 **/
855 BUG_ON(!session->tt->check_protection);
856
857 ascq = session->tt->check_protection(task, §or);
^^^
This returns 0x1 - 0x3 on error. Some error paths set sector but some
don't.
858 if (ascq) {
859 sc->result = DRIVER_SENSE << 24 |
860 SAM_STAT_CHECK_CONDITION;
861 scsi_build_sense_buffer(1, sc->sense_buffer,
862 ILLEGAL_REQUEST, 0x10,
ascq);
863 scsi_set_sense_information(sc->sense_buffer,
864
SCSI_SENSE_BUFFERSIZE,
865 sector);
866 goto out;
867 }
868 }
The error check_protection() function ends up calling
iser_check_task_pi_status()
drivers/infiniband/ulp/iser/iser_verbs.c
1112 u8 iser_check_task_pi_status(struct iscsi_iser_task *iser_task,
1113 enum iser_data_dir cmd_dir, sector_t
*sector)
1114 {
1115 struct iser_mem_reg *reg = &iser_task->rdma_reg[cmd_dir];
1116 struct iser_fr_desc *desc = reg->mem_h;
1117 unsigned long sector_size = iser_task->sc->device->sector_size;
1118 struct ib_mr_status mr_status;
1119 int ret;
1120
1121 if (desc && desc->pi_ctx->sig_protected) {
1122 desc->pi_ctx->sig_protected = 0;
1123 ret = ib_check_mr_status(desc->pi_ctx->sig_mr,
1124 IB_MR_CHECK_SIG_STATUS,
&mr_status);
1125 if (ret) {
1126 pr_err("ib_check_mr_status failed, ret %d\n",
ret);
1127 goto err;
"sector" not set on this error path.
1128 }
1129
1130 if (mr_status.fail_status & IB_MR_CHECK_SIG_STATUS) {
1131 sector_t sector_off =
mr_status.sig_err.sig_err_offset;
1132
1133 sector_div(sector_off, sector_size + 8);
1134 *sector = scsi_get_lba(iser_task->sc) +
sector_off;
1135
1136 pr_err("PI error found type %d at sector %llx "
1137 "expected %x vs actual %x\n",
1138 mr_status.sig_err.err_type,
1139 (unsigned long long)*sector,
1140 mr_status.sig_err.expected,
1141 mr_status.sig_err.actual);
1142
1143 switch (mr_status.sig_err.err_type) {
1144 case IB_SIG_BAD_GUARD:
1145 return 0x1;
1146 case IB_SIG_BAD_REFTAG:
1147 return 0x3;
1148 case IB_SIG_BAD_APPTAG:
1149 return 0x2;
1150 }
1151 }
1152 }
1153
1154 return 0;
1155 err:
1156 /* Not alot we can do here, return ambiguous guard error */
1157 return 0x1;
^^
1158 }
regards,
dan carpenter
