On Thu, 2017-02-09 at 00:58 -0800, Nicholas A. Bellinger wrote: > Hi Bart, > > So I'll be going through this entire series again with a fined tooth > comb, top to bottom. > > Also, since I have close to zero confidence now after the obvious fatal > issues with -v2 that the larger changes here have seen any type of > rigorous Q/A and/or automation regression testing. > > That said, I'll go ahead and pick up the straight-forward changes for > target-pending/for-next, and take the rest from there. > > Also, I'll be commenting on how the patch series itself is structured, > etc. > > On Wed, 2017-02-08 at 14:24 -0800, Bart Van Assche wrote: > > The most important changes in this patch series are: > > * ABORT and LUN RESET handling is made synchronous. > > * LUN lookup during ABORT handling is moved from the tcm_qla2xxx > > driver into the target core. > > * A deadlock in the XCOPY implementation has been fixed. > > * The SCSI target core is simplified significantly. > > > > This is far too vague a description for a significant set of changes > like this, and far too significant a set of changes to include in one > large patch series. > > Each of these bullets need to be it's own patch series. > > Also, the cover letter for each must include: > > If it's bug fix, describe the scenario when bug occurs, and how it > manifests itself to users. Also describe how the bug has been > addressed, and the possible approaches considered. > > If it's improvements, describe how it improves the code, and the testing > that has been done to ensure it doesn't introduce regressions. For > former, obviously this can be subjective. > > If it's simple or obvious cleanups, then it's much less important. > > > These patches have been tested against the iSCSI, ib_srpt and > > tcm_qla2xxx target drivers. > > > > This is also far to vague to be useful for significant changes. > > Instead, describe what you've tested, eg: > > - Was active I/O shutdown tested..? > - Which TMRs and how many times they where triggered..? > - How many volumes where tested concurrently..? > - With what workload..? > - With what types of hosts..? > - How many nodes where used..? > - And for how long did the test run..? > > > Please review the patches that have not yet been reviewed, > > I'll be responding inline to each patch, and giving a summary here at > the end. Hi Bart, As promised, here is the summary for -v4 review, based on the commit's in your linux-next branch: Patch #1 - dcf7dd7 target/iscsi: Fix spelling of "perform" Applied Patch #2 - 6aeb10b target/iscsi: Fix spelling of "reallegiance" Applied Patch #3 - 74af7eb target/iscsi: Introduce a helper function for TMF translation Applied. Patch #4 - c28a4f6 target/iscsi: Fix iSCSI task reassignment handling Applied, but need testing info. Patch #5 - 7d1b85a target: Remove se_tmr_req.tmr_lun Applied Patch #6 - e1657ad target: Make core_tmr_abort_task() consider all commands Applied Patch #7 - ad4774f target: Introduce a new workqueue for TMF processing Scale issues with global alloc_workqueue() + destroy_workqueue() synchronization for all sessions across all endpoints. Dropped. Patch #8 - 0d79190 target: Rename tmr_wq into alua_wq Depends on ad4774f, ignoring Patch #9 - b041c83 target: Make it possible to specify I_T nexus for SCSI abort Deadlock in target_core_tmr.c code. Dropped Patch #10 - ae4f59d tcm_qla2xxx: Let the target core look up the LUN of the aborted cmd Depends on b041c83, ignoring. Patch #11 - 40c920f target: Correct transport_wait_for_tasks() documentation Applied Patch #12 - 11d0be0 target: Remove an overly chatty debug message Applied Patch #13 - 20179e3 target: Stop execution if CMD_T_STOP has been set Applied Patch #14 - 0323a74 target: Use the command reference counting mechanism consistently Breaks existing code due to flaws in assumptions about how forward process works. Dropped. Patch #15 - 65c062f7 target: Inline transport_cmd_check_stop() Applied with minor fuzz Patch #16 - 81c7658 target: Move session check from target_put_sess_cmd() into target_release_cmd_kref() Applied Patch #17 - 24b6ebc target: Introduce a target driver reference count per command Concerns about adding another per se_cmd reference count and struct completion. Dropped. Patch #18 - 1e974ed target: Make ABORT and LUN RESET handling synchronous Changes don't actually abort in flight I/O, but waits until the command completes. Removal of CMD_T_STOP checks in target_complete_cmd() breaks non SAM_STAT_GOOD commands. Also removal of se_tfo->write_pending_status() breaks multiple drivers that must wait for WRITE transfer to complete before proceeding with CMD_T_ABORT. Dropped. Patch #19 - 606d26c target: Simplify session shutdown tests Breaks iscsi-target which doesn't set se_sess->sess_tearing_down. Dropped. Patch #20 - fddf38b target: Simplify session shutdown code Depends on changes from commit 1e974ed. Ignored. Patch #21 - 344efbd target: Remove the SCF_SEND_DELAYED_TAS command flag Removes delayed task aborted status, that multiple drivers depend upon to wait for WRITE transfer to complete, before proceeding with CMD_T_ABORTED. Dropped. Patch #22 - 0bd8c98 target: Inline transport_check_aborted_status() Depends on commit 344efbd, and further removes DELAYED_TAS with no alternative. Dropped. Patch #23 - de2b657 target: Remove the write_pending_status() callback function Breaks qla2xxx for CTIO completion and iscsi-target solicited data-out WRITE transfer completion during CMD_T_ABORTED, with no alternative. Dropped. Patch #24 - be28aca target: Remove several state tests from TMF code Breaks iscsi-target second order scenario where CMD_T_ABORTED blocks waiting for outstanding backend I/o to complete, and session logout or reinstatement happens concurrently. Dropped. Patch #25 - 1e3e187 target: Remove command flag CMD_T_BUSY Applied Patch #26 - b502e98 target: Simplify LUN RESET implementation Breaks iscsi-target second order scenario where CMD_T_ABORTED blocks waiting for outstanding backend I/o to complete, and session logout or reinstatement happens concurrently. Dropped. Patch #27 - c79fac3 target: Remove command flag CMD_T_DEV_ACTIVE Applied Patch #28 - 1de03aa target: Reduce number of __transport_wait_for_tasks() arguments Depends on dropped patch b502e98. Ignoring. Patch #29 - 5aa8339 target: Remove command flag CMD_T_TAS Depends on dropped patch 1e974ed. Ignoring. Patch #30 - e1d04feb target: Remove unused arguments from __target_check_io_state() Depends on dropped patch 1e974ed. Ignoring. Patch #31 - 9e6394f target: Change return type of transport_wait_for_tasks() into void Depends on dropped patch 1e974ed. Ignoring. Patch #32 - 1a7fea8 target: Inline transport_put_cmd() Depends on dropped patch 1e974ed. Ignoring. Patch #33 - 20445b5 target: Inline transport_lun_remove_cmd() Breaks existing logic where configfs LUN shutdown must be able to proceed, without waiting for fabric ACKs to perform final se_cmd->cmd_kref put. Patch #34 - ade1501 target: Move target_remove_from_state_list() into target_release_cmd_kref() Breaks existing logic where se_cmd must be removed from se_device->active_list, without waiting for fabric ACKs to perform final se_cmd->cmd_kref put. Patch #35 - b79cfc0 configfs: Introduce config_item_get_unless_zero() No reviews - Needs ACK fro configfs folks Patch #36 - 154a6cb target: Introduce target_get_device() and target_put_device() Looks OK, but depends on commit b79cfc0 Patch #37 - 6de3e93 target: Avoid that XCOPY commands trigger a deadlock Bug-fix looks OK, but depends on commit b79cfc0. -- 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
