On Fri, 2011-11-04 at 16:15 +0100, Christoph Hellwig wrote:
> On Fri, Nov 04, 2011 at 02:14:27PM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
> >
> > This patch removes legacy usage of PYX_TRANSPORT_* return codes in a number
> > of locations and addresses cases where transport_generic_request_failure()
> > was returning the incorrect sense upon CHECK_CONDITION status after the
> > v3.1 converson to use errno return codes.
> >
> > This includes the conversion of transport_generic_request_failure() to
> > process cmd->scsi_sense_reason and handle extra TCM_RESERVATION_CONFLICT
> > before calling transport_send_check_condition_and_sense() to queue up
> > response status. It also drops PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES legacy
> > usgae, and returns TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE w/ a response
> > for these cases.
> >
> > transport_generic_allocate_tasks(), transport_generic_new_cmd(), backend
> > SCF_SCSI_DATA_SG_IO_CDB ->do_task(), and emulated ->execute_task() have
> > all been updated to set se_cmd->scsi_sense_reason and return errno codes
> > universally upon failure. This includes cmd->scsi_sense_reason assignment
> > in target_core_alua.c, target_core_pr.c and target_core_cdb.c emulation code.
> >
> > Finally it updates fabric modules to remove the legacy usage, and for
> > TFO->new_cmd_map() callers forwards return values outside of fabric code.
> > iscsi-target has also been updated to remove a handful of special cases
> > related to the cleanup, along w/ signaling QUEUE_FULL handling for
> > ft_write_pending() and ibmvscsis_write_pending() fabric callers.
>
> I really like the simplication by removing various layers of error
> codes, but I wonder if it's still one too much. What prevents
> us from returning the sense codes in-line from the submission functions
> instead of returning some negative error value and then looking at
> scsi_sense_reason?
>
Well, I think the removal of target specific returns universally cleans
things up (more) from the perspective of fabric module code. I don't
really see a reason for doing this anymore if cmd->scsi_sense_reason is
set directly by target core, and if fabric module code is just checking
for non zero or less than zero return values to determine exception
status.
> A few additional comments below:
>
> > + if (ret < 0 || cmd->se_cmd.se_cmd_flags & SCF_SCSI_CDB_EXCEPTION) {
>
> It's generally safer to keep the braces around & expressions.
>
Actually, I think this SCF_SCSI_CDB_EXCEPTION checking is left over
cruft from before where iscsi-target did a context switch on write to
wait for memory allocation to complete.
Double checking and removing this now..
> > if (cmd->t_tasks_failed) {
> > - if (!task->task_error_status) {
> > - task->task_error_status =
> > - PYX_TRANSPORT_UNKNOWN_SAM_OPCODE;
> > - cmd->transport_error_status =
> > - PYX_TRANSPORT_UNKNOWN_SAM_OPCODE;
> > - }
> > + if (!task->task_error_status)
> > + cmd->scsi_sense_reason =
> > + TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> > +
>
> task_error_status is never set after your patch.
>
Whoops. I'm not sure if we really have a functional need for
task->task_error_status anymore, but it still might be useful for
debugging purposes. I'll look at dropping this as well.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
