Message ID | 0C7C69F2-1753-46CE-BC8B-7959CE2962F1@delphix.com |
---|---|
State | New |
Headers | show |
Series | scsi: Revert "target/core: Inline transport_lun_remove_cmd()" | expand |
On Fri, Feb 28, 2020 at 04:17:56PM -0500, Pavel Zakharov wrote: > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux-hwe/+bug/1862682 > > scsi: Revert "target/core: Inline transport_lun_remove_cmd()" > > Commit 83f85b8ec305 postponed the percpu_ref_put(&se_cmd->se_lun->lun_ref) > call from command completion to the time when the final command reference > is dropped. That approach is not compatible with the iSCSI target driver > because the iSCSI target driver keeps the command with the highest stat_sn > after it has completed until the next command is received (see also > iscsit_ack_from_expstatsn()). Fix this regression by reverting commit > 83f85b8ec305. It looks like this patch landed upstream. I think we can add: (cherry picked from commit c14335ebb92a98646ddbf447e6cacc66de5269ad) Apart than that looks good to me, so: Acked-by: Andrea Righi <andrea.righi@canonical.com> > > Fixes: 83f85b8ec305 ("scsi: target/core: Inline transport_lun_remove_cmd()") > Cc: Pavel Zakharov <pavel.zakharov@delphix.com> > Cc: Mike Christie <mchristi@redhat.com> > Cc: <stable@vger.kernel.org> > Link: https://lore.kernel.org/r/20200210051202.12934-1-bvanassche@acm.org > Reported-by: Pavel Zakharov <pavel.zakharov@delphix.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> > --- > drivers/target/target_core_transport.c | 31 +++++++++++++++++++++++--- > 1 file changed, 28 insertions(+), 3 deletions(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index eda8b4736c15..d542e26ca56a 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -666,6 +666,11 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd) > > target_remove_from_state_list(cmd); > > + /* > + * Clear struct se_cmd->se_lun before the handoff to FE. > + */ > + cmd->se_lun = NULL; > + > spin_lock_irqsave(&cmd->t_state_lock, flags); > /* > * Determine if frontend context caller is requesting the stopping of > @@ -693,6 +698,17 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd) > return cmd->se_tfo->check_stop_free(cmd); > } > > +static void transport_lun_remove_cmd(struct se_cmd *cmd) > +{ > + struct se_lun *lun = cmd->se_lun; > + > + if (!lun) > + return; > + > + if (cmpxchg(&cmd->lun_ref_active, true, false)) > + percpu_ref_put(&lun->lun_ref); > +} > + > static void target_complete_failure_work(struct work_struct *work) > { > struct se_cmd *cmd = container_of(work, struct se_cmd, work); > @@ -783,6 +799,8 @@ static void target_handle_abort(struct se_cmd *cmd) > > WARN_ON_ONCE(kref_read(&cmd->cmd_kref) == 0); > > + transport_lun_remove_cmd(cmd); > + > transport_cmd_check_stop_to_fabric(cmd); > } > > @@ -1695,6 +1713,7 @@ static void target_complete_tmr_failure(struct work_struct *work) > se_cmd->se_tmr_req->response = TMR_LUN_DOES_NOT_EXIST; > se_cmd->se_tfo->queue_tm_rsp(se_cmd); > > + transport_lun_remove_cmd(se_cmd); > transport_cmd_check_stop_to_fabric(se_cmd); > } > > @@ -1885,6 +1904,7 @@ void transport_generic_request_failure(struct se_cmd *cmd, > goto queue_full; > > check_stop: > + transport_lun_remove_cmd(cmd); > transport_cmd_check_stop_to_fabric(cmd); > return; > > @@ -2182,6 +2202,7 @@ static void transport_complete_qf(struct se_cmd *cmd) > transport_handle_queue_full(cmd, cmd->se_dev, ret, false); > return; > } > + transport_lun_remove_cmd(cmd); > transport_cmd_check_stop_to_fabric(cmd); > } > > @@ -2276,6 +2297,7 @@ static void target_complete_ok_work(struct work_struct *work) > if (ret) > goto queue_full; > > + transport_lun_remove_cmd(cmd); > transport_cmd_check_stop_to_fabric(cmd); > return; > } > @@ -2301,6 +2323,7 @@ static void target_complete_ok_work(struct work_struct *work) > if (ret) > goto queue_full; > > + transport_lun_remove_cmd(cmd); > transport_cmd_check_stop_to_fabric(cmd); > return; > } > @@ -2336,6 +2359,7 @@ static void target_complete_ok_work(struct work_struct *work) > if (ret) > goto queue_full; > > + transport_lun_remove_cmd(cmd); > transport_cmd_check_stop_to_fabric(cmd); > return; > } > @@ -2371,6 +2395,7 @@ static void target_complete_ok_work(struct work_struct *work) > break; > } > > + transport_lun_remove_cmd(cmd); > transport_cmd_check_stop_to_fabric(cmd); > return; > > @@ -2697,6 +2722,9 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks) > */ > if (cmd->state_active) > target_remove_from_state_list(cmd); > + > + if (cmd->se_lun) > + transport_lun_remove_cmd(cmd); > } > if (aborted) > cmd->free_compl = &compl; > @@ -2768,9 +2796,6 @@ static void target_release_cmd_kref(struct kref *kref) > struct completion *abrt_compl = se_cmd->abrt_compl; > unsigned long flags; > > - if (se_cmd->lun_ref_active) > - percpu_ref_put(&se_cmd->se_lun->lun_ref); > - > if (se_sess) { > spin_lock_irqsave(&se_sess->sess_cmd_lock, flags); > list_del_init(&se_cmd->se_cmd_list); > -- > 2.21.1 (Apple Git-122.3) > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On Fri, Feb 28, 2020 at 04:17:56PM -0500, Pavel Zakharov wrote: > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux-hwe/+bug/1862682 > > scsi: Revert "target/core: Inline transport_lun_remove_cmd()" > > Commit 83f85b8ec305 postponed the percpu_ref_put(&se_cmd->se_lun->lun_ref) > call from command completion to the time when the final command reference > is dropped. That approach is not compatible with the iSCSI target driver > because the iSCSI target driver keeps the command with the highest stat_sn > after it has completed until the next command is received (see also > iscsit_ack_from_expstatsn()). Fix this regression by reverting commit > 83f85b8ec305. > > Fixes: 83f85b8ec305 ("scsi: target/core: Inline transport_lun_remove_cmd()") > Cc: Pavel Zakharov <pavel.zakharov@delphix.com> > Cc: Mike Christie <mchristi@redhat.com> > Cc: <stable@vger.kernel.org> > Link: https://lore.kernel.org/r/20200210051202.12934-1-bvanassche@acm.org > Reported-by: Pavel Zakharov <pavel.zakharov@delphix.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> Acked-by: Seth Forshee <seth.forshee@canonical.com>
On 28.02.20 22:17, Pavel Zakharov wrote: > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux-hwe/+bug/1862682 > > scsi: Revert "target/core: Inline transport_lun_remove_cmd()" > > Commit 83f85b8ec305 postponed the percpu_ref_put(&se_cmd->se_lun->lun_ref) > call from command completion to the time when the final command reference > is dropped. That approach is not compatible with the iSCSI target driver > because the iSCSI target driver keeps the command with the highest stat_sn > after it has completed until the next command is received (see also > iscsit_ack_from_expstatsn()). Fix this regression by reverting commit > 83f85b8ec305. > > Fixes: 83f85b8ec305 ("scsi: target/core: Inline transport_lun_remove_cmd()") > Cc: Pavel Zakharov <pavel.zakharov@delphix.com> > Cc: Mike Christie <mchristi@redhat.com> > Cc: <stable@vger.kernel.org> > Link: https://lore.kernel.org/r/20200210051202.12934-1-bvanassche@acm.org > Reported-by: Pavel Zakharov <pavel.zakharov@delphix.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> > --- Since the HWE kernel is a backport from the Eoan kernel, this would affect anything v5.3 and later. I modified the bug report accordingly (when applying, this should go into Eoan). Not sure about Focal, it could already have landed while pulling in stable updates. -Stefan > drivers/target/target_core_transport.c | 31 +++++++++++++++++++++++--- > 1 file changed, 28 insertions(+), 3 deletions(-) >
On Thu, Mar 12, 2020 at 09:08:18AM +0100, Stefan Bader wrote: > On 28.02.20 22:17, Pavel Zakharov wrote: > > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux-hwe/+bug/1862682 > > > > scsi: Revert "target/core: Inline transport_lun_remove_cmd()" > > > > Commit 83f85b8ec305 postponed the percpu_ref_put(&se_cmd->se_lun->lun_ref) > > call from command completion to the time when the final command reference > > is dropped. That approach is not compatible with the iSCSI target driver > > because the iSCSI target driver keeps the command with the highest stat_sn > > after it has completed until the next command is received (see also > > iscsit_ack_from_expstatsn()). Fix this regression by reverting commit > > 83f85b8ec305. > > > > Fixes: 83f85b8ec305 ("scsi: target/core: Inline transport_lun_remove_cmd()") > > Cc: Pavel Zakharov <pavel.zakharov@delphix.com> > > Cc: Mike Christie <mchristi@redhat.com> > > Cc: <stable@vger.kernel.org> > > Link: https://lore.kernel.org/r/20200210051202.12934-1-bvanassche@acm.org > > Reported-by: Pavel Zakharov <pavel.zakharov@delphix.com> > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> > > --- > > Since the HWE kernel is a backport from the Eoan kernel, this would affect > anything v5.3 and later. I modified the bug report accordingly (when applying, > this should go into Eoan). Not sure about Focal, it could already have landed > while pulling in stable updates. I confirm this one is already applied to Focal. Thanks, -Andrea
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index eda8b4736c15..d542e26ca56a 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -666,6 +666,11 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd) target_remove_from_state_list(cmd); + /* + * Clear struct se_cmd->se_lun before the handoff to FE. + */ + cmd->se_lun = NULL; + spin_lock_irqsave(&cmd->t_state_lock, flags); /* * Determine if frontend context caller is requesting the stopping of @@ -693,6 +698,17 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd) return cmd->se_tfo->check_stop_free(cmd); } +static void transport_lun_remove_cmd(struct se_cmd *cmd) +{ + struct se_lun *lun = cmd->se_lun; + + if (!lun) + return; + + if (cmpxchg(&cmd->lun_ref_active, true, false)) + percpu_ref_put(&lun->lun_ref); +} + static void target_complete_failure_work(struct work_struct *work) { struct se_cmd *cmd = container_of(work, struct se_cmd, work); @@ -783,6 +799,8 @@ static void target_handle_abort(struct se_cmd *cmd) WARN_ON_ONCE(kref_read(&cmd->cmd_kref) == 0); + transport_lun_remove_cmd(cmd); + transport_cmd_check_stop_to_fabric(cmd); } @@ -1695,6 +1713,7 @@ static void target_complete_tmr_failure(struct work_struct *work) se_cmd->se_tmr_req->response = TMR_LUN_DOES_NOT_EXIST; se_cmd->se_tfo->queue_tm_rsp(se_cmd); + transport_lun_remove_cmd(se_cmd); transport_cmd_check_stop_to_fabric(se_cmd); } @@ -1885,6 +1904,7 @@ void transport_generic_request_failure(struct se_cmd *cmd, goto queue_full; check_stop: + transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); return; @@ -2182,6 +2202,7 @@ static void transport_complete_qf(struct se_cmd *cmd) transport_handle_queue_full(cmd, cmd->se_dev, ret, false); return; } + transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); } @@ -2276,6 +2297,7 @@ static void target_complete_ok_work(struct work_struct *work) if (ret) goto queue_full; + transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); return; } @@ -2301,6 +2323,7 @@ static void target_complete_ok_work(struct work_struct *work) if (ret) goto queue_full; + transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); return; } @@ -2336,6 +2359,7 @@ static void target_complete_ok_work(struct work_struct *work) if (ret) goto queue_full; + transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); return; } @@ -2371,6 +2395,7 @@ static void target_complete_ok_work(struct work_struct *work) break; } + transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); return; @@ -2697,6 +2722,9 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks) */ if (cmd->state_active) target_remove_from_state_list(cmd); + + if (cmd->se_lun) + transport_lun_remove_cmd(cmd); } if (aborted) cmd->free_compl = &compl; @@ -2768,9 +2796,6 @@ static void target_release_cmd_kref(struct kref *kref) struct completion *abrt_compl = se_cmd->abrt_compl; unsigned long flags; - if (se_cmd->lun_ref_active) - percpu_ref_put(&se_cmd->se_lun->lun_ref); - if (se_sess) { spin_lock_irqsave(&se_sess->sess_cmd_lock, flags); list_del_init(&se_cmd->se_cmd_list);