RE: [PATCH v6 3/4] scsi_transport_fc: Added a new rport state FC_PORTSTATE_MARGINAL

[Muneendra Kumar M <muneendra.kumar@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 2202 bytes --]

Hi Mike,
Thanks for the detailed input.
I will apply the below changes and will resubmit the patches.

Regards,
Muneendra.

-----Original Message-----
From: Mike Christie [mailto:michael.christie@oracle.com]
Sent: Thursday, November 5, 2020 9:29 PM
To: Muneendra <muneendra.kumar@broadcom.com>; linux-scsi@vger.kernel.org;
hare@suse.de
Cc: jsmart2021@gmail.com; emilne@redhat.com; mkumar@redhat.com
Subject: Re: [PATCH v6 3/4] scsi_transport_fc: Added a new rport state
FC_PORTSTATE_MARGINAL

On 11/5/20 12:09 AM, Muneendra wrote:
>   int fc_block_scsi_eh(struct scsi_cmnd *cmnd)
>   {
>   	struct fc_rport *rport =
> starget_to_rport(scsi_target(cmnd->device));
> +	int ret = 0;
>
>   	if (WARN_ON_ONCE(!rport))
>   		return FAST_IO_FAIL;
>
> -	return fc_block_rport(rport);
> +	ret = fc_block_rport(rport);
> +	/*
> +	 * Clear the SCMD_NORETRIES_ABORT bit if the Port state has
> +	 * changed from marginal to online due to
> +	 * fc_remote_port_delete and fc_remote_port_add
> +	 */
> +	if (rport->port_state != FC_PORTSTATE_MARGINAL)
> +		clear_bit(SCMD_NORETRIES_ABORT, &cmnd->state);
> +	return ret;
>   }


Hey sorry for the late reply. I was trying to test some things out but am
not sure if all drivers work the same.

For the code above, what will happen if we have passed that check in the
driver, then the driver does the report del and add sequence? Let's say it's
initially calling the abort callout, and we passed that check, we then do
the del/add seqeuence, what will happen next? Do the fc drivers return
success or failure for the abort call. What happens for the other callouts
too?

If failure, then the eh escalates and when we call the next callout, and we
hit the check above and will clear it, so we are ok.

If success then we would not get a chance to clear it right? If this is the
case, then I think you need to instead go the route where you add the eh cmd
completion/decide_disposition callout. You would call it in
scmd_eh_abort_handler, scsi_eh_bus_device_reset, etc when we are deciding if
we want to retry/fail the command. In this approach you do not need the
eh_timed_out changes, since we only seem to care about the port state after
the eh callout has completed.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]