| Message ID | 1449535747-2850-10-git-send-email-himanshu.madhani@qlogic.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 07, 2015 at 07:48:56PM -0500, Himanshu Madhani wrote: > From: Quinn Tran <quinn.tran@qlogic.com> > > change tcm_qla2xxx_check_stop_free to always return 1 > to prevent transport_cmd_finish_abort from accidently > taking extra kref_put. This looks a bit fishy. Even if this was the right behavior it's something all something all drivers returning the value of target_put_sess_cmd in their check_stop_free would need to do. Can you explain the issue you're trying to address in a little more detail? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/08/2015 01:48 AM, Himanshu Madhani wrote: > From: Quinn Tran <quinn.tran@qlogic.com> > > change tcm_qla2xxx_check_stop_free to always return 1 > to prevent transport_cmd_finish_abort from accidently > taking extra kref_put. > > Signed-off-by: Quinn Tran <quinn.tran@qlogic.com> > Signed-off-by: Himanshu Madhani <himanshu.madhani@qlogic.com> > --- > drivers/scsi/qla2xxx/tcm_qla2xxx.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c > index 74c6e9b..366142a 100644 > --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c > +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c > @@ -314,7 +314,8 @@ static int tcm_qla2xxx_check_stop_free(struct se_cmd *se_cmd) > cmd->cmd_flags |= BIT_14; > } > > - return target_put_sess_cmd(se_cmd); > + target_put_sess_cmd(se_cmd); > + return 1; > } > > /* tcm_qla2xxx_release_cmd - Callback from TCM Core to release underlying > Odd. I would have expected target_put_sess_cmd() to never fail. But if it does, doesn't it mean that the command is hosed, and we should have accessed it in the first place? Very suspicious. Cheers, Hannes
On 12/8/15, 10:56 PM, "target-devel-owner@vger.kernel.org on behalf of Hannes Reinecke" <target-devel-owner@vger.kernel.org on behalf of hare@suse.de> wrote: >On 12/08/2015 01:48 AM, Himanshu Madhani wrote: >> From: Quinn Tran <quinn.tran@qlogic.com> >> >> change tcm_qla2xxx_check_stop_free to always return 1 >> to prevent transport_cmd_finish_abort from accidently >> taking extra kref_put. >> >> Signed-off-by: Quinn Tran <quinn.tran@qlogic.com> >> Signed-off-by: Himanshu Madhani <himanshu.madhani@qlogic.com> >> --- >> drivers/scsi/qla2xxx/tcm_qla2xxx.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c >>b/drivers/scsi/qla2xxx/tcm_qla2xxx.c >> index 74c6e9b..366142a 100644 >> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c >> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c >> @@ -314,7 +314,8 @@ static int tcm_qla2xxx_check_stop_free(struct >>se_cmd *se_cmd) >> cmd->cmd_flags |= BIT_14; >> } >> >> - return target_put_sess_cmd(se_cmd); >> + target_put_sess_cmd(se_cmd); >> + return 1; >> } >> >> /* tcm_qla2xxx_release_cmd - Callback from TCM Core to release >>underlying >> >Odd. I would have expected target_put_sess_cmd() to never fail. QT> It does not failed under normal usage. >But if it does, doesn't it mean that the command is hosed, and we should >have accessed it in the first place? >Very suspicious. QT> here are the cases I see. Note: The patch series is in reference of pre-Bart¹s fix. - cmd_kref 2 -> 1, target_put_sess_cmd return true. This is Normal case. Backend finished with the command and send it to the Front end. - cmd_kref 1 -> 0, target_put_sess_cmd return true. Case 1: Early free by TCM/TMR thread. TMR thread call 2nd check_stop. core_tmr_drain_tmr_list->transport_cmd_finish_abort-> transport_cmd_check_stop_to_fabric - cmd_kref 0 -> -1, target_put_sess_cmd return false. QLA does not call ³check_stop² in either cases below. Case 1: QLA cause double free the command as the command is coming out of HW queue. In other word QLA access stale pointer during cleanup/free. Case 2: QLA driver beat TMR thread by a few nano second by freeing the command 1st(cmd_kref=0). TMR thread call the extra check_stop(cmd_kref = -1). transport_cmd_finish_abort do additional kref_put (cmd_kref = -2). void transport_cmd_finish_abort(struct se_cmd *cmd, int remove) { if (transport_cmd_check_stop_to_fabric(cmd)) return; if (remove) transport_put_cmd(cmd); } > >Cheers, > >Hannes >-- >Dr. Hannes Reinecke zSeries & Storage >hare@suse.de +49 911 74053 688 >SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg >GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) >-- >To unsubscribe from this list: send the line "unsubscribe target-devel" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c index 74c6e9b..366142a 100644 --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c @@ -314,7 +314,8 @@ static int tcm_qla2xxx_check_stop_free(struct se_cmd *se_cmd) cmd->cmd_flags |= BIT_14; } - return target_put_sess_cmd(se_cmd); + target_put_sess_cmd(se_cmd); + return 1; } /* tcm_qla2xxx_release_cmd - Callback from TCM Core to release underlying