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@xxxxxxx>
>>> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
>>> ---
>>> 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@xxxxxxxxxx>
>>
>> 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.
