Re: [PATCH] scsi: ibmvscsi: protect abort handler from done-scmd in flight

[Paolo Bonzini <pbonzini@redhat.com>]

Il 05/06/2014 08:16, Liu Ping Fan ha scritto:
> Take the following scene in guest:
> seqA: scsi_done() -> gapX (before taking REQ_ATOM_COMPLETE)
> seqB: scmd_eh_abort_handler()-> ...-> ibmvscsi_eh_abort_handler()->
>       ...->scsi_put_command(scmd)
>
> If seqA is scheduled at gapX, and seqB reclaims scmd. Then when seqA
> comes back, it tries to access the scmd when is turned back to mempool.
>
> This patch fixes the race by ensuring when ibmvscsi_eh_abort_handler()
> returns, no scsi_done is in flight
>
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
> When trying to figure the scsi_cmnd in flight issue, I learned from Paolo (thanks).
> He showed me the way how virtscsi resolves the race between abort-handler
> and scsi_done in flight. And I think that this method is also needed by ibmvscsi.
> ---
>  drivers/scsi/ibmvscsi/ibmvscsi.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index fa76440..325cef6 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -1828,16 +1828,19 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
>
>  	if ((crq->status != VIOSRP_OK && crq->status != VIOSRP_OK2) && evt_struct->cmnd)
>  		evt_struct->cmnd->result = DID_ERROR << 16;
> -	if (evt_struct->done)
> -		evt_struct->done(evt_struct);
> -	else
> -		dev_err(hostdata->dev, "returned done() is NULL; not running it!\n");
>
>  	/*
>  	 * Lock the host_lock before messing with these structures, since we
>  	 * are running in a task context
> +	 * Also, this lock helps ibmvscsi_eh_abort_handler() to shield the
> +	 * scsi_done() in flight.
>  	 */
>  	spin_lock_irqsave(evt_struct->hostdata->host->host_lock, flags);
> +	if (evt_struct->done)
> +		evt_struct->done(evt_struct);
> +	else
> +		dev_err(hostdata->dev, "returned done() is NULL; not running it!\n");
> +
>  	list_del(&evt_struct->list);
>  	free_event_struct(&evt_struct->hostdata->pool, evt_struct);
>  	spin_unlock_irqrestore(evt_struct->hostdata->host->host_lock, flags);
>

I think this is not necessary because ibmvscsi places TMFs and commands 
in the same queue; virtio-scsi instead uses two different queues.

So ibmvscsi_handle_crq processes all completed requests, which naturally 
serializes the processing of the TMF and the command.

Paolo