On Tue, 2017-02-07 at 22:27 +0000, Bart Van Assche wrote: > On Tue, 2017-02-07 at 07:40 -0800, Nicholas A. Bellinger wrote: > > However, there is a basic fatal flaw in this approach > > with this patch as-is that results in se_cmd->finished never being able > > to complete for any se_cmd with CMD_T_ABORTED . > > > > For the above, core_tmr_abort_task() calls __target_check_io_state() to > > obtain a se_cmd->cmd_kref if the descriptor is able to be aborted and > > sets CMD_T_ABORTED. Then, wait_for_completion_timeout() sits in a while > > looping waiting for se_cmd->finished to be completed. > > > > However, the complete_all(&se_cmd->finished) you've added below is only > > invoked from the final se_cmd->cmd_kref callback in > > target_release_cmd_kref(). > > > > Which means core_tmr_abort_task() will always be stuck indefinitely on > > se_cmd->finished above, because the subsequent target_put_sess_cmd() > > after the wait_for_completion_timeout() is the caller responsible for > > doing the final kref_put(&se_cmd->cmd_kref, target_release_cmd_kref) > > to perform complete_all(&se_cmd->finished). > > A fix for this is in test. I will post a new patch as soon as my tests have > finished. Can you elaborate a bit on how exactly you've been testing these changes for the first order (handle ABORT_TASKs and LUN_RESET with outstanding backend I/O) and second order (handle session shutdown while order one is active) issues mentioned earlier..? My main concern is that your tests didn't catch the basic first order issue from the earlier review, or at least they didn't check for hung tasks or proper target shutdown after the test completed The nightly automated runs that we (Datera) for v4.1.y and v3.14.y production include the following: Provision 500 target endpoints + luns per node in a 2500 volume cluster. Run a mixed 4k random workload, and drive up the backend I/O latency to the point where open-iscsi starts to send ABORT_TASK (15 second default), LUN_RESET and target reset / session reinstatement (30 second default). This typically generates 1000's of ABORT_TASKs and session resets, and while that is going on we also perform explicit target migration via configfs (eg: third order issue), that performs target configfs shutdown while the first and second order issues are occurring. ESX is also a really good test for TMRs, because it generates ABORT_TASK after 5 seconds of backend I/O latency, as is very aggressive in attempting to bring the device back online via repeated TMRs. So I don't think you need 100s of LUNs to verify these changes, but I'd at least like you to verify with a few dozen target + LUNs with a mixed random small block workload with a sufficiently deep iodepth (say 16), and ensure at least a few hundred ABORT_TASK, LUN_RESET and session shutdowns are really occurring during your test. Once the test is completed, perform an explicit fabric logout and target shutdown. This is to ensure there are no reference leaks, kernel hung tasks in un-interruptible sleep, or other memory leaks that have been introduced by the series. -- 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
