Mikey,
Thanks for reviewing. Responses to your comments inline below.
-matt
> On Aug 10, 2015, at 9:05 PM, Michael Neuling <mi...@neuling.org> wrote:
>
> On Mon, 2015-08-10 at 12:09 -0500, Matthew R. Ochs wrote:
>> Introduce support for enhanced I/O error handling.
>
> This needs more description of what you're doing. What's the overall
> approach? There seems to be a new limbo queue thats created stall things
> while the device is in error state. That should be described here.
Sure, will do that in v5.
>> struct afu_cmd {
>> diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
>> index 76a7286..18359d4 100644
>> --- a/drivers/scsi/cxlflash/main.c
>> +++ b/drivers/scsi/cxlflash/main.c
>> @@ -380,6 +380,18 @@ static int cxlflash_queuecommand(struct Scsi_Host
>> *host, struct scsi_cmnd *scp)
>> }
>> spin_unlock_irqrestore(&cfg->tmf_waitq.lock, lock_flags);
>>
>> + switch (cfg->state) {
>> + case STATE_LIMBO:
>> + pr_debug_ratelimited("%s: device in limbo!\n", __func__);
>> + rc = SCSI_MLQUEUE_HOST_BUSY;
>
> So if the client gets BUSY, it should retry until it suceeds or gets a
> terminal
> failure?
Correct, although this situation won’t hit often because as part of
transitioning
to limbo, we tell the host to stop handing down new requests.
>
>> + goto out;
>> + case STATE_FAILTERM:
>> + pr_debug_ratelimited("%s: device has failed!\n", __func__);
>> + goto error;
>
> error is only used here, so there is no need for a goto.
This was a holdover from when we did have a need for a common error exit. Will
remove in v5.
>> cmd = cxlflash_cmd_checkout(afu);
>> if (unlikely(!cmd)) {
>> pr_err("%s: could not get a free command\n", __func__);
>> @@ -428,6 +440,10 @@ static int cxlflash_queuecommand(struct Scsi_Host
>> *host, struct scsi_cmnd *scp)
>>
>> out:
>> return rc;
>> +error:
>> + scp->result = (DID_NO_CONNECT << 16);
>> + scp->scsi_done(scp);
>> + return 0;
>
> 0 is success. That doesn’t seem right for an error.
This is simply how this interface works.
It does seem a bit silly but what we’re doing here is telling the stack “yes we
accept your request” and then pushing the request back via done() with an error
due to the host being unable to satisfy the request.
>
>> }
>>
>> /**
>> @@ -455,9 +471,21 @@ static int cxlflash_eh_device_reset_handler(struct
>> scsi_cmnd *scp)
>> get_unaligned_be32(&((u32 *)scp->cmnd)[2]),
>> get_unaligned_be32(&((u32 *)scp->cmnd)[3]));
>>
>> - rcr = send_tmf(afu, scp, TMF_LUN_RESET);
>> - if (unlikely(rcr))
>> + switch (cfg->state) {
>> + case STATE_NORMAL:
>> + rcr = send_tmf(afu, scp, TMF_LUN_RESET);
>> + if (unlikely(rcr))
>> + rc = FAILED;
>> + break;
>> + case STATE_LIMBO:
>> + wait_event(cfg->limbo_waitq, cfg->state != STATE_LIMBO);
>
> So we wait here till we are our of limbo?
Correct, the idea here is that we only go into limbo when a host-wide
reset action has occurred. In such a scenario, it doesn’t make sense
to send a device specific reset as a host-wide reset is a bigger hammer.
So we wait here until the reset has completed and then (assuming it was
successful) we return success.
>
>> + if (cfg->state == STATE_NORMAL)
>> + break;
>> + /* fall through */
>> + default:
>> rc = FAILED;
>> + break;
>> + }
>>
>> pr_debug("%s: returning rc=%d\n", __func__, rc);
>> return rc;
>> @@ -487,11 +515,27 @@ static int cxlflash_eh_host_reset_handler(struct
>> scsi_cmnd *scp)
>> get_unaligned_be32(&((u32 *)scp->cmnd)[2]),
>> get_unaligned_be32(&((u32 *)scp->cmnd)[3]));
>>
>> - rcr = cxlflash_afu_reset(cfg);
>> - if (rcr == 0)
>> - rc = SUCCESS;
>> - else
>> + switch (cfg->state) {
>> + case STATE_NORMAL:
>> + cfg->state = STATE_LIMBO;
>> + scsi_block_requests(cfg->host);
>> +
>> + rcr = cxlflash_afu_reset(cfg);
>> + if (!rcr)
>> + rc = FAILED;
>
> This is some sort of recovery once we get back into normal state? Can you
> comment what you’re doing here?
What’s occurring here is that we’ve been asked to reset the host and have
determined that another reset is not already taking place. To perform the
reset, we need to transition to the limbo state so that other running threads
and new threads will wait while the reset takes place. Note that we’re also
putting user contexts in an error state so that they are notified and will have
to be recovered before they can resume operation (the reset is catastrophic
to them).
>
>> + cfg->state = STATE_NORMAL;
>> + wake_up_all(&cfg->limbo_waitq);
>> + scsi_unblock_requests(cfg->host);
>
> Now we actually go to normal?
Correct, assuming the reset completes successfully then we’re no longer
in limbo and have returned to a good, working state (normal). Note that
Daniel Axtens pointed out that we need to transition to a failed state when
the reset does not complete successfully. That will be changed in v5.
>
>> + break;
>> + case STATE_LIMBO:
>> + wait_event(cfg->limbo_waitq, cfg->state != STATE_LIMBO);
>
> Wait here till we are out of limbo? What happens if that never occurs?
This case would be hit if there was already a reset action taking place
(state of device in limbo). These events should not take long. A transition
out of limbo never occurring would be a bug on the driver’s part.
>> rc = init_mc(cfg);
>> if (rc) {
>> dev_err(dev, "%s: call to init_mc failed, rc=%d!\n",
>> @@ -2021,6 +2069,12 @@ void cxlflash_wait_resp(struct afu *afu, struct
>> afu_cmd *cmd)
>> * the sync. This design point requires calling threads to not be on
>> interrupt
>> * context due to the possibility of sleeping during concurrent sync
>> operations.
>> *
>> + * AFU sync operations are only necessary and allowed when the device is
>> + * operating normally. When not operating normally, sync requests can occur
>> as
>> + * part of cleaning up resources associated with an adapter prior to
>> removal.
>> + * In this scenario, these requests are simply ignored (safe due to the AFU
>> + * going away).
>> + *
>
> What about if we are in limbo state and it comes back?
Limbo state indicates that the device is being reset. When the device
is being reset, a sync does not matter as it’s state (the device’s) is being
lost during the reset. The device will be reloaded with the proper state as
part of the reset and recovery [of user contexts].
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
