ACK/Cmnt: [PATCH 1/1] [bionic hwe kernel] scsi: Revert "target/core: Inline transport_lun_remove_cmd()"

Andrea Righi andrea.righi at canonical.com
Fri Mar 6 17:20:51 UTC 2020


On Fri, Feb 28, 2020 at 04:17:56PM -0500, Pavel Zakharov wrote:
> BugLink: https://bugs.launchpad.net/ubuntu/+source/linux-hwe/+bug/1862682
> 
> scsi: Revert "target/core: Inline transport_lun_remove_cmd()"
> 
> Commit 83f85b8ec305 postponed the percpu_ref_put(&se_cmd->se_lun->lun_ref)
> call from command completion to the time when the final command reference
> is dropped. That approach is not compatible with the iSCSI target driver
> because the iSCSI target driver keeps the command with the highest stat_sn
> after it has completed until the next command is received (see also
> iscsit_ack_from_expstatsn()). Fix this regression by reverting commit
> 83f85b8ec305.

It looks like this patch landed upstream. I think we can add:

(cherry picked from commit c14335ebb92a98646ddbf447e6cacc66de5269ad)

Apart than that looks good to me, so:

Acked-by: Andrea Righi <andrea.righi at canonical.com>

> 
> Fixes: 83f85b8ec305 ("scsi: target/core: Inline transport_lun_remove_cmd()")
> Cc: Pavel Zakharov <pavel.zakharov at delphix.com>
> Cc: Mike Christie <mchristi at redhat.com>
> Cc: <stable at vger.kernel.org>
> Link: https://lore.kernel.org/r/20200210051202.12934-1-bvanassche@acm.org
> Reported-by: Pavel Zakharov <pavel.zakharov at delphix.com>
> Signed-off-by: Bart Van Assche <bvanassche at acm.org>
> Signed-off-by: Martin K. Petersen <martin.petersen at oracle.com>
> ---
>  drivers/target/target_core_transport.c | 31 +++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index eda8b4736c15..d542e26ca56a 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -666,6 +666,11 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd)
>  
>  	target_remove_from_state_list(cmd);
>  
> +	/*
> +	 * Clear struct se_cmd->se_lun before the handoff to FE.
> +	 */
> +	cmd->se_lun = NULL;
> +
>  	spin_lock_irqsave(&cmd->t_state_lock, flags);
>  	/*
>  	 * Determine if frontend context caller is requesting the stopping of
> @@ -693,6 +698,17 @@ 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);
> @@ -783,6 +799,8 @@ 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);
>  }
>  
> @@ -1695,6 +1713,7 @@ 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);
>  }
>  
> @@ -1885,6 +1904,7 @@ 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;
>  
> @@ -2182,6 +2202,7 @@ 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);
>  }
>  
> @@ -2276,6 +2297,7 @@ 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;
>  	}
> @@ -2301,6 +2323,7 @@ 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;
>  		}
> @@ -2336,6 +2359,7 @@ 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;
>  		}
> @@ -2371,6 +2395,7 @@ static void target_complete_ok_work(struct work_struct *work)
>  		break;
>  	}
>  
> +	transport_lun_remove_cmd(cmd);
>  	transport_cmd_check_stop_to_fabric(cmd);
>  	return;
>  
> @@ -2697,6 +2722,9 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
>  		 */
>  		if (cmd->state_active)
>  			target_remove_from_state_list(cmd);
> +
> +		if (cmd->se_lun)
> +			transport_lun_remove_cmd(cmd);
>  	}
>  	if (aborted)
>  		cmd->free_compl = &compl;
> @@ -2768,9 +2796,6 @@ static void target_release_cmd_kref(struct kref *kref)
>  	struct completion *abrt_compl = se_cmd->abrt_compl;
>  	unsigned long flags;
>  
> -	if (se_cmd->lun_ref_active)
> -		percpu_ref_put(&se_cmd->se_lun->lun_ref);
> -
>  	if (se_sess) {
>  		spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
>  		list_del_init(&se_cmd->se_cmd_list);
> -- 
> 2.21.1 (Apple Git-122.3)
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team



More information about the kernel-team mailing list