On Fri, 2012-03-30 at 11:03 +0100, Stefan Hajnoczi wrote:
> For consistency, free the target se_cmd before putting the pages related
> to the command.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@xxxxxxxxxxxxxxxxxx>
> ---
> This function is invoked in vhost worker thread context to complete a request.
> I'm not sure whether it's safe to call transport_generic_free_cmd() from
> another kernel thread.
It's fine to call transport_generic_free_cmd() from another thread,
you'll just want to avoid calling it from interrupt context..
> I'm also not sure if we really need to set the
> wait_for_tasks argument here since the task really should be done by now.
> Basically this function is supposed to quiesce the task, put page references,
> and then free the struct.
>
> Thoughts?
>
The wait_for_tasks=1 usage here with transport_generic_free_cmd is not
necessary, as long as the caller can be sure the se_cmd is not in use..
Unfortuately the TFO->queue_data_in() and TFO->queue_rsp() response
callbacks are *not* those locations.. ;)
So the main question is how to handle the race between:
*) The queueing of tcm_vhost_cmd with vhost_scsi_complete_cmd() from
tcm_vhost_queue_data_in() and tcm_vhost_queue_status() callbacks to
invoke the running of vhost_scsi_complete_cmd_work() + release se_cmd
memory, and
*) the transport_cmd_check_stop_to_fabric() call made from
target_core_mod after TFO->queue_data_in() and TFO->queue_status()
complete to relinquish access to outgoing for the se_cmd.
Some fabrics like tcm_loop use the TFO->check_stop_free() to call
transport_generic_free_cmd() directly and release se_cmd memory. Other
fabrics like qla2xxx use an target_submit_cmd() w/ TARGET_SCF_ACK_KREF
and call target_put_sess_cmd() from within their TFO->check_stop_free()
and seperate HW descriptor acknowledgment path to determine which
process context actually releases the command.
I would recommend either following what qla2xxx does here and use
target_submit_cmd() w/ TARGET_SCF_ACK_KREF, and allow the last
target_put_sess_cmd() to happen from either TFO->check_stop_free() or
vhost_scsi_free_cmd().
I'm fine with applying this patch for now, but it will end up changing
again after the conversion to target_submit_cmd().
--nab
> drivers/target/tcm_vhost/tcm_vhost_scsi.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/target/tcm_vhost/tcm_vhost_scsi.c b/drivers/target/tcm_vhost/tcm_vhost_scsi.c
> index 93b3a82..f887ed5 100644
> --- a/drivers/target/tcm_vhost/tcm_vhost_scsi.c
> +++ b/drivers/target/tcm_vhost/tcm_vhost_scsi.c
> @@ -42,6 +42,7 @@ static void vhost_scsi_free_cmd(struct tcm_vhost_cmd *tv_cmd)
> struct se_cmd *se_cmd = &tv_cmd->tvc_se_cmd;
>
> /* TODO locking against target/backend threads? */
> + transport_generic_free_cmd(se_cmd, 1);
>
> if (tv_cmd->tvc_sgl_count) {
> u32 i;
> @@ -49,8 +50,6 @@ static void vhost_scsi_free_cmd(struct tcm_vhost_cmd *tv_cmd)
> put_page(sg_page(&tv_cmd->tvc_sgl[i]));
> }
>
> - /* TODO what does wait_for_tasks do? */
> - transport_generic_free_cmd(se_cmd, 1);
> kfree(tv_cmd);
> }
>
--
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
