RE: [PATCH v3 05/17] scsi_transport_fc: Added a new rport state FC_PORTSTATE_MARGINAL

From: Muneendra Kumar M <muneendra.kumar@broadcom.com>
To: Mike Christie <michael.christie@oracle.com>,
	linux-scsi@vger.kernel.org, hare@suse.de
Cc: jsmart2021@gmail.com, emilne@redhat.com, mkumar@redhat.com
Subject: RE: [PATCH v3 05/17] scsi_transport_fc: Added a new rport state FC_PORTSTATE_MARGINAL
Date: Mon, 19 Oct 2020 16:17:47 +0530	[thread overview]
Message-ID: <c3d65f732be0b73e4d4ebb742bc754cd@mail.gmail.com> (raw)
In-Reply-To: <5ca752c2-94e1-444a-7755-f48b09b38577@oracle.com>

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

Hi Mike,
Thanks for the review.

>> +fc_remote_port_chkready(struct fc_rport *rport, struct scsi_cmnd
>> +*cmd)
> >  {
>   >	int result;
>  >
>   >	switch (rport->port_state) {
>   >	case FC_PORTSTATE_ONLINE:
> >+	case FC_PORTSTATE_MARGINAL:
>   >		if (rport->roles & FC_PORT_ROLE_FCP_TARGET)
>   >			result = 0;
>   >		else if (rport->flags & FC_RPORT_DEVLOSS_PENDING)
>>   			result = DID_IMM_RETRY << 16;
>>   		else
>>   			result = DID_NO_CONNECT << 16;
> >+
> >+		if (cmd)
> >+			fc_rport_chkmarginal_set_noretries(rport, cmd);

>I was just wondering why don't you do drop all the SCMD_NORETRIES_ABORT
>code and then in this function when you check for the marginal state do:

>result = DID_TRANSPORT_MARGINAL;

?
[Muneendra] Correct me if my understanding is correct?
You mean to say ,if the SCMD_NORETRIES_ABORT is set end up failing the IO
with DID_TRANSPORT_MARGINAL instead of retry?
I assume that your below point 3 talks the same.
Let me know if this is correct.

>So you would do:

>1. Userspace calls in scsi_transport_fc and sets the port state to
>marginal.
2. New commands would see the above check and complete the command with
DID_TRANSPORT_MARGINAL.
[Muneendra]Correct me if my understanding is right.
You mean to say if the port_state is set to marginal return all the new
commands with DID_TRANSPORT_MARGINAL  (without checking
SCMD_NORETRIES_ABORT)?

If yes then below are my concerns
In marginal state  what we want is the ios to be attempted at least once .

In marginal state we cannot drop all the commands as one of the concern we
have is if a target is non-mpio based targets then we will be dropping it
without any attempt.
With this we will be even dropping the TUR commands also which unnecessarily
lead to error handling.

3. If a command is retried by the scsi layer we would end up hitting this
function and see the check and end up faling the IO like in #2. It would be
similar to what happens when the dev loss or fast io fail timers fire.

[Muneendra]

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