| Message ID | 20200717161212.10731-2-bstroesser@ts.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: target: tcmu: Add TMR notification for tcmu | expand |
On 7/17/20 11:12 AM, Bodo Stroesser wrote: > This patch modifies core_tmr_abort_task() to use same looping > and locking scheme as core_tmr_drain_state_list() does. > > This frees the state_list element in se_cmd for later use > by tmr notification handling. > > Note: __target_check_io_state() now is called with param 0 > instead of dev->dev_attrib.emulate_tas, because tas is not > relevant since we always get ABRT on same session like the > aborted command. > > Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com> > --- > drivers/target/target_core_tmr.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c > index 89c84d472cd7..b65d7a0a5df1 100644 > --- a/drivers/target/target_core_tmr.c > +++ b/drivers/target/target_core_tmr.c > @@ -116,14 +116,15 @@ void core_tmr_abort_task( > struct se_tmr_req *tmr, > struct se_session *se_sess) > { > - struct se_cmd *se_cmd; > + struct se_cmd *se_cmd, *next; > unsigned long flags; > + bool rc; > u64 ref_tag; > > - spin_lock_irqsave(&se_sess->sess_cmd_lock, flags); > - list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) { > + spin_lock_irqsave(&dev->execute_task_lock, flags); > + list_for_each_entry_safe(se_cmd, next, &dev->state_list, state_list) { > > - if (dev != se_cmd->se_dev) > + if (se_sess != se_cmd->se_sess) > continue; > > /* skip task management functions, including tmr->task_cmd */ > @@ -137,11 +138,16 @@ void core_tmr_abort_task( > printk("ABORT_TASK: Found referenced %s task_tag: %llu\n", > se_cmd->se_tfo->fabric_name, ref_tag); > > - if (!__target_check_io_state(se_cmd, se_sess, > - dev->dev_attrib.emulate_tas)) > + spin_lock_irqsave(&se_sess->sess_cmd_lock, flags); I don't think you can use flags twice. This call seems to be overwriting the outer spin lock caller's value, and I'm seeing a lot of warnings. > + rc = __target_check_io_state(se_cmd, se_sess, 0); > + spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags); Use spin_lock/spin_unlock since we know above we have disabled irqs right above. Or make the drain users work like above. Just make it consistent so future devs don't have to wonder why we use different calls. > + if (!rc) > continue; > > - spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);> + list_del_init(&se_cmd->state_list); > + se_cmd->state_active = false; > + > + spin_unlock_irqrestore(&dev->execute_task_lock, flags); > > /* > * Ensure that this ABORT request is visible to the LU RESET > @@ -159,7 +165,7 @@ void core_tmr_abort_task( > atomic_long_inc(&dev->aborts_complete); > return; > } > - spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags); > + spin_unlock_irqrestore(&dev->execute_task_lock, flags); > > printk("ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST for ref_tag: %lld\n", > tmr->ref_task_tag); >
On 2020-07-26 02:03, Mike Christie wrote: > On 7/17/20 11:12 AM, Bodo Stroesser wrote: >> This patch modifies core_tmr_abort_task() to use same looping >> and locking scheme as core_tmr_drain_state_list() does. >> >> This frees the state_list element in se_cmd for later use >> by tmr notification handling. >> >> Note: __target_check_io_state() now is called with param 0 >> instead of dev->dev_attrib.emulate_tas, because tas is not >> relevant since we always get ABRT on same session like the >> aborted command. >> >> Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com> >> --- >> drivers/target/target_core_tmr.c | 22 ++++++++++++++-------- >> 1 file changed, 14 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c >> index 89c84d472cd7..b65d7a0a5df1 100644 >> --- a/drivers/target/target_core_tmr.c >> +++ b/drivers/target/target_core_tmr.c >> @@ -116,14 +116,15 @@ void core_tmr_abort_task( >> struct se_tmr_req *tmr, >> struct se_session *se_sess) >> { >> - struct se_cmd *se_cmd; >> + struct se_cmd *se_cmd, *next; >> unsigned long flags; >> + bool rc; >> u64 ref_tag; >> >> - spin_lock_irqsave(&se_sess->sess_cmd_lock, flags); >> - list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) { >> + spin_lock_irqsave(&dev->execute_task_lock, flags); >> + list_for_each_entry_safe(se_cmd, next, &dev->state_list, state_list) { >> >> - if (dev != se_cmd->se_dev) >> + if (se_sess != se_cmd->se_sess) >> continue; >> >> /* skip task management functions, including tmr->task_cmd */ >> @@ -137,11 +138,16 @@ void core_tmr_abort_task( >> printk("ABORT_TASK: Found referenced %s task_tag: %llu\n", >> se_cmd->se_tfo->fabric_name, ref_tag); >> >> - if (!__target_check_io_state(se_cmd, se_sess, >> - dev->dev_attrib.emulate_tas)) >> + spin_lock_irqsave(&se_sess->sess_cmd_lock, flags); > > I don't think you can use flags twice. This call seems to be overwriting the outer spin lock caller's value, and I'm seeing a lot of warnings. Oops. Thank you! Bad cut and paste. I wanted to make core_tmr_abort_task similar to core_tmr_drain_state_list. But then I moved the calls for se_sess->sess_cmd_lock inside core_tmr_abort_task instead of copying the ones from core_tmr_drain_state_list. I'm wondering why I don't see warnings when waiting with irq blocked ... I'll resend without _irqsave / _irqrestore. > > >> + rc = __target_check_io_state(se_cmd, se_sess, 0); >> + spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags); > > > Use spin_lock/spin_unlock since we know above we have disabled irqs right above. Or make the drain users work like above. > > Just make it consistent so future devs don't have to wonder why we use different calls. > >> + if (!rc) >> continue; >> >> - spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);> + list_del_init(&se_cmd->state_list); >> + se_cmd->state_active = false; >> + >> + spin_unlock_irqrestore(&dev->execute_task_lock, flags); >> >> /* >> * Ensure that this ABORT request is visible to the LU RESET >> @@ -159,7 +165,7 @@ void core_tmr_abort_task( >> atomic_long_inc(&dev->aborts_complete); >> return; >> } >> - spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags); >> + spin_unlock_irqrestore(&dev->execute_task_lock, flags); >> >> printk("ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST for ref_tag: %lld\n", >> tmr->ref_task_tag); >> >
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c index 89c84d472cd7..b65d7a0a5df1 100644 --- a/drivers/target/target_core_tmr.c +++ b/drivers/target/target_core_tmr.c @@ -116,14 +116,15 @@ void core_tmr_abort_task( struct se_tmr_req *tmr, struct se_session *se_sess) { - struct se_cmd *se_cmd; + struct se_cmd *se_cmd, *next; unsigned long flags; + bool rc; u64 ref_tag; - spin_lock_irqsave(&se_sess->sess_cmd_lock, flags); - list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) { + spin_lock_irqsave(&dev->execute_task_lock, flags); + list_for_each_entry_safe(se_cmd, next, &dev->state_list, state_list) { - if (dev != se_cmd->se_dev) + if (se_sess != se_cmd->se_sess) continue; /* skip task management functions, including tmr->task_cmd */ @@ -137,11 +138,16 @@ void core_tmr_abort_task( printk("ABORT_TASK: Found referenced %s task_tag: %llu\n", se_cmd->se_tfo->fabric_name, ref_tag); - if (!__target_check_io_state(se_cmd, se_sess, - dev->dev_attrib.emulate_tas)) + spin_lock_irqsave(&se_sess->sess_cmd_lock, flags); + rc = __target_check_io_state(se_cmd, se_sess, 0); + spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags); + if (!rc) continue; - spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags); + list_del_init(&se_cmd->state_list); + se_cmd->state_active = false; + + spin_unlock_irqrestore(&dev->execute_task_lock, flags); /* * Ensure that this ABORT request is visible to the LU RESET @@ -159,7 +165,7 @@ void core_tmr_abort_task( atomic_long_inc(&dev->aborts_complete); return; } - spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags); + spin_unlock_irqrestore(&dev->execute_task_lock, flags); printk("ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST for ref_tag: %lld\n", tmr->ref_task_tag);
This patch modifies core_tmr_abort_task() to use same looping and locking scheme as core_tmr_drain_state_list() does. This frees the state_list element in se_cmd for later use by tmr notification handling. Note: __target_check_io_state() now is called with param 0 instead of dev->dev_attrib.emulate_tas, because tas is not relevant since we always get ABRT on same session like the aborted command. Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com> --- drivers/target/target_core_tmr.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)