On Sun, 2011-10-09 at 08:14 -0400, Christoph Hellwig wrote: > 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 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.. > > 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. > Fixed > > 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 :)) > Fair enough > > > 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? > 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. > 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. > Clearing se_cmd->se_lun allows for a straight-forward way for target-core to perform active I/O shutdown of se_lun. Keeping this pointer around after the fabric hand-off is way, way more of a headache that it's worth in order to simply remove SCF_SE_LUN_CMD. Please have a deeper look at what's required to perform active I/O se_lun shutdown before attempting this type of conversion. > 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. --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
