On Sat, Oct 08, 2011 at 06:48:21PM -0700, Nicholas A. Bellinger wrote: > > Another thing I would really like to to is to clean > > upcmd->transport_wait_for_tasks. It is a fairly odd use of a callback > > in every cmd, which essentially replaces a try-state: > > > > - noop if explicitly set by assigning transport_nop_wait_for_tasks > > - finish sutdown if explicitly assigning transport_lun_wait_for_tasks > > - > > > > I think you mean transport_generic_wait_for_tasks() here.. Yes. > > In generic code we only call it with both flags sent to 0/false from > > transport_generic_free_cmd, but the iSCSI target uses all variants, > > and passes the arguments. > > > > Now that the session_reinstatement bit is gone, iscsi-target will only > call this to signal 'Stop the command', or 'Stop the command, and > release it directly via transport_generic_free_cmd()'. Yes. And now that session reinstatement is gone the call to transport_generic_free_cmd is the last thing it does. Which means we can apply the bool return value trick again and move the actual removal to the caller. > > It seems to me like we should simple have flags in the se_cmdn to > > not wait, and one inside the iscsi code for the current semantics if > > neither callback is set (if that is indeed still needed), and then > > call the 0/0 variant directly from transport_generic_free_cmd. > > > > I think it should be OK to call transport_generic_free_cmd() directly > here, get rid of the se_cmd->transport_wait_for_tasks() pointer, and the > 'int remove' parameter from transport_generic_wait_for_tasks() as well. Or do it exactly the other way around, as I suggested above. Making a _wait_for_tasks routine do just that and not also conditionally free the command seems like a fairly sensible idea. In fact all but one iscsi callers are in a weird form where they also free the commands directly if a few conditions don't fit, including ->transport_wait_for_tasks not beeing set at all and the infamous SCF_SE_LUN_CMD flag. I think moving the freeing always to the caller, and making it more sensible would be a good idea. -- 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
