| Message ID | 1496468143.27407.302.camel@haakon3.risingtidesystems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2017-06-02 at 22:35 -0700, Nicholas A. Bellinger wrote: > On Fri, 2017-06-02 at 17:11 +0000, Bart Van Assche wrote: > > On Thu, 2017-06-01 at 21:44 -0700, Nicholas A. Bellinger wrote: > > > On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote: > > > > Introduce a function that sends the SCSI status "BUSY" back to the > > > > initiator. > > > > > > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > > > Reviewed-by: Hannes Reinecke <hare@suse.com> > > > > Cc: Christoph Hellwig <hch@lst.de> > > > > Cc: Andy Grover <agrover@redhat.com> > > > > Cc: David Disseldorp <ddiss@suse.de> > > > > --- > > > > drivers/target/target_core_transport.c | 10 ++++++++++ > > > > include/target/target_core_fabric.h | 1 + > > > > 2 files changed, 11 insertions(+) > > > > > > > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > > > > index cfe69c1fc879..c28e3b58150b 100644 > > > > --- a/drivers/target/target_core_transport.c > > > > +++ b/drivers/target/target_core_transport.c > > > > @@ -3130,6 +3130,16 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd, > > > > } > > > > EXPORT_SYMBOL(transport_send_check_condition_and_sense); > > > > > > > > +int target_send_busy(struct se_cmd *cmd) > > > > +{ > > > > + WARN_ON_ONCE(cmd->se_cmd_flags & SCF_SCSI_TMR_CDB); > > > > + > > > > + cmd->scsi_status = SAM_STAT_BUSY; > > > > + trace_target_cmd_complete(cmd); > > > > + return cmd->se_tfo->queue_status(cmd); > > > > +} > > > > +EXPORT_SYMBOL(target_send_busy); > > > > + > > > > > > This doesn't handle queue-full, or any of the other interesting failure > > > conditions. > > > > > > It really needs to be using the normal completion path that already > > > handles all of those cases instead. > > > > Hello Nic, > > > > Have you noticed that this function is *only* used if target_submit_tmr() > > fails and that the normal completion path is not triggered at all if > > target_submit_tmr() fails? > > Doesn't matter. If common code is added to target-core it needs to work > for all cases, and handle error conditions. > > > > > > That said, MNC and I are working on a patch to allow this. > > > > Which patch are you referring to? Has it already been posted on the > > target-devel mailing list? > > > > Yes. > > The bottom part of http://www.spinics.net/lists/target-devel/msg11835.html > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 6becc94..eb12ae2 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -732,11 +732,20 @@ static unsigned char *transport_get_sense_buffer(struct se_cmd *cmd) > void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) > { > struct se_device *dev = cmd->se_dev; > - int success = scsi_status == GOOD; > + int success; > unsigned long flags; > > cmd->scsi_status = scsi_status; > - > + switch (cmd->scsi_status) { > + case SAM_STAT_GOOD: > + case SAM_STAT_BUSY: > + case SAM_STAT_TASK_SET_FULL: > + success = 1; > + break; > + default: > + success = 0; > + break; > + } > > spin_lock_irqsave(&cmd->t_state_lock, flags); > cmd->transport_state &= ~CMD_T_BUSY; > > And more recently MNC's version: > > http://www.spinics.net/lists/target-devel/msg15436.html Sorry but you make me repeat my previous reply: both patches you referred to are only relevant if target_submit_tmr() has *succeeded*. Patches 12 and 13 in this series are for the case when target_submit_tmr() *failed*. So please reconsider these patches. Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/05/2017 11:24 AM, Bart Van Assche wrote: > On Fri, 2017-06-02 at 22:35 -0700, Nicholas A. Bellinger wrote: >> On Fri, 2017-06-02 at 17:11 +0000, Bart Van Assche wrote: >>> On Thu, 2017-06-01 at 21:44 -0700, Nicholas A. Bellinger wrote: >>>> On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote: >>>>> Introduce a function that sends the SCSI status "BUSY" back to the >>>>> initiator. >>>>> >>>>> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> >>>>> Reviewed-by: Hannes Reinecke <hare@suse.com> >>>>> Cc: Christoph Hellwig <hch@lst.de> >>>>> Cc: Andy Grover <agrover@redhat.com> >>>>> Cc: David Disseldorp <ddiss@suse.de> >>>>> --- >>>>> drivers/target/target_core_transport.c | 10 ++++++++++ >>>>> include/target/target_core_fabric.h | 1 + >>>>> 2 files changed, 11 insertions(+) >>>>> >>>>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c >>>>> index cfe69c1fc879..c28e3b58150b 100644 >>>>> --- a/drivers/target/target_core_transport.c >>>>> +++ b/drivers/target/target_core_transport.c >>>>> @@ -3130,6 +3130,16 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd, >>>>> } >>>>> EXPORT_SYMBOL(transport_send_check_condition_and_sense); >>>>> >>>>> +int target_send_busy(struct se_cmd *cmd) >>>>> +{ >>>>> + WARN_ON_ONCE(cmd->se_cmd_flags & SCF_SCSI_TMR_CDB); >>>>> + >>>>> + cmd->scsi_status = SAM_STAT_BUSY; >>>>> + trace_target_cmd_complete(cmd); >>>>> + return cmd->se_tfo->queue_status(cmd); >>>>> +} >>>>> +EXPORT_SYMBOL(target_send_busy); >>>>> + >>>> >>>> This doesn't handle queue-full, or any of the other interesting failure >>>> conditions. >>>> >>>> It really needs to be using the normal completion path that already >>>> handles all of those cases instead. >>> >>> Hello Nic, >>> >>> Have you noticed that this function is *only* used if target_submit_tmr() >>> fails and that the normal completion path is not triggered at all if >>> target_submit_tmr() fails? >> >> Doesn't matter. If common code is added to target-core it needs to work >> for all cases, and handle error conditions. >> >>> >>>> That said, MNC and I are working on a patch to allow this. >>> >>> Which patch are you referring to? Has it already been posted on the >>> target-devel mailing list? >>> >> >> Yes. >> >> The bottom part of http://www.spinics.net/lists/target-devel/msg11835.html >> >> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c >> index 6becc94..eb12ae2 100644 >> --- a/drivers/target/target_core_transport.c >> +++ b/drivers/target/target_core_transport.c >> @@ -732,11 +732,20 @@ static unsigned char *transport_get_sense_buffer(struct se_cmd *cmd) >> void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) >> { >> struct se_device *dev = cmd->se_dev; >> - int success = scsi_status == GOOD; >> + int success; >> unsigned long flags; >> >> cmd->scsi_status = scsi_status; >> - >> + switch (cmd->scsi_status) { >> + case SAM_STAT_GOOD: >> + case SAM_STAT_BUSY: >> + case SAM_STAT_TASK_SET_FULL: >> + success = 1; >> + break; >> + default: >> + success = 0; >> + break; >> + } >> >> spin_lock_irqsave(&cmd->t_state_lock, flags); >> cmd->transport_state &= ~CMD_T_BUSY; >> >> And more recently MNC's version: >> >> http://www.spinics.net/lists/target-devel/msg15436.html > > Sorry but you make me repeat my previous reply: both patches you referred to > are only relevant if target_submit_tmr() has *succeeded*. Patches 12 and 13 > in this series are for the case when target_submit_tmr() *failed*. So please > reconsider these patches. > Hey, If it is for when target_submit_cmd fails, then my patches do not handle that case. The problem is that right now as you guys know that function does some cmd setup, like it calls transport_init_se_cmd, but other things like being on the sess_cmd_list is not done. With my patches if you tried to do: if (target_submit_cmd(cmd....)) target_complete_cmd(cmd, SAM_STAT_BUSY)) then I think we have these issues still: 1. The API seems dangerous. The caller has to assume some parts of the command got setup even though the submit call failed. 2. I think we need some other bits set right? se_cmd_list is only initialized, so I was not sure what happens if the status fails to send and we drop down to the QF retry handling and that keeps failing. If we get an abort for that command but it never made it to the sess_cmd_list, would the abort code send a function complete response for the abort, but we could still have the command running in the QF retry code? You might then get a status for a command after a successful abort response which will throw off initiators. My end goal for the patchset was to make it so target_complete_cmd/transport_generic_request_failure could be called by core code, backend and fabric modules and it would just work for the caller. But, I punted on Bart's case because my patchset was getting big and there are these open issues still: 1. Why are some modules sending sense through lio target functions and some calling their status functions directly. It did not seem like a SCSI spec reason for this and was just whatever the fabric module maintainer decided on. In the end they just needed their some failure value returned and their command related structs cleaned up. 2. We could make target_complete_cmd support this case, or we could add a wrapper that initializes the needed fields or add checks in the completion path for partially setup commands. 3. I actually liked Bart's comment about if the initial submission failed, and sending the status/sense failed, then it would be ok for the driver to just drop the command and clean up internally. When we eventually get an abort, we can figure things out. However, it would be nice, if we can at least get all fabric modules doing the same thing so we do not have so many different cases. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 6becc94..eb12ae2 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -732,11 +732,20 @@ static unsigned char *transport_get_sense_buffer(struct se_cmd *cmd) void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status) { struct se_device *dev = cmd->se_dev; - int success = scsi_status == GOOD; + int success; unsigned long flags; cmd->scsi_status = scsi_status; - + switch (cmd->scsi_status) { + case SAM_STAT_GOOD: + case SAM_STAT_BUSY: + case SAM_STAT_TASK_SET_FULL: + success = 1; + break; + default: + success = 0; + break; + } spin_lock_irqsave(&cmd->t_state_lock, flags); cmd->transport_state &= ~CMD_T_BUSY;