Re: [PATCH 08/20] ibmvfc: open-code reset loop for target reset

From: Hannes Reinecke <hare@suse.de>
To: Ewan Milne <emilne@redhat.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>,
	Christoph Hellwig <hch@lst.de>,
	James Bottomley <james.bottomley@hansenpartnership.com>,
	linux-scsi@vger.kernel.org, Hannes Reinecke <hare@suse.com>,
	Johannes Thumshirn <johannes.thumshirn@wdc.com>,
	Tyrel Datwyler <tyreld@linux.ibm.com>
Subject: Re: [PATCH 08/20] ibmvfc: open-code reset loop for target reset
Date: Fri, 20 May 2022 07:52:40 +0200	[thread overview]
Message-ID: <9e69b1d4-f00d-01e4-2425-b3db6c1a5d62@suse.de> (raw)
In-Reply-To: <CAGtn9rn=RZRYpjD68ug+j6+ogenH-DSYD3-023bc5basjvq-4g@mail.gmail.com>

On 5/19/22 13:05, Ewan Milne wrote:
> This patch looks like it will call ibmvfc_reset_device() w/IBMVFC_TARGET_RESET
> before it has called ibmvfc_cancel_all() on all the devices, the
> existing code calls
> ibmvfc_reset_device() w/IBMVFC_TARGET_RESET after the iterator.
> 
> Since you have the starget, why change to use shost_for_each_device()
> and check the
> sdev->channel and sdev->id ?  That's what starget_for_each_device() does.
> 
> -Ewan
> 
> On Thu, May 12, 2022 at 7:13 AM Hannes Reinecke <hare@suse.de> wrote:
>>
>> From: Hannes Reinecke <hare@suse.com>
>>
>> For target reset we need a device to send the target reset to,
>> so open-code the loop in target reset to send the target reset TMF
>> to the correct device.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> Cc: Tyrel Datwyler <tyreld@linux.ibm.com>
>> ---
>>   drivers/scsi/ibmvscsi/ibmvfc.c | 42 +++++++++++++++++++---------------
>>   1 file changed, 23 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
>> index d0eab5700dc5..721d965f4b0e 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
>> @@ -2925,18 +2925,6 @@ static void ibmvfc_dev_cancel_all_noreset(struct scsi_device *sdev, void *data)
>>          *rc |= ibmvfc_cancel_all(sdev, IBMVFC_TMF_SUPPRESS_ABTS);
>>   }
>>
>> -/**
>> - * ibmvfc_dev_cancel_all_reset - Device iterated cancel all function
>> - * @sdev:      scsi device struct
>> - * @data:      return code
>> - *
>> - **/
>> -static void ibmvfc_dev_cancel_all_reset(struct scsi_device *sdev, void *data)
>> -{
>> -       unsigned long *rc = data;
>> -       *rc |= ibmvfc_cancel_all(sdev, IBMVFC_TMF_TGT_RESET);
>> -}
>> -
>>   /**
>>    * ibmvfc_eh_target_reset_handler - Reset the target
>>    * @cmd:       scsi command struct
>> @@ -2946,22 +2934,38 @@ static void ibmvfc_dev_cancel_all_reset(struct scsi_device *sdev, void *data)
>>    **/
>>   static int ibmvfc_eh_target_reset_handler(struct scsi_cmnd *cmd)
>>   {
>> -       struct scsi_device *sdev = cmd->device;
>> -       struct ibmvfc_host *vhost = shost_priv(sdev->host);
>> -       struct scsi_target *starget = scsi_target(sdev);
>> +       struct scsi_target *starget = scsi_target(cmd->device);
>> +       struct fc_rport *rport = starget_to_rport(starget);
>> +       struct Scsi_Host *shost = rport_to_shost(rport);
>> +       struct ibmvfc_host *vhost = shost_priv(shost);
>>          int block_rc;
>>          int reset_rc = 0;
>>          int rc = FAILED;
>>          unsigned long cancel_rc = 0;
>> +       bool tgt_reset = false;
>>
>>          ENTER;
>> -       block_rc = fc_block_scsi_eh(cmd);
>> +       block_rc = fc_block_rport(rport);
>>          ibmvfc_wait_while_resetting(vhost);
>>          if (block_rc != FAST_IO_FAIL) {
>> -               starget_for_each_device(starget, &cancel_rc, ibmvfc_dev_cancel_all_reset);
>> -               reset_rc = ibmvfc_reset_device(sdev, IBMVFC_TARGET_RESET, "target");
>> +               struct scsi_device *sdev;
>> +
>> +               shost_for_each_device(sdev, shost) {
>> +                       if ((sdev->channel != starget->channel) ||
>> +                           (sdev->id != starget->id))
>> +                               continue;
>> +
>> +                       cancel_rc |= ibmvfc_cancel_all(sdev,
>> +                                                      IBMVFC_TMF_TGT_RESET);
>> +                       if (!tgt_reset) {
>> +                               reset_rc = ibmvfc_reset_device(sdev,
>> +                                       IBMVFC_TARGET_RESET, "target");
>> +                               tgt_reset = true;
>> +                       }
>> +               }
>>          } else
>> -               starget_for_each_device(starget, &cancel_rc, ibmvfc_dev_cancel_all_noreset);
>> +               starget_for_each_device(starget, &cancel_rc,
>> +                                       ibmvfc_dev_cancel_all_noreset);
>>
>>          if (!cancel_rc && !reset_rc)
>>                  rc = ibmvfc_wait_for_ops(vhost, starget, ibmvfc_match_target);
>> --
>> 2.29.2
>>
> 
You are right; the call to ibmvfc_reset_device() needs to be moved out 
of the loop.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman