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.
--
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
