| Message ID | 20170523234854.21452-13-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/23/2017 06:48 PM, 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); > +} Hey, How are drivers supposed to handler queue_status failures or should that not happen when this is called? If it can happen, do they just drop the command? Is the tmr code able to figure this out? Will it not even be on the sess_cmd_list so would hit the does not exist tmr abort code path, or will it just sit in transport_wait_for_tasks? Are drivers supposed to call transport_handle_queue_full to queue up retrying sending something? -- 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 Wed, 2017-05-24 at 17:28 -0500, Mike Christie wrote: > On 05/23/2017 06:48 PM, 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); > > +} > > Hey, > > How are drivers supposed to handler queue_status failures or should that > not happen when this is called? > > If it can happen, do they just drop the command? Is the tmr code able to > figure this out? Will it not even be on the sess_cmd_list so would hit > the does not exist tmr abort code path, or will it just sit in > transport_wait_for_tasks? Are drivers supposed to call > transport_handle_queue_full to queue up retrying sending something? Hello Mike, This function is intended to be used only if target_submit_cmd*() fails. If target_submit_cmd*() fails se_cmd won't be on sess_cmd_list. If the .queue_status() call from target_send_busy() fails it is safe to ignore that failure because the initiator will resend the command anyway after the command timeout has expired. The function transport_handle_queue_full() is intended for commands that have been submitted to the target core through target_submit_cmd*(). I don't think it is safe to call that function if target_submit_cmd*() failed. 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 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. That said, MNC and I are working on a patch to allow this. Until then dropping this for now. -- 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 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? > 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? 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
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); + static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status) __releases(&cmd->t_state_lock) __acquires(&cmd->t_state_lock) diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h index 33d2e3e5773c..77a6cebf5a8a 100644 --- a/include/target/target_core_fabric.h +++ b/include/target/target_core_fabric.h @@ -156,6 +156,7 @@ bool transport_wait_for_tasks(struct se_cmd *); int transport_check_aborted_status(struct se_cmd *, int); int transport_send_check_condition_and_sense(struct se_cmd *, sense_reason_t, int); +int target_send_busy(struct se_cmd *cmd); int target_get_sess_cmd(struct se_cmd *, bool); int target_put_sess_cmd(struct se_cmd *); void target_sess_cmd_list_set_waiting(struct se_session *);