On Sun, Oct 09, 2011 at 01:26:43AM -0700, Nicholas A. Bellinger wrote: > > 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' A quick count (before your recent changes) sais that we have 13 callers that simply want to free the command, and just 7 that also want to free. In general keeping unrelated functionaly un-entangled is the best way to maintain both maintainability and a chance for people to actually understand what is going on. > 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(). As mentioned in my other mail all iscsi invocation should really go through a helper, so doing two callers there instead of one is not a big difference. > 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. Yeah, that's a good start. I'll send my variant as an RFC later, then we an see if it really is as bad as it sounds (which I doubt :)) > > 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. 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? Also I think at this point we'd be better off at least removing SCF_SE_LUN_CMD and always directly checking for ->se_lun so that there aren't two different ways to express the same thing. 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. -- 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
