Re: [PATCH] scsi: target: put lun_ref at end of tmr processing

[Bodo Stroesser <bstroesser@ts.fujitsu.com>]

On 05/20/20 17:50, Pavel Zakharov wrote:
> Hopefully I didn’t mess-up the recipients/Subject list as I just subscribed to target-devel and so following-up to an email I found in the archive.
> 
> Bodo, I happened to have run into the same issue and was just about to submit a similar patch when I randomly stumbled on yours.
> I think we can improve on that patch by moving the call to transport_lun_remove_cmd() into transport_cmd_check_stop_to_fabric().

Yeah, I also thought about doing this. But then decided to better
add the one lost line for fixing only.

> 
> Here’s the patch I was about to propose. Let me know what you think.
> Note that we could also do this simplification as a separate patch later if you think it’s better.

I think it is a good idea to do this simplification in a separate patch.


> 
> ---
> When ABORT and LUN RESET were made synchronous, a call to
> transport_lun_remove_cmd() was acidentally removed. This caused a lun_ref
> to be leaked every time an ABORT or LUN RESET was processed in
> target_tmr_work().
> 
> Given that transport_lun_remove_cmd() should always be called before
> transport_cmd_check_stop_to_fabric(), move that call into this function.
> This ensures that the lun_ref is released before setting .se_lun to NULL.
> 
> Signed-off-by: Pavel Zakharov <pavel.zakharov@delphix.com>
> ---
>   drivers/target/target_core_transport.c | 36 ++++++++++++--------------
>   1 file changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 594b724bbf79..ec578822709a 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -653,6 +653,17 @@ static void target_remove_from_state_list(struct se_cmd *cmd)
>   	spin_unlock_irqrestore(&dev->execute_task_lock, flags);
>   }
> 
> +static void transport_lun_remove_cmd(struct se_cmd *cmd)
> +{
> +	struct se_lun *lun = cmd->se_lun;
> +
> +	if (!lun)
> +		return;
> +
> +	if (cmpxchg(&cmd->lun_ref_active, true, false))
> +		percpu_ref_put(&lun->lun_ref);
> +}
> +
>   /*
>    * This function is called by the target core after the target core has
>    * finished processing a SCSI command or SCSI TMF. Both the regular command
> @@ -664,6 +675,11 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd)
>   {
>   	unsigned long flags;
> 
> +	/*
> +	 * Remove cmd ref from lun if needed before clearing the se_lun.
> +	 */
> +	transport_lun_remove_cmd(cmd);
> +
>   	target_remove_from_state_list(cmd);
> 
>   	/*
> @@ -698,17 +714,6 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd)
>   	return cmd->se_tfo->check_stop_free(cmd);
>   }
> 
> -static void transport_lun_remove_cmd(struct se_cmd *cmd)
> -{
> -	struct se_lun *lun = cmd->se_lun;
> -
> -	if (!lun)
> -		return;
> -
> -	if (cmpxchg(&cmd->lun_ref_active, true, false))
> -		percpu_ref_put(&lun->lun_ref);
> -}
> -
>   static void target_complete_failure_work(struct work_struct *work)
>   {
>   	struct se_cmd *cmd = container_of(work, struct se_cmd, work);
> @@ -799,8 +804,6 @@ static void target_handle_abort(struct se_cmd *cmd)
> 
>   	WARN_ON_ONCE(kref_read(&cmd->cmd_kref) == 0);
> 
> -	transport_lun_remove_cmd(cmd);
> -
>   	transport_cmd_check_stop_to_fabric(cmd);
>   }
> 
> @@ -1726,7 +1729,6 @@ static void target_complete_tmr_failure(struct work_struct *work)
>   	se_cmd->se_tmr_req->response = TMR_LUN_DOES_NOT_EXIST;
>   	se_cmd->se_tfo->queue_tm_rsp(se_cmd);
> 
> -	transport_lun_remove_cmd(se_cmd);
>   	transport_cmd_check_stop_to_fabric(se_cmd);
>   }
> 
> @@ -1918,7 +1920,6 @@ void transport_generic_request_failure(struct se_cmd *cmd,
>   		goto queue_full;
> 
>   check_stop:
> -	transport_lun_remove_cmd(cmd);
>   	transport_cmd_check_stop_to_fabric(cmd);
>   	return;
> 
> @@ -2216,7 +2217,6 @@ static void transport_complete_qf(struct se_cmd *cmd)
>   		transport_handle_queue_full(cmd, cmd->se_dev, ret, false);
>   		return;
>   	}
> -	transport_lun_remove_cmd(cmd);
>   	transport_cmd_check_stop_to_fabric(cmd);
>   }
> 
> @@ -2311,7 +2311,6 @@ static void target_complete_ok_work(struct work_struct *work)
>   		if (ret)
>   			goto queue_full;
> 
> -		transport_lun_remove_cmd(cmd);
>   		transport_cmd_check_stop_to_fabric(cmd);
>   		return;
>   	}
> @@ -2337,7 +2336,6 @@ static void target_complete_ok_work(struct work_struct *work)
>   			if (ret)
>   				goto queue_full;
> 
> -			transport_lun_remove_cmd(cmd);
>   			transport_cmd_check_stop_to_fabric(cmd);
>   			return;
>   		}
> @@ -2373,7 +2371,6 @@ static void target_complete_ok_work(struct work_struct *work)
>   			if (ret)
>   				goto queue_full;
> 
> -			transport_lun_remove_cmd(cmd);
>   			transport_cmd_check_stop_to_fabric(cmd);
>   			return;
>   		}
> @@ -2409,7 +2406,6 @@ static void target_complete_ok_work(struct work_struct *work)
>   		break;
>   	}
> 
> -	transport_lun_remove_cmd(cmd);
>   	transport_cmd_check_stop_to_fabric(cmd);
>   	return;
> 
> --
> 2.21.1 (Apple Git-122.3)
>