| Message ID | 20220208172514.3481-23-bvanassche@acm.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove the SCSI pointer from struct scsi_cmnd | expand |
On 2/8/22 18:24, Bart Van Assche wrote: > Instead of storing the iSCSI task pointer and the session age in the SCSI > pointer, use command-private variables. This patch prepares for removal of > the SCSI pointer from struct scsi_cmnd. > > The list of iSCSI drivers has been obtained as follows: > $ git grep -lw iscsi_host_alloc > drivers/infiniband/ulp/iser/iscsi_iser.c > drivers/scsi/be2iscsi/be_main.c > drivers/scsi/bnx2i/bnx2i_iscsi.c > drivers/scsi/cxgbi/libcxgbi.c > drivers/scsi/iscsi_tcp.c > drivers/scsi/libiscsi.c > drivers/scsi/qedi/qedi_main.c > drivers/scsi/qla4xxx/ql4_os.c > include/scsi/libiscsi.h > > Note: it is not clear to me how the qla4xxx driver can work without this > patch since it uses the scsi_cmnd::SCp.ptr member for two different > purposes: > - The qla4xxx driver uses this member to store a struct srb pointer. > - libiscsi uses this member to store a struct iscsi_task pointer. > > Cc: Lee Duncan <lduncan@suse.com> > Cc: Chris Leech <cleech@redhat.com> > Cc: Sagi Grimberg <sagi@grimberg.me> > Cc: Nilesh Javali <njavali@marvell.com> > Cc: Manish Rangankar <mrangankar@marvell.com> > Cc: Karen Xie <kxie@chelsio.com> > Cc: Ketan Mukadam <ketan.mukadam@broadcom.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/infiniband/ulp/iser/iscsi_iser.c | 1 + > drivers/scsi/be2iscsi/be_main.c | 3 ++- > drivers/scsi/bnx2i/bnx2i_iscsi.c | 1 + > drivers/scsi/cxgbi/cxgb3i/cxgb3i.c | 1 + > drivers/scsi/cxgbi/cxgb4i/cxgb4i.c | 1 + > drivers/scsi/iscsi_tcp.c | 1 + > drivers/scsi/libiscsi.c | 22 ++++++++++++---------- > drivers/scsi/qedi/qedi_fw.c | 2 +- > drivers/scsi/qedi/qedi_iscsi.c | 1 + > drivers/scsi/qla4xxx/ql4_def.h | 13 ++++++++++--- > drivers/scsi/qla4xxx/ql4_os.c | 13 +++++++------ > include/scsi/libiscsi.h | 12 ++++++++++++ > 12 files changed, 50 insertions(+), 21 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
> On Feb 8, 2022, at 9:24 AM, Bart Van Assche <bvanassche@acm.org> wrote: > > Instead of storing the iSCSI task pointer and the session age in the SCSI > pointer, use command-private variables. This patch prepares for removal of > the SCSI pointer from struct scsi_cmnd. > > The list of iSCSI drivers has been obtained as follows: > $ git grep -lw iscsi_host_alloc > drivers/infiniband/ulp/iser/iscsi_iser.c > drivers/scsi/be2iscsi/be_main.c > drivers/scsi/bnx2i/bnx2i_iscsi.c > drivers/scsi/cxgbi/libcxgbi.c > drivers/scsi/iscsi_tcp.c > drivers/scsi/libiscsi.c > drivers/scsi/qedi/qedi_main.c > drivers/scsi/qla4xxx/ql4_os.c > include/scsi/libiscsi.h > > Note: it is not clear to me how the qla4xxx driver can work without this > patch since it uses the scsi_cmnd::SCp.ptr member for two different > purposes: > - The qla4xxx driver uses this member to store a struct srb pointer. > - libiscsi uses this member to store a struct iscsi_task pointer. > > Cc: Lee Duncan <lduncan@suse.com> > Cc: Chris Leech <cleech@redhat.com> > Cc: Sagi Grimberg <sagi@grimberg.me> > Cc: Nilesh Javali <njavali@marvell.com> > Cc: Manish Rangankar <mrangankar@marvell.com> > Cc: Karen Xie <kxie@chelsio.com> > Cc: Ketan Mukadam <ketan.mukadam@broadcom.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/infiniband/ulp/iser/iscsi_iser.c | 1 + > drivers/scsi/be2iscsi/be_main.c | 3 ++- > drivers/scsi/bnx2i/bnx2i_iscsi.c | 1 + > drivers/scsi/cxgbi/cxgb3i/cxgb3i.c | 1 + > drivers/scsi/cxgbi/cxgb4i/cxgb4i.c | 1 + > drivers/scsi/iscsi_tcp.c | 1 + > drivers/scsi/libiscsi.c | 22 ++++++++++++---------- > drivers/scsi/qedi/qedi_fw.c | 2 +- > drivers/scsi/qedi/qedi_iscsi.c | 1 + > drivers/scsi/qla4xxx/ql4_def.h | 13 ++++++++++--- > drivers/scsi/qla4xxx/ql4_os.c | 13 +++++++------ > include/scsi/libiscsi.h | 12 ++++++++++++ > 12 files changed, 50 insertions(+), 21 deletions(-) > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c > index 07e47021a71f..f8d0bab4424c 100644 > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > @@ -971,6 +971,7 @@ static struct scsi_host_template iscsi_iser_sht = { > .proc_name = "iscsi_iser", > .this_id = -1, > .track_queue_depth = 1, > + .cmd_size = sizeof(struct iscsi_cmd), > }; > > static struct iscsi_transport iscsi_iser_transport = { > diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c > index ab55681145f8..3bb0adefbe06 100644 > --- a/drivers/scsi/be2iscsi/be_main.c > +++ b/drivers/scsi/be2iscsi/be_main.c > @@ -218,7 +218,7 @@ static char const *cqe_desc[] = { > > static int beiscsi_eh_abort(struct scsi_cmnd *sc) > { > - struct iscsi_task *abrt_task = (struct iscsi_task *)sc->SCp.ptr; > + struct iscsi_task *abrt_task = iscsi_cmd(sc)->task; > struct iscsi_cls_session *cls_session; > struct beiscsi_io_task *abrt_io_task; > struct beiscsi_conn *beiscsi_conn; > @@ -403,6 +403,7 @@ static struct scsi_host_template beiscsi_sht = { > .cmd_per_lun = BEISCSI_CMD_PER_LUN, > .vendor_id = SCSI_NL_VID_TYPE_PCI | BE_VENDOR_ID, > .track_queue_depth = 1, > + .cmd_size = sizeof(struct iscsi_cmd), > }; > > static struct scsi_transport_template *beiscsi_scsi_transport; > diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c > index e21b053b4f3e..fe86fd61a995 100644 > --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c > +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c > @@ -2268,6 +2268,7 @@ static struct scsi_host_template bnx2i_host_template = { > .sg_tablesize = ISCSI_MAX_BDS_PER_CMD, > .shost_groups = bnx2i_dev_groups, > .track_queue_depth = 1, > + .cmd_size = sizeof(struct iscsi_cmd), > }; > > struct iscsi_transport bnx2i_iscsi_transport = { > diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c > index f949a4e00783..ff9d4287937a 100644 > --- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c > +++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c > @@ -98,6 +98,7 @@ static struct scsi_host_template cxgb3i_host_template = { > .dma_boundary = PAGE_SIZE - 1, > .this_id = -1, > .track_queue_depth = 1, > + .cmd_size = sizeof(struct iscsi_cmd), > }; > > static struct iscsi_transport cxgb3i_iscsi_transport = { > diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c > index efb3e2b3398e..53d91bf9c12a 100644 > --- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c > +++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c > @@ -116,6 +116,7 @@ static struct scsi_host_template cxgb4i_host_template = { > .dma_boundary = PAGE_SIZE - 1, > .this_id = -1, > .track_queue_depth = 1, > + .cmd_size = sizeof(struct iscsi_cmd), > }; > > static struct iscsi_transport cxgb4i_iscsi_transport = { > diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c > index 1bc37593c88f..9fee70d6434a 100644 > --- a/drivers/scsi/iscsi_tcp.c > +++ b/drivers/scsi/iscsi_tcp.c > @@ -1007,6 +1007,7 @@ static struct scsi_host_template iscsi_sw_tcp_sht = { > .proc_name = "iscsi_tcp", > .this_id = -1, > .track_queue_depth = 1, > + .cmd_size = sizeof(struct iscsi_cmd), > }; > > static struct iscsi_transport iscsi_sw_tcp_transport = { > diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c > index 059dae8909ee..0337f7888ebe 100644 > --- a/drivers/scsi/libiscsi.c > +++ b/drivers/scsi/libiscsi.c > @@ -462,7 +462,7 @@ static void iscsi_free_task(struct iscsi_task *task) > > if (sc) { > /* SCSI eh reuses commands to verify us */ > - sc->SCp.ptr = NULL; > + iscsi_cmd(sc)->task = NULL; > /* > * queue command may call this to free the task, so > * it will decide how to return sc to scsi-ml. > @@ -1344,10 +1344,10 @@ struct iscsi_task *iscsi_itt_to_ctask(struct iscsi_conn *conn, itt_t itt) > if (!task || !task->sc) > return NULL; > > - if (task->sc->SCp.phase != conn->session->age) { > + if (iscsi_cmd(task->sc)->age != conn->session->age) { > iscsi_session_printk(KERN_ERR, conn->session, > "task's session age %d, expected %d\n", > - task->sc->SCp.phase, conn->session->age); > + iscsi_cmd(task->sc)->age, conn->session->age); > return NULL; > } > > @@ -1645,8 +1645,8 @@ static inline struct iscsi_task *iscsi_alloc_task(struct iscsi_conn *conn, > (void *) &task, sizeof(void *))) > return NULL; > > - sc->SCp.phase = conn->session->age; > - sc->SCp.ptr = (char *) task; > + iscsi_cmd(sc)->age = conn->session->age; > + iscsi_cmd(sc)->task = task; > > refcount_set(&task->refcount, 1); > task->state = ISCSI_TASK_PENDING; > @@ -1683,7 +1683,7 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc) > struct iscsi_task *task = NULL; > > sc->result = 0; > - sc->SCp.ptr = NULL; > + iscsi_cmd(sc)->task = NULL; > > ihost = shost_priv(host); > > @@ -1997,7 +1997,7 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc) > > spin_lock_bh(&session->frwd_lock); > spin_lock(&session->back_lock); > - task = (struct iscsi_task *)sc->SCp.ptr; > + task = iscsi_cmd(sc)->task; > if (!task) { > /* > * Raced with completion. Blk layer has taken ownership > @@ -2260,7 +2260,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc) > * if session was ISCSI_STATE_IN_RECOVERY then we may not have > * got the command. > */ > - if (!sc->SCp.ptr) { > + if (!iscsi_cmd(sc)->task) { > ISCSI_DBG_EH(session, "sc never reached iscsi layer or " > "it completed.\n"); > spin_unlock_bh(&session->frwd_lock); > @@ -2273,7 +2273,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc) > * then let the host reset code handle this > */ > if (!session->leadconn || session->state != ISCSI_STATE_LOGGED_IN || > - sc->SCp.phase != session->age) { > + iscsi_cmd(sc)->age != session->age) { > spin_unlock_bh(&session->frwd_lock); > mutex_unlock(&session->eh_mutex); > ISCSI_DBG_EH(session, "failing abort due to dropped " > @@ -2282,7 +2282,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc) > } > > spin_lock(&session->back_lock); > - task = (struct iscsi_task *)sc->SCp.ptr; > + task = iscsi_cmd(sc)->task; > if (!task || !task->sc) { > /* task completed before time out */ > ISCSI_DBG_EH(session, "sc completed while abort in progress\n"); > @@ -2792,6 +2792,8 @@ struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht, > struct Scsi_Host *shost; > struct iscsi_host *ihost; > > + WARN_ON_ONCE(sht->cmd_size < sizeof(struct iscsi_cmd)); > + > shost = scsi_host_alloc(sht, sizeof(struct iscsi_host) + dd_data_size); > if (!shost) > return NULL; > diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c > index 5916ed7662d5..d3170f2d023b 100644 > --- a/drivers/scsi/qedi/qedi_fw.c > +++ b/drivers/scsi/qedi/qedi_fw.c > @@ -603,7 +603,7 @@ static void qedi_scsi_completion(struct qedi_ctx *qedi, > goto error; > } > > - if (!sc_cmd->SCp.ptr) { > + if (!iscsi_cmd(sc_cmd)->task) { > QEDI_WARN(&qedi->dbg_ctx, > "SCp.ptr is NULL, returned in another context.\n"); > goto error; > diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c > index 282ecb4e39bb..8196f89f404e 100644 > --- a/drivers/scsi/qedi/qedi_iscsi.c > +++ b/drivers/scsi/qedi/qedi_iscsi.c > @@ -59,6 +59,7 @@ struct scsi_host_template qedi_host_template = { > .dma_boundary = QEDI_HW_DMA_BOUNDARY, > .cmd_per_lun = 128, > .shost_groups = qedi_shost_groups, > + .cmd_size = sizeof(struct iscsi_cmd), > }; > > static void qedi_conn_free_login_resources(struct qedi_ctx *qedi, > diff --git a/drivers/scsi/qla4xxx/ql4_def.h b/drivers/scsi/qla4xxx/ql4_def.h > index 69a590546bf9..a122909169ee 100644 > --- a/drivers/scsi/qla4xxx/ql4_def.h > +++ b/drivers/scsi/qla4xxx/ql4_def.h > @@ -216,11 +216,18 @@ > #define IDC_COMP_TOV 5 > #define LINK_UP_COMP_TOV 30 > > -#define CMD_SP(Cmnd) ((Cmnd)->SCp.ptr) > +struct qla4xxx_cmd_priv { > + struct iscsi_cmd iscsi_data; /* must be the first member */ > + struct srb *srb; > +}; > + > +static inline struct qla4xxx_cmd_priv *qla4xxx_cmd_priv(struct scsi_cmnd *cmd) > +{ > + return scsi_cmd_priv(cmd); > +} > > /* > - * SCSI Request Block structure (srb) that is placed > - * on cmd->SCp location of every I/O [We have 22 bytes available] > + * SCSI Request Block structure (srb) that is associated with each scsi_cmnd. > */ > struct srb { > struct list_head list; /* (8) */ > diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c > index 0ae936d839f1..d64eda961412 100644 > --- a/drivers/scsi/qla4xxx/ql4_os.c > +++ b/drivers/scsi/qla4xxx/ql4_os.c > @@ -226,6 +226,7 @@ static struct scsi_host_template qla4xxx_driver_template = { > .name = DRIVER_NAME, > .proc_name = DRIVER_NAME, > .queuecommand = qla4xxx_queuecommand, > + .cmd_size = sizeof(struct qla4xxx_cmd_priv), > > .eh_abort_handler = qla4xxx_eh_abort, > .eh_device_reset_handler = qla4xxx_eh_device_reset, > @@ -4054,7 +4055,7 @@ static struct srb* qla4xxx_get_new_srb(struct scsi_qla_host *ha, > srb->ddb = ddb_entry; > srb->cmd = cmd; > srb->flags = 0; > - CMD_SP(cmd) = (void *)srb; > + qla4xxx_cmd_priv(cmd)->srb = srb; > > return srb; > } > @@ -4067,7 +4068,7 @@ static void qla4xxx_srb_free_dma(struct scsi_qla_host *ha, struct srb *srb) > scsi_dma_unmap(cmd); > srb->flags &= ~SRB_DMA_VALID; > } > - CMD_SP(cmd) = NULL; > + qla4xxx_cmd_priv(cmd)->srb = NULL; > } > > void qla4xxx_srb_compl(struct kref *ref) > @@ -4640,7 +4641,7 @@ static int qla4xxx_cmd_wait(struct scsi_qla_host *ha) > * the scsi/block layer is going to prevent > * the tag from being released. > */ > - if (cmd != NULL && CMD_SP(cmd)) > + if (cmd != NULL && qla4xxx_cmd_priv(cmd)->srb) > break; > } > spin_unlock_irqrestore(&ha->hardware_lock, flags); > @@ -9079,7 +9080,7 @@ struct srb *qla4xxx_del_from_active_array(struct scsi_qla_host *ha, > if (!cmd) > return srb; > > - srb = (struct srb *)CMD_SP(cmd); > + srb = qla4xxx_cmd_priv(cmd)->srb; > if (!srb) > return srb; > > @@ -9121,7 +9122,7 @@ static int qla4xxx_eh_wait_on_command(struct scsi_qla_host *ha, > > do { > /* Checking to see if its returned to OS */ > - rp = (struct srb *) CMD_SP(cmd); > + rp = qla4xxx_cmd_priv(cmd)->srb; > if (rp == NULL) { > done++; > break; > @@ -9215,7 +9216,7 @@ static int qla4xxx_eh_abort(struct scsi_cmnd *cmd) > } > > spin_lock_irqsave(&ha->hardware_lock, flags); > - srb = (struct srb *) CMD_SP(cmd); > + srb = qla4xxx_cmd_priv(cmd)->srb; > if (!srb) { > spin_unlock_irqrestore(&ha->hardware_lock, flags); > ql4_printk(KERN_INFO, ha, "scsi%ld:%d:%llu: Specified command has already completed.\n", > diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h > index 4ee233e5a6ff..cb805ed9cbf1 100644 > --- a/include/scsi/libiscsi.h > +++ b/include/scsi/libiscsi.h > @@ -19,6 +19,7 @@ > #include <linux/refcount.h> > #include <scsi/iscsi_proto.h> > #include <scsi/iscsi_if.h> > +#include <scsi/scsi_cmnd.h> > #include <scsi/scsi_transport_iscsi.h> > > struct scsi_transport_template; > @@ -152,6 +153,17 @@ static inline bool iscsi_task_is_completed(struct iscsi_task *task) > task->state == ISCSI_TASK_ABRT_SESS_RECOV; > } > > +/* Private data associated with struct scsi_cmnd. */ > +struct iscsi_cmd { > + struct iscsi_task *task; > + int age; > +}; > + > +static inline struct iscsi_cmd *iscsi_cmd(struct scsi_cmnd *cmd) > +{ > + return scsi_cmd_priv(cmd); > +} > + > /* Connection's states */ > enum { > ISCSI_CONN_INITIAL_STAGE, Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com> -- Himanshu Madhani Oracle Linux Engineering
On 2/8/22 09:24, Bart Van Assche wrote: > Instead of storing the iSCSI task pointer and the session age in the SCSI > pointer, use command-private variables. This patch prepares for removal of > the SCSI pointer from struct scsi_cmnd. > > The list of iSCSI drivers has been obtained as follows: > $ git grep -lw iscsi_host_alloc > drivers/infiniband/ulp/iser/iscsi_iser.c > drivers/scsi/be2iscsi/be_main.c > drivers/scsi/bnx2i/bnx2i_iscsi.c > drivers/scsi/cxgbi/libcxgbi.c > drivers/scsi/iscsi_tcp.c > drivers/scsi/libiscsi.c > drivers/scsi/qedi/qedi_main.c > drivers/scsi/qla4xxx/ql4_os.c > include/scsi/libiscsi.h > > Note: it is not clear to me how the qla4xxx driver can work without this > patch since it uses the scsi_cmnd::SCp.ptr member for two different > purposes: > - The qla4xxx driver uses this member to store a struct srb pointer. > - libiscsi uses this member to store a struct iscsi_task pointer. > > Cc: Lee Duncan <lduncan@suse.com> > Cc: Chris Leech <cleech@redhat.com> > Cc: Sagi Grimberg <sagi@grimberg.me> > Cc: Nilesh Javali <njavali@marvell.com> > Cc: Manish Rangankar <mrangankar@marvell.com> > Cc: Karen Xie <kxie@chelsio.com> > Cc: Ketan Mukadam <ketan.mukadam@broadcom.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/infiniband/ulp/iser/iscsi_iser.c | 1 + > drivers/scsi/be2iscsi/be_main.c | 3 ++- > drivers/scsi/bnx2i/bnx2i_iscsi.c | 1 + > drivers/scsi/cxgbi/cxgb3i/cxgb3i.c | 1 + > drivers/scsi/cxgbi/cxgb4i/cxgb4i.c | 1 + > drivers/scsi/iscsi_tcp.c | 1 + > drivers/scsi/libiscsi.c | 22 ++++++++++++---------- > drivers/scsi/qedi/qedi_fw.c | 2 +- > drivers/scsi/qedi/qedi_iscsi.c | 1 + > drivers/scsi/qla4xxx/ql4_def.h | 13 ++++++++++--- > drivers/scsi/qla4xxx/ql4_os.c | 13 +++++++------ > include/scsi/libiscsi.h | 12 ++++++++++++ > 12 files changed, 50 insertions(+), 21 deletions(-) > > ... Reviewed-by: Lee Duncan <lduncan@suse.com>
On 2/8/22 11:24 AM, Bart Van Assche wrote: > Instead of storing the iSCSI task pointer and the session age in the SCSI > pointer, use command-private variables. This patch prepares for removal of > the SCSI pointer from struct scsi_cmnd. > > The list of iSCSI drivers has been obtained as follows: > $ git grep -lw iscsi_host_alloc > drivers/infiniband/ulp/iser/iscsi_iser.c > drivers/scsi/be2iscsi/be_main.c > drivers/scsi/bnx2i/bnx2i_iscsi.c > drivers/scsi/cxgbi/libcxgbi.c > drivers/scsi/iscsi_tcp.c > drivers/scsi/libiscsi.c > drivers/scsi/qedi/qedi_main.c > drivers/scsi/qla4xxx/ql4_os.c > include/scsi/libiscsi.h > > Note: it is not clear to me how the qla4xxx driver can work without this > patch since it uses the scsi_cmnd::SCp.ptr member for two different > purposes: qla4xxx doesn't use libiscsi for scsi_cmd based IO. It has it's own queuecommand, completion path and error handlers, because it offloads the entire scsi cmd operation. It only uses libiscsi for iscsi passthrough IO which doesn't use the scsi_cmnd. > > static void qedi_conn_free_login_resources(struct qedi_ctx *qedi, > diff --git a/drivers/scsi/qla4xxx/ql4_def.h b/drivers/scsi/qla4xxx/ql4_def.h > index 69a590546bf9..a122909169ee 100644 > --- a/drivers/scsi/qla4xxx/ql4_def.h > +++ b/drivers/scsi/qla4xxx/ql4_def.h > @@ -216,11 +216,18 @@ > #define IDC_COMP_TOV 5 > #define LINK_UP_COMP_TOV 30 > > -#define CMD_SP(Cmnd) ((Cmnd)->SCp.ptr) > +struct qla4xxx_cmd_priv { > + struct iscsi_cmd iscsi_data; /* must be the first member */ > + struct srb *srb; > +}; So you don't need the iscsi_cmd above.
On 2/9/22 1:24 PM, Mike Christie wrote: > On 2/8/22 11:24 AM, Bart Van Assche wrote: >> Instead of storing the iSCSI task pointer and the session age in the SCSI >> pointer, use command-private variables. This patch prepares for removal of >> the SCSI pointer from struct scsi_cmnd. >> >> The list of iSCSI drivers has been obtained as follows: >> $ git grep -lw iscsi_host_alloc >> drivers/infiniband/ulp/iser/iscsi_iser.c >> drivers/scsi/be2iscsi/be_main.c >> drivers/scsi/bnx2i/bnx2i_iscsi.c >> drivers/scsi/cxgbi/libcxgbi.c >> drivers/scsi/iscsi_tcp.c >> drivers/scsi/libiscsi.c >> drivers/scsi/qedi/qedi_main.c >> drivers/scsi/qla4xxx/ql4_os.c >> include/scsi/libiscsi.h >> >> Note: it is not clear to me how the qla4xxx driver can work without this >> patch since it uses the scsi_cmnd::SCp.ptr member for two different >> purposes: > > qla4xxx doesn't use libiscsi for scsi_cmd based IO. It has it's own > queuecommand, completion path and error handlers, because it offloads > the entire scsi cmd operation. > > It only uses libiscsi for iscsi passthrough IO which doesn't use the > scsi_cmnd. > > >> >> static void qedi_conn_free_login_resources(struct qedi_ctx *qedi, >> diff --git a/drivers/scsi/qla4xxx/ql4_def.h b/drivers/scsi/qla4xxx/ql4_def.h >> index 69a590546bf9..a122909169ee 100644 >> --- a/drivers/scsi/qla4xxx/ql4_def.h >> +++ b/drivers/scsi/qla4xxx/ql4_def.h >> @@ -216,11 +216,18 @@ >> #define IDC_COMP_TOV 5 >> #define LINK_UP_COMP_TOV 30 >> >> -#define CMD_SP(Cmnd) ((Cmnd)->SCp.ptr) >> +struct qla4xxx_cmd_priv { >> + struct iscsi_cmd iscsi_data; /* must be the first member */ >> + struct srb *srb; >> +}; > > > So you don't need the iscsi_cmd above. Mike's of course right about qla4xxx, but it still calls iscsi_host_alloc so only accounting for the srb pointer in cmd_size is going to trigger the WARN_ON_ONCE Bart added. - Chris Leech
On 2/8/22 9:24 AM, Bart Van Assche wrote: > Instead of storing the iSCSI task pointer and the session age in the SCSI > pointer, use command-private variables. This patch prepares for removal of > the SCSI pointer from struct scsi_cmnd. > --- a/drivers/scsi/qedi/qedi_fw.c > +++ b/drivers/scsi/qedi/qedi_fw.c > @@ -603,7 +603,7 @@ static void qedi_scsi_completion(struct qedi_ctx *qedi, > goto error; > } > > - if (!sc_cmd->SCp.ptr) { > + if (!iscsi_cmd(sc_cmd)->task) { > QEDI_WARN(&qedi->dbg_ctx, > "SCp.ptr is NULL, returned in another context.\n"); > goto error; > diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c > index 282ecb4e39bb..8196f89f404e 100644 Minor nit, but this warning should probably be changed as well. - Chris Leech
On 2/9/22 13:24, Mike Christie wrote: > On 2/8/22 11:24 AM, Bart Van Assche wrote: >> [ ... ] > qla4xxx doesn't use libiscsi for scsi_cmd based IO. It has it's own > queuecommand, completion path and error handlers, because it offloads > the entire scsi cmd operation. > > It only uses libiscsi for iscsi passthrough IO which doesn't use the > scsi_cmnd. > >> static void qedi_conn_free_login_resources(struct qedi_ctx *qedi, >> diff --git a/drivers/scsi/qla4xxx/ql4_def.h b/drivers/scsi/qla4xxx/ql4_def.h >> index 69a590546bf9..a122909169ee 100644 >> --- a/drivers/scsi/qla4xxx/ql4_def.h >> +++ b/drivers/scsi/qla4xxx/ql4_def.h >> @@ -216,11 +216,18 @@ >> #define IDC_COMP_TOV 5 >> #define LINK_UP_COMP_TOV 30 >> >> -#define CMD_SP(Cmnd) ((Cmnd)->SCp.ptr) >> +struct qla4xxx_cmd_priv { >> + struct iscsi_cmd iscsi_data; /* must be the first member */ >> + struct srb *srb; >> +}; > > So you don't need the iscsi_cmd above. Thanks for the feedback Mike. However, leaving out struct iscsi_cmd from the above data structure is something I'm nervous about because if there would be any code in the qla4xxx driver that calls a libiscsi function that writes into struct iscsi_cmd then that would trigger data corruption. Bart.
On 2/9/22 15:37, Chris Leech wrote: > On 2/8/22 9:24 AM, Bart Van Assche wrote: >> --- a/drivers/scsi/qedi/qedi_fw.c >> +++ b/drivers/scsi/qedi/qedi_fw.c >> @@ -603,7 +603,7 @@ static void qedi_scsi_completion(struct qedi_ctx *qedi, >> goto error; >> } >> >> - if (!sc_cmd->SCp.ptr) { >> + if (!iscsi_cmd(sc_cmd)->task) { >> QEDI_WARN(&qedi->dbg_ctx, >> "SCp.ptr is NULL, returned in another context.\n"); >> goto error; >> diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c >> index 282ecb4e39bb..8196f89f404e 100644 > > Minor nit, but this warning should probably be changed as well. Thanks, I will fix the reference to SCp.ptr. Bart.
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c index 07e47021a71f..f8d0bab4424c 100644 --- a/drivers/infiniband/ulp/iser/iscsi_iser.c +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c @@ -971,6 +971,7 @@ static struct scsi_host_template iscsi_iser_sht = { .proc_name = "iscsi_iser", .this_id = -1, .track_queue_depth = 1, + .cmd_size = sizeof(struct iscsi_cmd), }; static struct iscsi_transport iscsi_iser_transport = { diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c index ab55681145f8..3bb0adefbe06 100644 --- a/drivers/scsi/be2iscsi/be_main.c +++ b/drivers/scsi/be2iscsi/be_main.c @@ -218,7 +218,7 @@ static char const *cqe_desc[] = { static int beiscsi_eh_abort(struct scsi_cmnd *sc) { - struct iscsi_task *abrt_task = (struct iscsi_task *)sc->SCp.ptr; + struct iscsi_task *abrt_task = iscsi_cmd(sc)->task; struct iscsi_cls_session *cls_session; struct beiscsi_io_task *abrt_io_task; struct beiscsi_conn *beiscsi_conn; @@ -403,6 +403,7 @@ static struct scsi_host_template beiscsi_sht = { .cmd_per_lun = BEISCSI_CMD_PER_LUN, .vendor_id = SCSI_NL_VID_TYPE_PCI | BE_VENDOR_ID, .track_queue_depth = 1, + .cmd_size = sizeof(struct iscsi_cmd), }; static struct scsi_transport_template *beiscsi_scsi_transport; diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c index e21b053b4f3e..fe86fd61a995 100644 --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c @@ -2268,6 +2268,7 @@ static struct scsi_host_template bnx2i_host_template = { .sg_tablesize = ISCSI_MAX_BDS_PER_CMD, .shost_groups = bnx2i_dev_groups, .track_queue_depth = 1, + .cmd_size = sizeof(struct iscsi_cmd), }; struct iscsi_transport bnx2i_iscsi_transport = { diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c index f949a4e00783..ff9d4287937a 100644 --- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c +++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c @@ -98,6 +98,7 @@ static struct scsi_host_template cxgb3i_host_template = { .dma_boundary = PAGE_SIZE - 1, .this_id = -1, .track_queue_depth = 1, + .cmd_size = sizeof(struct iscsi_cmd), }; static struct iscsi_transport cxgb3i_iscsi_transport = { diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c index efb3e2b3398e..53d91bf9c12a 100644 --- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c +++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c @@ -116,6 +116,7 @@ static struct scsi_host_template cxgb4i_host_template = { .dma_boundary = PAGE_SIZE - 1, .this_id = -1, .track_queue_depth = 1, + .cmd_size = sizeof(struct iscsi_cmd), }; static struct iscsi_transport cxgb4i_iscsi_transport = { diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index 1bc37593c88f..9fee70d6434a 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -1007,6 +1007,7 @@ static struct scsi_host_template iscsi_sw_tcp_sht = { .proc_name = "iscsi_tcp", .this_id = -1, .track_queue_depth = 1, + .cmd_size = sizeof(struct iscsi_cmd), }; static struct iscsi_transport iscsi_sw_tcp_transport = { diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c index 059dae8909ee..0337f7888ebe 100644 --- a/drivers/scsi/libiscsi.c +++ b/drivers/scsi/libiscsi.c @@ -462,7 +462,7 @@ static void iscsi_free_task(struct iscsi_task *task) if (sc) { /* SCSI eh reuses commands to verify us */ - sc->SCp.ptr = NULL; + iscsi_cmd(sc)->task = NULL; /* * queue command may call this to free the task, so * it will decide how to return sc to scsi-ml. @@ -1344,10 +1344,10 @@ struct iscsi_task *iscsi_itt_to_ctask(struct iscsi_conn *conn, itt_t itt) if (!task || !task->sc) return NULL; - if (task->sc->SCp.phase != conn->session->age) { + if (iscsi_cmd(task->sc)->age != conn->session->age) { iscsi_session_printk(KERN_ERR, conn->session, "task's session age %d, expected %d\n", - task->sc->SCp.phase, conn->session->age); + iscsi_cmd(task->sc)->age, conn->session->age); return NULL; } @@ -1645,8 +1645,8 @@ static inline struct iscsi_task *iscsi_alloc_task(struct iscsi_conn *conn, (void *) &task, sizeof(void *))) return NULL; - sc->SCp.phase = conn->session->age; - sc->SCp.ptr = (char *) task; + iscsi_cmd(sc)->age = conn->session->age; + iscsi_cmd(sc)->task = task; refcount_set(&task->refcount, 1); task->state = ISCSI_TASK_PENDING; @@ -1683,7 +1683,7 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc) struct iscsi_task *task = NULL; sc->result = 0; - sc->SCp.ptr = NULL; + iscsi_cmd(sc)->task = NULL; ihost = shost_priv(host); @@ -1997,7 +1997,7 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc) spin_lock_bh(&session->frwd_lock); spin_lock(&session->back_lock); - task = (struct iscsi_task *)sc->SCp.ptr; + task = iscsi_cmd(sc)->task; if (!task) { /* * Raced with completion. Blk layer has taken ownership @@ -2260,7 +2260,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc) * if session was ISCSI_STATE_IN_RECOVERY then we may not have * got the command. */ - if (!sc->SCp.ptr) { + if (!iscsi_cmd(sc)->task) { ISCSI_DBG_EH(session, "sc never reached iscsi layer or " "it completed.\n"); spin_unlock_bh(&session->frwd_lock); @@ -2273,7 +2273,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc) * then let the host reset code handle this */ if (!session->leadconn || session->state != ISCSI_STATE_LOGGED_IN || - sc->SCp.phase != session->age) { + iscsi_cmd(sc)->age != session->age) { spin_unlock_bh(&session->frwd_lock); mutex_unlock(&session->eh_mutex); ISCSI_DBG_EH(session, "failing abort due to dropped " @@ -2282,7 +2282,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc) } spin_lock(&session->back_lock); - task = (struct iscsi_task *)sc->SCp.ptr; + task = iscsi_cmd(sc)->task; if (!task || !task->sc) { /* task completed before time out */ ISCSI_DBG_EH(session, "sc completed while abort in progress\n"); @@ -2792,6 +2792,8 @@ struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht, struct Scsi_Host *shost; struct iscsi_host *ihost; + WARN_ON_ONCE(sht->cmd_size < sizeof(struct iscsi_cmd)); + shost = scsi_host_alloc(sht, sizeof(struct iscsi_host) + dd_data_size); if (!shost) return NULL; diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c index 5916ed7662d5..d3170f2d023b 100644 --- a/drivers/scsi/qedi/qedi_fw.c +++ b/drivers/scsi/qedi/qedi_fw.c @@ -603,7 +603,7 @@ static void qedi_scsi_completion(struct qedi_ctx *qedi, goto error; } - if (!sc_cmd->SCp.ptr) { + if (!iscsi_cmd(sc_cmd)->task) { QEDI_WARN(&qedi->dbg_ctx, "SCp.ptr is NULL, returned in another context.\n"); goto error; diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c index 282ecb4e39bb..8196f89f404e 100644 --- a/drivers/scsi/qedi/qedi_iscsi.c +++ b/drivers/scsi/qedi/qedi_iscsi.c @@ -59,6 +59,7 @@ struct scsi_host_template qedi_host_template = { .dma_boundary = QEDI_HW_DMA_BOUNDARY, .cmd_per_lun = 128, .shost_groups = qedi_shost_groups, + .cmd_size = sizeof(struct iscsi_cmd), }; static void qedi_conn_free_login_resources(struct qedi_ctx *qedi, diff --git a/drivers/scsi/qla4xxx/ql4_def.h b/drivers/scsi/qla4xxx/ql4_def.h index 69a590546bf9..a122909169ee 100644 --- a/drivers/scsi/qla4xxx/ql4_def.h +++ b/drivers/scsi/qla4xxx/ql4_def.h @@ -216,11 +216,18 @@ #define IDC_COMP_TOV 5 #define LINK_UP_COMP_TOV 30 -#define CMD_SP(Cmnd) ((Cmnd)->SCp.ptr) +struct qla4xxx_cmd_priv { + struct iscsi_cmd iscsi_data; /* must be the first member */ + struct srb *srb; +}; + +static inline struct qla4xxx_cmd_priv *qla4xxx_cmd_priv(struct scsi_cmnd *cmd) +{ + return scsi_cmd_priv(cmd); +} /* - * SCSI Request Block structure (srb) that is placed - * on cmd->SCp location of every I/O [We have 22 bytes available] + * SCSI Request Block structure (srb) that is associated with each scsi_cmnd. */ struct srb { struct list_head list; /* (8) */ diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c index 0ae936d839f1..d64eda961412 100644 --- a/drivers/scsi/qla4xxx/ql4_os.c +++ b/drivers/scsi/qla4xxx/ql4_os.c @@ -226,6 +226,7 @@ static struct scsi_host_template qla4xxx_driver_template = { .name = DRIVER_NAME, .proc_name = DRIVER_NAME, .queuecommand = qla4xxx_queuecommand, + .cmd_size = sizeof(struct qla4xxx_cmd_priv), .eh_abort_handler = qla4xxx_eh_abort, .eh_device_reset_handler = qla4xxx_eh_device_reset, @@ -4054,7 +4055,7 @@ static struct srb* qla4xxx_get_new_srb(struct scsi_qla_host *ha, srb->ddb = ddb_entry; srb->cmd = cmd; srb->flags = 0; - CMD_SP(cmd) = (void *)srb; + qla4xxx_cmd_priv(cmd)->srb = srb; return srb; } @@ -4067,7 +4068,7 @@ static void qla4xxx_srb_free_dma(struct scsi_qla_host *ha, struct srb *srb) scsi_dma_unmap(cmd); srb->flags &= ~SRB_DMA_VALID; } - CMD_SP(cmd) = NULL; + qla4xxx_cmd_priv(cmd)->srb = NULL; } void qla4xxx_srb_compl(struct kref *ref) @@ -4640,7 +4641,7 @@ static int qla4xxx_cmd_wait(struct scsi_qla_host *ha) * the scsi/block layer is going to prevent * the tag from being released. */ - if (cmd != NULL && CMD_SP(cmd)) + if (cmd != NULL && qla4xxx_cmd_priv(cmd)->srb) break; } spin_unlock_irqrestore(&ha->hardware_lock, flags); @@ -9079,7 +9080,7 @@ struct srb *qla4xxx_del_from_active_array(struct scsi_qla_host *ha, if (!cmd) return srb; - srb = (struct srb *)CMD_SP(cmd); + srb = qla4xxx_cmd_priv(cmd)->srb; if (!srb) return srb; @@ -9121,7 +9122,7 @@ static int qla4xxx_eh_wait_on_command(struct scsi_qla_host *ha, do { /* Checking to see if its returned to OS */ - rp = (struct srb *) CMD_SP(cmd); + rp = qla4xxx_cmd_priv(cmd)->srb; if (rp == NULL) { done++; break; @@ -9215,7 +9216,7 @@ static int qla4xxx_eh_abort(struct scsi_cmnd *cmd) } spin_lock_irqsave(&ha->hardware_lock, flags); - srb = (struct srb *) CMD_SP(cmd); + srb = qla4xxx_cmd_priv(cmd)->srb; if (!srb) { spin_unlock_irqrestore(&ha->hardware_lock, flags); ql4_printk(KERN_INFO, ha, "scsi%ld:%d:%llu: Specified command has already completed.\n", diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h index 4ee233e5a6ff..cb805ed9cbf1 100644 --- a/include/scsi/libiscsi.h +++ b/include/scsi/libiscsi.h @@ -19,6 +19,7 @@ #include <linux/refcount.h> #include <scsi/iscsi_proto.h> #include <scsi/iscsi_if.h> +#include <scsi/scsi_cmnd.h> #include <scsi/scsi_transport_iscsi.h> struct scsi_transport_template; @@ -152,6 +153,17 @@ static inline bool iscsi_task_is_completed(struct iscsi_task *task) task->state == ISCSI_TASK_ABRT_SESS_RECOV; } +/* Private data associated with struct scsi_cmnd. */ +struct iscsi_cmd { + struct iscsi_task *task; + int age; +}; + +static inline struct iscsi_cmd *iscsi_cmd(struct scsi_cmnd *cmd) +{ + return scsi_cmd_priv(cmd); +} + /* Connection's states */ enum { ISCSI_CONN_INITIAL_STAGE,
Instead of storing the iSCSI task pointer and the session age in the SCSI pointer, use command-private variables. This patch prepares for removal of the SCSI pointer from struct scsi_cmnd. The list of iSCSI drivers has been obtained as follows: $ git grep -lw iscsi_host_alloc drivers/infiniband/ulp/iser/iscsi_iser.c drivers/scsi/be2iscsi/be_main.c drivers/scsi/bnx2i/bnx2i_iscsi.c drivers/scsi/cxgbi/libcxgbi.c drivers/scsi/iscsi_tcp.c drivers/scsi/libiscsi.c drivers/scsi/qedi/qedi_main.c drivers/scsi/qla4xxx/ql4_os.c include/scsi/libiscsi.h Note: it is not clear to me how the qla4xxx driver can work without this patch since it uses the scsi_cmnd::SCp.ptr member for two different purposes: - The qla4xxx driver uses this member to store a struct srb pointer. - libiscsi uses this member to store a struct iscsi_task pointer. Cc: Lee Duncan <lduncan@suse.com> Cc: Chris Leech <cleech@redhat.com> Cc: Sagi Grimberg <sagi@grimberg.me> Cc: Nilesh Javali <njavali@marvell.com> Cc: Manish Rangankar <mrangankar@marvell.com> Cc: Karen Xie <kxie@chelsio.com> Cc: Ketan Mukadam <ketan.mukadam@broadcom.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/infiniband/ulp/iser/iscsi_iser.c | 1 + drivers/scsi/be2iscsi/be_main.c | 3 ++- drivers/scsi/bnx2i/bnx2i_iscsi.c | 1 + drivers/scsi/cxgbi/cxgb3i/cxgb3i.c | 1 + drivers/scsi/cxgbi/cxgb4i/cxgb4i.c | 1 + drivers/scsi/iscsi_tcp.c | 1 + drivers/scsi/libiscsi.c | 22 ++++++++++++---------- drivers/scsi/qedi/qedi_fw.c | 2 +- drivers/scsi/qedi/qedi_iscsi.c | 1 + drivers/scsi/qla4xxx/ql4_def.h | 13 ++++++++++--- drivers/scsi/qla4xxx/ql4_os.c | 13 +++++++------ include/scsi/libiscsi.h | 12 ++++++++++++ 12 files changed, 50 insertions(+), 21 deletions(-)