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?
> > 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.
> > 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. 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.
--
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
