| Message ID | 5315A3E4.508@dev.mellanox.co.il (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 04/03/2014 11:59, Sagi Grimberg wrote: > On 3/4/2014 11:38 AM, Or Gerlitz wrote: >> On 03/03/2014 06:44, Mike Christie wrote: >>> The xmit_task callout does handle failures like EINVAL. If the above >>> map >>> calls fail then you would get infinite retries. You would currently >>> want >>> to do the mapping in the init_task callout instead. >>> >>> If it makes it easier on the driver implementation then it is ok to >>> modify the xmit_task callers so that they handle multiple error codes >>> for drivers like iser that have the xmit_task callout called from >>> iscsi_queuecommand. >> >> Mike, >> >> After looking on the code with Sagi, it seems to us that the correct >> way to go here, would be to enhance in iscsi_queuecommand the >> processing of the result returned by session->tt->xmit_task(task) to >> behave in a similar manner to how the return value of >> iscsi_prep_scsi_cmd_pdu() is treated. E.g for errors such as ENOMEM >> and EGAIN take the "reject" flow which would cause the SCSI midlayer >> to retry the command and for other return values go to the "fault" >> flow which will cause the ML to abort the command. >> >> Or. >> > > Yes, we were thinking about the following: > --- a/drivers/scsi/libiscsi.c > +++ b/drivers/scsi/libiscsi.c > @@ -1707,10 +1707,17 @@ int iscsi_queuecommand(struct Scsi_Host *host, > struct scsi_cmnd *sc) > goto prepd_fault; > } > } > - if (session->tt->xmit_task(task)) { > - session->cmdsn--; > - reason = FAILURE_SESSION_NOT_READY; > - goto prepd_reject; > + > + reason = session->tt->xmit_task(task); > + if (reason) { > + if (reason == -ENOMEM || reason == -EAGAIN) { > + session->cmdsn--; I am pretty sure this has to be done anyway, no matter why the xmit_task callback failed > + reason = FAILURE_SESSION_NOT_READY; > + goto prepd_reject; > + } else { > + sc->result = DID_ABORT << 16; > + goto prepd_fault; > + } > } > } else { > list_add_tail(&task->running, &conn->cmdqueue); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/4/2014 1:25 PM, Or Gerlitz wrote: > On 04/03/2014 11:59, Sagi Grimberg wrote: >> On 3/4/2014 11:38 AM, Or Gerlitz wrote: >>> On 03/03/2014 06:44, Mike Christie wrote: >>>> The xmit_task callout does handle failures like EINVAL. If the >>>> above map >>>> calls fail then you would get infinite retries. You would currently >>>> want >>>> to do the mapping in the init_task callout instead. >>>> >>>> If it makes it easier on the driver implementation then it is ok to >>>> modify the xmit_task callers so that they handle multiple error codes >>>> for drivers like iser that have the xmit_task callout called from >>>> iscsi_queuecommand. >>> >>> Mike, >>> >>> After looking on the code with Sagi, it seems to us that the >>> correct way to go here, would be to enhance in iscsi_queuecommand >>> the processing of the result returned by >>> session->tt->xmit_task(task) to behave in a similar manner to how >>> the return value of iscsi_prep_scsi_cmd_pdu() is treated. E.g for >>> errors such as ENOMEM and EGAIN take the "reject" flow which would >>> cause the SCSI midlayer to retry the command and for other return >>> values go to the "fault" flow which will cause the ML to abort the >>> command. >>> >>> Or. >>> >> >> Yes, we were thinking about the following: >> --- a/drivers/scsi/libiscsi.c >> +++ b/drivers/scsi/libiscsi.c >> @@ -1707,10 +1707,17 @@ int iscsi_queuecommand(struct Scsi_Host >> *host, struct scsi_cmnd *sc) >> goto prepd_fault; >> } >> } >> - if (session->tt->xmit_task(task)) { >> - session->cmdsn--; >> - reason = FAILURE_SESSION_NOT_READY; >> - goto prepd_reject; >> + >> + reason = session->tt->xmit_task(task); >> + if (reason) { >> + if (reason == -ENOMEM || reason == -EAGAIN) { >> + session->cmdsn--; > > I am pretty sure this has to be done anyway, no matter why the > xmit_task callback failed Even if we abort? this just follows the same logic as iscsi_prep_scsi_cmd_pdu error flow. > >> + reason = FAILURE_SESSION_NOT_READY; >> + goto prepd_reject; >> + } else { >> + sc->result = DID_ABORT << 16; >> + goto prepd_fault; >> + } >> } >> } else { >> list_add_tail(&task->running, &conn->cmdqueue); >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/03/2014 16:44, Sagi Grimberg wrote: >>> @@ -1707,10 +1707,17 @@ int iscsi_queuecommand(struct Scsi_Host >>> *host, struct scsi_cmnd *sc) >>> goto prepd_fault; >>> } >>> } >>> - if (session->tt->xmit_task(task)) { >>> - session->cmdsn--; >>> - reason = FAILURE_SESSION_NOT_READY; >>> - goto prepd_reject; >>> + >>> + reason = session->tt->xmit_task(task); >>> + if (reason) { >>> + if (reason == -ENOMEM || reason == -EAGAIN) { >>> + session->cmdsn--; >> >> I am pretty sure this has to be done anyway, no matter why the >> xmit_task callback failed > > Even if we abort? this just follows the same logic as > iscsi_prep_scsi_cmd_pdu error flow. yes, take a 2nd look on iscsi_prep_scsi_cmd_pdu and you'll see that all the possible error cases take place **before** session->cmdsn is incremented -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/4/2014 6:16 PM, Or Gerlitz wrote: > On 04/03/2014 16:44, Sagi Grimberg wrote: >>>> @@ -1707,10 +1707,17 @@ int iscsi_queuecommand(struct Scsi_Host >>>> *host, struct scsi_cmnd *sc) >>>> goto prepd_fault; >>>> } >>>> } >>>> - if (session->tt->xmit_task(task)) { >>>> - session->cmdsn--; >>>> - reason = FAILURE_SESSION_NOT_READY; >>>> - goto prepd_reject; >>>> + >>>> + reason = session->tt->xmit_task(task); >>>> + if (reason) { >>>> + if (reason == -ENOMEM || reason == -EAGAIN) { >>>> + session->cmdsn--; >>> >>> I am pretty sure this has to be done anyway, no matter why the >>> xmit_task callback failed >> >> Even if we abort? this just follows the same logic as >> iscsi_prep_scsi_cmd_pdu error flow. > > yes, take a 2nd look on iscsi_prep_scsi_cmd_pdu and you'll see that > all the possible error cases take place **before** session->cmdsn is > incremented Ahhh, yes... So dec the cmdsn regardless. Mike, are you on board with this? Sagi. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/04/2014 03:59 AM, Sagi Grimberg wrote: > On 3/4/2014 11:38 AM, Or Gerlitz wrote: >> On 03/03/2014 06:44, Mike Christie wrote: >>> The xmit_task callout does handle failures like EINVAL. If the above map >>> calls fail then you would get infinite retries. You would currently want >>> to do the mapping in the init_task callout instead. >>> >>> If it makes it easier on the driver implementation then it is ok to >>> modify the xmit_task callers so that they handle multiple error codes >>> for drivers like iser that have the xmit_task callout called from >>> iscsi_queuecommand. >> >> Mike, >> >> After looking on the code with Sagi, it seems to us that the correct >> way to go here, would be to enhance in iscsi_queuecommand the >> processing of the result returned by session->tt->xmit_task(task) to >> behave in a similar manner to how the return value of >> iscsi_prep_scsi_cmd_pdu() is treated. E.g for errors such as ENOMEM >> and EGAIN take the "reject" flow which would cause the SCSI midlayer >> to retry the command and for other return values go to the "fault" >> flow which will cause the ML to abort the command. >> >> Or. >> > > Yes, we were thinking about the following: > --- a/drivers/scsi/libiscsi.c > +++ b/drivers/scsi/libiscsi.c > @@ -1707,10 +1707,17 @@ int iscsi_queuecommand(struct Scsi_Host *host, > struct scsi_cmnd *sc) > goto prepd_fault; > } > } > - if (session->tt->xmit_task(task)) { > - session->cmdsn--; > - reason = FAILURE_SESSION_NOT_READY; > - goto prepd_reject; > + > + reason = session->tt->xmit_task(task); > + if (reason) { > + if (reason == -ENOMEM || reason == -EAGAIN) { > + session->cmdsn--; > + reason = FAILURE_SESSION_NOT_READY; > + goto prepd_reject; > + } else { > + sc->result = DID_ABORT << 16; > + goto prepd_fault; > + } Or is correct. If iscsi_prep_scsi_cmd_pdu is successful then it will increment cmdsn. In this code path for xmit_task above, we assume that the driver can either take the entire command and can send it here, or return error and we requeue. For your new error case where we cannot send the command due to a hard failure so we want to fail the command, then we still need to decrement the cmdsn or there would be a hole since it was never put on the wire. Also, it is probably safest to check for the error code you are adding support for: reason = session->tt->xmit_task(task); if (reason) { session->cmdsn--; if (reason == -EINVAL) { sc->result = DID_ABORT << 16; goto prepd_fault; } else { sc->result = DID_ABORT << 16; goto prepd_fault; } } -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/drivers/scsi/libiscsi.c +++ b/drivers/scsi/libiscsi.c @@ -1707,10 +1707,17 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc) goto prepd_fault; } } - if (session->tt->xmit_task(task)) { - session->cmdsn--; - reason = FAILURE_SESSION_NOT_READY; - goto prepd_reject; + + reason = session->tt->xmit_task(task); + if (reason) { + if (reason == -ENOMEM || reason == -EAGAIN) { + session->cmdsn--; + reason = FAILURE_SESSION_NOT_READY; + goto prepd_reject; + } else { + sc->result = DID_ABORT << 16; + goto prepd_fault; + } } } else {