On Sat, 2011-10-08 at 22:24 -0400, Christoph Hellwig wrote: > 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. > I'd much rather just make a single call here to handle both instead of requiring two calls in fabric code for 'wait for outstanding tasks' and 'free se_cmd descriptor resources' > > > > > 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. > I think it's cleaner to just call transport_generic_free_cmd() w/ an internal invocation of transport_generic_wait_for_tasks(). The vast majority of iscsi-target usage expects this anyways, and all other fabrics are already calling transport_generic_free_cmd(). I've gone ahead and removed usage of se_cmd->transport_wait_for_tasks() from iscsi-target in favor of transport_generic_free_cmd(cmd, 1), and added an external call to transport_generic_wait_for_tasks() for the one case ub uscsu0target where se_cmd->transport_wait_for_tasks(se_cmd, 0) was being called. > 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. > I'm also sending out a patch to clean up SCF_SE_LUN_CMD abuses in iscsi-target, instead keying off iscsi_cmd->iscsi_opcode to determine when transport_generic_free_cmd() or iscsit_release_cmd() should be called. However, one important thing that I forgot to mention is that target core still needs this flag in some areas because se_cmd->se_lun actually gets cleared in transport_cmd_check_stop() before handing off back to fabric for processing. --nab -- 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
