> On Oct 19, 2020, at 5:47 AM, Muneendra Kumar M <muneendra.kumar@xxxxxxxxxxxx> wrote:
>
> 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?
Sort of. I was saying to just drop the SCMD_NORETRIES_ABORT related code
and when you detect the port state here fail the IO.
> 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)?
Yes.
>
> 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.
>
I’m a little confused. In the 0/17 email you wrote:
-----
This feature is intended to be used when the device is part of a multipath
environment. When the service detects the poor connectivity, the multipath
path can be placed in a marginal path group and ignored further io
operations.
After placing a path in the marginal path group,the daemon sets the
port_state to Marginal which sets bit in scmd->state for all the
——
So it sounded like this is would only be used for mpio enabled paths.
Is this the daemon you mention in the 0/17 email:
https://github.com/brocade/bsn-fc-txptd
?
I might be completely misunderstanding you though. If the above link
is for your daemon then going off the 0/17 email and the GitHub README it
sounds like you are doing:
1. bsn-fc-txptd gets ELS, and moves affected paths to new marginal path group.
2. The non-marginal paths stay in the active path group and are continued to
be used like normal.
3. The paths in the marginal path group are not used until we get link up
or another ELS that indicates the paths are ok.
For these outstanding IOs on marginal paths, it sounds like you are flushing
the existing IO on them.mOnce they complete they will be in the marginal group
and not be used until #3.
So it’s not clear to me if you know the path is not optimal and might hit
a timeout, and you are not going to use it once the existing IO completes why
even try to send it? I mean in this setup, new commands that are entering
the dm-multipath layer will not be sent to these marginal paths right?
Also for the TUR comment, what TUR do you mean exactly? From the SCSI EH?
