On Mon, 2017-04-17 at 21:33 +0000, Bart Van Assche wrote: > On Sun, 2017-04-02 at 15:38 -0700, Nicholas A. Bellinger wrote: > > No. Your assumption is incorrect wrt transport_generic_free_cmd() > > having a wrong return value during LUN_RESET. > > > > It's incorrect because when iscsit_free_cmd() is called with > > shutdown=true resulting in transport_generic_free_cmd() with > > wait_for_tasks=true, target_wait_free_cmd() checks for CMD_T_ABORTED to > > determine if se_cmd has been aborted by target_core_tmr.c logic, and > > returns aborted=true back up to transport_generic_free_cmd(). > > > > When transport_generic_free_cmd() gets aborted=true, it waits for > > se_cmd->cmd_wait_comp to finish and calls cmd->se_tfo->release_cmd() > > to release se_cmd back to the pre-allocated session pool. > > > > At this point, transport_generic_free_cmd() with aborted=true must > > return '1' it's caller, signaling iscsit_free_cmd() to not attempt to > > perform the extra __iscsi_free_cmd() or target_put_sess_cmd(), because > > se_cmd has already been released back to the session pool. > > Hello Nic, > > Today the iSCSI target driver relies on transport_generic_free_cmd() to figure > out whether target_put_sess_cmd() has been called once or twice for a SCSI > command. transport_generic_free_cmd() relies on the command reference count and > the "aborted" flag to perform that check. I think that's an unfortunate design > choice because it makes it impossible to use the command reference count outside > any context where it is used today (calling .release_cmd() upon final put / TMF > handling), e.g. to simplify the implementation of target_wait_for_sess_cmds(). > This choice also makes it necessary to perform direct calls to .release_cmd() > from several functions, something that is really unfortunate. I think it is > important to make all that code easier to understand and to verify. Hence my > proposal to introduce a second reference count that tracks the number of > references a target driver holds on a SCSI command. Such a patch has been posted > before. See also "target: Introduce a target driver reference count per command" > (February 9, 2017, https://www.spinics.net/lists/target-devel/msg14487.html). Is > it OK for you if I rework my most recently posted iSCSI target driver patch > series on top of that patch? There is no valid purpose to introduce a second se_cmd kref, and you've not been able to articulate a reason for doing so other than 'it makes the code easier to read'. So no, unless there is a specific bug that absolutely cannot be addressed without adding a second se_cmd kref, I'll not be considering these patches for reasons I've mentioned numerous times already. -- 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
