On Tue, 2011-09-13 at 23:08 +0200, Christoph Hellwig wrote:
> Inline two simple functions only used by it, and replace a goto
> with a simple if else construct.
>
> Note that the code moved from transport_dec_and_check seems fairly
> buggy - the atomic_read check on a variable where we'd do an
> atomic_dec_and_test looks racy if we'll ever get someone increment
> it without the lock held around them (which it looks like we do),
> and not decrementing the second counter if the first one doesn't
> hit zero also at least needs an explanation.
>
Hey Christoph,
Sorry for the long delay here. I've finally been reviewing this code.
Comments are below..
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
>
> Index: lio-core/drivers/target/target_core_transport.c
> ===================================================================
> --- lio-core.orig/drivers/target/target_core_transport.c 2011-09-13 14:54:21.892088669 -0400
> +++ lio-core/drivers/target/target_core_transport.c 2011-09-13 14:54:25.125089859 -0400
> @@ -3754,36 +3754,6 @@ static inline void transport_free_pages(
> cmd->t_bidi_data_nents = 0;
> }
>
> -static inline void transport_release_tasks(struct se_cmd *cmd)
> -{
> - transport_free_dev_tasks(cmd);
> -}
> -
> -static inline int transport_dec_and_check(struct se_cmd *cmd)
> -{
> - unsigned long flags;
> -
> - spin_lock_irqsave(&cmd->t_state_lock, flags);
> - if (atomic_read(&cmd->t_fe_count)) {
> - if (!atomic_dec_and_test(&cmd->t_fe_count)) {
> - spin_unlock_irqrestore(&cmd->t_state_lock,
> - flags);
> - return 1;
> - }
> - }
> -
> - if (atomic_read(&cmd->t_se_count)) {
> - if (!atomic_dec_and_test(&cmd->t_se_count)) {
> - spin_unlock_irqrestore(&cmd->t_state_lock,
> - flags);
> - return 1;
> - }
> - }
> - spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> -
> - return 0;
> -}
> -
> /**
> * transport_put_cmd - release a reference to a command
> * @cmd: command to release
> @@ -3794,25 +3764,33 @@ static bool transport_put_cmd(struct se_
> {
> unsigned long flags;
>
> - if (transport_dec_and_check(cmd))
> - return false;
> -
> spin_lock_irqsave(&cmd->t_state_lock, flags);
> - if (!atomic_read(&cmd->transport_dev_active)) {
> - spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> - goto free_pages;
> + if (atomic_read(&cmd->t_fe_count)) {
> + if (!atomic_dec_and_test(&cmd->t_fe_count))
> + goto out_busy;
> }
> - atomic_set(&cmd->transport_dev_active, 0);
> - transport_all_task_dev_remove_state(cmd);
> - spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>
> - transport_release_tasks(cmd);
> - return true;
> + if (atomic_read(&cmd->t_se_count)) {
> + if (!atomic_dec_and_test(&cmd->t_se_count))
> + goto out_busy;
> + }
> +
> + if (atomic_read(&cmd->transport_dev_active)) {
> + spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> + transport_free_pages(cmd);
> + transport_release_cmd(cmd);
> + } else {
> + atomic_set(&cmd->transport_dev_active, 0);
> + transport_all_task_dev_remove_state(cmd);
> + spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> +
> + transport_free_dev_tasks(cmd);
> + }
>
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;
}
if (atomic_read(&cmd->transport_dev_active)) {
atomic_set(&cmd->transport_dev_active, 0);
transport_all_task_dev_remove_state(cmd);
free_tasks = 1;
}
spin_unlock_irqrestore(&cmd->t_state_lock, flags);
if (free_tasks != 0)
transport_free_dev_tasks(cmd);
transport_free_pages(cmd);
transport_release_cmd(cmd);
return true;
out_busy:
spin_unlock_irqrestore(&cmd->t_state_lock, flags);
return false;
}
--
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
