| Message ID | 20231016092430.55557-10-hare@suse.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: EH rework prep patches, part 2 | expand |
On 10/16/23 4:24 AM, Hannes Reinecke wrote: > iscsi_eh_target_reset() and iscsi_eh_session_reset() only depend > on the cls_session, so use that as an argument. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > Reviewed-by: Christoph Hellwig <hch@lst.de> > --- > drivers/scsi/be2iscsi/be_main.c | 10 +++++++++- > drivers/scsi/libiscsi.c | 21 +++++++++------------ > include/scsi/libiscsi.h | 2 +- > 3 files changed, 19 insertions(+), 14 deletions(-) > > diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c > index e48f14ad6dfd..441ad2ebc5d5 100644 > --- a/drivers/scsi/be2iscsi/be_main.c > +++ b/drivers/scsi/be2iscsi/be_main.c > @@ -385,6 +385,14 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc) > return rc; > } > > +static int beiscsi_eh_session_reset(struct scsi_cmnd *sc) > +{ > + struct iscsi_cls_session *cls_session; > + > + cls_session = starget_to_session(scsi_target(sc->device)); > + return iscsi_eh_session_reset(cls_session); > +} > + > /*------------------- PCI Driver operations and data ----------------- */ > static const struct pci_device_id beiscsi_pci_id_table[] = { > { PCI_DEVICE(BE_VENDOR_ID, BE_DEVICE_ID1) }, > @@ -408,7 +416,7 @@ static const struct scsi_host_template beiscsi_sht = { > .eh_timed_out = iscsi_eh_cmd_timed_out, > .eh_abort_handler = beiscsi_eh_abort, > .eh_device_reset_handler = beiscsi_eh_device_reset, > - .eh_target_reset_handler = iscsi_eh_session_reset, > + .eh_target_reset_handler = beiscsi_eh_session_reset, > .shost_groups = beiscsi_groups, > .sg_tablesize = BEISCSI_SGLIST_ELEMENTS, > .can_queue = BE2_IO_DEPTH, > diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c > index 0fda8905eabd..a561eefabb50 100644 > --- a/drivers/scsi/libiscsi.c > +++ b/drivers/scsi/libiscsi.c > @@ -2600,13 +2600,11 @@ EXPORT_SYMBOL_GPL(iscsi_session_recovery_timedout); > * This function will wait for a relogin, session termination from > * userspace, or a recovery/replacement timeout. > */ > -int iscsi_eh_session_reset(struct scsi_cmnd *sc) > +int iscsi_eh_session_reset(struct iscsi_cls_session *cls_session) > { Patch looks ok to me. Reviewed-by: Mike Christie <michael.christie@oracle.com> As an alternative to this approach though it might be easier to have this function take a scsi_target. You won't need beiscsi_eh_session_reset and for iscsi_eh_recover_target you can pass the scsi_target to iscsi_eh_recover_target/iscsi_eh_session_reset. Either way is ok to me though since we have to convert from scsi_target to cls_session somewhere. > - struct iscsi_cls_session *cls_session; > struct iscsi_session *session; > struct iscsi_conn *conn; > > - cls_session = starget_to_session(scsi_target(sc->device)); > session = cls_session->dd_data; > > mutex_lock(&session->eh_mutex); > @@ -2653,7 +2651,7 @@ int iscsi_eh_session_reset(struct scsi_cmnd *sc) > } > EXPORT_SYMBOL_GPL(iscsi_eh_session_reset); > > -static void iscsi_prep_tgt_reset_pdu(struct scsi_cmnd *sc, struct iscsi_tm *hdr) > +static void iscsi_prep_tgt_reset_pdu(struct iscsi_tm *hdr) > { > memset(hdr, 0, sizeof(*hdr)); > hdr->opcode = ISCSI_OP_SCSI_TMFUNC | ISCSI_OP_IMMEDIATE; > @@ -2668,19 +2666,16 @@ static void iscsi_prep_tgt_reset_pdu(struct scsi_cmnd *sc, struct iscsi_tm *hdr) > * > * This will attempt to send a warm target reset. > */ > -static int iscsi_eh_target_reset(struct scsi_cmnd *sc) > +static int iscsi_eh_target_reset(struct iscsi_cls_session *cls_session) > { > - struct iscsi_cls_session *cls_session; > struct iscsi_session *session; > struct iscsi_conn *conn; > struct iscsi_tm *hdr; > int rc = FAILED; > > - cls_session = starget_to_session(scsi_target(sc->device)); > session = cls_session->dd_data; > > - ISCSI_DBG_EH(session, "tgt Reset [sc %p tgt %s]\n", sc, > - session->targetname); > + ISCSI_DBG_EH(session, "tgt Reset [tgt %s]\n", session->targetname); > > mutex_lock(&session->eh_mutex); > spin_lock_bh(&session->frwd_lock); > @@ -2698,7 +2693,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc) > session->tmf_state = TMF_QUEUED; > > hdr = &session->tmhdr; > - iscsi_prep_tgt_reset_pdu(sc, hdr); > + iscsi_prep_tgt_reset_pdu(hdr); > > if (iscsi_exec_task_mgmt_fn(conn, hdr, session->age, > session->tgt_reset_timeout)) { > @@ -2750,11 +2745,13 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc) > */ > int iscsi_eh_recover_target(struct scsi_cmnd *sc) > { > + struct iscsi_cls_session *cls_session; > int rc; > > - rc = iscsi_eh_target_reset(sc); > + cls_session = starget_to_session(scsi_target(sc->device)); > + rc = iscsi_eh_target_reset(cls_session); > if (rc == FAILED) > - rc = iscsi_eh_session_reset(sc); > + rc = iscsi_eh_session_reset(cls_session); > return rc; > } > EXPORT_SYMBOL_GPL(iscsi_eh_recover_target); > diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h > index 7282555adfd5..7dddf785edd0 100644 > --- a/include/scsi/libiscsi.h > +++ b/include/scsi/libiscsi.h > @@ -390,7 +390,7 @@ struct iscsi_host { > */ > extern int iscsi_eh_abort(struct scsi_cmnd *sc); > extern int iscsi_eh_recover_target(struct scsi_cmnd *sc); > -extern int iscsi_eh_session_reset(struct scsi_cmnd *sc); > +extern int iscsi_eh_session_reset(struct iscsi_cls_session *cls_session); > extern int iscsi_eh_device_reset(struct scsi_cmnd *sc); > extern int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc); > extern enum scsi_timeout_action iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc);
On 10/19/23 22:02, Mike Christie wrote: > On 10/16/23 4:24 AM, Hannes Reinecke wrote: >> iscsi_eh_target_reset() and iscsi_eh_session_reset() only depend >> on the cls_session, so use that as an argument. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> --- >> drivers/scsi/be2iscsi/be_main.c | 10 +++++++++- >> drivers/scsi/libiscsi.c | 21 +++++++++------------ >> include/scsi/libiscsi.h | 2 +- >> 3 files changed, 19 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c >> index e48f14ad6dfd..441ad2ebc5d5 100644 >> --- a/drivers/scsi/be2iscsi/be_main.c >> +++ b/drivers/scsi/be2iscsi/be_main.c >> @@ -385,6 +385,14 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc) >> return rc; >> } >> >> +static int beiscsi_eh_session_reset(struct scsi_cmnd *sc) >> +{ >> + struct iscsi_cls_session *cls_session; >> + >> + cls_session = starget_to_session(scsi_target(sc->device)); >> + return iscsi_eh_session_reset(cls_session); >> +} >> + >> /*------------------- PCI Driver operations and data ----------------- */ >> static const struct pci_device_id beiscsi_pci_id_table[] = { >> { PCI_DEVICE(BE_VENDOR_ID, BE_DEVICE_ID1) }, >> @@ -408,7 +416,7 @@ static const struct scsi_host_template beiscsi_sht = { >> .eh_timed_out = iscsi_eh_cmd_timed_out, >> .eh_abort_handler = beiscsi_eh_abort, >> .eh_device_reset_handler = beiscsi_eh_device_reset, >> - .eh_target_reset_handler = iscsi_eh_session_reset, >> + .eh_target_reset_handler = beiscsi_eh_session_reset, >> .shost_groups = beiscsi_groups, >> .sg_tablesize = BEISCSI_SGLIST_ELEMENTS, >> .can_queue = BE2_IO_DEPTH, >> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c >> index 0fda8905eabd..a561eefabb50 100644 >> --- a/drivers/scsi/libiscsi.c >> +++ b/drivers/scsi/libiscsi.c >> @@ -2600,13 +2600,11 @@ EXPORT_SYMBOL_GPL(iscsi_session_recovery_timedout); >> * This function will wait for a relogin, session termination from >> * userspace, or a recovery/replacement timeout. >> */ >> -int iscsi_eh_session_reset(struct scsi_cmnd *sc) >> +int iscsi_eh_session_reset(struct iscsi_cls_session *cls_session) >> { > > Patch looks ok to me. > > Reviewed-by: Mike Christie <michael.christie@oracle.com> > > As an alternative to this approach though it might be easier to have > this function take a scsi_target. You won't need beiscsi_eh_session_reset > and for iscsi_eh_recover_target you can pass the scsi_target to > iscsi_eh_recover_target/iscsi_eh_session_reset. > > Either way is ok to me though since we have to convert from scsi_target > to cls_session somewhere. > Yeah, one could do that. But the relationship between the target and the host is not fixed, but rather depends on the driver and/or transport class. For simpler devices the host is the parent, for others there are elements in between so the parent device is something else entirely. So for generic routines (like libiscsi) I prefer to use dedicated elements such that the relationship is known. Cheers, Hannes
On 10/20/23 12:53 AM, Hannes Reinecke wrote: > On 10/19/23 22:02, Mike Christie wrote: >> On 10/16/23 4:24 AM, Hannes Reinecke wrote: >>> iscsi_eh_target_reset() and iscsi_eh_session_reset() only depend >>> on the cls_session, so use that as an argument. >>> >>> Signed-off-by: Hannes Reinecke <hare@suse.de> >>> Reviewed-by: Christoph Hellwig <hch@lst.de> >>> --- >>> drivers/scsi/be2iscsi/be_main.c | 10 +++++++++- >>> drivers/scsi/libiscsi.c | 21 +++++++++------------ >>> include/scsi/libiscsi.h | 2 +- >>> 3 files changed, 19 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c >>> index e48f14ad6dfd..441ad2ebc5d5 100644 >>> --- a/drivers/scsi/be2iscsi/be_main.c >>> +++ b/drivers/scsi/be2iscsi/be_main.c >>> @@ -385,6 +385,14 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc) >>> return rc; >>> } >>> +static int beiscsi_eh_session_reset(struct scsi_cmnd *sc) >>> +{ >>> + struct iscsi_cls_session *cls_session; >>> + >>> + cls_session = starget_to_session(scsi_target(sc->device)); >>> + return iscsi_eh_session_reset(cls_session); >>> +} >>> + >>> /*------------------- PCI Driver operations and data ----------------- */ >>> static const struct pci_device_id beiscsi_pci_id_table[] = { >>> { PCI_DEVICE(BE_VENDOR_ID, BE_DEVICE_ID1) }, >>> @@ -408,7 +416,7 @@ static const struct scsi_host_template beiscsi_sht = { >>> .eh_timed_out = iscsi_eh_cmd_timed_out, >>> .eh_abort_handler = beiscsi_eh_abort, >>> .eh_device_reset_handler = beiscsi_eh_device_reset, >>> - .eh_target_reset_handler = iscsi_eh_session_reset, >>> + .eh_target_reset_handler = beiscsi_eh_session_reset, >>> .shost_groups = beiscsi_groups, >>> .sg_tablesize = BEISCSI_SGLIST_ELEMENTS, >>> .can_queue = BE2_IO_DEPTH, >>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c >>> index 0fda8905eabd..a561eefabb50 100644 >>> --- a/drivers/scsi/libiscsi.c >>> +++ b/drivers/scsi/libiscsi.c >>> @@ -2600,13 +2600,11 @@ EXPORT_SYMBOL_GPL(iscsi_session_recovery_timedout); >>> * This function will wait for a relogin, session termination from >>> * userspace, or a recovery/replacement timeout. >>> */ >>> -int iscsi_eh_session_reset(struct scsi_cmnd *sc) >>> +int iscsi_eh_session_reset(struct iscsi_cls_session *cls_session) >>> { >> >> Patch looks ok to me. >> >> Reviewed-by: Mike Christie <michael.christie@oracle.com> >> >> As an alternative to this approach though it might be easier to have >> this function take a scsi_target. You won't need beiscsi_eh_session_reset >> and for iscsi_eh_recover_target you can pass the scsi_target to >> iscsi_eh_recover_target/iscsi_eh_session_reset. >> >> Either way is ok to me though since we have to convert from scsi_target >> to cls_session somewhere. >> > Yeah, one could do that. But the relationship between the target and > the host is not fixed, but rather depends on the driver and/or transport class. For simpler devices the host is the parent, for others there are elements in between so the parent device is something else entirely. > So for generic routines (like libiscsi) I prefer to use dedicated > elements such that the relationship is known. > Based on the first part of your mail, I think you want what I suggested :) libiscsi and the transport class setup the relationship between the scsi_target and iscsi session. They handle deciding what is the targe's parent and pass the session struct device as the parent. The drivers like be2iscsi don't know this info normally (be2iscsi has to do it in the abort and reset function because it works reverse of the other drivers). So, you don't want beiscsi_eh_session_reset function since all it does it translate between target and session which it shouldn't be handling since the class/lib handle it. You just want to pass the libiscsi function iscsi_eh_session_reset the target and it will figure out the relationship since it set it up.
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c index e48f14ad6dfd..441ad2ebc5d5 100644 --- a/drivers/scsi/be2iscsi/be_main.c +++ b/drivers/scsi/be2iscsi/be_main.c @@ -385,6 +385,14 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc) return rc; } +static int beiscsi_eh_session_reset(struct scsi_cmnd *sc) +{ + struct iscsi_cls_session *cls_session; + + cls_session = starget_to_session(scsi_target(sc->device)); + return iscsi_eh_session_reset(cls_session); +} + /*------------------- PCI Driver operations and data ----------------- */ static const struct pci_device_id beiscsi_pci_id_table[] = { { PCI_DEVICE(BE_VENDOR_ID, BE_DEVICE_ID1) }, @@ -408,7 +416,7 @@ static const struct scsi_host_template beiscsi_sht = { .eh_timed_out = iscsi_eh_cmd_timed_out, .eh_abort_handler = beiscsi_eh_abort, .eh_device_reset_handler = beiscsi_eh_device_reset, - .eh_target_reset_handler = iscsi_eh_session_reset, + .eh_target_reset_handler = beiscsi_eh_session_reset, .shost_groups = beiscsi_groups, .sg_tablesize = BEISCSI_SGLIST_ELEMENTS, .can_queue = BE2_IO_DEPTH, diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c index 0fda8905eabd..a561eefabb50 100644 --- a/drivers/scsi/libiscsi.c +++ b/drivers/scsi/libiscsi.c @@ -2600,13 +2600,11 @@ EXPORT_SYMBOL_GPL(iscsi_session_recovery_timedout); * This function will wait for a relogin, session termination from * userspace, or a recovery/replacement timeout. */ -int iscsi_eh_session_reset(struct scsi_cmnd *sc) +int iscsi_eh_session_reset(struct iscsi_cls_session *cls_session) { - struct iscsi_cls_session *cls_session; struct iscsi_session *session; struct iscsi_conn *conn; - cls_session = starget_to_session(scsi_target(sc->device)); session = cls_session->dd_data; mutex_lock(&session->eh_mutex); @@ -2653,7 +2651,7 @@ int iscsi_eh_session_reset(struct scsi_cmnd *sc) } EXPORT_SYMBOL_GPL(iscsi_eh_session_reset); -static void iscsi_prep_tgt_reset_pdu(struct scsi_cmnd *sc, struct iscsi_tm *hdr) +static void iscsi_prep_tgt_reset_pdu(struct iscsi_tm *hdr) { memset(hdr, 0, sizeof(*hdr)); hdr->opcode = ISCSI_OP_SCSI_TMFUNC | ISCSI_OP_IMMEDIATE; @@ -2668,19 +2666,16 @@ static void iscsi_prep_tgt_reset_pdu(struct scsi_cmnd *sc, struct iscsi_tm *hdr) * * This will attempt to send a warm target reset. */ -static int iscsi_eh_target_reset(struct scsi_cmnd *sc) +static int iscsi_eh_target_reset(struct iscsi_cls_session *cls_session) { - struct iscsi_cls_session *cls_session; struct iscsi_session *session; struct iscsi_conn *conn; struct iscsi_tm *hdr; int rc = FAILED; - cls_session = starget_to_session(scsi_target(sc->device)); session = cls_session->dd_data; - ISCSI_DBG_EH(session, "tgt Reset [sc %p tgt %s]\n", sc, - session->targetname); + ISCSI_DBG_EH(session, "tgt Reset [tgt %s]\n", session->targetname); mutex_lock(&session->eh_mutex); spin_lock_bh(&session->frwd_lock); @@ -2698,7 +2693,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc) session->tmf_state = TMF_QUEUED; hdr = &session->tmhdr; - iscsi_prep_tgt_reset_pdu(sc, hdr); + iscsi_prep_tgt_reset_pdu(hdr); if (iscsi_exec_task_mgmt_fn(conn, hdr, session->age, session->tgt_reset_timeout)) { @@ -2750,11 +2745,13 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc) */ int iscsi_eh_recover_target(struct scsi_cmnd *sc) { + struct iscsi_cls_session *cls_session; int rc; - rc = iscsi_eh_target_reset(sc); + cls_session = starget_to_session(scsi_target(sc->device)); + rc = iscsi_eh_target_reset(cls_session); if (rc == FAILED) - rc = iscsi_eh_session_reset(sc); + rc = iscsi_eh_session_reset(cls_session); return rc; } EXPORT_SYMBOL_GPL(iscsi_eh_recover_target); diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h index 7282555adfd5..7dddf785edd0 100644 --- a/include/scsi/libiscsi.h +++ b/include/scsi/libiscsi.h @@ -390,7 +390,7 @@ struct iscsi_host { */ extern int iscsi_eh_abort(struct scsi_cmnd *sc); extern int iscsi_eh_recover_target(struct scsi_cmnd *sc); -extern int iscsi_eh_session_reset(struct scsi_cmnd *sc); +extern int iscsi_eh_session_reset(struct iscsi_cls_session *cls_session); extern int iscsi_eh_device_reset(struct scsi_cmnd *sc); extern int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc); extern enum scsi_timeout_action iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc);