On Mon, 2011-10-10 at 10:48 -0400, Christoph Hellwig wrote:
> On Sun, Oct 09, 2011 at 11:18:40PM -0700, Nicholas A. Bellinger wrote:
> > I really don't think having an optional call in the 'free se_cmd
> > descriptor resources' path to 'wait_for_tasks' is considered entangled.
> > It's something that is an optional safety check to ensure target-core is
> > done with it's own internal processing before releasing a command.
> >
> > I've considering just making 'wait_for_tasks' mandatory in
> > transport_generic_free_cmd(), but the issue is that this would require
> > taking se_cmd->t_state_lock in order to check active state. I would
> > rather not endure the extra overhead of disabling interrupts for
> > se_cmd->t_state_lock (again) when a fabric actually knows that the path
> > calling transport_generic_free_cmd() is after TFO->queue_status(),
> > TFO->queue_data_in() or something else in the normal release path after
> > target-core has finished it's own internal processing.
> >
> > Where transport_generic_free_cmd(se_cmd, 1) is useful is where the
> > fabric caller is not sure if target-core has finished or not..
>
> Are you find with replacing the current
>
> void transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
> {
> ..
> }
>
> with:
>
> void target_free_cmd_nowait(struct se_cmd *cmd)
> {
> ..
> }
>
> and
>
> target_generic_free_cmd(struct se_cmd *cmd)
> {
> target_wait_for_tasks(cmd);
> target_free_cmd_nowait(cmd);
> }
>
> including comments documenting their uses properly?
>
Sounds reasonable. Are you going to include this in your RFC..?
> > > That is a fairly nasty thing to start with btw, do you remember why
> > > you did, and why it's only done for the ->t_transport_stop path, but not
> > > the ->transport_lun_stop path?
> > >
> >
> > Absoulutely. This is because clearing se_cmd->se_lun means that during
> > active I/O se_lun shutdown we don't have to track that command any
> > longer. Eg: The se_cmd->se_lun pointer has been cleared, and tasks
> > removed from se_device->state_task_list and handed back to the fabric so
> > that target-core doesn't need to try to account for it's outstanding
> > processing any longer during se_lun shutdown.
> >
> > This gets incredibly complicated because we don't know when, or if the
> > acknowledgment of the se_cmd (for iscsi-target) will actually occur, or
> > when we can safely preform active I/O se_lun shutdown while there are
> > still se_cmd->se_lun pointer references active waiting for said
> > acknowledgments after the response hand-off back to fabric code.
>
> Please add a comment describing this to the code.
>
Ok, will add a docbook comment to transport_cmd_check_stop() describing
the processing of clearing se_cmd->se_lun for fabric response hand-off.
> > > I'm also a bit worried about the SCF_SUPPORTED_SAM_OPCODE flag that
> > > had been around and got another user in this series. Why would we
> > > need a flag that a command actually is valid, it seems like we could
> > > instead check for TCM_UNSUPPORTED_SCSI_OPCODE or TCM_INVALID_CDB_FIELD
> > > on command that returned an error from transport_generic_allocate_tasks.
> > >
> >
> > Because SCF_SUPPORTED_SAM_OPCODE is a single check that can be used to
> > determine when transport_generic_cmd_sequencer() has succeeded, and to
> > determine when transport_wait_for_tasks() can safely be called. Adding
> > multiple checks for TCM_UNSUPPORTED_SCSI_OPCODE or TCM_INVALID_CDB_FIELD
> > or any other (new) failure case intransport_generic_cmd_sequencer just
> > adds more possible failures states to keep track of here.
>
> One major thing I fail to grasp is why we would ever have a command
> that gets rejected from the sequencer on the target core lists. In
> any other code I deal with we try to never have commands that were
> rejected early on to stay around.
Those fabrics using TRANSPORT_NEW_CMD_MAP following the path:
transport_processing_thread() ->
TFO->new_cmd_map() ->
transport_generic_allocate_tasks() ->
transport_generic_cmd_sequencer()
are removed from target core lists during TFO->new_cmd_map failure in
transport_generic_cmd_sequencer following:
transport_generic_request_failure() ->
.. Call to transport_send_check_condition_and_sense() ..
.. Call to transport_lun_remove_cmd() ..
transport_cmd_check_stop_to_fabric() ->
transport_cmd_check_stop()
Of course, iscsi-target is the one fabric that does not use
TRANSPORT_NEW_CMD_MAP, and has cases with immediate data where
transport_send_check_condition_and_sense() cannot be called immediately.
The classic example is during a transport_generic_cmd_sequencer()
failure where we have to receive the immediate data payload before
sending the CHECK_CONDITION response.
So that said, it does appear that an cmd sequencer failure with
iscsi-target + direct call to transport_send_check_condition_and_sense()
will not call transport_lun_remove_cmd() in the immediate exception
path, and will wait until transport_generic_free_cmd() is invoked from
the release path before dropping se_cmd from se_lun->lun_cmd_list.
I'm not sure how much of a problem this is actually is in practice, but
the main issue involved for iscsi-target is still the cases where we
have to wait for something else to happen (like immediate data payload)
after the transport_generic_cmd_sequencer() failure has occurred before
calling transport_send_check_condition_and_sense() to push the response
onto the fabric.
> This also relates to the
> SCF_SE_LUN_CMD, as it appears to be used for similar purposes in
> some areas of the iscsi target as far as I can see.
>
In the iscsi-target patch to remove SCF_SE_LUN_CMD abuses, it's usage
has been completely dropped:
debian-amd64:/usr/src/lio-core-2.6.git# grep SCF_SE_LUN_CMD drivers/target/iscsi/*.[c,h]
debian-amd64:/usr/src/lio-core-2.6.git#
--
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
