On Sat, 2011-10-08 at 20:37 -0400, Christoph Hellwig wrote:
> On Sat, Oct 08, 2011 at 02:11:48PM -0700, Nicholas A. Bellinger wrote:
> > The logic here is wrong.. The check for &cmd->transport_dev_active is
> > inverted, and both blocks should be able to be called during normal
> > transport_put_cmd() execution..
> >
> > Here's what i'm thinking instead for this path (in full context) in
> > transport_put_cmd() to following original logic:
> >
> > static bool transport_put_cmd(struct se_cmd *cmd)
> > {
> > unsigned long flags;
> > int free_tasks = 0;
> >
> > spin_lock_irqsave(&cmd->t_state_lock, flags);
> > if (atomic_read(&cmd->t_fe_count)) {
> > if (!atomic_dec_and_test(&cmd->t_fe_count))
> > goto out_busy;
> > }
> >
> > if (atomic_read(&cmd->t_se_count)) {
> > if (!atomic_dec_and_test(&cmd->t_se_count))
> > goto out_busy;
> > }
>
> In general I think doing the atomic_read followed by the atomic_dec
> and test here is wrong, as it doesn't actually give you atomic
> behaviour. Now when I looked weeks ago we generally incremented these
> with the lock so the code was kindof fine, but it's still very bad
> style. In fact I have a half finished patch to make these and a few
> others non-atomic and always rely on t_state_lock instead.
>
Agreed. This is also some left-over logic from back when we actually
played with this values directly.. In modern code we never directly
decrement t_fe_count outside of this path, so the extra atomic_read()
should be safe to drop.
For t_se_count, there is one case in the task timeout handling code
where this can still be decremented outside of this path. However, I'm
in the process of removing all of the legacy task timeout and
transport_processing_shutdown() code for v3.2, so once this is removed
the extra atomic_read() around t_se_count can be dropped as well.
--nab
--
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
