On Tue, Oct 13, 2015 at 06:00:27AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 12, 2015 at 10:10:20AM -0700, Bart Van Assche wrote:
> > This patch fixes the following kernel warning because it avoids that
> > IRQs are disabled while ft_release_cmd() is invoked (fc_seq_set_resp()
> > invokes spin_unlock_bh()):
>
> This looks fine to me. It also removes the only user of
> kref_put_spinlock_irqsave so you should be able to remove it as well.
>
> Joern, I think the use of kref_get_unless_zero in Barts patch sould deal
> with the issue that you added kref_put_spinlock_irqsave for just fine,
> but can you review it as well?
kref_get_unless_zero() and kref_put_spinlock_irqsave() are pretty much
equivalent. Either you check when taking the refcount, with lock held,
or you check when releasing the refcount, taking the lock in cases of
doubt. And I think kref_get_unless_zero() should be slightly faster.
ACK on the general principle and on the patch below.
> > +++ b/drivers/target/target_core_tmr.c
> > @@ -130,6 +130,9 @@ void core_tmr_abort_task(
> > if (tmr->ref_task_tag != ref_tag)
> > continue;
> >
> > + if (!kref_get_unless_zero(&se_cmd->cmd_kref))
> > + continue;
> > +
> > printk("ABORT_TASK: Found referenced %s task_tag: %llu\n",
> > se_cmd->se_tfo->get_fabric_name(), ref_tag);
> >
> > @@ -139,13 +142,15 @@ void core_tmr_abort_task(
> > " skipping\n", ref_tag);
> > spin_unlock(&se_cmd->t_state_lock);
> > spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > +
> > + target_put_sess_cmd(se_cmd);
> > +
> > goto out;
> > }
> > se_cmd->transport_state |= CMD_T_ABORTED;
> > spin_unlock(&se_cmd->t_state_lock);
> >
> > list_del_init(&se_cmd->se_cmd_list);
> > - kref_get(&se_cmd->cmd_kref);
> > spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> >
> > cancel_work_sync(&se_cmd->work);
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index eae4924..7f4d3ac 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -2507,23 +2507,24 @@ out:
> > EXPORT_SYMBOL(target_get_sess_cmd);
> >
> > static void target_release_cmd_kref(struct kref *kref)
> > - __releases(&se_cmd->se_sess->sess_cmd_lock)
> > {
> > struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
> > struct se_session *se_sess = se_cmd->se_sess;
> > + unsigned long flags;
> >
> > + spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> > if (list_empty(&se_cmd->se_cmd_list)) {
> > - spin_unlock(&se_sess->sess_cmd_lock);
> > + spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > se_cmd->se_tfo->release_cmd(se_cmd);
> > return;
> > }
> > if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
> > - spin_unlock(&se_sess->sess_cmd_lock);
> > + spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > complete(&se_cmd->cmd_wait_comp);
> > return;
> > }
> > list_del(&se_cmd->se_cmd_list);
> > - spin_unlock(&se_sess->sess_cmd_lock);
> > + spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> >
> > se_cmd->se_tfo->release_cmd(se_cmd);
> > }
> > @@ -2539,8 +2540,7 @@ int target_put_sess_cmd(struct se_cmd *se_cmd)
> > se_cmd->se_tfo->release_cmd(se_cmd);
> > return 1;
> > }
> > - return kref_put_spinlock_irqsave(&se_cmd->cmd_kref, target_release_cmd_kref,
> > - &se_sess->sess_cmd_lock);
> > + return kref_put(&se_cmd->cmd_kref, target_release_cmd_kref);
> > }
> > EXPORT_SYMBOL(target_put_sess_cmd);
> >
> > --
> > 2.1.4
> >
> > --
> > 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
> ---end quoted text---
Jörn
--
There's nothing better for promoting creativity in a medium than
making an audience feel "Hmm I could do better than that!"
-- Douglas Adams in a slashdot interview
--
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
